Clarify, simplify and align parameter descriptions #1621

emlun · 2021-06-13T21:36:51Z

Motivated by #1618, though I'm not sure this will fully resolve all the concerns there.

These parameter descriptions are the primary API documentation for Relying Parties, so this rewrite attempts to simplify the descriptions and clearer explain how to use them from an RP perspective. Also removes some unnecessary words and aligns some similar descriptions into more recognizable patterns.

Preview | Diff

We already do this for UserVerificationRequirement values even though both types are "non-exhaustive" enumerations for DOMString attributes.

These parameter descriptions are the primary API documentation for Relying Parties, so this rewrites attempts to simplify the descriptions and clearer explain how to use them from an RP perspective. Also removes some unnecessary words and aligns some similar descriptions into more recognizable patterns.

Firstyear · 2021-06-15T23:53:06Z

Looks good :)

index.bs

equalsJeffH

Hi @emlun, thanks for taking on this gnarly complicated polishing! Overall it looks good, although I have several detail-level comments & suggestions.

An overall comment/suggestion is that we ought to define/circumscribe the difference between a "ceremony" and an "operation" and then use those terms consistently. Here's a back-of-the-envelope attempt:

A "ceremony" is the overall entire flow includes the user's actions, the UI the user interacts with, the RP's particular webauthn deployment approach, and the behavior of the webauthn-defined .create() / .get() operations (plus the "stack" underneath those operations (eg CTAP2 or a platform API+implementation)).

An "operation" is either a "registration operation" or "authentication operation" which are discrete invocations of .create() or .get(), including the marshalling of arguments and the subsequent evaluation of results.

Then in the below we may wish to use "operation" in some places where you're presently suggesting "ceremony".

Another item is that in various places we are using the term "user" where we actually mean "user account", and that creds are mapped to user accounts by RPs, via user handles. We ought to define "user account" in the terminology section.

index.bs

Co-authored-by: =JeffH <jdhodges@google.com>

equalsJeffH

I do not understand why you marked various comments below as "resolved" without incorporating the suggestion or offering rationale as to why not to incorporate the suggestion.

I'm away for an extended period and unable to split hairs over this, so do what you think is best.

emlun · 2021-07-21T04:14:30Z

I do not understand why you marked various comments below as "resolved" without incorporating the suggestion or offering rationale as to why not to incorporate the suggestion.

Sorry, my mistake. I'd resolved them locally but hadn't pushed the changes, I think I was waiting to see what happens in #1649 and base the continuation here on that. I'll go back and un-resolve them for now for proper bookkeeping.

Co-authored-by: =JeffH <jdhodges@google.com>

equalsJeffH · 2021-07-21T07:17:59Z

Sorry, my mistake. I'd resolved them locally but hadn't pushed the changes, I think I was waiting to see what happens in #1649 and base the continuation here on that.

Ah, ok, thanks, that makes sense.

equalsJeffH

LGTM, thx @emlun !

Define "user account"

equalsJeffH · 2021-08-12T09:30:13Z

is this waiting on @sbweeden ?

emlun · 2021-08-16T17:08:28Z

No, it's waiting on me. I'll update it within the next few days.

Co-authored-by: =JeffH <jdhodges@google.com>

equalsJeffH

LGTM, thx @emlun!

sbweeden

A few minor comments to address/discuss, but overall looks really good. Thanks for the massive effort.

index.bs

sbweeden · 2021-09-16T03:21:18Z

index.bs

+        The value SHOULD be a member of {{AuthenticatorAttachment}} but [=client platforms=] MUST ignore unknown values,
+        treating an unknown value as if the [=map/exist|member does not exist=].
+
+        The [=[RP]=] can determine the resulting [=authenticator attachment modality|attachment=] for the created credential


I don't know that this is entirely true. My understanding is that the use of getTransports() will return all attachments supported by the authenticator that generated the credential - not just the particular transport used in this ceremony. There is a discussion in #1668 about potentially including a transport field in PublicKeyCredential for the purpose of determining the transport that was actually used.

Maybe the change here is just to reword to the plural attachments, as at the moment it reads as if it returns the singular transport that was used for the ceremony.

Also a good point. I don't think we really clarify how one can determine the authenticator attachment(s) from the transports list, so it might be appropriate to introduce a small section on that. We already have §5.2.1.1. Easily accessing credential data as a subsection of the AuthenticatorAttestationResponse section, so another sibling subsection there seems reasonable. I think I'll do that as a meta-pull request into this one.

Addressed in meta-PR #1670.

It is good that #1670 adds an explanation of how getTransports() may be interpreted by the RP. That said, I don't think it addresses the original comment here, at least not always. If getTransports() returned a list, with one of those values being internal, and another being a different transport (such as the mobile phone as an authenticator use case), the RP cannot determine the resulting authenticator attachment modality for the created credential. This however could be determined with the proposal in #1668, which I think better addresses the use case of an RP trying to determine if a platform-bound credential has been created as part of this ceremony.

I am in favour of keeping the changes in #1670 to aid in the understanding of RPs interpretation and use of getTransports(), but I don't think that this section should attempt to claim that the getTransports() API is alone enough to determine the resulting authenticator attachment modality.

Ah right, looks like I missed that nuance in your first comment. getTransports() (if non-empty) is enough to determine the possible attachment options for the credential, but indeed it is not enough to determine the attachment used in this particular ceremony. So I agree that needs to be clearer. But perhaps an even better solution is to extend the proposal in issue #1666 and PR #1668 to apply to registration too?

To land this PR, we could just remove this section completely:

The [=[RP]=] can determine the resulting [=authenticator attachment modality|attachment=] for the created credential using the {{AuthenticatorAttestationResponse/getTransports()}} method of the resulting {{AuthenticatorAttestationResponse}}. See [[#enum-transport]] for details.

and let #1668 deal with the issue of deterministically allowing an RP to understand the attachment modality of an authenticator for a particular ceremony.

Ok, yeah, that seems like a fair middle ground. We can keep #1670 open for tracking that we should add this back in once we have #1668 sorted.

I'm thinking we need to resolve and land PRs #1668 and #1670 before landing this PR, and also before landing this PR we will need to revise this paragraph based on the final form of PRs #1668 and #1670.

Yeah, that works too I guess. There's not really much reason to hurry to get this one merged.

@sbweeden: Is this thread is now resolved given @emlun's having "...updated this to align with the new authenticatorAttachment property" ?

index.bs

@sbweeden

Address review comments by @sbweeden

equalsJeffH · 2021-09-22T19:27:20Z

on 2021-09-22 call:
@sbweeden asks about whether self-attstn is returned when the attstnConvyPref is "none" ...?
(because chrome behavior replaces the self-attstn with none?)
@emlun points out that close spec reading is that it is not required to replace self-attstn
@agl: the behavior @sbweeden is seeing is a "special case on MacOS" because (mumble). offhand present spec lang seems correct.
@sbweeden is thinking the behavior he is seeing is a chrome bug.
@emlun: will remove the offending section in this PR and land it (modulo further reviews by others)

equalsJeffH · 2021-09-24T17:46:47Z

I could be wrong, but it seems this PR has not recently had a merge-from-main. Catching it up with the changes in the main branch would be helpful for reviewing it. thanks.

equalsJeffH

I re-reviewed. Thanks again for taking on this large polishing task, it looks good. I found only a few nits for your consideration.

We ought to wait for PRs #1668 and #1670 to resolve and land before finishing and landing this, it seems.

index.bs

equalsJeffH · 2021-09-27T21:52:24Z

index.bs

+        The value SHOULD be a member of {{AuthenticatorAttachment}} but [=client platforms=] MUST ignore unknown values,
+        treating an unknown value as if the [=map/exist|member does not exist=].
+
+        The [=[RP]=] can determine the resulting [=authenticator attachment modality|attachment=] for the created credential


I'm thinking we need to resolve and land PRs #1668 and #1670 before landing this PR, and also before landing this PR we will need to revise this paragraph based on the final form of PRs #1668 and #1670.

index.bs

…er a successful ceremony

- Use terms "create()/get() operation" more instead of ceremony - Align "extensions" description between create() and get() options - Reference #sctn-credential-id-privacy-leak in allowCredentials description - Tweak wordings a bit

emlun · 2021-10-14T14:50:04Z

Alright, with #1670 done I've updated this to align with the new authenticatorAttachment property, and with that I think this is close to done. I did find a few more opportunities for further polish, which I've added as meta-PR #1674 for easier review of those changes in isolation.

equalsJeffH

LGTM @emlun, thanks again for this non-trivial polishing. ( I've one minor comment below )

index.bs

Additional polish for PR #1621

sbweeden

Thanks Emlun - this looks good.

@YubicoDemo

SHA: bc87207 Reason: push, by @YubicoDemo Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

emlun added 6 commits June 1, 2021 03:22

Fix lowercase start of a sentence

c3be90f

Link AttestationConveyancePreference values in switch cases

91dde16

We already do this for UserVerificationRequirement values even though both types are "non-exhaustive" enumerations for DOMString attributes.

Explicitly point out that same user handle overwrites existing cred

6ad9a0c

Change a "behaviour" to "behavior" to align with majority in doc

a1b6df5

Introduce terms "contains" and "created on" as synonyms of "bound to"

339cbee

emlun added the type:editorial label Jun 13, 2021

emlun added this to the L3-WD-01 milestone Jun 13, 2021

emlun requested a review from equalsJeffH June 13, 2021 21:36

emlun self-assigned this Jun 13, 2021

nadalin requested review from akshayku and sbweeden June 16, 2021 19:11

sbweeden requested changes Jun 22, 2021

View reviewed changes

emlun added 2 commits June 22, 2021 21:50

Link to "User Handle Contents" section from User Handle definition

39b7cc3

Address review comments

03becb6

equalsJeffH previously requested changes Jul 7, 2021

View reviewed changes

emlun force-pushed the issue-1618-clarify-params branch from 5d354ef to 03becb6 Compare July 14, 2021 02:01

Define "user account"

8056f09

Co-authored-by: =JeffH <jdhodges@google.com>

emlun mentioned this pull request Jul 14, 2021

Define "user account" #1649

Merged

equalsJeffH reviewed Jul 20, 2021

View reviewed changes

Address review comments

9d7bc35

Co-authored-by: =JeffH <jdhodges@google.com>

equalsJeffH approved these changes Jul 21, 2021

View reviewed changes

Merge pull request #1649 from w3c/issue-1618-define-user-account

d55d733

Define "user account"

Apply suggestions from code review

07b26bc

Co-authored-by: =JeffH <jdhodges@google.com>

equalsJeffH approved these changes Aug 19, 2021

View reviewed changes

sbweeden requested changes Sep 16, 2021

View reviewed changes

Address review comments by @sbweeden

756f964

This was referenced Sep 17, 2021

Address review comments by @sbweeden #1669

Merged

Describe how to determine authenticator attachment from transports #1670

Closed

Merge pull request #1669 from w3c/issue-1618-review-sbweeden

54e1a92

Address review comments by @sbweeden

emlun mentioned this pull request Sep 20, 2021

Add authenticator attachment used during authentication to assertion payload #1668

Merged

equalsJeffH self-requested a review September 22, 2021 19:20

equalsJeffH reviewed Sep 27, 2021

View reviewed changes

emlun added 6 commits October 1, 2021 12:31

Merge branch 'main' into issue-1618-clarify-params

5ba3556

Address review comments by @equalsJeffH

f175479

Address one more review comment by @equalsJeffH

2f25a98

Merge branch 'main' into issue-1618-clarify-params

9406857

Rewrite authenticatorAttachment note about discovering attachment aft…

05d05fc

…er a successful ceremony

Polish PR #1621

9a508da

- Use terms "create()/get() operation" more instead of ceremony - Align "extensions" description between create() and get() options - Reference #sctn-credential-id-privacy-leak in allowCredentials description - Tweak wordings a bit

emlun mentioned this pull request Oct 14, 2021

Additional polish for PR #1621 #1674

Merged

equalsJeffH approved these changes Nov 3, 2021

View reviewed changes

index.bs Outdated Show resolved Hide resolved

emlun added 4 commits November 4, 2021 17:10

Reference #sctn-extensions in extensions parameter descriptions

d9d608e

Apply part of commit f175479 to authentication ceremony definition too

3d852f2

Address review comments by @equalsJeffH

42db62a

Merge pull request #1674 from w3c/pr-1621-polish

1651515

Additional polish for PR #1621

sbweeden approved these changes Nov 26, 2021

View reviewed changes

akshayku approved these changes Dec 15, 2021

View reviewed changes

YubicoDemo merged commit bc87207 into main Dec 15, 2021

equalsJeffH deleted the issue-1618-clarify-params branch December 15, 2021 20:09

github-actions bot added a commit that referenced this pull request Dec 15, 2021

Merge pull request #1621 from w3c/issue-1618-clarify-params

34e90dc

SHA: bc87207 Reason: push, by @YubicoDemo Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

emlun mentioned this pull request Mar 23, 2022

Why not make things simple? #1709

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify, simplify and align parameter descriptions #1621

Clarify, simplify and align parameter descriptions #1621

emlun commented Jun 13, 2021 •

edited by pr-preview bot

Loading

Firstyear commented Jun 15, 2021

equalsJeffH left a comment

equalsJeffH left a comment

emlun commented Jul 21, 2021

equalsJeffH commented Jul 21, 2021

equalsJeffH left a comment

equalsJeffH commented Aug 12, 2021

emlun commented Aug 16, 2021

equalsJeffH left a comment

sbweeden left a comment

sbweeden Sep 16, 2021

emlun Sep 17, 2021

emlun Sep 17, 2021

sbweeden Sep 20, 2021

emlun Sep 20, 2021

sbweeden Sep 21, 2021

emlun Sep 21, 2021

equalsJeffH Sep 27, 2021

emlun Oct 1, 2021

equalsJeffH Nov 3, 2021 •

edited

Loading

equalsJeffH commented Sep 22, 2021

equalsJeffH commented Sep 24, 2021

equalsJeffH left a comment

equalsJeffH Sep 27, 2021

emlun commented Oct 14, 2021

equalsJeffH left a comment

sbweeden left a comment

Clarify, simplify and align parameter descriptions #1621

Clarify, simplify and align parameter descriptions #1621

Conversation

emlun commented Jun 13, 2021 • edited by pr-preview bot Loading

Firstyear commented Jun 15, 2021

equalsJeffH left a comment

Choose a reason for hiding this comment

equalsJeffH left a comment

Choose a reason for hiding this comment

emlun commented Jul 21, 2021

equalsJeffH commented Jul 21, 2021

equalsJeffH left a comment

Choose a reason for hiding this comment

equalsJeffH commented Aug 12, 2021

emlun commented Aug 16, 2021

equalsJeffH left a comment

Choose a reason for hiding this comment

sbweeden 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

equalsJeffH Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

equalsJeffH commented Sep 22, 2021

equalsJeffH commented Sep 24, 2021

equalsJeffH left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlun commented Oct 14, 2021

equalsJeffH left a comment

Choose a reason for hiding this comment

sbweeden left a comment

Choose a reason for hiding this comment

emlun commented Jun 13, 2021 •

edited by pr-preview bot

Loading

equalsJeffH Nov 3, 2021 •

edited

Loading