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 1 commit
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.
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.
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.
If I understood you correctly, done (both parts).
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.
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.
Am I following it right that if a user is "signed out", any use of the API fails without a dialog?
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.
Yes, just like (today) the API fails without a dialog if the user is not logged in.
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.
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.
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 added a "Issue:" for now. I think the multi-IDP PR is maybe a better place to discuss how to integrate these two?
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.
does this mean that the sign-in status map and the actual state is out of sync at this point?
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 previous step should ensure they are in sync, right?
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 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.
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 reworded the note a bit to hopefully make this more clear.