Add function to switch local user
drewish - July 25, 2008 - 20:44
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | user.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | boombatower |
| Status: | patch (code needs work) |
Description
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.
#1
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.
#2
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?
#3
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.
#4
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();
?>
#5
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.
#6
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
#7
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.
#8
This patch makes sense - I support it. We will always need to switch users, as you say.
#9
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?
#10
WHat about putting user switch function into user module or common.inc instead of simpletest?
#11
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.
#12
Moving.
#13
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).
#14
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!!