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

cbiesinger · 2022-09-23T22:02:38Z

npm1

Could we integrate the changes into the existing processing logic in the spec?

npm1 · 2022-09-26T14:14:32Z

spec/index.bs

+even with IDP SDKs that do not cooperate with each other. To make this
+possible, the UA has to merge get() calls as follows:
+
+    1. If `navigator.credentials.get()` is called before or during onload the


Need to clarify this is window onload

npm1 · 2022-09-26T14:15:18Z

spec/index.bs

+
+    1. If `navigator.credentials.get()` is called before or during onload the
+        UA saves the provided options and delays further processing until
+        immediately after onload.


Unclear what immediately after means. Microtask/task/something else?

npm1 · 2022-09-26T14:16:02Z

spec/index.bs

+        processing until that microtask.
+    1. When resuming processing, the saved options from all calls to get are
+        combined as follows:
+      1. The provider arrays are concatenated


What happens if there are repeated configURLs?

samuelgoto · 2022-09-26T21:27:52Z

spec/index.bs

@@ -1424,6 +1424,28 @@ steps:
    1. <a for=Promise>Resolve</a> |promise| with [undefined] and return |promise|.
 </div>

+<!-- ======================================================= -->


I was expecting to see this PR being intermingled with the [[DiscoverFromExternalSource]] algorithm, rather than as a separate section.

Should we merge this with that?

This should be done, please take another look

npm1 · 2022-10-11T16:25:00Z

spec/index.bs

@@ -1112,6 +1112,24 @@ 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 providing consent to share the [=IDP=] account information with the [=RP=].
+      1. If the load event on the window object has not fired yet, or is


Can we link to load event

npm1 · 2022-10-11T16:28:09Z

spec/index.bs

@@ -1112,6 +1112,24 @@ 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 providing consent to share the [=IDP=] account information with the [=RP=].
+      1. If the load event on the window object has not fired yet, or is


Is there a link for fired? Also not entirely clear what 'currently being fired' means?

npm1 · 2022-10-11T16:28:55Z

spec/index.bs

@@ -1112,6 +1112,24 @@ 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 providing consent to share the [=IDP=] account information with the [=RP=].
+      1. If the load event on the window object has not fired yet, or is
+          currently being fired, save the provided options and delay further


You should use the variable name. "|options|". Also save it where?

npm1 · 2022-10-11T16:29:26Z

spec/index.bs

+          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. Otherwise, enqueue a microtask and delay further processing until that


Links missing here too

npm1 · 2022-10-11T16:29:56Z

spec/index.bs

+          other. This and the next step make this possible.
+      1. Otherwise, enqueue a microtask and delay further processing until that
+          microtask.
+      1. When resuming processing, the saved options from all calls to get are


The scope of the saving here is unclear since it's not clear where this is being saved...

npm1 · 2022-10-11T16:31:01Z

spec/index.bs

+          other. This and the next step make this possible.
+      1. Otherwise, enqueue a microtask and delay further processing until that
+          microtask.
+      1. When resuming processing, the saved options from all calls to get are


Should probably be a new function

cbiesinger

Please take another look!

cbiesinger · 2022-10-14T19:18:49Z

spec/index.bs

@@ -1112,6 +1112,24 @@ 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 providing consent to share the [=IDP=] account information with the [=RP=].
+      1. If the load event on the window object has not fired yet, or is


cbiesinger · 2022-10-14T19:18:53Z

spec/index.bs

@@ -1112,6 +1112,24 @@ 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 providing consent to share the [=IDP=] account information with the [=RP=].
+      1. If the load event on the window object has not fired yet, or is


cbiesinger · 2022-10-14T19:19:00Z

spec/index.bs

@@ -1112,6 +1112,24 @@ 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 providing consent to share the [=IDP=] account information with the [=RP=].
+      1. If the load event on the window object has not fired yet, or is
+          currently being fired, save the provided options and delay further


cbiesinger · 2022-10-14T19:19:09Z

spec/index.bs

+          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. Otherwise, enqueue a microtask and delay further processing until that


cbiesinger · 2022-10-14T19:19:13Z

spec/index.bs

+          other. This and the next step make this possible.
+      1. Otherwise, enqueue a microtask and delay further processing until that
+          microtask.
+      1. When resuming processing, the saved options from all calls to get are


cbiesinger · 2022-10-14T19:34:54Z

spec/index.bs

+          other. This and the next step make this possible.
+      1. Otherwise, enqueue a microtask and delay further processing until that
+          microtask.
+      1. When resuming processing, the saved options from all calls to get are


samuelgoto · 2022-10-11T23:07:42Z

spec/index.bs

+            promises are rejected.
+        1. All AbortSignals are saved and if any is aborted, all further
+            processing is aborted and the account chooser, if any, is closed.
+            (TODO: What is the desirable behavior here?)
      1. Let |provider| be |options|["{{CredentialRequestOptions/identity}}"]["{{IdentityCredentialRequestOptions/providers}}"][0].


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

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?

samuelgoto · 2022-10-15T00:05:03Z

spec/index.bs

+          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


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?

For a): See the paragraph starting at line 1119

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

cbiesinger

Sam, please see my responses below.

cbiesinger · 2022-10-17T19:25:59Z

spec/index.bs

+            promises are rejected.
+        1. All AbortSignals are saved and if any is aborted, all further
+            processing is aborted and the account chooser, if any, is closed.
+            (TODO: What is the desirable behavior here?)
      1. Let |provider| be |options|["{{CredentialRequestOptions/identity}}"]["{{IdentityCredentialRequestOptions/providers}}"][0].


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?

cbiesinger · 2022-10-17T19:26:45Z

spec/index.bs

+          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


For a): See the paragraph starting at line 1119

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

npm1

Hmm thinking about this more, it might actually be better to have a section for multiIDP that monkey patches the main sections because we are not fully confident on the design here yet and because Mozilla may want to implement the single IDP case like us for now, so having this in the main algorithm might cause some confusion.

npm1 · 2022-10-18T16:17:10Z

spec/index.bs

@@ -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]]}}


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"

npm1 · 2022-10-18T16:17:51Z

spec/index.bs

+          and {{Promise}} objects.
+          This list is reused for all {{Credential/[[DiscoverFromExternalSource]]}}
+          calls within the same global object.
+      1. Create a new promise and append (|options|, new promise) to |all_options|.


Might be clearer to name the new promise some var.

npm1 · 2022-10-18T16:20:04Z

spec/index.bs

+        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


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

npm1 · 2022-10-18T16:20:20Z

spec/index.bs

+          This list is reused for all {{Credential/[[DiscoverFromExternalSource]]}}
+          calls within the same global object.
+      1. Create a new promise and append (|options|, new promise) to |all_options|.
+      1. If a load event listener (next step) has already been registered or


Could use a boolean to track this more formally.

npm1 · 2022-10-18T16:25:36Z

spec/index.bs

+          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].


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.

cbiesinger · 2022-10-19T19:39:36Z

I made #368 to support multi IDP in general, as a prerequisite for this PR.

samuelgoto · 2022-10-19T22:02:59Z

Two high level comments:

can we add a section with use cases / requirements (e.g. a table with "with vs without gesture" vs "single vs multiple")
can we add examples for each of these cases?
should we introduce a "mediation" flag to make these use cases more explicit?

I made #368 to support multi IDP in general, as a prerequisite for this PR.

I don't know if we should submit this PR right now.

npm1 · 2024-01-16T21:02:27Z

This seems outdated and I am trying to clean up open pull requests so I'm closing. If you still wanted to work on this feel free to reopen.

cbiesinger requested review from npm1 and samuelgoto September 23, 2022 22:02

npm1 reviewed Sep 26, 2022

View reviewed changes

samuelgoto mentioned this pull request Sep 26, 2022

Allow multiple IDPs to be used #319

Open

cbiesinger force-pushed the merge branch 3 times, most recently from cb0e7de to edd7af8 Compare October 7, 2022 21:54

samuelgoto reviewed Oct 11, 2022

View reviewed changes

npm1 reviewed Oct 11, 2022

View reviewed changes

Document merging get() calls for multiple IDP usage

0a2d80a

cbiesinger force-pushed the merge branch from edd7af8 to 0a2d80a Compare October 14, 2022 19:47

cbiesinger commented Oct 14, 2022

View reviewed changes

cbiesinger mentioned this pull request Oct 14, 2022

Export the definition for "completely loaded" whatwg/html#8382

Closed

3 tasks

samuelgoto reviewed Oct 15, 2022

View reviewed changes

cbiesinger commented Oct 17, 2022

View reviewed changes

npm1 reviewed Oct 18, 2022

View reviewed changes

npm1 closed this Jan 16, 2024

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

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

Conversation

cbiesinger commented Sep 23, 2022 • edited by pr-preview bot Loading

npm1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbiesinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbiesinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npm1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbiesinger commented Oct 19, 2022

samuelgoto commented Oct 19, 2022

npm1 commented Jan 16, 2024

cbiesinger commented Sep 23, 2022 •

edited by pr-preview bot

Loading