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

PublicKeyCredentialRequestOptions and MakeCredentialOptions could both inherit from a dictionary which has their shared members #278

Closed
bzbarsky opened this issue Nov 4, 2016 · 16 comments

Comments

@bzbarsky
Copy link

bzbarsky commented Nov 4, 2016

In particular, everything except excludeList/allowList.

@equalsJeffH equalsJeffH added this to the WD-04 milestone Nov 7, 2016
@vijaybh vijaybh modified the milestones: WD-04, WD-05 Feb 16, 2017
@selfissued
Copy link
Contributor

Am I correct that this won't change the API, only the way it is described? If it would change the API, we need to decide this before we declare an Implementer's Draft.

@selfissued
Copy link
Contributor

There was agreement on the call to do this.

@equalsJeffH
Copy link
Contributor

this may be addressed by the credman alignment

@equalsJeffH equalsJeffH changed the title AssertionOptions and ScopedCredentialOptions could both inherit from a dictionary which has their shared members PublicKeyCredentialRequestOptions and MakeCredentialOptions could both inherit from a dictionary which has their shared members Jun 5, 2017
@equalsJeffH
Copy link
Contributor

equalsJeffH commented Jun 5, 2017

We've done some renaming since this issue was submitted. Presently:

  • AssertionOptions -> PublicKeyCredentialRequestOptions
  • ScopedCredentialOptions -> MakeCredentialOptions

..so I will update the title of this issue.

Also, the alignment of WebAuthn with CredMan did not address this issue as I'd been (incorrectly) suspecting.

PublicKeyCredentialRequestOptions and MakeCredentialOptions inherit from different dictionaries defined in CredMan.

Also, PublicKeyCredentialRequestOptions and MakeCredentialOptions continue to share three common members:

required BufferSource                challenge;

unsigned long                        timeout;

AuthenticationExtensions             extensions;

So, if this is indeed something that we ought to address, then we should define a "PublicKeyCredentialOptions"dictionary, say, whose members are the above three items, and then have PublicKeyCredentialRequestOptions and MakeCredentialOptions inherit from it.

Also, rpId is common between PublicKeyCredentialRequestOptions and MakeCredentialOptions, but at different depths within the dictionaries. Is this also something to deal with or is it fine as it is?

See also #488

@equalsJeffH
Copy link
Contributor

@bzbarsky @jyasskin -- your thoughts on above #278 (comment), should we just do this, and do we do something with rpId or just leave it as-is ?

@bzbarsky
Copy link
Author

bzbarsky commented Jun 6, 2017

PublicKeyCredentialRequestOptions and MakeCredentialOptions inherit from different dictionaries defined in CredMan.

I'm not obviously seeing that in https://w3c.github.io/webauthn/ as of today.

then have PublicKeyCredentialRequestOptions and MakeCredentialOptions inherit from it

You can only inherit from one thing.

The point of this issue was that there was some stuff that was defined multiple times and could just be defined once instead. But that may not be viable with a different inheritance model for these dictionaries, in which case you do just copy/paste as needed, at least until whatwg/webidl#195 exists.

@equalsJeffH
Copy link
Contributor

I'm not obviously seeing that in https://w3c.github.io/webauthn/ as of today.

pls see..

https://w3c.github.io/webauthn/#credentialcreationoptions-extension
https://w3c.github.io/webauthn/#credentialrequestoptions-extension

You can only inherit from one thing

d0h! 0_o

The point of this issue was that there was some stuff that was defined multiple times and could just be defined once instead. But that may not be viable with a different inheritance model for these dictionaries, in which case you do just copy/paste as needed

Ok, so shall we close this issue? thx for your help.

@bzbarsky
Copy link
Author

bzbarsky commented Jun 7, 2017

https://w3c.github.io/webauthn/#credentialcreationoptions-extension
https://w3c.github.io/webauthn/#credentialrequestoptions-extension

Neither of those is inheritance... Those are a dictionary with a member that is itself a nullable dictionary.

@bzbarsky
Copy link
Author

bzbarsky commented Jun 7, 2017

So in particular, to pass a CredentialCreationOptions thing to create with the links above, you'd do:

navigator.credentials.create({ publicKey: { rp: stuff, ... } })

@equalsJeffH
Copy link
Contributor

@bzbarsky patiently explained:

Neither of those is inheritance... Those are a dictionary with a member that is itself a nullable dictionary.

d0h! 0_o ...gotcha.

Ok, thanks, so we can close this issue?

@bzbarsky
Copy link
Author

bzbarsky commented Jun 7, 2017

That's up to you, basically. I don't have strong opinions about which member definitions should be duplicated and which should be shared for which of these dictionaries.

@AngeloKai
Copy link
Contributor

Reading through the comments, it seems like we may be able to close the issue though some more discussions may be needed. Adding a discussion tag.

@nadalin
Copy link
Contributor

nadalin commented Jul 26, 2017

Close w/o action pending review

@jcjones
Copy link
Contributor

jcjones commented Jul 26, 2017

(Going to get @jyasskin's commentary, and he should close if he agrees)

@jyasskin
Copy link
Member

jyasskin commented Aug 2, 2017

So, we have:

dictionary MakePublicKeyCredentialOptions {
    required PublicKeyCredentialEntity           rp;
    required PublicKeyCredentialUserEntity       user;

    required BufferSource                             challenge;
    required sequence<PublicKeyCredentialParameters>  pubKeyCredParams;

    unsigned long                                timeout;
    sequence<PublicKeyCredentialDescriptor>      excludeCredentials = [];
    AuthenticatorSelectionCriteria               authenticatorSelection;
    AuthenticationExtensions                     extensions;
};

and

dictionary PublicKeyCredentialRequestOptions {
    required BufferSource                challenge;
    unsigned long                        timeout;
    USVString                            rpId;
    sequence<PublicKeyCredentialDescriptor> allowCredentials = [];
    AuthenticationExtensions             extensions;
};

We could extract challenge, timeout, and extensions into a base dictionary, probably of the form:

dictionary PublicKeyCredentialOptions {
    required BufferSource                challenge;
    unsigned long                        timeout;
    AuthenticationExtensions             extensions;
};

However, I tend to find that the specification of extracted fields is harder to understand than if they're just duplicated, so I endorse duplicating these. If the two dictionaries were passed to a common operation, that might be a reason to extract the base class, but they're not, so +1 for just closing this.

@jcjones
Copy link
Contributor

jcjones commented Aug 2, 2017

Agreed with @jyasskin's analysis. Closing.

@jcjones jcjones closed this as completed Aug 2, 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

8 participants