Repository navigation
zigbee-lock: centralize credential/user table updates and tighten addCredential/updateCredential validation - #3331
Merged
Conversation
|
Channel deleted. |
|
zigbee-lock_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against af2903c |
Test Results0 tests 0 ✅ 0s ⏱️ Results for commit 1cbcfe5. ♻️ This comment has been updated with latest results. |
hcarter-775
changed the title
This was referenced Nov 5, 2026
ctowns
reviewed
Nov 5, 2026
ctowns
reviewed
Nov 5, 2026
hcarter-775
force-pushed
the
simplify-reported-cred-management
branch
from
November 6, 2026 19:59
1eab2cd to
528c7fb
Compare
Centralize how the lock's credential and user tables are updated after a PIN is set on the lock, whether the PIN was set by a SmartThings command or reported by the lock (sync or programming event). addCredential - The credential always takes the next available credential slot. - userIndex 0 creates a new user for the credential; the user index is chosen when the lock confirms the code (next free user index). - A non-zero userIndex must name an existing user, and the credential is attached to it. An unknown userIndex returns invalidCommand without contacting the lock. If that user vanishes before the lock confirms, it is re-added rather than failing an accepted code. - resourceExhausted is returned up front when the credentials table is full, or when a new user is requested and the users table is full. updateCredential - Only handled when the given userIndex already owns the credentialIndex (otherwise invalidCommand); a credential can no longer be moved to another user. Only the credential name can change. Reported credentials - Codes reported by the lock without a command from us get a user and PIN credential via lock_utils.add_reported_credential. Helpers - lock_utils.add_credential_and_user(device, args, add_user) adds a credential and, when add_user is true, its user afterwards so a failed credential add never leaves a user behind. - tables.next_index accepts an optional start index. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
hcarter-775
force-pushed
the
simplify-reported-cred-management
branch
from
November 9, 2026 21:34
528c7fb to
4192673
Compare
tpmanley
reviewed
Nov 9, 2026
tpmanley
reviewed
Nov 9, 2026
Contributor
|
The commit/PR title makes it sound like this is just a refactor, but there are also behavior changes. How about this instead?
|
Contributor
|
The 61 tests failed for some reason. Do they just need to be run again? |
tpmanley
reviewed
Nov 10, 2026
| userIndex = args.userIndex, | ||
| credentialIndex = args.credentialIndex, | ||
| credentialType = args.credentialType or consts.CRED_TYPE_PIN, | ||
| credentialName = args.credentialName or name, |
Contributor
There was a problem hiding this comment.
If I'm not mistaken using credentialName is a change in behavior that isn't called out in the description. I haven't checked - is this how we handle it in the other lock drivers as well?
Contributor
Author
There was a problem hiding this comment.
No, but we should. It is an optional field that is currently basically not utilized. If it is used, it can effectively pass a userName to a newly created user without explicitly calling update. Description updated.
tpmanley
reviewed
Nov 10, 2026
Contributor
Author
Yeah, it's a failure for Zwave lock. Unrelated to this PR. |
hcarter-775
changed the title
tpmanley
approved these changes
Nov 11, 2026
ctowns
approved these changes
Nov 11, 2026
| local credentials_full = #(tables.get_state(device, "credentials") or {}) >= tables.get_max_entries(device, "credentials") | ||
| -- if we are being asked to create a new user (index 0), this also needs a free user index. | ||
| local users_full = (user_index == 0) and #(tables.get_state(device, "users") or {}) >= tables.get_max_entries(device, "users") | ||
| if credentials_full or users_full then |
Contributor
There was a problem hiding this comment.
This is inconsistent with our plan to allow credentials to be created even if the users table is full. I don't think it's a blocker, but it looks like we have two different policies for handling this case dependent on whether the add originated from a keypad or from the capability command. Worth revisiting in the follow up ticket, but I wouldn't consider this a blocker here.
Contributor
Author
There was a problem hiding this comment.
I think this is closer to the expectation, where we don't allow a cred to be made if it can't have an associated user. I think the other case is the "problem", since we can't know that ahead of time, but yes, they both speak to the deeper problem at hand.
shinasys
added a commit
to shinasys/SmartThingsEdgeDrivers
that referenced
this pull request
Nov 12, 2026
ctowns
added a commit
that referenced
this pull request
Nov 12, 2026
Match the semantics merged for zigbee-lock in #3331. addCredential - userIndex selects the user, not the credential slot. 0 creates a new user (its index is chosen when the lock confirms the code); any other value must name an existing user without a credential, otherwise invalidCommand is returned without contacting the lock. - The credential always takes the next free slot. - resourceExhausted is returned up front when the credentials table is full, or when a new user is requested and the users table is full. - A new user is named after credentialName when one is given. updateCredential - The given userIndex must already own the credential (otherwise invalidCommand); only the credential name can change. A credential can no longer be moved to another user, which left the old user without a credential and allowed pointing it at a missing user. Users table - Restore the totalUsersSupported limit on the users table. Helpers - add_user_and_credential becomes add_credential_and_user(device, args, add_user), matching zigbee-lock. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Change
Centralize how the lock's credential and user tables are updated after
a PIN is set on the lock, whether the PIN was set by a SmartThings
command or reported by the lock (sync or programming event).
addCredential
chosen when the lock confirms the code (next free user index).
is attached to it. An unknown userIndex returns invalidCommand
without contacting the lock. If that user vanishes before the lock
confirms, it is re-added rather than failing an accepted code.
full, or when a new user is requested and the users table is full.
after code is set.
updateCredential
(otherwise invalidCommand); a credential can no longer be moved to
another user. Only the credential name can change.
Reported credentials
PIN credential via lock_utils.add_reported_credential.
Helpers
credential and, when add_user is true, its user afterwards so a
failed credential add never leaves a user behind.
Summary of Completed Tests
Tested on-device. Unit tests updated