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

New PublicKeyCredential methods for JSON (de)serialization #1703

Merged
merged 27 commits into from
Jun 29, 2022
Merged
Changes from 22 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
59ecbf3
Add sections for new API methods
MasterKale Feb 23, 2022
493095f
Migrate toJSON to instance method defs
MasterKale Mar 2, 2022
e87c96d
Add notes
MasterKale Mar 2, 2022
bec86b5
Add placeholder IDL for optionsFromJSON
MasterKale Mar 2, 2022
002d0f5
Take a stab at toJSON IDL definitions
MasterKale Mar 2, 2022
a476667
Clarify toJSON behavior
MasterKale Mar 2, 2022
161e102
Group related IDL defs together
MasterKale Mar 2, 2022
16aab08
Define IDLs for optionsFromJSON()
MasterKale Mar 2, 2022
d264b61
Finish a thought
MasterKale Mar 2, 2022
c1aef27
Mark arguments required
MasterKale Mar 2, 2022
e11e518
Remove notes rationalizing new API existence
MasterKale Mar 23, 2022
91161a2
Try to take care of fatal build errors
MasterKale Mar 23, 2022
1161328
Define AuthenticationExtensionsClientInputsJSON
MasterKale Mar 23, 2022
f13cb35
Add required and align properties
MasterKale Mar 23, 2022
bfa21b0
Split up optionsFromJSON for simplicity's sake
MasterKale Mar 23, 2022
4a904af
Refine definition of `toJSON()` method
MasterKale Mar 23, 2022
078e432
Move JSON xmp out of PublicKeyCredential dl
MasterKale Mar 23, 2022
c0c9454
Remove required from property with default value
MasterKale Mar 23, 2022
22a796c
Split up options methods into two sections
MasterKale Mar 23, 2022
6fa0da7
Return authenticatorAttachment and transports
MasterKale Apr 17, 2022
592271d
Define clientExtensionResults in toJSON()
MasterKale Apr 17, 2022
408b323
Try to support all extension inputs
MasterKale Apr 18, 2022
582b2e2
Incorporate Emil's feedback
MasterKale Jun 9, 2022
ae3a7d4
Rename options methods from `get-` to `parse-`
MasterKale Jun 9, 2022
b1b5fd1
Incorporate @emlun's copy advice
MasterKale Jun 10, 2022
88bd8fb
Attempt to define rules for when encoding fails
MasterKale Jun 10, 2022
12e5071
Incorporate feedback on wording for exceptions
MasterKale Jun 20, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 133 additions & 0 deletions index.bs
Original file line number Diff line number Diff line change
Expand Up @@ -1407,6 +1407,7 @@ that are returned to the caller when a new credential is created, or a new asser
[SameObject] readonly attribute AuthenticatorResponse response;
[SameObject] readonly attribute DOMString? authenticatorAttachment;
AuthenticationExtensionsClientOutputs getClientExtensionResults();
PublicKeyCredentialJSON toJSON();
};
</xmp>
<dl dfn-type="attribute" dfn-for="PublicKeyCredential">
Expand Down Expand Up @@ -1450,6 +1451,60 @@ that are returned to the caller when a new credential is created, or a new asser
[=extension identifier=] → [=client extension output=] entries produced by the extension's
[=client extension processing=].

: {{PublicKeyCredential/toJSON()}}
emlun marked this conversation as resolved.
Show resolved Hide resolved
:: This operation returns {{RegistrationResponseJSON}} or {{AuthenticationResponseJSON}}, that is a JSON-serialized
MasterKale marked this conversation as resolved.
Show resolved Hide resolved
representation of the instance of {{PublicKeyCredential}} suitable for submission to a [=[RP]=] server as a DOMString
payload with `application/json` mimetype. The [=client=] is in charge of
MasterKale marked this conversation as resolved.
Show resolved Hide resolved
[serializing values to JSON as usual](https://webidl.spec.whatwg.org/#idl-tojson-operation), but must take additional
steps to encode any {{ArrayBuffer}} values to {{DOMString}} values using [=base64url encoding=] to ensure compatibility
MasterKale marked this conversation as resolved.
Show resolved Hide resolved
with JSON syntax. {{ArrayBuffer}} values to be encoded are identified according to the detected type of the instance's
`response` property.
MasterKale marked this conversation as resolved.
Show resolved Hide resolved
:: |clientExtensionResults| below should be the output of {{PublicKeyCredential/getClientExtensionResults()}}, with any
{{ArrayBuffer}} values encoded to {{DOMString}} values using [=base64url encoding=]. This may include {{ArrayBuffer}}
MasterKale marked this conversation as resolved.
Show resolved Hide resolved
values from extensions registered in the IANA "WebAuthn Extension Identifiers" registry [[!IANA-WebAuthn-Registries]]
but not defined in [[#sctn-extensions]].
MasterKale marked this conversation as resolved.
Show resolved Hide resolved
</dl>

<xmp class="idl">
typedef DOMString Base64URLString;
typedef (RegistrationResponseJSON or AuthenticationResponseJSON) PublicKeyCredentialJSON;

dictionary RegistrationResponseJSON {
Base64URLString id;
Base64URLString rawId;
AuthenticatorAttestationResponseJSON response;
DOMString? authenticatorAttachment;
AuthenticationExtensionsClientOutputsJSON clientExtensionResults;
DOMString type;
};

dictionary AuthenticatorAttestationResponseJSON {
Base64URLString clientDataJSON;
Base64URLString attestationObject;
sequence<DOMString> transports;
};

dictionary AuthenticationResponseJSON {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about, instead of separate RegistrationResponseJSON and AuthenticationResponseJSON type, we have a single PublicKeyCredentialJSON dictionary type with an AuthenticatorResponseJSON member, with typedef (AuthenticatorAttestationResponseJSON or AuthenticatorAssertionResponseJSON) AuthenticatorResponseJSON?

That would be more akin to how the regular PublicKeyCredential is defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's frustrating to me that PublicKeyCredential has been overloaded to mean two possible data structures, since response can be one of two types. I think this is a current developer pain point, that there aren't subclasses of PublicKeyCredential coming out of .create() and .get() to be uniquely typed. That's why I've been trying to emphasize the idea of "RegistrationResponse" and "AuthenticationResponse" here, to get to a 1:1 mapping between navigator.credentials method and return type.

To the suggestion that a PublicKeyCredentialJSON gets defined as a union type, is it a compromise to make it easier for the client to implement this feature? I personally want to see these additions err on the side of better developer usability, and necessarily see this as hoisting some of this complexity onto the client. The aim is to make it easier for devs to consume WebAuthn, and to achieve this I think we should be more straightforward/explicit with return types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To the suggestion that a PublicKeyCredentialJSON gets defined as a union type, is it a compromise to make it easier for the client to implement this feature? I personally want to see these additions err on the side of better developer usability, and necessarily see this as hoisting some of this complexity onto the client.

@kreichgauer It's been a few weeks, what do you currently think about my response above to your PublicKeyCredentialJSON suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies for the slow reply.

To the suggestion that a PublicKeyCredentialJSON gets defined as a union type, is it a compromise to make it easier for the client to implement this feature

No, I think it makes no difference implementation wise, and if it did that would not be a convincing argument as you point out.

I agree with the desire to aid developers as much as we can, and I agree that the structure of the API with create() and get() returning the same interface type but encapsulating two different response dictionary types is…awkward. (It's a side effect of having WebAuthn bolted on top of Credential Management, but our developers/users shouldn't care about that.) However, I'm kind of on the fence whether I find the separate inheritance hierarchy for JSON serialization more or less confusing. I guess it depends on whether you think the hypothetical WebAuthn developer would first look at the PublicKeyCredential interface IDL or not? If they did, comparing the two and realizing that they're different in weird subtle ways (beyond the binary->base64url conversion) I think could be a bit puzzling? If on the other hand, we'd assume the hypothetical developer would only ever see the JSON IDL, I agree what you propose would be simpler to grok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it depends on whether you think the hypothetical WebAuthn developer would first look at the PublicKeyCredential interface IDL or not? If they did, comparing the two and realizing that they're different in weird subtle ways (beyond the binary->base64url conversion) I think could be a bit puzzling? If on the other hand, we'd assume the hypothetical developer would only ever see the JSON IDL, I agree what you propose would be simpler to grok.

@kreichgauer Can we leave comments in IDL definitions? I want to believe a developer who doesn't want to get too deep into things will jump to these JSON methods, but it's probably more realistic that they'd stumble on PublicKeyCredential first and then the JSON IDL. If we could leave comments in the JSON IDL explaining why there are split types then perhaps we can have the best of both worlds.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, IDL can contain comments. The syntax is the same as in JavaScript.

Base64URLString id;
Base64URLString rawId;
AuthenticatorAssertionResponseJSON response;
DOMString? authenticatorAttachment;
AuthenticationExtensionsClientOutputsJSON clientExtensionResults;
DOMString type;
};

dictionary AuthenticatorAssertionResponseJSON {
Base64URLString clientDataJSON;
Base64URLString authenticatorData;
Base64URLString signature;
Base64URLString? userHandle;
};

dictionary AuthenticationExtensionsClientOutputsJSON {
};
</xmp>

<dl dfn-type="attribute" dfn-for="PublicKeyCredential">
: <dfn>\[[type]]</dfn>
:: The {{PublicKeyCredential}} [=interface object=]'s {{Credential/[[type]]}} [=internal slot=]'s value is the string
"`public-key`".
Expand Down Expand Up @@ -2387,6 +2442,84 @@ Note: Invoking this method from a [=browsing context=] where the [=Web Authentic

</div>

### Deserialize Registration ceremony options - PublicKeyCredential's `getCreationOptionsFromJSON()` Method ### {#sctn-getCreationOptionsFromJSON}
MasterKale marked this conversation as resolved.
Show resolved Hide resolved

<div link-for-hint="WebAuthentication/getCreationOptionsFromJSON">

[=[WRPS]=] use this method to convert JSON-compatible serialized representations of options for
{{CredentialsContainer/create()|navigator.credentials.create()}} to {{PublicKeyCredentialCreationOptions}}.

Upon invocation, the [=client=] must take the provided options and return an identically-structured object,
making sure to decode to {{ArrayBuffer}} those {{DOMString}} properties that are encoded with [=base64url encoding=]
but must be {{ArrayBuffer}} values according to the definition of {{PublicKeyCredentialCreationOptions}}.

{{AuthenticationExtensionsClientInputsJSON}} may include extensions registered in the IANA
"WebAuthn Extension Identifiers" registry [[!IANA-WebAuthn-Registries]] but not defined in [[#sctn-extensions]].
In either case the [=client=] must decode to {{ArrayBuffer}} those {{DOMString}} values that are encoded with
[=base64url encoding=] but must be {{ArrayBuffer}} values according to the extension's definition.
MasterKale marked this conversation as resolved.
Show resolved Hide resolved

<xmp class="idl">
partial interface PublicKeyCredential {
static PublicKeyCredentialCreationOptions getCreationOptionsFromJSON(PublicKeyCredentialCreationOptionsJSON options);
};

dictionary PublicKeyCredentialCreationOptionsJSON {
required PublicKeyCredentialRpEntity rp;
required PublicKeyCredentialUserEntityJSON user;
required Base64URLString challenge;
required sequence<PublicKeyCredentialParameters> pubKeyCredParams;
unsigned long timeout;
sequence<PublicKeyCredentialDescriptorJSON> excludeCredentials = [];
AuthenticatorSelectionCriteria authenticatorSelection;
DOMString attestation = "none";
AuthenticationExtensionsClientInputsJSON extensions;
};

dictionary PublicKeyCredentialUserEntityJSON {
required Base64URLString id;
required DOMString name;
required DOMString displayName;
};

dictionary PublicKeyCredentialDescriptorJSON {
required Base64URLString id;
required DOMString type;
sequence<DOMString> transports;
};

dictionary AuthenticationExtensionsClientInputsJSON {
};
</xmp>

</div>

### Deserialize Authentication ceremony options - PublicKeyCredential's `getRequestOptionsFromJSON()` Methods ### {#sctn-getRequestOptionsFromJSON}

<div link-for-hint="WebAuthentication/getRequestOptionsFromJSON">

[=[WRPS]=] use this method to convert JSON-compatible serialized representations of options for {{CredentialsContainer/get()|navigator.credentials.get()}} to {{PublicKeyCredentialRequestOptions}}.

Upon invocation, the [=client=] must take the provided options and return an identically-structured object,
making sure to decode to {{ArrayBuffer}} those {{DOMString}} properties that are encoded with [=base64url encoding=]
but must be {{ArrayBuffer}} values according to the definition of {{PublicKeyCredentialRequestOptions}}.
MasterKale marked this conversation as resolved.
Show resolved Hide resolved

MasterKale marked this conversation as resolved.
Show resolved Hide resolved
<xmp class="idl">
partial interface PublicKeyCredential {
static PublicKeyCredentialRequestOptions getRequestOptionsFromJSON(PublicKeyCredentialRequestOptionsJSON options);
};

dictionary PublicKeyCredentialRequestOptionsJSON {
required Base64URLString challenge;
unsigned long timeout;
DOMString rpId;
sequence<PublicKeyCredentialDescriptorJSON> allowCredentials = [];
DOMString userVerification = "preferred";
AuthenticationExtensionsClientInputsJSON extensions;
};
</xmp>

</div>

## Authenticator Responses (interface <dfn interface>AuthenticatorResponse</dfn>) ## {#iface-authenticatorresponse}

[=Authenticators=] respond to [=[RP]=] requests by returning an object derived from the
Expand Down