Add support for NFC #103
Add support for NFC #103
Conversation
iinuwa
left a comment
iinuwa
left a comment
There was a problem hiding this comment.
This looks good! A couple of doc comments need to be updated, and then I'm fine with merging this. Thanks for bringing this to the finish line!
| /// Awaiting FIDO USB device to be plugged in. | ||
| Waiting, | ||
|
|
||
| /// USB device connected, prompt user to tap |
There was a problem hiding this comment.
| /// USB device connected, prompt user to tap | |
| /// NFC device connected, prompt user to tap |
credentialsd-common/src/model.rs
Outdated
| #[default] | ||
| Idle, | ||
|
|
||
| /// Awaiting FIDO USB device to be plugged in. |
There was a problem hiding this comment.
| /// Awaiting FIDO USB device to be plugged in. | |
| /// Awaiting FIDO NFC device to connect. |
credentialsd-common/src/model.rs
Outdated
| /// Used to share public state between credential service and UI. | ||
| #[derive(Clone, Debug, Default)] | ||
| pub enum NfcState { | ||
| /// Not polling for FIDO USB device. |
There was a problem hiding this comment.
| /// Not polling for FIDO USB device. | |
| /// Not polling for FIDO NFC device. |
There was a problem hiding this comment.
This is fine to reuse for now, but I think soon we'll need to distinguish between different types of NFC devices to display the right kind of icon to the user. Cf. w3c/webauthn#2360.
| } else { | ||
| tracing::warn!( | ||
| "Failed to list NFC authenticators: {:?}. Throttling NFC state updates", | ||
| err | ||
| ); | ||
| tokio::time::sleep(Duration::from_secs(1)).await; | ||
| Ok(prev_nfc_state.clone()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Do these failures happen often on NFC? Maybe we can add a comment saying that we should try to push this error handling into libwebauthn?
There was a problem hiding this comment.
I don't think I have ever seen one in my testing. This was also copied over from usb and left as a "just in case"-error handling.
Yeah, we've only got 2 transports left. I think once those are done, I'll want to go back and at least extract out the common methods between all the different transports, because they all have basically the same shape, especially near the end of the ceremonies.
let's keep it merged for now.
Yeah, the NFC one is trademarked, so you have to sign a licensing agreement to be able to use it. Not sure if we're even able to sign it since this project is not a legal entity worth it, but I'll add an issue to check it out.
Thank you! |
|
Thank you! |
3 participants
First shot at supporting NFC.
nfc.rsis more or less a direct copy ofusb.rswith the removal of "selecting device" which as of now isn't a thing in the NFC-implementation of libwebauthn. They probably could also share most/all of the code, but I'm not sure yet if we would want that.get_available_public_key_devices()is not using a static list anymore but dynamically checks on each call if NFC is available (because someone may plug in their NFC reader and try again).