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

Normalize CredentialsContainer.get() #35255

Merged
merged 16 commits into from
Aug 6, 2024

Conversation

wbamberg
Copy link
Collaborator

@wbamberg wbamberg commented Jul 30, 2024

This is a companion to #33360, doing the same thing for CredentialsContainer.get() that #33360 did for CredentialsContainer.create().

It's also some kind of step towards https://github.com/orgs/mdn/discussions/683.

I've created new dictionary pages for PublicKeyCredentialRequestOptions and IdentityCredentialRequestOptions to give us space for the complex options for these credential types. The other, simpler, options I've continued to document inline.

I've fixed some errors (e.g. ISTM that attestation options are not given to get() when using WebAuth, they are only used for create()) and various misleading things (e.g. the implication that AbortError is only thrown when you're requesting an OTP credential). I've factored the parts on different credential types into a common format. It's very helpful to have a "credential types" guide we can refer to, to provide context for the API.

I haven't done a lot of work on the detailed prose, in the interests of keeping this PR mostly focused on structural issues. I'd prefer to tackle things like that in follow-ups, unless I've actually regressed anything here.

@wbamberg wbamberg requested review from a team as code owners July 30, 2024 23:19
@wbamberg wbamberg requested review from sideshowbarker and pepelsbey and removed request for a team July 30, 2024 23:19
@github-actions github-actions bot added the Content:WebAPI Web API docs label Jul 30, 2024
@wbamberg wbamberg requested review from hamishwillee and removed request for sideshowbarker July 30, 2024 23:19
@github-actions github-actions bot added the size/l [PR only] 501-1000 LoC changed label Jul 30, 2024
@wbamberg wbamberg removed the request for review from pepelsbey July 30, 2024 23:19
Copy link
Contributor

github-actions bot commented Jul 30, 2024

- : An array of strings representing the credentials' federated identity providers (for example `"https://www.facebook.com"` or `"https://accounts.google.com"`).

### `password` boolean value
- `federated` {{optional_inline}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI only - a bit of a pity this isn't in BCD so it could be marked as deprecated by our system.s

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would like to understand the situation regarding federated versus identity better, and generally improve our messaging on it.

AFAIK FedCM (which adds identity) is part of Google's efforts to deprecate third party cookies - so the big issue with federated is that it depends on third-party cookies. So I know that Google (at least) would like people not to use federated. But noone else even supports FedCM. So is it even right to talk about federated as deprecated? To what extent do web developers use identity versus federated? And how is this affected by Google's recent announcement that third-party cookies are not after all going away?

Perhaps @chrisdavidmills knows?

But anyway that's surely for a later PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. This is resolved since we don't know enough and it shouldn't be blocking. However leaving discussion open for @chrisdavidmills to comment if desired.

Comment on lines 5 to 7
status:
- experimental
spec-urls: https://w3c.github.io/webauthn/#dictdef-publickeycredentialrequestoptions
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as https://github.com/mdn/content/pull/35255/files#r1703549848 - we need the spec url, but not sure whether we should have status (because we can't automate it) and whether we should have browser compat data for the associated get() option.

@hamishwillee
Copy link
Collaborator

@wbamberg Yes, structure makes get() a lot more digestible - it will never be a simple API though. I've added a few minor comments, but most of them are nits.

Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
wbamberg and others added 2 commits August 5, 2024 07:45
Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
…mjson_static/index.md

Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
@wbamberg wbamberg requested a review from a team as a code owner August 5, 2024 18:37
@wbamberg wbamberg requested review from chrisdavidmills and removed request for a team August 5, 2024 18:37
@github-actions github-actions bot added the Content:Glossary Glossary entries label Aug 5, 2024
wbamberg and others added 3 commits August 5, 2024 11:42
Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
… credentials-management-get

* origin/credentials-management-get:
  Update files/en-us/web/api/identitycredentialrequestoptions/index.md
@wbamberg
Copy link
Collaborator Author

wbamberg commented Aug 5, 2024

All right, I think I have addressed or argued with everything. Thank you for the review @hamishwillee , much appreciated as always!


{{GlossarySidebar}}

An **authenticator** is an entity that is inside or attached to the user's device, and that can perform the cryptographic operations needed to register and authenticate users, and securely store the cryptographic keys used in these operations.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This sentence is a bit of a mouthful, and should ideally be chopped up. We might remove how it is attached/integrated in the first sentence because this is covered in the second. Also answer the question "can this be software based"? That may be why you used the term "entity"?

Anyway, something like

Suggested change
An **authenticator** is an entity that is inside or attached to the user's device, and that can perform the cryptographic operations needed to register and authenticate users, and securely store the cryptographic keys used in these operations.
An **authenticator** is a device that that can perform the cryptographic operations needed to register and authenticate users, and securely store the cryptographic keys used in these operations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also answer the question "can this be software based"? That may be why you used the term "entity"?

Yes and yes. I agree about "attached to or inside" being covered well enough in paragraph 2, and I don't like "entity" very much either. But, I don't think we can use "device" here unless we also change "device" in paragraph 2. I think it is worth being clear about the hardware/software thing though.

Will make a suggestion.

Copy link
Collaborator

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

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

@wbamberg Looks great. Yes you have addressed my issues. Two left, but approving so you can merge when you have looked at them:

Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
Co-authored-by: wbamberg <will@bootbonnet.ca>
@wbamberg
Copy link
Collaborator Author

wbamberg commented Aug 6, 2024

Thanks for the reviews Hamish!

@wbamberg wbamberg merged commit a8ff915 into mdn:main Aug 6, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:Glossary Glossary entries Content:WebAPI Web API docs size/l [PR only] 501-1000 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants