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

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

Merged
merged 47 commits into from
Oct 13, 2023
Merged

Conversation

cbiesinger
Copy link
Collaborator

@cbiesinger cbiesinger commented Feb 14, 2023

@cbiesinger
Copy link
Collaborator Author

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

Copy link
Collaborator

@samuelgoto samuelgoto left a comment

Choose a reason for hiding this comment

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

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

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
Contributor

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

State clearing?

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

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

spec/index.bs Outdated

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`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

does this mean that the sign-in status map and the actual state is out of sync at this 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.

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

@bvandersloot-mozilla
Copy link
Collaborator

@cbiesinger - what happened to the Javascript API?

Copy link
Collaborator Author

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

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

Choose a reason for hiding this comment

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

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

spec/index.bs Outdated

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
@bvandersloot-mozilla
Copy link
Collaborator

bvandersloot-mozilla commented Feb 15, 2023

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

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

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.

spec/index.bs Outdated
Comment on lines 333 to 334
User agents must also clear the [=IDP Sign-in Status map=] when the user clears
cookies or stored site data.
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

spec/index.bs Outdated
Comment on lines 350 to 351
Note: Website-initiated cookie changes should not affect this map. When
IDP sign-in state changes, it should send an explicit
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated
Comment on lines 338 to 339
:: The user agent must remote all entries that would be affected
by the deleted cookies.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

  1. 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.
  2. 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I filed #493

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.

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 Outdated Show resolved Hide resolved
@@ -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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

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

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

  1. That is unrelated to IDP signin status
  2. We've shipped those for a while
  3. 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 Outdated Show resolved Hide resolved
@@ -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:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

aarongable pushed a commit to chromium/chromium that referenced this pull request Oct 2, 2023
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: w3c-fedid/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 Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
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
spec/index.bs Outdated
@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

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

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
T3-M4 pushed a commit to bayandin/chromedriver that referenced this pull request Oct 3, 2023
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: w3c-fedid/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 Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Outdated

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
<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
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if the URL is null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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)

@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

[=logged-out=]).
1. If |loginStatus| is [=logged-out=], the user agent must do one of the following:

* Return (failure, false).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this to allow UAs to reduce prompt spam?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes


1. Wait until one of the following occurs:

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

Choose a reason for hiding this comment

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

Why does this return failure immediately?

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Let's pick one here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator Author

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

Comments addressed/responded to, please take another looo

spec/index.bs Show resolved Hide resolved
[=logged-out=]).
1. If |loginStatus| is [=logged-out=], the user agent must do one of the following:

* Return (failure, false).
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes

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

Choose a reason for hiding this comment

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

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?


1. Wait until one of the following occurs:

* If the user closes the dialog, return (failure, true).
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Done.

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Done

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

* 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@samuelgoto samuelgoto left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Took another pass. Looks great! Some nits

@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

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

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

nit: [=/origin=]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

spec/index.bs Outdated
@@ -765,6 +934,7 @@ dictionary IdentityProviderAPIConfig {
required USVString accounts_endpoint;
required USVString client_metadata_endpoint;
required USVString id_assertion_endpoint;
USVString login_url;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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 point, done.

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

Choose a reason for hiding this comment

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

nit: this requires parameter

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

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

oops, just realized I did not publish my responses

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Done.

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated

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

Choose a reason for hiding this comment

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

Done

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@npm1 npm1 merged commit a49343e into w3c-fedid:main Oct 13, 2023
2 checks passed
@cbiesinger cbiesinger deleted the status2 branch October 13, 2023 15:31
@cbiesinger cbiesinger restored the status2 branch October 13, 2023 15:31
@cbiesinger cbiesinger deleted the status2 branch October 13, 2023 15:31
@cbiesinger cbiesinger restored the status2 branch October 13, 2023 15:31
@cbiesinger cbiesinger deleted the status2 branch October 13, 2023 15:31
github-actions bot added a commit that referenced this pull request Oct 13, 2023
SHA: a49343e
Reason: push, by npm1

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to mattdanielbrown/WebID that referenced this pull request Oct 14, 2023
SHA: a49343e
Reason: push, by pull[bot]

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

Finalize naming for the IDP sign-in status API
7 participants