From 168be649f8ffcc003cd868b92048705e86ed8a34 Mon Sep 17 00:00:00 2001
From: Jon Rhees <jrhees@caengineering.com>
Date: Thu, 27 Jun 2024 14:54:35 -0600
Subject: [PATCH 1/3] [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
---
.../door-lock-server/door-lock-server.cpp | 35 ++
.../tests/suites/DL_UsersAndCredentials.yaml | 328 +++++++++++++++++-
2 files changed, 361 insertions(+), 2 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 528cbd6eb0971d..0cebca44a53e5c 100644
--- a/src/app/clusters/door-lock-server/door-lock-server.cpp
+++ b/src/app/clusters/door-lock-server/door-lock-server.cpp
@@ -788,6 +788,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())
{
@@ -2192,6 +2205,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)
@@ -2312,6 +2336,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 credentialIndex
diff --git a/src/app/tests/suites/DL_UsersAndCredentials.yaml b/src/app/tests/suites/DL_UsersAndCredentials.yaml
index 5030145412db2f..c4a0c9d058497f 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,131 @@ 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 +1515,81 @@ 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 +1668,67 @@ 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 +2378,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
+ # 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 24
# LockUserChange events before this removal.
- eventNumber: 25
+ eventNumber: 27
response:
value:
{
From 6ad8e6f1f3676a43f684607346c0287472bed286 Mon Sep 17 00:00:00 2001
From: "Restyled.io" <commits@restyled.io>
Date: Thu, 27 Jun 2024 20:59:07 +0000
Subject: [PATCH 2/3] Restyled by prettier-yaml
---
.../tests/suites/DL_UsersAndCredentials.yaml | 29 ++++++++++---------
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/src/app/tests/suites/DL_UsersAndCredentials.yaml b/src/app/tests/suites/DL_UsersAndCredentials.yaml
index c4a0c9d058497f..15f389f43889e9 100644
--- a/src/app/tests/suites/DL_UsersAndCredentials.yaml
+++ b/src/app/tests/suites/DL_UsersAndCredentials.yaml
@@ -511,7 +511,9 @@ tests:
- name: "NextUserIndex"
value: null
- - label: "Modify UserStatus, UserType, CredentialRule for existing user from different fabric"
+ - label:
+ "Modify UserStatus, UserType, CredentialRule for existing user from
+ different fabric"
command: "SetUser"
identity: "beta"
timedInteractionTimeoutMs: 10000
@@ -561,7 +563,8 @@ tests:
- name: "NextUserIndex"
value: null
- - label: "Attempt to modify UserName for existing user from different fabric"
+ - label:
+ "Attempt to modify UserName for existing user from different fabric"
command: "SetUser"
identity: "beta"
timedInteractionTimeoutMs: 10000
@@ -584,7 +587,9 @@ tests:
response:
error: INVALID_COMMAND
- - label: "Attempt to modify userUniqueId for existing user from different fabric"
+ - label:
+ "Attempt to modify userUniqueId for existing user from different
+ fabric"
command: "SetUser"
identity: "beta"
timedInteractionTimeoutMs: 10000
@@ -1515,7 +1520,9 @@ tests:
- name: "NextCredentialIndex"
value: 2
- - label: "Attempt to create new RFID credential from different fabric and add it to existing user"
+ - label:
+ "Attempt to create new RFID credential from different fabric and add
+ it to existing user"
command: "SetCredential"
identity: "beta"
timedInteractionTimeoutMs: 10000
@@ -1668,9 +1675,9 @@ tests:
- name: "NextCredentialIndex"
value: null
-
-
- - label: "Attempt to modify credentialData of existing RFID credential from different fabric"
+ - label:
+ "Attempt to modify credentialData of existing RFID credential from
+ different fabric"
command: "SetCredential"
identity: "beta"
timedInteractionTimeoutMs: 10000
@@ -1698,8 +1705,8 @@ tests:
value: 2
- label:
- "Verify that credential was not changed by attempting to create
- new credential with unmodified data"
+ "Verify that credential was not changed by attempting to create new
+ credential with unmodified data"
command: "SetCredential"
timedInteractionTimeoutMs: 10000
arguments:
@@ -1725,10 +1732,6 @@ tests:
- name: "NextCredentialIndex"
value: 3
-
-
-
-
- label: "Create new RFID credential and user with index 0 fails"
command: "SetCredential"
timedInteractionTimeoutMs: 10000
From 28d1fcddcdf8c16a4039836e20afb081d82af18d Mon Sep 17 00:00:00 2001
From: jrhees-cae <61466710+jrhees-cae@users.noreply.github.com>
Date: Mon, 8 Jul 2024 08:12:42 -0600
Subject: [PATCH 3/3] Update src/app/tests/suites/DL_UsersAndCredentials.yaml
Co-authored-by: Andrei Litvin <andy314@gmail.com>
---
src/app/tests/suites/DL_UsersAndCredentials.yaml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/app/tests/suites/DL_UsersAndCredentials.yaml b/src/app/tests/suites/DL_UsersAndCredentials.yaml
index 15f389f43889e9..6edbef1d645aa9 100644
--- a/src/app/tests/suites/DL_UsersAndCredentials.yaml
+++ b/src/app/tests/suites/DL_UsersAndCredentials.yaml
@@ -2382,7 +2382,7 @@ tests:
command: "readEvent"
event: "LockUserChange"
# 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 24
+ # determined: doing a read without an eventNumber filter here shows 26
# LockUserChange events before this removal.
eventNumber: 27
response: