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.

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

Specify the mode API #660

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Specify the mode API #660

wants to merge 17 commits into from

Conversation

tttzach
Copy link
Collaborator

@tttzach tttzach commented Oct 2, 2024

spec/index.bs Outdated Show resolved Hide resolved
Copy link
Collaborator

@cbiesinger cbiesinger left a comment

Choose a reason for hiding this comment

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

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.

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
spec/index.bs Outdated Show resolved Hide resolved
Copy link
Contributor

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

Minor, human-facing

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
@tttzach tttzach changed the title Add mode for FedCM Specify the mode API Oct 4, 2024
Copy link
Contributor

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

trivial, editorial

spec/index.bs Outdated Show resolved Hide resolved
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Copy link
Collaborator

@cbiesinger cbiesinger left a comment

Choose a reason for hiding this comment

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

Thanks, looks good modulo two minor comments

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
Co-authored-by: Ted Thibodeau Jr <[email protected]>
@wseltzer
Copy link
Collaborator

wseltzer commented Oct 15, 2024

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

@tttzach
Copy link
Collaborator Author

tttzach commented Oct 15, 2024

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

spec/index.bs Outdated Show resolved Hide resolved
@philsmart
Copy link
Contributor

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.

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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 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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

spec/index.bs Outdated
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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
Copy link
Collaborator

@bvandersloot-mozilla bvandersloot-mozilla left a comment

Choose a reason for hiding this comment

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

why SHOULD?

@hlflanagan
Copy link
Contributor

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)

Copy link
Collaborator Author

@tttzach tttzach left a comment

Choose a reason for hiding this comment

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

Thanks for the review, please take another look!

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
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
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.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

spec/index.bs Outdated
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
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 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?

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
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
@tttzach tttzach requested a review from npm1 October 31, 2024 02:38
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
@tttzach tttzach requested a review from npm1 October 31, 2024 22:02
Copy link
Collaborator

@npm1 npm1 left a comment

Choose a reason for hiding this comment

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

Looks good!

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
cbiesinger added a commit to cbiesinger/WebID that referenced this pull request Nov 4, 2024
@tttzach
Copy link
Collaborator Author

tttzach commented Nov 5, 2024

@bvandersloot-mozilla Friendly ping :)

@yi-gu yi-gu added the agenda+ Regular CG meeting agenda items label Nov 6, 2024
@hlflanagan
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agenda+ Regular CG meeting agenda items
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A not-yet logged in IDP has no route to success with this flow
10 participants