Hello,

In simplesamlphp_auth.module, simplesamlphp_auth_user_logout().

  // Only interfere if this user was logged in through simplesaml.
  if ($simplesaml->isAuthenticated()) {

Does not work and if you are loggin in using a user authorized to log in the standard way (not with simplesaml), you can't log out.

I will upload a patch.

CommentFileSizeAuthor
#10 simplesamlphp_prevents_logout-3088028-10.patch1.15 KBjastraat
#2 simplesamlphp_auth-logout_standard-3088028-2.patch1.84 KBgrimreaper

Comments

Grimreaper created an issue. See original summary.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » Unassigned
Status: Active » Needs review
StatusFileSize
new simplesamlphp_auth-logout_standard-3088028-2.patch1.84 KB

Here is the patch.

Thanks for the review.

zealialize’s picture

zealialize
Primary language Japanese
Location Japan 🇯🇵
commented

Patch applied and solves the issue.

aniketto’s picture

aniketto
Primary language Japanese
Location Tokyo
commented
Status: Needs review » Reviewed & tested by the community

Colleague of @zealialize and confirmed that the patch applies successfully thereby solving the issue.
I am marking this as Reviewed and tested by the community and request to take it ahead.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

I'm not sure what problem this exactly solves, can you explain a bit more?

The only thing this hook does is destroy the simplesamlphp specific session as well, but if you did not log in using simplesamlphp, then you shouldn't have such a session in the first place?

Also, these settings don't mean those users/roles can *only* log in locally, they could still be logged in and then if they did log in using saml then they wouldn't be logged out .

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I guess this is the same problem as #2837029: All site forms fail with "The form has become outdated" after allowing "Authenticated user" role to log in with local accounts.

norman.lol’s picture

norman.lol
he/him
Primary language German
Location Berlin
commented

I'm having the same problem, too. Though I'm not connected to an identity provider, yet.

norman.lol’s picture

norman.lol
he/him
Primary language German
Location Berlin
commented

Ah yeah see, the problem was probably related to either still having phpsession as store type OR not being connected to an identity provider. Now it works with store type sql and connected to an idp.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Hello,

The problem I have is when the SimpleSAMLPHP library is not installed:

$simplesaml->isAuthenticated()

Prevents user logout.

But no problem on 8.x-3.2 without any patch if the library is installed and configured.

So I guess that now on the environments where the authentication provider is not present, I will use config override to disable authentication via SimpleSAMLphp.

You can close the issue if you want.

jastraat’s picture

jastraat commented
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new simplesamlphp_prevents_logout-3088028-10.patch1.15 KB

This is still a problem in the latest version of the module. It's primarily an issue if the local development environment does not have simplesaml. A user who logged in via drush for example cannot logout.

Rerolled earlier patch to be compatible with latest dev.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

If you don't have simplesaml configured properly you should use a settings.php config override to not have it enabled (or the opposite, only enable it through a config override on production), I'm not fond of adding extra logic to check for that.

I still don't quite get what "can not logout" means. Do you get an exception? We could put a try/catch around isAuthenticated() then to ignore an error in that case.

jastraat’s picture

jastraat commented

We typically only have saml enabled in the staging and production config splits. But the latest version of the module added a new configuration variable and didn't maintain the pre-existing behavior when it was added, so I had to do some local debugging of the user login form alter. I can only do that with the module enabled even though I don't locally have saml installed.

I believe the error is related to:

Warning: session_create_id(): Failed to create new ID in SimpleSAML\SessionHandlerPHP->newSessionId() (line 173 of /var/www/html/vendor/simplesamlphp/simplesamlphp/lib/SimpleSAML/SessionHandlerPHP.php) which is slightly odd on the logout page.

To the user trying to log out, it's as though the logout link just doesn't do anything.

chrisgross’s picture

chrisgross commented

I can confirm that this is an issue. If the module is installed in an environment where simplesaml is not configured, local users cannot log out. I think this is a pretty common scenario in development, so it could use a fix. Patch #10 works, but I can't say one way or another whether that's the right way to handle it.

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
commented

I can confirm the same behavior happens when simplesaml is configured, but the IDP has not finalized registration of the endpoint. Logged in as super user whose account has not been enabled to allow SAML login, and clicking "Logout" fails silently. Navigating to /user/logout has no effect whatsoever, no error in logs.

fabienly’s picture

fabienly
Location Paris
commented

Hello with :
- core 9.4.7
- php 7.4.33
- PostgreSQL 13.9
- SimpleSAMLphp authentication 3.3.0
and patch 10 install with composer, I still can't disconnect user if they connect without SAM, in an preprod environment.
And no error or warning in the log.