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

Rename PublicKeyCredentialEntity fields #646

Closed
emlun opened this issue Oct 16, 2017 · 3 comments
Closed

Rename PublicKeyCredentialEntity fields #646

emlun opened this issue Oct 16, 2017 · 3 comments

Comments

@emlun
Copy link
Member

emlun commented Oct 16, 2017

This is split out from #622.

The field names in PublicKeyCredentialEntity and its descendants are unnecessarily confusing. In particular, PublicKeyCredentialEntity.name means different things when inherited by PublicKeyCredentialRpEntity vs PublicKeyCredentialUserEntity - in the former case it's a human-friendly display name, in the latter case it's a unique identifier for a user account. I suggest eliminating that ambiguity by renaming the fields:

  • Rename rp.name to rp.displayName
  • Rename user.id to user.handle
  • Rename user.name to user.id

The hierarchy would then look something like this:

dictionary PublicKeyCredentialEntity {
    required DOMString      displayName;
    USVString               icon;
};
dictionary PublicKeyCredentialRpEntity : PublicKeyCredentialEntity {
    required DOMString      id;
};
dictionary PublicKeyCredentialUserEntity : PublicKeyCredentialEntity {
    required DOMString      id;
    BufferSource   handle;
};

I haven't lifted id up to the parent here because it will have different descriptions in the child types. The authenticator shouldn't give out a PublicKeyCredentialUserEntity without first verifying the user, so it shouldn't hurt to make all three fields required, right?

I think this would improve clarity a lot, but it would be a breaking change. What do people think?

@jcjones
Copy link
Contributor

jcjones commented Oct 18, 2017

This makes sense, and I struggle with this the same as you, but I'm tempted to reject it just because of how close we are to CR.

@AngeloKai, @balfanz , thoughts?

@emlun
Copy link
Member Author

emlun commented Oct 19, 2017

Yes, I understand it's likely not a good idea to do this this close to CR. In that case we'll just have to work around it in the prose descriptions.

@AngeloKai
Copy link
Contributor

It seems like everyone on the thread agrees that we shouldn't do this so close to CR. I am closing the issue as no action.

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