From b740b187ce504b8d1a2fcc26a22ba0e1748eaac4 Mon Sep 17 00:00:00 2001 From: Marc Lepage Date: Fri, 25 Feb 2022 15:54:24 -0500 Subject: [PATCH] Support empty subjects in group ACL entries Spec was revised to allow empty subjects as wildcard for group auth mode (as for CASE auth mode). This PR updates the code, unit tests, and YAML tests. Fixes #15355 --- src/access/AccessControl.cpp | 3 - src/access/tests/TestAccessControl.cpp | 27 +++++--- src/app/tests/suites/TestGroupMessaging.yaml | 36 +++++++--- .../chip-tool/zap-generated/test/Commands.h | 69 +++++++++++++++---- 4 files changed, 98 insertions(+), 37 deletions(-) diff --git a/src/access/AccessControl.cpp b/src/access/AccessControl.cpp index 35a82740704b14..b193b0fb0716be 100644 --- a/src/access/AccessControl.cpp +++ b/src/access/AccessControl.cpp @@ -341,9 +341,6 @@ bool AccessControl::IsValid(const Entry & entry) // Privilege must not be administer. VerifyOrExit(privilege != Privilege::kAdminister, log = "invalid privilege"); - - // Subject must be present. - VerifyOrExit(subjectCount > 0, log = "invalid subject count"); } for (size_t i = 0; i < subjectCount; ++i) diff --git a/src/access/tests/TestAccessControl.cpp b/src/access/tests/TestAccessControl.cpp index 50ca023542a3a2..9961793aa6879d 100644 --- a/src/access/tests/TestAccessControl.cpp +++ b/src/access/tests/TestAccessControl.cpp @@ -1067,6 +1067,23 @@ void TestAclValidateAuthModeSubject(nlTestSuite * inSuite, void * inContext) // Each case tries to update the first entry, then add a second entry, then unconditionally delete it NL_TEST_ASSERT(inSuite, accessControl.CreateEntry(nullptr, entry) == CHIP_NO_ERROR); + // CASE and group may have empty subjects list + { + NL_TEST_ASSERT(inSuite, entry.RemoveSubject(0) == CHIP_NO_ERROR); + + NL_TEST_ASSERT(inSuite, entry.SetAuthMode(AuthMode::kCase) == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, accessControl.UpdateEntry(0, entry) == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, accessControl.CreateEntry(nullptr, entry) == CHIP_NO_ERROR); + accessControl.DeleteEntry(1); + + NL_TEST_ASSERT(inSuite, entry.SetAuthMode(AuthMode::kGroup) == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, accessControl.UpdateEntry(0, entry) == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, accessControl.CreateEntry(nullptr, entry) == CHIP_NO_ERROR); + accessControl.DeleteEntry(1); + + NL_TEST_ASSERT(inSuite, entry.AddSubject(nullptr, kOperationalNodeId0) == CHIP_NO_ERROR); + } + NL_TEST_ASSERT(inSuite, entry.SetAuthMode(AuthMode::kCase) == CHIP_NO_ERROR); for (auto subject : validCaseSubjects) { @@ -1200,17 +1217,9 @@ void TestAclValidateAuthModeSubject(nlTestSuite * inSuite, void * inContext) // Next cases have no subject NL_TEST_ASSERT(inSuite, entry.RemoveSubject(0) == CHIP_NO_ERROR); - // Group must have subject - { - NL_TEST_ASSERT(inSuite, entry.SetAuthMode(AuthMode::kGroup) == CHIP_NO_ERROR); - NL_TEST_ASSERT(inSuite, accessControl.UpdateEntry(0, entry) != CHIP_NO_ERROR); - NL_TEST_ASSERT(inSuite, accessControl.CreateEntry(nullptr, entry) != CHIP_NO_ERROR); - accessControl.DeleteEntry(1); - } - // PASE must have subject { - NL_TEST_ASSERT(inSuite, entry.SetAuthMode(AuthMode::kGroup) == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, entry.SetAuthMode(AuthMode::kPase) == CHIP_NO_ERROR); NL_TEST_ASSERT(inSuite, accessControl.UpdateEntry(0, entry) != CHIP_NO_ERROR); NL_TEST_ASSERT(inSuite, accessControl.CreateEntry(nullptr, entry) != CHIP_NO_ERROR); accessControl.DeleteEntry(1); diff --git a/src/app/tests/suites/TestGroupMessaging.yaml b/src/app/tests/suites/TestGroupMessaging.yaml index f0c8f6f1ab74fb..3949afc555fcda 100644 --- a/src/app/tests/suites/TestGroupMessaging.yaml +++ b/src/app/tests/suites/TestGroupMessaging.yaml @@ -116,26 +116,26 @@ tests: { FabricIndex: 1, GroupId: 0x0102, GroupKeySetID: 0x01a2 }, ] - - label: "Install ACLs for test" + - label: "Install ACLs" cluster: "Access Control" command: "writeAttribute" attribute: "ACL" arguments: value: [ - # Any CASE can admin + # Any CASE can administer { - FabricIndex: 1, - Privilege: 5, - AuthMode: 2, + FabricIndex: 0, + Privilege: 5, # administer + AuthMode: 2, # case Subjects: null, Targets: null, }, - # These groups can operate + # Any group can operate { - FabricIndex: 1, - Privilege: 3, - AuthMode: 3, - Subjects: [0x0101, 0x0102], + FabricIndex: 0, + Privilege: 3, # operate + AuthMode: 3, # group + Subjects: null, Targets: null, }, ] @@ -185,3 +185,19 @@ tests: endpoint: 1 response: value: 1 + + - label: "Cleanup ACLs" + cluster: "Access Control" + command: "writeAttribute" + attribute: "ACL" + arguments: + value: [ + # Any CASE can administer + { + FabricIndex: 0, + Privilege: 5, # administer + AuthMode: 2, # case + Subjects: null, + Targets: null, + }, + ] \ No newline at end of file diff --git a/zzz_generated/chip-tool/zap-generated/test/Commands.h b/zzz_generated/chip-tool/zap-generated/test/Commands.h index e447b17a323ede..e713d331746c94 100644 --- a/zzz_generated/chip-tool/zap-generated/test/Commands.h +++ b/zzz_generated/chip-tool/zap-generated/test/Commands.h @@ -92154,8 +92154,8 @@ class TestGroupMessagingSuite : public TestCommand err = TestWriteGroupKeys_5(); break; case 6: - ChipLogProgress(chipTool, " ***** Test Step 6 : Install ACLs for test\n"); - err = TestInstallACLsForTest_6(); + ChipLogProgress(chipTool, " ***** Test Step 6 : Install ACLs\n"); + err = TestInstallACLs_6(); break; case 7: ChipLogProgress(chipTool, " ***** Test Step 7 : Group Write Attribute\n"); @@ -92182,6 +92182,10 @@ class TestGroupMessagingSuite : public TestCommand " ***** Test Step 12 : Check on/off attribute value is true after on command for endpoint 1\n"); err = TestCheckOnOffAttributeValueIsTrueAfterOnCommandForEndpoint1_12(); break; + case 13: + ChipLogProgress(chipTool, " ***** Test Step 13 : Cleanup ACLs\n"); + err = TestCleanupACLs_13(); + break; } if (CHIP_NO_ERROR != err) @@ -92193,7 +92197,7 @@ class TestGroupMessagingSuite : public TestCommand private: std::atomic_uint16_t mTestIndex; - const uint16_t mTestCount = 13; + const uint16_t mTestCount = 14; chip::Optional mNodeId; chip::Optional mCluster; @@ -92269,6 +92273,13 @@ class TestGroupMessagingSuite : public TestCommand (static_cast(context))->OnSuccessResponse_12(onOff); } + static void OnFailureCallback_13(void * context, CHIP_ERROR error) + { + (static_cast(context))->OnFailureResponse_13(error); + } + + static void OnSuccessCallback_13(void * context) { (static_cast(context))->OnSuccessResponse_13(); } + // // Tests methods // @@ -92495,7 +92506,7 @@ class TestGroupMessagingSuite : public TestCommand void OnSuccessResponse_5() { NextTest(); } - CHIP_ERROR TestInstallACLsForTest_6() + CHIP_ERROR TestInstallACLs_6() { const chip::EndpointId endpoint = mEndpoint.HasValue() ? mEndpoint.Value() : 0; chip::Controller::AccessControlClusterTest cluster; @@ -92508,24 +92519,16 @@ class TestGroupMessagingSuite : public TestCommand auto * listHolder_0 = new ListHolder(2); listFreer.add(listHolder_0); - listHolder_0->mList[0].fabricIndex = 1; + listHolder_0->mList[0].fabricIndex = 0; listHolder_0->mList[0].privilege = static_cast(5); listHolder_0->mList[0].authMode = static_cast(2); listHolder_0->mList[0].subjects.SetNull(); listHolder_0->mList[0].targets.SetNull(); - listHolder_0->mList[1].fabricIndex = 1; + listHolder_0->mList[1].fabricIndex = 0; listHolder_0->mList[1].privilege = static_cast(3); listHolder_0->mList[1].authMode = static_cast(3); - listHolder_0->mList[1].subjects.SetNonNull(); - - { - auto * listHolder_3 = new ListHolder(2); - listFreer.add(listHolder_3); - listHolder_3->mList[0] = 257ULL; - listHolder_3->mList[1] = 258ULL; - listHolder_0->mList[1].subjects.Value() = chip::app::DataModel::List(listHolder_3->mList, 2); - } + listHolder_0->mList[1].subjects.SetNull(); listHolder_0->mList[1].targets.SetNull(); aclArgument = chip::app::DataModel::List( @@ -92696,6 +92699,42 @@ class TestGroupMessagingSuite : public TestCommand NextTest(); } + + CHIP_ERROR TestCleanupACLs_13() + { + const chip::EndpointId endpoint = mEndpoint.HasValue() ? mEndpoint.Value() : 0; + chip::Controller::AccessControlClusterTest cluster; + cluster.Associate(mDevices[kIdentityAlpha], endpoint); + + ListFreer listFreer; + chip::app::DataModel::List aclArgument; + + { + auto * listHolder_0 = new ListHolder(1); + listFreer.add(listHolder_0); + + listHolder_0->mList[0].fabricIndex = 0; + listHolder_0->mList[0].privilege = static_cast(5); + listHolder_0->mList[0].authMode = static_cast(2); + listHolder_0->mList[0].subjects.SetNull(); + listHolder_0->mList[0].targets.SetNull(); + + aclArgument = chip::app::DataModel::List( + listHolder_0->mList, 1); + } + + ReturnErrorOnFailure(cluster.WriteAttribute( + aclArgument, this, OnSuccessCallback_13, OnFailureCallback_13)); + return CHIP_NO_ERROR; + } + + void OnFailureResponse_13(CHIP_ERROR error) + { + chip::app::StatusIB status(error); + ThrowFailureResponse(); + } + + void OnSuccessResponse_13() { NextTest(); } }; class TestGroupsClusterSuite : public TestCommand