Add multiple IDP support

[npm1]

Relevant issue: #319

This PR adds support for multiple IDPs. It does so by adding some state to the document to allow bundling multiple get() calls with potentially multiple providers each. The bundling follows the pattern explained in #319 (comment): all calls before onload are bundled together while after onload we just use a task for bundling. There are also checks added to ensure that dupes in configURLs are not allowed. A few methods now return pairs so as to enable tracking the provider to which the selected account belongs to, which in turn enables determining which get() call to provide the credential to, in case of a successful flow.

---

Preview | Diff

[npm1]

@bvandersloot-mozilla I was hoping you could take a look at this before you go on leave! I know it is a bit late, so if you're unable to get a chance then I will bug your teammates :)

[cbiesinger]

You are doing this so that you always return a 2-tuple? I don't think this is necessary... I'd just say "or failure"

[npm1]

It's not necessary but it was cleaner to always return a tuple here instead of maybe returning failure and then having to do checks about it in the caller.

[cbiesinger]

FYI, longer term we should add a callback from the HTML spec to FedCM as described here: whatwg/html#8382 (comment)

(and also get rid of the DOM event listener)

[npm1]

Yea I should probably do that, for the other part (the one using event listener)

[bvandersloot-mozilla]

I have one major structural complaint: I'd like to be able to have a provider-chooser before the account chooser. Could we have an optional provider list filter step after the config fetches but before the account list fetches? This would enable both UI designs

I haven't gone through for bugs or deep-dived on the Bikeshed, and think @cbiesinger had some good points about figuring out how to handle the HTML integration and async events better. But other than the line above, this matches my expectations.

[bvandersloot-mozilla]

Do you want to coalesce requests in separate iframes? If so, we should probably unify these to be one per window. For example, using globalObject's top-level traversable's active Document.

[npm1]

Good question. I think for now we do not want to mix requests from different frames.

[bvandersloot-mozilla]

My thoughts on this depend on the behavior of major IDPs that I don't know off of the top of my head... Do the APIs included via script tags usually iframe themselves off before trying to establish a session on page load? Put another way: does the equivalent of the google.accounts.id.initialize call do the meat of its work inside of an iframe for any major IDP?

If so, I think mixing requests from frames is important to this extension's utility.

[bvandersloot-mozilla]

Did you consider any alternatives to the DOM manipulation task source? It seems reasonable, but I'm not confident.

[npm1]

It could also be the networking task source since those steps will produce a bunch of fetches. I'm open to change this but not sure if it makes a big difference (as long as similar fedcm steps run on the same task source)

[npm1]

I have one major structural complaint: I'd like to be able to have a provider-chooser before the account chooser. Could we have an optional provider list filter step after the config fetches but before the account list fetches? This would enable both UI designs

Added

I haven't gone through for bugs or deep-dived on the Bikeshed, and think @cbiesinger had some good points about figuring out how to handle the HTML integration and async events better. But other than the line above, this matches my expectations.

Ah yea I think for now we should monkeypatch the HTML spec instead of what I did. Haven't fixed this part but ptal at the rest

[achimschloss]

There is no mentioning here how this dialogue is constructed w.r.t. to ordering suggestions by the RP - This was discussed at length - #319 (comment)

@npm1

All of these suggestions sound good to me. Having the bar 'show at least after onload' achieves the goal of enabling all IDPs to register while preserving flexibility on the timing. This coupled with RP provided ordering seems like a good path forward. I like how this is shaping up!

@bvandersloot-mozilla

Given this, I think that is probably the way forward. How about instead of "the onload event" we talk about "the display of IDP chooser, which must occur after the onload event"? This leaves more flexibility for quiet UI and doesn't artificially constrain the "early calls" to before onload when there may be more time. Also, we could add an option to the RP's ordering suggestion call that can suggest to the browser to use a louder UI.

The general context was discussed here #348 and in a fedccg meeting see here - https://github.com/fedidcg/meetings/blob/main/2022/2022-10-10-notes.md

[npm1]

Yea we could certainly add the RP-specified ordering, although it is a minor addition in terms of how it impacts the processing model, and I don't think we are settled on the API shape yet. I lean towards having a separate method, something like navigator.credentials.setProviderOrdering(["configURL1", "configURL2"]) but I don't think we have reached consensus on that. Once we do then I can add that to this PR or a later PR if this has merged, WDYT?

[bvandersloot-mozilla]

I think it would be good to include here given the interest and our initial commitments.

If we're hanging it off of navigator.credentials we should generalize it to be applicable to all credentials.

However, I prefer to put it somewhere like window.IdentityProvider.setPromptOrdering, and maybe move the Sign-In status API to window.IdentityProvider.status.*. This makes it clear that it is tied to Identity Provider concerns, rather than general credentials.

[samuelgoto]

navigator.credentials.setProviderOrdering(["configURL1", "configURL2"])

If we start from the assumption that the relying party is going to be making a JS call (which I'm not sure is a good idea), then we can revisit the entirety of this API design here and take one of the other alternatives that don't rely on batching API calls, that is the Register and Prompt alternative we considered.

We could do:

// Called from JS SDKs
let {token} = await IdentityProvider.register({
  configURL: // ...
});

And then later the RP can call:

await navigator.credentials.get({
  identity: {
    providers: [{
      order: ["configUrl1", "configUrl2", ...]
    }]
  }
});

[npm1]

However, I prefer to put it somewhere like window.IdentityProvider.setPromptOrdering, and maybe move the Sign-In status API to window.IdentityProvider.status.*. This makes it clear that it is tied to Identity Provider concerns, rather than general credentials.

I'm fine with IdentityProvider.setPromptOrdering() too, as we don't have use-cases for making this more general to credentials. Although we might in the future.

If we start from the assumption that the relying party is going to be making a JS call (which I'm not sure is a good idea), then we can revisit the entirety of this API design here and take one of the other alternatives that don't rely on batching API calls, that is the Register and Prompt alternative we considered.

I don't think that suggestion works, and the reasons are stated in the section you linked. We need to fulfill these requirements:

1. The API should be easily invocable from IDP SDKs, independently.
2. The IDP ordering should be customizable by an RP that wishes to do so. Note that this customization is entirely optional: an RP does not have to specify an IDP ordering.

Your suggestion does not satisfy 1), a key requirement. We are not assuming that the RP is going to directly make a JS call. I think however it is fair to assume that if the RP wants to specify IDP ordering, then they should be capable of adding a JS call. Note that this JS call only requires very limited info about the IDPs (their origins or configURLs), not the full set of things required from a get() call. But this JS call is also not required: the user agent can decide an IDP ordering if the RP provides no signal about their preference.

An alternative I considered is passing an order parameter directly in the get() call. I worry that this means the IDP is the one setting the value since they are the ones invoking the FedCM API, and there's not a lot of incentive to respect the desired RP value (an IDP would always want to be shown as the first one).

[achimschloss]

An alternative I considered is passing an order parameter directly in the get() call. I worry that this means the IDP is the one setting the value since they are the ones invoking the FedCM API, and there's not a lot of incentive to respect the desired RP value (an IDP would always want to be shown as the first one).

I agree, this could also lead to conflicting values passed to the get calls. Having some sort of independent way for RPs to define their preferences in general seems sensible outside of some defined default behaviour. Making this optional makes sense to me too. I guess this API surface should then also not be available in IDP IFrames.

However, I prefer to put it somewhere like window.IdentityProvider.setPromptOrdering, and maybe move the Sign-In status API to window.IdentityProvider.status.*.

Agree - this would also leave room for catering to additional needs of RPs (#348) by adding additional means to define this via api surfaces to window.IdentityProvider. (for example a means to differentiate if they want to trigger a sign-in vs. a sign-up flows or don't care (default), which is not really a multi-idp question thus not in scope for this PR)

[bvandersloot-mozilla]

I have one major structural complaint: I'd like to be able to have a provider-chooser before the account chooser. Could we have an optional provider list filter step after the config fetches but before the account list fetches? This would enable both UI designs

Added

👍 TY!

I haven't gone through for bugs or deep-dived on the Bikeshed, and think @cbiesinger had some good points about figuring out how to handle the HTML integration and async events better. But other than the line above, this matches my expectations.

Ah yea I think for now we should monkeypatch the HTML spec instead of what I did. Haven't fixed this part but ptal at the rest

Agreed. Then once this is graduated to a WG (or more stable) we can upstream the monkeypatch.
I've looked at the rest and it makes logical sense and represents the vision I have.

I don't have the time to deep-dive on the Bikeshed before my leave, unfortunately. This is mostly because of my general unfamiliarity with the weeds of what is allowed "in parallel" and how to play nice with WHATWG HTML. I think that a lot of the complication here stems from the Credential internal methods being run in parallel. Like the "wait for X to be set" lines where promises are natural. However, I don't spot any bugs in the constructions you make and the algorithms you add. Great work here- there is creativity in this I hadn't gotten far enough to see needed in my sketches.

[npm1]

This seems outdated and I am trying to clean up open pull requests so I'm closing. I do plan to send a PR for multi IDP in the near future, but not including the load event heuristic.