Session won't read if user is blocked.

Ends up logging out current user but with message saying now masquerading as.

CommentFileSizeAuthor
#15 masquerade-blocked_users-932814-15-7.x.patch2.52 KBgngn
#6 932814.6-masquerade_blocked_users_6.x.patch15.02 KBdeviantintegral
#6 932814.6-masquerade_blocked_users_7.x.patch14.13 KBdeviantintegral
#5 932814.5-masqurade_blocked_users.patch10.34 KBdeviantintegral
#4 masquerade_blocked_users_1.patch5.33 KBafreeman
#2 masquerade_blocked_users.patch819 byteshefox
masquerade_blocked_users.patch691 byteshefox
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

deekayen’s picture

deekayen CreditAttribution: deekayen commented
Status: Active » Needs work

How come you didn't pair the watchdog with a dsm?

hefox’s picture

hefox CreditAttribution: hefox commented
FileSize
masquerade_blocked_users.patch819 bytes

Copy and paste from another error condition where there is no drupal set message.

hefox’s picture

hefox CreditAttribution: hefox commented
Status: Needs work » Needs review
afreeman’s picture

afreeman CreditAttribution: afreeman commented
FileSize
masquerade_blocked_users_1.patch5.33 KB

Hefox's patch is a good start. I've rerolled it for the dev branch and added some code to remove menu items and quick links for blocked user accounts and to filter blocked users out of the options presented by the autocomplete fields in admin/settings/masquerade and the masquerade block. If this meets with everyone's approval I'll open an issue and roll a patch for D7.

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral commented
Version: 6.x-1.4 » 6.x-1.x-dev
FileSize
932814.5-masqurade_blocked_users.patch10.34 KB

I've rerolled this patch against HEAD. I also fixed a few minor issues. The attached patch contains:

a266968 Issue #932814: Prevent switching to blocked user accounts.
d945e9a Issue #932814: Fix minor code style issues.
609f06b Issue #932814: Don't deny access to a page when unable to switch accounts.
4aa3823 Issue #932814: Fix typo in dsm() when removing a blocked account from the list of quick switches.

I'll be working on a 7.x version in a moment.

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
FileSize
932814.6-masquerade_blocked_users_7.x.patch14.13 KB
932814.6-masquerade_blocked_users_6.x.patch15.02 KB

Here's a 7.x version of the patch. I also fixed an issue with the user admin form not properly handling removing blocked users in both versions.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

is this still an issue?

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented
Status: Needs review » Needs work

This is still a bug (for 7.x-rc4)

The patch from #6 looks OK but I guess is probably not applicable anymore.

+++ b/masquerade.module
@@ -757,6 +786,14 @@ function masquerade_switch_user($uid) {
+  ¶

Whitespace

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Suppose we should not loose data on status change.
This check should be done as #2 and additionally clean-up Quick switches on render.
In case of form we just need to throw error message

wjaspers’s picture

wjaspers CreditAttribution: wjaspers commented

Just throwing in an additional idea in here.

What if a situation occurs where someone wants to masquerade as a "blocked" user; but wants to restrict this functionality to a handful of trusted accounts?

Would an additional permission make sense? Is this a realistic scenario?

Those that don't have the permissions applied would see the error page; those that do, would be able to masquerade as the blocked account.

Taxoman’s picture

Taxoman CreditAttribution: Taxoman commented

#10: if a user account is blocked, it cannot be logged into, hence what is there to check? There is no scenario as there is no way to get into that user account during blocked state, and Masquerade would only receive the default error message you get if a login fails...

hefox’s picture

hefox CreditAttribution: hefox commented

Since session handling can be overridden (memcache for example), it's possible to override the inability to login to blocked users.. but wow, that'd be bad. Don't think masquerade should support that extremely specific use case.

ResidentR6’s picture

ResidentR6 CreditAttribution: ResidentR6 commented

So after 4 years a bug is still here. There's a simple solution: DO NOT allow to found blocked user. Just hardcode a filter over request.
What's wrong with this "minor" bug: when you step on it, you will be thrown out of AMDIN session, not the user you tried to masquerade.

noopal’s picture

noopal CreditAttribution: noopal commented
Issue summary: View changes

Hi, its still here another 3 years later :p.
Sometimes I pick a random user to see how a page looks to non-admins, and just get logged out.

gngn’s picture

gngn CreditAttribution: gngn at Computer Manufaktur GmbH commented
Status: Needs work » Needs review
FileSize
masquerade-blocked_users-932814-15-7.x.patch2.52 KB

New patch applying to current 7.x-1.x-dev and 7.x-1.0-rc7

Includes:

  • Require users.status = 1 in masquerade_autocomplete() and masquerade_autocomplete_multiple(), so you do not get any blocked users via autocomplete.
  • Validate user's status in masquerade_block_1_validate(), so you will get a form validation error if you entered a blocked user.
  • Check user's status in masquerade_switch_user().

I do not think that we need to log to watchdog (above patches did).

I have one question: the other patches checked user's status like:

if (!empty($new_user->uid) && empty($new_user->status)) { ... }

What's the purpose of !empty($new_user->uid) &&?
My patch just checks empty($new_user->status).