Move getAssertion()'s challenge into AssertionOptions
#398
Conversation
This patch adds an 'AuthenticatorResponse' interface, representing the generic attributes of responses from authenticators. It then redefines 'ScopedCredentialInfo' and 'AuthenticatorAssertion' to derive from this interface, and renames them to 'AuthenticatorAttestionResponse' and 'AuthenticatorAssertionResponse' respectively. These new interfaces are a drop-in replacement for the old interfaces, no normative changes are intended in this patch, other than the renaming.
Passing a single dictionary parameter into `getAssertion()` provides for greater forward compatibility, as new data can be flexibly added to the method invocation without restructuring the existing structure. It also helps developers understand what they're passing in. This is less important for `getAssertion()` than it is for `makeCredential()`, obviously, but aligning both in a similar structure seems like a good change to make.
|
As @vijaybh suggeted in #2 of https://lists.w3.org/Archives/Public/public-webauthn/2017Apr/0158.html, moving |
index.bs
Outdated
| @@ -892,8 +883,12 @@ user consent to a specific transaction. The structure of these signatures is def | |||
|
|
|||
| ## Additional options for Assertion Generation (dictionary <dfn dictionary>AssertionOptions</dfn>) ## {#assertion-options} | |||
There was a problem hiding this comment.
Editorial: Could we remove the word "additional here, since this is now all the options?
There was a problem hiding this comment.
Good call. Done in 0039a13.
There was a problem hiding this comment.
this seems nominally fine to me, thx @mikewest! My recollection is that we had kept the challenge separate from the truly optional options because the challenge is a must-have. So "options" are now not truely all options. Is the term "options" typically used to name the parameters dictionary passed to web platform methods? (I'd be inclined to term it "params" or "parameters" but it is not a big deal)
As I'm sure you noted, the |
|
How about |
understood and agreed. |
|
I noted in #398 (comment):
@mikewest replied:
Well, in thinking about it let's leave that argument/parameter name as "options" -- "request", as attempted 3a5fefb, seems misleading because the dictionary semantically contains options/arguments/parameters and "options" is shorter. So going with the two commits -- a9da992 and 0039a13 -- works for me. |
| @@ -557,30 +554,24 @@ authorizing an authenticator. | |||
|
|
|||
| ### Use an existing credential - getAssertion() method ### {#getAssertion} | |||
|
|
|||
| <div link-for-hint="WebAuthentication/getAssertion(assertionChallenge, options)"> | |||
| <div link-for-hint="WebAuthentication/getAssertion(options)"> | |||
There was a problem hiding this comment.
shouldn't options be request here?
index.bs
Outdated
| [[#assertion-options]]. | ||
|
|
||
| </ul> | ||
| : <dfn>options</dfn> |
There was a problem hiding this comment.
I suggest not naming this request and just using options or parameters here (see #398 (comment)).
|
On-call: Drop the 3rd commit, and then clear to merge. |
|
LGTM, thanks! |
Passing a single dictionary parameter into
getAssertion()providesfor greater forward compatibility, as new data can be flexibly added
to the method invocation without restructuring the existing
structure. It also helps developers understand what they're passing
in. This is less important for
getAssertion()than it is formakeCredential(), obviously, but aligning both in a similarstructure seems like a good change to make.