Support to logout from the OAuth2 Server provider

This should use OAuth 2.0 Token Revocation, rather than a browser redirect, where possible:
https://tools.ietf.org/html/rfc7009

Also interested in this as an added feature.
We have a situation where multiple users could be logging in from a the same computer and accessing as the previous user.
Following.

As far as I can tell token revocation does not appear to be supported by the provider I intend to use this with (OpenID Connect Windows AAD), so in this case I think a redirect is acceptable. The #1 patch works for me, a caveat being the need for the endSession function to exist for additional providers. I modified the patch to check for the endSession function in the provider first and leave an error message in the log if it doesn't support it. The endSession function for the generic provider may need modified as I'm not using it so I did not test.

Note: This breaks the masquerade module, have to add some hackery in to check if called by the masquerade module to skip the logout hook if you want masquerading to work.

This patch improves upon #5, because it fixes the masquerade issue mentioned in this issue, as well as fixing an issue with Azure AD on logout (there was a php error because the tokens did not exist in the $_SESSION variable). Tokens are not used on Azure AD, so a check needs to be added to prevent PHP errors on logout in that case.

Nvm

I've been using patch #6 in production for nearly an entire year, and I haven't had any issues.

I've now committed #2559543: Add pre_login hook similar to post_authorize hook, which this patch is duplicating. This needs to be re-rolled without that code. Note that the committed code uses hook_openid_connect_pre_authorize

Patch updated as mentioned in comment #10.

#12 works for me when applied to dev when not masquerading.

One thing I noticed, however, is that if I visit /user/logout while masquerading, it looks like it is logged out, showing the anonymous front page, and clicking any page will act like I'm not logged in, but if I then click to log in or visit the login link, I'm instantly authenticated (as the parent user, not the user I was masquerading as), bypassing login. If this is an edge case that doesn't need to be handled then it's fine (after all, I normally replace the login button for people masquerading with a button to stop masquerading).

I have not tested if it behaves this way without the patch.

+++ b/openid_connect.module
@@ -912,3 +912,50 @@ function openid_connect_connect_current_user($client, $tokens) {
+        session_destroy();

I don't think that the session should be destroyed here. Other implementations of hook_user_logout() that run after this might rely on session data. Also, the session gets destroyed in user_logout_current_user() immediately after all of the logout hooks run, so it seems unnecessary. I could provide a patch, but I don't have a convenient way to test it myself because I'm only consulting for someone else who is using this module and patch.

Other than that, the patch is working well for us. We don't use Masquerade, so I'm choosing not to comment on whether the issue mentioned in #12 is an edge case or not.

Removing session_destroy(); here completely prevents effective logout. On redirect after logout, it will be as if no logout occurred, with the user still logged in, despite the watchdog message showing the user_logout_current_user() function ending the session.

I tried testing without it, as well trying setting the session global variable to an empty array instead, but neither works. All I had to do to test this was change the code and log in and out with SSO.

If it's necessary, the module could alter the order of hook implementations to ensure this hook is called last.

Thank you for testing my suggestion, @solideogloria. I appreciate the assistance. Based on your description I see what's happening. The patch uses drupal_goto() to redirect the user. drupal_goto() calls drupal_exit(), which interrupts the execution. So yeah, the session is never destroyed by user_logout_current_user(). Sessions would have to be destroyed in this hook or we would have to not use redirects for logging out, which sounds like it's impossible in some cases.

By the way, this also means any hook_user_logout() implementations that are executed after this one get skipped entirely. So it's potentially a bigger problem than the session data simply being unavailable to them. @solideogloria is right, this could require adjusting the module weight.

Actually the watchdog line in user_logout_current_user() is still called (I see the message in the dblog), so I don't think we're preventing execution of other code...

Re: #13, after looking at masquerade.module, I decided that's (kind of) the intended behavior. Clicking to log out while masquerading logs out the masqueraded user and does not affect the parent user in any way:

function masquerade_user_logout($account) {
  if (!empty($account->masquerading)) {
    global $user;
    cache_clear_all($user->uid, 'cache_menu', TRUE);
    $real_user = user_load($user->masquerading);
    watchdog('masquerade', "User %user no longer masquerading as %masq_as.", array('%user' => $real_user->name, '%masq_as' => $user->name), WATCHDOG_INFO);

    $query = db_delete('masquerade');
    $query->condition('sid', session_id());
    $query->condition('uid_as', $account->uid);
    $query->execute();
  }
}

This begs the question about whether the following code should even be included in the patches, since if I'm right, then the user should/would be logged out:

  // Check if we're masquerading, if so don't do this.
  if (isset($_SESSION['masquerade_user_logout']) || isset($_SESSION['masquerading'])) {
    return;
  }

However, the patch is working well as-is for me. If nobody has any objections, I'm going to mark it RTBC. The above could always be put as a separate issue.

Removed unused global user;

I reviewed this again during the process of creating a patch to support logout for OpenID Connect Microsoft Azure Active Directory client, and I updated the URLs in a comment.

Realized that @see comments should come after @param comments.

I realized that the base class and interface should match this change.

Rerolled.

re-rolled for tag: 7.x-1.0

The patch in #27 applies and the resulting code changes are the same as the previous patch.

If someone else will review, this could be RTBC.

Is this also a problem in the 3.x branch?

Since September 12, 2022 there is an official spec for this.
See https://openid.net/specs/openid-connect-rpinitiated-1_0.html

re-rolled against current 7.x-1.x

Tested with Auth0 and worked.

@colan No, logout is supported in 3.x

@xaris.tsimpouris, thanks for starting this!
And to everyone in the thread that pushed it forward.

I have been trying the latest iteration at #31, and it seems to work OK 👍

I have iterated over it, and added some changes on top, but first, let me reply a bit to previous comments.

The comment at #16 is quite relevant:

By the way, this also means any hook_user_logout() implementations that are executed after this one get skipped entirely. So it's potentially a bigger problem than the session data simply being unavailable to them. @solideogloria is right, this could require adjusting the module weight.

⚠️ In other words, using this patch also means that the site may need to adjust the module weight for this module, so it runs at the end of the set.
There seems to not be a way around it, since a redirect is quite definitive.

Actually the watchdog line in user_logout_current_user() is still called (I see the message in the dblog), so I don't think we're preventing execution of other code...

Re #17: The watchdog message happens before invoking the hooks, hence it will appear at the start.
E.g. if module A and openid_connect are implementing hook_user_logout(), and the openid_connect module gets called first, the A_user_logout() hook will not be run.

Since September 12, 2022 there is an official spec for this.
See https://openid.net/specs/openid-connect-rpinitiated-1_0.html

Re #30: Yes.
Looking into the way this works, and the spec about Relying Party initiated logout linked, I think these changes actually implement it.

On masquerade module support, I added a check to verify if the module is there on the related code, since it may not be there.

I have also added the client identifier as parameter on the redirect, which is part of the optional parameters to pass on the spec, and will add an extra hint to the IdP that will produce an extra check.

Also, I have been trying to avoid the persistence of the token data into the session, but I could not arrive to a working solution.
I even tried to use the internal mechanism for redirect, but I could not make it work, basically the following hunk.
Ideas are welcome about how that may be possible.
In the mean time, I restricted this to only the token_id, since that is the only consumed piece.

    diff --git a/plugins/openid_connect_client/generic/OpenIDConnectClientGeneric.class.php b/plugins/openid_connect_client/generic/OpenIDConnectClientGeneric.class.php
    index 9db59a5..51fff9e 100644
    --- a/plugins/openid_connect_client/generic/OpenIDConnectClientGeneric.class.php
    +++ b/plugins/openid_connect_client/generic/OpenIDConnectClientGeneric.class.php
    @@ -66,7 +66,12 @@ class OpenIDConnectClientGeneric extends OpenIDConnectClientBase {
           'post_logout_redirect_uri' => htmlentities($post_logout_redirect_url),
           'client_id' => $this->getSetting('client_id'),
         ));
    -    drupal_goto($endpoints['end_session'], $url_options);
    +    $_SESSION['openid_connect_destination'] = array(
    +        $endpoints['end_session'],
    +        $url_options,
    +    );
    +    $_SESSION['openid_connect_op'] = 'logout';
    +    drupal_goto(OPENID_CONNECT_REDIRECT_PATH_BASE . '/' . $this->getName());
       }
     
     }

I opened a 2882270-logout-support topic branch and related new MR !107 with the following commits.

• 73288fd Add patch from #2882270-31 by solideogloria, xaris.tsimpouris, romanos, nrogers: Support to logout from the OAuth2 Server provider
• 17462c5 Formatting related changes
• 8bcd65e Link to relying party initiated logout, not to session management logout
• c8b3e9c Pass client identifier on logout request
• f453cce Check for masquerade module before checking its data
• c9edac6 Only store id_token for endSession
• 1f9b430 Trust openid_connect_get_connected_accounts() output
• 248beb3 Skip watchdog and show message on no IdP logout
• 09e48d8 Tweak logic on hook_user_logout() implementation

⚠️ In other words, using this patch also means that the site may need to adjust the module weight for this module, so it runs at the end of the set.

Rather than module weighting, hook_module_implements_alter() is a bit nicer to control hook ordering, in case some hooks need to fire early and others late.

Rather than module weighting, hook_module_implements_alter() is a bit nicer to control hook ordering, in case some hooks need to fire early and others late.

@drumm, that makes sense.

I have added an implementation of that hook moving the new hook_user_logout() to be executed at the end.

New commits on the 2882270-logout-support topic branch and related MR !107:

- 56dfedf Move hook_user_logout implementation to the end of the set

I've done a code only review of the latest MR. It looks in order. I hesitate to mark RTBC since I haven't manually tested it.

Looks good to me too. We will be running this on Drupal.org’s codebase.