Move `::WriteAttribute` validation inside Interaction Model Write Handling (#37322)
* Move write validity inside the write handler rather than requesting the datamodel provider to perform the checks
* Restyle
* A bit of code cleanup and reuse, to minimize flash impact
* Restyle
* Drop the mPreviousSuccessPath, making objects smaller and saving yet another small amount of flash
* Cleanup some includes
* Restyle
* Remove obsolete tests from codgen, as we do not do more validation now
* Test write interaction passes
* Test write passes
* Slight clarity of code update
* Fix comment
* Manual check for last written saves 16 bytes of flash on QPG.
* Status success is 2 bytes smaller in code generation than CHIP_NO_ERROR
* Not using endpoint finder saves flash
* Clean up code. We now save flash
* More flash savings by reusing the server cluster finder
* Update src/app/data-model-provider/MetadataLookup.cpp
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* Update src/app/data-model-provider/MetadataLookup.h
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* Update src/app/data-model-provider/Provider.h
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* Update src/app/data-model-provider/MetadataLookup.h
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* Updated comment
* Hoist "writing" log up in processing, to preserve similar functionality
* Code clarity updates
* Fix includes and update the code AGAIN because it does seem we use more flash
* Fix unsupported write on global attributes
* Fix includes
* Update src/app/WriteHandler.cpp
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* Post merge cleanup
* Fix merge error
---------
Co-authored-by: Andrei Litvin <andreilitvin@google.com>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
index da21906..6a96afc 100644
--- a/src/app/InteractionModelEngine.cpp
+++ b/src/app/InteractionModelEngine.cpp
@@ -1801,28 +1801,8 @@
}
}
- {
- DataModel::ServerClusterFinder finder(provider);
- if (finder.Find(aCommandPath).has_value())
- {
- // cluster exists, so command is invalid
- return Protocols::InteractionModel::Status::UnsupportedCommand;
- }
- }
-
- // At this point either cluster or endpoint does not exist. If we find the endpoint, then the cluster
- // is invalid
- {
- DataModel::EndpointFinder finder(provider);
- if (finder.Find(aCommandPath.mEndpointId))
- {
- // endpoint exists, so cluster is invalid
- return Protocols::InteractionModel::Status::UnsupportedCluster;
- }
- }
-
- // endpoint not found
- return Protocols::InteractionModel::Status::UnsupportedEndpoint;
+ // invalid command, return the right failure status
+ return DataModel::ValidateClusterPath(provider, aCommandPath, Protocols::InteractionModel::Status::UnsupportedCommand);
}
DataModel::Provider * InteractionModelEngine::SetDataModelProvider(DataModel::Provider * model)
diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp
index 9847641..09a9ec0 100644
--- a/src/app/WriteHandler.cpp
+++ b/src/app/WriteHandler.cpp
@@ -19,11 +19,15 @@
#include <app/AppConfig.h>
#include <app/AttributeAccessInterfaceRegistry.h>
#include <app/AttributeValueDecoder.h>
+#include <app/ConcreteAttributePath.h>
+#include <app/GlobalAttributes.h>
#include <app/InteractionModelEngine.h>
#include <app/MessageDef/EventPathIB.h>
#include <app/MessageDef/StatusIB.h>
+#include <app/RequiredPrivilege.h>
#include <app/StatusResponse.h>
#include <app/WriteHandler.h>
+#include <app/data-model-provider/ActionReturnStatus.h>
#include <app/data-model-provider/MetadataLookup.h>
#include <app/data-model-provider/MetadataTypes.h>
#include <app/data-model-provider/OperationTypes.h>
@@ -44,6 +48,8 @@
namespace {
+using Protocols::InteractionModel::Status;
+
/// Wraps a EndpointIterator and ensures that `::Release()` is called
/// for the iterator (assuming it is non-null)
class AutoReleaseGroupEndpointIterator
@@ -754,6 +760,75 @@
ChipLogDetail(DataManagement, "IM WH moving to [%s]", GetStateStr());
}
+DataModel::ActionReturnStatus WriteHandler::CheckWriteAllowed(const Access::SubjectDescriptor & aSubject,
+ const ConcreteAttributePath & aPath)
+{
+ // TODO: ordering is to check writability/existence BEFORE ACL and this seems wrong, however
+ // existing unit tests (TC_AcessChecker.py) validate that we get UnsupportedWrite instead of UnsupportedAccess
+ //
+ // This should likely be fixed in spec (probably already fixed by
+ // https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/9024)
+ // and tests and implementation
+ //
+ // Open issue that needs fixing: https://github.com/project-chip/connectedhomeip/issues/33735
+
+ std::optional<DataModel::AttributeEntry> attributeEntry;
+ DataModel::AttributeFinder finder(mDataModelProvider);
+
+ attributeEntry = finder.Find(aPath);
+
+ // if path is not valid, return a spec-compliant return code.
+ if (!attributeEntry.has_value())
+ {
+ // Global lists are not in metadata and not writable. Return the correct error code according to the spec
+ Status attributeErrorStatus =
+ IsSupportedGlobalAttributeNotInMetadata(aPath.mAttributeId) ? Status::UnsupportedWrite : Status::UnsupportedAttribute;
+
+ return DataModel::ValidateClusterPath(mDataModelProvider, aPath, attributeErrorStatus);
+ }
+
+ // Allow writes on writable attributes only
+ VerifyOrReturnValue(attributeEntry->writePrivilege.has_value(), Status::UnsupportedWrite);
+
+ bool checkAcl = true;
+ if (mLastSuccessfullyWrittenPath.has_value())
+ {
+ // only validate ACL if path has changed
+ //
+ // Note that this is NOT operator==: we could do `checkAcl == (aPath != *mLastSuccessfullyWrittenPath)`
+ // however that seems to use more flash.
+ if ((aPath.mEndpointId == mLastSuccessfullyWrittenPath->mEndpointId) &&
+ (aPath.mClusterId == mLastSuccessfullyWrittenPath->mClusterId) &&
+ (aPath.mAttributeId == mLastSuccessfullyWrittenPath->mAttributeId))
+ {
+ checkAcl = false;
+ }
+ }
+
+ if (checkAcl)
+ {
+ Access::RequestPath requestPath{ .cluster = aPath.mClusterId,
+ .endpoint = aPath.mEndpointId,
+ .requestType = Access::RequestType::kAttributeWriteRequest,
+ .entityId = aPath.mAttributeId };
+ CHIP_ERROR err = Access::GetAccessControl().Check(aSubject, requestPath, RequiredPrivilege::ForWriteAttribute(aPath));
+
+ if (err != CHIP_NO_ERROR)
+ {
+ VerifyOrReturnValue(err != CHIP_ERROR_ACCESS_DENIED, Status::UnsupportedAccess);
+ VerifyOrReturnValue(err != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL, Status::AccessRestricted);
+
+ return err;
+ }
+ }
+
+ // validate that timed write is enforced
+ VerifyOrReturnValue(IsTimedWrite() || !attributeEntry->flags.Has(DataModel::AttributeQualityFlags::kTimed),
+ Status::NeedsTimedInteraction);
+
+ return Status::Success;
+}
+
CHIP_ERROR WriteHandler::WriteClusterData(const Access::SubjectDescriptor & aSubject, const ConcreteDataAttributePath & aPath,
TLV::TLVReader & aData)
{
@@ -761,16 +836,21 @@
// the write is done via the DataModel interface
VerifyOrReturnError(mDataModelProvider != nullptr, CHIP_ERROR_INCORRECT_STATE);
- DataModel::WriteAttributeRequest request;
+ ChipLogDetail(DataManagement, "Writing attribute: Cluster=" ChipLogFormatMEI " Endpoint=0x%x AttributeId=" ChipLogFormatMEI,
+ ChipLogValueMEI(aPath.mClusterId), aPath.mEndpointId, ChipLogValueMEI(aPath.mAttributeId));
- request.path = aPath;
- request.subjectDescriptor = &aSubject;
- request.previousSuccessPath = mLastSuccessfullyWrittenPath;
- request.writeFlags.Set(DataModel::WriteFlags::kTimed, IsTimedWrite());
+ DataModel::ActionReturnStatus status = CheckWriteAllowed(aSubject, aPath);
+ if (status.IsSuccess())
+ {
+ DataModel::WriteAttributeRequest request;
- AttributeValueDecoder decoder(aData, aSubject);
+ request.path = aPath;
+ request.subjectDescriptor = &aSubject;
+ request.writeFlags.Set(DataModel::WriteFlags::kTimed, IsTimedWrite());
- DataModel::ActionReturnStatus status = mDataModelProvider->WriteAttribute(request, decoder);
+ AttributeValueDecoder decoder(aData, aSubject);
+ status = mDataModelProvider->WriteAttribute(request, decoder);
+ }
mLastSuccessfullyWrittenPath = status.IsSuccess() ? std::make_optional(aPath) : std::nullopt;
diff --git a/src/app/WriteHandler.h b/src/app/WriteHandler.h
index fa1d324..afb29b0 100644
--- a/src/app/WriteHandler.h
+++ b/src/app/WriteHandler.h
@@ -24,6 +24,7 @@
#include <app/ConcreteAttributePath.h>
#include <app/InteractionModelDelegatePointers.h>
#include <app/MessageDef/WriteResponseMessage.h>
+#include <app/data-model-provider/ActionReturnStatus.h>
#include <app/data-model-provider/Provider.h>
#include <lib/core/CHIPCore.h>
#include <lib/core/TLVDebug.h>
@@ -185,6 +186,14 @@
System::PacketBufferHandle && aPayload) override;
void OnResponseTimeout(Messaging::ExchangeContext * apExchangeContext) override;
+ /// Validate that a write is acceptable on the given path.
+ ///
+ /// Validates that ACL, writability and Timed interaction settings are ok.
+ ///
+ /// Returns a success status if all is ok, failure otherwise.
+ DataModel::ActionReturnStatus CheckWriteAllowed(const Access::SubjectDescriptor & aSubject,
+ const ConcreteAttributePath & aPath);
+
// Write the given data to the given path
CHIP_ERROR WriteClusterData(const Access::SubjectDescriptor & aSubject, const ConcreteDataAttributePath & aPath,
TLV::TLVReader & aData);
diff --git a/src/app/data-model-provider/MetadataLookup.cpp b/src/app/data-model-provider/MetadataLookup.cpp
index a249b7e..cfbdb91 100644
--- a/src/app/data-model-provider/MetadataLookup.cpp
+++ b/src/app/data-model-provider/MetadataLookup.cpp
@@ -21,6 +21,8 @@
namespace app {
namespace DataModel {
+using Protocols::InteractionModel::Status;
+
std::optional<ServerClusterEntry> ServerClusterFinder::Find(const ConcreteClusterPath & path)
{
VerifyOrReturnValue(mProvider != nullptr, std::nullopt);
@@ -63,25 +65,6 @@
return std::nullopt;
}
-EndpointFinder::EndpointFinder(ProviderMetadataTree * provider) : mProvider(provider)
-{
- VerifyOrReturn(mProvider != nullptr);
- mEndpoints = mProvider->EndpointsIgnoreError();
-}
-
-std::optional<EndpointEntry> EndpointFinder::Find(EndpointId endpointId)
-{
- for (auto & endpointEntry : mEndpoints)
- {
- if (endpointEntry.id == endpointId)
- {
- return endpointEntry;
- }
- }
-
- return std::nullopt;
-}
-
Protocols::InteractionModel::Status ValidateClusterPath(ProviderMetadataTree * provider, const ConcreteClusterPath & path,
Protocols::InteractionModel::Status successStatus)
{
@@ -90,6 +73,7 @@
return successStatus;
}
+ // If we get here, the cluster identified by the path does not exist.
auto endpoints = provider->EndpointsIgnoreError();
for (auto & endpointEntry : endpoints)
{
diff --git a/src/app/data-model-provider/MetadataLookup.h b/src/app/data-model-provider/MetadataLookup.h
index 7526bd3..29ded48 100644
--- a/src/app/data-model-provider/MetadataLookup.h
+++ b/src/app/data-model-provider/MetadataLookup.h
@@ -24,6 +24,7 @@
#include <app/data-model-provider/ProviderMetadataTree.h>
#include <lib/core/DataModelTypes.h>
#include <lib/support/CodeUtils.h>
+#include <protocols/interaction_model/StatusCode.h>
#include <optional>
@@ -65,20 +66,14 @@
ReadOnlyBuffer<AttributeEntry> mAttributes;
};
-/// Helps search for a specific server endpoint in the given
-/// metadata provider.
+/// Validates that the cluster identified by `path` exists within the given provider.
///
-/// Facilitates the operation of "find an endpoint with a given ID".
-class EndpointFinder
-{
-public:
- EndpointFinder(ProviderMetadataTree * provider);
- std::optional<EndpointEntry> Find(EndpointId endpointId);
-
-private:
- ProviderMetadataTree * mProvider;
- ReadOnlyBuffer<EndpointEntry> mEndpoints;
-};
+/// If the endpoint identified by the path does not exist, will return Status::UnsupportedEndpoint.
+/// If the endpoint exists but does not have the cluster identified by the path, will return Status::UnsupportedCluster.
+///
+/// otherwise, it will return successStatus.
+Protocols::InteractionModel::Status ValidateClusterPath(ProviderMetadataTree * provider, const ConcreteClusterPath & path,
+ Protocols::InteractionModel::Status successStatus);
/// Validates that the cluster identified by `path` exists within the given provider.
/// If the endpoint does not exist, will return Status::UnsupportedEndpoint.
diff --git a/src/app/data-model-provider/OperationTypes.h b/src/app/data-model-provider/OperationTypes.h
index 294bf26..35d1654 100644
--- a/src/app/data-model-provider/OperationTypes.h
+++ b/src/app/data-model-provider/OperationTypes.h
@@ -88,17 +88,6 @@
{
ConcreteDataAttributePath path; // NOTE: this also contains LIST operation options (i.e. "data" path type)
BitFlags<WriteFlags> writeFlags;
-
- // The path of the previous successful write in the same write transaction, if any.
- //
- // In particular this means that a write to this path has succeeded before (i.e. it passed required ACL checks).
- // The intent for this is to allow short-cutting ACL checks when ACL is in progress of being updated:
- // - During write chunking, list writes can be of the form "reset list" followed by "append item by item"
- // - When ACL is updating, a reset to empty would result in the entire ACL being deny and the "append"
- // would fail.
- // callers are expected to keep track of a `previousSuccessPath` whenever a write succeeds (otherwise ACL
- // checks may fail)
- std::optional<ConcreteAttributePath> previousSuccessPath;
};
enum class InvokeFlags : uint32_t
diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h
index 36a13d0..fc5a204 100644
--- a/src/app/data-model-provider/Provider.h
+++ b/src/app/data-model-provider/Provider.h
@@ -76,12 +76,7 @@
///
/// When this is invoked, caller is expected to have already done some validations:
/// - cluster `data version` has been checked for the incoming request if applicable
- ///
- /// TEMPORARY/TRANSITIONAL requirement for transitioning from ember-specific code
- /// WriteAttribute is REQUIRED to perform:
- /// - ACL validation (see notes on OperationFlags::kInternal)
- /// - Validation of readability/writability (also controlled by OperationFlags::kInternal)
- /// - Validation of timed interaction required (also controlled by OperationFlags::kInternal)
+ /// - validation of ACL/timed interaction flags/writability, if those checks are desired.
virtual ActionReturnStatus WriteAttribute(const WriteAttributeRequest & request, AttributeValueDecoder & decoder) = 0;
/// `handler` is used to send back the reply.
diff --git a/src/app/data-model-provider/tests/WriteTesting.h b/src/app/data-model-provider/tests/WriteTesting.h
index 7651ffe..2ab3988 100644
--- a/src/app/data-model-provider/tests/WriteTesting.h
+++ b/src/app/data-model-provider/tests/WriteTesting.h
@@ -60,12 +60,6 @@
return *this;
}
- WriteOperation & SetPreviousSuccessPath(std::optional<ConcreteAttributePath> path)
- {
- mRequest.previousSuccessPath = path;
- return *this;
- }
-
WriteOperation & SetDataVersion(Optional<DataVersion> version)
{
mRequest.path.mDataVersion = version;
diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp
index fbcd9d3..7b8654d 100644
--- a/src/app/reporting/Engine.cpp
+++ b/src/app/reporting/Engine.cpp
@@ -52,20 +52,6 @@
using Protocols::InteractionModel::Status;
-Status EventPathValid(DataModel::Provider * model, const ConcreteEventPath & eventPath)
-{
- {
- DataModel::ServerClusterFinder serverClusterFinder(model);
- if (serverClusterFinder.Find(eventPath).has_value())
- {
- return Status::Success;
- }
- }
-
- DataModel::EndpointFinder endpointFinder(model);
- return endpointFinder.Find(eventPath.mEndpointId).has_value() ? Status::UnsupportedCluster : Status::UnsupportedEndpoint;
-}
-
/// Returns the status of ACL validation.
/// If the return value has a status set, that means the ACL check failed,
/// the read must not be performed, and the returned status (which may
@@ -530,7 +516,9 @@
}
ConcreteEventPath path(current->mValue.mEndpointId, current->mValue.mClusterId, current->mValue.mEventId);
- Status status = EventPathValid(mpImEngine->GetDataModelProvider(), path);
+
+ // A event path is valid only if the cluster is valid
+ Status status = DataModel::ValidateClusterPath(mpImEngine->GetDataModelProvider(), path, Status::Success);
if (status != Status::Success)
{
diff --git a/src/app/tests/TestWriteInteraction.cpp b/src/app/tests/TestWriteInteraction.cpp
index e0560dd..dfe01ee 100644
--- a/src/app/tests/TestWriteInteraction.cpp
+++ b/src/app/tests/TestWriteInteraction.cpp
@@ -23,6 +23,7 @@
#include <app/reporting/tests/MockReportScheduler.h>
#include <app/tests/AppTestContext.h>
#include <app/tests/test-interaction-model-api.h>
+#include <app/util/mock/MockNodeConfig.h>
#include <credentials/GroupDataProviderImpl.h>
#include <crypto/CHIPCryptoPAL.h>
#include <crypto/DefaultSessionKeystore.h>
@@ -53,6 +54,74 @@
namespace chip {
namespace app {
+
+using namespace chip::Test;
+
+const MockNodeConfig & TestMockNodeConfig()
+{
+ using namespace Clusters::Globals::Attributes;
+
+ // clang-format off
+ static const MockNodeConfig config({
+ MockEndpointConfig(kRootEndpointId, {
+ MockClusterConfig(Clusters::IcdManagement::Id, {
+ ClusterRevision::Id, FeatureMap::Id,
+ Clusters::IcdManagement::Attributes::OperatingMode::Id,
+ }),
+ }),
+ MockEndpointConfig(kTestEndpointId, {
+ MockClusterConfig(Clusters::UnitTesting::Id, {
+ ClusterRevision::Id, FeatureMap::Id,
+ Clusters::UnitTesting::Attributes::Boolean::Id,
+ Clusters::UnitTesting::Attributes::Int16u::Id,
+ Clusters::UnitTesting::Attributes::ListFabricScoped::Id,
+ Clusters::UnitTesting::Attributes::ListStructOctetString::Id,
+ }),
+ }),
+ MockEndpointConfig(kMockEndpoint1, {
+ MockClusterConfig(MockClusterId(1), {
+ ClusterRevision::Id, FeatureMap::Id,
+ }, {
+ MockEventId(1), MockEventId(2),
+ }),
+ MockClusterConfig(MockClusterId(2), {
+ ClusterRevision::Id, FeatureMap::Id, MockAttributeId(1),
+ }),
+ }),
+ MockEndpointConfig(kMockEndpoint2, {
+ MockClusterConfig(MockClusterId(1), {
+ ClusterRevision::Id, FeatureMap::Id,
+ }),
+ MockClusterConfig(MockClusterId(2), {
+ ClusterRevision::Id, FeatureMap::Id, MockAttributeId(1), MockAttributeId(2),
+ }),
+ MockClusterConfig(MockClusterId(3), {
+ ClusterRevision::Id, FeatureMap::Id, MockAttributeId(1), MockAttributeId(2), MockAttributeId(3),
+ }),
+ }),
+ MockEndpointConfig(kMockEndpoint3, {
+ MockClusterConfig(MockClusterId(1), {
+ ClusterRevision::Id, FeatureMap::Id, MockAttributeId(1),
+ }),
+ MockClusterConfig(MockClusterId(2), {
+ ClusterRevision::Id, FeatureMap::Id, MockAttributeId(1), MockAttributeId(2), MockAttributeId(3), MockAttributeId(4),
+ }),
+ MockClusterConfig(MockClusterId(3), {
+ ClusterRevision::Id, FeatureMap::Id,
+ }),
+ MockClusterConfig(MockClusterId(4), {
+ ClusterRevision::Id, FeatureMap::Id,
+ }),
+ }),
+ /// For `TestWriteRoundtripWithClusterObjects` where path 2/3/4 is used
+ MockEndpointConfig(2, {
+ MockClusterConfig(3, {4}),
+ })
+ });
+ // clang-format on
+ return config;
+}
+
class TestWriteInteraction : public chip::Test::AppContext
{
public:
@@ -72,9 +141,11 @@
ASSERT_EQ(chip::GroupTesting::InitData(&gGroupsProvider, GetBobFabricIndex(), span), CHIP_NO_ERROR);
mOldProvider = InteractionModelEngine::GetInstance()->SetDataModelProvider(&TestImCustomDataModel::Instance());
+ chip::Test::SetMockNodeConfig(TestMockNodeConfig());
}
void TearDown() override
{
+ chip::Test::ResetMockNodeConfig();
InteractionModelEngine::GetInstance()->SetDataModelProvider(mOldProvider);
chip::Credentials::GroupDataProvider * provider = chip::Credentials::GetGroupDataProvider();
if (provider != nullptr)
diff --git a/src/controller/tests/data_model/TestWrite.cpp b/src/controller/tests/data_model/TestWrite.cpp
index 2188327..c50e7aa 100644
--- a/src/controller/tests/data_model/TestWrite.cpp
+++ b/src/controller/tests/data_model/TestWrite.cpp
@@ -21,8 +21,8 @@
#include "DataModelFixtures.h"
-#include "app-common/zap-generated/ids/Clusters.h"
#include <app-common/zap-generated/cluster-objects.h>
+#include <app-common/zap-generated/ids/Clusters.h>
#include <app/AttributeValueDecoder.h>
#include <app/InteractionModelEngine.h>
#include <app/WriteClient.h>
@@ -39,9 +39,72 @@
using namespace chip::app::Clusters::UnitTesting;
using namespace chip::app::DataModelTests;
using namespace chip::Protocols;
+using namespace chip::Test;
namespace {
+const MockNodeConfig & TestMockNodeConfig()
+{
+ using namespace Clusters::Globals::Attributes;
+
+ // clang-format off
+ static const MockNodeConfig config({
+ MockEndpointConfig(kRootEndpointId, {
+ MockClusterConfig(Clusters::IcdManagement::Id, {
+ ClusterRevision::Id, FeatureMap::Id,
+ Clusters::IcdManagement::Attributes::OperatingMode::Id,
+ }),
+ }),
+ MockEndpointConfig(kTestEndpointId, {
+ MockClusterConfig(Clusters::UnitTesting::Id, {
+ ClusterRevision::Id, FeatureMap::Id,
+ Clusters::UnitTesting::Attributes::Boolean::Id,
+ Clusters::UnitTesting::Attributes::Int16u::Id,
+ Clusters::UnitTesting::Attributes::Int8u::Id,
+ Clusters::UnitTesting::Attributes::ListFabricScoped::Id,
+ Clusters::UnitTesting::Attributes::ListStructOctetString::Id,
+ }),
+ }),
+ MockEndpointConfig(kMockEndpoint1, {
+ MockClusterConfig(MockClusterId(1), {
+ ClusterRevision::Id, FeatureMap::Id,
+ }, {
+ MockEventId(1), MockEventId(2),
+ }),
+ MockClusterConfig(MockClusterId(2), {
+ ClusterRevision::Id, FeatureMap::Id, MockAttributeId(1),
+ }),
+ }),
+ MockEndpointConfig(kMockEndpoint2, {
+ MockClusterConfig(MockClusterId(1), {
+ ClusterRevision::Id, FeatureMap::Id,
+ }),
+ MockClusterConfig(MockClusterId(2), {
+ ClusterRevision::Id, FeatureMap::Id, MockAttributeId(1), MockAttributeId(2),
+ }),
+ MockClusterConfig(MockClusterId(3), {
+ ClusterRevision::Id, FeatureMap::Id, MockAttributeId(1), MockAttributeId(2), MockAttributeId(3),
+ }),
+ }),
+ MockEndpointConfig(kMockEndpoint3, {
+ MockClusterConfig(MockClusterId(1), {
+ ClusterRevision::Id, FeatureMap::Id, MockAttributeId(1),
+ }),
+ MockClusterConfig(MockClusterId(2), {
+ ClusterRevision::Id, FeatureMap::Id, MockAttributeId(1), MockAttributeId(2), MockAttributeId(3), MockAttributeId(4),
+ }),
+ MockClusterConfig(MockClusterId(3), {
+ ClusterRevision::Id, FeatureMap::Id,
+ }),
+ MockClusterConfig(MockClusterId(4), {
+ ClusterRevision::Id, FeatureMap::Id,
+ }),
+ }),
+ });
+ // clang-format on
+ return config;
+}
+
class SingleWriteCallback : public WriteClient::Callback
{
public:
@@ -89,11 +152,13 @@
{
chip::Test::AppContext::SetUp();
mOldProvider = InteractionModelEngine::GetInstance()->SetDataModelProvider(&CustomDataModel::Instance());
+ chip::Test::SetMockNodeConfig(TestMockNodeConfig());
}
// Performs teardown for each individual test in the test suite
void TearDown() override
{
+ chip::Test::ResetMockNodeConfig();
InteractionModelEngine::GetInstance()->SetDataModelProvider(mOldProvider);
chip::Test::AppContext::TearDown();
}
diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider_Write.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider_Write.cpp
index e0d9c15..3160bbd 100644
--- a/src/data-model-providers/codegen/CodegenDataModelProvider_Write.cpp
+++ b/src/data-model-providers/codegen/CodegenDataModelProvider_Write.cpp
@@ -92,18 +92,6 @@
DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const DataModel::WriteAttributeRequest & request,
AttributeValueDecoder & decoder)
{
- ChipLogDetail(DataManagement, "Writing attribute: Cluster=" ChipLogFormatMEI " Endpoint=0x%x AttributeId=" ChipLogFormatMEI,
- ChipLogValueMEI(request.path.mClusterId), request.path.mEndpointId, ChipLogValueMEI(request.path.mAttributeId));
-
- // TODO: ordering is to check writability/existence BEFORE ACL and this seems wrong, however
- // existing unit tests (TC_AcessChecker.py) validate that we get UnsupportedWrite instead of UnsupportedAccess
- //
- // This should likely be fixed in spec (probably already fixed by
- // https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/9024)
- // and tests and implementation
- //
- // Open issue that needs fixing: https://github.com/project-chip/connectedhomeip/issues/33735
-
auto metadata = Ember::FindAttributeMetadata(request.path);
// Explicit failure in finding a suitable metadata
@@ -130,74 +118,11 @@
const EmberAfAttributeMetadata ** attributeMetadata = std::get_if<const EmberAfAttributeMetadata *>(&metadata);
VerifyOrDie(*attributeMetadata != nullptr);
- // All the global attributes that we do not have metadata for are
- // read-only. Specifically only the following list-based attributes match the
- // "global attributes not in metadata" (see GlobalAttributes.h :: GlobalAttributesNotInMetadata):
- // - AttributeList
- // - EventList
- // - AcceptedCommands
- // - GeneratedCommands
+ // Extra check: internal requests can bypass the read only check, however global attributes
+ // have no underlying storage, so write still cannot be done
//
- // Given the above, UnsupportedWrite should be correct (attempt to write to a read-only list)
- bool isReadOnly = (*attributeMetadata)->IsReadOnly();
-
- // Internal is allowed to bypass timed writes and read-only.
- if (!request.operationFlags.Has(DataModel::OperationFlags::kInternal))
- {
- VerifyOrReturnError(!isReadOnly, Status::UnsupportedWrite);
- }
-
- // ACL check for non-internal requests
- bool checkAcl = !request.operationFlags.Has(DataModel::OperationFlags::kInternal);
-
- // For chunking, ACL check is not re-done if the previous write was successful for the exact same
- // path. We apply this everywhere as a shortcut, although realistically this is only for AccessControl cluster
- if (checkAcl && request.previousSuccessPath.has_value())
- {
- // NOTE: explicit cast/check only for attribute path and nothing else.
- //
- // In particular `request.path` is a DATA path (contains a list index)
- // and we do not want request.previousSuccessPath to be auto-cast to a
- // data path with a empty list and fail the compare.
- //
- // This could be `request.previousSuccessPath != request.path` (where order
- // is important) however that would seem more brittle (relying that a != b
- // behaves differently than b != a due to casts). Overall Data paths are not
- // the same as attribute paths.
- //
- // Also note that Concrete path have a mExpanded that is not used in compares.
- const ConcreteAttributePath & attributePathA = request.path;
- const ConcreteAttributePath & attributePathB = *request.previousSuccessPath;
-
- checkAcl = (attributePathA != attributePathB);
- }
-
- if (checkAcl)
- {
- VerifyOrReturnError(request.subjectDescriptor != nullptr, Status::UnsupportedAccess);
-
- Access::RequestPath requestPath{ .cluster = request.path.mClusterId,
- .endpoint = request.path.mEndpointId,
- .requestType = Access::RequestType::kAttributeWriteRequest,
- .entityId = request.path.mAttributeId };
- CHIP_ERROR err = Access::GetAccessControl().Check(*request.subjectDescriptor, requestPath,
- RequiredPrivilege::ForWriteAttribute(request.path));
-
- if (err != CHIP_NO_ERROR)
- {
- VerifyOrReturnValue(err != CHIP_ERROR_ACCESS_DENIED, Status::UnsupportedAccess);
- VerifyOrReturnValue(err != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL, Status::AccessRestricted);
-
- return err;
- }
- }
-
- // Internal is allowed to bypass timed writes and read-only.
- if (!request.operationFlags.Has(DataModel::OperationFlags::kInternal))
- {
- VerifyOrReturnError(!(*attributeMetadata)->MustUseTimedWrite() || request.writeFlags.Has(DataModel::WriteFlags::kTimed),
- Status::NeedsTimedInteraction);
- }
+ // I.e. if we get a `EmberAfCluster*` value from finding metadata, we fail here.
+ VerifyOrReturnError(attributeMetadata != nullptr, Status::UnsupportedWrite);
if (request.path.mDataVersion.HasValue())
{
diff --git a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp
index 4d54a19..2505fdd 100644
--- a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp
+++ b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp
@@ -1905,29 +1905,6 @@
}
}
-TEST_F(TestCodegenModelViaMocks, EmberAttributeWriteAclDeny)
-{
- UseMockNodeConfig config(gTestNodeConfig);
- CodegenDataModelProviderWithContext model;
- ScopedMockAccessControl accessControl;
-
- /* Using this path is also failing existence checks, so this cannot be enabled
- * until we fix ordering of ACL to be done before existence checks
-
- WriteOperation test(kMockEndpoint1, MockClusterId(1), MockAttributeId(10));
- AttributeValueDecoder decoder = test.DecoderFor<uint32_t>(1234);
-
- ASSERT_EQ(model.WriteAttribute(test.GetRequest(), decoder), Status::UnsupportedAccess);
- ASSERT_TRUE(model.ChangeListener().DirtyList().empty());
- */
-
- WriteOperation test(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZCL_INT32U_ATTRIBUTE_TYPE));
- AttributeValueDecoder decoder = test.DecoderFor<uint32_t>(1234);
-
- ASSERT_EQ(model.WriteAttribute(test.GetRequest(), decoder), Status::UnsupportedAccess);
- ASSERT_TRUE(model.ChangeListener().DirtyList().empty());
-}
-
TEST_F(TestCodegenModelViaMocks, EmberAttributeWriteBasicTypes)
{
TestEmberScalarTypeWrite<uint8_t, ZCL_INT8U_ATTRIBUTE_TYPE>(0x12);
@@ -2208,42 +2185,6 @@
EXPECT_EQ(writtenData[4], 13u);
}
-TEST_F(TestCodegenModelViaMocks, EmberAttributeWriteTimedWrite)
-{
- UseMockNodeConfig config(gTestNodeConfig);
- CodegenDataModelProviderWithContext model;
- ScopedMockAccessControl accessControl;
-
- WriteOperation test(kMockEndpoint3, MockClusterId(4), kAttributeIdTimedWrite);
- test.SetSubjectDescriptor(kAdminSubjectDescriptor);
-
- AttributeValueDecoder decoder = test.DecoderFor<int32_t>(1234);
-
- ASSERT_EQ(model.WriteAttribute(test.GetRequest(), decoder), Status::NeedsTimedInteraction);
-
- // writing as timed should be fine
- test.SetWriteFlags(WriteFlags::kTimed);
- ASSERT_EQ(model.WriteAttribute(test.GetRequest(), decoder), CHIP_NO_ERROR);
-}
-
-TEST_F(TestCodegenModelViaMocks, EmberAttributeWriteReadOnlyAttribute)
-{
- UseMockNodeConfig config(gTestNodeConfig);
- CodegenDataModelProviderWithContext model;
- ScopedMockAccessControl accessControl;
-
- WriteOperation test(kMockEndpoint3, MockClusterId(4), kAttributeIdReadOnly);
- test.SetSubjectDescriptor(kAdminSubjectDescriptor);
-
- AttributeValueDecoder decoder = test.DecoderFor<int32_t>(1234);
-
- ASSERT_EQ(model.WriteAttribute(test.GetRequest(), decoder), Status::UnsupportedWrite);
-
- // Internal writes bypass the read only requirement
- test.SetOperationFlags(OperationFlags::kInternal);
- ASSERT_EQ(model.WriteAttribute(test.GetRequest(), decoder), CHIP_NO_ERROR);
-}
-
TEST_F(TestCodegenModelViaMocks, EmberAttributeWriteDataVersion)
{
UseMockNodeConfig config(gTestNodeConfig);
