I have a website where some of the users are authenticated through SAML. However most users are not (and none of the admin users are).

In simplesamlphp_auth_user_logout() the current user's session is destroyed to avoid infinite loops.

However, this breaks the functionality of the Masquerade module (that offers admin the ability to login as another user).

Can we commit this patch that will make the code in simplesamlphp_auth_user_logout() inactive for non-SAML authenticated users?

CommentFileSizeAuthor
logout_only_when_active.patch1.39 KBbjaxelsen

Comments

bjaxelsen created an issue. See original summary.

dpagini’s picture

dpagini commented

I am having a very similar problem with custom logout redirects. SimpleSAMLPHP allows you to set a logout redirect URL, but not really dynamically. Since `simplesamlphp_auth_user_logout()` hijacks the logout stack, my custom redirect code gets completely skipped. This PR would help me with some of that issue if it only affects the SAML users (employees) and not our end clients.

Just a question, the Drupal core `user_logout` function uses the `session_handler` service, which I sort of think SimpleSaml should do as well, and set the user to anonymous, since that code is being "skipped".

So it seems like the "session_destory()" line should be replaced with the following...

  // From user.module method user_logout().
  \Drupal::service('session_manager')->destroy();
  $user->setAccount(new AnonymousUserSession());

Thoughts?

dpagini’s picture

dpagini commented
Status: Active » Needs review
dpagini’s picture

dpagini commented
Status: Needs review » Reviewed & tested by the community

I've been using this patch and it is working successfully for me. I'm going to mark as RTBC. I'm going to open a new ticket for my suggestion in comment #2 and submit my own patch, since it is sort of a separate issue.

trwill’s picture

trwill commented

Does anything else need to be done for this patch?

The hook_user_logout function is very heavy handed and basically breaks any other hooks and/or event subscribers that are running on logout. This is exactly the fix needed for those.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

This may need a reroll if #2900442: Use Drupal session manager in logout gets in first.

RTBC++

dakku’s picture

dakku commented

Thanks guys, I will get this committed later today.

  • dakku committed 0990ab0 on 8.x-3.x authored by bjaxelsen 
    Issue #2764733 by bjaxelsen: Logout compatibility with Masquerade module
dakku’s picture

dakku commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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

partyka’s picture

partyka commented

Hi,

Thank you for fixing this -- but I've noticed it's still in the 'dev' branch and yet to be incorporated into a released version. Please consider this a request to make a new release with this fix in it.

Thanks!

--partyka

kuldeep k’s picture

kuldeep k commented

Does it also work for another custom "hook_user_logout"? I want to destroy some extra cookies when user getting logged out but saml logout won't allow me to do that from my custom logout.

etiennejacquot’s picture

etiennejacquot commented

Was this ever fixed? I still get issues using 8.x-2.0-rc4 on Drupal 9.5.9 when trying to masquerade as a SAML authenticated user

Error: Call to undefined method Drupal\user\Entity\User::setAccount() in simplesamlphp_auth_user_logout() (line 68 of /code/web/modules/contrib/simplesamlphp_auth/simplesamlphp_auth.module)