Handle ACL and readability in reporting/Engine.cpp

src/app/BUILD.gn

@@ -201,7 +201,10 @@ static_library("interaction-model") {
     "reporting/reporting.h",
   ]
 
-  deps = [ "${chip_root}/src/app:events" ]
+  deps = [
+    "${chip_root}/src/app:events",
+    "${chip_root}/src/app:global-attributes",
+  ]
 
   # Temporary dependency: codegen data provider instance should be provided
   # by the application

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

@@ -104,33 +104,6 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::ReadAttribute(const Data
                   ChipLogValueMEI(request.path.mClusterId), request.path.mEndpointId, ChipLogValueMEI(request.path.mAttributeId),
                   request.path.mExpanded);
 
-    // ACL check for non-internal requests
-    if (!request.operationFlags.Has(DataModel::OperationFlags::kInternal))
-    {
-        VerifyOrReturnError(request.subjectDescriptor != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
-
-        Access::RequestPath requestPath{ .cluster     = request.path.mClusterId,
-                                         .endpoint    = request.path.mEndpointId,
-                                         .requestType = Access::RequestType::kAttributeReadRequest,
-                                         .entityId    = request.path.mAttributeId };
-        CHIP_ERROR err = Access::GetAccessControl().Check(*request.subjectDescriptor, requestPath,
-                                                          RequiredPrivilege::ForReadAttribute(request.path));
-        if (err != CHIP_NO_ERROR)
-        {
-            VerifyOrReturnError((err == CHIP_ERROR_ACCESS_DENIED) || (err == CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL), err);
-
-            // Implementation of 8.4.3.2 of the spec for path expansion
-            if (request.path.mExpanded)
-            {
-                return CHIP_NO_ERROR;
-            }
-
-            // access denied and access restricted have specific codes for IM
-            return err == CHIP_ERROR_ACCESS_DENIED ? CHIP_IM_GLOBAL_STATUS(UnsupportedAccess)
-                                                   : CHIP_IM_GLOBAL_STATUS(AccessRestricted);
-        }
-    }
-
     auto metadata = Ember::FindAttributeMetadata(request.path);
 
     // Explicit failure in finding a suitable metadata

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

@@ -1321,20 +1321,6 @@ TEST(TestCodegenModelViaMocks, CommandHandlerInterfaceAcceptedCommands)
     EXPECT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 33)).has_value());
 }
 
-TEST(TestCodegenModelViaMocks, EmberAttributeReadAclDeny)
-{
-    UseMockNodeConfig config(gTestNodeConfig);
-    CodegenDataModelProviderWithContext model;
-    ScopedMockAccessControl accessControl;
-
-    ReadOperation testRequest(kMockEndpoint1, MockClusterId(1), MockAttributeId(10));
-    testRequest.SetSubjectDescriptor(kDenySubjectDescriptor);
-
-    std::unique_ptr<AttributeValueEncoder> encoder = testRequest.StartEncoding();
-
-    ASSERT_EQ(model.ReadAttribute(testRequest.GetRequest(), *encoder), Status::UnsupportedAccess);
-}
-
 TEST(TestCodegenModelViaMocks, ReadForInvalidGlobalAttributePath)
 {
     UseMockNodeConfig config(gTestNodeConfig);
@@ -1392,24 +1378,6 @@ TEST(TestCodegenModelViaMocks, EmberAttributeInvalidRead)
     }
 }
 
-TEST(TestCodegenModelViaMocks, EmberAttributePathExpansionAccessDeniedRead)
-{
-    UseMockNodeConfig config(gTestNodeConfig);
-    CodegenDataModelProviderWithContext model;
-    ScopedMockAccessControl accessControl;
-
-    ReadOperation testRequest(kMockEndpoint1, MockClusterId(1), MockAttributeId(10));
-    testRequest.SetSubjectDescriptor(kDenySubjectDescriptor);
-    testRequest.SetPathExpanded(true);
-
-    std::unique_ptr<AttributeValueEncoder> encoder = testRequest.StartEncoding();
-
-    // For expanded paths, access control failures succeed without encoding anything
-    // This is temporary until ACL checks are moved inside the IM/ReportEngine
-    ASSERT_EQ(model.ReadAttribute(testRequest.GetRequest(), *encoder), CHIP_NO_ERROR);
-    ASSERT_FALSE(encoder->TriedEncode());
-}
-
 TEST(TestCodegenModelViaMocks, AccessInterfaceUnsupportedRead)
 {
     UseMockNodeConfig config(gTestNodeConfig);

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

@@ -60,15 +60,7 @@ class Provider : public ProviderMetadataTree
     virtual InteractionModelContext CurrentContext() const { return mContext; }
 
     /// TEMPORARY/TRANSITIONAL requirement for transitioning from ember-specific code
-    ///   ReadAttribute is REQUIRED to perform:
-    ///     - ACL validation (see notes on OperationFlags::kInternal)
-    ///     - Validation of readability/writability (also controlled by OperationFlags::kInternal)
-    ///     - use request.path.mExpanded to skip encoding replies for data according
-    ///       to 8.4.3.2 of the spec:
-    ///         > If the path indicates attribute data that is not readable, then the path SHALL
-    ///           be discarded.
-    ///         > Else if reading from the attribute in the path requires a privilege that is not
-    ///           granted to access the cluster in the path, then the path SHALL be discarded.
+    ///   ReadAttribute is REQUIRED to respond to GlobalAttribute read requests
     ///
     /// Return value notes:
     ///   ActionReturnStatus::IsOutOfSpaceEncodingResponse

src/app/reporting/Engine.cpp

@@ -16,17 +16,24 @@
  *    limitations under the License.
  */
 
+#include <access/AccessRestrictionProvider.h>
+#include <access/Privilege.h>
 #include <app/AppConfig.h>
 #include <app/ConcreteEventPath.h>
+#include <app/GlobalAttributes.h>
 #include <app/InteractionModelEngine.h>
 #include <app/RequiredPrivilege.h>
 #include <app/data-model-provider/ActionReturnStatus.h>
+#include <app/data-model-provider/MetadataTypes.h>
 #include <app/data-model-provider/Provider.h>
 #include <app/icd/server/ICDServerConfig.h>
 #include <app/reporting/Engine.h>
 #include <app/reporting/reporting.h>
 #include <app/util/MatterCallbacks.h>
+#include <lib/core/CHIPError.h>
 #include <lib/core/DataModelTypes.h>
+#include <lib/support/CodeUtils.h>
+#include <optional>
 #include <protocols/interaction_model/StatusCode.h>
 
 #if CHIP_CONFIG_ENABLE_ICD_SERVER
@@ -52,6 +59,58 @@ Status EventPathValid(DataModel::Provider * model, const ConcreteEventPath & eve
     return Status::Success;
 }
 
+/// Returns the status of ACL validation.
+///   if the status is set, the status is FINAL (i.e. permanent failure OR success due to path expansion logic.)
+///   if the status is not set, the processing can continue
+std::optional<CHIP_ERROR> ValidateReadACL(DataModel::Provider * dataModel, const Access::SubjectDescriptor & subjectDescriptor,
+                                          const ConcreteReadAttributePath & path)
+{
+
+    Access::RequestPath requestPath{ .cluster     = path.mClusterId,
+                                     .endpoint    = path.mEndpointId,
+                                     .requestType = Access::RequestType::kAttributeReadRequest,
+                                     .entityId    = path.mAttributeId };
+
+    std::optional<DataModel::AttributeInfo> info = dataModel->GetAttributeInfo(path);
+
+    chip::Access::Privilege requiredPrivilege = chip::Access::Privilege::kView; // default
+    if (info.has_value() && info->readPrivilege.has_value())
+    {
+        // set a default even if we do not know and later report the real error if ACL looks ok
+        requiredPrivilege = *info->readPrivilege;
+    }
+
+    CHIP_ERROR err = Access::GetAccessControl().Check(subjectDescriptor, requestPath, requiredPrivilege);
+    if (err == CHIP_NO_ERROR)
+    {
+        if (IsSupportedGlobalAttributeNotInMetadata(path.mAttributeId))
+        {
+            // Global attributes passing a kView check is ok
+            return std::nullopt;
+        }
+
+        // attribute does not exist, however we do not now if this is a unsupported endpoint, cluster or attribute,
+        // so the info.has_value() is only handled by the ReadAttribute in the DataModel::Provider.
+        //
+        // What we can validate is `write-only attributes` (i.e if we have info, readPrivilege should be set)
+
+        // if the attribute does exist, it should be readable
+        VerifyOrReturnError(!info.has_value() || info->readPrivilege.has_value(), CHIP_IM_GLOBAL_STATUS(UnsupportedRead));
+
+        return std::nullopt;
+    }
+    VerifyOrReturnError((err == CHIP_ERROR_ACCESS_DENIED) || (err == CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL), err);
+
+    // Implementation of 8.4.3.2 of the spec for path expansion
+    if (path.mExpanded)
+    {
+        return CHIP_NO_ERROR;
+    }
+
+    // access denied and access restricted have specific codes for IM
+    return err == CHIP_ERROR_ACCESS_DENIED ? CHIP_IM_GLOBAL_STATUS(UnsupportedAccess) : CHIP_IM_GLOBAL_STATUS(AccessRestricted);
+}
+
 DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel,
                                                   const Access::SubjectDescriptor & subjectDescriptor, bool isFabricFiltered,
                                                   AttributeReportIBs::Builder & reportBuilder,
@@ -64,10 +123,7 @@ DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataMode
 
     DataModel::ReadAttributeRequest readRequest;
 
-    if (isFabricFiltered)
-    {
-        readRequest.readFlags.Set(DataModel::ReadFlags::kFabricFiltered);
-    }
+    readRequest.readFlags.Set(DataModel::ReadFlags::kFabricFiltered, isFabricFiltered);
     readRequest.subjectDescriptor = &subjectDescriptor;
     readRequest.path              = path;
 
@@ -84,9 +140,17 @@ DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataMode
     TLV::TLVWriter checkpoint;
     reportBuilder.Checkpoint(checkpoint);
 
+    DataModel::ActionReturnStatus status(CHIP_NO_ERROR);
     AttributeValueEncoder attributeValueEncoder(reportBuilder, subjectDescriptor, path, version, isFabricFiltered, encoderState);
 
-    DataModel::ActionReturnStatus status = dataModel->ReadAttribute(readRequest, attributeValueEncoder);
+    if (auto access_status = ValidateReadACL(dataModel, subjectDescriptor, path); access_status.has_value())
+    {
+        status = *access_status;
+    }
+    else
+    {
+        status = dataModel->ReadAttribute(readRequest, attributeValueEncoder);
+    }
 
     if (status.IsSuccess())
     {