Validate for a valid cluster path when Invoke is called (project-chip…

…#37207) * Fix invoke verification of data existence of cluster and endpoint * Add unit test for codegen logic issue * Add integration test for return codes * Restule * Remove odd comment * Remove unused import * Use asserts fail * Add unit test that checks invalid cluster return code as well * Fix comment * Code review feedback: listing of commands should fail if the cluster does not exist * Update src/data-model-providers/codegen/CodegenDataModelProvider.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Update src/data-model-providers/codegen/CodegenDataModelProvider.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Update src/data-model-providers/codegen/CodegenDataModelProvider.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Updated comment * Try to reduce amount of code overhead for extra validation check * Update logic yet again since just checking for null fails unit tests and having unit tests seems reasonable * Restyle * Update src/data-model-providers/codegen/EmberMetadata.h Co-authored-by: Boris Zbarsky <[email protected]> * Update src/data-model-providers/codegen/EmberMetadata.h Co-authored-by: Boris Zbarsky <[email protected]> * Update src/data-model-providers/codegen/CodegenDataModelProvider.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Update comment * Update comment * Update based on code review ... if we cannot enforce an interface, we document. Not happy and hoping we can improve in the future --------- Co-authored-by: Boris Zbarsky <[email protected]> Co-authored-by: Andrei Litvin <[email protected]>
espressif · Jan 31, 2025 · c56eb2b · c56eb2b
1 parent 26341b4
commit c56eb2b
Show file tree

Hide file tree

Showing 6 changed files with 218 additions and 13 deletions.
diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h
@@ -85,6 +85,16 @@ class Provider : public ProviderMetadataTree
     ///      This includes cases where command handling and value return will be done asynchronously.
     ///    - returning a value other than Success implies an error reply (error and data are mutually exclusive)
     ///
+    /// Preconditions:
+    ///    - `request.path` MUST be valid: Invoke` is only guaranteed to function correctly for
+    ///      VALID paths (i.e. use `ProviderMetadataTree::AcceptedCommands` to check). This is
+    ///      because we assume ACL or flags (like timed invoke) have to happen before invoking
+    ///      this command.
+    ///    - TODO: as interfaces are updated, we may want to make the above requirement more
+    ///            relaxed, as it seems desirable for users of this interface to have guaranteed
+    ///            behavior (like error on invalid paths) where as today this seems unclear as some
+    ///            command intercepts do not validate if the path is valid per endpoints.
+    ///
     /// Return value expectations:
     ///   - if a response has been placed into `handler` then std::nullopt MUST be returned. In particular
     ///     note that CHIP_NO_ERROR is NOT the same as std::nullopt:

diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp
@@ -14,6 +14,7 @@
  *    See the License for the specific language governing permissions and
  *    limitations under the License.
  */
+#include "data-model-providers/codegen/EmberMetadata.h"
 #include <data-model-providers/codegen/CodegenDataModelProvider.h>
 
 #include <access/AccessControl.h>
@@ -324,6 +325,12 @@ const EmberAfCluster * CodegenDataModelProvider::FindServerCluster(const Concret
 CHIP_ERROR CodegenDataModelProvider::AcceptedCommands(const ConcreteClusterPath & path,
                                                       DataModel::ListBuilder<DataModel::AcceptedCommandEntry> & builder)
 {
+    // Some CommandHandlerInterface instances are registered of ALL endpoints, so make sure first that
+    // the cluster actually exists on this endpoint before asking the CommandHandlerInterface whether it
+    // supports the command.
+    const EmberAfCluster * serverCluster = FindServerCluster(path);
+    VerifyOrReturnError(serverCluster != nullptr, CHIP_ERROR_NOT_FOUND);
+
     CommandHandlerInterface * interface =
         CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(path.mEndpointId, path.mClusterId);
     if (interface != nullptr)
@@ -377,8 +384,6 @@ CHIP_ERROR CodegenDataModelProvider::AcceptedCommands(const ConcreteClusterPath
         VerifyOrReturnError(err == CHIP_ERROR_NOT_IMPLEMENTED, err);
     }
 
-    const EmberAfCluster * serverCluster = FindServerCluster(path);
-    VerifyOrReturnError(serverCluster != nullptr, CHIP_ERROR_NOT_FOUND);
     VerifyOrReturnError(serverCluster->acceptedCommandList != nullptr, CHIP_NO_ERROR);
 
     const chip::CommandId * endOfList = serverCluster->acceptedCommandList;
@@ -404,6 +409,12 @@ CHIP_ERROR CodegenDataModelProvider::AcceptedCommands(const ConcreteClusterPath
 CHIP_ERROR CodegenDataModelProvider::GeneratedCommands(const ConcreteClusterPath & path,
                                                        DataModel::ListBuilder<CommandId> & builder)
 {
+    // Some CommandHandlerInterface instances are registered of ALL endpoints, so make sure first that
+    // the cluster actually exists on this endpoint before asking the CommandHandlerInterface whether it
+    // supports the command.
+    const EmberAfCluster * serverCluster = FindServerCluster(path);
+    VerifyOrReturnError(serverCluster != nullptr, CHIP_ERROR_NOT_FOUND);
+
     CommandHandlerInterface * interface =
         CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(path.mEndpointId, path.mClusterId);
     if (interface != nullptr)
@@ -454,8 +465,6 @@ CHIP_ERROR CodegenDataModelProvider::GeneratedCommands(const ConcreteClusterPath
         VerifyOrReturnError(err == CHIP_ERROR_NOT_IMPLEMENTED, err);
     }
 
-    const EmberAfCluster * serverCluster = FindServerCluster(path);
-    VerifyOrReturnError(serverCluster != nullptr, CHIP_ERROR_NOT_FOUND);
     VerifyOrReturnError(serverCluster->generatedCommandList != nullptr, CHIP_NO_ERROR);
 
     const chip::CommandId * endOfList = serverCluster->generatedCommandList;

diff --git a/src/data-model-providers/codegen/EmberMetadata.cpp b/src/data-model-providers/codegen/EmberMetadata.cpp
@@ -56,16 +56,11 @@ FindAttributeMetadata(const ConcreteAttributePath & aPath)
 
     if (metadata == nullptr)
     {
-        const EmberAfEndpointType * type = emberAfFindEndpointType(aPath.mEndpointId);
-        if (type == nullptr)
-        {
-            return Status::UnsupportedEndpoint;
-        }
+        Status status = ValidateClusterPath(aPath);
 
-        const EmberAfCluster * cluster = emberAfFindClusterInType(type, aPath.mClusterId, CLUSTER_MASK_SERVER);
-        if (cluster == nullptr)
+        if (status != Status::Success)
         {
-            return Status::UnsupportedCluster;
+            return status;
         }
 
         // Since we know the attribute is unsupported and the endpoint/cluster are
@@ -76,6 +71,24 @@ FindAttributeMetadata(const ConcreteAttributePath & aPath)
     return metadata;
 }
 
+Status ValidateClusterPath(const ConcreteClusterPath & path)
+{
+
+    const EmberAfEndpointType * type = emberAfFindEndpointType(path.mEndpointId);
+    if (type == nullptr)
+    {
+        return Status::UnsupportedEndpoint;
+    }
+
+    const EmberAfCluster * cluster = emberAfFindClusterInType(type, path.mClusterId, CLUSTER_MASK_SERVER);
+    if (cluster == nullptr)
+    {
+        return Status::UnsupportedCluster;
+    }
+
+    return Status::Success;
+}
+
 } // namespace Ember
 } // namespace app
 } // namespace chip
diff --git a/src/data-model-providers/codegen/EmberMetadata.h b/src/data-model-providers/codegen/EmberMetadata.h
@@ -16,6 +16,7 @@
  */
 #pragma once
 
+#include "app/ConcreteClusterPath.h"
 #include <app/util/af-types.h>
 #include <lib/core/CHIPError.h>
 #include <protocols/interaction_model/StatusCode.h>
@@ -42,6 +43,14 @@ std::variant<const EmberAfCluster *,             // global attribute, data from
              >
 FindAttributeMetadata(const ConcreteAttributePath & aPath);
 
+/// Returns the status for a given cluster existing in the ember metadata.
+///
+/// Return code will be one of:
+///    - Status::UnsupportedEndpoint if the path endpoint does not exist
+///    - Status::UnsupportedCluster if the cluster does not exist on the given endpoint
+///    - Status::Success if the cluster exists in the ember metadata.
+Protocols::InteractionModel::Status ValidateClusterPath(const ConcreteClusterPath & path);
+
 } // namespace Ember
 } // namespace app
 } // namespace chip
diff --git a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp
@@ -218,6 +218,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
@@ -229,7 +267,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
     {
@@ -268,6 +319,7 @@ class CustomListCommandHandler : public CommandHandlerInterface
 private:
     bool mOverrideAccepted  = false;
     bool mOverrideGenerated = false;
+    bool mHandleCommand     = false;
 
     std::vector<CommandId> mAccepted;
     std::vector<CommandId> mGenerated;
@@ -1156,6 +1208,37 @@ TEST_F(TestCodegenModelViaMocks, IterateOverGeneratedCommands)
     ASSERT_TRUE(cmds.data_equal(Span<const CommandId>(expectedCommands3)));
 }
 
+TEST_F(TestCodegenModelViaMocks, AcceptedGeneratedCommandsOnInvalidEndpoints)
+{
+    UseMockNodeConfig config(gTestNodeConfig);
+    CodegenDataModelProviderWithContext model;
+
+    // register a CHI on ALL endpoints
+    CustomListCommandHandler handler(chip::NullOptional, MockClusterId(1));
+    handler.SetHandleCommands(true);
+
+    DataModel::ListBuilder<CommandId> generatedBuilder;
+    DataModel::ListBuilder<DataModel::AcceptedCommandEntry> acceptedBuilder;
+
+    // valid endpoint will result in valid data (even though list is empty)
+    ASSERT_EQ(model.GeneratedCommands(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1)), generatedBuilder), CHIP_NO_ERROR);
+    ASSERT_TRUE(generatedBuilder.IsEmpty());
+    ASSERT_EQ(model.AcceptedCommands(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1)), acceptedBuilder), CHIP_NO_ERROR);
+    ASSERT_TRUE(acceptedBuilder.IsEmpty());
+
+    // Invalid endpoint fails - we will get no commands there (even though CHI is registered)
+    ASSERT_EQ(model.GeneratedCommands(ConcreteClusterPath(kEndpointIdThatIsMissing, MockClusterId(1)), generatedBuilder),
+              CHIP_ERROR_NOT_FOUND);
+    ASSERT_EQ(model.AcceptedCommands(ConcreteClusterPath(kEndpointIdThatIsMissing, MockClusterId(1)), acceptedBuilder),
+              CHIP_ERROR_NOT_FOUND);
+
+    // same for invalid cluster ID
+    ASSERT_EQ(model.GeneratedCommands(ConcreteClusterPath(kMockEndpoint1, MockClusterId(0x1123)), generatedBuilder),
+              CHIP_ERROR_NOT_FOUND);
+    ASSERT_EQ(model.AcceptedCommands(ConcreteClusterPath(kMockEndpoint1, MockClusterId(0x1123)), acceptedBuilder),
+              CHIP_ERROR_NOT_FOUND);
+}
+
 TEST_F(TestCodegenModelViaMocks, CommandHandlerInterfaceCommandHandling)
 {
 

diff --git a/src/python_testing/TestInvokeReturnCodes.py b/src/python_testing/TestInvokeReturnCodes.py
@@ -0,0 +1,81 @@
+#
+#    Copyright (c) 2025 Project CHIP Authors
+#    All rights reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License");
+#    you may not use this file except in compliance with the License.
+#    You may obtain a copy of the License at
+#
+#        http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS,
+#    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#    See the License for the specific language governing permissions and
+#    limitations under the License.
+#
+
+# === BEGIN CI TEST ARGUMENTS ===
+# test-runner-runs:
+#   run1:
+#     app: ${ALL_CLUSTERS_APP}
+#     app-args: >
+#       --discriminator 1234
+#       --KVS kvs1
+#       --trace-to json:${TRACE_APP}.json
+#     script-args: >
+#       --storage-path admin_storage.json
+#       --commissioning-method on-network
+#       --discriminator 1234
+#       --passcode 20202021
+#       --trace-to json:${TRACE_TEST_JSON}.json
+#       --trace-to perfetto:${TRACE_TEST_PERFETTO}.perfetto
+#     factory-reset: true
+#     quiet: true
+# === END CI TEST ARGUMENTS ===
+
+import chip.clusters as Clusters
+from chip.interaction_model import InteractionModelError, Status
+from chip.testing.matter_testing import MatterBaseTest, async_test_body, default_matter_test_main
+from mobly import asserts
+
+
+class TestInvokeReturnCodes(MatterBaseTest):
+    """
+    Validates that the invoke action correctly refuses commands
+    on invalid endpoints.
+    """
+
+    @async_test_body
+    async def test_invalid_endpoint_command(self):
+        self.print_step(0, "Commissioning - already done")
+
+        self.print_step(1, "Find an invalid endpoint id")
+        root_parts = await self.read_single_attribute_check_success(
+            cluster=Clusters.Descriptor,
+            attribute=Clusters.Descriptor.Attributes.PartsList,
+            endpoint=0,
+        )
+        endpoints = set(root_parts)
+        invalid_endpoint_id = 1
+        while invalid_endpoint_id in endpoints:
+            invalid_endpoint_id += 1
+
+        self.print_step(
+            2,
+            "Attempt to invoke SoftwareDiagnostics::ResetWatermarks on an invalid endpoint",
+        )
+        try:
+            await self.send_single_cmd(
+                cmd=Clusters.SoftwareDiagnostics.Commands.ResetWatermarks(),
+                endpoint=invalid_endpoint_id,
+            )
+            asserts.fail("Unexpected command success on an invalid endpoint")
+        except InteractionModelError as e:
+            asserts.assert_equal(
+                e.status, Status.UnsupportedEndpoint, "Unexpected error returned"
+            )
+
+
+if __name__ == "__main__":
+    default_matter_test_main()