Fix #622: Clarify PublicKeyCredentialEntity name descriptions #666

emlun · 2017-10-31T10:48:13Z

This resolves #622. This also changes some display name examples to
include non-ASCII characters.

This resolves w3c#622. This also changes some display name examples to include non-ASCII characters.

jovasco · 2017-10-31T12:03:31Z

Why SHOULD for the name uniqueness? Isn't that supposed to be the login name?

emlun · 2017-10-31T12:21:30Z

Do you mean "why SHOULD and not MUST"? If so: yes, it'll probably be the login name in most cases, but there's no reason to require (nor a way to enforce) that the RP uses it for that. As I understand it the intended use for it is primarily as a recognizable identifier to help the user choose a credential when an authenticator contains multiple first factor credentials with the same displayName, but I can't seem to find where I got that from.

emlun · 2017-10-31T12:22:38Z

I mean, the user.name doesn't absolutely need to be unique because we already have user.id which fills that role.
EDIT: Of the user members, user.id is also the only one that is actually returned to the RP from an assertion operation.

jovasco · 2017-10-31T12:43:36Z

In case of list selection, the (user.name, user.displayName) pair needs to be unique because user.id is not displayable. It seems more logical to require user.name to be unique.

Do you see a downside to that requirement?

emlun · 2017-10-31T12:59:14Z

First of all, there's no way to enforce such a requirement - all that would happen by ignoring it would be that one's own users would be confused.

Second, it only really needs to be unique between the accounts owned by that user, and reviewing it now after my previous comments made me realize the uniqueness requirement really doesn't matter since the RP won't see the user.name again anyway. In fact I think it's better to drop the uniqueness recommendation entirely - the recommendation "SHOULD be easily distinguishable to the user from identifiers for other user accounts" still covers the actual need.

jovasco · 2017-10-31T13:46:33Z

Now we are back to two different names which are both only intended for display...

emlun · 2017-10-31T16:15:30Z

Yes, as far as I can tell that is their only function - please correct me if I'm wrong. However we decided not to make any breaking changes to this this close to CR (see #646), so we're stuck with these fields for now.

rlin1 · 2017-11-09T20:03:19Z

Changes look good to me.
One more question: purpose of "id" field is not explained as of now.

rlin1

Purpose of "id" field is not explained as of now.

emlun · 2017-11-09T20:04:46Z

@rlin1 Which "id" field? rp.id or user.id?

rlin1 · 2017-11-09T21:27:46Z

both, similar to the existing description of name, e.g. "When inherited by {{PublicKeyCredentialRpEntity}} it is a human-friendly identifier for the [=[RP]=]"

emlun · 2017-11-09T21:45:05Z

They both link to the definitions of the RP ID and user handle, respectively. Do you want me to duplicate their descriptions?

rlin1 · 2017-11-09T22:05:23Z

Good point. Only user.id:

I mean id in https://www.w3.org/TR/webauthn/#dom-publickeycredentialentity-id
as
id, of type DOMString

A unique identifier for the entity. For a relying party entity, sets the RP ID. For a user account entity, this will be an arbitrary string specified by the relying party.

Where RP ID is clear (IMHO), the id in the case of the user entity needs clarification (i.e. purely RP internal use or so).

emlun · 2017-11-09T22:24:33Z

Sorry, I don't understand what you mean. The user.id description references the definition of the User Handle which states that the user handle is a unique account identifier specified by the RP.

emlun · 2017-11-10T15:31:06Z

@rlin1 Are we ready to merge?

equalsJeffH

this seems overall fine, though could use some modest polishing. HTH.

equalsJeffH · 2017-11-20T18:48:36Z

index.bs

-        name member's value to a length equal to or greater than 64 bytes.
+    ::  A human-readable identifier for the entity. Its function depends on what the {{PublicKeyCredentialEntity}} represents:
+
+          - When inherited by {{PublicKeyCredentialRpEntity}} it is a human-friendly identifier for the [=[RP]=], intended only


s/identifier/name/

equalsJeffH · 2017-11-20T18:55:19Z

index.bs

+          - When inherited by {{PublicKeyCredentialRpEntity}} it is a human-friendly identifier for the [=[RP]=], intended only
+            for display. For example, "ACME Corporation", "Wonderful Widgets, Inc." or "Awesome Site".
+          - When inherited by {{PublicKeyCredentialUserEntity}}, it is a human-readable identifier for a user account. It is
+            intended only for display, and SHOULD allow the user to easily tell the difference between user accounts with similar


well, when inherited by {{PublicKeyCredentialUserEntity}}, an RP MAY use the name member's value to store an account identifier (aka a username), yes?

I'm thinking we should add a Note: wrt an RP's options when employing PublicKeyCredentialUserEntity.id and PublicKeyCredentialUserEntity.name.

I'm not sure what you mean by your first paragraph. The member is required and won't be passed back to the RP, only (optionally) stored by the authenticator. The RP MAY use name for a username, yes, MUST use it, and SHOULD NOT use it for anything too functionally different from a conventional username.

@equalsJeffH Bump

see comments below...

rlin1 · 2017-12-06T18:22:37Z

Looks good to me.

equalsJeffH

This overall looks good, thanks @emlun !
I do have some detail-level suggestions to discuss below, though. HTH.

equalsJeffH · 2017-12-06T20:30:51Z

index.bs

+            for display. For example, "ACME Corporation", "Wonderful Widgets, Inc." or "Awesome Site".
+          - When inherited by {{PublicKeyCredentialUserEntity}}, it is a human-readable identifier for a user account. It is
+            intended only for display, and SHOULD allow the user to easily tell the difference between user accounts with similar
+            {{PublicKeyCredentialUserEntity/displayName}}s. For example, "alexm", [email protected]" or "+14255551234". The


s/, alex.p.mueller/, "alex.p.mueller/

Good catch, thanks!

equalsJeffH · 2017-12-06T20:50:09Z

index.bs

-        user's name. This identifier is intended for display. [=Authenticators=] MUST accept and store a 64 byte minimum length
-        for a name members's value. Authenticators MAY truncate a
-        name member's value to a length equal to or greater than 64 bytes.
+    ::  A human-readable name for the entity. Its function depends on what the {{PublicKeyCredentialEntity}} represents:


I would s/-readable/-palatable/. And add this note:
"Note: An identifier that is human-palatable is intended to be rememberable and reproducible by typical human users, in contrast to identifiers that are, for example, randomly generated sequences of bits."

c.f. https://www.internet2.edu/media/medialibrary/2013/09/04/internet2-mace-dir-eduperson-200604.html

Sounds good. I'll change to "palatable" in the ...UserEntity bullet point, but leave the top one as "readable" since that applies to both the User and Rp variants and Rp.name doesn't necessarily need to be reproducible. Does that sound good?

What does copyright legalese say about flat-out copying that definition from the EduPerson spec? Should we reference the EduPerson spec for it?

As per the 2017-12-13 WG call, this is fair use and attribution is good practice.

equalsJeffH · 2017-12-06T20:58:10Z

index.bs

+          - When inherited by {{PublicKeyCredentialUserEntity}}, it is a human-readable identifier for a user account. It is
+            intended only for display, and SHOULD allow the user to easily tell the difference between user accounts with similar
+            {{PublicKeyCredentialUserEntity/displayName}}s. For example, "alexm", [email protected]" or "+14255551234". The
+            [=[RP]=] SHOULD let the user choose this, and MAY restrict the choice as needed or appropriate.


I suggest: s/ [=[RP]=] SHOULD /=[RP]=] MAY / because this is really up to the RP's deployment context.

Also, after "appropriate.", I would add: "For example, an [=[RP]=] may choose to map human-palatable [=username=] account identifiers to {{PublicKeyCredentialUserEntity}}'s {{PublicKeyCredentialUserEntity/name}}."

..where [=username=] perhaps should link to https://html.spec.whatwg.org/#attr-fe-autocomplete-username (AFAICT, it seems to be the most appropriate place to link "username" to in the greater Web specs space, for our purposes here).

Apparently, [=username=] already links to https://url.spec.whatwg.org/#concept-url-username . Should we change that?

Should we change that?

yeah, none of the autolinks for "username" amongst whatwg/w3c specs seemed very satisfying to me, so i was thinking that the link to https://html.spec.whatwg.org/#attr-fe-autocomplete-username would be the most clear for readers in terms of implying that we mean username in the sense of "unique-for-this-rp user account name". Though, I suppose we could just define it as such ourselves.

equalsJeffH · 2017-12-06T21:04:55Z

index.bs

+          - When inherited by {{PublicKeyCredentialRpEntity}} it is a human-friendly identifier for the [=[RP]=], intended only
+            for display. For example, "ACME Corporation", "Wonderful Widgets, Inc." or "Awesome Site".
+          - When inherited by {{PublicKeyCredentialUserEntity}}, it is a human-readable identifier for a user account. It is
+            intended only for display, and SHOULD allow the user to easily tell the difference between user accounts with similar


see comments below...

emlun · 2017-12-20T15:38:35Z

@equalsJeffH Changes made, please re-review!

equalsJeffH · 2017-12-29T15:18:43Z

LGTM, thx @emlun !

akshayku · 2017-12-29T22:30:59Z

index.bs

-
-        Its value's {{PublicKeyCredentialUserEntity/id}} member contains the [=user handle=] for the account, specified by the
-        [=[RP]=].
+        Its value's {{PublicKeyCredentialEntity/name}}, {{PublicKeyCredentialUserEntity/displayName}} and


user's "name" and "displayName" are not required according to CTAP spec. Only user's "id" is required.
For very security conscious RPs, who don't want to store any user identifiable information on the FIDO device, they will only set "id" (which is supposed to be random bytes). There is no UI requirement for single account per RP case.

On FIDO device, RP's "id" and users "id" makes a unique combination to identify a credentials. Rest of the fields are helper details which are optional.

For an authenticator with a display (or, for example, an authenticator app on a phone), needs this information to show a useful message. "Do you want to register at example.com with Alex P. Mueller?" is a lot more informative (and, I would argue, security conscious) than "Do you want to register?".

Storing this information on the authenticator is more privacy than security sensitive, especially considering that the device is already entrusted with the keys. Perhaps I am biased but I assume authenticator vendors are security and privacy conscious enough to encrypt this additional information. If not, don't buy their products.

For getAssertion, I am fine with making this optional. If the device needs the information, it can store it.

akshayku · 2017-12-29T22:34:55Z

index.bs

@@ -1343,8 +1350,7 @@ optionally evidence of [=user consent=] to a specific transaction.
    :   <dfn>rp</dfn>
    ::  This member contains data about the [=[RP]=] responsible for the request.

-        Its value's {{PublicKeyCredentialEntity/name}} member contains the friendly name of the [=[RP]=] (e.g. "Acme Corporation",
-        "Widgets, Inc.", or "Awesome Site".
+        Its value's {{PublicKeyCredentialEntity/name}} member is required.


RP's "name" is optional according to CTAP spec. See below comment.

emlun · 2017-12-29T22:49:48Z

@akshayku So should we make rp.name user.name and user.displayName optional in WebAuthn as well?

Technically, the empty string is a valid value for each of them... but that would of course be a rather ugly workaround.

(Replying outside review thread for increased visibility)

akshayku · 2017-12-29T22:53:29Z

Yes, they should be optional. rp.icon and user.icon are also optional.

emlun · 2017-12-29T23:00:36Z

Would that cause problems for rendering UX for creating/picking credentials? I think especially user.name (classic user account name) is needed for first factor credentials to be properly usable, but making it required only in some cases would be a bit awkward.

akshayku · 2017-12-29T23:11:26Z

There is no UX rendering problem as UX anyway has to deal with similar pattern via CTAP spec and It is RP's decision what user/RP information is wants to store on the FIDO device.

If RP supports multiple accounts, it will set the "name" for distinguishing purpose.
If RP's does not mind/care storing "name" on the FIDO device (even for single account), it will set the "name".
If RP's does not want to store any user identifiable information on the device, he can choose not to set any information.

emlun · 2017-12-29T23:35:50Z

If RP supports multiple accounts, it will set the "name" for distinguishing purpose.

Just note that the RP always supports multiple accounts, because it doesn't know what device the credential will end up on and therefore cannot know whether that device contains (or will contain) a credential for another account.

Other than that, I agree. I'll make the requested changes.

akshayku · 2017-12-29T23:56:49Z

By "multiple accounts", I meant "multiple accounts per RP on a FIDO device"

RP does not have to always supports multiple accounts per RP case (its his choice). It can however choose to support multiple credential per account on its servers (again it is RP's choice and its the normal case).

Even if RP does not know what device the credential will end up on, if it supports one account, the credential will be overwritten if the FIDO device has already been provisioned OR will be created if not already provisioned. In both the cases, no user information is required to be passed by the RP.

emlun · 2017-12-30T00:49:18Z

What I mean is the RP doesn't know if the user is using the same authenticator for multiple separate accounts for that RP - for example, if a user has one work account and one private account, or if two people share an authenticator. U2F devices don't support this use case, but WebAuthn does.

akshayku · 2017-12-30T01:02:01Z

In normal scenarios, I agree that RP should be setting the name but that is its choice.

User has one work account and one private account or two people share an authenticator: These are multiple accounts per RP case and yes, if RP supports it, it should set the name.

For RP who don't want to support such scenario, like may be it is an bank/enterprise where they distribute the keys and may want to support only one account per RP, it may not want to set user information due to privacy reasons.

emlun · 2017-12-30T01:26:43Z

There's still no way in the current spec for the bank to actually enforce that, though, only to make it inconvenient by not setting the account name. I might very well be bikeshedding here, since the RP can always just set an empty or bogus value, but I feel more inclined to take the user's side in this. I've changed my mind back to thinking user.name should probably be left required, but I agree with making rp.name and user.displayName optional.

akshayku · 2017-12-30T01:35:44Z

We are extrapolating what RP's can and will do. Empty/Bogus value don't provide any value. Same can be said about displayName and making this required just make it confusing.

Best is to stay consistent with CTAP spec. I don't see any reason departing from CTAP spec and I don't agree with making user.name as a required parameter.

emlun · 2017-12-30T08:28:29Z

All right. Does @equalsJeffH object to making rp.name, user.name and user.displayName optional?

equalsJeffH · 2018-01-02T00:15:19Z

we ought not rush into making this breaking change. i will write longer thoughts on it tomorrow.

akshayku · 2018-01-02T16:30:30Z

Ah.. If it is breaking, lets not change them.

equalsJeffH · 2018-01-02T21:13:37Z

It seems to me that @akshayku is raising a new issue (in #666 (comment) and #666 (review)) that is not per se related to the thrust of this PR. I suggest that we merge this PR as-is and @akshayku submit a new issue (if still desired).

Note that we recently went through the exercise of aligning the webauthn IDL and the #createCredential/#getAssertion algorithms' language: PR #669, Issues #538, #537.

The so-called "enforced single account per RP use case" (i.e., what @akshayku is calling "single account per RP case") seems to me a special case. IIUC, an RP wishes to not reveal values for rp.name, user.name and user.displayName, it can simply set them to empty strings, i.e.: "". In any case, this seems like yet another "deployment considerations" item, i.e., explaining how RPs might implement particular deployment scenarios such as the "enforced single account per RP use case".

Clarify PublicKeyCredentialEntity name descriptions

8cdeac8

This resolves w3c#622. This also changes some display name examples to include non-ASCII characters.

emlun added the type:editorial label Oct 31, 2017

emlun added this to the CR milestone Oct 31, 2017

emlun self-assigned this Oct 31, 2017

Drop user.name uniqueness recommendation

be5143f

Merge branch 'master' into issue-622

343b89d

rlin1 self-requested a review November 9, 2017 20:03

rlin1 reviewed Nov 9, 2017

View reviewed changes

equalsJeffH suggested changes Nov 20, 2017

View reviewed changes

Partially address review comments

819b77a

rlin1 approved these changes Dec 6, 2017

View reviewed changes

equalsJeffH suggested changes Dec 6, 2017

View reviewed changes

emlun added 3 commits December 7, 2017 15:55

Change link target for [=username=]

1f8e10c

Address review comments

7508d60

Extract formal definition of Human Palatability

64b728e

Reference EduPerson for definition of Human Palatability

1904e73

equalsJeffH approved these changes Dec 29, 2017

View reviewed changes

akshayku reviewed Dec 29, 2017

View reviewed changes

Merge branch 'master' into issue-622

432e531

emlun merged commit 35b730b into w3c:master Jan 3, 2018

emlun deleted the issue-622 branch January 3, 2018 19:11

WebAuthnBot pushed a commit that referenced this pull request Jan 3, 2018

Built by Travis-CI: 35b730b Merge pull request #666 from emlun/issue-622

7346cac

emlun mentioned this pull request Jan 24, 2018

Aligning PublicKeyCredentialUserEntity with CTAP #757

Closed

Fix #622: Clarify PublicKeyCredentialEntity name descriptions #666

Fix #622: Clarify PublicKeyCredentialEntity name descriptions #666

Conversation

emlun commented Oct 31, 2017 • edited by pr-preview bot Loading

jovasco commented Oct 31, 2017

emlun commented Oct 31, 2017

emlun commented Oct 31, 2017 • edited Loading

jovasco commented Oct 31, 2017

emlun commented Oct 31, 2017

jovasco commented Oct 31, 2017

emlun commented Oct 31, 2017

rlin1 commented Nov 9, 2017

rlin1 left a comment

Choose a reason for hiding this comment

emlun commented Nov 9, 2017

rlin1 commented Nov 9, 2017

emlun commented Nov 9, 2017

rlin1 commented Nov 9, 2017

emlun commented Nov 9, 2017

emlun commented Nov 10, 2017

equalsJeffH 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

rlin1 commented Dec 6, 2017

equalsJeffH 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlun commented Dec 20, 2017

equalsJeffH commented Dec 29, 2017

akshayku Dec 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlun commented Dec 29, 2017

akshayku commented Dec 29, 2017

emlun commented Dec 29, 2017

akshayku commented Dec 29, 2017

emlun commented Dec 29, 2017

akshayku commented Dec 29, 2017

emlun commented Dec 30, 2017

akshayku commented Dec 30, 2017

emlun commented Dec 30, 2017

akshayku commented Dec 30, 2017

emlun commented Dec 30, 2017

equalsJeffH commented Jan 2, 2018

akshayku commented Jan 2, 2018

equalsJeffH commented Jan 2, 2018

emlun commented Oct 31, 2017 •

edited by pr-preview bot

Loading

emlun commented Oct 31, 2017 •

edited

Loading

akshayku Dec 29, 2017 •

edited

Loading