Groups cluster: Check that keys are set before AddGroup. (#20688)

* Groups cluster: Check that keys are set before AddGroup. * Groups cluster: Review comments applied.
project-chip · Jul 7, 2023 · 4931485 · 4931485
1 parent 20c926c
commit 4931485
Show file tree

Hide file tree

Showing 6 changed files with 1,094 additions and 505 deletions.
diff --git a/src/app/clusters/groups-server/groups-server.cpp b/src/app/clusters/groups-server/groups-server.cpp
@@ -68,6 +68,9 @@ using namespace app::Clusters;
 using namespace app::Clusters::Groups;
 using namespace chip::Credentials;
 
+/**
+ * @brief Checks if group-endpoint association exist for the given fabric
+ */
 static bool GroupExists(FabricIndex fabricIndex, EndpointId endpointId, GroupId groupId)
 {
     GroupDataProvider * provider = GetGroupDataProvider();
@@ -76,13 +79,40 @@ static bool GroupExists(FabricIndex fabricIndex, EndpointId endpointId, GroupId
     return provider->HasEndpoint(fabricIndex, groupId, endpointId);
 }
 
+/**
+ * @brief Checks if there are key set associated with the given GroupId
+ */
+static bool KeyExists(FabricIndex fabricIndex, GroupId groupId)
+{
+    GroupDataProvider * provider = GetGroupDataProvider();
+    VerifyOrReturnError(nullptr != provider, false);
+    GroupDataProvider::GroupKey entry;
+
+    auto it    = provider->IterateGroupKeys(fabricIndex);
+    bool found = false;
+    while (it->Next(entry) && !found)
+    {
+        found = (entry.group_id == groupId);
+    }
+    it->Release();
+
+    if (found)
+    {
+        GroupDataProvider::KeySet keys;
+        found = (CHIP_NO_ERROR == provider->GetKeySet(fabricIndex, entry.keyset_id, keys));
+    }
+    return found;
+}
+
 static EmberAfStatus GroupAdd(FabricIndex fabricIndex, EndpointId endpointId, GroupId groupId, const CharSpan & groupName)
 {
     VerifyOrReturnError(IsFabricGroupId(groupId), EMBER_ZCL_STATUS_CONSTRAINT_ERROR);
 
     GroupDataProvider * provider = GetGroupDataProvider();
     VerifyOrReturnError(nullptr != provider, EMBER_ZCL_STATUS_NOT_FOUND);
+    VerifyOrReturnError(KeyExists(fabricIndex, groupId), EMBER_ZCL_STATUS_UNSUPPORTED_ACCESS);
 
+    // Add a new entry to the GroupTable
     CHIP_ERROR err = provider->SetGroupInfo(fabricIndex, GroupDataProvider::GroupInfo(groupId, groupName));
     if (CHIP_NO_ERROR == err)
     {

diff --git a/src/app/tests/suites/TestGroupKeyManagementCluster.yaml b/src/app/tests/suites/TestGroupKeyManagementCluster.yaml
@@ -41,40 +41,6 @@ tests:
       response:
           value: 2
 
-    - label: "Add Group 1"
-      cluster: "Groups"
-      endpoint: 1
-      command: "AddGroup"
-      arguments:
-          values:
-              - name: "GroupId"
-                value: 0x0101
-              - name: "GroupName"
-                value: "Group #1"
-      response:
-          values:
-              - name: "status"
-                value: 0
-              - name: "GroupId"
-                value: 0x0101
-
-    - label: "Add Group 2"
-      cluster: "Groups"
-      endpoint: 1
-      command: "AddGroup"
-      arguments:
-          values:
-              - name: "GroupId"
-                value: 0x0102
-              - name: "GroupName"
-                value: "Group #2"
-      response:
-          values:
-              - name: "status"
-                value: 0
-              - name: "GroupId"
-                value: 0x0102
-
     - label: "KeySet Write 1"
       command: "KeySetWrite"
       arguments:
@@ -166,6 +132,40 @@ tests:
                   { FabricIndex: 1, GroupId: 0x0102, GroupKeySetID: 0x01a2 },
               ]
 
+    - label: "Add Group 1"
+      cluster: "Groups"
+      endpoint: 1
+      command: "AddGroup"
+      arguments:
+          values:
+              - name: "GroupId"
+                value: 0x0101
+              - name: "GroupName"
+                value: "Group #1"
+      response:
+          values:
+              - name: "status"
+                value: 0
+              - name: "GroupId"
+                value: 0x0101
+
+    - label: "Add Group 2"
+      cluster: "Groups"
+      endpoint: 1
+      command: "AddGroup"
+      arguments:
+          values:
+              - name: "GroupId"
+                value: 0x0102
+              - name: "GroupName"
+                value: "Group #2"
+      response:
+          values:
+              - name: "status"
+                value: 0
+              - name: "GroupId"
+                value: 0x0102
+
     - label: "Read GroupTable"
       command: "readAttribute"
       attribute: "GroupTable"

diff --git a/src/app/tests/suites/TestGroupMessaging.yaml b/src/app/tests/suites/TestGroupMessaging.yaml
@@ -41,40 +41,6 @@ tests:
               - name: "nodeId"
                 value: nodeId
 
-    - label: "Add Group 1 (endpoint 1)"
-      cluster: "Groups"
-      command: "AddGroup"
-      endpoint: 1
-      arguments:
-          values:
-              - name: "groupId"
-                value: 0x0101
-              - name: "groupName"
-                value: "Group #1"
-      response:
-          values:
-              - name: "status"
-                value: 0
-              - name: "groupId"
-                value: 0x0101
-
-    - label: "Add Group 2 (endpoint 0)"
-      cluster: "Groups"
-      command: "AddGroup"
-      endpoint: 0
-      arguments:
-          values:
-              - name: "groupId"
-                value: 0x0102
-              - name: "groupName"
-                value: "Group #2"
-      response:
-          values:
-              - name: "status"
-                value: 0
-              - name: "groupId"
-                value: 0x0102
-
     - label: "KeySet Write 1"
       cluster: "Group Key Management"
       command: "KeySetWrite"
@@ -122,6 +88,40 @@ tests:
                   { FabricIndex: 0, GroupId: 0x0102, GroupKeySetID: 0x01a2 },
               ]
 
+    - label: "Add Group 1 (endpoint 1)"
+      cluster: "Groups"
+      command: "AddGroup"
+      endpoint: 1
+      arguments:
+          values:
+              - name: "groupId"
+                value: 0x0101
+              - name: "groupName"
+                value: "Group #1"
+      response:
+          values:
+              - name: "status"
+                value: 0
+              - name: "groupId"
+                value: 0x0101
+
+    - label: "Add Group 2 (endpoint 0)"
+      cluster: "Groups"
+      command: "AddGroup"
+      endpoint: 0
+      arguments:
+          values:
+              - name: "groupId"
+                value: 0x0102
+              - name: "groupName"
+                value: "Group #2"
+      response:
+          values:
+              - name: "status"
+                value: 0
+              - name: "groupId"
+                value: 0x0102
+
     - label: "Install ACLs"
       cluster: "Access Control"
       command: "writeAttribute"
@@ -169,7 +169,7 @@ tests:
       arguments:
           values:
               - name: "ms"
-                value: 100
+                value: 1000
 
       # Test Pair 2 : Validates previous group write attribute with a unicast to read
     - label: "Read back Attribute"
@@ -290,42 +290,6 @@ tests:
               - name: "nodeId"
                 value: nodeId2
 
-    - label: "Add Group 1 (endpoint 1) for gamma"
-      identity: "gamma"
-      cluster: "Groups"
-      command: "AddGroup"
-      endpoint: 1
-      arguments:
-          values:
-              - name: "groupId"
-                value: 0x0101
-              - name: "groupName"
-                value: "Group #1"
-      response:
-          values:
-              - name: "status"
-                value: 0
-              - name: "groupId"
-                value: 0x0101
-
-    - label: "Add Group 2 (endpoint 0) for gamma"
-      identity: "gamma"
-      cluster: "Groups"
-      command: "AddGroup"
-      endpoint: 0
-      arguments:
-          values:
-              - name: "groupId"
-                value: 0x0102
-              - name: "groupName"
-                value: "Group #2"
-      response:
-          values:
-              - name: "status"
-                value: 0
-              - name: "groupId"
-                value: 0x0102
-
     - label: "KeySet Write 1 for gamma"
       identity: "gamma"
       cluster: "Group Key Management"
@@ -376,6 +340,42 @@ tests:
                   { FabricIndex: 0, GroupId: 0x0102, GroupKeySetID: 0x01a2 },
               ]
 
+    - label: "Add Group 1 (endpoint 1) for gamma"
+      identity: "gamma"
+      cluster: "Groups"
+      command: "AddGroup"
+      endpoint: 1
+      arguments:
+          values:
+              - name: "groupId"
+                value: 0x0101
+              - name: "groupName"
+                value: "Group #1"
+      response:
+          values:
+              - name: "status"
+                value: 0
+              - name: "groupId"
+                value: 0x0101
+
+    - label: "Add Group 2 (endpoint 0) for gamma"
+      identity: "gamma"
+      cluster: "Groups"
+      command: "AddGroup"
+      endpoint: 0
+      arguments:
+          values:
+              - name: "groupId"
+                value: 0x0102
+              - name: "groupName"
+                value: "Group #2"
+      response:
+          values:
+              - name: "status"
+                value: 0
+              - name: "groupId"
+                value: 0x0102
+
     - label: "Install ACLs for gamma"
       identity: "gamma"
       cluster: "Access Control"