From 78b61379221da5675e6f1bf3b76ea5831547b1fb Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 9 Nov 2022 03:44:57 -0500 Subject: [PATCH] Don't use EmberAfPluginDoorLockUserInfo if findUserIndexByCredential failed. (#23467) A few changes here: * Initialize the user status in EmberAfPluginDoorLockUserInfo to Available, to represent no user. * If we fail to find a user for the given PIN, don't include a bogus user index and credential index in the operation error event we send. * If we fail to find a user for the given PIN, send InvalidCredential as the operation error, not Unspecified. --- .../door-lock-server/door-lock-server.cpp | 50 ++++++++++++------- .../door-lock-server/door-lock-server.h | 12 ++--- 2 files changed, 39 insertions(+), 23 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 4b46b80ffe6f5f..a1e5307b66169d 100644 --- a/src/app/clusters/door-lock-server/door-lock-server.cpp +++ b/src/app/clusters/door-lock-server/door-lock-server.cpp @@ -3234,10 +3234,10 @@ bool DoorLockServer::HandleRemoteLockOperation(chip::app::CommandHandler * comma EndpointId endpoint = commandPath.mEndpointId; DlOperationError reason = DlOperationError::kUnspecified; - uint16_t pinUserIdx = 0; - uint16_t pinCredIdx = 0; - bool success = false; - bool sendEvent = true; + Nullable pinUserIdx; // Will get set to non-null if we find a user for the PIN. + Optional pinCredIdx; // Will get set to a value if the PIN is one we know about. + bool success = false; + bool sendEvent = true; auto currentTime = chip::System::SystemClock().GetMonotonicTimestamp(); @@ -3253,9 +3253,9 @@ bool DoorLockServer::HandleRemoteLockOperation(chip::app::CommandHandler * comma VerifyOrExit(nullptr != endpointContext, ChipLogError(Zcl, "Failed to get endpoint index for cluster [endpoint=%d]", endpoint)); if (endpointContext->lockoutEndTimestamp >= currentTime) { - emberAfDoorLockClusterPrintln("Rejecting unlock command -- lockout is in action [endpoint=%d,lockoutEnd=%u,currentTime=%u]", - endpoint, static_cast(endpointContext->lockoutEndTimestamp.count()), - static_cast(currentTime.count())); + emberAfDoorLockClusterPrintln( + "Rejecting remote lock operation -- lockout is in action [endpoint=%d,lockoutEnd=%u,currentTime=%u]", endpoint, + static_cast(endpointContext->lockoutEndTimestamp.count()), static_cast(currentTime.count())); sendEvent = false; goto exit; } @@ -3272,14 +3272,27 @@ bool DoorLockServer::HandleRemoteLockOperation(chip::app::CommandHandler * comma // Look up the user index and credential index -- it should be used in the Lock Operation event EmberAfPluginDoorLockUserInfo user; - findUserIndexByCredential(endpoint, DlCredentialType::kPin, pinCode.Value(), pinUserIdx, pinCredIdx, user); + uint16_t userIdx; + uint16_t credIdx; + if (findUserIndexByCredential(endpoint, DlCredentialType::kPin, pinCode.Value(), userIdx, credIdx, user)) + { + pinUserIdx.SetNonNull(userIdx); + pinCredIdx.Emplace(credIdx); + } + else + { + emberAfDoorLockClusterPrintln("Rejecting lock operation: unknown PIN provided [endpoint=%d, lock_op=%d]", endpoint, + to_underlying(opType)); + reason = DlOperationError::kInvalidCredential; + goto exit; + } // If the user status is OccupiedDisabled we should deny the access and send out the appropriate event VerifyOrExit(user.userStatus != DlUserStatus::kOccupiedDisabled, { reason = DlOperationError::kDisabledUserDenied; emberAfDoorLockClusterPrintln( "Unable to perform remote lock operation: user is disabled [endpoint=%d, lock_op=%d, userIndex=%d]", endpoint, - to_underlying(opType), pinUserIdx); + to_underlying(opType), userIdx); }); } else @@ -3306,12 +3319,13 @@ bool DoorLockServer::HandleRemoteLockOperation(chip::app::CommandHandler * comma // credentials check succeeded, try to lock/unlock door success = opHandler(endpoint, pinCode, reason); + // The app should trigger the lock state change as it may take a while before the lock actually locks/unlocks +exit: if (!success && reason == DlOperationError::kInvalidCredential) { TrackWrongCodeEntry(endpoint); } - // The app should trigger the lock state change as it may take a while before the lock actually locks/unlocks -exit: + // Send command response emberAfSendImmediateDefaultResponse(success ? EMBER_ZCL_STATUS_SUCCESS : EMBER_ZCL_STATUS_FAILURE); @@ -3321,20 +3335,22 @@ bool DoorLockServer::HandleRemoteLockOperation(chip::app::CommandHandler * comma return success; } - // Send LockOperation/LockOperationError event - LockOpCredentials foundCred[] = { { DlCredentialType::kPin, pinCredIdx } }; + // Send LockOperation/LockOperationError event. The credential index in + // foundCred will be filled in if we actually have a value to fill in. + LockOpCredentials foundCred[] = { { DlCredentialType::kPin, UINT16_MAX } }; LockOpCredentials * credList = nullptr; size_t credListSize = 0; // appclusters.pdf 5.3.5.3, 5.3.5.4: // The list of credentials used in performing the lock operation. This SHALL be null if no credentials were involved. - if (pinCode.HasValue()) + if (pinCode.HasValue() && pinCredIdx.HasValue()) { - credList = foundCred; - credListSize = 1; + foundCred[0].credentialIndex = pinCredIdx.Value(); + credList = foundCred; + credListSize = 1; } - SendLockOperationEvent(endpoint, opType, DlOperationSource::kRemote, reason, Nullable(pinUserIdx), + SendLockOperationEvent(endpoint, opType, DlOperationSource::kRemote, reason, pinUserIdx, Nullable(getFabricIndex(commandObj)), Nullable(getNodeId(commandObj)), credList, credListSize, success); return success; diff --git a/src/app/clusters/door-lock-server/door-lock-server.h b/src/app/clusters/door-lock-server/door-lock-server.h index c7ddf369f8871a..67314c63a158f6 100644 --- a/src/app/clusters/door-lock-server/door-lock-server.h +++ b/src/app/clusters/door-lock-server/door-lock-server.h @@ -571,12 +571,12 @@ struct EmberAfPluginDoorLockCredentialInfo */ struct EmberAfPluginDoorLockUserInfo { - chip::CharSpan userName; /**< Name of the user. */ - chip::Span credentials; /**< Credentials that are associated with user (without data).*/ - uint32_t userUniqueId; /**< Unique user identifier. */ - DlUserStatus userStatus; /**< Status of the user slot (available/occupied). */ - DlUserType userType; /**< Type of the user. */ - DlCredentialRule credentialRule; /**< Number of supported credentials. */ + chip::CharSpan userName; /**< Name of the user. */ + chip::Span credentials; /**< Credentials that are associated with user (without data).*/ + uint32_t userUniqueId; /**< Unique user identifier. */ + DlUserStatus userStatus = DlUserStatus::kAvailable; /**< Status of the user slot (available/occupied). */ + DlUserType userType; /**< Type of the user. */ + DlCredentialRule credentialRule; /**< Number of supported credentials. */ DlAssetSource creationSource; chip::FabricIndex createdBy; /**< ID of the fabric that created the user. */