Skip to content
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.

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

Remove StatusUpdates that include a U2FDeviceInfo #273

Merged
merged 1 commit into from
Jun 2, 2023

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.

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