Skip to content

Conversation

@jschanck
Copy link
Collaborator

@jschanck jschanck commented Jun 1, 2023

(This is conceptually part of #270, but I separated it out since it has side effects.)

The abstract FidoDevice trait depends on U2FDeviceInfo through

fn get_device_info(&self) -> U2FDeviceInfo;
fn set_device_info(&mut self, dev_info: U2FDeviceInfo);

The data in U2FDeviceInfo is specific to the HID transport, and it's somewhat awkward to include it in virtual devices.

Apart from checking for the CBOR capability in FidoDevice::init (which is probably not necessary), we only use get_device_info in status updates. Firefox doesn't do anything with these status updates, so I don't see any reason to keep them.

@jschanck jschanck requested a review from mozkeeler June 1, 2023 22:10
Copy link
Collaborator

@mozkeeler mozkeeler left a comment

Choose a reason for hiding this comment

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

LGTM!

@msirringhaus
Copy link
Contributor

At least in the current draft of the about-page, I am relying on the DeviceAvailable/Unavailable messages (e.g. to determine when resetting a device, if all have been removed and only the one we want to reset has been re-inserted).
It is not using the dev-info, however (yet). If #217 would land, then we could use that to display the names of the token and the manufacturer, too (not strictly needed, but nice to have).
So I feel hesitant to remove it all, although I do agree that its a bit awkward in general and could use some refactoring.

@jschanck
Copy link
Collaborator Author

jschanck commented Jun 2, 2023

Your use of DeviceAvailable and DeviceUnavailable on the about page doesn't require a U2FDeviceInfo, so I would at least want to simplify the status updates. But I'd suggest that, rather than counting DeviceAdded and DeviceUnavailable messages, you add a StatusUpdate::ResetError(...) that tells the application what needs to be done to perform a reset.

To your second point, we could include a human-readable debug_info: String with StatusUpdate::InteractiveManagement and let each device type define what goes in it.

@jschanck jschanck merged commit c19a3ea into mozilla:ctap2-2021 Jun 2, 2023
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.

3 participants