Fix checking for required timed invoke for commands.

We weren't checking it for commands implemented via CommandHandlerInterface.

Fixes #12437

src/app/CommandHandler.cpp

@@ -28,6 +28,7 @@
 #include "messaging/ExchangeContext.h"
 
 #include <access/AccessControl.h>
+#include <app-common/zap-generated/cluster-objects.h>
 #include <app/RequiredPrivilege.h>
 #include <app/util/MatterCallbacks.h>
 #include <credentials/GroupDataProvider.h>
@@ -257,7 +258,7 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand
  VerifyOrExit(mpExchangeCtx != nullptr && mpExchangeCtx->HasSessionHandle(), err = CHIP_ERROR_INCORRECT_STATE);
 
  {
- Access::SubjectDescriptor subjectDescriptor = mpExchangeCtx->GetSessionHandle()->GetSubjectDescriptor();
+ Access::SubjectDescriptor subjectDescriptor = GetSubjectDescriptor();
  Access::RequestPath requestPath{ .cluster = concretePath.mClusterId, .endpoint = concretePath.mEndpointId };
  Access::Privilege requestPrivilege = RequiredPrivilege::ForInvokeCommand(concretePath);
  err = Access::GetAccessControl().Check(subjectDescriptor, requestPath, requestPrivilege);
@@ -273,11 +274,18 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand
  {
  return AddStatus(concretePath, Protocols::InteractionModel::Status::Failure);
  }
- // TODO: when wildcard/group invokes are supported, handle them to discard rather than fail with status
+ // TODO: when wildcard invokes are supported, handle them to discard rather than fail with status
  return AddStatus(concretePath, Protocols::InteractionModel::Status::UnsupportedAccess);
  }
  }
 
+ if (CommandNeedsTimedInvoke(concretePath.mClusterId, concretePath.mCommandId) && !IsTimedInvoke())
+ {
+ // TODO: when wildcard invokes are supported, discard a
+ // wildcard-expanded path instead of returning a status.
+ return AddStatus(concretePath, Protocols::InteractionModel::Status::NeedsTimedInteraction);
+ }
+
  err = aCommandElement.GetData(&commandDataReader);
  if (CHIP_END_OF_TLV == err)
  {
@@ -359,6 +367,17 @@ CHIP_ERROR CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo
  }
  SuccessOrExit(err);
 
+ // Per spec, we do the "is this a timed command?" check for every path, but
+ // since all paths that fail it just get silently discarded we can do it
+ // 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 (CommandNeedsTimedInvoke(clusterId, commandId))
+ {
+ // Group commands are never timed.
+ ExitNow();
+ }
+
  iterator = groupDataProvider->IterateEndpoints(fabric);
  VerifyOrExit(iterator != nullptr, err = CHIP_ERROR_NO_MEMORY);
 
@@ -384,7 +403,7 @@ CHIP_ERROR CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo
  }
 
  {
- Access::SubjectDescriptor subjectDescriptor = mpExchangeCtx->GetSessionHandle()->GetSubjectDescriptor();
+ Access::SubjectDescriptor subjectDescriptor = GetSubjectDescriptor();
  Access::RequestPath requestPath{ .cluster = concretePath.mClusterId, .endpoint = concretePath.mEndpointId };
  Access::Privilege requestPrivilege = RequiredPrivilege::ForInvokeCommand(concretePath);
  err = Access::GetAccessControl().Check(subjectDescriptor, requestPath, requestPrivilege);

src/app/InteractionModelEngine.cpp

@@ -581,9 +581,6 @@ void InteractionModelEngine::DispatchCommand(CommandHandler & apCommandObj, cons
 
  if (handler)
  {
- // TODO: Figure out who is responsible for handling checking
- // apCommandObj->IsTimedInvoke() for commands that require a timed
- // invoke and have a CommandHandlerInterface handling them.
  CommandHandlerInterface::HandlerContext context(apCommandObj, aCommandPath, apPayload);
  handler->InvokeCommand(context);
 

src/app/zap-templates/partials/im_command_handler_cluster_commands.zapt

@@ -1,10 +1,4 @@
 {{#if (isServer parent.clusterSide)}}
-{{#if mustUseTimedInvoke}}
-if (!apCommandObj->IsTimedInvoke()) {
- apCommandObj->AddStatus(aCommandPath, Protocols::InteractionModel::Status::NeedsTimedInteraction);
- return;
-}
-{{/if}}
 Commands::{{asUpperCamelCase commandName}}::DecodableType commandData;
 TLVError = DataModel::Decode(aDataTlv, commandData);
 if (TLVError == CHIP_NO_ERROR) {

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

@@ -86,7 +86,7 @@ CHIP_ERROR TypeInfo::DecodableType::Decode(TLV::TLVReader &reader, const Concret
 
  return CHIP_NO_ERROR;
 }
-}
+} // namespace Attributes
 
 namespace Events {
 {{#zcl_events}}
@@ -126,11 +126,42 @@ CHIP_ERROR DecodableType::Decode(TLV::TLVReader &reader) {
 }
 } // namespace {{asUpperCamelCase name}}.
 {{/zcl_events}}
-} // namespace Commands
+} // namespace Events
 
 } // namespace {{asUpperCamelCase name}}
 {{/zcl_clusters}}
 
 } // namespace Clusters
+
+bool CommandNeedsTimedInvoke(ClusterId aCluster, CommandId aCommand)
+{
+ // Maybe it would be smaller code to codegen a table and walk over it?
+ // Not sure.
+ switch (aCluster)
+ {
+ {{#zcl_clusters}}
+ {{#zcl_commands_that_need_timed_invoke}}
+ {{#first}}
+ case Clusters::{{asUpperCamelCase parent.name}}::Id:
+ {
+ switch (aCommand) {
+ {{/first}}
+ case Clusters::{{asUpperCamelCase parent.name}}::Commands::{{asUpperCamelCase name}}::Id:
+ {{#last}}
+ return true;
+ default:
+ return false;
+ }
+ }
+ {{/last}}
+ {{/zcl_commands_that_need_timed_invoke}}
+ {{/zcl_clusters}}
+ default:
+ break;
+ }
+
+ return false;
+}
+
 } // namespace app
 } // namespace chip

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

@@ -7,6 +7,7 @@
 #include <app/data-model/Encode.h>
 #include <app/data-model/List.h>
 #include <app/data-model/NullObject.h>
+#include <app/ConcreteAttributePath.h>
 #include <app/EventLoggingTypes.h>
 #include <app/util/basic-types.h>
 #include <lib/support/BitFlags.h>
@@ -244,5 +245,8 @@ public:
 {{/zcl_clusters}}
 
 } // namespace Clusters
+
+bool CommandNeedsTimedInvoke(ClusterId aCluster, CommandId aCommand);
+
 } // namespace app
 } // namespace chip

src/app/zap-templates/templates/app/helper.js

@@ -822,6 +822,16 @@ async function zcl_events_fields_by_event_name(name, options)
  return templateUtil.templatePromise(this.global, promise)
 }
 
+// Must be used inside zcl_clusters
+async function zcl_commands_that_need_timed_invoke(options)
+{
+ const { db } = this.global;
+ let packageId = await templateUtil.ensureZclPackageId(this);
+ let commands = await queryCommand.selectCommandsByClusterId(db, this.id, packageId);
+ commands = commands.filter(cmd => cmd.mustUseTimedInvoke);
+ return templateUtil.collectBlocks(commands, options, this);
+}
+
 //
 // Module exports
 //
@@ -845,3 +855,4 @@ exports.isWeaklyTypedEnum = isWeaklyTypedEnum;
 exports.getPythonFieldDefault = getPythonFieldDefault;
 exports.incrementDepth = incrementDepth;
 exports.zcl_events_fields_by_event_name = zcl_events_fields_by_event_name;
+exports.zcl_commands_that_need_timed_invoke = zcl_commands_that_need_timed_invoke;