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 13 commits into
base: main
Choose a base branch
from
Open

Specify the mode API #660

wants to merge 13 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
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
request where |mode| is [=passive=], cancel the previous request
as if a {{CredentialRequestOptions/signal}} of
[=AbortSignal/aborted=] was given to it.
1. Let |globalObject| be the [=current global object=].
Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont think there is a current global object while running in parallel? Why dont you just use the globalObject that is passed as a parameter to this algorithm?

as if a {{CredentialRequestOptions/signal}} of
[=AbortSignal/aborted=] was given to it.
1. Let |globalObject| be the [=current global object=].
1. Let |W| be |globalObject|'s [=associated Window=].
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can be defined inside the active mode if since only used there?

1. Let |W| be |globalObject|'s [=associated Window=].
1. If |mode| is [=active=]:
1. If |W| does not have [=transient activation=], return (failure, true).
1. Otherwise, and if there is a pending request on |W|'s top-level browsing
Copy link
Collaborator

Choose a reason for hiding this comment

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

the top-level browsing context needs a link (and not sure if it is directly from Window or need to use Document). Also this is not clear, right now it makes it sound like we just check if top-level has a pending request whereas we would need to check all frames nested in that top-level. Can you rephrase this to make it clear?

1. If |mode| is [=active=]:
1. If |W| does not have [=transient activation=], return (failure, true).
1. Otherwise, and if there is a pending request on |W|'s top-level browsing
context where |mode| is [=passive=], reject the previous request.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to say specifically what error is used for the rejection in this case, no?

[=fetch the config file=] and [=show an IDP login dialog=].

* If the user triggers this affordance:
1. Let |config| be the result of running [=fetch the config file=]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if it would be better to encapsulate this in a helper function so as not to rewrite it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
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