Fix status of fabric-scoped commands running in PASE (#20849) (#20895)

* Fix status on fabric-scoped commnads in PASE

* Add cirque test

* Fixed typo

* Revert file committed by mistake

* Fix CI

* Apply review comments from @bzbarsky-apple

Co-authored-by: Tennessee Carmel-Veilleux <[email protected]>

src/app/CommandHandler.cpp

@@ -315,6 +315,18 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand
         return AddStatus(concretePath, Protocols::InteractionModel::Status::NeedsTimedInteraction);
     }
 
+    if (CommandIsFabricScoped(concretePath.mClusterId, concretePath.mCommandId))
+    {
+        // Fabric-scoped commands are not allowed before a specific accessing fabric is available.
+        // This is mostly just during a PASE session before AddNOC.
+        if (GetAccessingFabricIndex() == kUndefinedFabricIndex)
+        {
+            // TODO: when wildcard invokes are supported, discard a
+            // wildcard-expanded path instead of returning a status.
+            return AddStatus(concretePath, Protocols::InteractionModel::Status::UnsupportedAccess);
+        }
+    }
+
     err = aCommandElement.GetFields(&commandDataReader);
     if (CHIP_END_OF_TLV == err)
     {
@@ -395,6 +407,10 @@ CHIP_ERROR CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo
         ExitNow();
     }
 
+    // No check for `CommandIsFabricScoped` unlike in `ProcessCommandDataIB()` since group commands
+    // always have an accessing fabric, by definition.
+
+    // Find which endpoints can process the command, and dispatch to them.
     iterator = groupDataProvider->IterateEndpoints(fabric);
     VerifyOrExit(iterator != nullptr, err = CHIP_ERROR_NO_MEMORY);
 
@@ -680,4 +696,5 @@ CHIP_ERROR __attribute__((weak)) MatterPreCommandReceivedCallback(const chip::ap
     return CHIP_NO_ERROR;
 }
 void __attribute__((weak)) MatterPostCommandReceivedCallback(const chip::app::ConcreteCommandPath & commandPath,
-                                                             const chip::Access::SubjectDescriptor & subjectDescriptor) {}
+                                                             const chip::Access::SubjectDescriptor & subjectDescriptor)
+{}

src/app/data-model/Nullable.h

@@ -79,7 +79,7 @@ struct Nullable : protected Optional<T>
         return true;
     }
 
-    // The only fabric-scoped objects in the spec are events and structs inside lists, and neither one can be nullable.
+    // The only fabric-scoped objects in the spec are commands, events and structs inside lists, and none of those can be nullable.
     static constexpr bool kIsFabricScoped = false;
 
     bool operator==(const Nullable & other) const { return Optional<T>::operator==(other); }

src/app/zap-templates/templates/app/cluster-objects-src.zapt

@@ -173,5 +173,68 @@ bool CommandNeedsTimedInvoke(ClusterId aCluster, CommandId aCommand)
     return false;
 }
 
+// TODO(#20811): Actually generate the following based on ZAP metadata
+// See https://github.com/project-chip/zap/issues/609
+bool CommandIsFabricScoped(ClusterId aCluster, CommandId aCommand)
+{
+    // Maybe it would be smaller code to codegen a table and walk over it?
+    // Not sure.
+    switch (aCluster)
+    {
+    case Clusters::Groups::Id: {
+        switch (aCommand)
+        {
+        case Clusters::Groups::Commands::AddGroup::Id:
+        case Clusters::Groups::Commands::ViewGroup::Id:
+        case Clusters::Groups::Commands::GetGroupMembership::Id:
+        case Clusters::Groups::Commands::RemoveGroup::Id:
+        case Clusters::Groups::Commands::RemoveAllGroups::Id:
+            return true;
+        default:
+            return false;
+        }
+    }
+    case Clusters::GroupKeyManagement::Id: {
+        switch (aCommand)
+        {
+        case Clusters::GroupKeyManagement::Commands::KeySetWrite::Id:
+        case Clusters::GroupKeyManagement::Commands::KeySetRead::Id:
+        case Clusters::GroupKeyManagement::Commands::KeySetRemove::Id:
+        case Clusters::GroupKeyManagement::Commands::KeySetReadAllIndices::Id:
+            return true;
+        default:
+            return false;
+        }
+    }
+    case Clusters::GeneralCommissioning::Id: {
+        switch (aCommand)
+        {
+        case Clusters::GeneralCommissioning::Commands::CommissioningComplete::Id:
+            return true;
+        default:
+            return false;
+        }
+    }
+    case Clusters::Scenes::Id: {
+        // Entire cluster is fabric-scoped.
+        return true;
+    }
+    case Clusters::OperationalCredentials::Id: {
+        switch (aCommand)
+        {
+        case Clusters::OperationalCredentials::Commands::UpdateNOC::Id:
+        case Clusters::OperationalCredentials::Commands::UpdateFabricLabel::Id:
+            return true;
+        default:
+            return false;
+        }
+    }
+    default:
+        break;
+    }
+
+    return false;
+}
+
 } // namespace app
 } // namespace chip

src/app/zap-templates/templates/app/cluster-objects.zapt

@@ -215,6 +215,7 @@ public:
 } // namespace Clusters
 
 bool CommandNeedsTimedInvoke(ClusterId aCluster, CommandId aCommand);
+bool CommandIsFabricScoped(ClusterId aCluster, CommandId aCommand);
 
 } // namespace app
 } // namespace chip

src/controller/python/chip/ChipDeviceCtrl.py

@@ -151,7 +151,7 @@ def HandleKeyExchangeComplete(err):
                 self._ChipStack.callbackRes = self._ChipStack.ErrorToException(
                     err)
             else:
-                print("Established CASE with Device")
+                print("Established secure session with Device")
             if self.state != DCState.COMMISSIONING:
                 # During Commissioning, HandleKeyExchangeComplete will also be called,
                 # in this case the async operation should be marked as finished by
@@ -939,7 +939,7 @@ def ZCLSend(self, cluster, command, nodeid, endpoint, groupid, args, blocking=Fa
             print(f"CommandResponse {res}")
             return (0, res)
         except InteractionModelError as ex:
-            return (int(ex.state), None)
+            return (int(ex.status), None)
 
     def ZCLReadAttribute(self, cluster, attribute, nodeid, endpoint, groupid, blocking=True):
         self.CheckIsActive()

src/controller/python/chip/clusters/Command.py

@@ -30,6 +30,11 @@
 import inspect
 import sys
 import builtins
+import logging
+
+
+logger = logging.getLogger('chip.cluster.Command')
+logger.setLevel(logging.ERROR)
 
 
 @dataclass
@@ -94,7 +99,7 @@ def handleResponse(self, path: CommandPath, status: Status, response: bytes):
         self._event_loop.call_soon_threadsafe(
             self._handleResponse, path, status, response)
 
-    def _handleError(self, imError: int, chipError: int, exception: Exception):
+    def _handleError(self, imError: Status, chipError: int, exception: Exception):
         if exception:
             self._future.set_exception(exception)
         elif chipError != 0:
@@ -103,8 +108,9 @@ def _handleError(self, imError: int, chipError: int, exception: Exception):
         else:
             try:
                 self._future.set_exception(
-                    chip.interaction_model.InteractionModelError(chip.interaction_model.Status(status.IMStatus)))
-            except:
+                    chip.interaction_model.InteractionModelError(chip.interaction_model.Status(imError.IMStatus)))
+            except Exception as e2:
+                logger.exception("Failed to map interaction model status received: %s. Remapping to Failure." % imError)
                 self._future.set_exception(chip.interaction_model.InteractionModelError(
                     chip.interaction_model.Status.Failure))
 

src/controller/python/chip/interaction_model/__init__.py

@@ -74,12 +74,12 @@ class Status(enum.IntEnum):
 
 
 class InteractionModelError(ChipStackException):
-    def __init__(self, state: Status):
-        self._state = state
+    def __init__(self, status: Status):
+        self._status = status
 
     def __str__(self):
-        return f"InteractionModelError: {self._state.name} (0x{self._state.value:x})"
+        return f"InteractionModelError: {self._status.name} (0x{self._status.value:x})"
 
     @property
-    def state(self) -> Status:
-        return self._state
+    def status(self) -> Status:
+        return self._status

src/controller/python/test/test_scripts/base.py

@@ -1012,3 +1012,17 @@ def TestNonControllerAPIs(self):
             self.logger.exception(f"Failed to finish API test: {ex}")
             return False
         return True
+
+    def TestFabricScopedCommandDuringPase(self, nodeid: int):
+        '''Validates that fabric-scoped commands fail during PASE with UNSUPPORTED_ACCESS
+
+        The nodeid is the PASE pseudo-node-ID used during PASE establishment
+        '''
+        status = None
+        try:
+            response = asyncio.run(self.devCtrl.SendCommand(
+                nodeid, 0, Clusters.OperationalCredentials.Commands.UpdateFabricLabel("roboto")))
+        except IM.InteractionModelError as ex:
+            status = ex.status
+
+        return status == IM.Status.UnsupportedAccess

src/controller/python/test/test_scripts/split_commissioning_test.py

@@ -108,6 +108,10 @@ def main():
                                 nodeid=2),
               "Failed to establish PASE connection with device 2")
 
+    logger.info("Attempting to execute a fabric-scoped command during PASE before AddNOC")
+    FailIfNot(test.TestFabricScopedCommandDuringPase(nodeid=1),
+              "Did not get UNSUPPORTED_ACCESS for fabric-scoped command during PASE")
+
     FailIfNot(test.TestCommissionOnly(nodeid=1),
               "Failed to commission device 1")