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

Substitute enum types in dictionaries with DOMStrings #1392

Merged
merged 13 commits into from
May 27, 2020

Conversation

jcjones
Copy link
Contributor

@jcjones jcjones commented Mar 24, 2020

This fixes issue #1376, auditing all enums passed into WebAuthn to ensure that WebAuthn can be extended without causing WebIDL failures for lagging implementations.

I believe I caught them all. I prefixed all the targets with >> in the list below:

dictionary PublicKeyCredentialCreationOptions {
    required PublicKeyCredentialRpEntity         rp;
    required PublicKeyCredentialUserEntity       user;

    required BufferSource                             challenge;
    required sequence<PublicKeyCredentialParameters>  pubKeyCredParams;

    unsigned long                                timeout;
    sequence<PublicKeyCredentialDescriptor>      excludeCredentials = [];
    AuthenticatorSelectionCriteria               authenticatorSelection;
>>    AttestationConveyancePreference              attestation = "none";
    AuthenticationExtensionsClientInputs         extensions;
};


dictionary PublicKeyCredentialRequestOptions {
    required BufferSource                challenge;
    unsigned long                        timeout;
    USVString                            rpId;
    sequence<PublicKeyCredentialDescriptor> allowCredentials = [];
>>    UserVerificationRequirement          userVerification = "preferred";
    AuthenticationExtensionsClientInputs extensions;
};

dictionary AuthenticatorSelectionCriteria {
>>    AuthenticatorAttachment      authenticatorAttachment;
    boolean                      requireResidentKey = false;
>>    UserVerificationRequirement  userVerification = "preferred";
};

dictionary PublicKeyCredentialDescriptor {
>>    required PublicKeyCredentialType      type;
    required BufferSource                 id;
    sequence<DOMString>                   transports;
};

dictionary TokenBinding {
>>    required TokenBindingStatus status;
    DOMString id;
};

dictionary PublicKeyCredentialParameters {
>>    required PublicKeyCredentialType      type;
    required COSEAlgorithmIdentifier      alg;
};

Preview | Diff

@jcjones jcjones requested a review from equalsJeffH March 24, 2020 21:38
@jcjones jcjones self-assigned this Mar 24, 2020
@jcjones jcjones linked an issue Mar 24, 2020 that may be closed by this pull request
@jcjones
Copy link
Contributor Author

jcjones commented Mar 24, 2020

This will need to rebase on #1393.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@nadalin nadalin requested a review from akshayku March 25, 2020 19:15
@nadalin nadalin added this to the L2-WD-03 milestone Mar 25, 2020
@jcjones jcjones force-pushed the 1376-strict_enum_types branch from 9fbb2fa to f17baf0 Compare March 25, 2020 23:03
Copy link
Contributor

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, thx @jcjones! I've one comment below wrt consistency, and then an overall one regarding the Note: we added to 5.8.4. Authenticator Transport Enumeration (enum AuthenticatorTransport):

Note: The AuthenticatorTransport enumeration is not referenced by other parts of the Web IDL because that would preclude other values from being used without updating this specification and its implementations. It is important for backwards compatibility that client platforms and Relying Parties handle unknown values. Therefore it exists here for documentation and as a registry. Where transports are represented elsewhere, they are typed as DOMStrings, for example in transports.

...I'm thinking we ought to make that Note more general such that it applies to all the enums that are now effectively "registries", move the note somewhere, perhaps to a new subsection at the beginning of Section 5, and then have small Notes at all the enum registries that link back to the overall one.

WDYT?

index.bs Show resolved Hide resolved
@equalsJeffH equalsJeffH self-requested a review April 15, 2020 15:45
emlun
emlun previously requested changes Apr 22, 2020
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
@jcjones jcjones requested a review from emlun April 27, 2020 21:31
@equalsJeffH
Copy link
Contributor

where you are using the term "unset", do you mean by that "does not exist" (note link to infra spec)?

@jcjones
Copy link
Contributor Author

jcjones commented Apr 30, 2020

where you are using the term "unset", do you mean by that "does not exist" (note link to infra spec)?

Yes, I'll update all that wordage - thank you for the suggestion!

index.bs Outdated Show resolved Hide resolved
Copy link
Contributor

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thx @jcjones, LGTM, although, there's this one little outstanding comment #1392 (review). I suppose it's ok to land this without addressing the latter comment, tho then ISTM we ought to open an editorial issue containing that comment's substance. thx again.

index.bs Show resolved Hide resolved
@@ -2335,7 +2335,7 @@ during registration.
:: This operation returns the value of {{AuthenticatorAttestationResponse/[[transports]]}}.

: <dfn>\[[transports]]</dfn>
:: This [=internal slot=] contains a sequence of zero or more unique {{DOMString}}s in lexicographical order. These values are the transports that the [=authenticator=] is believed to support, or an empty sequence if the information is unavailable. The values SHOULD be members of {{AuthenticatorTransport}} but [=[RPS]=] MUST accept unknown values.
:: This [=internal slot=] contains a sequence of zero or more unique {{DOMString}}s in lexicographical order. These values are the transports that the [=authenticator=] is believed to support, or an empty sequence if the information is unavailable. The values SHOULD be members of {{AuthenticatorTransport}} but [=[RPS]=] MUST ignore unknown values.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right? Shouldn't the RP just pass all values through (known or unknown) and let the client ignore the ones it doesn't need?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what does the RP do with an unknown value?

The authenticator says it supports USB and ZWave. Does the RP need to permit ZWave, or just ignore it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this value isn't signed, the RP should never make permission decisions based on it, right (such decisions should be based on data derived from the attestation cert)? So I don't know what else the RP would do with this value other than echo it back to the client.

Addresses w3c#1392 (review)
by adding a new conformance section and referring to it at the description of
each enumeration type.
@jcjones
Copy link
Contributor Author

jcjones commented May 15, 2020

@equalsJeffH : Please take a look at 42dbe07 , as the change is perhaps larger than we had initially discussed but I like the way this ties together better.

@emlun: Please see responses above. I'm OK with changing things, but maybe you can propose the changes you'd like to see?

Thanks and have a good weekend!

index.bs Show resolved Hide resolved
Copy link
Contributor

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @jcjones !

@equalsJeffH equalsJeffH dismissed emlun’s stale review May 27, 2020 19:10

we'll submit this suggestion as a issue

@equalsJeffH equalsJeffH merged commit a133711 into w3c:master May 27, 2020
WebAuthnBot pushed a commit that referenced this pull request May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inputs to WebAuthn methods should not include strict Enum types
6 participants