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.

Sign up for GitHub

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 multiple IDP support #438

Closed
wants to merge 3 commits into from
Closed

Add multiple IDP support #438

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8ba3829
Add support for multiple IDPs
npm1 Feb 14, 2023
c7a3f9f
m
npm1 Feb 14, 2023
9bdfe44
b
npm1 Feb 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
164 changes: 121 additions & 43 deletions spec/index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,11 @@ NOTE: The {{CredentialRequestOptions/mediation}} flag is currently not used.
The {{CredentialRequestOptions/signal}} is used as an abort signal for the
requests.

Each {{Document}} has an associated <dfn>pending fedID request list</dfn>, an initially empty [=list=].

Each {{Document}} has an associated <dfn>fedID task status</dfn>, which is initially set to
"<code>[=no task=]</code>". Its possible values are "<code><dfn>no task</dfn></code>", "<code><dfn>pending</dfn></code>", or "<code><dfn>running</dfn></code>".

<div algorithm>
When the {{IdentityCredential}}'s
<dfn for="IdentityCredential" method>\[[DiscoverFromExternalSource]](origin, options, sameOriginWithAncestors)</dfn>
Expand All @@ -461,9 +466,67 @@ algorithm is invoked, the user agent MUST execute the following steps. This retu

1. Assert: These steps are running [=in parallel=].
1. Assert: |options|["{{CredentialRequestOptions/identity}}"]["{{IdentityCredentialRequestOptions/providers}}"] [=map/exists=].
1. Assert: |options|["{{CredentialRequestOptions/identity}}"]["{{IdentityCredentialRequestOptions/providers}}"] [=list/size=] is 1.
1. Let |document| be |globalObject|'s [=associated Document=].
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


Issue: Support choosing accounts from multiple [=IDP=]s, as described [here](https://github.com/fedidcg/FedCM/issues/319).
Note: The |globalObject| is not currently passed onto the
{{Credential/[[DiscoverFromExternalSource]](origin, options, sameOriginWithAncestors)}}
algorithm. See <a href="https://github.com/w3c/webappsec-credential-management/issues/210">issue</a>.

1. Let |credentialOrError| be a variable set to null.
1. If |document|'s [=fedID task status=] is "<code>[=running=]</code>":
1. [=Queue a global task=] with |globalObject| on the [=DOM manipulation task source=] to
set |credentialOrError| to a new "{{NotAllowedError}}" {{DOMException}}.
1. Wait until |credentialOrError| is set, and return it.
1. If |document|'s [=fedID task status=] is "<code>[=no task=]</code>":
1. If |document|'s {{Document/readyState}} is "complete":
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

1. [=queue a global task=] with |globalObject| on the [=DOM manipulation task source=] to
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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)

[=run pending fedID requests=] with |document|.
1. Set |document|'s [=fedID task status=] to "<code>[=pending=]</code>".
1. Otherwise:
1. Let |fedCMListener| be an [=event listener=] set as follows:

: <a spec=dom for="event listener">type</a>
:: "load"
: <a spec=dom for="event listener">callback</a>
:: new {{EventListener}} whose {{EventListener/handleEvent(event)}} invokes
[=run pending fedID requests=] with |document|.
: <a spec=dom for="event listener">once</a>
:: true
1. [=Add an event listener=] passing |document| and |fedCMListener|.

Issue: [better](https://github.com/fedidcg/FedCM/issues/440) invoke the method after
the {{Document}} has loaded.

1. Set |document|'s [=fedID task status=] to "<code>[=pending=]</code>".
1. Let |providers| be |options|["{{CredentialRequestOptions/identity}}"]["{{IdentityCredentialRequestOptions/providers}}"].
1. Let |allConfigURLs| be an [=ordered set=].
1. For each (|providerList|, <var ignore="">cred</var>) in |document|'s [=pending fedID request list=]:
1. For each |provider| in |providerList|:
1. [=set/Append=] |provider|'s {{IdentityProviderConfig/configURL}} to
|allConfigURLs|.
1. For each |provider| in |providers|:
1. If |allConfigURLs| [=list/contains=] |provider|'s {{IdentityProviderConfig/configURL}}:
1. [=Queue a global task=] with |globalObject| on the [=DOM manipulation task source=] to
set |credentialOrError| to a new "{{NotAllowedError}}" {{DOMException}}.
1. Wait until |credentialOrError| is set, and return it.
1. [=set/Append=] |provider|'s {{IdentityProviderConfig/configURL}} to
|allConfigURLs|.
1. [=list/Append=] (|providers|, |credentialOrError|) to |document|'s [=pending fedID request list=].
1. Wait until |credentialOrError| becomes set, either as an {{IdentityCredential}} or as an
exception.
1. Return |credentialOrError|.
</div>

<div algorithm>
When asked to <dfn>run pending fedID requests</dfn> with a {{Document}} |document|, run the
following steps [=in parallel=]:
1. The [=user agent=] MAY wait some time before proceeding with the following steps. This
provides the [=user agent=] with flexibility on how to bundle together multiple federated
identity requests from a site.
1. Set |document|'s [=fedID task status=] to "<code>[=running=]</code>".
1. Let |allProviders| be an initially empty [=list=].
1. For each (|providers|, |credentialOrError|) in |document|'s [=pending fedID request list=]:
1. [=Extend=] |allProviders| with |providers|.
1. Run {{WindowOrWorkerGlobalScope/setTimeout()}} passing a [=task=] which throws a
{{NetworkError}}, after a timeout of 60 seconds.

Expand All @@ -473,63 +536,76 @@ algorithm is invoked, the user agent MUST execute the following steps. This retu
Note: the purpose of having a timer here is to avoid leaking the reason causing this
method to throw an error. If there was no such timer, the developer could easily infer
whether the user has an account with the [=IDP=] or not, or whether the user closed the UI without granting permission to share the [=IDP=] account information with the [=RP=].
1. Let |provider| be |options|["{{CredentialRequestOptions/identity}}"]["{{IdentityCredentialRequestOptions/providers}}"][0].
1. Let |credential| be the result of running [=create an IdentityCredential=] with |provider| and
|globalObject|.

Note: The |globalObject| is not currently passed onto the
{{Credential/[[DiscoverFromExternalSource]](origin, options, sameOriginWithAncestors)}}
algorithm. See <a href="https://github.com/w3c/webappsec-credential-management/issues/210">issue</a>.

1. If |credential| is failure, [=queue a global task=] on the [=DOM manipulation task source=]
to throw a new "{{NetworkError}}" {{DOMException}}.
1. Let |globalObject| be |document|'s [=relevant global object=]
1. Let (|credential|, |selectedProvider|) be the result of running [=create an IdentityCredential=]
with |allProviders| and |globalObject|.
1. For each (|providers|, |credentialOrError|) in |document|'s [=pending fedID request list=]:
1. If |credential| is failure or if |providers| does not [=list/contain=] |selectedProvider|,
[=queue a global task=] on the [=DOM manipulation task source=] to set
|credentialOrError| to a new "{{NetworkError}}" {{DOMException}}.
1. Otherwise, set |credentialOrError| to |credential|.
1. Set |document|'s [=fedID task status=] to "<code>[=no task=]</code>".
1. [=list/Empty=] |document|'s [=pending fedID request list=].
</div>

<div algorithm>
To <dfn>create an IdentityCredential</dfn> given an {{IdentityProviderConfig}}
|provider| and a |globalObject|, run the following steps. This returns an {{IdentityCredential}} or
failure.
To <dfn>create an IdentityCredential</dfn> given a [=list=] of {{IdentityProviderConfig}}s
|providers| and a |globalObject|, run the following steps. This returns an ({{IdentityCredential}},
{{IdentityProviderConfig}}) [=tuple=] or the [=tuple=] (failure, failure).
1. Assert: These steps are running [=in parallel=].
1. Let |config| be the result of running [=fetch the config file=] with |provider| and
|globalObject|.
1. If |config| is failure, return failure.
1. Let |accountsList| be the result of [=fetch the accounts list=] with |config|, |provider|,
and |globalObject|.
1. For each |account| in |accountsList|:
1. If |account|["{{IdentityProviderAccount/picture}}"] is present,
[=fetch the account picture=] with |account| and |globalObject|.

Note: The [=user agent=] may choose to show UI which does not initially require fetching the
account pictures. In these cases, the [=user agent=] may delay these fetches until they are
needed.
1. If |accountsList|'s size is 1:
1. Let |account| be |accountsList|[0].
1. Let |configMap| be an initially empty [=map=].
1. For each {{IdentityProviderConfig}} |provider| in |providers|:
1. Let |config| be the result of running [=fetch the config file=] with |provider| and
|globalObject|.
1. Set |configMap|[|provider|] to |config|.
1. Let |allAccountsAndProviders| be an initially empty [=list=].
1. The user agent MAY show an [=IDP=] chooser to the user, and if it does so then it should set
|providers| to a list including only the selected [=IDP=].
1. For each |provider| over |providers|:
1. Let |config| be |configMap|[|provider|].
1. If |config| is failure, continue.
1. Let |accountsList| be the result of [=fetch the accounts list=] with |config|, |provider|,
and |globalObject|.
1. For each |account| in |accountsList|:
1. If |account|["{{IdentityProviderAccount/picture}}"] is present,
[=fetch the account picture=] with |account| and |globalObject|.

Note: The [=user agent=] may choose to show UI which does not initially require fetching the
account pictures. In these cases, the [=user agent=] may delay these fetches until they are
needed.
1. [=list/Append=] (|account|, |provider|) to |allAccountsAndProviders|.
1. If |allAccountsAndProviders|'s size is 0, return failure.
1. Let |account|, |provider| be both initially set to null.
1. If |allAccountsAndProviders|'s size is 1:
1. Set (|account|, |provider|) be |allAccountsAndProviders|[0].
1. Let |accountState| be the result of running the [=compute account state=] algorithm
given |provider|, |account|, and |globalObject|.
1. If |accountState|'s {{AccountState/registration state}} is {{unregistered}},
let |permission| be the result of running [=request permission to sign-up=] algorithm
with |account|, |accountState|, |config|, |provider|, and |globalObject|.
with |account|, |accountState|, |configMap|[|provider|], |provider|, and |globalObject|.
1. Otherwise, show a dialog to request user permission to sign in via |account|, and set the
result in |permission|.
1. If |permission|, [=sign-in=] with |accountState|.
1. Otherwise:
1. Let |account| be the result of running the [=select an account=] from the
|accountsList|.
1. Set (|account|, |provider|) to the result of running the [=select an account=] from
|allAccountsAndProviders|.
1. If |account| is failure, return failure.
1. Let |accountState| be the result of running the [=compute account state=] algorithm
given |provider| and |account|.
1. If |accountState|'s {{AccountState/registration state}} is {{unregistered}}:
1. Let |permission| be the result of running the [=request permission to sign-up=]
algorithm with |account|, |accountState|, |config|, |provider|, and |globalObject|.
algorithm with |account|, |accountState|, |configMap|[|provider|], |provider|, and
|globalObject|.
1. If |permission|, [=sign-in=] with |accountState|.
1. Otherwise, [=sign-in=] with |accountState|.
1. Wait until the [=user agent=]'s dialog is closed.
1. If |accountState|'s {{AccountState/registration state}} is {{unregistered}} then return
failure.
1. Assert: |account| and |provider| are not null or failure.
1. Let |credential| be the result of running the [=fetch an identity assertion=] algorithm with
|accountState|, |account|'s {{IdentityProviderAccount/id}}, |provider|, |config|, and
|globalObject|.
1. Return |credential|.
|accountState|, |account|'s {{IdentityProviderAccount/id}}, |provider|,
|configMap|[|provider|], and |globalObject|.
1. Return (|credential|, |provider|).
</div>

<div algorithm>
Expand Down Expand Up @@ -939,13 +1015,15 @@ dictionary IdentityProviderClientMetadata {
</xmp>

<div algorithm>
To <dfn>select an account</dfn> given an |accountsList|, run the following steps. This returns an
{{IdentityProviderAccount}} or failure.
1. Assert |accountsList|'s [=list/size=] is greater than 1.
1. Display an account chooser displaying the options from |accountsList|.
1. Let |account| be the {{IdentityProviderAccount}} of the account that the user
manually selects from the accounts chooser, or failure if no account is selected.
1. Return |account|.
To <dfn>select an account</dfn> given |allAccountsAndProviders|, run the following steps. This
returns an ({{IdentityProviderAccount}}, {{IdentityProviderAPIConfig}}) [=tuple=] or the [=tuple=]
(failure, failure).
Copy link
Collaborator

Choose a reason for hiding this comment

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

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"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

1. Assert |allAccountsAndProviders|'s [=list/size=] is greater than 1.
1. Display an account chooser displaying the options from |allAccountsAndProviders|.
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no mentioning here how this dialogue is constructed w.r.t. to ordering suggestions by the RP - This was discussed at length - https://github.com/fedidcg/FedCM/issues/319#issuecomment-1352319800

@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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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", ...]
    }]
  }
});

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

1. Let (|account|, |provider|) be the {{IdentityProviderAccount}} and associated
{{IdentityProviderConfig}} of the account that the user manually selects from the accounts
chooser, or (failure, failure) if no account is selected.
1. Return (|account|, |provider|).
</div>

<!-- ============================================================ -->
Expand Down