Skip to content
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

Account information merge #413

Closed
leshi opened this issue Apr 20, 2017 · 12 comments
Closed

Account information merge #413

leshi opened this issue Apr 20, 2017 · 12 comments

Comments

@leshi
Copy link
Contributor

leshi commented Apr 20, 2017

After the merge, the account information changed more than I initially realized.

We lost the "User displayName" and gained an "RP id". Let make sure this is the right thing to do.

@AngeloKai
Copy link
Contributor

We lost an RP id. RP id has always been there in options. The merge didn't change the fact that RP id is optional either.

@leshi
Copy link
Contributor Author

leshi commented Apr 20, 2017

Thanks, Angelo.

We had:

dictionary RelyingPartyAccount {
    required DOMString rpDisplayName;
    required DOMString displayName;
    DOMString id; // account identifier, not rp identifier
    DOMString name;
    DOMString imageURL;

So an RP name. And user display name, name, id, and icon URL.

We have:

dictionary ScopedCredentialEntity {
    DOMString id;
    DOMString name;
    USVString icon;
};

So an RP id, name, icon. And a User id, name, icon.

So just by looking at that, the delta is that we gained an RP id & icon and lost a user display name.

I think you're pointing out that rpID was removed from options. You are right, thanks for pointing that out.

So the correct statement is: we gained an RP icon and lost a user display name.

@equalsJeffH
Copy link
Contributor

to fix the lost user display name, perhaps we ought to define this:

dictionary ScopedCredentialUserEntity : ScopedCredentialEntity {
    DOMString displayName;
};

..and then alter MakeCredentialOptions..

dictionary MakeCredentialOptions {
    required ScopedCredentialEntity      rp;
    required ScopedCredentialUserEntity  user;
    ...

@mikewest
Copy link
Member

Hrm. I missed the display name. Thanks for catching it!

That said, on the one hand, it's easy to add a display name back, on the other, it's not clear to me how many names we need for a user. :) How do y'all expect this data to be used (by the authenticator, I assume)? Is the distinction between "name" and "display name" relevant for the UI that the authenticator will produce?

@equalsJeffH
Copy link
Contributor

I suspect our original intention for Account.name (see WD-04: https://www.w3.org/TR/webauthn/#iface-account) was for it to effectively be used for the unique-at-the-RP account identifier (i.e., in essentially the sense of id in the SCIM schema: https://tools.ietf.org/html/rfc7643#section-3.1). Perhaps name should be named differently.

displayName, as spec'd in WD-04, would AFAIK be used in events such as showing the user a pick list of accounts having creds registered with an RP the user is interacting with.

Yes?

@selfissued
Copy link
Contributor

rp.name is the friendly name "e.g. "Acme Corporation", "Widgets, Inc.", or "Awesome Site"".

user.name is the friendly name, currently "e.g., "john.p.smith@example.com", or "John P. Smith"", - which I'm proposing to change to "e.g. "John P. Smith"". It is consistent to have both "name" entries be the friendly name.

That's why I introduced detailedName "e.g., "john.p.smith@example.com"" - to carry the missing unique-at-the-RP account identifier.

Do you have a specific counter-proposal? If so, what new names would rp.name (friendly name), user.name (friendly name), and user.detailedName (detailed name) have?

@mikewest
Copy link
Member

That's why I introduced detailedName "e.g., "john.p.smith@example.com"" - to carry the missing unique-at-the-RP account identifier.

Is that not user.id?

I guess user.id could be your internal guid (say, I'm user 375632531), user.name could be a username (mkwst@google.com), and detailedName could be a friendly name (Mike West). Does the authenticator need all these to render something useful for its UI? If we need all these things, then sure, let's support them. I'm just unclear on the need. :)

@equalsJeffH
Copy link
Contributor

sigh -- i hit the [comment] button on my prior comment before I should have. apologies.

we had an id member of the account dictionary back in WD-04 (https://www.w3.org/TR/webauthn/#iface-account), and we presently have user.id, as @mikewest notes.

From the description of account.id in WD-04, it seems we nominally intended it to serve the semantic purpose of id as described in the SCIM schema, and thus the same should be true for the present ScopedCredentialEntity::id, tho the explanation in the spec could use some bolstering IMV (see also #403 "some RPs may wish to allow multiple registrations to same user account").

WRT the "name" thing, given that we do have the id member, then the user.names do not need to necessarily be unique (depending on how an RP may wish to utilize this API).

I suppose I suggest returning to using "displayName", or perhaps "friendlyName", for ScopedCredentialEntity::name.

@mikewest's question wrt needing to also have "detailedName" per (PR #423) is reasonable. I do not know that we wrote down our discussions that lead to having both Account.name and Account.displayName in earlier spec versions eg WD-04.

@leshi
Copy link
Contributor Author

leshi commented Apr 24, 2017

@mikewest is correct, the initial idea was that we have:

displayName: Alexei Czeskis
userName: alexei.czeskis@example.com
userId: 123456778

displayName: Alexei Czeskis
userName: someting.else@example.com
userId: 456491456

The first two fields would be used in the account chooser, the last field would be sent to the server. The reason being that displayName and userName could both in theory be malleable, while the userId would not.

Then it all got refactored and 'user' got removed.

@selfissued
Copy link
Contributor

To make Account Chooser work (which I think we can agree that we all want to be possible), it's important that we have clearly defined fields that contain these data values:
Display Name: John Smith
Account Identifier: John.Smith@example.com or +14255551212

We also also probably want the userID 123456778. PR #423 proposes one assignment of names to hold these values:
user.name: John Smith
user.detailedName: John.Smith@example.com
id: 123456778

Account Chooser won't work if we stay with what we have checked in right now, so I believe we have to make a choice. The other reasonable solution (originally proposed by @equalsJeffH ) is this assignment:
user.name: John.Smith@example.com
user.displayName: John Smith
id: 12345678

Could people please express a preference for one or the other, or proposal another actionable alternative for us to consider? Thanks.

@selfissued
Copy link
Contributor

I just updated PR #423 to add a displayName, rather than detailedName, per Jeff's original suggestion, and per his most recent comment. I also updated the examples to show that "name" can be either "John.P.Smith@example.com" or "+14255551234", reflecting the possibility of phone number accounts, and to use "John P. Smith" as the displayName example. This will now enable Account Chooser experiences to work. Any further comments, or should we merge this one in?

@equalsJeffH
Copy link
Contributor

fixed by PR #423

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants