Closed cbiesinger closed 1 year ago
@bvandersloot-mozilla , can you take a look and see if this makes sense?
@cbiesinger - what happened to the Javascript API?
Ben: I was going to do that one separately, do you prefer if I add it to this PR?
Yes, that would be great!
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!
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.
I've addressed all comments so far. In particular, I have now specified an IDP sign-in flow based on the signin_url. Please take a look, especially @bvandersloot-mozilla and @npm1
Looking at this and a lot has changed since February. Thoughts to start out:
navigatable
inside of show an IDP sign-in dialog
.Overall, I believe this represents what we agreed would be reasonable in February (although, admittedly that was a long time ago). I would hope we can resolve the log in status integration in person this week at TPAC.
While I work on npm's and Ted's issues, and the TPAC changes:
- you probably want a
navigatable
inside ofshow an IDP sign-in dialog
.
This is done now
- the steps for "If signinStatus is signed-out , the user agent must either:" are very broad. I think that this will end up working out because the second option includes a user-cancel to failure.
Do you want it to say something else? I'm not 100% sure on what your UI will look like here.
- Integration with Login Status API is still pending as a big issue.
Gladly that was resolved during TPAC. I will comment again once I have updated this PR accordingly. (See also https://github.com/fedidcg/login-status)
OK I think this should be fully ready to be reviewed again!
Explainer: https://github.com/fedidcg/FedCM/blob/main/proposals/idp-sign-in-status-api.md
Fixes #430, #447
Preview | Diff