Revise §9.5. "Authenticator extension processing" #776
Conversation
|
@selfissued @equalsJeffH Can you all please review as soon as possible |
index.bs
Outdated
| 1. Let |processedExtensions| be the result of [=authenticator extension processing=] for each supported [=extension | ||
| identifier=]/input pair in |extensions|. | ||
| 1. Let |processedExtensions| be the result of [=authenticator extension processing=] [=map/for each=] supported <var | ||
| ignore>extensionId</var> → <var ignore>authenticatorExtensionInput</var> in |extensions|. |
There was a problem hiding this comment.
Why did you remove the indexed term [=extension identifier=]? If anything, I think this should say "... supported [=extension identifier=] -> [=authenticator extension input=] pair". Please keep the indexed term and add the second one.
Also, why were you using "var ignore"? I don't understand your goals here.
There was a problem hiding this comment.
I made this change to closer reflect the usage description of map/for each in Infra. The ignore on the <var>s was then necessary to make Bikeshed not complain about that the variables were only used once.
I'll change that back to using the indexed terms.
index.bs
Outdated
| @@ -3562,7 +3562,7 @@ extension=] that does not otherwise require any result values MUST return a valu | |||
| An extension defines one or two request arguments. The <dfn>client extension input</dfn>, | |||
| which is a value that can be encoded in JSON, is passed from the [=[RP]=] to the client | |||
| in the {{CredentialsContainer/get()}} or {{CredentialsContainer/create()}} call, | |||
| while the [=CBOR=] <dfn>authenticator extension input</dfn> is | |||
| while the [=CBOR=] [=authenticator extension input=] is | |||
There was a problem hiding this comment.
I disagree with removing the dfn here and moving it to the processing steps. It makes more sense here. Please revert this change.
There was a problem hiding this comment.
Ok. The reason I did this was so that the <dfn> would be in the same section as the corresponding one for the authenticator outputs.
index.bs
Outdated
| [=authenticator extension=] is included in the extensions data part of the authenticator request. This part is a CBOR map, with | ||
| [=CBOR=] [=extension identifier=] values as keys, and the [=CBOR=] [=authenticator extension input=] value of each extension as | ||
| the value. | ||
| The [=CBOR=] <dfn>authenticator extension input</dfn> value of each processed [=authenticator extension=] is included in the |
There was a problem hiding this comment.
Please remove the "dfn" here.
index.bs
Outdated
|
|
||
| Likewise, the extension output is represented in the [=authenticator data=] as a CBOR map with [=CBOR=] [=extension | ||
| identifiers=] as keys, and the [=CBOR=] <dfn>authenticator extension output</dfn> value of each extension as the value. | ||
| Likewise, the <dfn>authenticator extension output</dfn> value of each processed [=authenticator extension=] is represented by one |
There was a problem hiding this comment.
This is just wrong. The "authenticator extension output" is the value of the key/value pair. You're using the term in a way that makes it sound like it's both the key and the value. Please revert this change.
|
@selfissued Thanks for reviewing. Do you mind if I squeeze #778 in as well? |
|
I will merge this on Friday unless @gmandyam objects by then. |
|
I am fine with the proposed text. Please merge now. |
This aims to fix #775.
I'm not entirely happy with it, but I think it's at least an improvement of section §9.5. What bugs me most is that it feels like "the result of authenticator extension processing" could be taken to mean processing the entire map or just one of the key/value pairs. It feels like it should be possible to rephrase the
steps of the authenticator operations so they don't require iterating over the map, but "authenticator extension processing" currently refers specifically to the processing for a single extension.
Preview | Diff