project-chip · mergify · Oct 9, 2024 · Oct 3, 2024 · Oct 3, 2024 · Oct 3, 2024
@@ -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
@@ -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
@@ -22,6 +22,7 @@
 #include <app/MessageDef/StatusIB.h>
 #include <app/RequiredPrivilege.h>
 #include <app/StatusResponse.h>
+#include <app/data-model-provider/OperationTypes.h>
 #include <app/util/MatterCallbacks.h>
 #include <credentials/GroupDataProvider.h>
 #include <lib/core/CHIPConfig.h>
@@ -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
@@ -21,6 +21,7 @@
 #include <app/CommandPathRegistry.h>
 #include <app/MessageDef/InvokeRequestMessage.h>
 #include <app/MessageDef/InvokeResponseMessage.h>
+#include <app/data-model-provider/OperationTypes.h>
 #include <lib/core/TLV.h>
 #include <lib/core/TLVDebug.h>
 #include <lib/support/BitFlags.h>
@@ -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
@@ -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
@@ -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
@@ -32,6 +32,9 @@
 #include <app/AppConfig.h>
 #include <app/CommandHandlerInterfaceRegistry.h>
 #include <app/RequiredPrivilege.h>
+#include <app/data-model-provider/ActionReturnStatus.h>
+#include <app/data-model-provider/MetadataTypes.h>
+#include <app/data-model-provider/OperationTypes.h>
 #include <app/util/IMClusterCommandHandler.h>
 #include <app/util/af-types.h>
 #include <app/util/ember-compatibility-functions.h>
@@ -42,10 +45,9 @@
 #include <lib/support/CHIPFaultInjection.h>
 #include <lib/support/CodeUtils.h>
 #include <lib/support/FibonacciUtils.h>
+#include <protocols/interaction_model/StatusCode.h>
 
 #if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
-#include <app/data-model-provider/ActionReturnStatus.h>
-
 // TODO: defaulting to codegen should eventually be an application choice and not
 //       hard-coded in the interaction model
 #include <app/codegen-data-model-provider/Instance.h>
@@ -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<DataModel::ActionReturnStatus> 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<DataModel::CommandInfo> 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<DataModel::CommandInfo> 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
@@ -49,6 +49,7 @@
 #include <app/TimedHandler.h>
 #include <app/WriteClient.h>
 #include <app/WriteHandler.h>
+#include <app/data-model-provider/OperationTypes.h>
 #include <app/data-model-provider/Provider.h>
 #include <app/icd/server/ICDServerConfig.h>
 #include <app/reporting/Engine.h>
@@ -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<FabricIndex> & aFabricIndex = NullOptional,
                                        const Optional<NodeId> & 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
@@ -19,6 +19,7 @@
 #include <access/SubjectDescriptor.h>
 #include <app/ConcreteAttributePath.h>
 #include <app/ConcreteCommandPath.h>
+#include <lib/core/DataModelTypes.h>
 #include <lib/support/BitFlags.h>
 
 #include <cstdint>
@@ -55,6 +56,15 @@ struct OperationRequest
     ///
     /// NOTE: once kInternal flag is removed, this will become non-optional
     std::optional<chip::Access::SubjectDescriptor> 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