-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add mediation to credential creation options #224
Conversation
…eavy lifting of conditional in create is handled in WebAuthn. See also https://github.com/w3c/webauthn/pull/1951/files https://github.com/w3c/webauthn/wiki/Explainer:-Conditional-Registration-Extension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your CL! The API surface looks good on the credman side. We just have to make sure the interpretation of mediation remains clear for other credential types. Here is my first pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with comments. Thank you for your great work!
Thank you for the review! |
06d81f0
to
b6c2261
Compare
index.bs
Outdated
{{Credential}}'s default implementation of {{Credential/willRequestConditionalCreation()}}: | ||
|
||
<ol class="algorithm"> | ||
1. Return a new {{Promise}} that immediately [=resolves=]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here you just want:
1. Return a new {{Promise}} that immediately [=resolves=]. | |
1. Return [=a promise resolved with=] undefined. |
The "immediately" could be confusing (it will happen "immediately", but let the underlying algorithm in WebIDL and JS handle that).
index.bs
Outdated
1. Return a new {{Promise}} that immediately [=resolves=]. | ||
</ol> | ||
|
||
Note: If this function is not present, {{CredentialMediationRequirement/conditional}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be "method" or "operation"
Note: If this function is not present, {{CredentialMediationRequirement/conditional}} | |
Note: If this method is not present, {{CredentialMediationRequirement/conditional}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed a couple small issues here... might be good to fix up before merging.
SHA: 4fa577c Reason: push, by pascoej Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This fixes
Most of the heavy lifting of conditional in create is handled in WebAuthn.
Fixes #225
See also:
https://github.com/w3c/webauthn/pull/1951/files
https://github.com/w3c/webauthn/wiki/Explainer:-Conditional-Registration-Extension
Preview | Diff