Skip to content

Comments

Implement a dialog for setting new device PIN#135

Draft
msirringhaus wants to merge 3 commits intolinux-credentials:mainfrom
msirringhaus:setpinpage
Draft

Implement a dialog for setting new device PIN#135
msirringhaus wants to merge 3 commits intolinux-credentials:mainfrom
msirringhaus:setpinpage

Conversation

@msirringhaus
Copy link
Collaborator

@msirringhaus msirringhaus commented Feb 11, 2026

Only NFC and USB for now. Hybrid makes no sense, and I don't have anything to test BLE right now. USB and NFC both work.

Flow of how this works:
Upon "PinNotSet"-error during registration, there will be a page telling the user that the RP requires additional protection on the device, e.g. a PIN, which will affect the whole device. There are two buttons: "Close", "Set PIN on device". Close closes the window. "Set PIN on device" brings the user to another page that offers 2 password-entry fields. "Continue" will be disabled until both fields are non-empty and identical. On pressing "Continue", the PIN is set for the device.

Some known issues with this implementation:

  1. On error (e.g. PinTooShort) the window just swaps to the general error page and the user needs to close and start again. [solved. User is now dropped to the page with the "Set PIN on device"-button, with error text about PIN policy violation]
  2. On success, the window simply closes. The PIN is now set and the request needs to be issued again. But this is not really communicated in a good way. This is actually a general bug. The window is supposed to stay open for a bit in general to show "Done", but in practice gets closed right away because some cancel-request overrides it.
  3. No tests. Not sure how to test that.
  4. No check yet if the device even supports a PIN, before we show this dialog. We would need the deviceInfo on the UI-side for this, which we currently don't have. Or an additional API call to ask credentialsd what kind of UV the device supports? For this specific case, it's ok, because CTAP2_ERR_PIN_NOT_SET is only ever returned by the device if it supports a PIN. But e.g. biometrics enrollment wouldn't work without having the device-info everywhere.
  5. I'm also not a big fan on how credentialsd handles the event, but also couldn't come up with a better approach. It basically starts a fresh "Select the device you want, then continue with request 'Setting new PIN' on that device"-cycle, which may result in weird UX, if multiple devices are plugged in. But I also don't really know how to go directly for the previously selected device.

Thus, I marked this PR a draft for now. Maybe @iinuwa has some suggestions for some of these problems?

@msirringhaus msirringhaus requested a review from iinuwa February 11, 2026 13:54
@iinuwa
Copy link
Member

iinuwa commented Feb 20, 2026

I'm also not a big fan on how credentialsd handles the event, but also couldn't come up with a better approach. It basically starts a fresh "Select the device you want, then continue with request 'Setting new PIN' on that device"-cycle, which may result in weird UX, if multiple devices are plugged in. But I also don't really know how to go directly for the previously selected device.

Yeah, this is getting pretty clunky to add new features into.

I think though that we're treating PinNotSet as a terminal condition, but it feels more like a branch of user interaction.

(In typing this up, I've already lost the thread... I need some sort of diagram for this.)

But maybe we can play around with

  • catching the CtapError::PINNotSet error in this match block
  • then emitting a new UsbUvMessage::NeedsPinSet with a response channel like we do for NeedsPin, (we might need to merge a new channel with the existing uv_update broadcast receiver we get from the authenticator in order to make this work cleanly)
  • that will get handled in process_user_interaction, which we will handle by forwarding that as a new UsbStateInternal::NeedsPinSet(HidDevice).
  • In the top-level UsbStateInternal::NeedsPinSet(HidDevice) state, we can wait for the SetPin message to return (or client cancellation once that's implemented), send it to the authenticator, transition back to Connected(HidDevice) on success, or retry setting the PIN if it didn't work for some reason.

This doesn't give us everything we need (for example, we probably want the UI to be aware that we're redoing the request after the PIN... maybe the UI can just keep that state on their end?), and might not even work. But maybe that's worth playing with.

We may need to shuffle some things around. E.g., instead of storing then spawning a new handle_events task after re-entering the Connected state might result in some weirdness. Maybe that needs to be moved up outside of the state machine loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants