Make user logout more robust

Taking a stab at doing something similar for the 8.x-2.x branch.

This looks good to me and the use case makes sense. Let's ship it.

Azinck, I'll leave this up for Roderik to take a peek on the D8 version. I know he's got a ton of stuff going in there with the 3.0 release. I can't readily speak for whether this might be redundant or not.

Sorry, I hadn't spotted / dit not remember that this issue is now actually for D8.

Because I overlooked this, a partial copy of azinck's patch has now appeared in #3132942: Log out Drupal session directly, don't wait for SLO. But I see we're trying to do two different but related things. Quoting myself from the other issue:

• klausi's original patch for the D7 version was to be able to start SAML logout also when the user was already logged out.
• 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.)
• [The new] patch only does the latter, and [adamfranco] do provide a specific reason: you want /saml/logout to perform regular Drupal logout also if Single-Log-Out isn't configured.

This last point of calling user_logout() at the beginning of the process is now moved off to #3132942-5: Log out Drupal session directly, don't wait for SLO. I'm not sure yet if I'll commit that, because 1) I'm confused about the reasons given, and 2) it undoes changes specifically made in #2863975: Fix SLS to move user_logout() to the end of the process. See my comment there.

That means I can use this issue to concentrate on the original issue: making /saml/logout available to users who are already logged out of Drupal, so they can log out of whatever session exists on the IdP side. I see the point, but there's an issue in the D8 version: the SAML session data is not available to logged-out users because we store it in a 'D8 tempStore' rather than in $_SESSION.

I'll do some tests around that.

Couple of things:

1) I said something wrong, yesterday: the user_logout() call immediately at the start of the SAML logout process, was also added by klausi in the D7 patch.

Still, I've separated that out to #3132942: Log out Drupal session directly, don't wait for SLO so I can wrap my head around things better.

2) I believe the $_SESSION stuff that klausi added to the D7 patch (which the D8 version effectively already has), has one drawback: remembering the SAML session data only works if you're still logged into Drupal before visiting /saml/logout. That makes the other change that was enabled (namely: for anonymous visitors to log out of their IdP session by visiting /saml/logout) potentially less effective.

However

• I tried to work around that, and failed.
• It still has use. (At least the IdP I tested on, doesn't even need the SAML session data in order to log people out.)

Sooo... I'm committing this.
The code added is almost useless in practice (except maybe an argument change to the processSLO() call), but maybe I'll be able to use it later. The actual change is just the lifted access restrictions in the routing.yml.

(The uploaded patch file contains the two commits in one, for overview.)

Just because I already largely typed up the following before sending the patch... I'll still dump it here in case I ever want a quick overview again of how SAML logout works.

We have handling of

• The start of the SP initiated logout process, in logout().
  • The SAML Toolkit at this point just constructs a SAML LogoutRequest to include in a HTTP request to the IdP.
  • For this LogoutRequest, we can pass tha NameID + Format + Session Index, which we've remembered upon login.... but only if we're not logged out already. If we visit /saml/logout when already logged out, these properties are not available so the LogoutRequest is much more generic. (Which seems to be no issue for some , or many?, IdPs.)
  • On the Drupal side, what we've just fixed is second-guessing this diagram / the changes done in #2863975: Fix SLS. It says that step 7 is "Perform logout", but we do not want to have the Drupal logout wait until step 7.
• LogoutResponses, i.e. step 6, in sls().
  • The SAML Toolkit does one thing here (by default): call session_destroy() and unset($_SESSION). I am not sure this makes much sense when we're already logged out; what data is there to destroy? So we're preventing this now.
  • Because of the changes in the previous point, the user would already be logged out from Drupal at this point - so we are done. (If somehow the user isn't logged out already, our code still works fine, though.)
• LogoutRequests, i.e. step 3, in sls(). In this case we're "SP 2" / in the middle of a logout flow and the user would usually still be logged in.
  • We also need to log the user out here, and then redirect to the LogoutResponse URL. (I just fixed the logout part in a recent commit.)
  • The SAML Toolkit returns the IdP's LogoutResponse URL - and also (by default) calls session_destroy() and unset($_SESSION) here. I'm sure there is no sense in the session_destroy() because user_logout() just did that.

Worth noting: the SAML Toolkit itself is session-less. It does not do anything re. storing session data and leaves that to the app/Drupal; the session_destroy() and unset($_SESSION) calls at the end just seem to be there "just in case the app forgot" / as a form of convenience.

@klausi - FWIW and in case you read this through a latest-updated-issues list:

I'm just reviewing some more details of the D7 version now.

This (from the patch in #2) was committed to the D7 version in 2019 but its equivalent is not present in any D8+ version. So if you expect issues with that, we should discuss.

+++ b/samlauth.module
@@ -207,10 +208,28 @@ function samlauth_logou() {
+  // Check if there is a SAML response already. We have no way of verifying the
+  // response because SAML PHP processResponse() does not support logout
+  // responses and processSlo() only looks at GET parameters.
+  if (isset($_POST['SAMLResponse'])) {
+    // Just assume everything went well and go to the frontpage.
+    drupal_goto();
+  }

I won't touch it because it was committed and I don't see an actual problem with ignoring a $_POST['SAMLResponse'] (which really doesn't contain much useful) but I'm hesitant about just lifting it into the D8 version:

A.
This smells like the IdP has been wrongly configured to have the single-logout URL be /saml/logout insted of /saml/sls.

I am not against making /saml/logout redirect to /saml/sls (and /saml/login to /saml/acs?) per se, if $_GET / $_POST are populated... but 1) this hasn't come up before; 2) I'm wondering if we should log a warning in that case. (The reason that the 'process-start endpoints' are different from the 'server-reply endpoints', I believe, is that the latter will not get into an eternal redirect if $_POST is somehow filtered out or the IdP gets wonky for some reason.)

B.
If we wanted this in the general D8+ version, I'd rather try to do this instead (after testing if there are issues - apparently the values are not fully equal):

  if (isset($_POST['SAMLResponse'])) {
    $_GET['SAMLResponse'] = $_POST['SAMLResponse'];
    samlauth_sls();
  }

Two small differences in practice: if the SAMLResponse is invalid / does not indicate success,

• an error message is shown (though it's not descriptive, so it's arguably not better than just logging the user out and ignoring the SAMLResponse);
• the user does not get logged out (though they were logged out already if the logout flow was SP-initiated).