As for D8, the patch in #19 was committed and currently D7 is targeted in this issue.

History

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

  1. Since masquerading as Anonymous is gone, drop the table and all queries to it entirely.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Agreed exactly on removal of the table. Also if someone will try to introduce anonymous functionality then D8 state() could be used for.

andypost’s picture

Status: Active » Needs review
FileSize
3.78 KB

It seem that it works

sun’s picture

Thanks - 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.

andypost’s picture

FileSize
4.07 KB

Not sure I get how sessions could leak...
patch adds upgrade function to remove table. Suppose it's ok to commit

Status: Needs review » Needs work

The last submitted patch, 4.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
4.61 KB

Re-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.

Status: Needs review » Needs work

The last submitted patch, masquerade.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

#6: masquerade.patch queued for re-testing.

andypost’s picture

FileSize
4.5 KB
+++ b/masquerade.installundefined
@@ -6,42 +6,17 @@
+function masquerade_install() {
+  module_set_weight('masquerade', -10);

Yes this should be dropped
Suppose this is a remains of hook_init()

sun’s picture

FileSize
8.29 KB

Alright, now with dedicated test coverage.

Status: Needs review » Needs work

The last submitted patch, masquerade.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
8.63 KB

Hrm... run-tests.sh does not have a session (at all).

That wasn't exactly trivial to figure out.

Status: Needs review » Needs work

The last submitted patch, masquerade.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
9.25 KB

Re-rolled/merged.

andypost’s picture

FileSize
8.72 KB

Commit collision

+++ b/lib/Drupal/masquerade/Tests/MasqueradeTest.phpundefined
@@ -107,6 +112,87 @@ class MasqueradeTest extends WebTestBase {
+        $old_session = isset($_SESSION) ? $_SESSION : NULL;
+        // Furthermore, if this test is executed on the command line, then
+        // Drupal denies to start a session. PHP throws a notice if the session
+        // is attempted to be started more than once.
+        // @see drupal_session_start()
...
+        // In any case, ensure that it is empty.
+        $_SESSION = array();

A kind of magic... but works!

sun’s picture

Status: Needs review » Fixed

Thanks! Committed and pushed #14.

andypost’s picture

Version: 8.x-2.x-dev » 7.x-1.x-dev
Status: Fixed » Patch (to be ported)

This should be backported, probably we need a 7.x-2.x branch

sun’s picture

That'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.

  • Commit da72f20 on 8.x-2.x, 8.x-2.x-admin-menu by sun:
    - #1926074 by andypost, sun: Remove {masquerade} table and rely on...
hgoto’s picture

It'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.

hgoto’s picture

Status: Patch (to be ported) » Needs review
hgoto’s picture

I 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'].

PS: also if get rid of this property (+1 to that) we need to update hook_help() that mention about that

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

This 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.

andypost’s picture

Thanx 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...

+++ b/masquerade.module
@@ -364,16 +315,11 @@ function _masquerade_user_load($username) {
 function masquerade_user_logout($account) {
-  if (!empty($account->masquerading)) {
+  if (!empty($_SESSION['masquerading'])) {
     global $user;
     cache_clear_all($user->uid, 'cache_menu', TRUE);
-    $real_user = user_load($user->masquerading);
+    $real_user = user_load($_SESSION['masquerading']);
     watchdog('masquerade', "User %user no longer masquerading as %masq_as.", array('%user' => $real_user->name, '%masq_as' => $user->name), WATCHDOG_INFO);
-
-    $query = db_delete('masquerade');
-    $query->condition('sid', session_id());
-    $query->condition('uid_as', $account->uid);
-    $query->execute();
   }
 }

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

andypost’s picture

btw in D6 this table cache_menu has keys that starts from links:%

| links:primary-links:page-cid:admin/user/user:1                                                     |
| links:primary-links:page-cid:admin/user:1   

In D7

| admin_menu:1:z6l9VF-2m2L2MWkcZcMMf-ZgfstckxXgsKDEh6NMK2U:en                                                |
...
| links:main-menu:page:admin/config/regional/language:en:1:1                                                 |
| links:main-menu:page:admin/config/regional/language:fr:1:1                                                 |

An so on... let's remove this hunk before creating new branch

hgoto’s picture

Thank you for updating and reviewing the patch!

Following andypost's feedback, I deleted the hook implementation masquerade_user_logout(). There was another call of cache_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...?)

andypost’s picture

Assigned: Unassigned » deekayen
Status: Needs review » Reviewed & tested by the community

@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

hgoto’s picture

Issue summary: View changes

Thanks, @andypost.

I'll update the issue summary a little to reflect the current status.

andypost’s picture

joelpittet’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
381 bytes
15.39 KB

When 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.

hgoto’s picture

@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 :)

joelpittet’s picture

Assigned: deekayen » Unassigned

Thanks @hgoto for confirming, yeah I don't see why this couldn't be RTBC. I'm using it in production.

andypost’s picture

Assigned: Unassigned » deekayen
Status: Needs review » Reviewed & tested by the community

RTBC and #28 again

joelpittet’s picture

Issue tags: +Performance
izmeez’s picture

This 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.

izmeez’s picture

solideogloria’s picture

I 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.

ciss’s picture

@andypost Are there any remaining objections that prevent the patch from getting committed?

andypost’s picture

I see no, except proper schedule of release

I've no d7 dites anymore to test it

ciss’s picture

I've no d7 dites anymore to test it

Well, 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?

solideogloria’s picture

All you have to do is setup a vanilla D7 instance and enable masquerade. It's not that hard.

andypost’s picture

It'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

solideogloria’s picture

I 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?

andypost’s picture

Yes, moreover right after upgrade all sessions of users which are masquerading before upgrade will be in strange state

ciss’s picture

Sounds 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?

andypost’s picture

No reason to NW but give a time to use the new patch to existing sites

solideogloria’s picture

Breaking schema changes should be made in a separate branch in my opinion, though I doubt many sites would directly access the table like that.

izmeez’s picture

I 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.

solideogloria’s picture

Here 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.

global $user;
return db_query("SELECT m.uid_from FROM {masquerade} m WHERE m.sid = :sid AND m.uid_as = :uid_as ", array(
    ':sid' => session_id(),
    ':uid_as' => $user->uid,
  ))->fetchField();

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:

return $_SESSION['masquerading'] ?? NULL;