Problem/Motivation

Currently Drupal has no unified API that allows safe user impersonation in code. This functionality is often needed by core and contrib and each module implements it in different ways some of them not secure. This may be considered a security improvement since:

  1. It can help people avoid unsafe impersonation constructs as outlined in Safely Impersonating Another User.
  2. It makes it possible to nest multiple impersonation levels without having the innermost impersonation logic incorrectly re-enable session saving when finished (see #21)

Proposed resolution

Add an AccountSwitcher class with the two methods switchTo($account) and switchBack(). Streamline the use of account "impersonations" throughout core (cron).

Remaining tasks

Commit.

User interface changes

None

API changes

API addition: a new service with two new methods for safely impersonating an account.

Original report by @drewish

I'm trying to write tests for file.inc and several of the validation functions need to switch to a uid=1 and non-uid=1 users. I know about DrupalWebTestCase::drupalLogin() but it affects the web browser's user not the global $user;. chx pointed me toward [#218104] which provided the method for doing this in tests:

  /**
   * Test the file_validate_extensions() function for the root user.
   */
  function testFileValidateExtensionsUid1() {
    global $user;
    $original_user = $user;
    session_save_session(FALSE);
    $user = user_load(array('uid' => 1));

    // Run these test as user 1
    $file = new stdClass();
    $file->filename = 'asdf.txt';
    $errors = file_validate_extensions($file, 'asdf txt pork');
    $this->assertEqual(count($errors), 0, t("Valid extension accepted."));

    $file->filename = 'asdf.txt';
    $errors = file_validate_extensions($file, 'exe png');
    $this->assertEqual(count($errors), 0, t("Invalid extension also accepted -- they're uid 1."));

    $user = $original_user;
    session_save_session(TRUE);
  }

  /**
   * Test the file_validate_extensions() function for the root user.
   */
  function testFileValidateExtensionsUidNot1() {
    global $user;
    $original_user = $user;
    session_save_session(FALSE);
    $user = $this->drupalCreateUser();

    // Run these test as a regular user
    $file = new stdClass();
    $file->filename = 'asdf.txt';
    $errors = file_validate_extensions($file, 'asdf txt pork');
    $this->assertEqual(count($errors), 0, t("Valid extension accepted."));

    $file->filename = 'asdf.txt';
    $errors = file_validate_extensions($file, 'exe png');
    $this->assertEqual(count($errors), 1, t("Invalid extension blocked."));

    $user = $original_user;
    session_save_session(TRUE);
  }

It's just enough code to be a pain and clutter up the tests. It seems like it be better to have DrupalWebTestCase::drupalSetUser() and ::drupalRestoreUser() or something like that to make this a little saner.

CommentFileSizeAuthor
#152 interdiff-287292-139-152.txt1.86 KBjacob.embree
#152 287292-152-impersonate-user.patch9.19 KBjacob.embree
#150 interdiff-287292-139-149.txt1.86 KBjacob.embree
#150 287292-149-impersonate-user.patch9.19 KBjacob.embree
#147 287292-147-impersonate-user.patch9.02 KBjacob.embree
#147 interdiff-287292-139-147.txt814 bytesjacob.embree
#139 Issue-287292-by-hass-Add-functionality-to-impersonat.patch8.88 KBhass
#124 287292-124-account_switcher.patch11.56 KBalmaudoh
#124 interdiff.txt1.94 KBalmaudoh
#122 287292-122-user-impersonation.patch11.53 KBalmaudoh
#122 interdiff.txt16.35 KBalmaudoh
#115 287292-115-user-impersonation.patch13.61 KBalmaudoh
#106 287292-106-user-impersonation.patch14.22 KBalmaudoh
#106 interdiff.txt1.6 KBalmaudoh
#104 interdiff.txt3.47 KBalmaudoh
#104 287292-103-user-impersonation.patch13.96 KBalmaudoh
#95 287292-95-user-impersonation.patch14.07 KBalmaudoh
#95 interdiff.txt1.88 KBalmaudoh
#91 287292-91-user-impersonation.patch14.11 KBznerol
#82 287292-69-82-interdiff.txt9.55 KBalmaudoh
#82 287292-82-user-impersonation.patch15.24 KBalmaudoh
#79 287292-79-user-impersonation.patch14.67 KBalmaudoh
#76 287292-69-76-interdiff.txt3.95 KBalmaudoh
#76 287292-76-user-impersonation.patch14.63 KBalmaudoh
#72 287292-69-72-interdiff.txt2.87 KBalmaudoh
#72 287292-72-user-impersonation.patch14.15 KBalmaudoh
#69 287292-64-69-interdiff.txt4.47 KBalmaudoh
#69 287292-69-user-impersonation.patch14.16 KBalmaudoh
#67 interdiff.txt4.43 KBalmaudoh
#66 287292-66-user-impersonation.patch14.11 KBalmaudoh
#64 287292-64-user-impersonation.patch12.06 KBalmaudoh
#56 287292-56-user-impersonation-do-not-test.patch9.93 KBalmaudoh
#44 287292-44-user-impersonation.patch11.5 KBmr.baileys
#43 287292-43-user-impersonation.patch15.3 KBmr.baileys
#39 287292-39-user-impersonate.patch10.34 KBmr.baileys
#34 user_impersonate2.patch10.53 KBBerdir
#32 user_impersonate2.patch9.69 KBBerdir
#29 user_impersonate.patch5.78 KBmr.baileys
#27 user_impersonate.patch5.8 KBmr.baileys
#25 user_287292_0.patch3.37 KBdawehner
#22 user_287292.patch3.15 KBdrewish
#17 user-switch-user.patch3.14 KBjpetso
#14 user_287292-14.patch3.05 KBfloretan
#13 user_287292.patch1.84 KBdrewish
#5 simpletest_287292.patch2 KBdrewish
#1 simpletest_switch_user.patch1.35 KBboombatower
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

boombatower’s picture

Assigned: Unassigned » boombatower
Status: Active » Needs review
FileSize
1.35 KB

This is an initial version, not sure this is how we want it to work. Do we want to accept UID or only user objects?

The restore function is only for looks, not sure if that is good style.

moshe weitzman’s picture

Status: Needs review » Needs work

the patch omits the important session calls ... why do we need to switch the user who is running the tests? why is it insufficient to switch the curl browser's user?

boombatower’s picture

The usefulness would be for running things "locally" in the test, not through the browser. (unit testing more so)

Perhaps the best solution would be to make drupalLogin switch the user locally as well for consistency.

drewish’s picture

or maybe calling it drupalSetLocalUser()? I also think that we shouldn't overwrite the original user if it's been set. I'd like to be able to do a couple of call like

$this->drupalSetLocalUser($root);
// do some tests...
$this->drupalSetLocalUser($normal_user);
// do some more tests...
$this->drupalRestoreLocalUser();
drewish’s picture

something more like this perhaps? the benefit would be that you could switch users in a setUp() and restore in a tearDown() and not worry about what happens in between.

moshe weitzman’s picture

IMO, the local user should not be used during testing, even for unit testing. We are now running very close to #283246: Node revisions test fails if admin has a personal timezone offset

drewish’s picture

Moshe, There's quite a bit of code in Drupal that depends on the current user for behavior and needs to tested and we need this functionality to do so. If you've got ideas on how to test functions like file_validate_extension() and file_validate_size() that vary their behavior based on the $user global I'd love to hear it. I don't think trying to test them by posting form to the upload.module is the right way to do that.

moshe weitzman’s picture

Status: Needs work » Needs review

This patch makes sense - I support it. We will always need to switch users, as you say.

boombatower’s picture

So does everyone like the way it works? Should we support UID and user object or just one or the other? Maybe just use drupalLogin to also switch local user?

moshe weitzman’s picture

WHat about putting user switch function into user module or common.inc instead of simpletest?

drewish’s picture

moshe that's actually a pretty good idea. i've got several modules where i use this functionality and in most of them (read: those done before this thread) i wasn't correctly calling session_save_session(). this is it's functionality that people need, and are likely to do wrong so from a security standpoint that would be a good argument for doing it right in core.

boombatower’s picture

Component: simpletest.module » user.module
Status: Needs review » Needs work

Moving.

drewish’s picture

Status: Needs work » Needs review
FileSize
1.84 KB

here's the patch for user.module. i also found and fixed a bug where is_int($user) was being tested rather than is_int($new_user).

floretan’s picture

Status: Needs review » Needs work
FileSize
3.05 KB

Re-rolled the patch, and fixed user_restore_user(), it was calling the non-existing function user_set_user() instead of user_switch_user().

Also added a test case to user.test. The test is pretty simple but currently fails. If I understood correctly, this is a typical case of what we want to be able to do, so we should get it working.

Test-driven development, Whoohoo!!

webchick’s picture

Subscribing. :)

jpetso’s picture

> The test is pretty simple but currently fails.

+    $this->assertEqual($user->uid, $this->original_user, t('Using original user.'));

(and the same assertEqual() after switching back) looks to me like it could rather look like

+    $this->assertEqual($user->uid, $this->original_user->uid, t('Using original user.'));

(with ->uid for $this->original_user). Also, assuming that there might be nested user_switch_user() calls - or is that an unreasonable assumption? - I think not much should be lost with making the static variable in user_switch_user() an array that appends and pops lists of users. Maybe that's not needed, though.

jpetso’s picture

FileSize
3.14 KB

Re-rolled the patch, and eliminated most of the problems:

- use drupal_save_session() instead of the (now) non-existent session_save_session()
- use is_numeric() instead of is_int() in order to make the "number vs. object" check work
- compare uid against uid instead of uid against user object, as mentioned in the above comment
- coding style: don't declare $this->original_user as instance variable, for consistency with other user tests

However, the test still fails as the DrupalWebTestCase->drupalLogin() and ->drupalLogout() functions do not set the global $user object.

moshe weitzman’s picture

Reading the code, the test fails because it compares the uid of the person running the test ($GLOBALS['user']->uid) with the user logged into the dummy site. See $this->assertEqual($user->uid, $new_user->uid. There are two web sites running in simpletest and it can be confusing which one you are dealing with.

I recommend not dealing with the drupalWebTestCase methods in the test. I think the test should just use the local install. Don't create any new users. So, here is a flow.

1. determine if the current user is uid=0. if not, switch to 0 and check that this worked. if yes, switch to 1 and check that this worked.
2. switch back and check that you are back to who you expect.

Dave Reid’s picture

Issue tags: +XML sitemap

Tagging since XML sitemap module maintains its own copy of these user functions until this is accepted into core (probably not likely until D8).

Dave Reid’s picture

Issue tags: +contrib dependency

Adding second tag for clarification.

mr.baileys’s picture

Assigned: boombatower » Unassigned

No longer applies to HEAD, so will need a slight re-roll.

+++ modules/user/user.module	(working copy)
@@ -2447,3 +2447,53 @@
+ * Set the current user stored in $GLOBALS['user'].

What about "Impersonates another user."? Should be third-person, and the actual implementation ($GLOBALS['user']) should not be part of the summary line.

+++ modules/user/user.module	(working copy)
@@ -2447,3 +2447,53 @@
+ * @param $new_user User to switch to, either UID or user object. If
+ *   nothing is provided the user will be reset to the original.
+ * @return Current user object.

The descriptions for the parameter and return values should go on a new line

+++ modules/user/user.module	(working copy)
@@ -2447,3 +2447,53 @@
+  static $user_original;

I think this should use drupal_static() instead.

+++ modules/user/user.module	(working copy)
@@ -2447,3 +2447,53 @@
+/**
+ * Restore the user that was originally loaded.
+ *
+ * @return Current user.
+ */

Same comments about the doxygen block: summary line should be third person, and the @return description should go on the next line.
The "returns current user" bit is confusing. Should be something like: "The user who was active before switching."

Also, assuming that there might be nested user_switch_user() calls - or is that an unreasonable assumption? - I think not much should be lost with making the static variable in user_switch_user() an array that appends and pops lists of users. Maybe that's not needed, though.

I would prefer the process of impersonating and reverting to be a LIFO stack. There are valid scenarios in which another function higher up in the stack has already impersonated a user. Take for example #431776: Cron should run as anonymous when invoked via the run-cron link on the status report page:

  1. drupal_cron_run() impersonates the anonymous user.
  2. custom module wants to node_delete() inside hook_cron() and impersonates a user with the sufficient permissions.
  3. custom module reverts the user => using the current patch, this will now revert to the user that was active before drupal_cron_run() switched to the anonymous user, and further hook_cron() implementations will no longer be running under the anonymous account.

The patch should also implement the user_switch_user() function wherever appropriate in core (drupal_cron_run() is one location, there might be more).

Also, I'd prefer more clear terminology on the function names (user_impersonate_user and user_revert_user maybe?)

Powered by Dreditor.

drewish’s picture

FileSize
3.15 KB

I totally agree with mr.baileys that we should have a stack of users but I ran out of time before implementing it.

drewish’s picture

Status: Needs work » Needs review

just want to get test bot feedback.

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.37 KB

Rerole and corrected patch syntax

Status: Needs review » Needs work

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

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
5.8 KB
  • Implemented the LIFO stack for nesting multiple impersonation calls. Each time user_impersonate_user() is called, the active user is pushed onto the stack before switching to the new user. Each time user_revert_user() is called, the most recent user is popped. Once no more users are on the stack (ie we are no longer impersonating a user), session saving is re-enabled.
  • Changed the doxygen comments for the functions to match the new approach, and some additional doxygen fixes to adhere to standards;
  • Changed the naming of the functions to "user_impersonate_user" and "user_revert_user", as these are more specific than "user_switch_user" and "user_restore_user";
  • Changed the tests so they run local, according to Moshe's instructions in #18, slightly altered to account for the nested impersonation calls;
  • Replaced the user impersonation done in drupal_cron_run with invocations of user_impersonate_user() and user_revert_user().

Status: Needs review » Needs work

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

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
5.78 KB

user_load() was called with an array instead of a uid (D6 syntax). This should make the testbot happy.

Berdir’s picture

#29: user_impersonate.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +XML sitemap, +contrib dependency

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
9.69 KB

Re-roll, this failed because of a trailing space fix that was applied elsewhere ;)

I also replaced all instances of manual user switching I could find.

The one in update.test could even a have pretty interesting impact as it is now, as it used $this->originalUser which is already used by simpletest so simpletest does not actually restore the original user but the one that update.test restored...

This will also make use of the user stack, since simpletest uses it for every test case and some tests use it again.

Let's see what the tests have to say about this.

Status: Needs review » Needs work

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
10.53 KB

Fixed three bugs...

- We can not use user_load() in UpgradeTestCase::setUP(). I've added a comment about that fact.

- Missed a switch in the file API test and fixed a typo...

- Since Simpletest uses user_impersonate_user() now too, the user impersonate tests did not work as expected anymore. This is actually also a bugfix, since user_impersonate_user() re-enabled session saving but Simpletest did not have it's origional user restored. Fixed and commented.

mr.baileys’s picture

Title: Add function to switch local user » Add function to impersonate a user
Category: feature » bug
Issue tags: +Security improvements

Tagging this as a security improvement since the addition of user_impersonate_user() and user_revert_user()

  1. … can help people avoid unsafe impersonation constructs as outlined in Safely Impersonating Another User
  2. … makes it possible to nest multiple impersonation levels without having the innermost impersonation logic incorrectly re-enable session saving when finished (see http://drupal.org/comment/reply/287292#comment-2873752)

The nested impersonations issue is also the reason why I think this is a bug rather than a feature request: the entire drupal_save_session(TRUE|FALSE) dance just doesn't cut it when you nest impersonations and in rare situations could lead privilege escalation*.

* unless you save the session saving state before impersonating and then restore that state with drupal_save_session($old_state) instead of drupal_save_session(TRUE); when done.

sanduhrs’s picture

Updated the code example on Safely Impersonating Another User to support nested impersonations as outlined above.
I tagged the node with Needs technical review, would be nice if someone else could confirm the solution and remove the tag.

Berdir’s picture

Version: 7.x-dev » 8.x-dev

Moving to the 8.x queue.

geek-merlin’s picture

good stuff, sub.

mr.baileys’s picture

Title: Add function to impersonate a user » Add a function to impersonate a user.
FileSize
10.34 KB

Re-rolled for 8.x-dev.

apaderno’s picture

#39: 287292-39-user-impersonate.patch queued for re-testing.

apaderno’s picture

Title: Add a function to impersonate a user. » Add a function to impersonate a user

Status: Needs review » Needs work

The last submitted patch, 287292-39-user-impersonate.patch, failed testing.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
15.3 KB

Re-rolled to keep up with HEAD

mr.baileys’s picture

Better patch -- I had cloned an existing test case and changed it, showing up in Git as changes to the cloned test case instead of a brand new one.

Status: Needs review » Needs work
Issue tags: -Security improvements, -XML sitemap, -contrib dependency

The last submitted patch, 287292-44-user-impersonation.patch, failed testing.

mr.baileys’s picture

Status: Needs work » Needs review
Issue tags: +Security improvements, +XML sitemap, +contrib dependency

#44: 287292-44-user-impersonation.patch queued for re-testing.

sanduhrs’s picture

Status: Needs review » Needs work
+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -622,7 +622,10 @@ abstract class TestBase {
+    // Impersonate the current user so it we can use user_revert_user() to

Wording: [...]so it we can[...]

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/FormatDateTest.phpundefined
@@ -103,11 +103,10 @@ class FormatDateTest extends WebTestBase {
     drupal_save_session(FALSE);

Is that still needed? seems to happen in user_impersonate_user() already.

hass’s picture

Any news on this case?

apaderno’s picture

Status: Needs work » Needs review
Issue tags: -Security improvements, -XML sitemap, -contrib dependency

#44: 287292-44-user-impersonation.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 287292-44-user-impersonation.patch, failed testing.

hass’s picture

Issue summary: View changes

Can someone re-role for D8 so that we do not miss final release, please?

almaudoh’s picture

Assigned: Unassigned » almaudoh

I need this for SMSFramework in D8. Will work on it.

Berdir’s picture

Note that this will have to be completely reworked for 8.x, maybe should live inside AccountProxy now?

almaudoh’s picture

Note that this will have to be completely reworked for 8.x, maybe should live inside AccountProxy now?

I agree. I'm not simply doing a re-roll of the previous patch (that would take longer). I'm actually rewriting based on the ideas in the patch.
Placing it inside AccountProxy makes sense, so we can create a new impersonateAccount() method. Or alternatively, add an optional $impersonate argument to the AccountProxy::setAccount() method so as to allow the stacking of accounts mentioned in #21, #22, #27 and #32.

almaudoh’s picture

Just to get the momentum going and for any reviews of the general approach. This patch fails at drupal install because \Drupal::currentUser() is called by FormBuilder and the injected session manager needs a database that doesn't yet exist.
Options to move forward:
1. Wait for #2238087: Rebase SessionManager onto Symfony NativeSessionStorage or #1530756: [meta] Use a proper kernel for the installer to land and see if it fixes the session storage issue with install_drupal()
2. Do we really need $this->sessionManager->disable()??? I'm thinking through that...
3. Any other ideas?

almaudoh’s picture

almaudoh’s picture

Issue summary: View changes
almaudoh’s picture

Title: Add a function to impersonate a user » Add functionality to impersonate a user
Component: user.module » user system
Issue summary: View changes
almaudoh’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Needs work

This certainly needs work in d8.

nicobot’s picture

Hi there,

I would like to make another proposal, instead of relying on the developer for doing the revert to previous user after concluding the actions needed; couldn't be better to try a different approach like this:

<?php

/**
 * (as an example) Do my action as anonymous 
 */
function my_action_as_another_user($param1) {

  $user = \Drupal::currentUser();

  // anonymous function
  $action = function() use ($param1) {

    // do something as an anonymous user
    $param1++;

    return $param1;
  };

  $action_result = $user->impersonateAccount(new AnonymousUserSession(), $action);

}

// .........................

class User {

  // .......

  function impersonate_action($user, $action) {

    try {

      $this->impersonation_change_user($user);

      $return = $action();

    } catch (Exception $e) {
      throw $e;

    }
  //  It would be good to use finally closure in case minimum requirements of Drupal8 was PHP 5.5
  //  finally {
    $this->revert_impersonation();
  //  }

    return $return;
  }
}

Thanks,
Nicolás

hass’s picture

That could be done as a followup if needed (I do not), but very first we need the basics done.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
12.06 KB

I updated the InstallerServiceProvider to replace the 'current_user' service with null implementation. Patch now installs. Let's see how this comes out...

Status: Needs review » Needs work

The last submitted patch, 64: 287292-64-user-impersonation.patch, failed testing.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
14.11 KB

Looked at the Drupal\simpletest\TestBase::prepareEnvironment() method again and found a mix of legacy and new code with regards to user impersonation. Fixed those. Another go at the testbot.

As an aside, TestBase::restoreEnvironment() doesn't do a really good job because it never persisted the previous Drupal container it had overridden in ::prepareEnvironment(). That may need another issue.

almaudoh’s picture

FileSize
4.43 KB

Sorry forgot the interdiff...:-)

Status: Needs review » Needs work

The last submitted patch, 66: 287292-66-user-impersonation.patch, failed testing.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
14.16 KB
4.47 KB

Status: Needs review » Needs work

The last submitted patch, 69: 287292-69-user-impersonation.patch, failed testing.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
    @@ -82,6 +106,52 @@ public function getAccount() {
    +    $this->setAccount($account);
    ...
    +    return $this->account;
    ...
    +    return $this->account;
    

    Let's return $this instead.

  2. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
    @@ -82,6 +106,52 @@ public function getAccount() {
    +    if (!empty($this->accountStack)) {
    +      $this->account = array_pop($this->accountStack);
    +    }
    

    I wonder whether we should throw an exception when the stack is empty?

  3. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
    @@ -82,6 +106,52 @@ public function getAccount() {
    +  /**
    +  /**
    

    double line

  4. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
    @@ -82,6 +106,52 @@ public function getAccount() {
    +      $this->account = $this->accountStack[0];
    

    Given how array_pop/push works I guess it would be safer to use array_shift instead of directly accessing [0]

almaudoh’s picture

Status: Needs work » Needs review
FileSize
14.15 KB
2.87 KB

Thanks @dawehner for the review.

  1. Done. Chaining - nice one :)
  2. I went with a \RuntimeException for now. Does this warrant creating another Exception class?
  3. Done.
  4. Done.

I still can't figure out the test fail on SessionTest->testSessionSaveRegenerate() - it's not failing on my system.

Status: Needs review » Needs work

The last submitted patch, 72: 287292-72-user-impersonation.patch, failed testing.

mradcliffe’s picture

Yeah, SessionTest isn't failing on my VM either.

Default value for static SessionManager::enabled is TRUE, and as far as I can tell after looking at NativeSessionStorage and SessionManager::__construct and core.services.yml, this is not being set to disabled when the object is returned.

almaudoh’s picture

...as far as I can tell after looking at NativeSessionStorage and SessionManager::__construct and core.services.yml, this is not being set to disabled when the object is returned...

It is set to FALSE in TestBase::prepareEnvironment() where the current_user impersonation is done at

    // Run all tests as an anonymous user by default, web tests will replace that
    // during the test set up.
    $this->container->set('current_user', \Drupal::currentUser());
    $this->container->get('current_user')->impersonateAccount(new AnonymousUserSession());

    \Drupal::setContainer($this->container);
almaudoh’s picture

Another re-roll to keep up to date with HEAD. Also fixed the UserImpersonatingUserTest so it actually runs - wasn't running before.

almaudoh’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 76: 287292-76-user-impersonation.patch, failed testing.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
14.67 KB

Re-rolled patch. Fixed the test. Bot still failing SessionTest while local system is passing. Troubleshooting...

Will not attach interdiffs again until it goes green.

Status: Needs review » Needs work

The last submitted patch, 79: 287292-79-user-impersonation.patch, failed testing.

almaudoh’s picture

The test fails have made me look deeper into the code. I've discovered all through core the use of "impersonations" at different places done in at least 3 different ways.

  1. $container->set('current_user', \Drupal\Core\Session\AccountInterface object)
  2. $container->set('current_user', \Drupal\Core\Session\AccountProxyInterface object)
  3. $container->get('current_user')->setAccount(\Drupal\Core\Session\AccountInterface or \Drupal\Core\Session\AccountInterface object)
  4. $container->get('current_user')->impersonateAccount(\Drupal\Core\Session\AccountInterface object) (introduced by this patch)

1) is only seen in Tests while 2) is introduced by this patch in TestBase. Between 3) and 4) (also introduced by this patch), there should be some documentation of best practice as to when they can be used. 3) should not need to be used except in tests also. Most contrib module developers won't need to use 1, 2, or 3.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
15.24 KB
9.55 KB

Found the cause of the one SessionTest fail. Fixed now and should go green. If it does go green, it would be nice to have comments on #81.

almaudoh’s picture

Issue summary: View changes
almaudoh’s picture

Assigned: almaudoh » Unassigned

Status: Needs review » Needs work

The last submitted patch, 82: 287292-82-user-impersonation.patch, failed testing.

mradcliffe’s picture

#2239969: Session of (UI) test runner leaks into web tests changed the way that tests work, and there's probably some significant rework on how that works or how impersonate will work in tests.

mradcliffe’s picture

xmlsitemap, footermap, site_map, etc... can sort of work around core not providing this functionality anymore by using the concept of manipulators in menu tree building.

1. Implement an anonymous user service to use the AnonymousUserSession class.
2. Implement a manipulator service that simple extends DefaultMenuLinkTreeManipulators, but uses the anonymous user session service above. It does not even need to extend any method.
3. Use the MenuTree builder service and use the manipulator service above so then it should transform the menu items.

I have found that there are menu routes in core that don't work so well such as the My Account menu link content, which will always be hidden to the anonymous user.

hass’s picture

I think I still need this in Link Checker as I need to update nodes via cron as an impersonated user.

znerol’s picture

Status: Needs work » Needs review
FileSize
14.11 KB

Reroll, left out the hunk in TestBase, that shouldn't be necessary anymore. Also the originalUser property in TestBase and WebTestBase seems to be orphaned, removing it also.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Looks nice and clean to me. Thanks @znerol.

mradcliffe’s picture

Status: Reviewed & tested by the community » Needs work

Looks good other wise. Next steps would be to create a change record or maybe update the documentation handbook regarding Impersonating Accounts instead? Also add in the menu transformation method listed earlier.

+++ b/core/modules/system/lib/Drupal/system/Tests/Session/UserImpersonatingUserTest.php
@@ -0,0 +1,95 @@
+    } else {

nitpick: else should be on a new line.

almaudoh’s picture

Small documentations nits:

  1. +++ b/core/lib/Drupal/Core/Session/AccountProxyInterface.php
    @@ -33,5 +33,34 @@ public function setAccount(AccountInterface $account);
    +   * Set the current wrapped account to impersonate another account.
    

    s/Set/Sets/

  2. +++ b/core/lib/Drupal/Core/Session/AccountProxyInterface.php
    @@ -33,5 +33,34 @@ public function setAccount(AccountInterface $account);
    +   * Revert from impersonating another account.
    

    s/Revert/Reverts/

Otherwise looks good to go in.

almaudoh’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.88 KB
14.07 KB

Fixed #93 and #94. Minor changes. Back to RTBC per #92.

almaudoh’s picture

Issue summary: View changes

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 95: 287292-95-user-impersonation.patch, failed testing.

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Testbot was broken.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
    @@ -84,6 +108,57 @@ public function getAccount() {
    +      throw new \RuntimeException(t('No more account impersonations to revert.'));
    ...
    +      throw new \RuntimeException(t('No more account impersonations to revert.'));
    

    We should not be using t() on exception messages.

  2. +++ b/core/modules/simpletest/src/WebTestBase.php
    --- /dev/null
    +++ b/core/modules/system/lib/Drupal/system/Tests/Session/UserImpersonatingUserTest.php
    

    psr-4? :) Also this means atm this patch has no explicit test coverage :)

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Session/UserImpersonatingUserTest.php
    @@ -0,0 +1,92 @@
    + * Contains Drupal\user\Tests\UserImpersonatingUserTest.
    

    This is wrong

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/Session/UserImpersonatingUserTest.php
    @@ -0,0 +1,92 @@
    +class UserImpersonatingUserTest extends WebTestBase {
    

    Why webtestbase? I'm sure this could be a KernelTestBase test.

almaudoh’s picture

Status: Needs work » Reviewed & tested by the community

#102.1: Fixed.
#102.2: Oops!! :) However, the tests were passing prior to the PSR-4 switch. Fixed.
#102.3: Fixed.
#102.4: Good point. Done.

Also adjusted the expectation in the test that the session saving status should initially be disabled - that cannot be assumed. New check is to verify that the session saving status was restored to initial status.

Back to RTBC since the changes were minor.

almaudoh’s picture

Sorry, forgot the patch...

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Session/AccountProxy.php
@@ -84,6 +108,57 @@ public function getAccount() {
+      throw new \RuntimeException('No more account impersonations to revert.');

+++ b/core/lib/Drupal/Core/Session/AccountProxyInterface.php
@@ -33,5 +33,34 @@ public function setAccount(AccountInterface $account);
+  /**
+   * Reverts from impersonating another account.
+   *
+   * @return \Drupal\Core\Session\AccountInterface
+   *   $this.
+   */
+  public function revertAccount();

I think it is pretty important that we document that the this is exception is thrown using the @throws doc. Since any code that calls this function is going to have to handle this.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
1.6 KB
14.22 KB

#105: Good point. Fixed.

Also updated AccountProxy::impersonateAccount() to not overwrite the very first session status in subsequent impersonate calls.

Status: Needs review » Needs work

The last submitted patch, 106: 287292-106-user-impersonation.patch, failed testing.

Status: Needs work » Needs review
alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Session/AccountProxyInterface.php
    @@ -33,5 +33,40 @@ public function setAccount(AccountInterface $account);
    +  public function revertAccount();
    ...
    +  public function revertAll();
    
    +++ b/core/modules/system/src/Tests/Session/UserImpersonatingUserTest.php
    @@ -0,0 +1,91 @@
    +      $user->revertAccount();
    ...
    +      $user->revertAll();
    

    These functions have odd names. Looking at it I'm wondering what we are reverting. Also there is no test coverage for the revertAll() method actually working.

  2. +++ b/core/modules/system/src/Tests/Session/UserImpersonatingUserTest.php
    @@ -0,0 +1,91 @@
    +    // If not currently logged in, use AccountProxy::impersonateAccount() to switch to
    +    // user 1. If logged in, switch to the anonymous user instead.
    +    if ($user->isAnonymous()) {
    +      $user->impersonateAccount(new UserSession(array('uid' => 1)));
    +    }
    +    else {
    +      $user->impersonateAccount(new AnonymousUserSession());
    +    }
    ...
    +    $this->assertEqual($user->id(), ($original_user->id() == 0 ? 1 : 0), 'User switched');
    ...
    +    $this->assertEqual($user->id(), ($original_user->id() == 0 ? 1 : 0), 'User switched.');
    

    This is a bit tricky since it means running tests through UI and the testbot are different code paths. Why not just use uids 2 and 3?

almaudoh’s picture

These functions have odd names...

Personally I think impersonateAccount() a bit too verbose myself. The current names originated from the procedural equivalents in #27.

Suggestions:
impersonateAccount() / revertImpersonatedAccount() / revertAllImpersonatedAccounts()
switchAccount() / restoreSwitchedAccount() / restoreAllSwitchedAccounts()

Any ideas?

almaudoh’s picture

Status: Needs work » Needs review

CNR so others can comment on #110 and also suggest names. Will post a patch at the end of the day.

znerol’s picture

From an architectural point of view, I do not like the fact that new responsibility is added to the AccountProxy. The proxy is designed to be a drop-in replacement for AccountInterface and - in most cases - also vice versa. In fact I believe that the getAccount(), setAccount() calls only should be used by a very restricted set of services (authentication) and all other code should assume that current_user is a plain account.

Therefore I suggest to introduce a new service impersonation (analogous to authentication) which implements the stack but calls to the account proxy whenever it is necessary to change the user.

almaudoh’s picture

From an architectural point of view, I do not like the fact that new responsibility is added to the AccountProxy. The proxy is designed to be a drop-in replacement for AccountInterface and - in most cases - also vice versa.

+1
Since most of the former use cases for account "impersonation" (mostly in tests) are now handled by AccountProxyInterface and the current_user service. It's only the stacked account functionality that's left.

introduce a new service impersonation (analogous to authentication) which implements the stack.

+1, however, I suggest account_stack as the service name since that reflects what it's actually doing (you can also do impersonation by AccountProxyInterface::setAccount() without maintaining track of past accounts).

znerol’s picture

you can also do impersonation by AccountProxyInterface::setAccount() without maintaining track of past accounts

Regrettably at the moment this is not save, except if the session writes are properly disabled before. The impersonation service would do that automatically (and also automatically would restore session writing afterwards).

almaudoh’s picture

New patch based on #112. I stuck with impersonation service name for now and the "impersonateAccount()/revertAccount()" nomenclature still remains - subject to bikeshedding.

Only use case left from the original patch is in Cron. No interdiff because it's a different approach.

znerol’s picture

Great work! I really like how this turned out.

No bike-shedding intended at all, I also have difficulties to find proper names for this functionality. Same for the service-name, so if you find a better name just use it.

  1. +++ b/core/lib/Drupal/Core/Session/AccountImpersonation.php
    @@ -0,0 +1,118 @@
    +   * @var \Drupal\Core\Session\SessionManager
    ...
    +   * @param \Drupal\Core\Session\SessionManager $session_manager
    ...
    +  public function __construct(AccountProxyInterface $current_user, SessionManager $session_manager) {
    

    Use SessionManagerInterface as a type-hint.

  2. +++ b/core/lib/Drupal/Core/Session/AccountImpersonation.php
    @@ -0,0 +1,118 @@
    +  public function impersonateAccount(AccountInterface $account) {
    ...
    +  public function revertAccount() {
    

    What about impersonate() and revertImpersonation()? Given that the methods now are part of a deticated AccountImpersonation class, maybe we do not have to repeat Account in the method name.

  3. +++ b/core/lib/Drupal/Core/Session/AccountImpersonation.php
    @@ -0,0 +1,118 @@
    +    // Prevent session information from being saved and push the previous account.
    ...
    +    // Restore original session saving status if all impersonations are reverted.
    ...
    +    // Restore original session saving status if all impersonations are reverted.
    

    Comment spans more than 80 characters.

  4. +++ b/core/lib/Drupal/Core/Session/AccountImpersonation.php
    @@ -0,0 +1,118 @@
    +  public function revertAll() {
    

    I think this is not necessary anymore. Let's remove it, if we find a use-case it is easily added back.

  5. +++ b/core/lib/Drupal/Core/Session/AccountImpersonation.php
    @@ -0,0 +1,118 @@
    +
    

    Blank newline at the end of the file.

  6. +++ b/core/lib/Drupal/Core/Session/AccountImpersonationInterface.php
    @@ -0,0 +1,54 @@
    +   * Always remember to call AccountImpersonationInterface::revertAccount() after this
    +   * call!
    

    Try to use neutral language and if possible avoid exclamation marks. I found the following sentence in man va_start which could serve as a template: "Each invocation of va_start() must be matched by a corresponding invocation of va_end() in the same function."

znerol’s picture

+++ b/core/modules/system/src/Tests/Session/UserImpersonatingUserTest.php
@@ -0,0 +1,93 @@
+class UserImpersonatingUserTest extends KernelTestBase {

Now that we have a deticated class, the test should match its name: AccountImpersonationTest.

znerol’s picture

+++ b/core/modules/system/src/Tests/Session/UserImpersonatingUserTest.php
@@ -0,0 +1,93 @@
+  public static function getInfo() {
+    return array(
+      'name'        => 'Impersonate users',
+      'description' => 'Temporarily impersonate another user account, and then restore the original account.',
+      'group'       => 'Session',
+    );
+  }

The getInfo() method is not necessary anymore, instead add a @group annotation to the class: @group Session.

znerol’s picture

+++ b/core/modules/system/src/Tests/Session/UserImpersonatingUserTest.php
@@ -0,0 +1,93 @@
+  function testUserImpersonateUser() {

Add the public visibility keyword.

znerol’s picture

Another idea for naming: AccountSwitcher with the methods switchTo($account) and switchBack(). Note that patches up to #27 had similar naming for the functions.

almaudoh’s picture

I think I like AccountSwitcher better.

almaudoh’s picture

Thanks @znerol for the reviews, this patch addresses #116 - #120.

znerol’s picture

Some really minor nitpicks, after that I think this will be ready.

  1. +++ b/core/lib/Drupal/Core/Session/AccountSwitcher.php
    @@ -0,0 +1,96 @@
    +   * @var \Drupal\Core\Session\SessionManager
    

    Use SessionManagerInterface

  2. +++ b/core/lib/Drupal/Core/Session/AccountSwitcherInterface.php
    @@ -0,0 +1,44 @@
    +   * @param \Drupal\Core\Session\AccountInterface
    

    Parameter name is missing here. Add $account.

  3. +++ b/core/lib/Drupal/Core/Session/AccountSwitcherInterface.php
    @@ -0,0 +1,44 @@
    +
    

    Extra newline at the end of the file.

  4. +++ b/core/modules/system/src/Tests/Session/AccountSwitcherTest.php
    @@ -0,0 +1,72 @@
    +    $original_user = clone $user;
    

    This looks like we are cloning the account proxy service. I think it would be cleaner to use $user->getAccount() instead.

almaudoh’s picture

#123 fixed.

znerol’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @almaudoh, great work.

Note to committer: this was RTBC twice (#102 and #105). In #112 it was suggested to introduce a new service instead of adding the methods to the account proxy and in #120 new class and method names have been suggested. This was in order to address #109.

znerol’s picture

Issue summary: View changes
znerol’s picture

Issue summary: View changes

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

The patch looks fantastic - the new service and method names are excellent. This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed ef5b0e3 and pushed to 8.0.x. Thanks!

I think we should update the setAccount documentation to point people to this service.

  /**
   * Sets the currently wrapped account.
   *
   * Setting the current account is highly discouraged! Instead, make sure to
   * inject the desired user object into the dependent code directly
   *
   * @param \Drupal\Core\Session\AccountInterface $account
   *   The current account.
   */
  public function setAccount(AccountInterface $account);

Can we get a followup to do this?

  • alexpott committed ef5b0e3 on 8.0.x
    Issue #287292 by almaudoh, mr.baileys, drewish, Berdir, znerol,...
mradcliffe’s picture

Yay!

I've updated the documentation page related to this: Safely Impersonating Another User.

hass’s picture

Added one more D8 example to these doc page.

mradcliffe’s picture

Issue tags: +Needs change record

We've also changed the way we've done this in core so a change record is applicable. If you disagree, delete it. If not, please review the change record and publish it.

https://www.drupal.org/node/2377441

almaudoh’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

David_Rothstein’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Closed (fixed) » Patch (to be ported)
Issue tags: +Needs backport to D7

This would be worth backporting, right?

One thing I've noticed while using this in Drupal 8: Although the switchBack() "stack" can be handy, it can also be confusing in the case where you're writing a test that needs to switch to a few different users during the course of the test. To use this properly, you essentially need to keep track of all your switches and call switchBack() once for each original switch you did. Otherwise you wind up logged in as the wrong user in the end.

The existing Drupal 7 method is easier since you can just switch directly back to the original account, regardless of what you did in between.

I wonder if it would make sense to add some new restoreOriginal() method that destroys the stack and sends you all the way back to the original account. And then update the documentation at https://www.drupal.org/node/218104 to focus on that, since in most cases using that would cover everything; switchBack() would only be needed in rare cases.

hass’s picture

@David: A backport is very trivial, but into what file(s) should the functions go?

David_Rothstein’s picture

Not sure offhand.. maybe just user.module (like the earlier versions of the patch in this issue)?

Or it could be done as a standalone class (similar to Drupal 8, just without the namespaces) in which case it would be its own file, autoloaded via the registry - AccountSwitcher.inc or similar.

hass’s picture

Status: Patch (to be ported) » Needs review
FileSize
8.88 KB

Reroled #34 from Bedir. Not sure if the changes are all correct as a lot of code has changed.

almaudoh’s picture

I wonder if it would make sense to add some new restoreOriginal() method that destroys the stack and sends you all the way back to the original account.

At #115 we still had a revertAll() in the patch to do so, but was removed based on #116.4 reviews.

I think this is not necessary anymore. Let's remove it, if we find a use-case it is easily added back.

We can always add that if there is a use case though.

David_Rothstein’s picture

Didn't review it carefully, but the backport patch looks pretty reasonable - however instead of always calling drupal_save_session(TRUE) at the end of impersonation don't we actually need to make sure to pass in whatever the original state was (as in the current method at https://www.drupal.org/node/218104)? This will usually be TRUE, but isn't guaranteed.

Also:

+    if (isset($user_original) && !empty($user_original)) {

Seems like that could just be !empty(), no need for isset()? And $user_original = &drupal_static(__FUNCTION__) should probably default to array(), since code elsewhere assumes it's an array.

@almaudoh, thanks - nice find. So maybe it would be a good idea to have a followup issue created for Drupal 8 to consider adding that back.

  • alexpott committed ef5b0e3 on 8.1.x
    Issue #287292 by almaudoh, mr.baileys, drewish, Berdir, znerol,...

  • alexpott committed ef5b0e3 on 8.3.x
    Issue #287292 by almaudoh, mr.baileys, drewish, Berdir, znerol,...

  • alexpott committed ef5b0e3 on 8.3.x
    Issue #287292 by almaudoh, mr.baileys, drewish, Berdir, znerol,...

  • alexpott committed ef5b0e3 on 8.4.x
    Issue #287292 by almaudoh, mr.baileys, drewish, Berdir, znerol,...

  • alexpott committed ef5b0e3 on 8.4.x
    Issue #287292 by almaudoh, mr.baileys, drewish, Berdir, znerol,...
jacob.embree’s picture

This is #139 plus David Rothstein's suggestions in #141.
I am not sure I got drupal_save_session() correct, but I think this is what he meant.

Status: Needs review » Needs work

The last submitted patch, 147: 287292-147-impersonate-user.patch, failed testing. View results

jacob.embree’s picture

If I understand correctly then session saving at the beginning of the test was disabled and my patch at the end sets session saving to what it was at the beginning (FALSE). The new patch just changes the test to check for what the original status was rather than TRUE.

The interdiff is against hass's #139.

jacob.embree’s picture

Status: Needs work » Needs review
FileSize
9.19 KB
1.86 KB

Status: Needs review » Needs work

The last submitted patch, 150: 287292-149-impersonate-user.patch, failed testing. View results

jacob.embree’s picture

Status: Needs work » Needs review
FileSize
9.19 KB
1.86 KB

So sorry. I missed a parenthesis.

Status: Needs review » Needs work

The last submitted patch, 152: 287292-152-impersonate-user.patch, failed testing. View results

jacob.embree’s picture

Status: Needs work » Needs review

Testing passed.

The last submitted patch, 56: 287292-56-user-impersonation-do-not-test.patch, failed testing. View results