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.
As for D8, the patch in #19 was committed and currently D7 is targeted in this issue.
History
- #822120: Get rid of masquerading as anonymous (sort of)
- #705858: Don't create session var when not masqerading
- #1813696: Set/unset Session flag when masquerade status changes.
Details
- 8.x-2.x no longer supports masquerading as Anonymous. That facility required tons of custom code and hacks for little benefit. One can simply log out instead, or use a separate browser. See #1836516: Port Masquerade to D8 and rewrite + simplify it into a new 2.x series
- The {masquerade} database table was originally introduced with the initial commit for Drupal 4.6.x: http://drupalcode.org/project/masquerade.git/history/HEAD:/masquerade.mysql
- We're setting the 'masquerading' flag on a user's session (when actively masquerading) and the flag is removed when unmasquerading.
masquerade_init()
performs a database query against {masquerade} on every page request. Studying the above historical issues reveals that this was only required to support masquerading as anonymous.
Proposed solution
- Since masquerading as Anonymous is gone, drop the table and all queries to it entirely.
Comment | File | Size | Author |
---|---|---|---|
#31 | masquerade-remove_masquerade_table_and_rely_on_session-d7-1926074-31.patch | 15.39 KB | joelpittet |
Comments
Comment #1
andypostAgreed exactly on removal of the table. Also if someone will try to introduce anonymous functionality then D8 state() could be used for.
Comment #2
andypostIt seem that it works
Comment #3
sunThanks - looks great :)
For this removal, we need to write some sophisticated test coverage though. The session property must not leak into other sessions and needs to be properly cleaned up.
Comment #4
andypostNot sure I get how sessions could leak...
patch adds upgrade function to remove table. Suppose it's ok to commit
Comment #6
sunRe-rolled against HEAD.
There were some more instances of queries against the table left. Also corrected the module update phpDoc summary.
Btw, I cannot see why the custom module weight is required. I suspect that's obsolete, too.
Comment #8
sun#6: masquerade.patch queued for re-testing.
Comment #9
andypostYes this should be dropped
Suppose this is a remains of hook_init()
Comment #10
sunAlright, now with dedicated test coverage.
Comment #12
sunHrm... run-tests.sh does not have a session (at all).
That wasn't exactly trivial to figure out.
Comment #14
sunRe-rolled/merged.
Comment #15
andypostCommit collision
A kind of magic... but works!
Comment #16
sunThanks! Committed and pushed #14.
Comment #17
andypostThis should be backported, probably we need a 7.x-2.x branch
Comment #18
sunThat's probably a good idea. — Although I'd recommend to wait with the backport to 7.x-2.x.
Most likely, it will make more sense to branch 8.x-2.x into 7.x-2.x.
Comment #20
hgoto CreditAttribution: hgoto as a volunteer commentedIt's so late but I created a patch to backport this to D7. The test passes in my local environment. I'd like someone to review this.
This backport needs to use php
$_SESSION
and the issue #2226531: Login hooks can't detect masqueraded user with $_SESSION['masquerading'] is (should be) also fixed with this patch.As andypost and sun told, I expected this patch to be used for 7.x-2.x branch that doesn't support masquerading as Anonymous. Thank you.
Comment #21
hgoto CreditAttribution: hgoto as a volunteer commentedComment #22
hgoto CreditAttribution: hgoto as a volunteer commentedI didn't fix the hook_help() message in the patch #20. I updated the patch #20 to update the hook_help() message following the andypost's comment on #2226531: Login hooks can't detect masqueraded user with $_SESSION['masquerading'].
Comment #23
das-peter CreditAttribution: das-peter at Cando commentedRe-roll
Comment #24
joelpittetThis seems to work very well and one less sql query on the page is a nice win for lack of anonymous masquerading which as the issue summary states is not needed.
Comment #25
andypostThanx a lot for this hard work, I'd like to get other maintainer opinion before starting new branch.
I bet we should remove this hook...
This is only unclear hunk for me
I think there's no such cache in d7 anymore and there was issue to remove it.
Moreover I'm sure there should be unset() for session but on logout session is cleared by core.
The only reason for this hook was to clear session from table that we are removing
Comment #26
andypostbtw in D6 this table
cache_menu
has keys that starts fromlinks:%
In D7
An so on... let's remove this hunk before creating new branch
Comment #27
hgoto CreditAttribution: hgoto as a volunteer commentedThank you for updating and reviewing the patch!
Following andypost's feedback, I deleted the hook implementation
masquerade_user_logout()
. There was another call ofcache_clear_all($user->uid, 'cache_menu', TRUE)
and I removed it, too.After applying this patch, I tested the function again in my environment and it looks to work well.
(Maybe, the points changed here are small and there's no need to move it from RTBC to NR again...?)
Comment #28
andypost@hgoto Thanx a lot for clean-up! makes a lot of sense
Assigned to maintainer to get opinion about start new branch or keep in 7.x-1.x
Comment #29
hgoto CreditAttribution: hgoto as a volunteer commentedThanks, @andypost.
I'll update the issue summary a little to reflect the current status.
Comment #30
andypostComment #31
joelpittetWhen saving a user form this came up.
Fatal error: Call to undefined function masquerade_user_submit() inincludes/form.inc on line 1520
I'm guessing that was intended to be removed, if not LMK, but here's a patch without the callback that was deleted.
Comment #32
hgoto CreditAttribution: hgoto as a volunteer commented@joelpittet thanks for pointing the error. Surely it must be removed. I myself want to move this to RTBC but I'd like someone else to do it :)
Comment #33
joelpittetThanks @hgoto for confirming, yeah I don't see why this couldn't be RTBC. I'm using it in production.
Comment #34
andypostRTBC and #28 again
Comment #35
joelpittetComment #36
izmeez CreditAttribution: izmeez commentedThis has been RTBC with remaining question from #28 whether to commit to 7.x-1.x-dev or start a new branch.
+1 to just commit to current branch.
Comment #37
izmeez CreditAttribution: izmeez commentedAdding this to #3010095: Masquerade 7.x-1.0 stable release plan
Comment #38
solideogloria CreditAttribution: solideogloria commentedI think committing to the current branch makes sense. There's no need for another branch when 7.x-1.0 hasn't been released yet. This issue is six years old, so I'd say just merge it.
Comment #39
ciss CreditAttribution: ciss at yousign GmbH commented@andypost Are there any remaining objections that prevent the patch from getting committed?
Comment #40
andypostI see no, except proper schedule of release
I've no d7 dites anymore to test it
Comment #41
ciss CreditAttribution: ciss at yousign GmbH commentedWell, there's always simplytest.me as well as the included web tests. Are you planning to hold out on committing all planned issues until you prep the release?
Comment #42
solideogloria CreditAttribution: solideogloria commentedAll you have to do is setup a vanilla D7 instance and enable masquerade. It's not that hard.
Comment #43
andypostIt's not about testing functionality (there's tests for that) but about upgrade path!
There's 50k existing installs which may need to drop the table from their database and nobody knows how many custom code is written around this table, that's why I'm waiting other maintainer's opinion
Comment #44
solideogloria CreditAttribution: solideogloria commentedI guess it's possible that someone might have created a block or something that should show a list of all masquerading users. Removing the table removes any possibility of that, right, since there would be no way to track that?
Comment #45
andypostYes, moreover right after upgrade all sessions of users which are masquerading before upgrade will be in strange state
Comment #46
ciss CreditAttribution: ciss at yousign GmbH commentedSounds to me like the issue should be left out of the initial 1.0 release then (given that there's already an RC), and committed as part of a 1.1 or even 2.0 release. Perhaps we should also set this issue back to Needs Review in order to evaluate which remaining cases have to be considered?
Comment #47
andypostNo reason to NW but give a time to use the new patch to existing sites
Comment #48
solideogloria CreditAttribution: solideogloria commentedBreaking schema changes should be made in a separate branch in my opinion, though I doubt many sites would directly access the table like that.
Comment #49
izmeez CreditAttribution: izmeez commentedI would like to see this patch committed along with other patches as described in #3010095: Masquerade 7.x-1.0 stable release plan where I identified that the order in which the patches are applied is important. Maybe others would be willing to review that list and maybe the whole bunch can be committed for the 7.x-1.0 release before the New Year. Thanks.
Comment #50
solideogloria CreditAttribution: solideogloria commentedHere is an example query I have. Would there be a way to do this without the
masquerade
table?This gets the UID of the original currently masquerading user.
For example, if you want to check for access based on the original access of the masquerading user, you need to be able to determine who the original user is.
Edit: Actually, I noticed that this query is the same as the one in masquerade_init(). I think it's the same to just use this: