Issue status:

  • depends on 2863340.
  • I will leave this sitting here for a while until I know how to test it well, and may just commit it later.

Contains the following:

  • Remove the user_logout() call from the saml/logout page callback; only log out from Drupal after we return to the saml/sls callback. (This is what the 'droath branch' also does.)
  • Store session_index / session_expiration properties upon login, and pass the session_index to the IDP's SLS callback on logout. (Ported from the 'droath branch, minus the indirection between various classes/methods. I didn't create dedicated setters/getters for these properties yet, on purpose.)
    • Note that nothing is (or was, in the other branch) done with the session_expiration property, so far.
  • If the SLS processing from the PMP SAML toolkit returns a URL to us, this will be used to redirect to. (I just found this in the SAML toolkit sources. I'm not 100% sure of the mechanism/specification behind it, but it seems that the SLS URL can be used to initiate logout too, and I see no harm in just adhering to that redirect.)
CommentFileSizeAuthor
#12 samlauth-logout_sessions-2863975-5.patch7.09 KBO-khainouski
#6 interdiff-2863975-0-4.txt1.75 KBroderik
samlauth-logout-sessions.patch6.77 KBroderik
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

roderik created an issue. See original summary.

interX’s picture

interX CreditAttribution: interX commented

I almost created a similar patch before I noticed this issue :o Mostly identical, but it had one small difference: the NameId was also stored in the session with the sessionIndex when authed. Both were passed to the logout method.

Sadly, the IdP I currently use in a project uses POST binding for logout, so I can't fully test the entire flow.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Related issues: +#2863340: Fix/unify usage of redirects

  • roderik committed befaa96 on 8.x-2.x 
    Issue #2863975 by roderik, droath, interX: Fix SLS
roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
CreditAttribution: roderik at Cheppers for Acquia commented
Issue tags: +Drupalaton 2017

It was time to commit this, because it fixes a bug on logout - but I committed the cardinal sin of not testing the code that I changed, first.

Changed as per interdiff: what I believe are InterX' suggestions.

@InterX: just curious / in case there is something I'm not getting...
Does the IDP of that project prevent you from doing Single Logout? Or did you implement more custom code to perform a POST request?

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
CreditAttribution: roderik at Cheppers for Acquia commented
FileSize
interdiff-2863975-0-4.txt1.75 KB

So as I said: interdiff. (The full patch can be found through the commit message.)

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
CreditAttribution: roderik at Cheppers for Acquia commented

Oops! I clearly need automated tests. Weird typo in SamlService. A fix commit was just pushed.

interX’s picture

interX CreditAttribution: interX commented

The IdP I was using was configured to do single logout via POST only. Unfortunately Onelogin doesn't or didn't support this.
This means I couldn't test your patch with that IdP to see if the logout flow was working.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
CreditAttribution: roderik at Cheppers for Acquia commented
Status: Needs review » Fixed

Thanks for the answer. I was half doubting whether the patch you almost created (i.e. code you already created) was somehow working with SLS in a usable way.... and maybe that was worth considering adding to the module even when the library doesn't support this natively.

Since that's not the case: closing.

Status: Fixed » Closed (fixed)

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

O-khainouski’s picture

O-khainouski CreditAttribution: O-khainouski at EPAM Systems commented
Version: 8.x-2.x-dev » 8.x-2.0-alpha1
Category: Task » Bug report

I use version of module: 8.x-2.0-alpha1 released 2 March 2017 and faced the problem described in this ticket
I need patches are applied using a composer without conflicts, therefore I created a patch samlauth-logout_sessions-2863975-5.patch from
branch remotes/origin/8.x-2.x and from commits:
befaa96 2017-08-11 | Issue #2863975 by roderik, droath, interX: Fix SLS [roderik]
feb41ae 2017-08-12 | Oops: 2863975 typo! [Roderik Muit]

    "patches": {
      "drupal/samlauth": {
        "Make destination configurable": "https://www.drupal.org/files/issues/2019-01-03/samlauth-destination_configurable-2670118-26.patch",
        "Fix/unify usage of redirects": "https://www.drupal.org/files/issues/samlauth-redirects-2863340-3.patch",
        "Fix SLS": "https://www.drupal.org/files/issues/samlauth-logout_sessions-2863975-5.patch",
        "Error: Call to a member function processSLO() on null": "https://www.drupal.org/files/issues/error_call_to_a_member_function_on_null-2910257-3.patch"
      }
    },
    "composer-exit-on-patch-failure": true,
    "enable-patching": true,
O-khainouski’s picture

O-khainouski CreditAttribution: O-khainouski at EPAM Systems commented
FileSize
samlauth-logout_sessions-2863975-5.patch7.09 KB