From f2b8a876d2af2f834354b800e5e5d582e10be8c1 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 9 Oct 2024 18:09:58 -0400 Subject: [PATCH] Use DataModel::Provider provided command information to handle command validation (#35897) * A first pass to implement better invoke logic decoupling * Fix some includes * Fix some includes * More include fixes * Fix dependency to make things build * Fixed tests * Fix includes * Restyle * Fix naming typo * Fix tizen build * Fix condition typo * Keep group command logic to use continue instead of status return ... there is no status to return * Minor update to kick restyler * one more decoupling update: sligtly more inefficient, however likely this is not important * Renamed based on code review feedback * Restyled by clang-format * Fix some code review comments * Review comment: default privilege to operate * Fix up renames * Fix hex prefix * Restyled by clang-format * Fix cirque after log formatting change. Load bearing log .... * Revert file change that I did not mean to change * Update src/app/CommandHandlerImpl.cpp Co-authored-by: Boris Zbarsky * Remove separate member for accessing fabric index * Restyled by clang-format --------- Co-authored-by: Andrei Litvin Co-authored-by: Restyled.io Co-authored-by: Boris Zbarsky --- .../lighting-app/tizen/src/DBusInterface.cpp | 9 +- src/app/BUILD.gn | 1 + src/app/CommandHandlerImpl.cpp | 87 +++++------------ src/app/CommandHandlerImpl.h | 27 ++++-- src/app/CommandResponseSender.cpp | 6 +- src/app/CommandResponseSender.h | 2 +- src/app/InteractionModelEngine.cpp | 94 ++++++++++++++++++- src/app/InteractionModelEngine.h | 7 +- src/app/data-model-provider/OperationTypes.h | 10 ++ src/app/tests/TestCommandInteraction.cpp | 18 +++- .../linux-cirque/MobileDeviceTest.py | 2 +- 11 files changed, 176 insertions(+), 87 deletions(-) diff --git a/examples/lighting-app/tizen/src/DBusInterface.cpp b/examples/lighting-app/tizen/src/DBusInterface.cpp index 36c0f19c35bb7a..5484c7daff3793 100644 --- a/examples/lighting-app/tizen/src/DBusInterface.cpp +++ b/examples/lighting-app/tizen/src/DBusInterface.cpp @@ -42,9 +42,12 @@ class CommandHandlerImplCallback : public CommandHandlerImpl::Callback { public: using Status = Protocols::InteractionModel::Status; - void OnDone(CommandHandlerImpl & apCommandObj) {} - void DispatchCommand(CommandHandlerImpl & apCommandObj, const ConcreteCommandPath & aCommandPath, TLV::TLVReader & apPayload) {} - Status CommandExists(const ConcreteCommandPath & aCommandPath) { return Status::Success; } + + void OnDone(CommandHandlerImpl & apCommandObj) override {} + void DispatchCommand(CommandHandlerImpl & apCommandObj, const ConcreteCommandPath & aCommandPath, + TLV::TLVReader & apPayload) override + {} + Status ValidateCommandCanBeDispatched(const DataModel::InvokeRequest & request) override { return Status::Success; } }; DBusInterface::DBusInterface(chip::EndpointId endpointId) : mEndpointId(endpointId) diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn index 98e7225549aaa4..f3565cf5d9a5e1 100644 --- a/src/app/BUILD.gn +++ b/src/app/BUILD.gn @@ -417,6 +417,7 @@ source_set("command-handler-impl") { "${chip_root}/src/access:types", "${chip_root}/src/app/MessageDef", "${chip_root}/src/app/data-model", + "${chip_root}/src/app/data-model-provider", "${chip_root}/src/app/util:callbacks", "${chip_root}/src/lib/support", "${chip_root}/src/messaging", diff --git a/src/app/CommandHandlerImpl.cpp b/src/app/CommandHandlerImpl.cpp index 366199a3b9f1d4..2142af63494348 100644 --- a/src/app/CommandHandlerImpl.cpp +++ b/src/app/CommandHandlerImpl.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -390,55 +391,16 @@ Status CommandHandlerImpl::ProcessCommandDataIB(CommandDataIB::Parser & aCommand VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction); { - Status commandExists = mpCallback->CommandExists(concretePath); - if (commandExists != Status::Success) - { - ChipLogDetail(DataManagement, "No command " ChipLogFormatMEI " in Cluster " ChipLogFormatMEI " on Endpoint 0x%x", - ChipLogValueMEI(concretePath.mCommandId), ChipLogValueMEI(concretePath.mClusterId), - concretePath.mEndpointId); - return FallibleAddStatus(concretePath, commandExists) != CHIP_NO_ERROR ? Status::Failure : Status::Success; - } - } - - { - Access::SubjectDescriptor subjectDescriptor = GetSubjectDescriptor(); - Access::RequestPath requestPath{ .cluster = concretePath.mClusterId, - .endpoint = concretePath.mEndpointId, - .requestType = Access::RequestType::kCommandInvokeRequest, - .entityId = concretePath.mCommandId }; - Access::Privilege requestPrivilege = RequiredPrivilege::ForInvokeCommand(concretePath); - err = Access::GetAccessControl().Check(subjectDescriptor, requestPath, requestPrivilege); - if (err != CHIP_NO_ERROR) - { - if ((err != CHIP_ERROR_ACCESS_DENIED) && (err != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL)) - { - return FallibleAddStatus(concretePath, Status::Failure) != CHIP_NO_ERROR ? Status::Failure : Status::Success; - } - // TODO: when wildcard invokes are supported, handle them to discard rather than fail with status - Status status = err == CHIP_ERROR_ACCESS_DENIED ? Status::UnsupportedAccess : Status::AccessRestricted; - return FallibleAddStatus(concretePath, status) != CHIP_NO_ERROR ? Status::Failure : Status::Success; - } - } + DataModel::InvokeRequest request; - if (CommandNeedsTimedInvoke(concretePath.mClusterId, concretePath.mCommandId) && !IsTimedInvoke()) - { - // TODO: when wildcard invokes are supported, discard a - // wildcard-expanded path instead of returning a status. - return FallibleAddStatus(concretePath, Status::NeedsTimedInteraction) != CHIP_NO_ERROR ? Status::Failure : Status::Success; - } - - if (CommandIsFabricScoped(concretePath.mClusterId, concretePath.mCommandId)) - { - // SPEC: Else if the command in the path is fabric-scoped and there is no accessing fabric, - // a CommandStatusIB SHALL be generated with the UNSUPPORTED_ACCESS Status Code. + request.path = concretePath; + request.subjectDescriptor = GetSubjectDescriptor(); + request.invokeFlags.Set(DataModel::InvokeFlags::kTimed, IsTimedInvoke()); - // 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) + Status preCheckStatus = mpCallback->ValidateCommandCanBeDispatched(request); + if (preCheckStatus != Status::Success) { - // TODO: when wildcard invokes are supported, discard a - // wildcard-expanded path instead of returning a status. - return FallibleAddStatus(concretePath, Status::UnsupportedAccess) != CHIP_NO_ERROR ? Status::Failure : Status::Success; + return FallibleAddStatus(concretePath, preCheckStatus) != CHIP_NO_ERROR ? Status::Failure : Status::Success; } } @@ -516,11 +478,19 @@ Status CommandHandlerImpl::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo // once up front and discard all the paths at once. Ordering with respect // to ACL and command presence checks does not matter, because the behavior // is the same for all of them: ignore the path. +#if !CHIP_CONFIG_USE_DATA_MODEL_INTERFACE + + // Without data model interface, we can query individual commands. + // Data model interface queries commands by a full path so we need endpointID as well. + // + // Since this is a performance update and group commands are never timed, + // missing this should not be that noticeable. if (CommandNeedsTimedInvoke(clusterId, commandId)) { // Group commands are never timed. return Status::Success; } +#endif // No check for `CommandIsFabricScoped` unlike in `ProcessCommandDataIB()` since group commands // always have an accessing fabric, by definition. @@ -542,30 +512,21 @@ Status CommandHandlerImpl::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo const ConcreteCommandPath concretePath(mapping.endpoint_id, clusterId, commandId); - if (mpCallback->CommandExists(concretePath) != Status::Success) { - ChipLogDetail(DataManagement, "No command " ChipLogFormatMEI " in Cluster " ChipLogFormatMEI " on Endpoint 0x%x", - ChipLogValueMEI(commandId), ChipLogValueMEI(clusterId), mapping.endpoint_id); + DataModel::InvokeRequest request; - continue; - } + request.path = concretePath; + request.subjectDescriptor = GetSubjectDescriptor(); + request.invokeFlags.Set(DataModel::InvokeFlags::kTimed, IsTimedInvoke()); - { - Access::SubjectDescriptor subjectDescriptor = GetSubjectDescriptor(); - Access::RequestPath requestPath{ .cluster = concretePath.mClusterId, - .endpoint = concretePath.mEndpointId, - .requestType = Access::RequestType::kCommandInvokeRequest, - .entityId = concretePath.mCommandId }; - Access::Privilege requestPrivilege = RequiredPrivilege::ForInvokeCommand(concretePath); - err = Access::GetAccessControl().Check(subjectDescriptor, requestPath, requestPrivilege); - if (err != CHIP_NO_ERROR) + Status preCheckStatus = mpCallback->ValidateCommandCanBeDispatched(request); + if (preCheckStatus != Status::Success) { - // NOTE: an expected error is CHIP_ERROR_ACCESS_DENIED, but there could be other unexpected errors; - // therefore, keep processing subsequent commands, and if any errors continue, those subsequent - // commands will likewise fail. + // Command failed for a specific path, but keep trying the rest of the paths. continue; } } + if ((err = DataModelCallbacks::GetInstance()->PreCommandReceived(concretePath, GetSubjectDescriptor())) == CHIP_NO_ERROR) { TLV::TLVReader dataReader(commandDataReader); diff --git a/src/app/CommandHandlerImpl.h b/src/app/CommandHandlerImpl.h index ddd499658e15d9..2603b96f223c7e 100644 --- a/src/app/CommandHandlerImpl.h +++ b/src/app/CommandHandlerImpl.h @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -50,21 +51,29 @@ class CommandHandlerImpl : public CommandHandler */ virtual void OnDone(CommandHandlerImpl & apCommandObj) = 0; + /** + * Perform pre-validation that the command dispatch can be performed. In particular: + * - check command existence/validity + * - validate ACL + * - validate timed-invoke and fabric-scoped requirements + * + * Returns Status::Success if the command can be dispatched, otherwise it will + * return the status to be forwarded to the client on failure. + * + * Possible error return codes: + * - UnsupportedEndpoint/UnsupportedCluster/UnsupportedCommand if the command path is invalid + * - NeedsTimedInteraction + * - UnsupportedAccess (ACL failure or fabric scoped without a valid fabric index) + * - AccessRestricted + */ + virtual Protocols::InteractionModel::Status ValidateCommandCanBeDispatched(const DataModel::InvokeRequest & request) = 0; + /* * Upon processing of a CommandDataIB, this method is invoked to dispatch the command * to the right server-side handler provided by the application. */ virtual void DispatchCommand(CommandHandlerImpl & apCommandObj, const ConcreteCommandPath & aCommandPath, TLV::TLVReader & apPayload) = 0; - - /* - * Check to see if a command implementation exists for a specific - * concrete command path. If it does, Success will be returned. If - * not, one of UnsupportedEndpoint, UnsupportedCluster, or - * UnsupportedCommand will be returned, depending on how the command - * fails to exist. - */ - virtual Protocols::InteractionModel::Status CommandExists(const ConcreteCommandPath & aCommandPath) = 0; }; struct InvokeResponseParameters diff --git a/src/app/CommandResponseSender.cpp b/src/app/CommandResponseSender.cpp index f23786e28dd318..0b028510c0da16 100644 --- a/src/app/CommandResponseSender.cpp +++ b/src/app/CommandResponseSender.cpp @@ -132,10 +132,10 @@ void CommandResponseSender::DispatchCommand(CommandHandlerImpl & apCommandObj, c mpCommandHandlerCallback->DispatchCommand(apCommandObj, aCommandPath, apPayload); } -Status CommandResponseSender::CommandExists(const ConcreteCommandPath & aCommandPath) +Protocols::InteractionModel::Status CommandResponseSender::ValidateCommandCanBeDispatched(const DataModel::InvokeRequest & request) { - VerifyOrReturnValue(mpCommandHandlerCallback, Protocols::InteractionModel::Status::UnsupportedCommand); - return mpCommandHandlerCallback->CommandExists(aCommandPath); + VerifyOrReturnValue(mpCommandHandlerCallback, Protocols::InteractionModel::Status::Failure); + return mpCommandHandlerCallback->ValidateCommandCanBeDispatched(request); } CHIP_ERROR CommandResponseSender::SendCommandResponse() diff --git a/src/app/CommandResponseSender.h b/src/app/CommandResponseSender.h index 0e03a87f1e8cab..f0b8f11d40ca86 100644 --- a/src/app/CommandResponseSender.h +++ b/src/app/CommandResponseSender.h @@ -64,7 +64,7 @@ class CommandResponseSender : public Messaging::ExchangeDelegate, void DispatchCommand(CommandHandlerImpl & apCommandObj, const ConcreteCommandPath & aCommandPath, TLV::TLVReader & apPayload) override; - Protocols::InteractionModel::Status CommandExists(const ConcreteCommandPath & aCommandPath) override; + Protocols::InteractionModel::Status ValidateCommandCanBeDispatched(const DataModel::InvokeRequest & request) override; /** * Gets the inner exchange context object, without ownership. diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 59888feaf916b7..5f0e392c26177a 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -32,6 +32,9 @@ #include #include #include +#include +#include +#include #include #include #include @@ -42,10 +45,9 @@ #include #include #include +#include #if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE -#include - // TODO: defaulting to codegen should eventually be an application choice and not // hard-coded in the interaction model #include @@ -1652,6 +1654,8 @@ void InteractionModelEngine::DispatchCommand(CommandHandlerImpl & apCommandObj, DataModel::InvokeRequest request; request.path = aCommandPath; + request.invokeFlags.Set(DataModel::InvokeFlags::kTimed, apCommandObj.IsTimedInvoke()); + request.subjectDescriptor = apCommandObj.GetSubjectDescriptor(); std::optional status = GetDataModelProvider()->Invoke(request, apPayload, &apCommandObj); @@ -1684,7 +1688,91 @@ void InteractionModelEngine::DispatchCommand(CommandHandlerImpl & apCommandObj, #endif // CHIP_CONFIG_USE_DATA_MODEL_INTERFACE } -Protocols::InteractionModel::Status InteractionModelEngine::CommandExists(const ConcreteCommandPath & aCommandPath) +Protocols::InteractionModel::Status InteractionModelEngine::ValidateCommandCanBeDispatched(const DataModel::InvokeRequest & request) +{ + + Status status = CheckCommandExistence(request.path); + + if (status != Status::Success) + { + ChipLogDetail(DataManagement, "No command " ChipLogFormatMEI " in Cluster " ChipLogFormatMEI " on Endpoint %u", + ChipLogValueMEI(request.path.mCommandId), ChipLogValueMEI(request.path.mClusterId), request.path.mEndpointId); + return status; + } + + status = CheckCommandAccess(request); + VerifyOrReturnValue(status == Status::Success, status); + + return CheckCommandFlags(request); +} + +Protocols::InteractionModel::Status InteractionModelEngine::CheckCommandAccess(const DataModel::InvokeRequest & aRequest) +{ + if (!aRequest.subjectDescriptor.has_value()) + { + return Status::UnsupportedAccess; // we require a subject for invoke + } + + Access::RequestPath requestPath{ .cluster = aRequest.path.mClusterId, + .endpoint = aRequest.path.mEndpointId, + .requestType = Access::RequestType::kCommandInvokeRequest, + .entityId = aRequest.path.mCommandId }; +#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE + std::optional commandInfo = mDataModelProvider->GetAcceptedCommandInfo(aRequest.path); + Access::Privilege minimumRequiredPrivilege = + commandInfo.has_value() ? commandInfo->invokePrivilege : Access::Privilege::kOperate; +#else + Access::Privilege minimumRequiredPrivilege = RequiredPrivilege::ForInvokeCommand(aRequest.path); +#endif + CHIP_ERROR err = Access::GetAccessControl().Check(*aRequest.subjectDescriptor, requestPath, minimumRequiredPrivilege); + if (err != CHIP_NO_ERROR) + { + if ((err != CHIP_ERROR_ACCESS_DENIED) && (err != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL)) + { + return Status::Failure; + } + return err == CHIP_ERROR_ACCESS_DENIED ? Status::UnsupportedAccess : Status::AccessRestricted; + } + + return Status::Success; +} + +Protocols::InteractionModel::Status InteractionModelEngine::CheckCommandFlags(const DataModel::InvokeRequest & aRequest) +{ +#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE + std::optional commandInfo = mDataModelProvider->GetAcceptedCommandInfo(aRequest.path); + // This is checked by previous validations, so it should not happen + VerifyOrDie(commandInfo.has_value()); + + const bool commandNeedsTimedInvoke = commandInfo->flags.Has(DataModel::CommandQualityFlags::kTimed); + const bool commandIsFabricScoped = commandInfo->flags.Has(DataModel::CommandQualityFlags::kFabricScoped); +#else + const bool commandNeedsTimedInvoke = CommandNeedsTimedInvoke(aRequest.path.mClusterId, aRequest.path.mCommandId); + const bool commandIsFabricScoped = CommandIsFabricScoped(aRequest.path.mClusterId, aRequest.path.mCommandId); +#endif + + if (commandNeedsTimedInvoke && !aRequest.invokeFlags.Has(DataModel::InvokeFlags::kTimed)) + { + return Status::NeedsTimedInteraction; + } + + if (commandIsFabricScoped) + { + // SPEC: Else if the command in the path is fabric-scoped and there is no accessing fabric, + // a CommandStatusIB SHALL be generated with the UNSUPPORTED_ACCESS Status Code. + + // Fabric-scoped commands are not allowed before a specific accessing fabric is available. + // This is mostly just during a PASE session before AddNOC. + if (aRequest.GetAccessingFabricIndex() == kUndefinedFabricIndex) + { + return Status::UnsupportedAccess; + } + } + + return Status::Success; +} + +Protocols::InteractionModel::Status InteractionModelEngine::CheckCommandExistence(const ConcreteCommandPath & aCommandPath) { #if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE auto provider = GetDataModelProvider(); diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index b52b4eb451fc5c..ef4b041f19d020 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -49,6 +49,7 @@ #include #include #include +#include #include #include #include @@ -508,7 +509,7 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, void DispatchCommand(CommandHandlerImpl & apCommandObj, const ConcreteCommandPath & aCommandPath, TLV::TLVReader & apPayload) override; - Protocols::InteractionModel::Status CommandExists(const ConcreteCommandPath & aCommandPath) override; + Protocols::InteractionModel::Status ValidateCommandCanBeDispatched(const DataModel::InvokeRequest & request) override; bool HasActiveRead(); @@ -612,6 +613,10 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, void ShutdownMatchingSubscriptions(const Optional & aFabricIndex = NullOptional, const Optional & aPeerNodeId = NullOptional); + Status CheckCommandExistence(const ConcreteCommandPath & aCommandPath); + Status CheckCommandAccess(const DataModel::InvokeRequest & aRequest); + Status CheckCommandFlags(const DataModel::InvokeRequest & aRequest); + /** * Check if the given attribute path is a valid path in the data model provider. */ diff --git a/src/app/data-model-provider/OperationTypes.h b/src/app/data-model-provider/OperationTypes.h index 8758e02ef89c5d..6bfffccc655f00 100644 --- a/src/app/data-model-provider/OperationTypes.h +++ b/src/app/data-model-provider/OperationTypes.h @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -55,6 +56,15 @@ struct OperationRequest /// /// NOTE: once kInternal flag is removed, this will become non-optional std::optional subjectDescriptor; + + /// Accessing fabric index is the subjectDescriptor fabric index (if any). + /// This is a readability convenience function. + /// + /// Returns kUndefinedFabricIndex if no subject descriptor is available + FabricIndex GetAccessingFabricIndex() const + { + return subjectDescriptor.has_value() ? subjectDescriptor->fabricIndex : kUndefinedFabricIndex; + } }; enum class ReadFlags : uint32_t diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index e4465c119d14f8..a44d76d1fc6258 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -25,7 +25,6 @@ #include #include -#include #include #include @@ -38,6 +37,7 @@ #include #include #include +#include #include #include #include @@ -372,9 +372,21 @@ class MockCommandHandlerCallback : public CommandHandlerImpl::Callback { DispatchSingleClusterCommand(aCommandPath, apPayload, &apCommandObj); } - Protocols::InteractionModel::Status CommandExists(const ConcreteCommandPath & aCommandPath) + + Protocols::InteractionModel::Status ValidateCommandCanBeDispatched(const DataModel::InvokeRequest & request) override { - return ServerClusterCommandExists(aCommandPath); + using Protocols::InteractionModel::Status; + + Status status = ServerClusterCommandExists(request.path); + if (status != Status::Success) + { + return status; + } + + // NOTE: IM does more validation here, however for now we do minimal options + // to pass the test. + + return Status::Success; } void ResetCounter() { onFinalCalledTimes = 0; } diff --git a/src/test_driver/linux-cirque/MobileDeviceTest.py b/src/test_driver/linux-cirque/MobileDeviceTest.py index a206dc31c25c03..c8a73baf0c2a78 100755 --- a/src/test_driver/linux-cirque/MobileDeviceTest.py +++ b/src/test_driver/linux-cirque/MobileDeviceTest.py @@ -126,7 +126,7 @@ def run_controller_test(self): "Toggle ep1 on/off from state 0 to 1", "Received command for Endpoint=1 Cluster=0x0000_0006 Command=0x0000_0000", "Toggle ep1 on/off from state 1 to 0", - "No command 0x0000_0001 in Cluster 0x0000_0006 on Endpoint 0xe9"]), + "No command 0x0000_0001 in Cluster 0x0000_0006 on Endpoint 233"]), "Datamodel test failed: cannot find matching string from device {}".format(device_id))