Add a function to impersonate a user

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.

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.

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

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

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

Subscribing. :)

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.

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

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

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

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

I think this should use drupal_static() instead.

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.

just want to get test bot feedback.

Rerole and corrected patch syntax

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

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

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

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

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.

Moving to the 8.x queue.

Re-rolled for 8.x-dev.