diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h index 13d5a4a4f55e7c..530911b484fcc0 100644 --- 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 index a3889fb1e25ebb..74fc5b769189a5 100644 --- 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 #include @@ -324,6 +325,12 @@ const EmberAfCluster * CodegenDataModelProvider::FindServerCluster(const Concret CHIP_ERROR CodegenDataModelProvider::AcceptedCommands(const ConcreteClusterPath & path, DataModel::ListBuilder & 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 & 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 index 22e31a569fe702..54842ee6f25882 100644 --- 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 index 64c0bfe736a25f..06e1568be950c7 100644 --- 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 #include #include @@ -42,6 +43,14 @@ std::variant 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 index 1045114b7f9b4c..cf156a786d0d53 100644 --- 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 mAccepted; std::vector mGenerated; @@ -1156,6 +1208,37 @@ TEST_F(TestCodegenModelViaMocks, IterateOverGeneratedCommands) ASSERT_TRUE(cmds.data_equal(Span(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 generatedBuilder; + DataModel::ListBuilder 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 new file mode 100644 index 00000000000000..cd01f8882338c4 --- /dev/null +++ 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()