Move ::WriteAttribute validation inside Interaction Model Write Handling (project-chip#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>

Author: 3 people

src/app/InteractionModelEngine.cpp

@@ -1801,28 +1801,8 @@ Protocols::InteractionModel::Status InteractionModelEngine::CheckCommandExistenc
         }
     }
 
-    {
-        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)

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 app {
 
 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,23 +760,97 @@ void WriteHandler::MoveToState(const State aTargetState)
     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)
 {
     // Writes do not have a checked-path. If data model interface is enabled (both checked and only version)
     // 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;
 

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 @@ class WriteHandler : public Messaging::ExchangeDelegate
                                  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);

src/app/data-model-provider/MetadataLookup.cpp

@@ -21,6 +21,8 @@ namespace chip {
 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 @@ std::optional<AttributeEntry> AttributeFinder::Find(const ConcreteAttributePath
     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 @@ Protocols::InteractionModel::Status ValidateClusterPath(ProviderMetadataTree * p
         return successStatus;
     }
 
+    // If we get here, the cluster identified by the path does not exist.
     auto endpoints = provider->EndpointsIgnoreError();
     for (auto & endpointEntry : endpoints)
     {

src/app/data-model-provider/MetadataLookup.h

@@ -24,6 +24,7 @@
 #include <app/data-model/MetadataList.h>
 #include <lib/core/DataModelTypes.h>
 #include <lib/support/CodeUtils.h>
+#include <protocols/interaction_model/StatusCode.h>
 
 #include <optional>
 
@@ -65,20 +66,14 @@ class AttributeFinder
     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.

src/app/data-model-provider/OperationTypes.h

@@ -88,17 +88,6 @@ struct WriteAttributeRequest : OperationRequest
 {
     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

src/app/data-model-provider/Provider.h

@@ -76,12 +76,7 @@ class Provider : public ProviderMetadataTree
     ///
     /// 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.

src/app/data-model-provider/tests/WriteTesting.h

@@ -60,12 +60,6 @@ class WriteOperation
         return *this;
     }
 
-    WriteOperation & SetPreviousSuccessPath(std::optional<ConcreteAttributePath> path)
-    {
-        mRequest.previousSuccessPath = path;
-        return *this;
-    }
-
     WriteOperation & SetDataVersion(Optional<DataVersion> version)
     {
         mRequest.path.mDataVersion = version;

src/app/reporting/Engine.cpp

@@ -52,20 +52,6 @@ namespace {
 
 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 @@ CHIP_ERROR Engine::CheckAccessDeniedEventPaths(TLV::TLVWriter & aWriter, bool &
         }
 
         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)
         {