New PublicKeyCredential methods for JSON (de)serialization

[MasterKale]

This PR introduces new methods to PublicKeyCredential that lean on the client to assist with JSON deserialization of PublicKeyCredentialCreationOptions and PublicKeyCredentialRequestOptions, and JSON serialization of PublicKeyCredential output from navigator.credentials.create() and navigator.credentials.get().

See Issue #1683 for more context.

---

Preview | Diff

[MasterKale]

I apologize for the roughness of these initial drafts, this is my first time attempting to submit something substantial to the spec. @nicksteele has graciously volunteered for a bit of hand-holding as I work on this, but I look forward to your comments and suggestions as we work to make this a reality.

[emlun]

Thanks for taking this on! For starters, some comments on the high-level disposition...

[MasterKale]

Alright, I've got new types defined for everything so I'm flipping this to Ready for Review.

One thing I need help with (aside from review of the IDL definitions) is figuring out the proper way to address "FATAL ERROR"s like this:

FATAL ERROR: 'dictionary' definitions don't use a 'for' attribute, but this one claims it's for 'PublicKeyCredential' (perhaps inherited from an ancestor). This is probably a markup error.

I wasn't able to intuit a solution from existing definitions.

I'm also uncertain how my intended API for optionsFromJSON() is best defined as WebIDL. I want to define two possible options blobs, as documented in https://docs.google.com/document/d/e/2PACX-1vTEyAjhn6a3Rqz2KLKcPg7NwoCGO31Lz7E_2zYt8J6Kzey8UUYycv5iukUos5waD4gsml-aEOEs1it0/pub, instead of using basic unions that make it ambiguous which input generates which output.

[emlun]

Right now the method descriptions don't quite agree with the WebIDL definitions - the descriptions say that they take in and put out JSON, which would be DOMString values, but the definitions work with JavaScript objects. We need to clarify which of the two it is - probably the latter, since there's already the JSON.parse() and .stringify() methods to transform to/from JSON?

I'm not sure whether WebIDL allows the overloaded signature for optionsFromJSON, but either way I think the API is a bit confusing. Clearly the method property is meant for the client to decide which method variant is being called, but it's specified with default values, which would probably work against that? It might be better to split this function into two PublicKeyCredentialCreationOptions.fromJson() and PublicKeyCredentialRequestOptions.fromJson() functions, to clearly separate them.

---

One thing I need help with (aside from review of the IDL definitions) is figuring out the proper way to address "FATAL ERROR"s like this:

These errors are because the WebIDL block is enclosed in the <dl dfn-type="attribute" dfn-for="PublicKeyCredential"> tag with the member definitions, so Bikeshed looks for definitions like PublicKeyCredential.Base64URLString and fails to find them. I tried a couple things:

• Move the toJSON() block out of the parent <dl>. This fixes the errors and otherwise seems to still work, but it seems a bit strange to have toJSON() outside the definition list where it does belong (it's just the inline type definitions clashing a bit).
• Enclose the <xmp> with <dl dfn-for="">. This also seems to fix the errors and otherwise work, but I don't know how correct it is.

[dwaite]

@emlun The optionsFromJSON method is defined on PublicKeyCredential rather than e.g. PublicKeyCredentialRequestOptions as the latter is a dictionary rather than an interface.

I'd lean toward having the method take JSON text - this would both increase symmetry with the toJSON()method, and allow us to declare the operation of the method in terms of prose rather than IDL.

[MasterKale]

Right now the method descriptions don't quite agree with the WebIDL definitions - the descriptions say that they take in and put out JSON, which would be DOMString values, but the definitions work with JavaScript objects. We need to clarify which of the two it is - probably the latter, since there's already the JSON.parse() and .stringify() methods to transform to/from JSON?

The idea is that the "JSON" inputs are application/json string payloads from the server, however the RP front end and back end wish to orchestrate that, parsed into complex JavaScript Object's. It's the objects that are being passed into .optionsFromJSON() to get back out an Object with everything formatted to be immediately passed into .create() or .get().

I'd lean toward having the method take JSON text - this would both increase symmetry with the toJSON()method, and allow us to declare the operation of the method in terms of prose rather than IDL.

Huh, so if the input is JSON then you don't need WebIDL definitions for it? I approached it as though I was writing something akin to TypeScript definitions for these methods.

[dwaite]

I'd lean toward having the method take JSON text - this would both increase symmetry with the toJSON()method, and allow us to declare the operation of the method in terms of prose rather than IDL.

Huh, so if the input is JSON then you don't need WebIDL definitions for it? I approached it as though I was writing something akin to TypeScript definitions for these methods.

You're right - toJSON() emits Objects not Strings, so the current IDL is proper.

[MasterKale]

In JS dev land it's easy to forget that "JSON" is actually a string with Content-Type: application/json. Most of the time when we're "working with JSON" we're talking about the Object you get out of JSON.parse() when you pass that string into it.

It seems when I mention "JSON" here in the context of this spec I'm evoking thoughts of passing string's around, not JavaScript Object's. Perhaps this is where I'm getting confused. Can someone confirm this for me so I have a chance at getting into the right mindset for some of this feedback?

[MasterKale]

Based on some discussion during the WG meeting today, it may be feasible to update the .toJSON() method here to reference https://webidl.spec.whatwg.org/#idl-tojson-operation, and then in plain language describe that the client has the responsibility to return an object with ArrayBuffer's encoded to strings using Base64URL.

@kreichgauer Did I understand this correctly? Or would we still need to wrangle IDL definitions to declare this behavior?

[dwaite]

Based on some discussion during the WG meeting today, it may be feasible to update the .toJSON() method here to reference https://webidl.spec.whatwg.org/#idl-tojson-operation, and then in plain language describe that the client has the responsibility to return an object with ArrayBuffer's encoded to strings using Base64URL.

I've been researching if this is appropriate to do indicate with a custom WebIDL extended attribute (for example [Base64URLSerialization]) which would also provide a hint usable by tooling.

[kreichgauer]

Based on some discussion during the WG meeting today, it may be feasible to update the .toJSON() method here to reference https://webidl.spec.whatwg.org/#idl-tojson-operation, and then in plain language describe that the client has the responsibility to return an object with ArrayBuffer's encoded to strings using Base64URL.

@kreichgauer Did I understand this correctly? Or would we still need to wrangle IDL definitions to declare this behavior?

That's how I interpret the toJSON section of the WebIDL spec at least.

I did a little digging, and Web Payments seems to want to define PaymentResponse.toJSON()
this way. Though they just refer to the default method steps of the regular operation, which we can't.

The Push API is a counter example. In PushSubscription.toJSON() it defines a separate PushSubscriptionJSON dictionary. But it defines its own processing rules.

[MasterKale]

@emlun Thank you for the verbiage tweaks! Those are all incorporated now, one step closer to landing this thing 💪

[dagnelies]

This is already a great step forward.

As far as I understand, it is converting all ArrayBuffer objects into base64url. I wonder if it would make sense to add some more nuanced conversions.

In particular, I would view 2 exceptions:

• During both creation and authentication, the response.clientDataJSON contains a raw JSON encoded string. So it would be better to parse this ArrayBuffer directly (like {"type":"webauthn.create","challenge":"ZmFrZS1pZC05ZDFqazgwa251NA","origin":"http://localhost:63342","crossOrigin":false} instead or re-encoding this string into base64url.
• During authentication, the response.userHandle contains the plain text "user.id" like my-john-doe-id and here also the raw value holds more value than its base64url encoded variant.

Basically, it would be nice to distinguish between ArrayBuffer objects containing plain strings and ones containing data.

[dwaite]

• During both creation and authentication, the response.clientDataJSON contains a raw JSON encoded string. So it would be better to parse this ArrayBuffer directly (like {"type":"webauthn.create","challenge":"ZmFrZS1pZC05ZDFqazgwa251NA","origin":"http://localhost:63342","crossOrigin":false} instead or re-encoding this string into base64url.

Speaking to this one, the clientDataJSON is part of the signed response, so any changes to white space, ordering, or Unicode canonicalization will ruin the response. Hence it is sent as binary rather than as a JSON structure or stringified JSON text.

[dagnelies]

@dwaite I'm aware of that. I was indeed referring to a raw JSON String, not the parsed object. I think it would be rather trivial to utf-8 encode it on the server side (instead of base64 decoding it) to get the correct byte array for signature verification. It just makes things more readable IMHO.

[dwaite]

@dwaite I'm aware of that. I was indeed referring to a raw JSON String, not the parsed object. I think it would be rather trivial to utf-8 encode it on the server side (instead of base64 decoding it) to get the correct byte array for signature verification.

Unicode normalization forms would be the issue here, not utf-8 encoding (I would expect the JSON to already be UTF-8 encoded).

The only way this works AFAIK would be if the client and the server (either through normalization step or intermediary transport guarantees) represented that string in Unicode normalization form C, for the purposes of re-creating the byte stream accurately from the unicode text.

I personally don't think such a requirement is worth saving a few bytes on the wire.

[dagnelies]

Hmmm... I did not expect unicode normalization to be a potential issue. I expected that what you send from the browser and what you obtain server side to always be identical. But you are right, in case of doubts and to avoid middleware issues, it might be better to send base64. That seems more bullet proof. ... So everything base64 it shall be😉 ... The single leftover exception is not worth it either.

[Firstyear]

Speaking to this one, the clientDataJSON is part of the signed response, so any changes to white space, ordering, or Unicode canonicalization will ruin the response. Hence it is sent as binary rather than as a JSON structure or stringified JSON text.

But it's sent / received into a Uint8Array which is the problem, so when you change it between bytes to base64 you aren't actually affecting or changing any of these things.

[emlun]

• During authentication, the response.userHandle contains the plain text "user.id" like my-john-doe-id and here also the raw value holds more value than its base64url encoded variant.

The spec states that "The user handle MUST NOT contain personally identifying information about the user, such as a username or e-mail address" and "It is RECOMMENDED to let the user handle be 64 random bytes". We should not add features to support a use case we actively recommend against.

I think clientDataJSON should also stay base64url-encoded to keep the emphasis that it needs to be conveyed byte-identically to the value that was signed. More generally I think this JSON variant of the API should mirror the BufferSource variant as closely as possible and not make any structural or semantic changes to the data, otherwise the mismatch will be confusing.

I guess that could be an argument against splitting the root object into two separate types for registration and authentication, but I think that one is fair enough since that is likely how most developers already think of it anyway.

[dagnelies]

Agreed

[MasterKale]

I haven't chimed in because @dwaite and @emlun beat me to my responses. I'm glad to see my intuition was right about how best to handle those values 🙂

Most of the feedback has been handled on my end. The one outstanding change suggestion that I know about right now is the suggestion to include directions for what to do if bad data is passed into these objects, namely throwing an EncodingError. Can someone please look over my addition that attempts to address that? This thing is so close to being ready, I'm very excited 😅

[emlun]

Apart from a couple more nits I think this seems good to go for now. I would like to be a bit more rigorous, but I can't see how to do it without essentially spelling out the entire conversions, which seems too obtuse and fragile. This is probably unambiguous enough, we can revise later if we think of a better way to express it. 🙂

[emlun]

Thanks a lot @MasterKale for taking this on!

[MasterKale]

I like to resolve all comments in my PR's before the changes land, so I'm highlighting the last unresolved comment on this PR. It's a conversation @kreichgauer and I are having on a comment made back in April (we're nearly 100 comments on this 😱): #1703 (comment)

Martin mentioned this:

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.

And I responded:

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.

I'm interested in seeing what others might think about what we're discussing. It might be more useful to continue hashing this out down here, though, since it's easier to find. When some kind of decision is reached I can address whatever might need to change, then go back up into the comment and resolve it.