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

PublicKeyCredentialDescriptor.id and PublicKeyCredentialEntity.id type differ #504

Closed
herrjemand opened this issue Jul 13, 2017 · 15 comments

Comments

@herrjemand
Copy link
Contributor

herrjemand commented Jul 13, 2017

PublicKeyCredentialEntity.id of type DOMString, but PublicKeyCredentialDescriptor.id of type BufferSource. I think it will be confusing for the future developers to work with the same id but in different types

@herrjemand
Copy link
Contributor Author

Maybe we should make them both DOMString?

@equalsJeffH equalsJeffH added this to the CR milestone Jul 13, 2017
@equalsJeffH
Copy link
Contributor

there were reasons we introduced using BufferSource rather than DOMString in various places: #61

though, we've had various further issues in this context:

https://github.com/w3c/webauthn/issues?utf8=%E2%9C%93&q=is%3Aissue%20is%3Aclosed%20buffersource

Am not sure offhand whether we ought to change PublicKeyCredentialEntity.id of type DOMString to BufferSource, but that's something to consider.

@herrjemand
Copy link
Contributor Author

Credential ID (e.g. U2F keyHandle is binary blob)

In U2F keyID is controlled by the authenticator, so it makes sense to have it binary blob.

In WebAuthn however ID is set by RP.

@herrjemand
Copy link
Contributor Author

My argument here is consistency. If we are saying that PKCDescription.id is string, the other id fields should be string as well

@herrjemand
Copy link
Contributor Author

And it makes things far more elegant:

Before

...
user: {
    id: "1098237235409872"
    name: "john.p.smith@example.com",
    displayName: "John P. Smith",
    icon: "https://pics.acme.com/00/p/aBjjjpqPb.png"
  },
...

...
var acceptableCredential1 = {
    type: "public-key",
    id: encoder.encode("1098237235409872")
};
...

After

...
user: {
    id: "1098237235409872"
    name: "john.p.smith@example.com",
    displayName: "John P. Smith",
    icon: "https://pics.acme.com/00/p/aBjjjpqPb.png"
  },
...

...
var acceptableCredential1 = {
    type: "public-key",
    id: "1098237235409872"
};
...

@nadalin nadalin modified the milestones: WD-07, CR Sep 7, 2017
@nadalin
Copy link
Contributor

nadalin commented Sep 7, 2017

@herrjemand this is a breaking change, so we have to decide to do this now as CR is too late

@AngeloKai
Copy link
Contributor

AngeloKai commented Sep 7, 2017

The two fields should be of those types. Looking through the description, my guess is there is a misunderstanding of what the fields do. When the MDN documentation is out, the issues would become clearer.

PublicKeyCredentialEntity.id is the rp.id field. This field is used for web developers to specify which origin the keys should belong to (no larger than eTLD+1).

PublicKeyCredentialUserEntity.id is the user.id field. This field is used for web developers to specify what the user account's id is. More often than not, developers have an established database that contain the user account information, including the unique id assigned to each user. Most databases just use string instead of binary array to specify such id. Using string for this id field also makes things easier for web developers.

PublicKeyCredentialDescriptor.id is used for developers to specify which credential they want to allow/exclude in this transaction. Here the id comes from the credential itself. Developers retrieve this id by decoding the Authenticator Data field and would most likely get a binary blob out anyway. Keeping this field as a BufferSource makes things much easier for them.

Overall, by keeping the two id field as two types of data, we are helping web developers understand the difference between all the various identities involved in the webauthn transaction and preventing them from making mistakes. Your above demo is a mistakened way of using the fields. Our current design helps you find your mistakes sooner.

@AngeloKai
Copy link
Contributor

@nadalin I'd recommend closing out the issue given the above reason. The filer is misinterpreting the spec.

@herrjemand
Copy link
Contributor Author

@AngeloKai Why would developers decode id from assertion when they are already aware of it, as they request registration for this specific id?

@AngeloKai
Copy link
Contributor

@herrjemand The developer will not be aware of the id from assertion before they receive the assertion.

The key to keep in mind here is there are two ids: the account id and the credential id. Multiple credentials can be registered for one account. Multiple account cannot register the same credential. When the developer requests registration, they are required to supply the account id.

In addition, when developer requests registration, they can also optionally provide an excludeList. The excludeList contains a list of credentials (identified by credential id). Developers find out about the list of credentials they want to include by querying the database to find out the credential they don't like from previous registrations.

@herrjemand
Copy link
Contributor Author

@AngeloKai Oh, so PublicKeyCredentialDescriptor.id is not binary version of PublicKeyCredentialUserEntity.id?

@AngeloKai
Copy link
Contributor

@herrjemand Perhaps in-person explanation would help. Maybe you can talk to Adam Power who explain this more in-depth in person?

@AngeloKai
Copy link
Contributor

@herrjemand No, PublicKeyCredentialDescriptor.id is not binary version of PublicKeyCredentialUserEntity.id

@AngeloKai
Copy link
Contributor

@herrjemand @nadalin Because the ask for the change is based on an misinterpretation of the spec, I am closing the issue.

@herrjemand
Copy link
Contributor Author

Oh, cool then we just need to update examples, cause they confused the hell out of me.

Thanks for clarifying that @AngeloKai

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

No branches or pull requests

4 participants