Specify the mode API by tttzach · Pull Request #660 · w3c-fedid/FedCM

tttzach · 2024-10-02T19:35:32Z

cbiesinger

Generally looks good. See comments below. We should probably add a Note: in some appropriate place that the user agent should consider displaying the account chooser in a louder way if mode is active.

TallTed

Minor, human-facing

TallTed

trivial, editorial

Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>

cbiesinger

Thanks, looks good modulo two minor comments

Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>

wseltzer · 2024-10-15T15:21:49Z

Discussed in Oct. 15 call. [minutes to be linked]
https://github.com/fedidcg/meetings/blob/main/2024/2024-10-15-notes.md

tttzach · 2024-10-15T15:24:47Z

@aaronpk @bvandersloot-mozilla @philsmart Please take a look!

philsmart · 2024-10-15T16:47:30Z

From the perspective of at least the R&E federations I am involved with, 'active' mode is a better fit. This mode supports authentication with Identity Providers (IdPs) that the user has not yet logged into (as signalled by the login status API).

Supporting multiple IdPs in 'active' mode would be helpful, but it could result in the same account fetching issues as enabling multi-IdP in 'passive' mode. The 'passive' mode addressed this (if I recall correctly) by including only the IdPs you've signed into, which brings us back to the same issue (unable to use an IdP you've not logged into). This can be addressed for now by using an external IdP discovery service and populating FedCM with a single IdP in 'active' mode.

For what it's worth, I agree with renaming 'button' and 'widget' to 'active' and 'passive', it is clearer.

npm1 · 2024-10-21T17:21:14Z

+    1. Let |mode| be |options|'s {{IdentityCredentialRequestOptions/mode}}.
+    1. If |mode| is [=active=]
+        1. If [=transient activation=] is not present, return (failure, true).
+        1. If [=transient activation=] is present and there is a pending


This part about pending request is also not precise enough. Presumably this is also scoped to something (perhaps top-level browsing context?). In addition, you are never going to reach this because we disallow this from credential management in 8.2 https://w3c.github.io/webappsec-credential-management/#algorithm-request so that will need to be modified as well at some point

Good catch on the scoping... let me know if |W|'s top-level browsing context works or if that should be even more specific. Should modifying 8.2 be in this PR?

No, you cannot modify 8.2 in this PR since that is a separate repo. I also filed an issue because that uses the wrong scope as well. Need to think how to fix it while also allowing the active mode to be prioritized (eg supersede a pending passive mode request)

npm1 · 2024-10-21T17:21:40Z

+        1. If [=transient activation=] is present and there is a pending
+            request where |mode| is [=passive=], cancel the previous request
+            as if a {{CredentialRequestOptions/signal}} of
+            [=AbortSignal/aborted=] was given to it.


Hmm but isnt the abort signal only for the developer to use? This does not make sense to me, is this actually what we do?

No we don't actually use the abort signal for this case, but instead we directly reject the previous request. Though this was why its worded this way: #660 (comment)

Maybe we can get rid of as if a {{CredentialRequestOptions/signal}} of [=AbortSignal/aborted=] was given to it.?

bvandersloot-mozilla

why SHOULD?

hlflanagan · 2024-10-22T16:15:18Z

PR further discussed in https://github.com/w3c-fedid/meetings/blob/main/2024/2024-10-22-notes.md
(@bvandersloot-mozilla will review; the issue raised (w3c-fedid/active-mode#6) will be added to the text as a related problem that still needs to be solved)

tttzach

Thanks for the review, please take another look!

tttzach · 2024-10-23T16:27:36Z

+        1. If [=transient activation=] is present and there is a pending
+            request where |mode| is [=passive=], cancel the previous request
+            as if a {{CredentialRequestOptions/signal}} of
+            [=AbortSignal/aborted=] was given to it.


No we don't actually use the abort signal for this case, but instead we directly reject the previous request. Though this was why its worded this way: #660 (comment)

Maybe we can get rid of as if a {{CredentialRequestOptions/signal}} of [=AbortSignal/aborted=] was given to it.?

tttzach · 2024-10-23T16:28:21Z

+    1. Let |mode| be |options|'s {{IdentityCredentialRequestOptions/mode}}.
+    1. If |mode| is [=active=]
+        1. If [=transient activation=] is not present, return (failure, true).
+        1. If [=transient activation=] is present and there is a pending


Good catch on the scoping... let me know if |W|'s top-level browsing context works or if that should be even more specific. Should modifying 8.2 be in this PR?

npm1

Looks good!

This depends on PR w3c-fedid#660 (Mode API). Bug: w3c-fedid/active-mode#3

tttzach · 2024-11-05T20:57:58Z

@bvandersloot-mozilla Friendly ping :)

hlflanagan · 2024-11-14T19:20:06Z

Note: discussed during https://github.com/fedidcg/meetings/blob/main/2024/2024-11-12-notes.md ; PR blocked pending @bvandersloot-mozilla review.

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

* Specify "Use another account". This depends on PR #660 (Mode API). Bug: w3c-fedid/active-mode#3 * ted changes * npm comment * fix indentation * fix indentation * fix nit * fix indentation * oxford commas * single boolean * otherwise * Fix review comments * MUST->SHOULD Co-authored-by: bvandersloot-mozilla <90582190+bvandersloot-mozilla@users.noreply.github.com> --------- Co-authored-by: bvandersloot-mozilla <90582190+bvandersloot-mozilla@users.noreply.github.com>

Add mode for FedCM

ebc24fc

Fixes w3c-fedid/active-mode#2

samuelgoto reviewed Oct 2, 2024

View reviewed changes

Comment thread spec/index.bs Outdated

cbiesinger reviewed Oct 2, 2024

View reviewed changes

Comment thread spec/index.bs Outdated

Comment thread spec/index.bs Outdated

Comment thread spec/index.bs Outdated

Comment thread spec/index.bs Outdated

Comment thread spec/index.bs Outdated

TallTed suggested changes Oct 3, 2024

View reviewed changes

Comment thread spec/index.bs Outdated

Comment thread spec/index.bs Outdated

yi-gu reviewed Oct 3, 2024

View reviewed changes

Comment thread spec/index.bs

tttzach added 3 commits October 3, 2024 15:59

Update index.bs

4fd24da

Update index.bs

d34ecd0

Lint index.bs

ce8f11e

tttzach requested review from cbiesinger, samuelgoto and yi-gu October 3, 2024 22:55

tttzach changed the title ~~Add mode for FedCM~~ Specify the mode API Oct 4, 2024

TallTed suggested changes Oct 4, 2024

View reviewed changes

Comment thread spec/index.bs Outdated

Update spec/index.bs

2a28995

Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>

cbiesinger approved these changes Oct 7, 2024

View reviewed changes

Comment thread spec/index.bs Outdated

Comment thread spec/index.bs Outdated

Update index.bs

d8488b8

tttzach requested a review from bvandersloot-mozilla October 8, 2024 15:36

TallTed reviewed Oct 8, 2024

View reviewed changes

Comment thread spec/index.bs Outdated

Update spec/index.bs

1e0d0be

Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>

samuelgoto reviewed Oct 15, 2024

View reviewed changes

Comment thread spec/index.bs

samuelgoto reviewed Oct 15, 2024

View reviewed changes

Comment thread spec/index.bs Outdated

samuelgoto reviewed Oct 15, 2024

View reviewed changes

Comment thread spec/index.bs

tttzach added 4 commits October 15, 2024 12:53

Update index.bs

a477975

Update index.bs

c4c3d5b

Update index.bs

400ac7f

Update index.bs

cbaa2d2

yi-gu approved these changes Oct 18, 2024

View reviewed changes

cbiesinger approved these changes Oct 21, 2024

View reviewed changes

npm1 requested changes Oct 21, 2024

View reviewed changes

bvandersloot-mozilla requested changes Oct 22, 2024

View reviewed changes

tttzach added 2 commits October 23, 2024 11:41

Update index.bs

0247dd2

Update index.bs

71e585b

tttzach commented Oct 23, 2024

View reviewed changes

tttzach requested review from bvandersloot-mozilla and npm1 October 23, 2024 16:29

npm1 reviewed Oct 24, 2024

View reviewed changes

Comment thread spec/index.bs Outdated

Comment thread spec/index.bs Outdated

Comment thread spec/index.bs Outdated

Comment thread spec/index.bs Outdated

Comment thread spec/index.bs Outdated

tttzach added 2 commits October 30, 2024 22:31

Update index.bs

4bf692f

Update index.bs

e58507b

tttzach requested a review from npm1 October 31, 2024 02:38

npm1 reviewed Oct 31, 2024

View reviewed changes

Comment thread spec/index.bs Outdated

Comment thread spec/index.bs

npm1 reviewed Oct 31, 2024

View reviewed changes

Comment thread spec/index.bs Outdated

Comment thread spec/index.bs Outdated

Update index.bs

121f5f7

tttzach requested a review from npm1 October 31, 2024 22:02

npm1 approved these changes Nov 1, 2024

View reviewed changes

Comment thread spec/index.bs Outdated

Comment thread spec/index.bs Outdated

Update index.bs

494dd9c

cbiesinger added a commit to cbiesinger/WebID that referenced this pull request Nov 4, 2024

Specify "Use another account".

a191a15

This depends on PR w3c-fedid#660 (Mode API). Bug: w3c-fedid/active-mode#3

cbiesinger mentioned this pull request Nov 4, 2024

Specify "Use another account". #678

Merged

yi-gu added the agenda+ Regular CG meeting agenda items label Nov 6, 2024

bvandersloot-mozilla approved these changes Nov 18, 2024

View reviewed changes

npm1 merged commit e6ce288 into main Nov 18, 2024

npm1 deleted the tttzach-patch-1 branch November 18, 2024 17:01

github-actions Bot added a commit that referenced this pull request Nov 18, 2024

Specify the mode API (#660)

28d0f50

SHA: e6ce288 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 Mar 13, 2025

Uh oh!

Conversation

tttzach commented Oct 2, 2024 • edited by pr-preview Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cbiesinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TallTed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TallTed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cbiesinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wseltzer commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tttzach commented Oct 15, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

philsmart commented Oct 15, 2024

Uh oh!

Uh oh!

npm1 Oct 21, 2024

Choose a reason for hiding this comment

Uh oh!

tttzach Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

npm1 Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

npm1 Oct 21, 2024

Choose a reason for hiding this comment

Uh oh!

tttzach Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bvandersloot-mozilla left a comment

Choose a reason for hiding this comment

Uh oh!

hlflanagan commented Oct 22, 2024

Uh oh!

tttzach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tttzach Oct 23, 2024

Choose a reason for hiding this comment

tttzach commented Oct 2, 2024 •

edited by pr-preview Bot

Loading

wseltzer commented Oct 15, 2024 •

edited

Loading