Implement Credential Management #67
Conversation
AlfioEmanueleFresta
left a comment
AlfioEmanueleFresta
left a comment
There was a problem hiding this comment.
Thank you so much for your work on this @msirringhaus!
A few notes, and as this is net new functionality without a stable API, I'll leave the decision as to how much to address here or a separate PR to your judgement.
| pin_provider: &mut Box<dyn PinProvider>, | ||
| timeout: Duration, | ||
| ) -> Result<Ctap2CredentialManagementMetadata, Error>; | ||
| async fn enumerate_rps_begin( |
There was a problem hiding this comment.
Would it make sense for the higher level APIs (enumerate_rps, enumerate_credentials) to return Iterators?
(No need to address this in this PR.)
There was a problem hiding this comment.
Not sure how nice that would be to use, since each iteration could fail and may needs to be retried.
Maybe simpler would be to move all the logic of the example inside libwebauthn, and only have a single enumerate_credentials()-function that already collects all RPs and all credentials and returns a map with all the content?
Depends a bit on how low-level we want the API to be. We have the same "problem" with e.g. BioEnrollments.
| ) | ||
| }?; | ||
| let metadata = Ctap2CredentialManagementMetadata::new( | ||
| resp.existing_resident_credentials_count.unwrap(), |
There was a problem hiding this comment.
I'm not very comfortable with unwraps on authenticator response fields. IMO we should treat the response as untrusted - WDYT?
There was a problem hiding this comment.
I would agree. We have this however in lots of other places. I'll create a follow up issue to fix this all in one PR, ok?
| &mut self, | ||
| pin_provider: &mut Box<dyn PinProvider>, | ||
| timeout: Duration, | ||
| ) -> Result<(Ctap2PublicKeyCredentialRpEntity, ByteBuf, u64), Error>; |
There was a problem hiding this comment.
These output types may not be obvious, I think it's worth creating a struct with named fields.
| async fn enumerate_rps<T: CredentialManagement>( | ||
| channel: &mut T, | ||
| pin_provider: &mut Box<dyn PinProvider>, | ||
| ) -> Result<Vec<(Ctap2PublicKeyCredentialRpEntity, ByteBuf)>, WebAuthnError> { |
There was a problem hiding this comment.
Could we expose Vec instead of ByteBuf? Thankfully this should be cheap with into_vec (source).
Context - I'd like for higher level APIs, including the WebAuthn trait, to eventually hide much of the CTAP ugliness: optional fields, protocol internals, serde types.
This isn't already the case and we have the following tech debt:
- Currently use Ctap2 models directly within high-level traits, eg.
pub struct MakeCredentialRequest { pub hash: Vec<u8>, pub origin: String, pub relying_party: Ctap2PublicKeyCredentialRpEntity,
- Typealias other types as a shortcut:
AlfioEmanueleFresta
left a comment
AlfioEmanueleFresta
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedback.
2 participants
Fixes #24