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

Map options member identifiers and credential types: public-key vs publicKey #181

Merged
merged 21 commits into from
Jan 17, 2022

Conversation

equalsJeffH
Copy link
Collaborator

@equalsJeffH equalsJeffH commented Nov 24, 2021

Please see issue #147 and w3c/webauthn#1004 for full context.

The summary is that this PR introduces a registry mapping credential types to options member identifiers, and updates the magical relevant credential interface objects algorithm to use the latter registry.

This is the "...define a mapping from dictionary member names to [[type]] values or vice versa." solution identified by @bzbarsky in w3c/webauthn#1004 (comment). @jyasskin had also independently suggested using a registry for this mapping.

Additionally, this PR contains some modest editorial updates/changes (that seem appropriate to make in conjunction with this PR's primary mission of mapping cred types to options member identifiers):

  • Clarify 2nd sentence of Introduction's first paragraph.
  • Clarify definition of "credential type", and note that other specs eg webauthn (can) define other cred types.
  • Explicitly delimit the (otherwise sort of hidden) "same-origin with its ancestors" algorithm by creating a "Infrastructure Algorithms" subsection containing it. Motivated in large part by adding the new Registry section after the algorithm. As a reader, I find this makes the spec more clear.
  • use the 1. numbering for all steps in algs that I modified.

fixes #147


Preview | Diff

@equalsJeffH equalsJeffH self-assigned this Nov 24, 2021
@equalsJeffH equalsJeffH changed the title Map options member identifiers and credential types Map options member identifiers and credential types: public-key vs publicKey Nov 24, 2021
Copy link
Member

@nsatragno nsatragno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this long standing issue! I like the registry mapping.

You mixed in some editorial changes in this PR. Can you update the PR description to point that out?

index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
Copy link
Member

@nsatragno nsatragno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm pending Jeffrey's comments

index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@equalsJeffH
Copy link
Collaborator Author

equalsJeffH commented Dec 21, 2021

I am thinking that we ought to land PR #182 first, then rebase this PR onto the resultant main branch, before finalizing and landing this PR.

update 6-Jan-2022: PR #182 is now merged.

equalsJeffH and others added 2 commits January 12, 2022 14:33
Use the registry to find interface objects for a CredentalOptions:
Merge branch 'jeffh-fix-147-options-member-name-and-cred-type' of https://github.com/w3c/webappsec-credential-management into jeffh-fix-147-options-member-name-and-cred-type
Copy link
Member

@jyasskin jyasskin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM aside from one nit.

index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
@equalsJeffH equalsJeffH merged commit 2311d7b into main Jan 17, 2022
@equalsJeffH equalsJeffH deleted the jeffh-fix-147-options-member-name-and-cred-type branch January 17, 2022 17:08
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

Successfully merging this pull request may close these issues.

publicKey member identifier in Options should be "public-key", or vice-versa?
3 participants