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

Document merging get() calls for multiple IDP usage #351

Closed
wants to merge 1 commit into from
Closed

Document merging get() calls for multiple IDP usage #351

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
46 changes: 43 additions & 3 deletions spec/index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1077,7 +1077,7 @@ NOTE: The {{CredentialRequestOptions/mediation}} flag is currently not used.
The {{CredentialRequestOptions/signal}} is used as an abort signal for the
requests.

<div algorithm>
<div algorithm="DiscoverFromExternalSource">
When the {{IdentityCredential}}'s
<dfn for="IdentityCredential" method>\[[DiscoverFromExternalSource]](origin, options, sameOriginWithAncestors)</dfn>\
algorithm is invoked, the user agent MUST execute the following steps:
Expand All @@ -1092,11 +1092,51 @@ requests.
Note: the purpose of having a timer here is to avoid leaking the reason causing this
method to return null. 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 |all_options| be a list of pairs of {{CredentialRequestOptions}}
and {{Promise}} objects.
This list is reused for all {{Credential/[[DiscoverFromExternalSource]]}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Generally how this is handled is that we add associated members to the relevant object. So here it would probably be "Each Window has an associated allOptions which is initially bla"

calls within the same global object.
1. Create a new promise and append (|options|, new promise) to |all_options|.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be clearer to name the new promise some var.

1. If a load event listener (next step) has already been registered or
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could use a boolean to track this more formally.

a task has already been posted (step after next), return the promise.
1. If the document is not <a spec=HTML>completely loaded</a>, register
an [=event handler=] on the {{Window}} object for the
{{Window/load}} event. When the listener is called, proceed to the
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is just better to add new algorithm as the listener, and that algorithm is actually the 'next steps'

next step.

Note: Typically, RPs interact with IDPs using an SDK provided by the
IDP. It is desirable to be able to show multiple IDPs together in the
account chooser even with IDP SDKs that do not cooperate with each
other. This and the next step make this possible.
1. [=Queue a global task=] where the global object is the current window
Copy link
Collaborator

Choose a reason for hiding this comment

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

A couple of things are unclear to me in this PR:

(a) what happens to all of the promises that are in the |all_options| array that aren't picked? Do they get resolved? Failed? I think that, as specified, they never resolve? Is that deliberate?

(b) what happens to the AbortSignals that were passed? If one of the AbortSignals that was passed in the |all_options| gets aborted, does that get ignored?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For a): See the paragraph starting at line 1119

For b): See the paragraph starting at line 1124.

and the task source is the <dfn export>federated credential management
task source</dfn> and steps is "go to the next step".
1. Let |options| be the result of running the [=combine CredentialRequestOptions=]
algorithm with |all_options|.
1. Let |provider| be |options|["{{CredentialRequestOptions/identity}}"]["{{IdentityCredentialRequestOptions/providers}}"][0].
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, this PR is combining the provider options but then throwing all-but-the-first away in this line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes... I wanted to decouple this specific behavior of merging get calls from general multi-IDP support. Do you think I should change this PR to cover both?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This would need to not limit to the first one, and the "potentially create IdentityCredential" would need to allow an array of providers instead of a single one.

1. Let |credential| be the result of running the [=potentially create IdentityCredential=]
algorithm with |provider|.
1. If |credential| is null, wait for the task that throws a {{NetworkError}}, otherwise return
|credential|.
1. If |credential| is null, wait for the task that throws a {{NetworkError}}, otherwise
find the pair in |all_options| where the {{IdentityProviderConfig/configURL}}
matches the selected |credential|. Resolve the promise in that pair with
this |credential| and reject all other promises in the list with
{{NetworkError}}.
1. If, at any time, any of the |options|.{{CredentialRequestOptions/signal}}s
in |all_options| is aborted, abort further processing, close any UI
that may have been shown as part of the request, and reject all
promises with an {{AbortError}}.
1. Clear |all_options|.
</div>

<div algorithm>
To <dfn>combine CredentialRequestOptions</dfn>:
1. Let |all_options| be the input options
1. Let |options| be a new {{CredentialRequestOptions}} object
1. The {{CredentialRequestOptions}}.{{CredentialRequestOptions/identity}}.
{{IdentityCredentialRequestOptions/providers}} arrays of all objects
in the list are concatenated and set as |options|`.identity.providers`.
1. If there is a duplicate provider configURL in the array, all
promises are rejected.
</div>

<div algorithm>
Expand Down