Separated proposed changes to extension semantics from PR #386 and use TypeError, per @jyasskin #389

selfissued · 2017-03-23T22:06:20Z

Per discussions on the 22-Mar-18 WG call, these semantic changes previously part of the mbj-registries-and-extensions branch proposed as pull request #386 are now in the separate branch mbj-extension-semantics. Thanks to @jyasskin for suggesting the use of the TypeError exception.

@jyasskin

… use TypeError, per @jyasskin

equalsJeffH

I copied over my comments from #386

equalsJeffH · 2017-03-23T23:24:34Z

index.bs

@@ -475,7 +475,8 @@ When this method is invoked, the user agent MUST execute the following algorithm
    1. If |extension| is not supported by this client platform, then [=continue=].

    1. Otherwise, let |result| be the result of running |extension|'s [=client processing=] algorithm on |argument|. If the
-        algorithm returned an error, [=continue=].
+        algorithm returned an error, return [=a promise rejected with=] a {{DOMException}} whose name is
+        "{{TypeError}}", and terminate this algorithm.


this adds signaling of errors from running |extension|'s [=client processing=] algorithm, and aborting of the entire makeCred() operation, where the prior behavior (whether actually good or not) is to ignore errors and go on to the next extension and not abort the overall operation.

Others e.g. @vijaybh @jcjones @jyasskin ought to weigh in on this algorithm change.

This makes sense to me, although I don't have a strong opinion.

If an extension is critical for security, the RP needs to check for it in the response regardless, because of the previous "if not supported then continue" step. If the extension is optional, you still want some way for developers to notice that their argument is malformed and so won't work on any browser. Throwing a TypeError here is plausible. Checking the "exts" extension and then for a missing response for supported extensions would also work, but takes a lot more developer effort.

I can understand the argument that a programmer error (such as a badly formed client argument to an extension) should be flagged. However, this has a couple of problems:

If the specification of an extension evolves in future (e.g. extension adds a new version and the version number is given in the client argument) then this will either leak data or break an operation that could have been completed correctly by ignoring the extension.

Client processing can also fail for a number of other reasons other than malformed arguments (e.g. authenticator selection not finding authenticators of the requested type). In those cases this error becomes misleading and also leaks privacy-sensitive info (which can be used to fingerprint the system).

Per your point 1, Vijay, if an extension is updated in an incompatible way, I'd argue that it should instead be a new extension.

Per your point 2, I'm good with clients throwing appropriate errors when they detect errors.

The point of this change is that it's, in general, a good architectural choice to enable errors to be caught and to fail early a the time they are caught. Ignoring errors makes things hard to debug and allows incorrect code to appear to work, which is almost always a bad thing. Fail fast.

equalsJeffH · 2017-03-23T23:24:50Z

index.bs

@@ -605,7 +606,9 @@ When this method is invoked, the user agent MUST execute the following algorithm
    |extension| → |argument| of <code>{{options}}.{{AssertionOptions/extensions}}</code>:
    1. If |extension| is not supported by this client platform, then [=continue=].
    1. Otherwise, let |result| be the result of running |extension|'s [=client processing=] algorithm on |argument|. If the
-        algorithm returned an error, [=continue=].
+        algorithm returned an error, return [=a promise rejected with=] a {{DOMException}} whose name is
+        "{{TypeError}}", and terminate this algorithm.


equalsJeffH · 2017-03-23T23:28:31Z

index.bs

+set to `true` to signify that the extension was understood and processed.
+Likewise, such an extension that is processed by the authenticator that does not otherwise require any result values
+SHOULD also return a Boolean extension result,
+set to `true` to signify that the extension was understood and processed.


per prior discussion with MikeJ @selfissued, I am inclined to model the above-suggested "the extension was honored" return value as a single bit (or byte) signifying "acknowledged", i.e., a value signifying "ACK" and not as a boolean, because we never intend to return false, and that seems potentially confusing to me.

The choice of "true" as the suggested default extension result was to be parallel with the choice of "true" as the client argument value at https://w3c.github.io/webauthn/#extension-request-parameters. It would be puzzling to use different conventions in the same cases when the reason for these default values is the same.

equalsJeffH · 2017-03-23T23:33:05Z

suggest we retitle this PR to be something like "changes to makeCred() and getAssn() extension handling"

jyasskin · 2017-03-24T18:29:29Z

index.bs

@@ -475,7 +475,8 @@ When this method is invoked, the user agent MUST execute the following algorithm
    1. If |extension| is not supported by this client platform, then [=continue=].

    1. Otherwise, let |result| be the result of running |extension|'s [=client processing=] algorithm on |argument|. If the
-        algorithm returned an error, [=continue=].
+        algorithm returned an error, return [=a promise rejected with=] a {{DOMException}} whose name is


I think you want just "... rejected with=] a {{TypeError}}, ..."

jyasskin · 2017-03-24T18:36:43Z

index.bs

@@ -475,7 +475,8 @@ When this method is invoked, the user agent MUST execute the following algorithm
    1. If |extension| is not supported by this client platform, then [=continue=].

    1. Otherwise, let |result| be the result of running |extension|'s [=client processing=] algorithm on |argument|. If the
-        algorithm returned an error, [=continue=].
+        algorithm returned an error, return [=a promise rejected with=] a {{DOMException}} whose name is
+        "{{TypeError}}", and terminate this algorithm.


This makes sense to me, although I don't have a strong opinion.

If an extension is critical for security, the RP needs to check for it in the response regardless, because of the previous "if not supported then continue" step. If the extension is optional, you still want some way for developers to notice that their argument is malformed and so won't work on any browser. Throwing a TypeError here is plausible. Checking the "exts" extension and then for a missing response for supported extensions would also work, but takes a lot more developer effort.

vijaybh · 2017-04-02T09:15:29Z

index.bs

@@ -475,7 +475,8 @@ When this method is invoked, the user agent MUST execute the following algorithm
    1. If |extension| is not supported by this client platform, then [=continue=].

    1. Otherwise, let |result| be the result of running |extension|'s [=client processing=] algorithm on |argument|. If the
-        algorithm returned an error, [=continue=].
+        algorithm returned an error, return [=a promise rejected with=] a {{DOMException}} whose name is
+        "{{TypeError}}", and terminate this algorithm.


I can understand the argument that a programmer error (such as a badly formed client argument to an extension) should be flagged. However, this has a couple of problems:

If the specification of an extension evolves in future (e.g. extension adds a new version and the version number is given in the client argument) then this will either leak data or break an operation that could have been completed correctly by ignoring the extension.

Client processing can also fail for a number of other reasons other than malformed arguments (e.g. authenticator selection not finding authenticators of the requested type). In those cases this error becomes misleading and also leaks privacy-sensitive info (which can be used to fingerprint the system).

vijaybh · 2017-04-02T09:21:18Z

index.bs

+set to `true` to signify that the extension was understood and processed.
+Likewise, such an extension that is processed by the authenticator that does not otherwise require any result values
+SHOULD also return a Boolean extension result,
+set to `true` to signify that the extension was understood and processed.


This paragraph seems redundant with the preceding one.

IMO there is no such thing as an extension that is not designed to enable the RP to know if it was understood and acted on. The rest of the paragraph is just recommending returning "true" as a specific way of letting the RP know the extension was honored (the latter part was specified in the previous paragraph). If we do want to add this, it seems like it could be added to the previous paragraph.

OK, I'll restructure this text as you suggest, Vijay.

@jyasskin

…s by @jyasskin and @vijaybh.

vijaybh · 2017-04-10T21:45:08Z

index.bs

@@ -475,7 +475,7 @@ When this method is invoked, the user agent MUST execute the following algorithm
    1. If |extension| is not supported by this client platform, then [=continue=].

    1. Otherwise, let |result| be the result of running |extension|'s [=client processing=] algorithm on |argument|. If the
-        algorithm returned an error, return [=a promise rejected with=] a {{DOMException}} whose name is
+        algorithm returned an error, return [=a promise rejected with=] a
        "{{TypeError}}", and terminate this algorithm.


I still don't like this. It seems to me like it is actually losing information for the developer. For instance, the appid extension's client processing is defined as "If {{AssertionOptions/rpId}} is present, reject promise with a DOMException whose name is "{{NotAllowedError}}", and terminate this algorithm." This text would catch that exception and replace it with TypeError. Why not simply terminate this algorithm and propagate the error returned from the client processing?

I'm fine with your suggestion to terminate the algorithm and propagate the error returned from the client processing. The main thing I was trying to achieve was to enable errors to be caught and cause the operation to fail at that time, rather than being silently ignored.

vijaybh · 2017-04-10T21:45:35Z

index.bs

@@ -606,7 +606,7 @@ When this method is invoked, the user agent MUST execute the following algorithm
    |extension| → |argument| of <code>{{options}}.{{AssertionOptions/extensions}}</code>:
    1. If |extension| is not supported by this client platform, then [=continue=].
    1. Otherwise, let |result| be the result of running |extension|'s [=client processing=] algorithm on |argument|. If the
-        algorithm returned an error, return [=a promise rejected with=] a {{DOMException}} whose name is
+        algorithm returned an error, return [=a promise rejected with=] a
        "{{TypeError}}", and terminate this algorithm.


See above comment about propagating the underlying error instead of masking it like this.

vijaybh · 2017-04-10T21:50:31Z

index.bs

@@ -2400,6 +2397,9 @@ error.
    the value of |rpId| for all client processing should be replaced by the
    value of |appid|.

+: Client data
+:: Returns the JSON value `true` to indicate to the RP that the extension was acted upon
+


I think having this section called out explicitly is useful. However "Client data" should say how the ClientData is actually changed - that means specifying a key and a value in the JSON. In this case, the client processing already specifies such a change by replacing the rpId; maybe this section should just point out this fact. In general "true" is not useful in this context because in order to validate it the RP has to remember what it asked for in the request and verify that against the rpId in the ClientData; it can do the latter even in the absence of a "true" value here.

Might this be clearer if the six standard sections had these titles, rather than the current ones?

Client input data

Client processing

Client output data

Authenticator input data

Authenticator processing

Authenticator output data

What you're mostly talking about I think is client processing, rather than the client output data. Of course, you could be proposing that a particular extension, the "appid" extension, have a different return value than "true". That's fine with me. If so, make a proposal.

vijaybh · 2017-04-10T21:53:51Z

index.bs

@@ -2424,6 +2424,9 @@ prompt string, intended for display on a trusted device on the authenticator.
 : Client processing
 :: None, except default forwarding of client argument to authenticator argument.

+: Client data
+:: Returns the authenticator data string as JSON string
+


ClientData (which must be hashed and fed into the authenticator for signature) cannot depend on AuthenticatorData (which comes out of the authenticator after the signature has been computed). The "Client processing" section above already defines what should happen here - the ClientData should be augmented with a key "txAuthSimple", with string value equal to the supplied client argument.

vijaybh · 2017-04-10T21:59:21Z

index.bs

@@ -2454,6 +2457,9 @@ allows authenticators without a font rendering engine to be used and also suppor
 : Client processing
 :: None, except default forwarding of client argument to authenticator argument.

+: Client data
+:: Returns the base64url encoding of the authenticator data value as a JSON string
+


ClientData (which must be hashed and fed into the authenticator for signature) cannot depend on AuthenticatorData (which comes out of the authenticator after the signature has been computed). The "Client processing" section above already defines what should happen here - the ClientData should be augmented with a key "txAuthGeneric", with string value equal to the JSON serialization of the client argument.

However this extension has an additional problem: Client argument should be something passed by JavaScript, and it is defined here as CBOR. We should either define this as a CBOR string encoded in say base64, or define it as a dictionary that gets CBOR-encoded before passing to the authenticator.

vijaybh · 2017-04-10T22:36:21Z

index.bs


 : Authenticator processing
-:: None.
+:: none


Editorial nit: Why omit capitalization and punctuation in this one item when others are capitalized and punctuated?

vijaybh · 2017-04-10T22:37:45Z

index.bs

@@ -2517,6 +2528,9 @@ This [=registration extension=] enables the [RP] to determine which extensions t
 : Client processing
 :: None, except default forwarding of client argument to authenticator argument.

+: Client data
+:: Returns the list of supported extensions as a JSON array of extension identifier strings
+


ClientData (which must be hashed and fed into the authenticator for signature) cannot depend on AuthenticatorData (which comes out of the authenticator after the signature has been computed). In this case it is better to include no client data since the authenticator data will reflect the option anyways.

vijaybh · 2017-04-10T22:38:09Z

index.bs

@@ -2541,6 +2555,9 @@ This [=registration extension=] and [=authentication extension=] enables use of
 : Client processing
 :: None, except default forwarding of client argument to authenticator argument.

+: Client data
+:: Returns a JSON string containing the base64url encoding of the authenticator data
+


ClientData (which must be hashed and fed into the authenticator for signature) cannot depend on AuthenticatorData (which comes out of the authenticator after the signature has been computed). TShould be "none".

vijaybh · 2017-04-10T22:38:27Z

index.bs

+: Client data
+:: Returns a JSON object that encodes the location information in the authenticator data as a Coordinates value,
+    as defined by [The W3C Geolocation API Specification](https://dev.w3.org/geo/api/spec-source.html#coordinates_interface).
+


ClientData (which must be hashed and fed into the authenticator for signature) cannot depend on AuthenticatorData (which comes out of the authenticator after the signature has been computed). Should be "none".

vijaybh · 2017-04-10T22:38:45Z

index.bs

@@ -2644,6 +2664,9 @@ This [=registration extension=] and [=authentication extension=] enables use of
 : Client processing
 :: None, except default forwarding of client argument to authenticator argument.

+: Client data
+:: Returns a JSON array of 3-element arrays of numbers that encodes the factors in the authenticator data
+


ClientData (which must be hashed and fed into the authenticator for signature) cannot depend on AuthenticatorData (which comes out of the authenticator after the signature has been computed). Should be "none".

…ension-semantics

…ons more precise.

jyasskin

This is significantly better than what was there before; thank you! I've also left several comments, but if you want to merge this and handle the comments in a separate PR, that's fine.

jyasskin · 2017-04-19T04:15:40Z

index.bs

@@ -454,7 +454,8 @@ When this method is invoked, the user agent MUST execute the following algorithm
    1. If |extension| is not supported by this client platform, then [=continue=].

    1. Otherwise, let |result| be the result of running |extension|'s [=client processing=] algorithm on |argument|. If the
-        algorithm returned an error, [=continue=].
+        algorithm returned an error, return [=a promise rejected with=] either that error or a


I'm skeptical of letting the UA choose which error to return, since it'll reduce interoperability. What was the reason for that? If it was a weak reason, I'd rather just unconditionally forward the client processing algorithm's error.

jyasskin · 2017-04-19T14:01:38Z

index.bs

 authentication extensions) or {{makeCredential()}} call (for [=registration extensions=]) to the client platform. The client
 platform performs additional processing for each extension that it supports, and augments the [=client data=] as required by the
-extension. In addition, the client collects the authenticator arguments for the above extensions, and passes them to the
+extension. In addition, the client collects the [=authenticator extension input=] values for the above extensions, and passes them to the


Is it that the client extension output becomes the authenticator extension input, or does the authenticator extension input come from somewhere else too? (This may be addressed farther down in the patch.)

jyasskin · 2017-04-19T14:03:46Z

index.bs

 authenticator. Since all extensions are optional, this will not cause a functional failure in the API operation.
+Likewise, clients can choose to produce a [=client extension output=] value for an extension that it does not understand by encoding the


Oh, the client extension output is created from the authenticator extension output, not from the result of the client processing steps? That wasn't clear from the discussion above. Does it mean that we need two client processing algorithms, one to process the input, and another to process the output?

the client extension output is created from the authenticator extension output, not from the result of the client processing steps?

It seems the answer is "no, see 1st parag of [[sctn-client-extension-processing]]"

Does it mean that we need two client processing algorithms, one to process the input, and another to process the output?

that seems to be addressed by the last parag of [[sctn-client-extension-processing]] in combination with section [[#sctn-authenticator-extension-processing]].

we could perhaps have better linking to the latter sections from this opening [[#extensions]] section.

The next commit will make it clear that the client processing steps take in the [=client extension input=] and produce the [=authenticator extension input=] and they also take in the [=authenticator extension output=] and produce the [=client extension output=].

jyasskin · 2017-04-19T14:10:21Z

index.bs

 {{MakeCredentialOptions/extensions}} option to the {{makeCredential()}} or {{getAssertion()}} call. The entry key MUST be the
-extension identifier, and the value MUST be the [=client argument=].
+[=extension identifier=], and the value MUST be the [=client extension input=].


You didn't introduce this problem, but we can't MUST the Relying Party in this section since that mixes conformance requirements on the UA+Authenticator vs the Relying Party. Instead, I suggest saying something like "The entry key is interpreted as the extension identifier, and the value is interpreted as the client extension input." You could omit "interpreted as" if you prefer.

jyasskin · 2017-04-19T14:10:52Z

index.bs


 <pre class="example" highlight="js">
    var assertionPromise = credentials.getAssertion(..., /* extensions */ {
        "webauthnExample_foobar": 42
    });
 </pre>

-Extension definitions MUST specify the valid values for their client argument. Clients SHOULD ignore extensions with an invalid
-client argument. If an extension does not require any parameters from the [RP], it SHOULD be defined as taking a Boolean client
+Extension definitions MUST specify the valid values for their [=client extension input=]. Clients SHOULD ignore extensions with a invalid


"with an invalid"

jyasskin · 2017-04-19T14:56:23Z

index.bs

-: Client processing
-:: None, except default forwarding of client argument to authenticator argument.
+: Client extension processing
+:: None, except creating the authenticator extension input from the client extension input.


Maybe "[=utf-8 encode=] the client extension input into a CBOR text string."

jyasskin · 2017-04-19T14:57:24Z

index.bs

@@ -2439,20 +2461,23 @@ prompt string, intended for display on a trusted device on the authenticator.
 : Extension identifier
 :: `txAuthSimple`

-: Client argument
+: Client extension input
 :: A single [=UTF-8 encoded=] string prompt.


I believe the client extension input should be a Javascript type, and Javascript doesn't have UTF-8 strings. It have DOMStrings, which are sequences of 16-bit code units and USVStrings, which are UTF-16.

jyasskin · 2017-04-19T14:58:14Z

index.bs

-: Authenticator argument
-:: The client argument encoded as a CBOR text string (major type 3).
+: Authenticator extension input
+:: The client extension input encoded as a CBOR text string (major type 3).


This could just be "A CBOR text string" since the client extension processing is in charge of producing it.

jyasskin · 2017-04-19T14:59:06Z

index.bs

 :: The authenticator MUST display the prompt to the user before performing either [=user verification=] or [=test of user
    presence=]. The authenticator may insert line breaks if needed.

-: Authenticator data
+: Authenticator extension output
 :: A single [=UTF-8 encoded=] string, representing the prompt as displayed (including any eventual line breaks).


Is this a CBOR text string?

jyasskin · 2017-04-19T15:05:29Z

index.bs

+:: None, except creating the authenticator extension input from the client extension input.
+
+: Client extension output
+:: Returns the authenticator extension output string as JSON string


Assuming this is the algorithm to convert the authenticator extension output into the client extension output, you'll want to mention that it [=utf-8 decodes=] the CBOR into a DOMString.

equalsJeffH

Hi, bikeshed generates linking errors when processing this file. they stem in part from the lines I've commented on below. there's perhaps further ones to address once those are resolved.

I'll review other PRs while these nominal issues are addressed -- i'd like to generate an error-free index.html in order to further review.

Here's what bikeshed says:

> curl https://api.csswg.org/bikeshed/ -F file=@index.bs -F force=1 -F output=err
\033[1;96mWARNING:\033[0m Multiple elements have the same ID 'client-extension-processing'.
Deduping, but this ID may not be stable across revisions.
\033[1;96mWARNING:\033[0m Multiple elements have the same ID 'authenticator-extension-processing'.
Deduping, but this ID may not be stable across revisions.
\033[1;33mLINK ERROR:\033[0m No 'dfn' refs found for 'client processing' that are marked for export.
&lt;a data-link-type="dfn" data-lt="client processing">client processing&lt;/a>
\033[1;33mLINK ERROR:\033[0m No 'dfn' refs found for 'client processing' that are marked for export.
&lt;a data-link-type="dfn" data-lt="client processing" data-link-for-hint="ScopedCredential/[[DiscoverFromExternalSource]](options)">client processing&lt;/a>
\033[1;33mLINK ERROR:\033[0m No 'dfn' refs found for 'authenticator extension output'.
&lt;a data-link-type="dfn" data-lt="authenticator extension output">authenticator extension output&lt;/a>
\033[1;33mLINK ERROR:\033[0m No 'dfn' refs found for 'client extension output'.
&lt;a data-link-type="dfn" data-lt="client extension output">client extension output&lt;/a>
\033[7;32m ✔ \033[0m Successfully generated, with 4 linking errorsLM-SJN-0087

equalsJeffH · 2017-04-19T21:40:53Z

index.bs


 Note: Extensions should aim to define authenticator arguments that are as small as possible. Some authenticators communicate
    over low-bandwidth links such as Bluetooth Low-Energy or NFC.


-## Extending <dfn>client processing</dfn> ## {#extension-client-processing}
+## <dfn>Client extension processing</dfn> ## {#client-extension-processing}


there's a couple problems with the change in this line:

it removes the dfn for "client processing", thus occurrences of [=client processing=] are dangling, and,

<dfn>Client extension processing</dfn> and {#client-extension-processing} both cause bikeshed to render html elements with ids of "client-extension-processing"

equalsJeffH · 2017-04-19T21:43:53Z

index.bs


-## Extending authenticator processing ## {#extension-authenticator-processing}
+## <dfn>Authenticator extension processing</dfn> ## {#authenticator-extension-processing}


similar issues in this line as in comment above on L2388

selfissued · 2017-04-19T23:27:13Z

Thanks, Jeff. I've corrected the indexing errors.

equalsJeffH

Thanks MikeJ (@selfissued), this is overall great. I did find some modest things to fix (a couple of which are technical) -- plus I agree with @jyasskin's comments.

equalsJeffH · 2017-04-20T16:24:50Z

index.bs

-    1. Otherwise, let |result| be the result of running |extension|'s [=client processing=] algorithm on |argument|. If the
-        algorithm returned an error, [=continue=].
+    1. Otherwise, let |result| be the result of running |extension|'s [=client extension processing=] algorithm on |argument|. If the
+        algorithm returned an error, return [=a promise rejected with=] either that error or a


a) This algorithm is now synchronous, see the Note: up above in the source.

b) I do not think we can simply say "return that error" (the one returned by running [=client extension processing=]) because who knows what sort of error codes those are and whether they play nice in the browser-guts context in which this alg runs? Perhaps there is some way to return either a "{{TypeError}}" or the extension-generated error packaged-up somehow to differentiate it? OR, perhaps we need to stipulate that all extensions MUST return only DOMExceptions from https://heycam.github.io/webidl/#dfn-error-names-table ?

also, this is still fundamentally changing how extensions returning errors are handled, see #389 (comment) and #389 (comment).

Is there some way to note the error and [=continue=] with remaining extensions?

Since this aspect of the PR still seems to be generating discussion, I'll plan to remove this part and file it as a separate issue, letting us continue discussing it, while applying the changes in the PR that are non-controversial clear improvements over what's there.

equalsJeffH · 2017-04-20T16:28:27Z

index.bs

@@ -666,8 +667,9 @@ When this method is invoked, the user agent MUST execute the following algorithm
 1. If the {{ScopedCredentialRequestOptions/extensions}} member of |scopedOptions| is [=present=], then [=map/for each=]
    |extension| → |argument| of <code>|scopedOptions|.{{ScopedCredentialRequestOptions/extensions}}</code>:
    1. If |extension| is not supported by this client platform, then [=continue=].
-    1. Otherwise, let |result| be the result of running |extension|'s [=client processing=] algorithm on |argument|. If the
-        algorithm returned an error, [=continue=].
+    1. Otherwise, let |result| be the result of running |extension|'s [=client extension processing=] algorithm on |argument|. If the


above comment applies here also.

equalsJeffH · 2017-04-20T17:52:10Z

index.bs

@@ -1282,8 +1284,8 @@ The [=authenticator data=] structure is a byte array of 37 bytes or more, as fol
    <tr>
        <td>variable (if present)</td>
        <td>
-            Extension-defined [=authenticator data=]. This is a CBOR [[RFC7049]] map with extension identifiers as keys, and
-            extension [=authenticator data=] values as values. See [[#extensions]] for details.
+            Extension-defined [=authenticator data=]. This is a CBOR [[RFC7049]] map with [=extension identifier=] values as keys, and


rather than [=extension identifier=] values, [=extension identifiers=] may simply magically link (delete "values"), or if not, this will: [=extension identifier|extension identifiers=].

equalsJeffH · 2017-04-20T17:55:06Z

index.bs

@@ -2269,23 +2273,23 @@ The mechanism for generating scoped credentials, as well as requesting and gener
 [[#api]], can be extended to suit particular use cases. Each case is addressed by defining a <dfn>registration extension</dfn>
 and/or an <dfn>authentication extension</dfn>. Extensions can define additions to the following steps and data:

- {{CredentialsContainer/create()|navigator.credentials.create()}} request parameters for [=registration extension=].
+- {{CredentialsContainer/create()|navigator.credentials.create()}} request and response parameters for [=registration extensions=].


should this be "request parameters and response values" ?

equalsJeffH · 2017-04-20T17:55:18Z

index.bs


- {{CredentialsContainer/get()|navigator.credentials.get()}} request parameters for [=authentication extensions=].
+- {{CredentialsContainer/get()|navigator.credentials.get()}} request and response parameters for [=authentication extensions=].


equalsJeffH · 2017-04-20T18:02:41Z

index.bs

-[=authenticatorGetAssertion=] call (for [=authentication extensions=]). These authenticator arguments are passed as name-value
-pairs, with the extension identifier as the name, and the corresponding authenticator argument as the value. The authenticator,
-in turn, performs additional processing for the extensions that it supports, and augments the [=authenticator data=] as
+required by the extension. In addition, the client collects the [=authenticator extension input=] for the above extensions, and passes


hm, seems we are lacking a definitions for "authenticator extension" and "client extension" -- probably should have them.

equalsJeffH · 2017-04-20T18:10:48Z

index.bs


- Authenticator processing, and the [=authenticator data=], for [=registration extensions=] and [=authentication extensions=].
+- Authenticator extension request parameters, processing, and response parameters for [=registration extensions=] and [=authentication extensions=].

 When requesting an assertion for a scoped credential, a [RP] can list a set of extensions to be used, if they are supported by


should this first sentence more properly be:

When creating a [=scoped credential=] or requesting an [=authentication assertion=], a [RP] can list a set of extensions. These extensions will be invoked during the requested operation if they are supported by the client and/or the authenticator.

..?

equalsJeffH · 2017-04-20T18:35:17Z

index.bs

-values in the [=client data=], [=authenticator data=] (in the case of [=authentication extensions=]), or both. Finally, if the
-extension requires any authenticator processing, it must also specify an authenticator argument to be sent via the
-[=authenticatorGetAssertion=] or [=authenticatorMakeCredential=] call.
+A definition of an extension must specify an [=extension identifier=], a [=client extension input=] argument


nit: s/sent/to be sent/ ?

equalsJeffH · 2017-04-20T18:36:52Z

index.bs

+A definition of an extension must specify an [=extension identifier=], a [=client extension input=] argument
+sent via the {{CredentialsContainer/get()}} or {{CredentialsContainer/create()}} call,
+the [=client extension processing=] rules, and a [=client extension output=] value.
+If the extension communicates with the authenticator (meaning it is an [=authentication extension=]),


could not an "authenticator extension" be invoked at either or both create() or get() time? i.e., a "registration extension" could involve authenticator processing, no? see also comment above on L2288, as well as the rest of the sentence this comment is on...

equalsJeffH · 2017-04-20T20:14:24Z

index.bs

 be any data that can be encoded in CBOR. An authenticator that processes an [=authentication extension=] that defines such a
-value must include it in the [=authenticator data=].
+value must include it in the [=authenticator data=] extensions section.


it seems that this sentence..

An authenticator that processes an [=authentication extension=] that defines such a value must include it in the [=authenticator data=] extensions section.

..and this sentence..

As specified in [[#sec-authenticator-data]], the [=authenticator extension input=] value of each processed extension is included in the
-extended data part of the [=authenticator data=].

..are redundant?

equalsJeffH

Thanks @selfissued -- this improves extension exposition by orders of magnitude :)

I am fine with merging this now -- LGTM! The only thing I can think of we might want to do (eg for CR milestone) is to delineate the orthongonality of {registration,authentication} extensions from {authenticator,client} extensions in some fashion (may include devising enhanced terminology, i dunno right now) (perhaps an issue to submit...)

equalsJeffH · 2017-04-21T00:18:46Z

merged it since we were authz'd to do so on the Wed 19-Apr-2017 webauthn call.

@jyasskin

…emantics from PR #386 and use TypeError, per @jyasskin (#389)

jyasskin · 2017-04-21T00:21:45Z

index.bs

@@ -2495,7 +2509,7 @@ error.
 :: `appid`

 : Client extension input
-:: A single [=UTF-8 encoded=] string specifying a FIDO |appId|.
+:: A single JSON string specifying a FIDO |appId|.


To be nitpicky, since the extension input is a Javascript object, it's a {{DOMString}} rather than a JSON string. JSON is a serialization of many Javascript objects, but it's not the type of things that get passed to web APIs.

No need to fix this before merging, just mentioning it for future patches.

Separated proposed changes to extension semantics from PR w3c#386 and…

878739c

… use TypeError, per @jyasskin

equalsJeffH reviewed Mar 23, 2017

View reviewed changes

equalsJeffH added this to the CR milestone Mar 23, 2017

equalsJeffH added subtype:algorithms/WebIDL type:technical labels Mar 23, 2017

equalsJeffH assigned selfissued Mar 23, 2017

equalsJeffH requested review from jyasskin, jcjones and vijaybh March 23, 2017 23:30

jyasskin approved these changes Mar 24, 2017

View reviewed changes

selfissued mentioned this pull request Apr 2, 2017

Processing model for extensions is very underdefined #270

Closed

selfissued added the priority:implementation label Apr 2, 2017

vijaybh suggested changes Apr 2, 2017

View reviewed changes

selfissued added 3 commits April 7, 2017 13:03

Merged changes made to master

c6cd5a7

Added client data descriptions to all extensions. Accepted suggestion…

7dc05a9

…s by @jyasskin and @vijaybh.

Addressed comments by @jyasskin in issue w3c#270

1a41514

vijaybh suggested changes Apr 10, 2017

View reviewed changes

selfissued added 2 commits April 18, 2017 16:06

Merge branch 'master' of https://github.com/w3c/webauthn into mbj-ext…

83b3f85

…ension-semantics

Gave distinct names to extension inputs and outputs to make descripti…

5fa1c98

…ons more precise.

jyasskin reviewed Apr 19, 2017

View reviewed changes

equalsJeffH modified the milestones: WD-05, CR Apr 19, 2017

Merged with master now that PR w3c#384 is in place

b02764f

equalsJeffH suggested changes Apr 19, 2017

View reviewed changes

Corrected indexing errors

c568f93

equalsJeffH suggested changes Apr 20, 2017

View reviewed changes

selfissued mentioned this pull request Apr 20, 2017

UVM should be "method" rather than "mode" ? #411

Closed

Addressed additional comments by Jeff Hodges and Jeffrey Yasskin

c795c31

equalsJeffH approved these changes Apr 21, 2017

View reviewed changes

equalsJeffH merged commit 8eb7b5c into w3c:master Apr 21, 2017

WebAuthnBot pushed a commit that referenced this pull request Apr 21, 2017

Built by Travis-CI: 8eb7b5c Separated proposed changes to extension s…

3718861

…emantics from PR #386 and use TypeError, per @jyasskin (#389)

jyasskin reviewed Apr 21, 2017

View reviewed changes

selfissued mentioned this pull request Feb 16, 2018

agl doesn't understand extensions #803

Closed

		authenticator. Since all extensions are optional, this will not cause a functional failure in the API operation.
		Likewise, clients can choose to produce a [=client extension output=] value for an extension that it does not understand by encoding the


		## Extending authenticator processing ## {#extension-authenticator-processing}
		## <dfn>Authenticator extension processing</dfn> ## {#authenticator-extension-processing}


		- {{CredentialsContainer/get()\|navigator.credentials.get()}} request parameters for [=authentication extensions=].
		- {{CredentialsContainer/get()\|navigator.credentials.get()}} request and response parameters for [=authentication extensions=].

Separated proposed changes to extension semantics from PR #386 and use TypeError, per @jyasskin #389

Separated proposed changes to extension semantics from PR #386 and use TypeError, per @jyasskin #389

Conversation

selfissued commented Mar 23, 2017

equalsJeffH left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

equalsJeffH commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyasskin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

equalsJeffH left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selfissued commented Apr 19, 2017

equalsJeffH left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

equalsJeffH Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

equalsJeffH left a comment

Choose a reason for hiding this comment

equalsJeffH commented Apr 21, 2017

Choose a reason for hiding this comment

equalsJeffH Apr 20, 2017 •

edited

Loading