[Group] Fix Group Write Attribute sending and receiving path (#12052)

* Fix Group Write Attribute sending and receiving * Apply PR comments
project-chip · Dec 6, 2023 · 3500836 · 3500836
1 parent 5a4a10b
commit 3500836
Show file tree

Hide file tree

Showing 7 changed files with 204 additions and 13 deletions.
diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
@@ -424,7 +424,7 @@ CHIP_ERROR InteractionModelEngine::OnMessageReceived(Messaging::ExchangeContext
     }
 
 exit:
-    if (status != Protocols::InteractionModel::Status::Success)
+    if (status != Protocols::InteractionModel::Status::Success && !apExchangeContext->IsGroupExchangeContext())
     {
         err = StatusResponse::SendStatusResponse(status, apExchangeContext, false /*aExpectResponse*/);
     }

diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp
@@ -60,7 +60,12 @@ CHIP_ERROR WriteHandler::OnWriteRequest(Messaging::ExchangeContext * apExchangeC
 
     err = ProcessWriteRequest(std::move(aPayload));
     SuccessOrExit(err);
-    err = SendWriteResponse();
+
+    // Do not send response on Group Write
+    if (!apExchangeContext->IsGroupExchangeContext())
+    {
+        err = SendWriteResponse();
+    }
 
 exit:
     Shutdown();
@@ -110,6 +115,8 @@ CHIP_ERROR WriteHandler::SendWriteResponse()
 CHIP_ERROR WriteHandler::ProcessAttributeDataIBs(TLV::TLVReader & aAttributeDataIBsReader)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
+    VerifyOrExit(mpExchangeCtx != nullptr, err = CHIP_ERROR_INTERNAL);
+
     while (CHIP_NO_ERROR == (err = aAttributeDataIBsReader.Next()))
     {
         chip::TLV::TLVReader dataReader;
@@ -131,9 +138,19 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataIBs(TLV::TLVReader & aAttributeData
         {
             err = CHIP_NO_ERROR;
         }
+        if (mpExchangeCtx->IsGroupExchangeContext())
+        {
+            // TODO retrieve Endpoint ID with GroupDataProvider using GroupId and FabricId
+            // Issue 11075
 
-        err = attributePath.GetEndpoint(&(clusterInfo.mEndpointId));
-        SuccessOrExit(err);
+            // Using endpoint 0 for test purposes
+            clusterInfo.mEndpointId = 0;
+        }
+        else
+        {
+            err = attributePath.GetEndpoint(&(clusterInfo.mEndpointId));
+            SuccessOrExit(err);
+        }
 
         err = attributePath.GetCluster(&(clusterInfo.mClusterId));
         SuccessOrExit(err);
@@ -210,6 +227,7 @@ CHIP_ERROR WriteHandler::ProcessWriteRequest(System::PacketBufferHandle && aPayl
 
     err = writeRequestParser.GetWriteRequests(&AttributeDataIBsParser);
     SuccessOrExit(err);
+
     AttributeDataIBsParser.GetReader(&AttributeDataIBsReader);
     err = ProcessAttributeDataIBs(AttributeDataIBsReader);
 

diff --git a/src/app/tests/suites/TestGroupMessaging.yaml b/src/app/tests/suites/TestGroupMessaging.yaml
@@ -33,21 +33,18 @@ tests:
     - label: "Read back Attribute"
       command: "readAttribute"
       attribute: "location"
-      disabled: true
       response:
           value: "us"
 
     - label: "Restore initial location value"
       command: "writeAttribute"
       attribute: "location"
       groupId: "1234"
-      disabled: true
       arguments:
           value: ""
 
     - label: "Read back Attribute"
       command: "readAttribute"
       attribute: "location"
-      disabled: true
       response:
           value: ""
diff --git a/src/darwin/Framework/CHIPTests/CHIPClustersTests.m b/src/darwin/Framework/CHIPTests/CHIPClustersTests.m
diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp
@@ -159,6 +159,12 @@ CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgTyp
     }
 
     {
+        // ExchangeContext for group are supposed to always be Initiator
+        if (IsGroupExchangeContext() && !IsInitiator())
+        {
+            return CHIP_ERROR_INTERNAL;
+        }
+
         // Create a new scope for `err`, to avoid shadowing warning previous `err`.
         CHIP_ERROR err = mDispatch->SendMessage(mSession.Value(), mExchangeId, IsInitiator(), GetReliableMessageContext(),
                                                 reliableTransmissionRequested, protocolId, msgType, std::move(msgBuf));
@@ -445,8 +451,12 @@ CHIP_ERROR ExchangeContext::HandleMessage(uint32_t messageCounter, const Payload
         MessageHandled();
     });
 
-    ReturnErrorOnFailure(
-        mDispatch->OnMessageReceived(messageCounter, payloadHeader, peerAddress, msgFlags, GetReliableMessageContext()));
+    // TODO : Remove this bypass for group as to perform the MessagePermitted function Issue # 12101
+    if (!IsGroupExchangeContext())
+    {
+        ReturnErrorOnFailure(
+            mDispatch->OnMessageReceived(messageCounter, payloadHeader, peerAddress, msgFlags, GetReliableMessageContext()));
+    }
 
     if (IsAckPending() && !mDelegate)
     {

diff --git a/src/transport/SessionManager.cpp b/src/transport/SessionManager.cpp
@@ -131,6 +131,8 @@ CHIP_ERROR SessionManager::PrepareMessage(SessionHandle sessionHandle, PayloadHe
             {
                 return CHIP_ERROR_INTERNAL;
             }
+            // TODO #11911 Update SecureMessageCodec::Encrypt for Group
+            ReturnErrorOnFailure(payloadHeader.EncodeBeforeData(message));
 
 #if CHIP_PROGRESS_LOGGING
             destination = sessionHandle.GetPeerNodeId();
@@ -414,7 +416,7 @@ void SessionManager::SecureUnicastMessageDispatch(const PacketHeader & packetHea
 
     if (msg.IsNull())
     {
-        ChipLogError(Inet, "Secure transport received NULL packet, discarding");
+        ChipLogError(Inet, "Secure transport received Unicast NULL packet, discarding");
         return;
     }
 
@@ -485,9 +487,9 @@ void SessionManager::SecureGroupMessageDispatch(const PacketHeader & packetHeade
     FabricIndex fabricIndex = 0; // TODO : remove initialization once GroupDataProvider->Decrypt is implemented
     // Credentials::GroupDataProvider * groups = Credentials::GetGroupDataProvider();
 
-    if (!msg.IsNull())
+    if (msg.IsNull())
     {
-        ChipLogError(Inet, "Secure transport received NULL packet, discarding");
+        ChipLogError(Inet, "Secure transport received Groupcast NULL packet, discarding");
         return;
     }
 
@@ -503,6 +505,8 @@ void SessionManager::SecureGroupMessageDispatch(const PacketHeader & packetHeade
     // VerifyOrExit(CHIP_NO_ERROR == groups->DecryptMessage(packetHeader, payloadHeader, msg),
     //     ChipLogError(Inet, "Secure transport received group message, but failed to decode it, discarding"));
 
+    ReturnOnFailure(payloadHeader.DecodeAndConsume(msg));
+
     // MCSP check
     if (packetHeader.IsValidMCSPMsg())
     {

diff --git a/zzz_generated/chip-tool/zap-generated/test/Commands.h b/zzz_generated/chip-tool/zap-generated/test/Commands.h
-Original file line number
+Diff line change
@@ Expand Up @@
         }
     exit:
-        if (status != Protocols::InteractionModel::Status::Success)
+        if (status != Protocols::InteractionModel::Status::Success && !apExchangeContext->IsGroupExchangeContext())
         {
             err = StatusResponse::SendStatusResponse(status, apExchangeContext, false /*aExpectResponse*/);
         }
@@ Expand Down @@