Add missed code review comments (#28626)

* Fix KeySetRemove to fail on key set index 0

Problem:

- Removing KeySet index 0 is not allowed by spec, but SDK allowed it.
- Checking that we ran without accessing fabric is not done, so
  error is FAILURE instead of UNSUPPORTED_ACCESS.
- Fixes #28518

This PR:

- Adds the check and tests against removing fabric index zero
- Improves error reporting for several errors in the cluster
- Combines validation logic for accessing fabric that was missing

Testing:

- Added unit tests and integration tests for additions
  (except for the unsupported access that requires PASE to check)

* Regen tests with ZAP

* Restyled by clang-format

* Remove explicit check for undefined fabric index

* Fix build

* Rename ValidateAndGetProviderAndFabric to just GetProviderAndFabric

* Added back cleanup for VerifyOrDie argument checking

* Restyle

* Update based on code review comments

* Remove two more flight check comments

* Restyle

* Fix merge error

---------

Co-authored-by: [email protected] <[email protected]>
Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Andrei Litvin <[email protected]>

Author: 4 people

src/app/CommandHandler.cpp

@@ -484,7 +484,15 @@ void CommandHandler::AddStatusAndLogIfFailure(const ConcreteCommandPath & aComma
                      ChipLogValueIMStatus(aStatus), aMessage);
     }
 
-    LogErrorOnFailure(AddStatus(aCommandPath, aStatus));
+    CHIP_ERROR err = AddStatus(aCommandPath, aStatus);
+    if (err != CHIP_NO_ERROR)
+    {
+        ChipLogError(DataManagement,
+                     "Failed to set status on Endpoint=%u Cluster=" ChipLogFormatMEI " Command=" ChipLogFormatMEI
+                     ": %" CHIP_ERROR_FORMAT,
+                     aCommandPath.mEndpointId, ChipLogValueMEI(aCommandPath.mClusterId), ChipLogValueMEI(aCommandPath.mCommandId),
+                     err.Format());
+    }
 }
 
 CHIP_ERROR CommandHandler::AddClusterSpecificSuccess(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus)

src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp

@@ -450,7 +450,6 @@ bool emberAfGroupKeyManagementClusterKeySetWriteCallback(
     Credentials::GroupDataProvider * provider = nullptr;
     const FabricInfo * fabric                 = nullptr;
 
-    // Flight-check fabric scoping.
     if (!GetProviderAndFabric(commandObj, commandPath, &provider, &fabric))
     {
         // Command will already have status populated from validation.
@@ -555,7 +554,6 @@ bool emberAfGroupKeyManagementClusterKeySetReadCallback(
     Credentials::GroupDataProvider * provider = nullptr;
     const FabricInfo * fabric                 = nullptr;
 
-    // Flight-check fabric scoping.
     if (!GetProviderAndFabric(commandObj, commandPath, &provider, &fabric))
     {
         // Command will already have status populated from validation.
@@ -621,7 +619,6 @@ bool emberAfGroupKeyManagementClusterKeySetRemoveCallback(
     Credentials::GroupDataProvider * provider = nullptr;
     const FabricInfo * fabric                 = nullptr;
 
-    // Flight-check fabric scoping.
     if (!GetProviderAndFabric(commandObj, commandPath, &provider, &fabric))
     {
         // Command will already have status populated from validation.
@@ -694,7 +691,6 @@ bool emberAfGroupKeyManagementClusterKeySetReadAllIndicesCallback(
     Credentials::GroupDataProvider * provider = nullptr;
     const FabricInfo * fabric                 = nullptr;
 
-    // Flight-check fabric scoping.
     if (!GetProviderAndFabric(commandObj, commandPath, &provider, &fabric))
     {
         // Command will already have status populated from validation.