Replace the 60 second timeout with a random delay #453

cbiesinger · 2023-04-12T18:35:10Z

This is a better user experience and also gives user agents mores
flexibility for their UI. It also makes the spec match Chrome's current
implementation.

This change does not add a delay when the user closes the dialog
explicitly. This is not necessary because the timing is effectively
random and therefore indistinguishable from other failures that do
have this delay, and it allows RPs to fall back to different types
of authentication immediately in this case.

Closes #389

Preview | Diff

cbiesinger · 2023-04-12T18:35:33Z

@RByers @samuelgoto @yi-gu

cbiesinger · 2023-04-12T18:38:27Z

also cc @martinthomson, @bvandersloot-mozilla and @cboozar

spec/index.bs

RByers · 2023-04-12T19:01:55Z

FWIW Christian and I chatted about this and I think the principle here should be that zero-account failures should be indistinguishable from a user not interacting with the UI.

npm1 · 2023-04-17T17:50:03Z

spec/index.bs

@@ -484,6 +475,11 @@ failure.
    1. If |config| is failure, return failure.
    1. Let |accountsList| be the result of [=fetch the accounts list=] with |config|, |provider|,
        and |globalObject|.
+    1. If |accountsList|'s size is 0 or if the fetch threw an exception:
+        1. Stop processing the algorithm and wait indefinitely.


One issue I see with this is that we do not allow new FedCM calls when there is a pending call. So this effectively means that the FedCM API cannot be invoked again in this page load. Other than that, this might be ok though it seems to require an implementation change on the Chromium side, right? Also, would we need to note somewhere that this would also be the behavior when the user closes/dismisses the user agent UI?

Good point, and IMHO one that gets at the heart of why I think it's good to move the spec in this direction. Imagine an implementation which chooses to suppress or show very quiet UI when FedCM is invoked without a user gesture, but will show noisier UI when invoked in response to eg. a "login with X" button click. Such an implementation needs a way to cancel the first request and show the second (possibly with <60 seconds between them), right?

We do have a way to cancel the UI: https://w3c.github.io/webappsec-credential-management/#dom-credentialrequestoptions-signal

Ah, of course. So is it OK to leave aborting current requests up to the page, or should we auto-abort when a new request comes in? In particular, thinking about multi-IDP scenarios with two un-cooperating SDKs on the same page. Presumably the one with a user gesture should take priority over one without (regardless of whether it's before or after), right?

That's a good question, hmm

Currently the spec requires us to fail with NotAllowedErr (step 5 in https://w3c.github.io/webappsec-credential-management/#algorithm-store) and payments also fails a second request if the first one is not done yet

So for the hypothetical scenario where the page uses a FedCM call during the page load and then there is a click which triggers another FedCM call, we would have two options:

Remove the UI from the first FedCM call (possibly reject the promise, or just leave it unresolved) and initiate the second call. This has the advantage that there is no need to manually abort the first FedCM call.

Force the developer to abort the first FedCM call manually, otherwise reject the second call (this is the current behavior). This has the advantage that we don't have to guess which FedCM calls should take priority, but does seem to require a bit more work from the RP. It also assumes the IDP does provide an abort method to cancel the call.

I have updated the PR to not have the indefinite timeout. Please take another look. Also interested in thoughts from others (@martinthomson / @cboozar ?)

spec/index.bs

npm1 · 2023-04-18T18:00:44Z

spec/index.bs

@@ -484,6 +475,11 @@ failure.
    1. If |config| is failure, return failure.
    1. Let |accountsList| be the result of [=fetch the accounts list=] with |config|, |provider|,
        and |globalObject|.
+    1. If |accountsList|'s size is 0 or if the fetch threw an exception:
+        1. Stop processing the algorithm and wait indefinitely.


So for the hypothetical scenario where the page uses a FedCM call during the page load and then there is a click which triggers another FedCM call, we would have two options:

Remove the UI from the first FedCM call (possibly reject the promise, or just leave it unresolved) and initiate the second call. This has the advantage that there is no need to manually abort the first FedCM call.

Force the developer to abort the first FedCM call manually, otherwise reject the second call (this is the current behavior). This has the advantage that we don't have to guess which FedCM calls should take priority, but does seem to require a bit more work from the RP. It also assumes the IDP does provide an abort method to cancel the call.

spec/index.bs

As-is, the spec does not correctly handle fetch errors; this change ensures that we correctly turn them into exceptions. I am keeping the name of the processResponseConsumeBody parameter even though it is no longer directly passed to the fetch algorithm because conceptually this is a wrapper around fetch and the parameter does have the same meaning as in the fetch algorithm. This was split out of PR w3c-fedid#453

cbiesinger

I am splitting the relatively uncontroversial change to the fetch error handling into PR #455 so that this PR can focus on the timeout handling.

I will also update this PR to what Chrome currently implements which should be less controversial than this PR's current proposal.

spec/index.bs

The extract and convert steps can throw exceptions, but fetch's processResponseConsumeBody does not really handle that. Instead, set the respective return objects to failure and update the call site where necessary. This was split out of PR w3c-fedid#453 with major modifications.

* Correctly handle fetch errors The extract and convert steps can throw exceptions, but fetch's processResponseConsumeBody does not really handle that. Instead, set the respective return objects to failure and update the call site where necessary. This was split out of PR #453 with major modifications. * review comments

This is a better user experience and also gives user agents mores flexibility for their UI. It also makes the spec match Chrome's current implementation. This change does not add a delay when the user closes the dialog explicitly. This is not necessary because the timing is effectively random and therefore indistinguishable from other failures that do have this delay, and it allows RPs to fall back to different types of authentication immediately in this case. Closes w3c-fedid#389

spec/index.bs

cbiesinger · 2023-04-20T18:22:32Z

Please take another look!

npm1 · 2023-04-21T13:43:42Z

spec/index.bs

+        1. Let |throwImmediately| be the value of the second element of the pair.
+        1. [=Queue a global task=] on the [=DOM manipulation task source=]
+            to throw a new "{{NetworkError}}" {{DOMException}}. If |throwImmediately| is true, this
+            MUST happen after a random delay between 0 and 60 seconds.


A couple of comments:

I think it is a bit clearer to add the wait before the queue a global task step, otherwise. this sounds like this delay could happen in main thread! Something like 1. If |throwImmediately| is true, wait ... 1. Queue a global task

Do we need to hardcode the 60 seconds here? In fact, does the delay here need to be mandatory? I can imagine user agent UI such that it only attempts fetching once the user has interacted enough with the dialog, and may thus not need this delay. Perhaps making this wait optional and explaining the purpose is a better approach?

Done, but let me know if you want this less prescriptive

spec/index.bs

npm1

Looks good! We should get someone from Mozilla to take a look before merging though...

spec/index.bs

cbiesinger · 2023-04-21T21:31:21Z

@martinthomson, @bvandersloot-mozilla and @cboozar -- I'd love to hear your thoughts on this PR

spec/index.bs

cboozar · 2023-05-09T07:52:04Z

Sorry for the late reply, these changes look good to me 👍

npm1 · 2023-05-09T17:26:00Z

Thanks for the review! Merging.

SHA: c8609ec Reason: push, by npm1 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

cbiesinger requested a review from samuelgoto April 12, 2023 18:35

yi-gu reviewed Apr 12, 2023

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

cbiesinger force-pushed the timeout branch from 0afc3ac to d2e132b Compare April 12, 2023 18:51

npm1 reviewed Apr 17, 2023

View reviewed changes

cbiesinger force-pushed the timeout branch from d2e132b to 24f09cf Compare April 17, 2023 22:07

npm1 reviewed Apr 18, 2023

View reviewed changes

cbiesinger mentioned this pull request Apr 19, 2023

Correctly handle fetch errors #455

Merged

cbiesinger commented Apr 19, 2023

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

spec/index.bs Outdated Show resolved Hide resolved

spec/index.bs Outdated Show resolved Hide resolved

cbiesinger force-pushed the timeout branch from 24f09cf to b844d8e Compare April 19, 2023 20:20

cbiesinger changed the title ~~Replace the 60 second timeout with an unresolved promise~~ Replace the 60 second timeout with a random delay Apr 19, 2023

npm1 reviewed Apr 19, 2023

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

spec/index.bs Outdated Show resolved Hide resolved

spec/index.bs Outdated Show resolved Hide resolved

spec/index.bs Outdated Show resolved Hide resolved

pair

960f21c

npm1 reviewed Apr 21, 2023

View reviewed changes

review comments

2b1e312

npm1 reviewed Apr 21, 2023

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

add note

769392a

npm1 approved these changes Apr 21, 2023

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

spec/index.bs Outdated Show resolved Hide resolved

npm1 added the agenda+ Regular CG meeting agenda items label Apr 21, 2023

remove 60s

9f69e2e

cbiesinger mentioned this pull request May 8, 2023

Add WebDriver commands #465

Merged

Merge remote-tracking branch 'upstream/main' into timeout

ae94804

npm1 merged commit c8609ec into w3c-fedid:main May 9, 2023

cbiesinger deleted the timeout branch May 9, 2023 17:27

github-actions bot added a commit that referenced this pull request May 9, 2023

Replace the 60 second timeout with a random delay (#453)

18faecd

SHA: c8609ec Reason: push, by npm1 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

cbiesinger removed the agenda+ Regular CG meeting agenda items label May 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace the 60 second timeout with a random delay #453

Replace the 60 second timeout with a random delay #453

cbiesinger commented Apr 12, 2023 •

edited by pr-preview bot

Loading

cbiesinger commented Apr 12, 2023

cbiesinger commented Apr 12, 2023

RByers commented Apr 12, 2023

npm1 Apr 17, 2023

RByers Apr 17, 2023

cbiesinger Apr 17, 2023

RByers Apr 17, 2023

cbiesinger Apr 17, 2023

npm1 Apr 18, 2023

cbiesinger Apr 19, 2023

npm1 Apr 18, 2023

cbiesinger left a comment

cbiesinger commented Apr 20, 2023

npm1 Apr 21, 2023

cbiesinger Apr 21, 2023

npm1 left a comment

cbiesinger commented Apr 21, 2023

cboozar commented May 9, 2023

npm1 commented May 9, 2023

Replace the 60 second timeout with a random delay #453

Replace the 60 second timeout with a random delay #453

Conversation

cbiesinger commented Apr 12, 2023 • edited by pr-preview bot Loading

cbiesinger commented Apr 12, 2023

cbiesinger commented Apr 12, 2023

RByers commented Apr 12, 2023

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

cbiesinger commented Apr 20, 2023

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

cbiesinger commented Apr 21, 2023

cboozar commented May 9, 2023

npm1 commented May 9, 2023

cbiesinger commented Apr 12, 2023 •

edited by pr-preview bot

Loading