Skip to content

Conversation

@jkaiwar
Copy link

@jkaiwar jkaiwar commented Oct 22, 2025

This PR adds info! logs in the error paths for a couple of BioEnrollment implementation methods, which previously returned Error::Ctap(CtapError::Other) silently under some CTAP requests to channels, where the response had None fields that were expected to be Some.

Maintainers: Are these logs desirable in the context of #14 ? How appropriate are the messages and log-levels I have chosen?

@jkaiwar jkaiwar changed the title Log when CTAP2 bio_enrollment responses return None fields (#14) Log when CTAP2 bio_enrollment responses return None fields Oct 22, 2025
@AlfioEmanueleFresta
Copy link
Member

Thanks @jkaiwar!

I believe warn! here would be most appropriate if this is unexpected authenticator behaviour, and causes the request to fail.

If this is more common, debug! would be appropriate. IMO, info! should be few and far between during normal operation, only reserved to high level methods, reporting results.

I would defer to @msirringhaus on the specifics on biometrics enrollment.

@msirringhaus
Copy link
Collaborator

I would agree that info!() is too low. If devices do not return these fields, they are in direct violation of the spec. So I would also go to warn!() (or maybe even higher).

@AlfioEmanueleFresta
Copy link
Member

I would reserve error! to something the library genuinely can't handle, or high-level operation failures.

@jkaiwar
Copy link
Author

jkaiwar commented Oct 31, 2025

Sounds good @AlfioEmanueleFresta @msirringhaus. I have changed the levels to warn.

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