By jim_at_miramontes on

This might be one of those issues that has been around the block a few times, but it still bugs me: If a logged-out user goes to user/logout, they get a 403 / "access denied" message. This is because the access_callback for user/logout is user_is_logged_in(), which of course fails if the user is not logged in, and so denies access to user_logout().

I understand that there are various modules out there that are meant to address this, but am I wrong in thinking that this is just basically the wrong thing, from at least the user experience perspective? A less principled person than I would think about hacking the user/logout entry in user_menu() so that it always allows user_logout() to run, and then modifying user_logout() into something like:

function user_logout() {
  global $user;
  if (isset($user->name)) {
     watchdog('user', 'Session closed for %name.', array('%name' => $user->name));

     module_invoke_all('user_logout', $user);

     // Destroy the current session, and reset $user to the anonymous user.
     session_destroy();
  }
  else {
	drupal_set_message("You are already logged out.", 'error');
  }
  drupal_goto();
}

but that, of course, would be wrong...

Anyway, do you get my point? Am I missing something? Would this obviously misguided hacking of core break anything? Thanks!

⋅ Categories: Drupal 7.x

Comments

vm’s picture

vm commented

I've never seen anything in my logs in over 6 years where an anon user browses to user/logout.

If you think a change to core is required it is best to start a discussion in the issue queue for core. Changes are made in D8 and backported from there.

jim_at_miramontes’s picture

jim_at_miramontes commented

The place I'm running into it is one where a user (a) leaves a browser window open on the site, (b) gets timed out on the back end for having been inactive too long, (c) returns to the browser after that long delay, and (d) without refreshing the page, tries to log out via a link to user/logout. It's surely not a high-frequency matter; after having looked into it, I was just surprised to see how it was handled. Whatev.

jaypan’s picture

jaypan
Location Victoria, BC
commented

The question does beg to be asked, how would anonymous users be getting to the logout page? Do you have links to 'logout' for anonymous users? If so, I would say that is not great for usability, and while you could also make the change your are proposing, you should also remove the links to the logout page for anonymous users.

Also, I'd use hook_user_logout() myself:

function mymodule_user_logout($account) {
  if ($account->uid) {
     session_destroy();
  }
  else {
    drupal_set_message('You are already logged out.', 'error');
  }
  drupal_goto();
}

This kills the session (which you'd be bypassing by calling drupal_goto() in hook_user_logout), and sets the message as you'd like. And this doesn't require hacking core.

Contact me to contract me for D7 -> D10/11 migrations.

jim_at_miramontes’s picture

jim_at_miramontes commented

See above for the use case. Meanwhile, your approach almost works: it's probably a good idea to move the messaging into mymodule_user_logout, but it's still necessary to change the access callback on user/logout, and user_logout needs a test to keep the watchdog statement from trying to get the value of $user->name if it doesn't exist. But thanks!

jaypan’s picture

jaypan
Location Victoria, BC
commented

user_logout needs a test to keep the watchdog statement from trying to get the value of $user->name if it doesn't exist.

If you'll notice, I didn't use $user->name (or rather $account->name), I used $account->uid, as $account->uid always exists:

if ($account->uid)

Looking at your example of why/when this happens, it does make sense to me. I think this would be worth the submission of a core patch to be honest. Although you may first want to check if one has already been submitted somewhere.

Contact me to contract me for D7 -> D10/11 migrations.

jim_at_miramontes’s picture

jim_at_miramontes commented

Re your use of $account->uid: True enough. However, the problem is that core's user_logout uses $user->name:

watchdog('user', 'Session closed for %name.', array('%name' => $user->name));

which throws a warning if $user has a null value.

In any case, thanks -- I'll look into the patch issue.