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

Add single-get multi IDP support for passive mode #686

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

npm1
Copy link
Collaborator

@npm1 npm1 commented Dec 10, 2024

Adds support for multiple IDPs on passive mode by:

  • Not rejecting if the list passed is not of size 1.
  • Still rejecting if the list passed is not of size 1 and mode is active (it is possible this PR is later updated to remove this restriction)
  • Performing some per-IDP checks first, and computing a map based on the results
  • Showing UI for the relevant IDPs based on the results from the computations: optional UIs for logged out IDPs, mandatory UI for mismatch IDPs, and all accounts for IDPs that returned some accounts.
  • Add a configURL to IdentityCredential so the RP knows which IDP the credential corresponds to.

Also some unrelated errors noticed while drafting this PR are fixed.

Relevant issue: w3c-fedid/multi-idp#2


Preview | Diff

@npm1
Copy link
Collaborator Author

npm1 commented Jan 13, 2025

ptal

@wseltzer
Copy link
Collaborator

Discussed at Jan 21, 2025 meeting minutes.

@bvandersloot-mozilla
Copy link
Collaborator

This overall seems reasonable, but raises questions for me about how a browser with an IDP picker integrates with the spec. Right now it works with single IDP, however, I think that I want to be able to show account information in the IDP picker where there is already a linkage.

How does this handle where a response is in the connected account set? Does it still show the dialog?

@npm1
Copy link
Collaborator Author

npm1 commented Feb 6, 2025

Hey Ben, sorry not sure I follow your questions but trying to answer:

I think that I want to be able to show account information in the IDP picker where there is already a linkage.

Regarding IDP picker with account information: sure, this is intended to be allowed. You can show any UI before fetching the accounts, so in particular you can show UI using some information that you cache from an account in the connected accounts set. However, I do worry that this effectively removes auto reauthentication since you can only really know the returning accounts after fetching. You could guess when it may happen based on the connected accounts set but that seems brittle.

How does this handle where a response is in the connected account set? Does it still show the dialog?

Not sure I follow. If there are accounts in the connected account set, this just means it may be possible to perform auto reauthentication. Of course, the user agent is encouraged to nudge the user towards these accounts as well as accounts whose approved_clients imply that this is an IDP account that has already been previously used to login to the RP. But in general, unless there is exactly one account in the connected accounts set AND the RP requests auto reauthentication, then the IDP should try to perform the login automatically, whereas in any other case we let the user confirm the account they wish to use first.

@cbiesinger
Copy link
Collaborator

@bvandersloot-mozilla is your question maybe this: does the algorithm allow fetching and displaying accounts only from IDPs who have accounts in the connected set, and only showing the IDP name for the other IDPs (fetching accounts once you click that)?

@bvandersloot-mozilla
Copy link
Collaborator

@cbiesinger, that is precisely what I meant, thanks!

@npm1
Copy link
Collaborator Author

npm1 commented Feb 19, 2025

Added support for this (during the wait, the user agent may set the provider list to a subset of itself)

@npm1
Copy link
Collaborator Author

npm1 commented Mar 18, 2025

Ping @bvandersloot-mozilla. Let me know if you want me to add active mode support here as well or any other comments so I can merge this

@yi-gu yi-gu added the agenda+ Regular CG meeting agenda items label Mar 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agenda+ Regular CG meeting agenda items
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants