Fix ACL constraint error (return constraint error instead of failure …

…when the ACL entry is invalid) (#20736) * Define a test for access control constraints * Typgo fixes * Restyle * Fix some text and formatting * Zap regen * Make ACL cluster return constraint error if the ACL entry is not valid * Add one more test: Step 31 * Zap regen for the new test * Added the rest of the tests from #20672 * Restyle * Removed some steps: too large numbers for subjects, cannot be represented * Zap regen * Restyle * Fix build, add more ACL changes, zap regen after adjusting test case to match bug report * Fix more things to return CONSTRAINT_ERROR * Convert more invalid argument to constraint errors. This is not ideal and seems like a whack-a-mole bug fixing * Restyle * Fix comments in yaml * Restyle * Restyle messed things up. Corrected it * One more comment fix * Restyle * Split out IM status code header and cpp into a separate library for layering purposes. Layering still not ideal though. * Restyle * Also update TestAccessControlCluster * Zap regen * Updated test ACL error codes a bit and zap regen * Update logic to centrailize error code processing location * Added unit test for step 35 as well (pass) * Added even more tests and updated formatting of ACL a bit for readability * Restyle * One more test for invalid privilege * Restyle
project-chip · Jul 24, 2023 · 1452448 · 1452448
1 parent 8d97827
commit 1452448
Show file tree

Hide file tree

Showing 9 changed files with 1,538 additions and 30 deletions.
diff --git a/.github/workflows/darwin-tests.yaml b/.github/workflows/darwin-tests.yaml
@@ -102,7 +102,7 @@ jobs:
                   ./scripts/run_in_build_env.sh \
                   "./scripts/tests/run_test_suite.py \
                      --chip-tool ./out/darwin-x64-darwin-framework-tool-${BUILD_VARIANT}/darwin-framework-tool \
-                     --target-skip-glob '{TestGroupMessaging}' \
+                     --target-skip-glob '{TestGroupMessaging,TestAccessControlConstraints}' \
                      run \
                      --iterations 1 \
                      --test-timeout-seconds 120 \

diff --git a/src/app/clusters/access-control-server/access-control-server.cpp b/src/app/clusters/access-control-server/access-control-server.cpp
@@ -55,14 +55,27 @@ class AccessControlAttribute : public AttributeAccessInterface, public EntryList
 public:
     AccessControlAttribute() : AttributeAccessInterface(Optional<EndpointId>(0), AccessControlCluster::Id) {}
 
+    /// IM-level implementation of read
+    ///
+    /// Returns appropriately mapped CHIP_ERROR if applicable (may return CHIP_IM_GLOBAL_STATUS errors)
     CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override;
+
+    /// IM-level implementation of write
+    ///
+    /// Returns appropriately mapped CHIP_ERROR if applicable (may return CHIP_IM_GLOBAL_STATUS errors)
     CHIP_ERROR Write(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder) override;
 
 public:
     void OnEntryChanged(const SubjectDescriptor * subjectDescriptor, FabricIndex fabric, size_t index, const Entry * entry,
                         ChangeType changeType) override;
 
 private:
+    /// Business logic implementation of write, returns generic CHIP_ERROR.
+    CHIP_ERROR ReadImpl(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder);
+
+    /// Business logic implementation of write, returns generic CHIP_ERROR.
+    CHIP_ERROR WriteImpl(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder);
+
     CHIP_ERROR ReadAcl(AttributeValueEncoder & aEncoder);
     CHIP_ERROR ReadExtension(AttributeValueEncoder & aEncoder);
     CHIP_ERROR WriteAcl(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder);
@@ -125,7 +138,7 @@ CHIP_ERROR CheckExtensionEntryDataFormat(const ByteSpan & data)
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR AccessControlAttribute::Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder)
+CHIP_ERROR AccessControlAttribute::ReadImpl(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder)
 {
     switch (aPath.mAttributeId)
     {
@@ -205,7 +218,7 @@ CHIP_ERROR AccessControlAttribute::ReadExtension(AttributeValueEncoder & aEncode
     });
 }
 
-CHIP_ERROR AccessControlAttribute::Write(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder)
+CHIP_ERROR AccessControlAttribute::WriteImpl(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder)
 {
     switch (aPath.mAttributeId)
     {
@@ -415,6 +428,47 @@ void AccessControlAttribute::OnEntryChanged(const SubjectDescriptor * subjectDes
     ChipLogError(DataManagement, "AccessControlCluster: event failed %" CHIP_ERROR_FORMAT, err.Format());
 }
 
+CHIP_ERROR ChipErrorToImErrorMap(CHIP_ERROR err)
+{
+    // Map some common errors into an underlying IM error
+    // Separate logging is done to not lose the original error location in case such
+    // this are available.
+    CHIP_ERROR mappedError = err;
+
+    if (err == CHIP_ERROR_INVALID_ARGUMENT)
+    {
+        mappedError = CHIP_IM_GLOBAL_STATUS(ConstraintError);
+    }
+    else if (err == CHIP_ERROR_NOT_FOUND)
+    {
+        // Not found is generally also illegal argument: caused a lookup into an invalid location,
+        // like invalid subjects or targets.
+        mappedError = CHIP_IM_GLOBAL_STATUS(ConstraintError);
+    }
+    else if (err == CHIP_ERROR_NO_MEMORY)
+    {
+        mappedError = CHIP_IM_GLOBAL_STATUS(ResourceExhausted);
+    }
+
+    if (mappedError != err)
+    {
+        ChipLogError(DataManagement, "Re-mapped %" CHIP_ERROR_FORMAT " into %" CHIP_ERROR_FORMAT " for IM return codes",
+                     err.Format(), mappedError.Format());
+    }
+
+    return mappedError;
+}
+
+CHIP_ERROR AccessControlAttribute::Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder)
+{
+    return ChipErrorToImErrorMap(ReadImpl(aPath, aEncoder));
+}
+
+CHIP_ERROR AccessControlAttribute::Write(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder)
+{
+    return ChipErrorToImErrorMap(WriteImpl(aPath, aDecoder));
+}
+
 } // namespace
 
 void MatterAccessControlPluginServerInitCallback()

diff --git a/src/app/tests/suites/TestAccessControlCluster.yaml b/src/app/tests/suites/TestAccessControlCluster.yaml
@@ -177,7 +177,7 @@ tests:
                   },
               ]
       response:
-          error: 1
+          error: CONSTRAINT_ERROR
 
     - label: "Verify"
       command: "readAttribute"
@@ -214,7 +214,7 @@ tests:
                   },
               ]
       response:
-          error: 1
+          error: CONSTRAINT_ERROR
 
     - label: "Verify"
       command: "readAttribute"
@@ -251,7 +251,7 @@ tests:
                   },
               ]
       response:
-          error: 1
+          error: CONSTRAINT_ERROR
 
     - label: "Verify"
       command: "readAttribute"
@@ -289,7 +289,7 @@ tests:
                   },
               ]
       response:
-          error: 1
+          error: CONSTRAINT_ERROR
 
     - label: "Verify"
       command: "readAttribute"
@@ -348,7 +348,7 @@ tests:
                   },
               ]
       response:
-          error: 1
+          error: FAILURE
 
     - label: "Verify"
       command: "readAttribute"
@@ -407,7 +407,7 @@ tests:
                   },
               ]
       response:
-          error: 1
+          error: FAILURE
 
     - label: "Verify"
       command: "readAttribute"