Log out Drupal session directly, don't wait for SLO

Oops. Thanks for the notification. I didn't spot/remember that #3043704: Make user logout more robust is now actually an issue for the D8 version of the module.

I'll roll the two slightly different / related issues back into one extended patch, because the current patches have unintended consequences for the $this->tempStoreFactory->get() calls that come right after.
(Which are not 'get' calls to a temp store factory - but to a temp store - as per #3112135: Drupal 9 Deprecated Code Report)

(I'm in the process of writing up all details in case I need to look them over later... because I clearly didn't 'get' Single Logout last time I changed it.)

I changed my mind, and am going to do things in two steps as you hinted at, because there are indeed different things going on. Also, people have different reasons for 'fixing logout':

• klausi's original patch for the D7 version in #3043704: Make user logout more robust was to be able to start SAML logout also when the user was already logged out. That's different, and I discovered it's more complicated.
• azinck's original patch says it does something similar but actually does two things:
  • it enables the same as the D7 patch, by making /saml/logout and /saml/sls available to logged-out users with the change in SamlController
  • it adds a user_logout() call immediately at the start of the SAML logout process (which the D7 version already/still* had), rather than logging users out after we get the LogoutResponse returned. It does not specify a reason, though the comments hint at wanting to use /saml/logout to log out non-SAML users. (Another reason would be that logging users out of Drupal only at the end of the SAML logout process is problematic. I thought that was a consideration but am now unsure.)
• Your patch only does the latter, and you do provide a specific reason: you want /saml/logout to perform regular Drupal logout also if Single-Log-Out isn't configured.

Upon re-reading your issue report, I'm confused:

• I'm not sure why you would even want to access /saml/logout if Single-Log-Out isn't configured, because its primary function is starting starting Single-Log-Out.
• If Single-Log-Out isn't configured, the processSLO() call throws an exception "The IdP does not support Single Log Out" so you end up with an error message. Is your system doing that?
• I don't understand how this is connected to your assertion "even though the IdP session is logged out":
  • How is "the IdP session being logged out" supposed to be related to our SP, if Single-Log-Out isn't even configured?
  • How exactly does "logging out of the IdP" relate to the /saml/logout endpoint? (Did you configure this URL manually in the IdP? For official IdP-initiated logout, I would expect the IdP to redirect to /saml/sls instead - but maybe there's a gap in my knowledge.)

I'm clearly not getting something - but I'd like to get it, because it can influence other (session related) work that needs to be done.

On top of that, there are issues with the patch:

• If the /saml/sls route isn't opened up to not-logged-in users. this patch will break SAML Single Logout.
• After logout, $this->tempstore->get() calls will not retrieve the correct data anymore, so the call to processSLO() does not specify the original NameID/session index in the LogoutRequest sent to the IdP. That's a regression, and I'm guessing it might prevent the IdP session from being logged out, on some IdPs (though that's not the case on my test setup). This is the case both with the patch in this issue, as with azinck's patch.

I've already made a new patch that fixes these issues, and made the code more generic, so I can concentrate on the part that is not included here, separately. (Which is opening up /saml/logout to not-logged-in users, as originally requested for D7, and which I'll do in #3043704: Make user logout more robust.) The 'actual' code change, besides the access for /saml/sls, is the addition of the $this->drupalLogoutHelper() call in logout().

I've included code comments that justify this 'actual code change' to the point that I currently understand it. But I'm not totally sure I'll commit this yet. *This undoes the changes made in #2863975: Fix SLS, which... I don't have a definite opinion about, but the user_logout() call was deliberately moved to the end of the Single Logout process, which AFAICT does better adhere to the officially documented SAML logout procedure, and (for now) has a better guarantee that the NameID/session index are included in the logout request.

It's been a year since I've looked at this, so I can't recall all the factors at play here, to be honest. One thing does jump out at me, though:

I'm not sure why you would even want to access /saml/logout if Single-Log-Out isn't configured, because its primary function is starting starting Single-Log-Out.

We've got a situation where we have some users log into a site directly (no SAML involved) and some users log in via SAML. And we want them to be able to share the same logout link (/saml/logout) so we need to be sure that /saml/logout does actually log people out, even if they're not SAML-based users.

I hope I'm not spreading confusion here. As I said, I'm pretty foggy about this now looking back on it, so if anything I'm saying doesn't make sense please just chalk it up to that :).

@azinck thanks for responding and wading through my wordiness :) I plan to transfer my confusion into code comments so that I don't undo things later.

I interpreted "Single-Log-Out isn't configured" as: the SLO URL is not configured, so people cannot be redirected to the IdP. In which case an exception is thrown. (So I... have questions about that.)

I'll assume for now that you do have the SLO URL configured. But here's what I'm unsure about: In your case, if users who have logged in locally, use /saml/logout to log out, then... they are redirected to the IdP, right? Is that fine with you? (It may be that it's just an unnecessary but short roundtrip, before they are redirected back to the site they've logged out from.)

• If they are not redirected to the IdP, then... I don't get what's going on with the call to $this->getSamlAuth()->logout() that happens immediately after logout.
• If they are redirected, then
  • I can see your use case (because otherwise you'd need to make a custom endpoint that decides to route to either /user/logout or /saml/logout, which is unnecessary(?) custom code). But if all your logging-out 'non-SAML' users indeed are redirected to the IdP and then back to the site... I'm not sure (yet) if I should commit that functionality without a warning or an alternative implementation.
  • are they not redirected back to the /saml/sls endpoint, though? If so: what happens? Do they get an error? (My test IdP just redirects any users back to /saml/sls with a LogoutResponse saying "success" - which the samlauth module processes, after which the user gets logged out fine. Also before any patch.)

I can confirm that I do have the SLO URL configured. I do not recall what was happening in terms of redirection for my non-SAML users prior to my creating the patch in #3043704: Make user logout more robust but I have to assume it wasn't working well or else I wouldn't have done it. I don't have an easy test-bed for this right this second or else I'd give it a go. Will try to get info for you before long.

@adamfranco thanks.

Even if it would be "due to inability to correctly configure AzureAD", it's still a valid use case. But I'm hesitant to force this use case on everyone: I expect some people (who want the logout process to behave 'technically correcctly') to protest.

That would mean this needs to be Yet One More Configuration Option. Now that I've closed off #3043704: Make user logout more robust / thinking about 'the session stuff', and the /saml/logout route is already meant to (also) redirect logged-out Drupal users to the IdP... I'll think about what I think this option / these options should say.

Oh how I love going around in circles in my own head.

There's not going to be a configuration option because

• I'm fine with logging SAML users out early in the /saml/logout process - which now already supports handling users who are already unauthenticated, anyway. (I don't even know what a configuration option would say, and if someone wants to say that it is "not technically according to SAML spec"... I should wait for that argument being made. I'm not even sure of it.)
• I can understand that people want to log out non-SAML users through /saml/logout, and who am I to prevent them from doing that?
• I'm less happy with the fact that those non-SAML users are redirected to the IdP. However there's no useful way of preventing that with a configuration option. I suspect that we can prevent it with a patch that needs careful testing... but as long as noone complains about it, I'm not going to bother. I'm just going to wash my hands of it for now, using a big code comment :)

So the attached patch is what I'll commit. Featuring

• The simple Drupal logout at the beginning of logout(), from the original patch
• except taking care that we use the SAML session data for the LogoutRequest if we have it
• and a bunch of comments (and a fix sneaked in for my previous commits: don't call getSamlSessionValue() anymore for anonymous users.)