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

#createCredential alg: "if options.authenticatorSelection is present..." may be incorrect #692

Closed
equalsJeffH opened this issue Nov 17, 2017 · 6 comments

Comments

@equalsJeffH
Copy link
Contributor

The current step 18 of #createCredential alg says:

If options.authenticatorSelection is present, for each authenticator...

However, if options.authenticatorSelection is not present, we will not invoke any authenticators, which will likely make the user sad.

Given that we need to pass options.authenticatorSelection.requireResidentKey and options.authenticatorSelection.requireUserVerification to authenticatorMakeCredential(), perhaps we should make authenticatorSelection a required member of MakePublicKeyCredentialOptions ? (unless I'm misunderstanding things...?)

@emlun
Copy link
Member

emlun commented Nov 17, 2017

See the first commit in #687

@emlun
Copy link
Member

emlun commented Nov 20, 2017

I made a separate PR with just that first commit: #696

@emlun
Copy link
Member

emlun commented Nov 20, 2017

I support making options.authenticatorSelection required and default to {}. Then we can also get rid of the "If authenticatorSelection is present" branch, and the descriptions of requireResidentKey and requireUserVerification in authenticatorMakeCredential won't describe possibly undefined behaviour. I suppose it would technically be a breaking change to how credentials.create() is implemented, but it wouldn't at all affect observable behaviour or the API on either end.

@emlun
Copy link
Member

emlun commented Nov 20, 2017

The bikeshed issue appeared in bikeshed commit aa80a167c54650c1c8f2382945f3894ee5b80bb4.

@equalsJeffH
Copy link
Contributor Author

thx @emlun, see speced/bikeshed#1139

emlun added a commit that referenced this issue Nov 21, 2017
@emlun
Copy link
Member

emlun commented Nov 21, 2017

Fixed by #696

@emlun emlun closed this as completed Nov 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants