-
Notifications
You must be signed in to change notification settings - Fork 167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Convert makeCredential()
's parameters into a dictionary.
#399
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.
Passing a single dictionary parameter into `makrCredential()` 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, as each parameter will be labeled. This patch restructures the data passed into `makeCredential()` substantially, moving from four parameters to a single dictionary, and merging some existing types into a simpler structure. Most of it is straightforward; the only bit I know will be controversial is dropping `RelyingPartyUserInfo` in favor of two instances of a simpler `ScopedCredentialEntity` object: one for the RP, one for the user. Let's chat about how (un)reasonable this approach might be.
As @vijaybh suggeted in #2 of https://lists.w3.org/Archives/Public/public-webauthn/2017Apr/0158.html, moving getAssertion() and makeCredential() to dictionary-based invocation rather than list-of-parameter-based invocation seems pretty reasonable. This patch migrates |
index.bs
Outdated
@@ -889,6 +792,102 @@ user consent to a specific transaction. The structure of these signatures is def | |||
[[#op-get-assertion]]. | |||
</div> | |||
|
|||
## Additional options for Attestation (dictionary <dfn dictionary>MakeCredentialOptions</dfn>) ## {#dictionary-makecredentialoptions} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editorial: this isn't "additional options" any more, it's all the options. Maybe say something like "Parameters for credential creation" instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Picked up in 6dca154.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, as i noted in review of PR #398 maybe these are not "options" but are parameters, some of which are optional ?
index.bs
Outdated
BufferSource attestationChallenge, | ||
optional ScopedCredentialOptions options | ||
); | ||
Promise<ScopedCredentialInfo> makeCredential(MakeCredentialOptions options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<bikeshed> I like the request
terminology that you had in PR #398. Maybe also rename MakeCredentialOptions
to MakeRequest
or CredentialRequest
? </bikeshed>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, I do not like the term request
here because that dict instance is not a "request" in an of itself. I much prefer just using options
or parameters
here (see #398 (comment)). If I were to implement, I'd find request
misleading/confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with whatever term we use, as long as it's consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we agreed that we're running with options
for both.
index.bs
Outdated
|
||
// Attestation configuration: | ||
required BufferSource challenge; | ||
required sequence<ScopedCredentialParameters> parameters; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think these are the attestation parameters -- they're the parameters for the type of credential. We don't have parameters that allow an RP to request a particular type of attestation, though maybe we should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. the challenge is not a part of attestation per-se -- it provides per-protocol-run roundtrip integrity whether the protocol run is a registration operation or an authn operation.
ScopedCredentialParameters are params for the creation of the underlying credential key pair, which then is attested. see #404
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If y'all have suggestions for both parameters
and Attestation configuration
, I'm all ears. I don't have strong opinions about either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we agreed on the call to drop the comment, and leave the names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think "drop the comment, and leave the names" is fine for now", thx.
index.bs
Outdated
<xmp class="idl"> | ||
dictionary MakeCredentialOptions { | ||
// Relying Party / User Metadata: | ||
required ScopedCredentialEntity rp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, so we now have the same data for an rp and the user. Cool!
index.bs
Outdated
|
||
<xmp class="idl"> | ||
dictionary ScopedCredentialEntity { | ||
DOMString id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In RelyingPartyUserInfo
some of these were required, why the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to @leshi's question "In RelyingPartyUserInfo
some of these were required, why the change?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest making id
and name
required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the call, Jeffrey & Mike mentioned that requiring developers to set RP is a bad idea and the params that were required in RelyingPartyUserInfo can be enforced in the algorithm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I am fine with these changes. there's some modest items to fix before merging. thanks @mikewest !
index.bs
Outdated
1. If the {{ScopedCredentialEntity/id}} member of {{options}}.{{MakeCredentialOptions/rp}} is [=present|not present=], then set | ||
|rpId| to |callerOrigin|. | ||
|
||
Otherwise: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately, the as-rendered indent of this "otherwise" block is now lost and so it is visually confusing that this is a block of substeps. we need to fix this.
|
||
1. Let |normalizedParameters| be a new [=list=] whose [=list/items=] are pairs of ScopedCredentialType and a [=dictionary=] type | ||
(as returned by [=normalizing an algorithm=]). | ||
1. Let |normalizedParameters| be a new [=list=] whose [=list/items=] are pairs of {{ScopedCredentialType}} and a [=dictionary=] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately, now the as-rendered step numbering is re-started here, rather than properly continuing.
index.bs
Outdated
|
||
// Attestation configuration: | ||
required BufferSource challenge; | ||
required sequence<ScopedCredentialParameters> parameters; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. the challenge is not a part of attestation per-se -- it provides per-protocol-run roundtrip integrity whether the protocol run is a registration operation or an authn operation.
ScopedCredentialParameters are params for the creation of the underlying credential key pair, which then is attested. see #404
index.bs
Outdated
@@ -889,6 +792,102 @@ user consent to a specific transaction. The structure of these signatures is def | |||
[[#op-get-assertion]]. | |||
</div> | |||
|
|||
## Parameters for Credential Creation (dictionary <dfn dictionary>MakeCredentialOptions</dfn>) ## {#dictionary-makecredentialoptions} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nominally, I think I'm ok with this overall restructuring of the makeCredential parameters/options, tho I have detail-level comments below.
index.bs
Outdated
|
||
// Optional constraints: | ||
unsigned long timeout; | ||
sequence<ScopedCredentialDescriptor> exclude; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep the name of this as excludeList
. I note that the other PRs do not alter allowList
's name.
|
||
Its value's {{ScopedCredentialEntity/id}} member is required, and contains an identifier for the account, specified by | ||
the [=relying party=]. This is not meant to be displayed to the user, but is used by the relying party to control the | ||
number of credentials - an authenticator will never contain more than one credential for a given relying party under the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aside: see #403
index.bs
Outdated
:: This member specifies a time, in milliseconds, that the caller is willing to wait for the call to complete. This is | ||
treated as a hint, and may be overridden by the platform. | ||
|
||
: <dfn>exclude</dfn> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/exclude/excludeList/
index.bs
Outdated
same {{ScopedCredentialEntity/id}}. | ||
|
||
: <dfn>challenge</dfn> | ||
:: This member contains a channelge intended to be used for generating the newly created credential's [=attestation object=]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/channelge/challenge/ :)
need to expand the explanation (but not necessarily now) -- challenge is used in both regstn and authn protocol runs and has security implications. have submitted #404
index.bs
Outdated
|
||
<xmp class="idl"> | ||
dictionary ScopedCredentialEntity { | ||
DOMString id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to @leshi's question "In RelyingPartyUserInfo
some of these were required, why the change?"
LGTM, thx @mikewest ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @mikewest!
Passing a single dictionary parameter into
makrCredential()
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, as each parameter will be labeled.
This patch restructures the data passed into
makeCredential()
substantially, moving from four parameters to a single dictionary,
and merging some existing types into a simpler structure. Most of
it is straightforward; the only bit I know will be controversial is
dropping
RelyingPartyUserInfo
in favor of two instances of asimpler
ScopedCredentialEntity
object: one for the RP, one for theuser.
Let's chat about how (un)reasonable this approach might be.