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

Allow multiple IDPs in the provider array #368

Closed
wants to merge 1 commit into from

Conversation

cbiesinger
Copy link
Collaborator

@cbiesinger cbiesinger commented Oct 19, 2022

[=fetch the accounts list=] algorithm with |manifest| and |provider|.
To <dfn>potentially create IdentityCredential</dfn>, given a list of {{IdentityProviderConfig}} |providers|:
1. Let |accountsList| be an empty list.
1. For each |provider| in |providers|:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
1. For each |provider| in |providers|:
1. In parallel, for each |provider| in |providers|:

We could/should do these in parallel right?

algorithm with |provider|.
1. If |manifest| is null, return null.
1. Let |providerAccountsList| be the result of running the
[=fetch the accounts list=] algorithm with |manifest| and |provider|.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Didn't @bvandersloot-mozilla say that he was worried about enabling fetching multiple identity providers before we factored into the IdP Sign-in Status API (because, IIRC, that would allow trackers to be introduced here in this list and fail deliberately invisibly)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can add an "Issue" here to note that.

Copy link
Collaborator

@npm1 npm1 left a comment

Choose a reason for hiding this comment

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

Looks good to me modulo Sam's comments

algorithm with |provider|.
1. If |manifest| is null, return null.
1. Let |providerAccountsList| be the result of running the
[=fetch the accounts list=] algorithm with |manifest| and |provider|.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can add an "Issue" here to note that.

@samuelgoto
Copy link
Collaborator

Can we try to update this PR to contain the latest proposal for the Multiple IdP API @cbiesinger @npm1 and @tttzach ? A PR may be a useful/concrete way that TAG reviews / Mozilla / Webkit can look at to give guidance on (we already have the explainer, which is pretty good, but a spec PR may make things more concrete).

@npm1
Copy link
Collaborator

npm1 commented Jan 16, 2024

This is kinda outdated but relevant. Do we plan to update this PR or should we close and later send a separate new PR?

@npm1 npm1 closed this Jan 16, 2024
@cbiesinger
Copy link
Collaborator Author

I think we can keep this closed and we can pick it up later, either from this PR or start over

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.

3 participants