Refactoring of the door lock cluster: move credential error checking …

…into a single function, fix typos
project-chip · Jul 7, 2022 · 3b1732c · 3b1732c
1 parent 4b08a6a
commit 3b1732c
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 30 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
@@ -560,23 +560,10 @@ void DoorLockServer::setCredentialCommandHandler(
     }
 
     // appclusters, 5.2.4.40.3: If the credential data length is out of bounds we should return INVALID_COMMAND
-    // OPTIMIZE: we can move range checking to the single function
-    size_t minSize, maxSize;
-    if (!getCredentialRange(commandPath.mEndpointId, credentialType, minSize, maxSize))
-    {
-        emberAfDoorLockClusterPrintln(
-            "[SetCredential] Unable to get min/max range for credential: internal error [endpointId=%d,credentialIndex=%d]",
-            commandPath.mEndpointId, credentialIndex);
-        sendSetCredentialResponse(commandObj, commandPath, DlStatus::kFailure, 0, nextAvailableCredentialSlot);
-        return;
-    }
-    if (credentialData.size() < minSize || credentialData.size() > maxSize)
+    auto status = credentialLengthWithinRange(commandPath.mEndpointId, credentialType, credentialData);
+    if (DlStatus::kSuccess != status)
     {
-        emberAfDoorLockClusterPrintln("[SetCredential] Credential data size is out of range "
-                                      "[endpointId=%d,credentialType=%u,minLength=%u,maxLength=%u,length=%u]",
-                                      commandPath.mEndpointId, to_underlying(credentialType), static_cast<unsigned int>(minSize),
-                                      static_cast<unsigned int>(maxSize), static_cast<unsigned int>(credentialData.size()));
-        sendSetCredentialResponse(commandObj, commandPath, DlStatus::kInvalidField, 0, nextAvailableCredentialSlot);
+        sendSetCredentialResponse(commandObj, commandPath, status, 0, nextAvailableCredentialSlot);
         return;
     }
 
@@ -641,8 +628,8 @@ void DoorLockServer::setCredentialCommandHandler(
     case DlDataOperationType::kAdd: {
         uint16_t createdUserIndex = 0;
 
-        DlStatus status = createCredential(commandPath.mEndpointId, fabricIdx, sourceNodeId, credentialIndex, credentialType,
-                                           existingCredential, credentialData, userIndex, userStatus, userType, createdUserIndex);
+        status = createCredential(commandPath.mEndpointId, fabricIdx, sourceNodeId, credentialIndex, credentialType,
+                                  existingCredential, credentialData, userIndex, userStatus, userType, createdUserIndex);
         sendSetCredentialResponse(commandObj, commandPath, status, createdUserIndex, nextAvailableCredentialSlot);
         return;
     }
@@ -661,14 +648,14 @@ void DoorLockServer::setCredentialCommandHandler(
         // if userIndex is NULL then we're changing the programming user PIN
         if (userIndex.IsNull())
         {
-            auto status = modifyProgrammingPIN(commandPath.mEndpointId, fabricIdx, sourceNodeId, credentialIndex, credentialType,
-                                               existingCredential, credentialData);
+            status = modifyProgrammingPIN(commandPath.mEndpointId, fabricIdx, sourceNodeId, credentialIndex, credentialType,
+                                          existingCredential, credentialData);
             sendSetCredentialResponse(commandObj, commandPath, status, 0, nextAvailableCredentialSlot);
             return;
         }
 
-        auto status = modifyCredential(commandPath.mEndpointId, fabricIdx, sourceNodeId, credentialIndex, credentialType,
-                                       existingCredential, credentialData, userIndex.Value(), userStatus, userType);
+        status = modifyCredential(commandPath.mEndpointId, fabricIdx, sourceNodeId, credentialIndex, credentialType,
+                                  existingCredential, credentialData, userIndex.Value(), userStatus, userType);
         sendSetCredentialResponse(commandObj, commandPath, status, 0, nextAvailableCredentialSlot);
         return;
     }
@@ -1358,7 +1345,8 @@ bool DoorLockServer::credentialIndexValid(chip::EndpointId endpointId, DlCredent
     return true;
 }
 
-bool DoorLockServer::getCredentialRange(chip::EndpointId endpointId, DlCredentialType type, size_t & minSize, size_t & maxSize)
+DlStatus DoorLockServer::credentialLengthWithinRange(chip::EndpointId endpointId, DlCredentialType type,
+                                                     const chip::ByteSpan & credentialData)
 {
     bool statusMin = true, statusMax = true;
     uint8_t minLen, maxLen;
@@ -1375,20 +1363,26 @@ bool DoorLockServer::getCredentialRange(chip::EndpointId endpointId, DlCredentia
         statusMax = GetAttribute(endpointId, Attributes::MaxRFIDCodeLength::Id, Attributes::MaxRFIDCodeLength::Get, maxLen);
         break;
     default:
-        return false;
+        return DlStatus::kFailure;
     }
 
     if (!statusMin || !statusMax)
     {
         ChipLogError(Zcl, "Unable to read attributes to get min/max length for credentials [endpointId=%d,credentialType=%u]",
                      endpointId, to_underlying(type));
-        return false;
+        return DlStatus::kFailure;
     }
 
-    minSize = minLen;
-    maxSize = maxLen;
+    if (credentialData.size() < minLen || credentialData.size() > maxLen)
+    {
+        emberAfDoorLockClusterPrintln("Credential data size is out of range "
+                                      "[endpointId=%d,credentialType=%u,minLength=%u,maxLength=%u,length=%u]",
+                                      endpointId, to_underlying(type), minLen, maxLen,
+                                      static_cast<unsigned int>(credentialData.size()));
+        return DlStatus::kInvalidField;
+    }
 
-    return true;
+    return DlStatus::kSuccess;
 }
 
 bool DoorLockServer::getMaxNumberOfCredentials(chip::EndpointId endpointId, DlCredentialType credentialType,

diff --git a/src/app/clusters/door-lock-server/door-lock-server.h b/src/app/clusters/door-lock-server/door-lock-server.h
@@ -154,7 +154,7 @@ class DoorLockServer
     bool credentialIndexValid(chip::EndpointId endpointId, DlCredentialType type, uint16_t credentialIndex);
     bool credentialIndexValid(chip::EndpointId endpointId, DlCredentialType type, uint16_t credentialIndex,
                               uint16_t & maxNumberOfCredentials);
-    bool getCredentialRange(chip::EndpointId endpointId, DlCredentialType type, size_t & minSize, size_t & maxSize);
+    DlStatus credentialLengthWithinRange(chip::EndpointId endpointId, DlCredentialType type, const chip::ByteSpan & credentialData);
     bool getMaxNumberOfCredentials(chip::EndpointId endpointId, DlCredentialType credentialType, uint16_t & maxNumberOfCredentials);
 
     bool findOccupiedUserSlot(chip::EndpointId endpointId, uint16_t startIndex, uint16_t & userIndex);

diff --git a/src/app/tests/suites/DL_LockUnlock.yaml b/src/app/tests/suites/DL_LockUnlock.yaml
@@ -38,7 +38,7 @@ tests:
       response:
           value: 2
 
-    - label: "Try to unlock the door without a PIN"
+    - label: "Try to lock the door without a PIN"
       command: "LockDoor"
       timedInteractionTimeoutMs: 10000