Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Currently when the user logs out of the site they are not logged out of the authentication service. This means that when they log in to the site again they are already logged in to the service, and so don't have to give their username or password. This has security implications on shared computers.
I have a fix to this which I will upload shortly. Please let me know if there are any issues with my report/patch; this is my first time reporting a Drupal issue.
N.B. This is the same issue but in Drupal 7.
Comment | File | Size | Author |
---|---|---|---|
#73 | 3061438-73.patch | 14.43 KB | jcnventura |
#73 | interdiff.txt | 14.41 KB | jcnventura |
Comments
Comment #2
loisovervoorde CreditAttribution: loisovervoorde at Mirum Agency commentedAttached is my attempt at a patch for this issue.
So far it only works with the Windows Azure AD plugin, which I have included in the patch. This plugin was modified from the OpenID Connect Windows Azure AD / B2C module.
I used logout-support-2882270-5.patch as a starting point for this patch, which is a patch for the same issue in the Drupal 7 version of this module.
Comment #3
loisovervoorde CreditAttribution: loisovervoorde at Mirum Agency commentedComment #4
robin.ingelbrecht CreditAttribution: robin.ingelbrecht at EntityOne commentedI 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
2. Added my own 'user/logout' route:
3. In the controller end the Drupal session and next redirect to end the session:
And even this approach isn't 100% fool proof if you are connected to multiple clients...
Comment #5
robin.ingelbrecht CreditAttribution: robin.ingelbrecht at EntityOne commentedComment #6
loisovervoorde CreditAttribution: loisovervoorde at Mirum Agency commentedThanks for your comment Robin, I'll take a look.
In the meantime, here is a patch that works for 8.6.
The main change from the previous patch is:
-use Drupal\Component\Utility\EmailValidatorInterface;
+use Egulias\EmailValidator\EmailValidatorInterface;
Comment #7
loisovervoorde CreditAttribution: loisovervoorde at Mirum Agency commentedI have had a go at implementing your method Robin, I'd appreciate your input if you have time!
(N.B. this patch is against the beta5 version because that's what we needed for a project I'm working on, apologies if this causes issues)
Comment #8
loisovervoorde CreditAttribution: loisovervoorde at Mirum Agency commentedComment #9
robin.ingelbrecht CreditAttribution: robin.ingelbrecht at EntityOne commentedI 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.
Comment #10
loisovervoorde CreditAttribution: loisovervoorde at Mirum Agency commentedThis patch does not contain the windows plugin.
Comment #11
loisovervoorde CreditAttribution: loisovervoorde at Mirum Agency commentedAnd here is a patch to the windows plugin.
Comment #12
loisovervoorde CreditAttribution: loisovervoorde at Mirum Agency commentedComment #13
floydm CreditAttribution: floydm at Affinity Bridge commentedThe 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.
Comment #14
tim_djInstead of $collection->remove('user.logout'); and adding it to the routing yml I would just use:
Comment #15
brayfe CreditAttribution: brayfe commentedI 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!
Comment #16
floydm CreditAttribution: floydm at Affinity Bridge commentedSince 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.
Comment #17
arash.nejati-rad CreditAttribution: arash.nejati-rad commentedCreated child issue https://www.drupal.org/project/openid_connect/issues/3096855 and splitted patch
Comment #18
kolesnikoff CreditAttribution: kolesnikoff commentedRenamed function getRedirectUrl() to getLogoutRedirectUrl() in the previous patch, due to adding this function in the dev branch of the module.
Comment #19
slasher13re-roll
Comment #20
slasher13Renamed function getRedirectUrl() to getLogoutRedirectUrl()
Comment #21
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedThanks for the interdiff @slasher13.
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.
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.
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.Comment #22
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedOh, and assign this to one of you to avoid having duplicate patches being submitted.
Comment #23
slasher13#1 depends on end session endpoint
#2 done
#3 endsession() is only executed if end session endpoint exits
Comment #24
slasher13re-roll and diff to #20
#1 depends on end session endpoint
#2 done
#3 endsession() is only executed if end session endpoint exits
Comment #26
slasher13Comment #27
solideogloria CreditAttribution: solideogloria commentedWhen I use patch #24, every time I refresh the settings form (/admin/config/services/openid-connect), I get these PHP notices:
Also, I get a fatal error when trying to log out:
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)
Comment #28
solideogloria CreditAttribution: solideogloria commentedAlso, it asks me to select an account to log out, even if there's only one logged in. This shouldn't happen.
Comment #29
loisovervoorde CreditAttribution: loisovervoorde at Mirum Agency commentedComment #30
solideogloria CreditAttribution: solideogloria commentedI 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?
Comment #31
jcnventura CreditAttribution: jcnventura at 1xINTERNET commented@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.Comment #32
solideogloria CreditAttribution: solideogloria commentedI tried the following and it didn't seem to help:
However, using
isset
did work (!empty
would also work):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.
Comment #33
solideogloria CreditAttribution: solideogloria commentedHere's my guess at what the solution could be, using my suggestions from #30 and #32.
Comment #34
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedYes,
isset()
is indeed better thanempty()
. I think the hook_update_n is still needed, and it should not be changing the openid_connect.settings, but about changing the following:Also, please make an interdiff to #24, so the review can understand what changed.
Comment #35
solideogloria CreditAttribution: solideogloria commentedI used
?? ''
instead ofisset
, since PHP 7 is required for Drupal 8.Comment #36
solideogloria CreditAttribution: solideogloria commentedThe 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
Comment #37
slasher13re-roll
Comment #38
kdmcclel CreditAttribution: kdmcclel as a volunteer commentedLogging 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.
Comment #39
slasher13fixed patch from #38 and added interdiff.txt
Comment #40
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedInterdiff is highly appreciated. Thanks! Seems it still needs a re-roll.
Comment #41
slasher13re-roll
Comment #42
slasher13update okta settings, too
Comment #43
maijs CreditAttribution: maijs at Wunder commentedThe patch in #42 has three problems I have encountered:
OpenIDConnect::saveUserinfo()
invokeshook_openid_connect_userinfo_save()
while user has not been logged in. Providedhook_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.OpenIDConnectRedirectController::redirectLogout()
logs out the user from Drupal withuser_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.id_token_hint
inOpenIDConnectClientBase::endSession() is set to <code>post_logout_redirect_uri
value and not the token itself.Comment #44
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedComment #45
slasher13fix update hook
Comment #46
solideogloria CreditAttribution: solideogloria commentedDo the notes in #43 need to be taken into account?
Comment #47
jcnventura CreditAttribution: jcnventura at 1xINTERNET commented@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.
Comment #48
solideogloria CreditAttribution: solideogloria commentedComment #49
slasher13addresses #43 (1 and 2)
Comment #50
slasher13fixes #43.3
Comment #51
slasher13prevent early rendering
Comment #52
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedNeeds 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.
Comment #53
slasher13re-roll
Comment #54
slasher13fix service name
Comment #55
slasher13fix client usage
Comment #56
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedThe 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.
Comment #57
slasher13Fix update hook (update all clients).
I need the logout redirect per-client.
Comment #58
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedThe 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.
Comment #59
slasher13fixed update hook
I need different URLs for marketing purposes.
Comment #60
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedThese 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.
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:
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.
Comment #61
slasher13addresses #60 (1 and 2)
Comment #62
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedWow! Thanks a lot!
Comment #63
JeroenTComment #64
KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Association commentedComment #65
KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Association commentedHere a reroll patch.
Comment #66
KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Association commentedComment #67
KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Association commentedComment #68
slasher13src/Routing/RouteSubscriber.php is missing in patch file.
Comment #69
JeroenTComment #70
KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Association commentedHear a reroll patch.
Comment #71
slasher13use externalauth.authmap
Comment #72
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedNeeds 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.
Comment #73
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedIt 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.
Comment #75
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedNo 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.