add requireResidentKey param to the invocation step of authenticatorMakeCredential #502
Conversation
There was a problem hiding this comment.
overall this is a simple change and this PR addresses it.
Although, detail-wise, I suggest we have the argument lists for authenticator operations denoted in the #createCredential and #getAssertion algs match the lists, including ordering, as denoted in the Authenticator Operations.
Presently, in (5.2.1. The authenticatorMakeCredential operation)[https://w3c.github.io/webauthn/#op-make-cred], "The requireResidentKey parameter..." is listed last in the params list. I'd place it after the list of PublicKeyCredentialDescriptor objects, and before the "Extension data", and then have the param list here in in {#createCredential} step 24.3 match ordering-wise.
index.bs
Outdated
| @@ -28,7 +28,7 @@ Text Macro: RP Relying Party | |||
| Text Macro: RPS Relying Parties | |||
| Text Macro: INFORMATIVE <em>This section is not normative.</em> | |||
| Text Macro: WAC WebAuthn Client | |||
| Ignored Vars: op, alg, type, algorithm | |||
| Ignored Vars: op, alg, type, algorithm, authenticatorSelection | |||
There was a problem hiding this comment.
why add authenticatorSelection here?
index.bs
Outdated
| @@ -649,6 +649,7 @@ When this method is invoked, the user agent MUST execute the following algorithm | |||
| 1. Otherwise, [=list/Append=] |C| to |excludeCredentialDescriptorList|. | |||
| 1. [=In parallel=], invoke the [=authenticatorMakeCredential=] operation on |authenticator| with |rpId|, | |||
| |clientDataHash|, |options|.{{MakeCredentialOptions/rp}}, |options|.{{MakeCredentialOptions/user}}, | |||
| <code>|options|.|authenticatorSelection|.{{AuthenticatorSelectionCriteria/requireResidentKey}}</code>, | |||
There was a problem hiding this comment.
I think you can replace |authenticatorSelection| with {{MakeCredentialOptions/authenticatorSelection}} in order to remove it from Ignored Vars, but IM me if that gives you bikeshed errors.
Add Rate Limiting definition to terminology section
Algorithm Fix-up (Continued, #2)
…arameters to pubKeyCredParams
…renaming Renamed MakeCredentialOptions to MakePublicKeyCredentialOptions and parameters to pubKeyCredParams
| 1. If <code>|C|.{{transports}}</code> [=list/is not empty=], and |authenticator| is connected over a transport not | ||
| mentioned in <code>|C|.{{transports}}</code>, the client MAY [=continue=]. | ||
| 1. Otherwise, [=list/Append=] |C| to |excludeCredentialDescriptorList|. | ||
| 1. [=In parallel=], invoke the [=authenticatorMakeCredential=] operation on |authenticator| with |rpId|, | ||
| |clientDataHash|, |options|.{{MakeCredentialOptions/rp}}, |options|.{{MakeCredentialOptions/user}}, | ||
| |clientDataHash|, |options|.{{MakePublicKeyCredentialOptions/rp}}, |options|.{{MakePublicKeyCredentialOptions/user}}, | ||
| <code>|options|.{{MakePublicKeyCredentialOptions/authenticatorSelection}}.{{AuthenticatorSelectionCriteria/requireResidentKey}}</code>, |
There was a problem hiding this comment.
the param ordering here does not match the ordering down in Line 1556 thru 1559. I suggest they match ordering-wise. thx.
|
Modulo my line-level comment above, this looks ok I think (am sorta rushed right now and the conflict-fixing added in a bunch of noise into the diff...) |
|
Jeff. maybe I'm tired, but I couldn't figure out the ordering mismatch. Can you submit a PR fixing this and I'll quickly review it and approve it? Thanks. |
…ation step of authenticatorMakeCredential (#502)
fixes #485
Preview | Diff