Add the IDP sign-in status API to the spec #436

cbiesinger · 2023-02-14T20:26:00Z

Explainer: https://github.com/fedidcg/FedCM/blob/main/proposals/idp-sign-in-status-api.md

Fixes #430, #447

Preview | Diff

Explainer: https://github.com/fedidcg/FedCM/blob/main/proposals/idp-sign-in-status-api.md

cbiesinger · 2023-02-14T20:26:31Z

@bvandersloot-mozilla , can you take a look and see if this makes sense?

samuelgoto

Looks roughly right to me. Just some minor editorial suggestions, but the algorithm seems right.

spec/index.bs

martinthomson

State clearing?

npm1 · 2023-02-14T23:00:01Z

spec/index.bs

+"unknown", "signed-in", and "signed-out".
+
+The user agent checks all page and subresource loads for an HTTP response
+header named `IdP-SignIn-Status` containing [[RFC9110#parameter|Parameter]]s.


I think header response names need to be defined. Other than that this checking of all headers in everything sounds like something that needs to be integrated into the Fetch spec (this is a monkeypatch). Perhaps note that as an issue.

If I understood you correctly, done (both parts).

npm1 · 2023-02-14T23:00:47Z

spec/index.bs

+
+The user agent checks all page and subresource loads for an HTTP response
+header named `IdP-SignIn-Status` containing [[RFC9110#parameter|Parameter]]s.
+If that header exists and the first parameter has a name of `action`,


Presumably this needs to be parsed? Can we perhaps encapsulate this in an algorithm

I tried to clarify my intention here (the parsing is defined by the HTTP spec, is what I was trying to say)

npm1 · 2023-02-14T23:01:56Z

spec/index.bs

+the user agent must process it as follows:
+
+* If this is a subresource request, and the subresource does not have the
+    same [=/origin] as the toplevel page, ignore the header.


What do you mean by the origin of the subresource? Presumably the final origin? I think this needs to be pulled from the response itself (I imagine the request may only have the initial origin)

Done, I think, unless you had something else in mind for the wording?

spec/index.bs

bvandersloot-mozilla · 2023-02-15T16:51:01Z

spec/index.bs

+        with the key being the origin of |provider|'s configURL. If there is
+        no such entry, set it to a user-agent specific default (either "unknown" or
+        "signed-out").
+    1. If |signinStatus| is "signed-out", return failure.


Am I following it right that if a user is "signed out", any use of the API fails without a dialog?

Yes, just like (today) the API fails without a dialog if the user is not logged in.

It would be nice to have a way to cleanly recover from a signed-out state without having to navigate to the identity provider independently. This has something to do with how to integrate this with the Multi-IDP patch, since I think their integration is non-trivial.

I added a "Issue:" for now. I think the multi-IDP PR is maybe a better place to discuss how to integrate these two?

bvandersloot-mozilla · 2023-02-15T16:52:13Z

spec/index.bs

+    1. If the fetch failed, or the size of |accountsList| is 0:
+        1. Add (or replace) an entry in the [=IDP sign-in status map=] with the
+            key being the origin of the configURL and the value being "signed-out".
+        1. If |signinStatus| is "signed-in", show a dialog to the user and return


does this mean that the sign-in status map and the actual state is out of sync at this point?

The previous step should ensure they are in sync, right?

I was confused because showing a sign in link in the dialog when the status was signed-in is non-intuitive to me. I think this makes sense.

I reworded the note a bit to hopefully make this more clear.

bvandersloot-mozilla · 2023-02-15T16:53:39Z

@cbiesinger - what happened to the Javascript API?

cbiesinger

Martin: I added text that user agents should clear the sign-in state data when cookies or site data is cleared, is that what you meant? If not, please elaborate a bit on your concern

Ben: I was going to do that one separately, do you prefer if I add it to this PR?

spec/index.bs

cbiesinger · 2023-02-15T17:36:10Z

spec/index.bs

+"unknown", "signed-in", and "signed-out".
+
+The user agent checks all page and subresource loads for an HTTP response
+header named `IdP-SignIn-Status` containing [[RFC9110#parameter|Parameter]]s.


If I understood you correctly, done (both parts).

cbiesinger · 2023-02-15T18:30:38Z

spec/index.bs

+
+The user agent checks all page and subresource loads for an HTTP response
+header named `IdP-SignIn-Status` containing [[RFC9110#parameter|Parameter]]s.
+If that header exists and the first parameter has a name of `action`,


I tried to clarify my intention here (the parsing is defined by the HTTP spec, is what I was trying to say)

cbiesinger · 2023-02-15T18:51:06Z

spec/index.bs

+    1. If the fetch failed, or the size of |accountsList| is 0:
+        1. Add (or replace) an entry in the [=IDP sign-in status map=] with the
+            key being the origin of the configURL and the value being "signed-out".
+        1. If |signinStatus| is "signed-in", show a dialog to the user and return


The previous step should ensure they are in sync, right?

spec/index.bs

cbiesinger · 2023-02-15T18:53:32Z

spec/index.bs

+the user agent must process it as follows:
+
+* If this is a subresource request, and the subresource does not have the
+    same [=/origin] as the toplevel page, ignore the header.


Done, I think, unless you had something else in mind for the wording?

spec/index.bs

bvandersloot-mozilla · 2023-02-15T19:46:26Z

Ben: I was going to do that one separately, do you prefer if I add it to this PR?

Yes, that would be great!

bvandersloot-mozilla · 2023-02-15T20:18:30Z

I think this has potential conflicts (conceptual and textual) with #438 . Do you have an expectation on how that will resolve?

This also ties into this comment where I want to have an optional IDP chooser interface. If a user selects a "signed-out" IDP there, it would be nice to be able to spec in an IDP log-in flow.

In isolation, I think this patch makes sense though!

cbiesinger · 2023-02-15T20:22:06Z

I've added the JS API, please take a look!

At a high-level, I think integration with #438 would happen by showing a "sign in" link for each IDP that we have a signed out (or mismatch) status for. I am not sure how to best handle the case when the user is signed out of all IDPs, since the API design so far assumes that no dialog would be shown in such a case.

I'm actually not sure how prescriptive the spec should be with regards to what a sign-in flow should look like, but this PR does add a signin_url field to the config manifest which can be used for that purpose.

martinthomson · 2023-02-15T22:13:21Z

spec/index.bs

+User agents must also clear the [=IDP Sign-in Status map=] when the user clears
+cookies or stored site data.


This isn't sufficiently precise for me. Yes, if you nuke all state, this needs to go too, but there are other events that cause state for an origin to be removed. What if only the IdP clears state? What if only the RP clears state?

I elaborated a bit. Not sure I understand the concern about RP state clearing especially but please let me know if the additional text addresses your concern?

martinthomson · 2023-02-17T03:08:09Z

spec/index.bs

+Note: Website-initiated cookie changes should not affect this map. When
+    IDP sign-in state changes, it should send an explicit


I don't think that this is true: https://w3c.github.io/webappsec-clear-site-data/

I have added a section on interactions with Clear-Site-Data.

spec/index.bs

martinthomson · 2023-02-17T03:24:37Z

spec/index.bs

+    ::  The user agent must remote all entries that would be affected
+            by the deleted cookies.


This is the part that I think needs to be clearer. It might even need some more thought.

When you clear state for an RP, you probably just forget those identities (and the associated IdPs) that were used.

However, when you clear state for an IdP, there will be a bunch of RPs that have received identity information from the IdP. I see a two options, each with drawbacks.

You could clear ALL state for any RPs as well on the basis that retaining state at the RP might lead to information the RP has could propagate to the IdP, re-establishing same-site recognition when it shouldn't (see below for an attack on this). The drawback here is that clearing state in one place has an wider effect than users might expect.

You could clear just the login state, so that the RP would need to ask again. The attack here is that, if the user chooses to use the IdP again, the RP could remind the IdP of the user identity. The drawback is that now clearing state for the IdP ends up having unintended consequences. And though the reminding thing is a real problem, that only happens if the user gives permission to the RP to use the IdP again AND the RP state is not cleared AND the user intends to have a clean break. I don't believe that there is anything in FedCM that might leak the information prior to the user performing that grant (with the usual caveats about the effectiveness of cross-site information leakage protections).

Note that in the first case, if you allow for the possibility that each RP might also have established links to other IdPs, then you might also need to clear state for any of those RPs as well. It's a connected graph and the only way to make clean break is to burn out all the links. Of course, that might be even more surprising. At some point though, you need to recognize that this sort of purging needs to consider all forms of linkability, so you might want to stop before the only action you permit involves clearing all browser state.

I don't think that there is an easy cut to make between these two. I can see how different browsers might choose to take different approaches on the basis that they take different perspectives on how to empower their users. One option favours user control over privacy, the other favours convenience. For a specification, we generally try to document the considerations carefully and clearly, then leave these decisions to implementations. It is OK to do that because there are no guarantees on retention of site-level state, just as there are no firm rules about how private information is managed, so each browser gets to choose their own approach.

Note that I'm talking about a problem with broader scope than your change. You're the lucky one to have to contend with this.

Hmm, the more I think on this, the more I realize that this cuts in both directions. The only thing that might make the RP clearing case simpler is that we have a different perception of the IdP role in this process.

If the user deliberately chooses to reuse an identity, then the connection is remade, which is essentially an explicit request to re-establish state.

If the user chooses a different identity, they may or may not have an expectation that the IdP not share the fact that this RP previously had access to the previous identity.

For the latter, we already rely fairly heavily on the user making a trust decision with respect to the information the IdP might have about them. That is not a symmetric relationship with the RP. We don't assume that the user trusts the RP in the same way.

(Lots of stuff to write...)

I have elaborated on the line you commented on to describe the intent I had.

As you say, I think a lot of what you brought up here is only tangentially related to the IDP signin status API (this PR) and applies to FedCM in general.

However, the way I think about it is that the connection between RP and IDP is more akin to a permission. Permissions in general don't get deleted when the user clears cookies... To be honest, I think it would be surprising to users if deleting (e.g.) google.com cookies logged them out of (e.g.) Pinterest and Tripit?

I think I'm going to file a github issue to discuss all this, because it should probably involve not just you and me :)

I filed #493

It seems newer bikeshed versions show this error: LINE 681:91: Missing attribute value. Fix by using <var ignore> instead of <var ignore=>.

npm1

Some other places where signin/signup is used (no strong opinion on which is correct but figured I would flag:

Lots of text (intro and first sections) use signin instead of login (though this is probably fine)
"Request permission to sign-up" (not webexposed, so probably ok?)
Section 5.5 loginState is set to "SignIn" or "SignUp" (this is returned in the method)

spec/index.bs

npm1 · 2023-09-28T19:46:20Z

spec/index.bs

@@ -712,17 +711,19 @@ the exception thrown.
                impossible by always showing UI of some kind when credentials
                were sent to the server.

-            * If the user closes the dialog, return (failure, true).
+            1. Wait until one of the following occurs:


Does this need to be a substep of the mismatch dialog step? Or can it just be step 3 (the step after)

Yes, it needs to be a substep, because all of this only happens if loginStatus is logged-in.

cbiesinger

I did not want to update section 5.5 (webdriver accountlist command) because:

That is unrelated to IDP signin status
We've shipped those for a while
Changing this also requires changing Selenium (https://github.com/SeleniumHQ/selenium/blob/trunk/java/src/org/openqa/selenium/federatedcredentialmanagement/FederatedCredentialManagementAccount.java#L54)

spec/index.bs

cbiesinger · 2023-09-28T20:03:10Z

spec/index.bs

@@ -712,17 +711,19 @@ the exception thrown.
                impossible by always showing UI of some kind when credentials
                were sent to the server.

-            * If the user closes the dialog, return (failure, true).
+            1. Wait until one of the following occurs:


Yes, it needs to be a substep, because all of this only happens if loginStatus is logged-in.

Based on recent discussions in the fedidcg, we prefer Login over Signin. This CL changes the exposed commands accordingly. Note that the IDP signin feature is still in origin trial and thus subject to change. Additionally, the confirmidpsignin command is vendor-prefixed and is not shipped yet, so it should be safe to change: https://chromiumdash.appspot.com/commit/3362c0d9ff2667d26f37e04917489ec0eb4bc83f Spec PR: fedidcg/FedCM#436 Validate-Test-Flakiness: skip Low-Coverage-Reason: rename only, no functional changes Bug: 1451884 Change-Id: Ic3b3f3e144c6649b19f063258403c239e9ceac82 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4901302 Reviewed-by: Vladimir Nechaev <nechaev@chromium.org> Commit-Queue: Christian Biesinger <cbiesinger@chromium.org> Reviewed-by: Andrey Kosyakov <caseq@chromium.org> Cr-Commit-Position: refs/heads/main@{#1204075}

spec/index.bs

samuelgoto · 2023-10-02T23:56:52Z

spec/index.bs

@@ -559,21 +660,80 @@ To <dfn>create an IdentityCredential</dfn> given an {{IdentityProviderConfig}}
 or a pair (failure, bool), where the bool indicates whether to skip delaying
 the exception thrown.
    1. Assert: These steps are running [=in parallel=].
+    1. Let |loginStatus| be the value of the entry in [=Login Status map=]
+        with the key being the [=/origin=] of |provider|'s
+        {{IdentityProviderConfig/configURL}}. If there is no such entry, set it


Can we move the algorithm of "if there is no such entry return unknown" up to the Login Status section?

That seems like a useful thing for other APIs and othewrise the "unknown" value in the map section doesn't make any sense (since you can't set to "unkonwn?).

samuelgoto · 2023-10-02T23:59:11Z

spec/index.bs

+    1. If |loginStatus| is [=logged-out=], the user agent must do one of the following:
+
+        * Return (failure, false).
+        * Prompt the user whether to continue. If the user continues, the user


Is this so that we support button flows? Or is this strictly necessarily for dealing with the timing attack? If this is only to support button flows, can't we handle this case in a separate PR?

I added it so that the following flow works reasonably, which I think is what Firefox wants to do:

UA prompts user to select an IDP before it does anything

user clicks the IDP

IDP is logged out

UA should probably should show something in response to that user clicks?

This is inside the "create an IdentityCredential" algorithm: nothing happens before this, right? That is, "UA prompts user to select an IDP before it does anything" isn't captured before we get to this point, right?

What do you mean? The potential IDP selector is handled on (new) line 623, already existing in the spec. "create an IdentityCredential" is called by DiscoverFromExternalSource.

spec/index.bs

Based on recent discussions in the fedidcg, we prefer Login over Signin. This CL changes the exposed commands accordingly. Note that the IDP signin feature is still in origin trial and thus subject to change. Additionally, the confirmidpsignin command is vendor-prefixed and is not shipped yet, so it should be safe to change: https://chromiumdash.appspot.com/commit/c3773f834360675a1fffc8c147a8f9012b6b25af Spec PR: fedidcg/FedCM#436 Validate-Test-Flakiness: skip Low-Coverage-Reason: rename only, no functional changes Bug: 1451884 Change-Id: Ic3b3f3e144c6649b19f063258403c239e9ceac82 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4901302 Reviewed-by: Vladimir Nechaev <nechaev@chromium.org> Commit-Queue: Christian Biesinger <cbiesinger@chromium.org> Reviewed-by: Andrey Kosyakov <caseq@chromium.org> Cr-Commit-Position: refs/heads/main@{#1204075}

spec/index.bs

bvandersloot-mozilla · 2023-10-03T12:26:07Z

spec/index.bs

+
+<div algorithm="setStatus">
+When {{NavigatorLogin/setStatus()}} is called with argument |status|:
+1. If the [=/origin=] of the [=current settings object=] is not [=same origin=]


Do we care about A -embeds- B -embeds- A case?

I think you're right that we should since in that situation the inner A only gets partitioned (or no) cookies and thus is not aware of the "real" toplevel login state. I have updated this (and also the HTTP header part)

spec/index.bs

bvandersloot-mozilla · 2023-10-03T12:42:43Z

spec/index.bs

+<div algorithm>
+To <dfn>show an IDP login dialog</dfn> given an {{IdentityProviderAPIConfig}} |config|, run
+the following steps. This returns success or failure.
+    1. [=Create a fresh top-level traversable=] with URL


What if the URL is null?

I added a validation step to fetching the config file to ensure that signin_url is also valid here. I also think that this represents the desired semantics better (don't accept the config file if there is no login_url even if login_url is not triggered during this specific request)

bvandersloot-mozilla · 2023-10-03T12:44:41Z

spec/index.bs

@@ -1123,6 +1284,31 @@ and a |responseBody|, run the following steps. This returns an [=ordered map=].
    1. Return |json|.
 </div>

+<div algorithm>
+To <dfn>show an IDP login dialog</dfn> given an {{IdentityProviderAPIConfig}} |config|, run


This algorithm doesn't work for redirect login flows, correct?

It should work for same-origin redirects since all that is required is that the close() call (and the status setting) is from the same origin. Is there a use case for cross-origin redirect flows?

I meant for instances where the login dialog is in the same navigatable, and then navigate the user back to this page. I think I remember that being out of scope here.

I'm not positive I understand you correctly but I think that's out of scope yeah

I guess this refers to flow that require cross origin navigation, for technical reasons or cases with authentication intermediaries which are currently not properly covered by this

bvandersloot-mozilla · 2023-10-03T12:46:18Z

spec/index.bs

+        [=logged-out=]).
+    1. If |loginStatus| is [=logged-out=], the user agent must do one of the following:
+
+        * Return (failure, false).


Is this to allow UAs to reduce prompt spam?

bvandersloot-mozilla · 2023-10-03T13:01:06Z

spec/index.bs

+
+            1. Wait until one of the following occurs:
+
+                * If the user closes the dialog, return (failure, true).


Why does this return failure immediately?

This allows a user-visible read of the logged in bit.

Not sure I understand, since this requires the user to do something the timing is effectively random, indistinguishable from the delayed promise rejection?

Rejecting in a user-specified time is different than rejecting after a fixed 120s. So by seeing a rejection before that point, the IDP can determine they were not "logged-in".

Ah, as of c8609ec the 120s has been replaced with a random timer.

bvandersloot-mozilla · 2023-10-03T13:07:29Z

spec/index.bs

+                    1. If |result| is failure, return (failure, true). The user
+                        agent MAY show a dialog to the user before or after
+                        returning failure indicating this failure.
+                    1. Otherwise, go back to the [=fetch config step=]. As an


Let's pick one here.

cbiesinger

Comments addressed/responded to, please take another looo

spec/index.bs

cbiesinger · 2023-10-03T15:11:30Z

spec/index.bs

+        [=logged-out=]).
+    1. If |loginStatus| is [=logged-out=], the user agent must do one of the following:
+
+        * Return (failure, false).


cbiesinger · 2023-10-03T15:12:53Z

spec/index.bs

+    1. If |loginStatus| is [=logged-out=], the user agent must do one of the following:
+
+        * Return (failure, false).
+        * Prompt the user whether to continue. If the user continues, the user


I added it so that the following flow works reasonably, which I think is what Firefox wants to do:

UA prompts user to select an IDP before it does anything

user clicks the IDP

IDP is logged out

UA should probably should show something in response to that user clicks?

cbiesinger · 2023-10-03T15:14:02Z

spec/index.bs

+
+            1. Wait until one of the following occurs:
+
+                * If the user closes the dialog, return (failure, true).


Not sure I understand, since this requires the user to do something the timing is effectively random, indistinguishable from the delayed promise rejection?

cbiesinger · 2023-10-03T15:15:41Z

spec/index.bs

@@ -1123,6 +1284,31 @@ and a |responseBody|, run the following steps. This returns an [=ordered map=].
    1. Return |json|.
 </div>

+<div algorithm>
+To <dfn>show an IDP login dialog</dfn> given an {{IdentityProviderAPIConfig}} |config|, run


It should work for same-origin redirects since all that is required is that the close() call (and the status setting) is from the same origin. Is there a use case for cross-origin redirect flows?

spec/index.bs

cbiesinger · 2023-10-03T18:25:33Z

spec/index.bs

+                    1. If |result| is failure, return (failure, true). The user
+                        agent MAY show a dialog to the user before or after
+                        returning failure indicating this failure.
+                    1. Otherwise, go back to the [=fetch config step=]. As an


cbiesinger · 2023-10-03T18:38:08Z

spec/index.bs

+<div algorithm>
+To <dfn>show an IDP login dialog</dfn> given an {{IdentityProviderAPIConfig}} |config|, run
+the following steps. This returns success or failure.
+    1. [=Create a fresh top-level traversable=] with URL


I added a validation step to fetching the config file to ensure that signin_url is also valid here. I also think that this represents the desired semantics better (don't accept the config file if there is no login_url even if login_url is not triggered during this specific request)

spec/index.bs

cbiesinger · 2023-10-03T18:48:40Z

spec/index.bs

@@ -559,21 +660,80 @@ To <dfn>create an IdentityCredential</dfn> given an {{IdentityProviderConfig}}
 or a pair (failure, bool), where the bool indicates whether to skip delaying
 the exception thrown.
    1. Assert: These steps are running [=in parallel=].
+    1. Let |loginStatus| be the value of the entry in [=Login Status map=]
+        with the key being the [=/origin=] of |provider|'s
+        {{IdentityProviderConfig/configURL}}. If there is no such entry, set it


spec/index.bs

samuelgoto · 2023-10-04T22:40:17Z

spec/index.bs

+* Otherwise:
+    * Assert that |value| is a tuple.
+    * Let |token| be the first entry of |value|.
+    * If |token| is `logged-in`, [=map/set=] [=Login Status map=][|origin|]


Capture this in a "set" algorithm and use here instead?

I did not want to move the mapping of the HTTP token to the enum value to the algorithm, but I did add the set algorithm.

spec/index.bs

samuelgoto · 2023-10-04T22:44:11Z

spec/index.bs

+    1. If |loginStatus| is [=logged-out=], the user agent must do one of the following:
+
+        * Return (failure, false).
+        * Prompt the user whether to continue. If the user continues, the user


This is inside the "create an IdentityCredential" algorithm: nothing happens before this, right? That is, "UA prompts user to select an IDP before it does anything" isn't captured before we get to this point, right?

spec/index.bs

samuelgoto · 2023-10-10T23:52:57Z

spec/index.bs

+
+        * Return (failure, false).
+        * Prompt the user whether to continue. If the user continues, the user
+            agent SHOULD set |loginStatus| to [=unknown=]. This MAY include an


Suggested change

agent SHOULD set |loginStatus| to [=unknown=]. This MAY include an

agent SHOULD set |loginStatus| to [=unknown=]. This MUST include an

Isn't this a MUST? Otherwise, how can we move forward with the algorithm if the loginStatus is 'logged-out'?

Don't we also have to "wait" somewhere here until the user becomes 'logged-in'?

That happens if the user triggers the affordance to "show an IDP login dialog" and is encapsulated in that algorithm.

I don't think the SHOULD needs to be a MUST; the algorithm should work fine as-is, no? I left it that way because some user agents don't really want to use an unknown state.

samuelgoto

LGTM

npm1

Took another pass. Looks great! Some nits

npm1 · 2023-10-11T20:37:47Z

spec/index.bs

@@ -49,6 +49,7 @@ spec:html; type:dfn; for:environment settings object; text:global object
 spec:html; type:dfn; for:html-origin-def; text:origin
 spec:webidl; type:dfn; text:resolve
 spec:webdriver2; type:dfn; text:error
+spec:fetch; type:dfn; for:/; text:response


This should not be necessary... are you sure it's needed?

Yes, I got a warning from bikeshed without it (it was ambiguous. Bikeshed actually picked the right one, but it seemed better not to rely on that)

spec/index.bs

npm1 · 2023-10-11T20:42:17Z

spec/index.bs

+
+<div algorithm="process the login status header">
+* Let |origin| be the response's [=response/URL=]'s [=/origin=].
+* If |origin| is not [=same-origin with its ancestors=], ignore the header.


This invocation is incorrect. We are passing an origin but it takes an environment settings object.

spec/index.bs

npm1 · 2023-10-11T20:45:18Z

spec/index.bs

+* Otherwise:
+    * Assert that |value| is a tuple.
+    * Let |token| be the first entry of |value|.
+    * If |token| is `logged-in`, [=set the login status=] for |origin|


I think you need to use proper quotations for string, as this is a string comparison and I don't see quotes in the preview. ditto below.

Ah, I see now that tokens also get returned as strings, so you're right. Done.

npm1 · 2023-10-11T21:24:15Z

spec/index.bs

+                    1. Otherwise, go back to the [=fetch accounts list step=].
+
+    1. Assert: |accountsList| is not failure and the size of |accountsList| is not 0.
+    1. [=Set the login status=] for the [=/origin] of the


nit: [=/origin=]

You'd think bikeshed would warn me. Thanks, done.

npm1 · 2023-10-11T21:27:37Z

spec/index.bs

@@ -595,7 +761,7 @@ the exception thrown.
            [=compute the connection status=] algorithm given |provider| and |account|. When doing this,
            the user agent MAY show some UI to the user indicating that they are being
            <dfn>auto-reauthenticated</dfn>.
-    1. Otherwise, if |mediation| is "{{CredentialMediationRequirement/silent}}", return failure.
+    1. Otherwise, if |mediation| is "{{CredentialMediationRequirement/silent}}", return (failure, false).


In this case, we do not delay the rejection so (failure, true)

npm1 · 2023-10-11T21:29:44Z

spec/index.bs

@@ -765,6 +934,7 @@ dictionary IdentityProviderAPIConfig {
  required USVString accounts_endpoint;
  required USVString client_metadata_endpoint;
  required USVString id_assertion_endpoint;
+  USVString login_url;


Why is this not required like the other ones? Not that it matters since we don't really use this in JS but still

Good point, done.

npm1 · 2023-10-11T21:30:59Z

spec/index.bs

+        * Return (failure, false).
+        * Prompt the user whether to continue. If the user continues, the user
+            agent SHOULD set |loginStatus| to [=unknown=]. This MAY include an
+            affordance to [=show an IDP login dialog=]. If the user cancels


nit: this requires parameter

spec/index.bs

npm1 · 2023-10-11T21:39:19Z

spec/index.bs

+        * {{IdentityProvider}}.{{IdentityProvider/close}} is called in the
+            context of this new traversable:
+            1. Close the traversable.
+            1. Let |loginStatus| be the result of [=get the login status=]


Ah, this is a bit weird in the implementation. Because we do not know whether we process the header before the idp.close, we allow header processing to also work. But I guess the spec is sync enough that it's not possible...

I think the spec is conceptually better than the impl; maybe we can do things better in our impl...

cbiesinger

oops, just realized I did not publish my responses

cbiesinger · 2023-10-12T19:10:04Z

spec/index.bs

@@ -49,6 +49,7 @@ spec:html; type:dfn; for:environment settings object; text:global object
 spec:html; type:dfn; for:html-origin-def; text:origin
 spec:webidl; type:dfn; text:resolve
 spec:webdriver2; type:dfn; text:error
+spec:fetch; type:dfn; for:/; text:response


Yes, I got a warning from bikeshed without it (it was ambiguous. Bikeshed actually picked the right one, but it seemed better not to rely on that)

spec/index.bs

cbiesinger · 2023-10-12T19:22:31Z

spec/index.bs

+* Otherwise:
+    * Assert that |value| is a tuple.
+    * Let |token| be the first entry of |value|.
+    * If |token| is `logged-in`, [=set the login status=] for |origin|


Ah, I see now that tokens also get returned as strings, so you're right. Done.

spec/index.bs

cbiesinger · 2023-10-12T19:44:29Z

spec/index.bs

+User agents MUST also clear the [=Login Status map=] data when:
+    :   the user clears all cookies or site settings data
+    ::  The user agent MUST clear the entire map.
+    :   the user clears cookies or site data for a specific origin


the intent was:
if the user clears all cookies for the origin google.com, we also reset the signin status for accounts.google.com, because it is possible that the login state was relying on a domain cookie for .google.com

Tried to clarify this...

cbiesinger · 2023-10-12T20:00:34Z

spec/index.bs

+            Note: The user agent MAY want to reset the state to [=unknown=],
+                since is impossible to know whether this cookie affects
+                authorization state.
+    : the user agent receives a <a http-header>Clear-Site-Data</a> header with a


hmm. I found w3c/webappsec-clear-site-data#72... updated this wording.

cbiesinger · 2023-10-12T20:06:26Z

spec/index.bs

+        * Return (failure, false).
+        * Prompt the user whether to continue. If the user continues, the user
+            agent SHOULD set |loginStatus| to [=unknown=]. This MAY include an
+            affordance to [=show an IDP login dialog=]. If the user cancels


spec/index.bs

cbiesinger · 2023-10-12T20:26:33Z

spec/index.bs

+
+<div algorithm="process the login status header">
+* Let |origin| be the response's [=response/URL=]'s [=/origin=].
+* If |origin| is not [=same-origin with its ancestors=], ignore the header.


npm1 · 2023-10-13T15:19:50Z

spec/index.bs

-    * If |token| is `logged-in`, [=set the login status=] for |origin|
+1. Let |origin| be the response's [=response/URL=]'s [=/origin=].
+1. Let |client| be the [=/request=]'s [=request/client=].
+1. If the request is not a [=directly user-initiated request=]:


I don't think this is what you want, but rather request's destination = document https://fetch.spec.whatwg.org/#concept-request-destination.

Also the flow in this method is now broken, need to get rid of the "Otherwise".

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

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

Add the IDP sign-in status API to the spec

e2f48cc

Explainer: https://github.com/fedidcg/FedCM/blob/main/proposals/idp-sign-in-status-api.md

samuelgoto reviewed Feb 14, 2023

View reviewed changes

martinthomson suggested changes Feb 14, 2023

View reviewed changes

npm1 reviewed Feb 14, 2023

View reviewed changes

bvandersloot-mozilla reviewed Feb 15, 2023

View reviewed changes

cbiesinger commented Feb 15, 2023

View reviewed changes

Address comments

8304968

Add the JavaScript API

465c717

cbiesinger added 3 commits February 15, 2023 15:23

void->undefined and add an Issue comment regarding Promise

896e95d

Slightly more direct way to get the origin

07eb407

Add a multi-IDP note and clarify a signin note

8948f99

martinthomson reviewed Feb 15, 2023

View reviewed changes

cbiesinger mentioned this pull request Feb 16, 2023

A not-yet logged in IDP has no route to success with this flow #442

Open

bvandersloot-mozilla mentioned this pull request Feb 16, 2023

The IDP has to support additional infrastructure to support FedCM #441

Open

Elaborate on state clearing

f19c445

martinthomson reviewed Feb 17, 2023

View reviewed changes

cbiesinger mentioned this pull request Mar 22, 2023

Preventing silent timing attacks #447

Closed

samuelgoto mentioned this pull request Apr 12, 2023

Should FedCM use the Login Status API? privacycg/is-logged-in#53

Closed

yi-gu mentioned this pull request Apr 25, 2023

Returning users have to unnecessarily re-approve #429

Closed

cbiesinger added 5 commits August 11, 2023 16:00

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

663f421

Fix bikeshed syntax errors

090262d

It seems newer bikeshed versions show this error: LINE 681:91: Missing attribute value. Fix by using <var ignore> instead of <var ignore=>.

Merge branch 'syntax' into status2

d682586

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

9557220

define interaction with clear-site-data

0c154cb

npm1 approved these changes Sep 28, 2023

View reviewed changes

linkify

49fe2ad

cbiesinger commented Sep 28, 2023

View reviewed changes

samuelgoto reviewed Oct 3, 2023

View reviewed changes

bvandersloot-mozilla reviewed Oct 3, 2023

View reviewed changes

cbiesinger added 2 commits October 3, 2023 14:41

review comments

2133a0b

get a login status entry algorithm

d28201f

cbiesinger commented Oct 3, 2023

View reviewed changes

samuelgoto reviewed Oct 4, 2023

View reviewed changes

review comments

0d55b05

samuelgoto reviewed Oct 10, 2023

View reviewed changes

samuelgoto approved these changes Oct 10, 2023

View reviewed changes

npm1 reviewed Oct 11, 2023

View reviewed changes

review comments

60cdb08

cbiesinger commented Oct 13, 2023

View reviewed changes

bvandersloot-mozilla approved these changes Oct 13, 2023

View reviewed changes

npm1 reviewed Oct 13, 2023

View reviewed changes

last? comments

48e4d1a

npm1 approved these changes Oct 13, 2023

View reviewed changes

npm1 merged commit a49343e into fedidcg:main Oct 13, 2023
2 checks passed

cbiesinger deleted the status2 branch October 13, 2023 15:31

cbiesinger restored the status2 branch October 13, 2023 15:31

cbiesinger deleted the status2 branch October 13, 2023 15:31

cbiesinger restored the status2 branch October 13, 2023 15:31

cbiesinger deleted the status2 branch October 13, 2023 15:31

github-actions bot added a commit that referenced this pull request Oct 13, 2023

Add the IDP sign-in status API to the spec (#436)

38666b3

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

		User agents must also clear the [=IDP Sign-in Status map=] when the user clears
		cookies or stored site data.

		Note: Website-initiated cookie changes should not affect this map. When
		IDP sign-in state changes, it should send an explicit

		:: The user agent must remote all entries that would be affected
		by the deleted cookies.


		1. Wait until one of the following occurs:

		* If the user closes the dialog, return (failure, true).

	agent SHOULD set \|loginStatus\| to [=unknown=]. This MAY include an
	agent SHOULD set \|loginStatus\| to [=unknown=]. This MUST include an

Add the IDP sign-in status API to the spec #436

Add the IDP sign-in status API to the spec #436

Conversation

cbiesinger commented Feb 14, 2023 • edited by pr-preview bot Loading

cbiesinger commented Feb 14, 2023

samuelgoto left a comment

Choose a reason for hiding this comment

martinthomson 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

bvandersloot-mozilla commented Feb 15, 2023

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

bvandersloot-mozilla commented Feb 15, 2023 • edited Loading

bvandersloot-mozilla commented Feb 15, 2023

cbiesinger commented Feb 15, 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

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

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

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

cbiesinger commented Feb 14, 2023 •

edited by pr-preview bot

Loading

bvandersloot-mozilla commented Feb 15, 2023 •

edited

Loading