[1.4 branch] Fix command handling logic for invalid endpoint commands.

src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp

@@ -31,6 +31,7 @@
 #include <app/util/endpoint-config-api.h>
 #include <lib/core/CHIPError.h>
 #include <lib/core/DataModelTypes.h>
+#include <protocols/interaction_model/StatusCode.h>
 
 // separated out for code-reuse
 #include <app/ember_coupling/EventPathValidity.mixin.h>
@@ -417,6 +418,17 @@ std::optional<DataModel::ActionReturnStatus> CodegenDataModelProvider::Invoke(co
                                                                               TLV::TLVReader & input_arguments,
                                                                               CommandHandler * handler)
 {
+    // As some CommandHandlerInterface commands are registered on wildcard interfaces,
+    // the valid endpoint/cluster check is done BEFORE attempting command handler interface handling
+    const EmberAfCluster * cluster = FindServerCluster(request.path);
+    if (cluster == nullptr)
+    {
+        // The path `endpoint/cluster` is not valid. Determine what failed: endpoint id or cluster id.
+        return TryFindEndpointIndex(request.path.mEndpointId).has_value()
+            ? Protocols::InteractionModel::Status::UnsupportedCluster
+            : Protocols::InteractionModel::Status::UnsupportedEndpoint;
+    }
+
     CommandHandlerInterface * handler_interface =
         CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(request.path.mEndpointId, request.path.mClusterId);
 
@@ -723,6 +735,13 @@ DataModel::CommandEntry CodegenDataModelProvider::NextAcceptedCommand(const Conc
 
 std::optional<DataModel::CommandInfo> CodegenDataModelProvider::GetAcceptedCommandInfo(const ConcreteCommandPath & path)
 {
+    // As some CommandHandlerInterface commands are registered on wildcard interfaces,
+    // the valid endpoint/cluster check is done BEFORE attemptiong command handler interface handling
+    const EmberAfCluster * cluster = FindServerCluster(path);
+
+    VerifyOrReturnValue(cluster != nullptr, std::nullopt);
+
+    // This is a valid endpoint/cluster, can use CommandHandlerInterface to check for it.
     auto handlerInterfaceValue = EnumeratorCommandFinder(&CommandHandlerInterface::EnumerateAcceptedCommands)
                                      .FindCommandEntry(EnumeratorCommandFinder::Operation::kFindExact, path);
 
@@ -731,11 +750,8 @@ std::optional<DataModel::CommandInfo> CodegenDataModelProvider::GetAcceptedComma
         return handlerInterfaceValue->IsValid() ? std::make_optional(handlerInterfaceValue->info) : std::nullopt;
     }
 
-    const EmberAfCluster * cluster = FindServerCluster(path);
-
-    VerifyOrReturnValue(cluster != nullptr, std::nullopt);
+    // CommandHandlerInterface did not handle it, so check within ember.
     VerifyOrReturnValue(mAcceptedCommandsIterator.Exists(cluster->acceptedCommandList, path.mCommandId), std::nullopt);
-
     return CommandEntryFrom(path, path.mCommandId).info;
 }
 

src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp

@@ -196,6 +196,44 @@ class MockAccessControl : public Access::AccessControl::Delegate, public Access:
     bool IsDeviceTypeOnEndpoint(DeviceTypeId deviceType, EndpointId endpoint) override { return true; }
 };
 
+class MockCommandHandler : public CommandHandler
+{
+public:
+    CHIP_ERROR FallibleAddStatus(const ConcreteCommandPath & aRequestCommandPath,
+                                 const Protocols::InteractionModel::ClusterStatusCode & aStatus,
+                                 const char * context = nullptr) override
+    {
+        // MOCK: do not do anything here
+        return CHIP_NO_ERROR;
+    }
+
+    void AddStatus(const ConcreteCommandPath & aRequestCommandPath, const Protocols::InteractionModel::ClusterStatusCode & aStatus,
+                   const char * context = nullptr) override
+    {
+        // MOCK: do not do anything here
+    }
+
+    FabricIndex GetAccessingFabricIndex() const override { return 1; }
+
+    CHIP_ERROR AddResponseData(const ConcreteCommandPath & aRequestCommandPath, CommandId aResponseCommandId,
+                               const DataModel::EncodableToTLV & aEncodable) override
+    {
+        return CHIP_NO_ERROR;
+    }
+
+    void AddResponse(const ConcreteCommandPath & aRequestCommandPath, CommandId aResponseCommandId,
+                     const DataModel::EncodableToTLV & aEncodable) override
+    {}
+
+    bool IsTimedInvoke() const override { return false; }
+
+    void FlushAcksRightAwayOnSlowCommand() override {}
+
+    Access::SubjectDescriptor GetSubjectDescriptor() const override { return kAdminSubjectDescriptor; }
+
+    Messaging::ExchangeContext * GetExchangeContext() const override { return nullptr; }
+};
+
 /// Overrides Enumerate*Commands in the CommandHandlerInterface to allow
 /// testing of behaviors when command enumeration is done in the interace.
 class CustomListCommandHandler : public CommandHandlerInterface
@@ -207,7 +245,20 @@ class CustomListCommandHandler : public CommandHandlerInterface
     }
     ~CustomListCommandHandler() { CommandHandlerInterfaceRegistry::Instance().UnregisterCommandHandler(this); }
 
-    void InvokeCommand(HandlerContext & handlerContext) override { handlerContext.SetCommandNotHandled(); }
+    void InvokeCommand(HandlerContext & handlerContext) override
+    {
+        if (mHandleCommand)
+        {
+            handlerContext.mCommandHandler.AddStatus(handlerContext.mRequestPath, Protocols::InteractionModel::Status::Success);
+            handlerContext.SetCommandHandled();
+        }
+        else
+        {
+            handlerContext.SetCommandNotHandled();
+        }
+    }
+
+    void SetHandleCommands(bool handle) { mHandleCommand = handle; }
 
     CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) override
     {
@@ -246,6 +297,7 @@ class CustomListCommandHandler : public CommandHandlerInterface
 private:
     bool mOverrideAccepted  = false;
     bool mOverrideGenerated = false;
+    bool mHandleCommand     = false;
 
     std::vector<CommandId> mAccepted;
     std::vector<CommandId> mGenerated;
@@ -1267,6 +1319,71 @@ TEST(TestCodegenModelViaMocks, IterateOverGeneratedCommands)
     }
 }
 
+TEST(TestCodegenModelViaMocks, AcceptedCommandValidity)
+{
+    UseMockNodeConfig config(gTestNodeConfig);
+    CodegenDataModelProviderWithContext model;
+
+    // register a CHI on ALL endpoints
+    CustomListCommandHandler handler(chip::NullOptional, MockClusterId(1));
+    handler.SetHandleCommands(true);
+
+    handler.SetOverrideAccepted(true);
+    handler.AcceptedVec().push_back(1234);
+
+    // Command succeeds on a valid endpoint
+    EXPECT_TRUE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 1234)).has_value());
+
+    // but not if the command is invalid
+    EXPECT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 0x1122)).has_value());
+
+    // Fails on an invalid endpoint (even if the handler is on wildcard endpoint)
+    EXPECT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kEndpointIdThatIsMissing, MockClusterId(1), 1234)).has_value());
+    EXPECT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kEndpointIdThatIsMissing, MockClusterId(1), 0x1122)).has_value());
+}
+
+TEST(TestCodegenModelViaMocks, CommandHandlerInterfaceValidity)
+{
+    UseMockNodeConfig config(gTestNodeConfig);
+    CodegenDataModelProviderWithContext model;
+
+    // register a CHI on ALL endpoints
+    CustomListCommandHandler handler(chip::NullOptional, MockClusterId(1));
+    handler.SetHandleCommands(true);
+
+    MockCommandHandler commandHandler;
+
+    // Command succeeds on a valid endpoint
+    {
+        const ConcreteCommandPath kCommandPath(kMockEndpoint1, MockClusterId(1), kMockCommandId1);
+        const InvokeRequest kInvokeRequest{ .path = kCommandPath };
+        chip::TLV::TLVReader tlvReader;
+
+        // std::nullopt is returned when the command is handled
+        ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ &commandHandler), std::nullopt);
+    }
+
+    // Command fails on an invalid endpoint
+    {
+        const ConcreteCommandPath kCommandPath(kEndpointIdThatIsMissing, MockClusterId(1), kMockCommandId1);
+        const InvokeRequest kInvokeRequest{ .path = kCommandPath };
+        chip::TLV::TLVReader tlvReader;
+
+        ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ &commandHandler),
+                  Protocols::InteractionModel::Status::UnsupportedEndpoint);
+    }
+
+    // Command fails on an invalid cluster
+    {
+        const ConcreteCommandPath kCommandPath(kMockEndpoint1, MockClusterId(0x1123), kMockCommandId1);
+        const InvokeRequest kInvokeRequest{ .path = kCommandPath };
+        chip::TLV::TLVReader tlvReader;
+
+        ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ &commandHandler),
+                  Protocols::InteractionModel::Status::UnsupportedCluster);
+    }
+}
+
 TEST(TestCodegenModelViaMocks, CommandHandlerInterfaceAcceptedCommands)
 {