Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add the IDP sign-in status API to the spec #436
Changes from 7 commits
e2f48cc
8304968
465c717
896e95d
07eb407
8948f99
f19c445
663f421
090262d
d682586
9557220
0c154cb
d932e3f
f128898
71083c8
3fbe365
238fdf4
13ca4f7
4db1336
0c77ac9
afe7360
ddb3f24
2098134
5b091a0
6a38c4b
654908f
652b436
fb02431
d70fa1f
620848c
b6b9be3
436ee85
3c8eb4a
04e7156
99147b3
e7ab849
1fbab48
7f6eef5
0f0c4fe
e80e54a
bf30c3d
49fe2ad
2133a0b
d28201f
0d55b05
60cdb08
48e4d1a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to find a way to store the "type=idp" here too.
In addition to the "type, ideally, we would also make this extensible, so that different "types" could store different things. Here is a possible use of the Login Status API:
https://github.com/samuelgoto/login-status-api#browser-status-ui
So, maybe, we should have something that stores dict[origin] -≥ (status, dict[type] -> blob) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, as long as this is only used by FedCM, I think it's fine to only store something if type=idp was specified. The API should allow more but since the backend storage is abstract anyway and UAs will store it in a browser-specific way, I think there's no harm in pretending we only store type=idp data with no extra metadata.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"this is a subresource request" is a bit unclear for inside an algorithm. I wonder if this should use https://fetch.spec.whatwg.org/#concept-request-mode, or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up dropping the subresource part because it's not necessary IMO
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you mean here is that the user clears all cookies or site data which is accessible by a specific origin? That is, there is no way the user is still signed in to that origin. I'm not sure this is clear from the text
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I filed #493
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also need to get by "type=idp" here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not applicable with my note above