User is not logged out of the service, and so can log in again without inputing password

I don't think this solution is a 100% fool proof.

You force a redirect in hook_user_logout, which means all modules which also implement this hook but are invoked after this module, never get called.

I fixed this (in my custom module) like this:

1. Removed the default user.logout route in a route subscriber

class RouteSubscriber extends RouteSubscriberBase {

  /**
   * {@inheritdoc}
   */
  protected function alterRoutes(RouteCollection $collection) {

    if ($route = $collection->get('user.logout')) {
      $collection->remove('user.logout');
    }

  }
}

2. Added my own 'user/logout' route:

my_module.logout:
  path: 'user/logout'
  defaults:
    _controller: '\Drupal\my_module\Controller\MyModuleAuthorizationController::redirectLogout'
    _title: 'End session'
  requirements:
    _user_is_logged_in: 'TRUE'

3. In the controller end the Drupal session and next redirect to end the session:

  public function redirectLogout() {
    // Logout from Drupal.
    user_logout();

    ...
   // Code to fetch client(s)

    return $client->endSession($post_redirect_url);
  }

And even this approach isn't 100% fool proof if you are connected to multiple clients...

I think you added too much code in your patch: the windows plugin :p
But other than that, the patch seems fine. Still figuring out how to sign out if you are connected to multiple clients though. It's a tough one.

The attached patch is a reroll of the patch on comment 10 updated to work on 8.x-1.x HEAD at c2d54a2 and modified to allow other clients to use the new functionality.

My additional changes:

- I added logic in OpenIDConnectRedirectController::redirectLogout() so that log outs by non-mapped users work (we have both). Previously it was throwing an error about the controller returning NULL rather than a response when you tried to logout with a non-mapped user.

- I added the two new fields to OpenIDConnectClientBase.php because they are a standard part of the OpenID Connect Session Management 1.0 spec, right? I'm not aware of anything specifically Windows-y about those parameters. It seems like a useful additional to the base client.

Instead of $collection->remove('user.logout'); and adding it to the routing yml I would just use:

    if ($route = $collection->get('user.logout')) {
      $route->setDefault('_controller', '\Drupal\openid_connect\Controller\OpenIDConnectRedirectController::redirectLogout');
   }

I used the patch in both #10 and #11 (since we already has the openid_connect_windows_aad module installed) and they worked like a charm to replace the Drupal logout with SSO logout. The post logout redirect url didn't actually redirect, but my suspicion is that it's due to our Azure config. I agree that the broader module should include an End Session Endpoint as part of the base configuration. Look forward to seeing those improvements added for Drupal 8!

Since comment 13 we learned that our provider needs the id_token_hint parameter passed as well. Attached is an updated patch that adds that parameter and incorporates the suggestion from comment 14.

Created child issue https://www.drupal.org/project/openid_connect/issues/3096855 and splitted patch

Renamed function getRedirectUrl() to getLogoutRedirectUrl() in the previous patch, due to adding this function in the dev branch of the module.

re-roll

Renamed function getRedirectUrl() to getLogoutRedirectUrl()

Thanks for the interdiff @slasher13.

+        if (method_exists($client, 'endSession')) {

Considering that endSession was added to the OpenIDConnectClientBase class, this will always be true. Check instead if the return of $client->getLogoutRedirectUrl() is not empty.

+        \Drupal::logger('openid_connect')
+          ->error('@provider plugin does not support log out. You are logged out of the site but not out of the OpenID Connect provider.', ['@provider' => $plugin_name]);

Can this be made into a warning, please? It makes no sense to have this error repeated by every site that doesn't use a logout URL.

+  public function endSession() {
+    $endpoints = $this->getEndpoints();
+    $post_logout_redirect_url = $this->getLogoutRedirectUrl();
+    $uri = $endpoints['end_session'] . '?post_logout_redirect_uri=' . $post_logout_redirect_url;

Can the rest of the endSession() function be inside a if (!empty($post_logout_redirect_url)) block? Return FALSE if no URI is returned. And move the $endpoints variable assignment inside the if block.

Oh, and assign this to one of you to avoid having duplicate patches being submitted.

#1 depends on end session endpoint
#2 done
#3 endsession() is only executed if end session endpoint exits

re-roll and diff to #20

#1 depends on end session endpoint
#2 done
#3 endsession() is only executed if end session endpoint exits

When I use patch #24, every time I refresh the settings form (/admin/config/services/openid-connect), I get these PHP notices:

Notice: Undefined index: end_session_endpoint in Drupal\openid_connect\Plugin\OpenIDConnectClientBase->buildConfigurationForm() (line 199 of /var/www/html/web/modules/contrib/openid_connect/src/Plugin/OpenIDConnectClientBase.php)

Notice: Undefined index: post_logout_redirect_url in Drupal\openid_connect\Plugin\OpenIDConnectClientBase->buildConfigurationForm() (line 204 of /var/www/html/web/modules/contrib/openid_connect/src/Plugin/OpenIDConnectClientBase.php)
#0 /var/www/html/web/core/includes/bootstrap.inc(600): _drupal_error_handler_real(8, 'Undefined index...', '/var/www/html/w...', 204, Array)
#1 /var/www/html/web/modules/contrib/openid_connect/src/Plugin/OpenIDConnectClientBase.php(204): _drupal_error_handler(8, 'Undefined index...', '/var/www/html/w...', 204, Array)
#2 /var/www/html/web/modules/contrib/openid_connect/src/Plugin/OpenIDConnectClient/OpenIDConnectFacebookClient.php(52): Drupal\openid_connect\Plugin\OpenIDConnectClientBase->buildConfigurationForm(Array, Object(Drupal\Core\Form\SubformState))
#3 /var/www/html/web/modules/contrib/openid_connect/src/Form/OpenIDConnectSettingsForm.php(155): Drupal\openid_connect\Plugin\OpenIDConnectClient\OpenIDConnectFacebookClient->buildConfigurationForm(Array, Object(Drupal\Core\Form\SubformState))
#4 [internal function]: Drupal\openid_connect\Form\OpenIDConnectSettingsForm->buildForm(Array, Object(Drupal\Core\Form\FormState))
#5 /var/www/html/web/core/lib/Drupal/Core/Form/FormBuilder.php(532): call_user_func_array(Array, Array)
#6 /var/www/html/web/core/lib/Drupal/Core/Form/FormBuilder.php(278): Drupal\Core\Form\FormBuilder->retrieveForm('openid_connect_...', Object(Drupal\Core\Form\FormState))

Also, I get a fatal error when trying to log out:

LogicException: The controller must return a response (null given). Did you forget to add a return statement somewhere in your controller? in Symfony\Component\HttpKernel\HttpKernel->handleRaw() (line 169 of /var/www/html/vendor/symfony/http-kernel/HttpKernel.php).

This Windows AAD uses what used to be the patch #11 and split into another issue, but I updated it to work with #24.

#3096855: Add OpenID Windows Azure AD client (Patch #5)

Also, it asks me to select an account to log out, even if there's only one logged in. This shouldn't happen.

I updated my comment #27 to reflect an updated attempt to incorporate the changes for Windows AAD.

#3096855: Add OpenID Windows Azure AD client

The issue linked adds Windows AAD into this module, and it incorporates the API changes from #24.

However, I still get the warnings on the settings page and the fatal error. The fatal error can be fixed by returning a Redirect, like return new RedirectResponse(Url::fromRoute('<front>')->toString());. However, if using the Autologin patch (see #3011413: Autologin when one client enabled), a redirect will immediately log the user back into the site (since the SSO client wasn't logged out).

I haven't figured out what's causing the warnings. Any ideas?

@solideogloria I think the warnings seen in #27 are because there's no hook_update_n() function to add the new configs to the existing plugins.

I tried the following and it didn't seem to help:

/**
 * Update the active config with end session endpoint and logout redirect url.
 */
function openid_connect_update_8106() {
  $config_factory = \Drupal::configFactory();
  $config = $config_factory->getEditable('openid_connect.settings');
  $config->set('end_session_endpoint', '');
  $config->set('post_logout_redirect_url', '');
  $config->save(TRUE);
}

However, using isset did work (!empty would also work):

    $form['end_session_endpoint'] = [
      '#title' => $this->t('EndSession endpoint'),
      '#type' => 'textfield',
      '#default_value' => isset($this->configuration['end_session_endpoint']) ? $this->configuration['end_session_endpoint'] : '',
    ];
    $form['post_logout_redirect_url'] = [
      '#title' => $this->t('Post-logout redirect URL'),
      '#type' => 'textfield',
      '#default_value' => isset($this->configuration['post_logout_redirect_url']) ? $this->configuration['post_logout_redirect_url'] : '',
      '#description' => $this->t('"/": A path within the current site.'),
    ];

Also, I don't know what the proper solution is for the redirect part. So someone else can provide a patch that incorporates it all.

Here's my guess at what the solution could be, using my suggestions from #30 and #32.

Yes, isset() is indeed better than empty(). I think the hook_update_n is still needed, and it should not be changing the openid_connect.settings, but about changing the following:

• openid_connect.settings.facebook
• openid_connect.settings.generic
• openid_connect.settings.github
• openid_connect.settings.google
• openid_connect.settings.linkedin

Also, please make an interdiff to #24, so the review can understand what changed.

I used ?? '' instead of isset, since PHP 7 is required for Drupal 8.

The Windows AAD issue that I opened for adapting to work with this issue's changes is here, along with a patch that works with #35:

#3170018: Prepare for OpenID Connect 2.x changes

re-roll

Logging out wasn't working for me unless I cleared my cache between each time. I added a no_cache option to the new route and it resolved this problem for me.

fixed patch from #38 and added interdiff.txt

Interdiff is highly appreciated. Thanks! Seems it still needs a re-roll.

re-roll

update okta settings, too

The patch in #42 has three problems I have encountered:

1. Storing token in private storage. OpenIDConnect::saveUserinfo() invokes hook_openid_connect_userinfo_save() while user has not been logged in. Provided hook_openid_connect_userinfo_save() example suggest tokens to be stored in user's private storage, but the private storage at this time is for anonymous user. Anything stored in anonymous user storage will not be available later on.
2. Token retrieval from private storage. OpenIDConnectRedirectController::redirectLogout() logs out the user from Drupal with user_logout() and then proceeds with calling $client->endSession() which is supposed to retrieve the token from user's private storage. After user is logged out, access to previously logged in user private storage is no longer available.
3. id_token_hint in OpenIDConnectClientBase::endSession() is set to <code>post_logout_redirect_uri value and not the token itself.

fix update hook

Do the notes in #43 need to be taken into account?

@solideogloria, I set it to Needs Work in #44 because of those comments in #43.

If you don't agree with them, it's also OK. But I'd appreciate some comments on them. Feel free to set it to 'Needs Work' if you agree with those comments. It seems to me that storing data in the private storage that is no longer accessible once the user logs in or out is not optimal here.

addresses #43 (1 and 2)

fixes #43.3

prevent early rendering

Needs a re-roll for version 2.x of the module. Version 1 is no longer getting new features. The good news is that none of the plugin instance config is needed, so half the #51 patch is now obsolete.

re-roll

fix service name

fix client usage

The patch in #51 applied the new properties to all plugins, but #55 only applies it to the generic plugin. Is there a reason for it?

If not, then move the schema change to the * section, and change the hook_update_8201() to add these properties to all existing config entities. I should probably create an helper function to get this, but it is a simple \Drupal::entityTypeManager()->getStorage('openid_connect_client').

I also think it would be simpler if instead of the logout redirect being a per-client setting, if this would be a global module setting. I do not see many scenarios where logging out by a Facebook user would be different than logging out from a Google user.

Fix update hook (update all clients).

I need the logout redirect per-client.

The hook_update is still not right.. You don't cycle through the plugins anymore.. You cycle the config entities. You can now have multiple 'services' attached to a plugin (most useful for the Generic plugin, but could also be useful for IDPs that can be used by different organizations such as Google or Okta).

What is your scenario where you need different redirect URLs per client? I want to simplify the module's options as much as possible, and I'd rather have a simple login and logout redirect URLs pair in the global module settings.

fixed update hook

I need different URLs for marketing purposes.

1. +++ b/src/Controller/OpenIDConnectRedirectController.php
   @@ -218,4 +219,40 @@ class OpenIDConnectRedirectController extends ControllerBase implements AccessIn
   +    $authmap = \Drupal::service('openid_connect.authmap');
   +    $account = \Drupal::currentUser();
   

   These will need to be injected.

   Please note that very soon the module will stop using it's own authmap and switch to the one provided by the externalauth module.

2. +++ b/src/Controller/OpenIDConnectRedirectController.php
   @@ -218,4 +219,40 @@ class OpenIDConnectRedirectController extends ControllerBase implements AccessIn
   +    return $response;
   

   Can you create an alter hook to alter the response also?

Thanks a lot for working on this. This will probably take a while to merge, since it is adding a new dependency on the authmap use that I want to replace alltogether. Also, I think when this gets committed, I'll strip out the per-client logout URL.

Mostly for two reasons:

1. Most people won't need a client-specific logout URL. And I like to keep modules as simple as possible giving flexibility for less-needed features to be done in custom code.
2. The client-specific logout URL should not be a part of the code of the plugins. Conceptually, the plugins deal only with the connection to the remote service, and this has nothing to do with that. One way that this could be solved is by adding this code to the new client entity class.

If you're OK with the logic above, and you think the alter hook could allow you to handle it from your side, that would be extremely helpful.

addresses #60 (1 and 2)

Wow! Thanks a lot!

Here a reroll patch.

src/Routing/RouteSubscriber.php is missing in patch file.

Hear a reroll patch.

use externalauth.authmap

Needs that the hook_update now be 8207 and some other things. Among others, the endsession endpoint must be moved to the generic client class, as most of the other client plugins have hardcoded or code-derived URLs, and those should follow the existing pattern.

I'm working on this already, so please don't implement the changes, as I only described above the main items I've changed, and there's a few details outstanding.

It was basically a rewrite, as the interdiff is about as big as the patch. The patch in #71 was still attached to the client plugin concept, and had to be transformed into using the client entities. This resulted in even moving the endSession code out of the client base as it wasn't really doing anything there.

No idea why the patch failed to apply in the test runner. It applied fine for me.

I've merged this. Thanks a lot everyone involved in this. Feel free to test it now that it's in the 2.x-dev branch. I'll create an alpha3 release soon, as this was one of the last major changes I wanted to add before doing that.