Use DataModel::Provider provided command information to handle comman…

…d 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 <bzbarsky@apple.com>

* Remove separate member for accessing fabric index

* Restyled by clang-format

---------

Co-authored-by: Andrei Litvin <andreilitvin@google.com>
Co-authored-by: Restyled.io <commits@restyled.io>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

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)

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",

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);

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

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()

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.

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();

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.
  */

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