When a user's session expires and `SimplesamlSubscriber::checkAuthStatus` is called it kicks off the `user_logout` flow. That flow makes its way to `SimplesamlphpAuthManager::logout` which hardcodes the `redirect_path` to `base_path()`. The issue here is that this prevents navigation with an expired session by requiring users to bounce to the `base_path()` before going where they intended. A user case is:
* user logs in, through SAML on Monday and does a bunch of work
* leaves their session open, but walks away from the computer for the evening
* returns on Tuesday (after their session has expired) and clicks a link in their email to `/about`
* now, the Saml module gets involved and notices the session has expired
* the logout flow is run when `/about` is loaded
* the user is redirected to `base_path()` and never sees `/about`
This is more catastrophic if a user is clicking a "vanity" URL/redirect. If they come in on Tuesday and try to get to `/vanity` they'll be redirected to the homepage and not the intended redirect.
I would suggest removing `$redirect_path = base_path();` from the logout flow, but I'm not sure how that would affect actual logouts since our Saml provider hijacks logouts anyway.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | remove-base-path-2915245-2-reroll-8.patch | 557 bytes | anneke_vde |
| #6 | simplesamlphp_auth_d7_2x_session_expiry_redirect-2915245-6.patch | 465 bytes | alexiswatson |
| #2 | remove-base-path.patch | 670 bytes | markhuot |
Comments
Comment #2
markhuot commentedI have a patch for the above suggestion attached.
Comment #3
markhuot commentedComment #4
mstrelan commentedComment #5
mforbes commentedWe're also seeing this exact behavior running 7.x-2.0-alpha2 on one of our sites. Oddly, it only started having this issue around March 2018 (and it had been using 7.x-2.0-alpha2 for much longer, seeing as that was released in 2013) and was fine until something happened that we have not yet identified. We have several other sites also running 7.x-2.0-alpha2 and they do not have this issue.
Comment #6
alexiswatson commentedI understand that proper form is to vet the latest version first, though we do not yet have a D8 site ready to do so.
In the meantime, we also offer a similar patch against 7.x-2.x-dev, for those still on that branch. Once we have a site we can test the above patch on, we'd be happy to do so as well.
Comment #7
kanchamk commentedPatch given by #12 Here is working for me for this use case.
Comment #8
anneke_vde commentedReroll of patch #2
Comment #9
idebr commented