Support Single Sign-on with OpenID Connect

[alxndrsn]

Frontend portion of getodk/central#449. These are Frontend changes required for getodk/central-backend#910

[matthew-white]

@alxndrsn observed:

pressing back from the oauth provider takes you back to the login screen, but with the button disabled

I see that behavior as well. I think it's due to the back/forward cache. I've pushed a commit that handles that case by listening for a pageshow event. I've tried it out locally, and the button is now enabled after navigation back from the IdP login page. 🎉

[matthew-white]

In case this piece of Vue magic is unfamiliar, note that we're able to pass this.reenableIfPersisted directly, rather than as part of something like (event) => this.reenableIfPersisted(event). That's because methods are automatically bound to the component instance.

[alxndrsn]

Works for me 👍 checked on safari mobile, and firefox & chrome desktop.

[matthew-white]

I observed the following while trying the SSO login flow on dev:

I initially had difficulty logging in. I clicked Continue, selected my account, saw the redirect happen, but then ended back on the login page. I cleared cookies and local storage, then things started working.

…

OK, I have an idea about how this sequence could happen:

• Log into a server that doesn’t use SSO.
• Close the tab and let your session expire (24 hours). You will no longer have a cookie, but the sessionExpires timestamp will still exist in local storage.
• Because sessionExpires indicates that the session is expired, Frontend won’t try to restore the session after the redirect back from IdP login.
• And actually, I think this will be a problem even on a server that uses SSO. If you let your session expire, then you won’t be able to log in again.

I resolved this by having restoreSession() always send a request. Previously, it didn't send a request if it could infer from sessionExpires that the session was expired. I'm pretty sure I added that logic just as an optimization, but here it's ended up causing issues for SSO.

The optimization was helpful in some cases, but there were also several logout cases in which the request was still sent:

• If the user visited Frontend for the very first time.
• If the user clicked "Log out", then opened Frontend in a new tab (perhaps some time later).
• If Frontend automatically logged the user out before their session expired, then the user opened Frontend in a new tab.

Those are all cases in which sessionExpires is not set in local storage. There are times when it is important for the request to be sent even when sessionExpires isn't set, for example, after a logout error.

The optimization helped when sessionExpires was set, and its value was in the past. That would happen if the user's session expired, but they weren't logged out through Frontend.

I considered alternatives to the approach I took:

• After the user clicks the Continue button in the AccountLogin component, have the component remove sessionExpires if its value is in the past. I didn't opt for this approach because we don't currently export removeSessionFromStorage() from src/util/session.js, and I'm reluctant to add to that interface. This also felt like additional complexity in the component and in the login flow as a whole.
• When Frontend determines whether to send the session restore request in restoreSession(), remove sessionExpires if its value is in the past (and don't send a request). This felt like one more piece of logic in an already complex flow: I'm happy to be removing an if block here. Ultimately, the optimization doesn't seem helpful enough to me to justify additional complexity.

With this change, sessionExpires is only used for two things:

• Coordinating logout among tabs
• Showing an alert on the login page and preventing login if a user is already logged in

We may change the latter as part of getodk/central#400, further simplifying our use of sessionExpires.

[matthew-white]

Removing this if means that we will always remove sessionExpires after a 404, not just if sessionExpires was set to a future timestamp before the request was sent. However, I don't think that's a problem:

• If somehow there's a login in another tab between when this tab sends the request and when it receives the 404 (due to some race condition), any open tab that is logged in will be logged out after the 404. That may be inconvenient, but that sequence of events should also be rare.
• Even if the tab that logged in is closed before the 404 is received, things should mostly be OK. In that case, sessionExpires will not be set even though there is a session cookie. However, Frontend will still be able to coordinate logout among tabs.
• In that case, there would be more of an issue in the tab that receives the 404, because AccountLogin would allow a login attempt to proceed. However, that safeguard is there just to prevent a confusing user experience. If SSO is not enabled, the user would see a cryptic error message from Backend. If SSO is enabled, and if another tab is opened after the 404 is received, the user will be immediately logged out after logging in.

Given that there weren't tests about this or comments in the code, I think the reason we checked sessionExpires before wasn't to prevent these possibilities, but rather to prevent unneeded storage events from being triggered (not that those are a big deal either).

I went back and forth on this one, which you can see in the commit history. But given our current setup, including the fact that restoreSession() does need to remove sessionExpires in some cases, I can't think of a way to prevent these possibilities entirely. And again, this sort of sequence should be unlikely. I suspect we'll come back to this area soon as part of getodk/central#400. For now, I think it's nice for sessionExpires to be used in fewer ways.

[sadiqkhoja]

Few things I noticed on dev:

• After showing error message, should we clear the oidcError query parameter? Currently, when I refresh error page after dismissing the error, I see the error again which looks odd to me
• Email address is still editable for currently logged in user, though it's not changed at the backend. I think frontend should block this

[sadiqkhoja]

Steps to reproduce:

• Login using id1@gmail.com
• Press back button
• Select another user i.e. id2@gmail.com

[matthew-white]

After showing error message, should we clear the oidcError query parameter? Currently, when I refresh error page after dismissing the error, I see the error again which looks odd to me

Sounds good, I'll make that change!

Email address is still editable for currently logged in user, though it's not changed at the backend. I think frontend should block this

The release criteria say that admins should still be able to update email addresses in case someone's email address changes in the IdP (say, after a name change). However, a user who is not an admin should not be able to update their own email address (or anyone's for that matter). So I think we actually need to make a change to Backend here so that admins can continue to update email addresses.

[matthew-white]

Ooh that's an interesting one! That looks like a page that Backend is rendering: I don't think we have an error component like that in Frontend. If Backend can't handle this sequence, I think it'd be better for Backend to redirect to Frontend, specifying an oidcError query parameter.

Maybe there's a way for Frontend to stop the user from going back in this case? Is that something I should look into? (Or do you know a way to do that?)

[sadiqkhoja]

Besides minor comments about the functionality, code looks clean to me

[sadiqkhoja]

how about navigating to home page instead of show alert?

[matthew-white]

We thought about doing that in the regular/non-OIDC case, but we thought it'd be confusing if the credentials entered didn't match the user who was already logged in, or if the credentials were incorrect (because they wouldn't be checked, yet it'd seem like they were). It's definitely a confusing user experience though, and I've filed getodk/central#400 to improve it.

That said, your comment is making me realize that we're able to do something different in the OIDC case. There are no credentials entered in that case, so nothing to be mismatched or incorrect, and I think what you describe would be helpful behavior.

[sadiqkhoja]

this is not a blocker, I feel this can happen in next release or later

[matthew-white]

Sounds good. I probably won't get to it for .4, but I've linked to this discussion from getodk/central#400. It'd be great to make that change at some point.

[matthew-white]

@alxndrsn added a new error, internal-server-error. I reworded the message, but I'd be happy to revert it if others prefer.

This now works differently, though I'm still not sure it's quite right. @sadiqkhoja, would you mind trying the new behavior on dev and seeing what you think? Here's what's happens now:

• Log in to Central using one account.
• Press back.
• Select a different account.
• Backend redirects to /#/login?oidcError=internal-server-error
• However, because there's still a session, the user is not brought to the login page: the session is restored, and the user is brought to the homepage. The oidcError is ignored, and the first account remains logged in.

I looked into it a little, and we could probably prevent the user from pressing the back button to return to the IdP login page. However, I'm not seeing that it's possible to restrict all navigation back, for example, if the user holds down on the back button and selects the IdP login page. If there's an approach we could take without restricting navigation, I think that'd probably be better.

[matthew-white]

OK, I think I'm ready to merge! I've filed an issue about the sequence with pressing back, etc.: getodk/central#477. Let's continue related discussion there. If I've missed something else here, or if you think of something additional, please leave a comment on this PR or file an issue. Excited to get this merged!