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.create() method #33360

Merged
merged 14 commits into from
May 17, 2024

Conversation

wbamberg
Copy link
Collaborator

@wbamberg wbamberg commented May 1, 2024

This PR is an attempt to make https://developer.mozilla.org/en-US/docs/Web/API/CredentialsContainer/create more comprehensible, by making standalone pages for the three different dictionary types that are used to create each of the three credential types (password, federated, public key).

It makes https://developer.mozilla.org/en-US/docs/Web/API/CredentialsContainer/create much shorter, simpler, and consistent with the other method pages we have, and I hope exposes its high-level structure much more clearly.

I've also added glossary entries for "credential" and "authentication"- they are pretty short but a reasonable start.

Various things.

If you like this we will do the same for https://developer.mozilla.org/en-US/docs/Web/API/CredentialsContainer/get. There's a lot more scope for improving the credential management docs.

@github-actions github-actions bot added Content:WebAPI Web API docs Content:Glossary Glossary entries size/l [PR only] 501-1000 LoC changed labels May 1, 2024
@wbamberg wbamberg marked this pull request as ready for review May 1, 2024 21:45
@wbamberg wbamberg requested review from a team as code owners May 1, 2024 21:45
@wbamberg wbamberg requested review from sideshowbarker and hamishwillee and removed request for a team and sideshowbarker May 1, 2024 21:45
@hamishwillee
Copy link
Collaborator

My review list has grown out of control, but if no one else has dealt with this by Monday, I will look at it then. Feel free to ping me a reminder.

@hamishwillee
Copy link
Collaborator

hamishwillee commented May 10, 2024

I'm just having a scan now. Plan to look at this Monday/Tues in detail

  • Should we add the dictionaries FederatedCredentialInit and so on to https://developer.mozilla.org/en-US/docs/Web/API/Credential_Management_API and the sidebar?

  • The create() page has a note at the start saying "This method is restricted to top-level browsing contexts", which AFAICT is also wrong, I seem to get it working fine in an iframe.

    This doesn't seem to be defined in the spec. What I suspect is that this was true at some point for some kinds of credentials. But it isn't true or strictly true now.
    My evidence is https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Permissions-Policy/publickey-credentials-create which shows that in at least the Web authentication case, there is a permissions-policy gating. That means you can't just embed an iframe to anywhere - you need to specify the allowed non-same-origin sites via an HTTP header, and also specify them in the frame (except on FF, which doesn't send the header ever).

    We probably need to check the specs for each type of credential. I'd water this down with something that says this may be gated by permissions and link what we know.

- If the created credential type was a `password` object, the returned instance will be a {{domxref("PasswordCredential")}}.
- A {{domxref("FederatedCredential")}}, if the credential type was `federated`.
- A {{domxref("PasswordCredential")}}, if the credential type was `password`.
- A {{domxref("PublicKeyCredential")}}, if the credential type was `publicKey`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

PublicKeyCredential is missing from the sidebar and parent interface doc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

PublicKeyCredential is defined in the Web Authentication API, not the CM API: https://developer.mozilla.org/en-US/docs/Web/API/Web_Authentication_API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@wbamberg It fees wrong that it isn't listed on the parent interface but fair enough.

@hamishwillee
Copy link
Collaborator

@wbamberg Yes, this structure is vastly better - absolutely in favour of moving forward with this. How do you want to proceed. Merge and continue, or do get() and fixes to this as a follow on?

There are comments inline but here are a few more:

  1. PasswordCredential, the thing returned by create() has no cross linking to the create or init dictionary, and only refers to fact that you can pass this in fetch(). The example constructs one manually - I think we should at least note that you create or get one via create/get() (and/or state when manually creating one is a good idea.
  2. I have similar concerns about https://developer.mozilla.org/en-US/docs/Web/API/FederatedCredential
  3. https://developer.mozilla.org/en-US/docs/Web/API/PublicKeyCredential is part of another API. It should at least link as a related topic for all pages, and directly link to create()
  4. Init dictionaries in the sidebar.

@wbamberg wbamberg requested a review from a team as a code owner May 14, 2024 23:10
@wbamberg wbamberg requested review from pepelsbey and removed request for a team May 14, 2024 23:10
@wbamberg
Copy link
Collaborator Author

wbamberg commented May 15, 2024

@wbamberg Yes, this structure is vastly better - absolutely in favour of moving forward with this. How do you want to proceed. Merge and continue, or do get() and fixes to this as a follow on?

There are comments inline but here are a few more:

  1. PasswordCredential, the thing returned by create() has no cross linking to the create or init dictionary, and only refers to fact that you can pass this in fetch(). The example constructs one manually - I think we should at least note that you create or get one via create/get() (and/or state when manually creating one is a good idea.

  2. I have similar concerns about https://developer.mozilla.org/en-US/docs/Web/API/FederatedCredential

  3. https://developer.mozilla.org/en-US/docs/Web/API/PublicKeyCredential is part of another API. It should at least link as a related topic for all pages, and directly link to create()

For these, I want to say they are not in scope for this PR. This PR is about improving the create() page, not about improving adjacent pages. Looking at something like https://developer.mozilla.org/en-US/docs/Web/API/PasswordCredential for example, it's pretty close to a rewrite to make it connect properly with the CM API.

I think it's more valuable to merge this and then do the same with get(), and then think about some high-level guide material for the CM API, and maybe then improve the other reference pages.

  1. Init dictionaries in the sidebar.

-> f0c2d4f

@wbamberg
Copy link
Collaborator Author

Thanks for the review! I've made some changes and argued with some others :).

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.

I think it's more valuable to merge this and then do the same with get(), and then think about some high-level guide material for the CM API, and maybe then improve the other reference pages.

For sure. Though I'd work on the linked reference pages that are part of this API before the high level guide (just the way I roll :-).

PS, I note you also asked @pepelsbey for a review, but I am confident this is more than acceptable to merge based on the "much better than before" gate. Vadim, if you want to add notes as a post process, I'm sure Will will be happy to take them.

@hamishwillee hamishwillee merged commit 66afe9b into mdn:main May 17, 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