From 0d71711875e486c9961761808965d4f91c88bbea Mon Sep 17 00:00:00 2001 From: jrhees-cae <61466710+jrhees-cae@users.noreply.github.com> Date: Thu, 15 Aug 2024 08:22:41 -0600 Subject: [PATCH] [DRLK] Bugfix: return INVALID_COMMAND when attempting to add/modify (#34120) * [DRLK] Bugfix: return INVALID_COMMAND when attempting to add/modify credential from a different fabric than the User/Credential's creator fabric Add YAML test steps to verify correct behavior Fixes #34119 * Restyled by prettier-yaml * Update src/app/tests/suites/DL_UsersAndCredentials.yaml Co-authored-by: Andrei Litvin --------- Co-authored-by: Restyled.io Co-authored-by: Andrei Litvin --- .../door-lock-server/door-lock-server.cpp | 35 ++ .../tests/suites/DL_UsersAndCredentials.yaml | 333 +++++++++++++++++- 2 files changed, 365 insertions(+), 3 deletions(-) diff --git a/src/app/clusters/door-lock-server/door-lock-server.cpp b/src/app/clusters/door-lock-server/door-lock-server.cpp index 29a76853220ef0..3842130aa356f1 100644 --- a/src/app/clusters/door-lock-server/door-lock-server.cpp +++ b/src/app/clusters/door-lock-server/door-lock-server.cpp @@ -803,6 +803,19 @@ void DoorLockServer::setCredentialCommandHandler( return; } + // return INVALID_COMMAND if the accessing fabric index doesn’t match the + // CreatorFabricIndex of the credential being modified + if (existingCredential.createdBy != fabricIdx) + { + ChipLogProgress(Zcl, + "[createCredential] Unable to modify credential. Fabric index differs from creator fabric " + "[endpointId=%d,credentialIndex=%d,creatorIdx=%d,modifierIdx=%d]", + commandPath.mEndpointId, credentialIndex, existingCredential.createdBy, fabricIdx); + + sendSetCredentialResponse(commandObj, commandPath, DlStatus::kInvalidField, 0, nextAvailableCredentialSlot); + return; + } + // if userIndex is NULL then we're changing the programming user PIN if (userIndex.IsNull()) { @@ -2218,6 +2231,17 @@ DlStatus DoorLockServer::createNewCredentialAndAddItToUser(chip::EndpointId endp return DlStatus::kInvalidField; } + // return INVALID_COMMAND if the accessing fabric index doesn’t match the + // CreatorFabricIndex in the user record pointed to by UserIndex + if (user.createdBy != modifierFabricIdx) + { + ChipLogProgress(Zcl, + "[createCredential] Unable to create credential for user created by different fabric " + "[endpointId=%d,userIndex=%d,creatorIdx=%d,fabricIdx=%d]", + endpointId, userIndex, user.createdBy, modifierFabricIdx); + return DlStatus::kInvalidField; + } + // Add new credential to the user auto status = addCredentialToUser(endpointId, modifierFabricIdx, userIndex, credential); if (DlStatus::kSuccess != status) @@ -2338,6 +2362,17 @@ DlStatus DoorLockServer::modifyCredentialForUser(chip::EndpointId endpointId, ch return DlStatus::kFailure; } + // return INVALID_COMMAND if the accessing fabric index doesn’t match the + // CreatorFabricIndex in the user record pointed to by UserIndex + if (user.createdBy != modifierFabricIdx) + { + ChipLogProgress(Zcl, + "[createCredential] Unable to modify credential for user created by different fabric " + "[endpointId=%d,userIndex=%d,creatorIdx=%d,fabricIdx=%d]", + endpointId, userIndex, user.createdBy, modifierFabricIdx); + return DlStatus::kInvalidField; + } + for (size_t i = 0; i < user.credentials.size(); ++i) { // appclusters, 5.2.4.40: user should already be associated with given credential diff --git a/src/app/tests/suites/DL_UsersAndCredentials.yaml b/src/app/tests/suites/DL_UsersAndCredentials.yaml index 5030145412db2f..6edbef1d645aa9 100644 --- a/src/app/tests/suites/DL_UsersAndCredentials.yaml +++ b/src/app/tests/suites/DL_UsersAndCredentials.yaml @@ -18,6 +18,18 @@ config: nodeId: 0x12344321 cluster: "Door Lock" endpoint: 1 + payload: + type: char_string + defaultValue: "MT:-24J0AFN00KA0648G00" + discriminator: + type: int16u + defaultValue: 3840 + waitAfterCommissioning: + type: int16u + defaultValue: 1000 + PakeVerifier: + type: octet_string + defaultValue: "hex:b96170aae803346884724fe9a3b287c30330c2a660375d17bb205a8cf1aecb350457f8ab79ee253ab6a8e46bb09e543ae422736de501e3db37d441fe344920d09548e4c18240630c4ff4913c53513839b7c07fcc0627a1b8573a149fcd1fa466cf" tests: - label: "Wait for the commissioned device to be retrieved" @@ -64,6 +76,57 @@ tests: saveAs: NumberOfTotalUsersSupportedValue value: 10 + # + # Commission to second fabric to facilitate testing SetCredential/SetUser fabric restrictions + # + - label: "Open Commissioning Window from alpha" + endpoint: 0 + cluster: "Administrator Commissioning" + command: "OpenCommissioningWindow" + timedInteractionTimeoutMs: 10000 + arguments: + values: + - name: "CommissioningTimeout" + value: 180 + - name: "PAKEPasscodeVerifier" + value: PakeVerifier + - name: "Discriminator" + value: discriminator + - name: "Iterations" + value: 1000 + - name: "Salt" + value: "SPAKE2P Key Salt" + + - label: "Waiting after opening commissioning window" + cluster: "DelayCommands" + command: "WaitForMs" + arguments: + values: + - name: "ms" + value: waitAfterCommissioning + + - label: "Commission from TH2" + identity: "beta" + endpoint: 0 + cluster: "CommissionerCommands" + command: "PairWithCode" + arguments: + values: + - name: "nodeId" + value: nodeId + - name: "payload" + value: payload + + - label: "Wait for the commissioned device to be retrieved for TH2" + endpoint: 0 + identity: beta + cluster: "DelayCommands" + command: "WaitForCommissionee" + arguments: + values: + - name: "nodeId" + value: nodeId + - label: "Read fails for user with index 0" command: "GetUser" arguments: @@ -448,6 +511,136 @@ tests: - name: "NextUserIndex" value: null + - label: + "Modify UserStatus, UserType, CredentialRule for existing user from + different fabric" + command: "SetUser" + identity: "beta" + timedInteractionTimeoutMs: 10000 + arguments: + values: + - name: "OperationType" + value: 2 + - name: "UserIndex" + value: 1 + - name: "UserName" + value: null + - name: "UserUniqueID" + value: null + - name: "UserStatus" + value: 3 + - name: "UserType" + value: 6 + - name: "CredentialRule" + value: 2 + + - label: "Read the modified user back and verify its fields" + command: "GetUser" + arguments: + values: + - name: "UserIndex" + value: 1 + response: + values: + - name: "UserIndex" + value: 1 + - name: "UserName" + value: "test_user" + - name: "UserUniqueID" + value: 0x1BCDA0A0 + - name: "UserStatus" + value: 3 + - name: "UserType" + value: 6 + - name: "CredentialRule" + value: 2 + - name: "Credentials" + value: [] + - name: "CreatorFabricIndex" + value: 1 + - name: "LastModifiedFabricIndex" + value: 2 + - name: "NextUserIndex" + value: null + + - label: + "Attempt to modify UserName for existing user from different fabric" + command: "SetUser" + identity: "beta" + timedInteractionTimeoutMs: 10000 + arguments: + values: + - name: "OperationType" + value: 2 + - name: "UserIndex" + value: 1 + - name: "UserName" + value: "test_fab2" + - name: "UserUniqueID" + value: null + - name: "UserStatus" + value: null + - name: "UserType" + value: null + - name: "CredentialRule" + value: null + response: + error: INVALID_COMMAND + + - label: + "Attempt to modify userUniqueId for existing user from different + fabric" + command: "SetUser" + identity: "beta" + timedInteractionTimeoutMs: 10000 + arguments: + values: + - name: "OperationType" + value: 2 + - name: "UserIndex" + value: 1 + - name: "UserName" + value: null + - name: "UserUniqueID" + value: 0x1234ABCD + - name: "UserStatus" + value: null + - name: "UserType" + value: null + - name: "CredentialRule" + value: null + response: + error: INVALID_COMMAND + + - label: "Read the modified user back and verify its fields" + command: "GetUser" + arguments: + values: + - name: "UserIndex" + value: 1 + response: + values: + - name: "UserIndex" + value: 1 + - name: "UserName" + value: "test_user" + - name: "UserUniqueID" + value: 0x1BCDA0A0 + - name: "UserStatus" + value: 3 + - name: "UserType" + value: 6 + - name: "CredentialRule" + value: 2 + - name: "Credentials" + value: [] + - name: "CreatorFabricIndex" + value: 1 + - name: "LastModifiedFabricIndex" + value: 2 + - name: "NextUserIndex" + value: null + - label: "Add another user with non-default fields" command: "SetUser" timedInteractionTimeoutMs: 10000 @@ -1327,6 +1520,83 @@ tests: - name: "NextCredentialIndex" value: 2 + - label: + "Attempt to create new RFID credential from different fabric and add + it to existing user" + command: "SetCredential" + identity: "beta" + timedInteractionTimeoutMs: 10000 + arguments: + values: + - name: "OperationType" + value: 0 + - name: "Credential" + value: { CredentialType: 2, CredentialIndex: 1 } + - name: "CredentialData" + value: "rfid_data_123456" + - name: "UserIndex" + value: 1 + - name: "UserStatus" + value: null + - name: "UserType" + value: null + response: + values: + - name: "Status" + value: 0x85 + - name: "UserIndex" + value: null + - name: "NextCredentialIndex" + value: 2 + + - label: "Verify user has not been modified" + command: "GetUser" + arguments: + values: + - name: "UserIndex" + value: 1 + response: + values: + - name: "UserIndex" + value: 1 + - name: "UserName" + value: "" + - name: "UserUniqueID" + value: null + - name: "UserStatus" + value: 1 + - name: "UserType" + value: 0 + - name: "CredentialRule" + value: 0 + - name: "Credentials" + value: [{ CredentialType: 1, CredentialIndex: 1 }] + - name: "CreatorFabricIndex" + value: 1 + - name: "LastModifiedFabricIndex" + value: 1 + - name: "NextUserIndex" + value: null + + - label: "Verify no credential has been created" + command: "GetCredentialStatus" + arguments: + values: + - name: "Credential" + value: { CredentialType: 2, CredentialIndex: 1 } + response: + values: + - name: "CredentialExists" + value: false + - name: "UserIndex" + value: null + - name: "CreatorFabricIndex" + value: null + - name: "LastModifiedFabricIndex" + value: null + - name: "NextCredentialIndex" + value: null + - label: "Create new RFID credential and add it to existing user" command: "SetCredential" timedInteractionTimeoutMs: 10000 @@ -1405,6 +1675,63 @@ tests: - name: "NextCredentialIndex" value: null + - label: + "Attempt to modify credentialData of existing RFID credential from + different fabric" + command: "SetCredential" + identity: "beta" + timedInteractionTimeoutMs: 10000 + arguments: + values: + - name: "OperationType" + value: 2 + - name: "Credential" + value: { CredentialType: 2, CredentialIndex: 1 } + - name: "CredentialData" + value: "rfid_data_654321" + - name: "UserIndex" + value: 1 + - name: "UserStatus" + value: null + - name: "UserType" + value: null + response: + values: + - name: "Status" + value: 0x85 + - name: "UserIndex" + value: null + - name: "NextCredentialIndex" + value: 2 + + - label: + "Verify that credential was not changed by attempting to create new + credential with unmodified data" + command: "SetCredential" + timedInteractionTimeoutMs: 10000 + arguments: + values: + - name: "OperationType" + value: 0 + - name: "Credential" + value: { CredentialType: 2, CredentialIndex: 2 } + - name: "CredentialData" + value: "rfid_data_123456" + - name: "UserIndex" + value: null + - name: "UserStatus" + value: null + - name: "UserType" + value: null + response: + values: + - name: "Status" + value: 0x02 + - name: "UserIndex" + value: null + - name: "NextCredentialIndex" + value: 3 + - label: "Create new RFID credential and user with index 0 fails" command: "SetCredential" timedInteractionTimeoutMs: 10000 @@ -2054,10 +2381,10 @@ tests: - label: "Make sure a LockUserChange event was generated" command: "readEvent" event: "LockUserChange" - # I wish there were a way to not hardcode this 25, but it's experimentally - # determined: doing a read without an eventNumber filter here shows 24 + # I wish there were a way to not hardcode this 27, but it's experimentally + # determined: doing a read without an eventNumber filter here shows 26 # LockUserChange events before this removal. - eventNumber: 25 + eventNumber: 27 response: value: {