From be5f2d28b795efd6c6b2092d7e05d0ee1857e7f9 Mon Sep 17 00:00:00 2001 From: Justin Wood Date: Mon, 18 Jul 2022 16:55:06 -0700 Subject: [PATCH] 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 --- src/app/CommandHandler.cpp | 19 +++++- src/app/data-model/Nullable.h | 2 +- .../templates/app/cluster-objects-src.zapt | 63 +++++++++++++++++++ .../templates/app/cluster-objects.zapt | 1 + src/controller/python/chip/ChipDeviceCtrl.py | 4 +- .../python/chip/clusters/Command.py | 12 +++- .../python/chip/interaction_model/__init__.py | 10 +-- .../python/test/test_scripts/base.py | 14 +++++ .../test_scripts/split_commissioning_test.py | 4 ++ .../zap-generated/cluster-objects.cpp | 63 +++++++++++++++++++ .../zap-generated/cluster-objects.h | 1 + 11 files changed, 181 insertions(+), 12 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index cff9ae492cd4fa..2fc2dc55c4d850 100644 --- a/src/app/CommandHandler.cpp +++ b/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) +{} diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h index 31513d1f8a8817..c0e2a7ea8c9e58 100644 --- a/src/app/data-model/Nullable.h +++ b/src/app/data-model/Nullable.h @@ -79,7 +79,7 @@ struct Nullable : protected Optional 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::operator==(other); } diff --git a/src/app/zap-templates/templates/app/cluster-objects-src.zapt b/src/app/zap-templates/templates/app/cluster-objects-src.zapt index 1d1c070e9e2652..8ae7efca79c57d 100644 --- a/src/app/zap-templates/templates/app/cluster-objects-src.zapt +++ b/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 diff --git a/src/app/zap-templates/templates/app/cluster-objects.zapt b/src/app/zap-templates/templates/app/cluster-objects.zapt index 3ba302d78804ce..93d7637b5005be 100644 --- a/src/app/zap-templates/templates/app/cluster-objects.zapt +++ b/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 diff --git a/src/controller/python/chip/ChipDeviceCtrl.py b/src/controller/python/chip/ChipDeviceCtrl.py index 4679bfd72d8d2b..edc2944afea6aa 100644 --- a/src/controller/python/chip/ChipDeviceCtrl.py +++ b/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() diff --git a/src/controller/python/chip/clusters/Command.py b/src/controller/python/chip/clusters/Command.py index 21c9b476d21000..7d53b21784ff5e 100644 --- a/src/controller/python/chip/clusters/Command.py +++ b/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)) diff --git a/src/controller/python/chip/interaction_model/__init__.py b/src/controller/python/chip/interaction_model/__init__.py index 79e94646e08b63..bc1fa35c214865 100644 --- a/src/controller/python/chip/interaction_model/__init__.py +++ b/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 diff --git a/src/controller/python/test/test_scripts/base.py b/src/controller/python/test/test_scripts/base.py index afb775541f530a..6cd812371f32c2 100644 --- a/src/controller/python/test/test_scripts/base.py +++ b/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 diff --git a/src/controller/python/test/test_scripts/split_commissioning_test.py b/src/controller/python/test/test_scripts/split_commissioning_test.py index 35794b15420ce0..1652231b4ea222 100755 --- a/src/controller/python/test/test_scripts/split_commissioning_test.py +++ b/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") diff --git a/zzz_generated/app-common/app-common/zap-generated/cluster-objects.cpp b/zzz_generated/app-common/app-common/zap-generated/cluster-objects.cpp index e99aec2beec10f..6d5c4cb5cf654e 100644 --- a/zzz_generated/app-common/app-common/zap-generated/cluster-objects.cpp +++ b/zzz_generated/app-common/app-common/zap-generated/cluster-objects.cpp @@ -20695,5 +20695,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 diff --git a/zzz_generated/app-common/app-common/zap-generated/cluster-objects.h b/zzz_generated/app-common/app-common/zap-generated/cluster-objects.h index b1d5803f3fb8dc..d6b1c9d3e85fd9 100644 --- a/zzz_generated/app-common/app-common/zap-generated/cluster-objects.h +++ b/zzz_generated/app-common/app-common/zap-generated/cluster-objects.h @@ -27783,6 +27783,7 @@ struct DecodableType } // namespace Clusters bool CommandNeedsTimedInvoke(ClusterId aCluster, CommandId aCommand); +bool CommandIsFabricScoped(ClusterId aCluster, CommandId aCommand); } // namespace app } // namespace chip