Remove GroupKeyMap entries for removed KeySet (#23864)

* Remove GroupKeyMap entries for removed KeySet

Per Core spec 11.2.9.4, If there exist any entries for the accessing
fabric within the GroupKeyMap attribute that refer to the
GroupKeySetID just removed, then these entries SHALL be removed from
that list.

Fixes #23862

* Restyled by clang-format

* Restyled by prettier-yaml

* Update src/credentials/GroupDataProviderImpl.cpp

Co-authored-by: Boris Zbarsky <[email protected]>

Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>

Author: 3 people

src/app/tests/suites/TestGroupKeyManagementCluster.yaml

@@ -295,3 +295,77 @@ tests:
                 value: 0x01a2
       response:
           error: NOT_FOUND
+
+    - label: "KeySet Write 1"
+      command: "KeySetWrite"
+      arguments:
+          values:
+              - name: "GroupKeySet"
+                value:
+                    {
+                        GroupKeySetID: 0x01a1,
+                        GroupKeySecurityPolicy: 0,
+                        EpochKey0: "\xa0\xa1\xa2\xa3\xa4\xa5\xa6\xa7\xa8\xa9\xaa\xab\xac\xad\xae\xaf",
+                        EpochStartTime0: 1110000,
+                        EpochKey1: "\xb0\xb1\xb2\xb3\xb4\xb5\xb6\xb7\xb8\xb9\xba\xbb\xbc\xbd\xbe\xbf",
+                        EpochStartTime1: 1110001,
+                        EpochKey2: "\xc0\xc1\xc2\xc3\xc4\xc5\xc6\xc7\xc8\xc9\xca\xcb\xcc\xcd\xce\xcf",
+                        EpochStartTime2: 1110002,
+                    }
+
+    - label: "KeySet Write 2"
+      command: "KeySetWrite"
+      arguments:
+          values:
+              - name: "GroupKeySet"
+                value:
+                    {
+                        GroupKeySetID: 0x01a2,
+                        GroupKeySecurityPolicy: 1,
+                        EpochKey0: "\xd0\xd1\xd2\xd3\xd4\xd5\xd6\xd7\xd8\xd9\xda\xdb\xdc\xdd\xde\xdf",
+                        EpochStartTime0: 2110000,
+                        EpochKey1: "\xe0\xe1\xe2\xe3\xe4\xe5\xe6\xe7\xe8\xe9\xea\xeb\xec\xed\xee\xef",
+                        EpochStartTime1: 2110001,
+                        EpochKey2: "\xf0\xf1\xf2\xf3\xf4\xf5\xf6\xf7\xf8\xf9\xfa\xfb\xfc\xfd\xfe\xff",
+                        EpochStartTime2: 2110002,
+                    }
+
+    - label: "Map Group 1 and Group 2 to KeySet 1 and group 2 to KeySet 2"
+      command: "writeAttribute"
+      attribute: "GroupKeyMap"
+      arguments:
+          value:
+              [
+                  { FabricIndex: 1, GroupId: 0x0101, GroupKeySetID: 0x01a1 },
+                  { FabricIndex: 1, GroupId: 0x0102, GroupKeySetID: 0x01a2 },
+                  { FabricIndex: 1, GroupId: 0x0102, GroupKeySetID: 0x01a1 },
+              ]
+
+    - label: "Remove keyset 1"
+      command: "KeySetRemove"
+      arguments:
+          values:
+              - name: "GroupKeySetID"
+                value: 0x01a1
+
+    - label: "TH verifies GroupKeyMap entries for KeySet 1 have been removed"
+      cluster: "Group Key Management"
+      endpoint: 0
+      command: "readAttribute"
+      attribute: "GroupKeyMap"
+      response:
+          value: [{ FabricIndex: 1, GroupId: 0x0102, GroupKeySetID: 0x01a2 }]
+    - label: "Remove keyset 2"
+      command: "KeySetRemove"
+      arguments:
+          values:
+              - name: "GroupKeySetID"
+                value: 0x01a2
+
+    - label: "TH verifies GroupKeyMap entries for KeySet 2 have been removed"
+      cluster: "Group Key Management"
+      endpoint: 0
+      command: "readAttribute"
+      attribute: "GroupKeyMap"
+      response:
+          value: []

src/credentials/GroupDataProviderImpl.cpp

@@ -503,6 +503,37 @@ struct KeyMapData : public GroupDataProvider::GroupKey, LinkedData
         id = static_cast<uint16_t>(max_id + 1);
         return false;
     }
+
+    // returns index if the find_id is found, otherwise std::numeric_limits<size_t>::max
+    size_t Find(PersistentStorageDelegate * storage, const FabricData & fabric, const KeysetId find_id)
+    {
+        fabric_index = fabric.fabric_index;
+        id           = fabric.first_map;
+        max_id       = 0;
+        index        = 0;
+        first        = true;
+
+        while (index < fabric.map_count)
+        {
+            if (CHIP_NO_ERROR != Load(storage))
+            {
+                break;
+            }
+            if (keyset_id == find_id)
+            {
+                // Match found
+                return index;
+            }
+            max_id = std::max(id, max_id);
+            first  = false;
+            prev   = id;
+            id     = next;
+            index++;
+        }
+
+        id = static_cast<uint16_t>(max_id + 1);
+        return std::numeric_limits<size_t>::max();
+    }
 };
 
 struct EndpointData : GroupDataProvider::GroupEndpoint, PersistentData<kPersistentBufferMax>
@@ -1574,7 +1605,25 @@ CHIP_ERROR GroupDataProviderImpl::RemoveKeySet(chip::FabricIndex fabric_index, u
         fabric.keyset_count--;
     }
     // Update fabric info
-    return fabric.Save(mStorage);
+    ReturnErrorOnFailure(fabric.Save(mStorage));
+
+    // Removing a key set also removes the associated group mappings
+    KeyMapData map;
+    uint16_t original_count = fabric.map_count;
+    for (uint16_t i = 0; i < original_count; ++i)
+    {
+        fabric.Load(mStorage);
+        size_t idx = map.Find(mStorage, fabric, target_id);
+        if (idx == std::numeric_limits<size_t>::max())
+        {
+            break;
+        }
+        // NOTE: It's unclear what should happen here if we have removed the key set
+        // and possibly some mappings before failing. For now, ignoring errors, but
+        // open to suggestsions for the correct behavior.
+        RemoveGroupKeyAt(fabric_index, idx);
+    }
+    return CHIP_NO_ERROR;
 }
 
 GroupDataProvider::KeySetIterator * GroupDataProviderImpl::IterateKeySets(chip::FabricIndex fabric_index)