Fix FedCM connection status computations #526
Conversation
spec/index.bs
Outdated
| </div> | ||
|
|
||
| <div algorithm> | ||
| When asked whether <dfn>auto reauthentication is allowed</dfn> given an {{IdentityProviderConfig}} |
There was a problem hiding this comment.
"is allowed" feels a bit confusing. even if auto reauthn is "allowed" here, it may be "disallowed" later. i.e. there are other criteria for auto reauthn, right?
There was a problem hiding this comment.
Renamed a bit to try to avoid confusion
spec/index.bs
Outdated
| 1. Return whether [=connected accounts set=] [=list/contains=] |triple|. | ||
| </div> | ||
|
|
||
| <div algorithm="compute the login state"> |
There was a problem hiding this comment.
the audience may be confused when referring to login state and login status, is it possible to choose a different name? or is it intentional?
There was a problem hiding this comment.
Changed to the original name, this name change was a mistake.
spec/index.bs
Outdated
| 1. Let |triple| be the result of running [=compute the connected account key=] given |provider|, | ||
| |account|, and |globalObject|. | ||
| 1. If [=connected accounts set=] [=list/contains=] |triple|, return | ||
| [=compute the login state/loggedIn=]. |
There was a problem hiding this comment.
the triple being in the set shows the user has logged in before but not necessarily "logged in" at the moment (assuming "loggedIn" implies the current state like the LoginStatus API). Would something like "has(LoggedIn)Account" work?
There was a problem hiding this comment.
Actually I see we have hasReturningAccount below so that may also be confusing.. by the way, what's the difference between hasReturningAccount and auto reauthentication is allowed?
|
LGTM. |
|
Done. Ben, this PR looks scary but the actual change is fairly small and summarized in the PR description. Let me know what you think |
|
I'm having a hard time following the state machine of the account's state, but I think this doesn't actually modify it? It just makes it so that the IDP can suppress the TOS/PP dialog with Why is this needed though? When can an IDP have a client already approved but never have used it in this browser? |
This is correct, that is the idea of this change.
This is needed since the IdP may have updated the user state outside of the browser's knowledge. One example of this would be if the user logs in to the RP on another browser via the IdP account. Then the IdP considers this account to be a returning account, and while auto reauthn is not performed, things like highlighting the account and not showing PP/TOS can be done to improve the UX. |
|
Ah, right "client" here means RP. I continue to have reservations about the inclusion of this much IDP logic embedded in the API, but this seems like a reasonable fixup. |
SHA: 5013145 Reason: push, by npm1 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
In Chrome implementation, we actually have two different computations:
approved_clientssays an account is returning, we need to have observed in the browser.approved_clientsis present, we rely on it. Otherwise, we rely on browser storage.Note that the previous 'compute the connection status' algorithm now takes on the PP/TOS meaning, so its definition is slightly changed.
Fixes #626
Preview | Diff