Skip to content

zwave lock: ensure the "wildcard" 0 index is implemented safely #3317

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hcarter-775 merged 1 commit into from
Nov 3, 2026
Merged

zwave lock: ensure the "wildcard" 0 index is implemented safely #3317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 19 additions & 13 deletions drivers/SmartThings/zwave-lock/src/lock_handlers/capabilities.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,12 @@ function CapabilityHandlers.add_credential(driver, device, command)
end

function CapabilityHandlers.update_credential(driver, device, command)
-- Index 0 is the Z-Wave "all users" wildcard and is never a valid credential to update.
if command.args.credentialIndex == 0 then
lock_utils.emit_command_result(device, capabilities.lockCredentials, consts.LOCK_CREDENTIALS.UPDATE, consts.COMMAND_RESULT.INVALID_COMMAND)
return
end

if lock_utils.is_device_busy(device) then
lock_utils.emit_command_result(device, capabilities.lockCredentials, consts.LOCK_CREDENTIALS.UPDATE, consts.COMMAND_RESULT.BUSY)
return
Expand All @@ -165,6 +171,12 @@ function CapabilityHandlers.update_credential(driver, device, command)
end

function CapabilityHandlers.delete_credential(driver, device, command)
-- Index 0 is the Z-Wave "all users" wildcard; clearing everything must go through deleteAllCredentials.
if command.args.credentialIndex == 0 then
lock_utils.emit_command_result(device, capabilities.lockCredentials, consts.LOCK_CREDENTIALS.DELETE, consts.COMMAND_RESULT.INVALID_COMMAND)
return
end

local cmd_in_progress = device:get_field(consts.DRIVER_STATE.COMMAND_IN_PROGRESS)

if cmd_in_progress == consts.LOCK_USERS.DELETE then
Expand Down Expand Up @@ -199,19 +211,13 @@ function CapabilityHandlers.delete_all_credentials(driver, device, command)
local cmd_in_progress = device:get_field(consts.DRIVER_STATE.COMMAND_IN_PROGRESS)

local send_clear_commands = function()
local credentials = tables.get_state(device, "credentials") or {}
local delay = 0
for _, credential in ipairs(credentials) do
device.thread:call_with_delay(delay, function()
device:send(UserCode:Set({
user_identifier = credential.credentialIndex,
user_id_status = UserCode.user_id_status.AVAILABLE
}))
end)
delay = delay + 2
end
-- Delayed cleanup: ensure all tables are cleared after individual responses complete
device.thread:call_with_delay(delay + 4, function()
-- User Identifier 0 with status AVAILABLE tells the lock to clear every user code slot

@cjswedes cjswedes Nov 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am slightly concerned that we have devices in the field that do not actually support this functionality even though it is required by the spec. There is some risk to this change, and from it we get fewer network commands for this operation. We could skip this part of the change since it is not required for the bug fix.

@hcarter-775 hcarter-775 Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I was thinking about that too, but 1. This was never a function before, so it's not like we'd be breaking backwards compatibility on our platform (so we could switch it in the future if we see problems), and 2. It is in the spec and has no statements about being version-controlled (zwave is usually quite explicit about its versioning differences).

I am ok with reverting this, since like you said it's not actually required, but for the above reasons I wanted to give a modicum of pushback here.

@cjswedes cjswedes Nov 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to leave it in too, just bringing up the added risk because unicorn devices have gotten me almost every time I try to make a "lets change to use a different, more performant part of the spec" type change like this. I think its mostly been in zigbee though; Z-Wave certification was always better at enforcing devices following all parts of the spec.

@hcarter-775 hcarter-775 Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the description with testing done a v1 Zwave lock. Imo, if any of them are going to fail to use 0 correctly, it would be a v1. Yet, it worked! Agreed on the Zwave/Zigbee distinction for sure.

device:send(UserCode:Set({
user_identifier = 0,
user_id_status = UserCode.user_id_status.AVAILABLE
}))
-- Delayed cleanup: ensure all tables are cleared after the lock has processed the command
device.thread:call_with_delay(4, function()
if cmd_in_progress == consts.LOCK_USERS.DELETE_ALL then
tables.delete_all_entries(device, "users")
lock_utils.emit_command_result(device, capabilities.lockUsers, consts.LOCK_USERS.DELETE_ALL, consts.COMMAND_RESULT.SUCCESS)
Expand Down
59 changes: 51 additions & 8 deletions drivers/SmartThings/zwave-lock/src/test/test_lock_credentials_commands.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,7 @@ test.register_coroutine_test(
-- ============================================================================

test.register_coroutine_test(
"deleteAllCredentials: sends ClearAllPINCodes and emits success when the lock returns PASS",
"deleteAllCredentials: sends a single UserCode Set for identifier 0 and emits success",
function()
seed_users({
{ userIndex = 1, userName = "Alice", userType = "guest" },
Expand All @@ -519,14 +519,12 @@ test.register_coroutine_test(
{ capability = capabilities.lockCredentials.ID, command = "deleteAllCredentials", args = {} },
})

-- Per-credential clears fire at 2 * (i - 1) s; final cleanup at 2 * #credentials + 4 s.
test.timer.__create_and_queue_test_time_advance_timer(0, "oneshot")
test.timer.__create_and_queue_test_time_advance_timer(6, "oneshot")
expect_clear_pin_code(1)
test.mock_time.advance_time(0)
-- A single wildcard clear (identifier 0) is sent immediately; final cleanup fires at 4 s.
test.timer.__create_and_queue_test_time_advance_timer(4, "oneshot")
expect_clear_pin_code(0)
test.wait_for_events()

test.mock_time.advance_time(6)
test.mock_time.advance_time(4)
test.socket.capability:__expect_send(
mock_device:generate_test_message("main",
capabilities.lockCredentials.credentials({}, { visibility = { displayed = false } }))
Expand Down Expand Up @@ -578,7 +576,8 @@ test.register_coroutine_test(
mock_device.id,
{ capability = capabilities.lockCredentials.ID, command = "deleteAllCredentials", args = {} },
})
-- deleteAllCredentials sends individual UserCode:Set AVAILABLE commands
-- deleteAllCredentials sends a single UserCode:Set (identifier 0) AVAILABLE command
expect_clear_pin_code(0)
test.wait_for_events()

-- Second deleteAllCredentials while first is pending → busy
Expand All @@ -600,4 +599,48 @@ test.register_coroutine_test(
}
)

test.register_coroutine_test(
"deleteCredential: rejects credentialIndex 0 with invalidCommand and sends no z-wave command",
function()
test.socket.capability:__queue_receive({
mock_device.id,
{ capability = capabilities.lockCredentials.ID, command = "deleteCredential",
args = { 0, "pin" } },
})
test.socket.capability:__expect_send(
mock_device:generate_test_message("main",
capabilities.lockCredentials.commandResult(
{ commandName = "deleteCredential", statusCode = "invalidCommand" },
{ state_change = true, visibility = { displayed = false } }
))
)
test.wait_for_events()
end,
{
min_api_version = 15
}
)

test.register_coroutine_test(
"updateCredential: rejects credentialIndex 0 with invalidCommand and sends no z-wave command",
function()
test.socket.capability:__queue_receive({
mock_device.id,
{ capability = capabilities.lockCredentials.ID, command = "updateCredential",
args = { 1, 0, "pin", "1234" } },
})
test.socket.capability:__expect_send(
mock_device:generate_test_message("main",
capabilities.lockCredentials.commandResult(
{ commandName = "updateCredential", statusCode = "invalidCommand" },
{ state_change = true, visibility = { displayed = false } }
))
)
test.wait_for_events()
end,
{
min_api_version = 15
}
)

test.run_registered_tests()
14 changes: 4 additions & 10 deletions drivers/SmartThings/zwave-lock/src/test/test_lock_users_commands.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,7 @@ test.register_coroutine_test(
-- ============================================================================

test.register_coroutine_test(
"deleteAllUsers: sends ClearAllPINCodes and emits success for both users and credentials on PASS",
"deleteAllUsers: sends a single UserCode Set for identifier 0 and emits success for both users and credentials on PASS",
function()
seed_users({
{ userIndex = 1, userName = "Alice", userType = "guest" },
Expand All @@ -485,19 +485,13 @@ test.register_coroutine_test(
{ capability = capabilities.lockUsers.ID, command = "deleteAllUsers", args = {} },
})

test.timer.__create_and_queue_test_time_advance_timer(0, "oneshot")
test.timer.__create_and_queue_test_time_advance_timer(0.5, "oneshot")
test.timer.__create_and_queue_test_time_advance_timer(8, "oneshot")
test.mock_time.advance_time(0)
expect_clear_pin_code(1)
test.wait_for_events()
test.mock_time.advance_time(0.5)
expect_clear_pin_code(2)
test.timer.__create_and_queue_test_time_advance_timer(4, "oneshot")
expect_clear_pin_code(0)
test.wait_for_events()

-- The legacy subdriver consumes deletion notifications for this mock profile; advance
-- to the driver's cleanup timer, which clears both tables and reports success.
test.mock_time.advance_time(8)
test.mock_time.advance_time(4)
test.socket.capability:__expect_send(
mock_device:generate_test_message("main",
capabilities.lockUsers.users({}, { visibility = { displayed = false } }))
Expand Down
Loading