Contrib.social

"Log out different user upon re-authentication" option not working as expected

Open on Drupal.org →
Created on 27 July 2023, over 1 year ago
Updated 21 September 2023, over 1 year ago

Problem/Motivation

When "Log out different user upon re-authentication." is selected, the newly authenticated user is not logged in.

Steps to reproduce

1. Set up SAML authentication with "Log out different user upon re-authentication." checked
2. Log in via SAML
3. Trigger re-authentication, e.g. via redirecting to the relevant IDP URL
4. Log in as a different user

Expected result

You should be logged in as the new user

Actual result

Although the logs show a session being created, you are not logged in.
If the redirect URL is a restricted path, an error message appears:

To log in to this site, your browser must accept cookies from the domain 

  ID      Date           Type             Severity   Message
 ------- -------------- ---------------- ---------- ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  19116   27/Jul 17:59   access denied    Warning    Path: /user/dashboard/phone?check_logged_in=1. Drupal\Core\Http\Exception\CacheableAccessDeniedHttpException: This route can only be accessed by authenticated users. in Drupal\Core\Routing
  19111   27/Jul 17:59   externalauth     Notice     External login of user <second user>
  19106   27/Jul 17:59   user             Notice     Session opened for <second user>.
  19101   27/Jul 17:59   externalauth     Debug      Authmap change (3f653d0e-048d-462e-bc90-20e640f82baa => 716abc0c-dc5b-4c9d-93da-b977a7207d6b) for user <second user> with uid 636 from provider samlauth
  19096   27/Jul 17:59   samlauth         Info       SAML login for name <second user> (as provided in a SAML attribute) matches existing Drupal account 636; linking account and logging in.
  19091   27/Jul 17:59   samlauth         Debug      No matching local users found for unique SAML ID 716abc0c-dc5b-4c9d-93da-b977a7207d6b.
  19086   27/Jul 17:59   user             Notice     Session closed for <first user>.

Proposed resolution

tbc

🐛 Bug report
Status

Fixed

Version

3.0

Component

Code

Created by

🇬🇧United Kingdom malcomio

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @malcomio
  • Comment over 1 year ago →
    🇬🇧United Kingdom malcomio
  • Comment over 1 year ago →
    🇬🇧United Kingdom alex_sansom

    This seems to be because when user_logout() gets called it destroys the session. If you only invalidate the session rather than destroy it, you're able to switch users OK.

  • Comment over 1 year ago →
    System Message
    • roderik → committed 02be295f on 8.x-3.x 
      Issue #3377532 by roderik, alex_sansom, malcomio: Fix "Log out different...
  • Status changed to Fixed over 1 year ago
  • Comment over 1 year ago →
    🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU

    Thanks for that last comment, it convinced me it must be the module and I'd have to take a look.

    This meant that I couldn't delay learning more about session handling... because I couldn't just go along with the part of "not destroying the session", without knowing more:

    • Would that run any risk of data from the previous session into the next?
    • That would mean we can't use Core's helper method (user_logout()). How risky is that?
    • Am I sure about what you mean with "the session" anyway?

    ...so... several weeks later I emerged out of this rabbit hole with detailed knowledge about Drupal's Session code, two potential blog posts and at least three Core issues to work on. (I won't yet, because I really need to do other things now. They may appear as related issues here... sometime.)

    As it turns out:

    • This issue has existed (the option hasn't worked) since Drupal 9.2, or #2238561: Use the default PHP session ID instead of generating a custom one →
    • Before this, the option was... not very nice either (details withheld). Reading that older code proved to me that doing a logout + login was never really supported.
    • I guess you meant specifically "call Session::invalidate() instead of SessionManager::destroy()".
    • I agree now that this is the solution. Which unfortunately means that I can't call user_logout(), and we are not protected against any future changes to that function. No other workaround/preparation currently works.
    • Luckily, I'm now sure that the comment against calling Session::invalidate(), present in user_logout(), isn't actually true.

    Further: I couldn't reproduce this on D10. As it turns out, this whole thing isn't an issue / the configuration option is unimportant if you set the ' samesite policy → ' for the session cookie to either "Strict" or "Lax" (because Lax also strips the cookie from the POST request), if the IdP is on a different domain (or hostname, depending on cookie domain settings, I guess).

    Change is in the commit I just pushed. The comments/changes around PrivateTempStorage can be ignored; they are just adjustments to other changes in D9 core.

    I added a /saml/reauth route for easier testing (though I'm not documenting it; it's harmless but I don't see the usefulness for anything except testing re-login of a different user).

  • Comment about 1 year ago →
    System Message

    Automatically closed - issue fixed for 2 weeks with no activity.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024