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:

<?php
 
/**
   * 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.
Files: 
CommentFileSizeAuthor
#44 287292-44-user-impersonation.patch11.5 KBmr.baileys
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 287292-44-user-impersonation.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#43 287292-43-user-impersonation.patch15.3 KBmr.baileys
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 287292-43-user-impersonation.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#39 287292-39-user-impersonate.patch10.34 KBmr.baileys
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 287292-39-user-impersonate.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#34 user_impersonate2.patch10.53 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 22,026 pass(es).
[ View ]
#32 user_impersonate2.patch9.69 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 21,952 pass(es), 3 fail(s), and 0 exception(es).
[ View ]
#29 user_impersonate.patch5.78 KBmr.baileys
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_impersonate_1.patch.
[ View ]
#27 user_impersonate.patch5.8 KBmr.baileys
FAILED: [[SimpleTest]]: [MySQL] 19,944 pass(es), 0 fail(s), and 4 exception(es).
[ View ]
#25 user_287292_0.patch3.37 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 19,959 pass(es), 1 fail(s), and 2 exception(es).
[ View ]
#22 user_287292.patch3.15 KBdrewish
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_287292_0.patch.
[ View ]
#17 user-switch-user.patch3.14 KBjpetso
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-switch-user.patch.
[ View ]
#14 user_287292-14.patch3.05 KBfloretan
FAILED: [[SimpleTest]]: [MySQL] 19,971 pass(es), 2 fail(s), and 2 exception(es).
[ View ]
#13 user_287292.patch1.84 KBdrewish
PASSED: [[SimpleTest]]: [MySQL] 19,940 pass(es).
[ View ]
#5 simpletest_287292.patch2 KBdrewish
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch simpletest_287292.patch.
[ View ]
#1 simpletest_switch_user.patch1.35 KBboombatower
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch simpletest_switch_user.patch.
[ View ]

Comments

Assigned:Unassigned» boombatower
Status:Active» Needs review
StatusFileSize
new1.35 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch simpletest_switch_user.patch.
[ View ]

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.

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?

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.

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

<?php
$this
->drupalSetLocalUser($root);
// do some tests...
$this->drupalSetLocalUser($normal_user);
// do some more tests...
$this->drupalRestoreLocalUser();
?>

StatusFileSize
new2 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch simpletest_287292.patch.
[ View ]

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.

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

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.

Status:Needs work» Needs review

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

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?

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

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.

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

Moving.

Status:Needs work» Needs review
StatusFileSize
new1.84 KB
PASSED: [[SimpleTest]]: [MySQL] 19,940 pass(es).
[ View ]

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

Status:Needs review» Needs work
StatusFileSize
new3.05 KB
FAILED: [[SimpleTest]]: [MySQL] 19,971 pass(es), 2 fail(s), and 2 exception(es).
[ View ]

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!!

Subscribing. :)

> The test is pretty simple but currently fails.

<?php
+    $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

<?php
+    $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.

StatusFileSize
new3.14 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-switch-user.patch.
[ View ]

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.

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.

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

Issue tags:+contrib dependency

Adding second tag for clarification.

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.

StatusFileSize
new3.15 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_287292_0.patch.
[ View ]

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

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.

Status:Needs work» Needs review
StatusFileSize
new3.37 KB
FAILED: [[SimpleTest]]: [MySQL] 19,959 pass(es), 1 fail(s), and 2 exception(es).
[ View ]

Rerole and corrected patch syntax

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new5.8 KB
FAILED: [[SimpleTest]]: [MySQL] 19,944 pass(es), 0 fail(s), and 4 exception(es).
[ View ]
  • 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.

Status:Needs work» Needs review
StatusFileSize
new5.78 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_impersonate_1.patch.
[ View ]

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

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

Status:Needs work» Needs review
StatusFileSize
new9.69 KB
FAILED: [[SimpleTest]]: [MySQL] 21,952 pass(es), 3 fail(s), and 0 exception(es).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new10.53 KB
PASSED: [[SimpleTest]]: [MySQL] 22,026 pass(es).
[ View ]

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.

Title:Add function to switch local userAdd 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.

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.

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

Moving to the 8.x queue.

good stuff, sub.

Title:Add function to impersonate a userAdd a function to impersonate a user.
StatusFileSize
new10.34 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 287292-39-user-impersonate.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-rolled for 8.x-dev.

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

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.

Status:Needs work» Needs review
StatusFileSize
new15.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 287292-43-user-impersonation.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-rolled to keep up with HEAD

StatusFileSize
new11.5 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 287292-44-user-impersonation.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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

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

Any news on this case?

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.