From 96c66453f321bc4c25f5bdffc5e386de86d8a3b4 Mon Sep 17 00:00:00 2001
From: Marc Lepage <67919234+mlepage-google@users.noreply.github.com>
Date: Wed, 12 Jan 2022 00:27:30 -0500
Subject: [PATCH] Fix and refactor ReadSingleClusterData (#13468)

* Fix and refactor ReadSingleClusterData

PR #12660 seems to have refactored ReadSingleClusterData in such a way
that the access control check may be skipped. (Possibly due to merge?)

Fix this by refactoring the function to a sensible flow of checks.

Progress towards #10239

* Apply review comment.

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
---
 .../util/ember-compatibility-functions.cpp    | 60 ++++++++++---------
 1 file changed, 31 insertions(+), 29 deletions(-)

diff --git a/src/app/util/ember-compatibility-functions.cpp b/src/app/util/ember-compatibility-functions.cpp
index c9e092d82409f0..0bd07101e769e4 100644
--- a/src/app/util/ember-compatibility-functions.cpp
+++ b/src/app/util/ember-compatibility-functions.cpp
@@ -369,35 +369,32 @@ CHIP_ERROR ReadSingleClusterData(const SubjectDescriptor & aSubjectDescriptor, c
                   " (expanded=%d)",
                   ChipLogValueMEI(aPath.mClusterId), aPath.mEndpointId, ChipLogValueMEI(aPath.mAttributeId), aPath.mExpanded);
 
+    // Check attribute existence. This includes attributes with registered metadata, but also specially handled
+    // mandatory global attributes (which just check for cluster on endpoint).
+
+    EmberAfCluster * attributeCluster            = nullptr;
+    EmberAfAttributeMetadata * attributeMetadata = nullptr;
+
     if (aPath.mAttributeId == Clusters::Globals::Attributes::AttributeList::Id)
     {
-        // This is not in our attribute metadata, so we just check for this
-        // endpoint+cluster existing.
-        EmberAfCluster * cluster = emberAfFindCluster(aPath.mEndpointId, aPath.mClusterId, CLUSTER_MASK_SERVER);
-        if (cluster)
-        {
-            AttributeListReader reader(cluster);
-            bool ignored; // Our reader always tries to encode
-            return ReadViaAccessInterface(aSubjectDescriptor.fabricIndex, aPath, aAttributeReports, apEncoderState, &reader,
-                                          &ignored);
-        }
-
-        // else to save codesize just fall through and do the metadata search
-        // (which we know will fail and error out);
+        attributeCluster = emberAfFindCluster(aPath.mEndpointId, aPath.mClusterId, CLUSTER_MASK_SERVER);
+    }
+    else
+    {
+        attributeMetadata =
+            emberAfLocateAttributeMetadata(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId, CLUSTER_MASK_SERVER, 0);
     }
 
-    EmberAfAttributeMetadata * attributeMetadata =
-        emberAfLocateAttributeMetadata(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId, CLUSTER_MASK_SERVER, 0);
-
-    if (attributeMetadata == nullptr)
+    if (attributeCluster == nullptr && attributeMetadata == nullptr)
     {
         AttributeReportIB::Builder & attributeReport = aAttributeReports.CreateAttributeReport();
         ReturnErrorOnFailure(aAttributeReports.GetError());
-
-        // This path is not actually supported.
         return SendFailureStatus(aPath, attributeReport, Protocols::InteractionModel::Status::UnsupportedAttribute, nullptr);
     }
 
+    // Check access control. A failed check will disallow the operation, and may or may not generate an attribute report
+    // depending on whether the path was expanded.
+
     {
         Access::RequestPath requestPath{ .cluster = aPath.mClusterId, .endpoint = aPath.mEndpointId };
         Access::Privilege requestPrivilege = Access::Privilege::kView; // TODO: get actual request privilege
@@ -419,27 +416,32 @@ CHIP_ERROR ReadSingleClusterData(const SubjectDescriptor & aSubjectDescriptor, c
         }
     }
 
-    // Value encoder will encode the whole AttributeReport, including the path, value and the version.
-    // The AttributeValueEncoder may encode more than one AttributeReportIB for the list chunking feature.
-    if (auto * attrOverride = findAttributeAccessOverride(aPath.mEndpointId, aPath.mClusterId))
-    {
-        bool triedEncode;
-        ReturnErrorOnFailure(ReadViaAccessInterface(aSubjectDescriptor.fabricIndex, aPath, aAttributeReports, apEncoderState,
-                                                    attrOverride, &triedEncode));
+    // Read attribute using attribute override, if appropriate. This includes registered overrides, but also
+    // specially handled mandatory global attributes (which use unregistered overrides).
 
-        if (triedEncode)
+    {
+        // Special handling for mandatory global attributes: these are always for attribute list, using a special
+        // reader (which can be lightweight constructed even from nullptr).
+        AttributeListReader reader(attributeCluster);
+        AttributeAccessInterface * attributeOverride =
+            (attributeCluster != nullptr) ? &reader : findAttributeAccessOverride(aPath.mEndpointId, aPath.mClusterId);
+        if (attributeOverride)
         {
-            return CHIP_NO_ERROR;
+            bool triedEncode;
+            ReturnErrorOnFailure(ReadViaAccessInterface(aSubjectDescriptor.fabricIndex, aPath, aAttributeReports, apEncoderState,
+                                                        attributeOverride, &triedEncode));
+            ReturnErrorCodeIf(triedEncode, CHIP_NO_ERROR);
         }
     }
 
+    // Read attribute using Ember, if it doesn't have an override.
+
     AttributeReportIB::Builder & attributeReport = aAttributeReports.CreateAttributeReport();
     ReturnErrorOnFailure(aAttributeReports.GetError());
 
     TLV::TLVWriter backup;
     attributeReport.Checkpoint(backup);
 
-    // We have verified that the attribute exists.
     AttributeDataIB::Builder & attributeDataIBBuilder = attributeReport.CreateAttributeData();
     ReturnErrorOnFailure(attributeDataIBBuilder.GetError());