This module calls the PHP global method `session_destroy()` instead of using the Drupal session_manager service. This should use the service instead, since it's Drupal specific code.

There is no real "problem" I'm trying to solve here, just something I noticed when debugging this method. The general "call stack" is as follows...

user.module : user_logout()
- method invokes all hook_user_logout commands.
-- simplesamlphp_auth.module : simplesamlphp_auth_user_logout()
--- method uses php's built in function to destroy the session.
--- method calls the simplesamlephp library's logout method, which redirects the user to "/" or a setting variable. this skips the rest of the stack below.
- method destroys the session with the session_manager service.
- method sets current user account to a new anonymous user.

CommentFileSizeAuthor
#15 simplesamlphp_auth-use-session-manager-logout-2900442-15.patch1.36 KBangrytoast
#11 2900442-11.patch1.37 KBjoelpittet
#5 2900442-5.patch1.32 KBjoelpittet
#2 use_drupal_session-2900442-2.patch1.14 KBdpagini
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dpagini created an issue. See original summary.

dpagini’s picture

dpagini CreditAttribution: dpagini as a volunteer commented
Assigned: dpagini » Unassigned
Status: Active » Needs review
FileSize
use_drupal_session-2900442-2.patch1.14 KB

This patch uses the Drupal session_manager to destroy the session. In addition, since the stack I described skips the functionality in Drupal core's `user_logout()` method, I am also setting the $account variable's "Account" to a new Anonymous User session. I don't think this will have much an effect, but I wanted to ensure we are still accomplishing what Drupal Core is doing OOTB.

I think it's a little heavy handed for the SimpleSAMLPHP module to take over the current request and redirect the user. I wish there was a less intrusive method that could be called to clean up the user session, but still allow other Drupal modules to execute code.

dpagini’s picture

dpagini CreditAttribution: dpagini as a volunteer commented

Seeing this is a similar approach to #2604058. Adding it as a related item. Sorry for the duplicate. This is a more simple/straightforward change, and that issue is marked as Needs Work currently. Going to leave this open to get feedback.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

This seems correct and simple. The other issue can re-roll with this fix in place as it should help them concentrate on the their SSO issue title goal.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
FileSize
2900442-5.patch1.32 KB

Not changing the RTBC status, this is the same patch just rerolled due to use statement not there -use Drupal\Core\Link; and an extra line break. And I added the interface like the other issue because it gives a nice IDE hints.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 2900442-5.patch, failed testing. View results

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Needs review

That failed test looks unrelated, I've requeued testbot.

Status: Needs review » Needs work

The last submitted patch, 5: 2900442-5.patch, failed testing. View results

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Needs review

I think there is an issue with the test not the patch, this error happened on a completely different patch.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

Here's a fix for the failing unit test in HEAD #2969420: Tests failing in HEAD due to missing mocked method

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
FileSize
2900442-11.patch1.37 KB

Rerolled because #2764733: Logout compatibility with Masquerade module was committed first.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

We've been using this for a while. Just checking/bumping.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented 
+++ b/simplesamlphp_auth.module
@@ -48,10 +50,11 @@ function simplesamlphp_auth_help($route_name) {
  * Implements hook_user_logout().
  */
-function simplesamlphp_auth_user_logout($account) {
+function simplesamlphp_auth_user_logout(AccountProxyInterface $account) {

The documented argument for this is AccountInterface, not AccountProxyInterface.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Needs work
angrytoast’s picture

angrytoast CreditAttribution: angrytoast as a volunteer commented
FileSize
simplesamlphp_auth-use-session-manager-logout-2900442-15.patch1.36 KB

Updating with a patch; taking the past work and addressing the concerns in #13.

angrytoast’s picture

angrytoast CreditAttribution: angrytoast as a volunteer commented
Status: Needs work » Needs review
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

Thanks for fixing that, I was about to roll a patch and realized it was broken because of the issue in #13

  • Berdir committed 8f954ad on 8.x-3.x authored by angrytoast 
    Issue #2900442 by joelpittet, dpagini, angrytoast: Use Drupal session...
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Reviewed & tested by the community » Fixed

Thanks, committed.

Status: Fixed » Closed (fixed)

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