Maintaining our own logout function is duplicitous. It's also error prone, as in
cas_logout()</a> we call
<code>
$edit = array();
user_module_invoke( 'logout', $edit, $user);
which is incorrect. Instead, the call should be
module_invoke_all('user_logout', $user);
(This can be verified by looking at the arguments to hook_user_logout() or by reading through the similar user_logout().
Instead we should implement hook_user_logout(). This would eliminate the need to have two logout links (/logout and /caslogout). The redirection functionality could be maintained through a clever use of hook_drupal_goto_alter(), although I'd advocate depreciating the logout redirection option in the CAS module and instructing users to configure a logout option via the more general trigger.module anyway.
| Comment | File | Size | Author |
|---|---|---|---|
| #21 | cas-n1018904-21.patch | 434 bytes | damienmckenna |
| #9 | 546322-cas-user-logout.patch | 1014 bytes | bfroehle |
| #4 | 1018904-4.patch | 404 bytes | bfroehle |
Comments
Comment #1
metzlerd commentedNot sure I agree here, but am willing to chat. I use the drupal (vs. cas) logout all the time as a mechanism for testing, and redirecting to cas logout causes you to log completely out of cas and affects all other apps you're signed into.
We could make a setting and it might still go a long way toward simplifying the code, and would also allow for the logging out of CAS to affect all sites, but again it would have to behave differently depending on external vs internal account designation. Lets tackle this when we remove INTERNAL option ok? (It will be much easier to get right).
Comment #2
bfroehle commentedI agree the Drupal logout vs. CAS logout is great for testing. However, that could be easily solved in other ways (e.g., a devel-type module which just clears your session).
It's been my experience that my users generally expect that clicking 'logout' in a CAS-ified application fully logs them out of CAS. (Explaining the distinction of being logged into a site vs. logged in globally to a CAS server is something that generally goes way over their heads).
I'll add a note in #1011876: CAS users cannot change their password or e-mail address to see this issue as well. Marking as postponed, for now.
Comment #3
bfroehle commented~
Comment #4
bfroehle commentedWhile this issue is postponed until we tackle #1011876: CAS users cannot change their password or e-mail address, we should still fix the issue in the original post, namely the hook_user_logout call is incorrect.
The attached patch fixes cas_logout to mimic user_logout.
Comment #5
metzlerd commentedAgreed straightfoward, committed.
Comment #6
bfroehle commentedSetting back to postponed, for future discussion of depreciating /caslogout.
Comment #7
metzlerd commentedFYI: There's also the issue that doing this in a user_logout hook, may make other pages not fire correctly, because there is a redirect that needs to happen in order to properly accomplish this. So this tradeoff between doing this this way, and dealing with problems of module weights to make sure other hooks get fired.
I'm also not sure it will work properly because it sets up a module weight confilict of interest. Cas needs to be fired first for login and last for logout, but hooks fire in order of module weight. We can't have the weight being both ways here.
Short is, I'm still unconvinced that changing the way that this behaves is the right thing to do. I always move/hide the logout menu and promote the cas one instead so that I get controlled and predictable behavior.
Comment #8
bfroehle commentedOh interesting... didn't think of the module weight issue. Hmm.
D7 has a lot of hooks which may make this feasible. For example, maybe hook_user_logout sets a SESSION variable marking that we need to redirect to the logout screen. Then hook_drupal_goto_alter() checks for the presence of the SESSION variable and redirects at the last possible minute.
In D6, I don't see a clear way to do it.
Thanks for all your work, as always! :)
Comment #9
bfroehle commentedIt wasn't particularly intuitive for me to know to add the special CAS login / CAS logout links on my first installation. Especially since CAS login can work with the basic user login page, leaving you with no mechanism to log out of CAS until you discover the menu items.
This is not the only time this has been suggested, see also:
#144879: Tap into logout op of hook_user
#546322: hook_user for cas logout
This is totally true. Another way to accomplish this would be to alter the menu item for 'logout' to be a custom function which handles both CAS and non-CAS users. It could be as simple as the attached patch, or as complicated as an option for the following:
Please note that this issue is still at "postponed" -- I have no desire to fix this before 6.x-3.0 or 7.x-1.0.
Comment #10
bfroehle commented#732542: system_goto_action breaks core APIs is considering adding 'skip destination' and 'delayed' parameters to drupal_goto, which may make this issue solvable (and would also clean up a lot of our
unset($_REQUEST['destination'])code).Comment #11
bfroehle commentedWhat if instead of removing /caslogout we instead just tweak the behavior of the regular logout to print a message reminding users that they may want to also log out of CAS...e.g., "You have been logged out of Drupal. Log out of CAS."
Then we could keep two parallel tracks:
/caslogout -> full logout, including CAS
/logout -> drupal logout, plus message reminding you to log out of CAS
Comment #12
metzlerd commentedCreative! I like it, particularly if we include a link to the cas logout site to facilatate logout.
Comment #13
bfroehle commentedRetitling and changing to as task. No idea if this is even feasible (as caching might get in the way). We'll find out, I guess.
Comment #14
liam morlandSee also #1454682: Allow cas_logout() to be called from hook_user_logout().
Comment #15
bfroehle commentedComment #17
dobie_gillis commentedIs this patch still relevant? I have a problem with CAS v7.x-1.x-dev where a CAS user gets logged out of Drupal, but they're never logged out of CAS. After some reading, it looks like this function handleLogoutRequests() handles CAS logouts (since phpCAS 1.0). Should we be calling this function?
Comment #19
damienmckennaHow about logging out the user account and redirecting the user to the CAS logout path to end their session?
Comment #20
metzlerd commentedI'm not quite sure what's being asked here. THis is the behaviour of the current caslogout menu item that you can enable if no logout destination is set.
Comment #21
damienmckennaHow about something like this? If someone loads /user/logout it'll first let Drupal logout the user normally and then bounce the user to the CAS server to log them out.
Comment #23
bkosborneThe problem with the approach in #21 is discussed above. Doing so "short circuits" the logout hook because it redirects the user to the CAS server. All other implementations of hook_user_logout would not run, unless they executed first.