Address review comments

- move attribute read check back up - respond with status if access control has serious error during command handling
project-chip · Dec 9, 2021 · 7153b84 · 7153b84
1 parent c06a867
commit 7153b84
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 30 deletions.
diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp
@@ -255,7 +255,10 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand
         err                                = CHIP_NO_ERROR; // TODO: remove override
         if (err != CHIP_NO_ERROR)
         {
-            ReturnErrorCodeIf(err != CHIP_ERROR_ACCESS_DENIED, err);
+            if (err != CHIP_ERROR_ACCESS_DENIED)
+            {
+                return AddStatus(concretePath, Protocols::InteractionModel::Status::Failure);
+            }
             // TODO: when wildcard/group invokes are supported, handle them to discard rather than fail with status
             return AddStatus(concretePath, Protocols::InteractionModel::Status::UnsupportedAccess);
         }

diff --git a/src/app/util/ember-compatibility-functions.cpp b/src/app/util/ember-compatibility-functions.cpp
@@ -367,6 +367,29 @@ CHIP_ERROR ReadSingleClusterData(FabricIndex aAccessingFabricIndex, const Concre
         return SendFailureStatus(aPath, attributeReport, Protocols::InteractionModel::Status::UnsupportedAttribute, nullptr);
     }
 
+    {
+        Access::SubjectDescriptor subjectDescriptor; // TODO: get actual subject descriptor
+        Access::RequestPath requestPath{ .cluster = aPath.mClusterId, .endpoint = aPath.mEndpointId };
+        Access::Privilege requestPrivilege = Access::Privilege::kView; // TODO: get actual request privilege
+        bool pathWasExpanded               = false;                    // TODO: get actual expanded flag
+        CHIP_ERROR err                     = Access::GetAccessControl().Check(subjectDescriptor, requestPath, requestPrivilege);
+        err                                = CHIP_NO_ERROR; // TODO: remove override
+        if (err != CHIP_NO_ERROR)
+        {
+            ReturnErrorCodeIf(err != CHIP_ERROR_ACCESS_DENIED, err);
+            if (pathWasExpanded)
+            {
+                return CHIP_NO_ERROR;
+            }
+            else
+            {
+                AttributeReportIB::Builder attributeReport = aAttributeReports.CreateAttributeReport();
+                ReturnErrorOnFailure(aAttributeReports.GetError());
+                return SendFailureStatus(aPath, attributeReport, Protocols::InteractionModel::Status::UnsupportedAccess, nullptr);
+            }
+        }
+    }
+
     // 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))
@@ -381,14 +404,11 @@ CHIP_ERROR ReadSingleClusterData(FabricIndex aAccessingFabricIndex, const Concre
         }
     }
 
-    TLV::TLVWriter backupBeforeReport;
-    aAttributeReports.Checkpoint(backupBeforeReport);
-
     AttributeReportIB::Builder attributeReport = aAttributeReports.CreateAttributeReport();
     ReturnErrorOnFailure(aAttributeReports.GetError());
 
-    TLV::TLVWriter backupStartOfReport;
-    attributeReport.Checkpoint(backupStartOfReport);
+    TLV::TLVWriter backup;
+    attributeReport.Checkpoint(backup);
 
     // We have verified that the attribute exists.
     AttributeDataIB::Builder & attributeDataIBBuilder = attributeReport.CreateAttributeData();
@@ -417,29 +437,6 @@ CHIP_ERROR ReadSingleClusterData(FabricIndex aAccessingFabricIndex, const Concre
 
     if (emberStatus == EMBER_ZCL_STATUS_SUCCESS)
     {
-        {
-            Access::SubjectDescriptor subjectDescriptor; // TODO: get actual subject descriptor
-            Access::RequestPath requestPath{ .cluster = aPath.mClusterId, .endpoint = aPath.mEndpointId };
-            Access::Privilege requestPrivilege = Access::Privilege::kView; // TODO: get actual request privilege
-            bool pathWasExpanded               = false;                    // TODO: get actual expanded flag
-            CHIP_ERROR err                     = Access::GetAccessControl().Check(subjectDescriptor, requestPath, requestPrivilege);
-            err                                = CHIP_NO_ERROR; // TODO: remove override
-            if (err != CHIP_NO_ERROR)
-            {
-                ReturnErrorCodeIf(err != CHIP_ERROR_ACCESS_DENIED, err);
-                if (pathWasExpanded)
-                {
-                    aAttributeReports.Rollback(backupBeforeReport);
-                    return CHIP_NO_ERROR;
-                }
-                else
-                {
-                    return SendFailureStatus(aPath, attributeReport, Protocols::InteractionModel::Status::UnsupportedAccess,
-                                             &backupStartOfReport);
-                }
-            }
-        }
-
         EmberAfAttributeType attributeType = attributeMetadata->attributeType;
         bool isNullable                    = attributeMetadata->IsNullable();
         TLV::TLVWriter * writer            = attributeDataIBBuilder.GetWriter();
@@ -660,7 +657,7 @@ CHIP_ERROR ReadSingleClusterData(FabricIndex aAccessingFabricIndex, const Concre
         return SendSuccessStatus(attributeReport, attributeDataIBBuilder);
     }
 
-    return SendFailureStatus(aPath, attributeReport, imStatus, &backupStartOfReport);
+    return SendFailureStatus(aPath, attributeReport, imStatus, &backup);
 }
 
 namespace {