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 3 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.
Do we care about A -embeds- B -embeds- A case?
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 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)
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.
Is this to allow UAs to reduce prompt spam?
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
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.
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?
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 it so that the following flow works reasonably, which I think is what Firefox wants to do:
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 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?
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? The potential IDP selector is handled on (new) line 623, already existing in the spec. "create an IdentityCredential" is called by DiscoverFromExternalSource.
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.
Isn't this a MUST? Otherwise, how can we move forward with the algorithm if the loginStatus is 'logged-out'?
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.
Don't we also have to "wait" somewhere here until the user becomes '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.
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.
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.
nit: this requires parameter
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.
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 need to be a substep of the mismatch dialog step? Or can it just be step 3 (the step after)
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, it needs to be a substep, because all of this only happens if loginStatus is 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.
Why does this return failure immediately?
This allows a user-visible read of the logged in bit.
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 sure I understand, since this requires the user to do something the timing is effectively random, indistinguishable from the delayed promise rejection?
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.
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".
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.
Ah, as of c8609ec the 120s has been replaced with a random timer.
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.
Let's pick one 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.
Done.
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.
Why is this not required like the other ones? Not that it matters since we don't really use this in JS but still
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.
Good point, done.
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 algorithm doesn't work for redirect login flows, correct?
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 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?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 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.
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'm not positive I understand you correctly but I think that's out of scope yeah
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 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
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 if the URL is null?
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 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)