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:
- It can help people avoid unsafe impersonation constructs as outlined in Safely Impersonating Another User.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#152 | interdiff-287292-139-152.txt | 1.86 KB | jacob.embree |
#152 | 287292-152-impersonate-user.patch | 9.19 KB | jacob.embree |
#139 | Issue-287292-by-hass-Add-functionality-to-impersonat.patch | 8.88 KB | hass |
#124 | 287292-124-account_switcher.patch | 11.56 KB | almaudoh |
#115 | 287292-115-user-impersonation.patch | 13.61 KB | almaudoh |
Comments
Comment #1
boombatower CreditAttribution: boombatower commentedThis 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.
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedthe 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?
Comment #3
boombatower CreditAttribution: boombatower commentedThe 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.
Comment #4
drewish CreditAttribution: drewish commentedor 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
Comment #5
drewish CreditAttribution: drewish commentedsomething 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.
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commentedIMO, 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
Comment #7
drewish CreditAttribution: drewish commentedMoshe, 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.
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedThis patch makes sense - I support it. We will always need to switch users, as you say.
Comment #9
boombatower CreditAttribution: boombatower commentedSo 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?
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commentedWHat about putting user switch function into user module or common.inc instead of simpletest?
Comment #11
drewish CreditAttribution: drewish commentedmoshe 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.
Comment #12
boombatower CreditAttribution: boombatower commentedMoving.
Comment #13
drewish CreditAttribution: drewish commentedhere'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).
Comment #14
floretan CreditAttribution: floretan commentedRe-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!!
Comment #15
webchickSubscribing. :)
Comment #16
jpetso CreditAttribution: jpetso commented> The test is pretty simple but currently fails.
(and the same assertEqual() after switching back) looks to me like it could rather look like
(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.
Comment #17
jpetso CreditAttribution: jpetso commentedRe-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.
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedReading 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.
Comment #19
Dave ReidTagging since XML sitemap module maintains its own copy of these user functions until this is accepted into core (probably not likely until D8).
Comment #20
Dave ReidAdding second tag for clarification.
Comment #21
mr.baileysNo 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."
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:
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.
Comment #22
drewish CreditAttribution: drewish commentedI totally agree with mr.baileys that we should have a stack of users but I ran out of time before implementing it.
Comment #23
drewish CreditAttribution: drewish commentedjust want to get test bot feedback.
Comment #25
dawehnerRerole and corrected patch syntax
Comment #27
mr.baileysComment #29
mr.baileysuser_load()
was called with an array instead of a uid (D6 syntax). This should make the testbot happy.Comment #30
Berdir#29: user_impersonate.patch queued for re-testing.
Comment #32
BerdirRe-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.
Comment #34
BerdirFixed 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.
Comment #35
mr.baileysTagging this as a security improvement since the addition of
user_impersonate_user()
anduser_revert_user()
…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.
Comment #36
sanduhrsUpdated 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.
Comment #37
BerdirMoving to the 8.x queue.
Comment #38
geek-merlingood stuff, sub.
Comment #39
mr.baileysRe-rolled for 8.x-dev.
Comment #40
apaderno#39: 287292-39-user-impersonate.patch queued for re-testing.
Comment #41
apadernoComment #43
mr.baileysRe-rolled to keep up with HEAD
Comment #44
mr.baileysBetter 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.
Comment #46
mr.baileys#44: 287292-44-user-impersonation.patch queued for re-testing.
Comment #47
sanduhrsWording: [...]so it we can[...]
Is that still needed? seems to happen in user_impersonate_user() already.
Comment #48
hass CreditAttribution: hass commentedAny news on this case?
Comment #49
apaderno#44: 287292-44-user-impersonation.patch queued for re-testing.
Comment #52
hass CreditAttribution: hass commentedCan someone re-role for D8 so that we do not miss final release, please?
Comment #53
almaudoh CreditAttribution: almaudoh commentedI need this for SMSFramework in D8. Will work on it.
Comment #54
BerdirNote that this will have to be completely reworked for 8.x, maybe should live inside AccountProxy now?
Comment #55
almaudoh CreditAttribution: almaudoh commentedI 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.
Comment #56
almaudoh CreditAttribution: almaudoh commentedJust 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?
Comment #57
almaudoh CreditAttribution: almaudoh commentedComment #58
almaudoh CreditAttribution: almaudoh commentedComment #59
almaudoh CreditAttribution: almaudoh commentedComment #60
almaudoh CreditAttribution: almaudoh commentedComment #61
dawehnerThis certainly needs work in d8.
Comment #62
nicobot CreditAttribution: nicobot commentedHi 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:
Thanks,
Nicolás
Comment #63
hass CreditAttribution: hass commentedThat could be done as a followup if needed (I do not), but very first we need the basics done.
Comment #64
almaudoh CreditAttribution: almaudoh commentedI updated the InstallerServiceProvider to replace the 'current_user' service with null implementation. Patch now installs. Let's see how this comes out...
Comment #66
almaudoh CreditAttribution: almaudoh commentedLooked 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.Comment #67
almaudoh CreditAttribution: almaudoh commentedSorry forgot the interdiff...:-)
Comment #69
almaudoh CreditAttribution: almaudoh commentedComment #71
dawehnerLet's return $this instead.
I wonder whether we should throw an exception when the stack is empty?
double line
Given how array_pop/push works I guess it would be safer to use array_shift instead of directly accessing [0]
Comment #72
almaudoh CreditAttribution: almaudoh commentedThanks @dawehner for the review.
\RuntimeException
for now. Does this warrant creating another Exception class?I still can't figure out the test fail on
SessionTest->testSessionSaveRegenerate()
- it's not failing on my system.Comment #74
mradcliffeYeah, 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.
Comment #75
almaudoh CreditAttribution: almaudoh commentedIt is set to FALSE in
TestBase::prepareEnvironment()
where the current_user impersonation is done atComment #76
almaudoh CreditAttribution: almaudoh commentedAnother re-roll to keep up to date with HEAD. Also fixed the
UserImpersonatingUserTest
so it actually runs - wasn't running before.Comment #77
almaudoh CreditAttribution: almaudoh commentedComment #79
almaudoh CreditAttribution: almaudoh commentedRe-rolled patch. Fixed the test. Bot still failing SessionTest while local system is passing. Troubleshooting...
Will not attach interdiffs again until it goes green.
Comment #81
almaudoh CreditAttribution: almaudoh commentedThe 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.
$container->set('current_user', \Drupal\Core\Session\AccountInterface object)
$container->set('current_user', \Drupal\Core\Session\AccountProxyInterface object)
$container->get('current_user')->setAccount(\Drupal\Core\Session\AccountInterface or \Drupal\Core\Session\AccountInterface object)
$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.
Comment #82
almaudoh CreditAttribution: almaudoh commentedFound 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.
Comment #83
almaudoh CreditAttribution: almaudoh commentedComment #84
almaudoh CreditAttribution: almaudoh commentedComment #87
mradcliffe#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.
Comment #88
mradcliffexmlsitemap, 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.
Comment #89
hass CreditAttribution: hass commentedI think I still need this in Link Checker as I need to update nodes via cron as an impersonated user.
Comment #90
znerol CreditAttribution: znerol commented#2338727: Replace static SessionManager::$enabled property with WriteSafeSessionHandler class and resolve hidden circular dependency between SessionManager and SessionHandler attempts to extract
disable()
/enable()
from session manager.Comment #91
znerol CreditAttribution: znerol commentedReroll, left out the hunk in
TestBase
, that shouldn't be necessary anymore. Also theoriginalUser
property inTestBase
andWebTestBase
seems to be orphaned, removing it also.Comment #92
moshe weitzman CreditAttribution: moshe weitzman commentedLooks nice and clean to me. Thanks @znerol.
Comment #93
mradcliffeLooks 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.
nitpick: else should be on a new line.
Comment #94
almaudoh CreditAttribution: almaudoh commentedSmall documentations nits:
s/Set/Sets/
s/Revert/Reverts/
Otherwise looks good to go in.
Comment #95
almaudoh CreditAttribution: almaudoh commentedFixed #93 and #94. Minor changes. Back to RTBC per #92.
Comment #96
almaudoh CreditAttribution: almaudoh commentedComment #100
BerdirTestbot was broken.
Comment #102
alexpottWe should not be using t() on exception messages.
psr-4? :) Also this means atm this patch has no explicit test coverage :)
This is wrong
Why webtestbase? I'm sure this could be a KernelTestBase test.
Comment #103
almaudoh CreditAttribution: almaudoh commented#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.
Comment #104
almaudoh CreditAttribution: almaudoh commentedSorry, forgot the patch...
Comment #105
alexpottI 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.Comment #106
almaudoh CreditAttribution: almaudoh commented#105: Good point. Fixed.
Also updated
AccountProxy::impersonateAccount()
to not overwrite the very first session status in subsequent impersonate calls.Comment #109
alexpottThese 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.
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?
Comment #110
almaudoh CreditAttribution: almaudoh commentedPersonally 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?
Comment #111
almaudoh CreditAttribution: almaudoh commentedCNR so others can comment on #110 and also suggest names. Will post a patch at the end of the day.
Comment #112
znerol CreditAttribution: znerol commentedFrom 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 forAccountInterface
and - in most cases - also vice versa. In fact I believe that thegetAccount()
,setAccount()
calls only should be used by a very restricted set of services (authentication) and all other code should assume thatcurrent_user
is a plain account.Therefore I suggest to introduce a new service
impersonation
(analogous toauthentication
) which implements the stack but calls to the account proxy whenever it is necessary to change the user.Comment #113
almaudoh CreditAttribution: almaudoh commented+1
Since most of the former use cases for account "impersonation" (mostly in tests) are now handled by
AccountProxyInterface
and thecurrent_user
service. It's only the stacked account functionality that's left.+1, however, I suggest
account_stack
as the service name since that reflects what it's actually doing (you can also do impersonation byAccountProxyInterface::setAccount()
without maintaining track of past accounts).Comment #114
znerol CreditAttribution: znerol commentedRegrettably 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).
Comment #115
almaudoh CreditAttribution: almaudoh commentedNew 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.Comment #116
znerol CreditAttribution: znerol commentedGreat 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.
Use
SessionManagerInterface
as a type-hint.What about
impersonate()
andrevertImpersonation()
? Given that the methods now are part of a deticatedAccountImpersonation
class, maybe we do not have to repeatAccount
in the method name.Comment spans more than 80 characters.
I think this is not necessary anymore. Let's remove it, if we find a use-case it is easily added back.
Blank newline at the end of the file.
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."Comment #117
znerol CreditAttribution: znerol commentedNow that we have a deticated class, the test should match its name:
AccountImpersonationTest
.Comment #118
znerol CreditAttribution: znerol commentedThe
getInfo()
method is not necessary anymore, instead add a@group
annotation to the class:@group Session
.Comment #119
znerol CreditAttribution: znerol commentedAdd the
public
visibility keyword.Comment #120
znerol CreditAttribution: znerol commentedAnother idea for naming:
AccountSwitcher
with the methodsswitchTo($account)
andswitchBack()
. Note that patches up to #27 had similar naming for the functions.Comment #121
almaudoh CreditAttribution: almaudoh commentedI think I like
AccountSwitcher
better.Comment #122
almaudoh CreditAttribution: almaudoh commentedThanks @znerol for the reviews, this patch addresses #116 - #120.
Comment #123
znerol CreditAttribution: znerol commentedSome really minor nitpicks, after that I think this will be ready.
Use
SessionManagerInterface
Parameter name is missing here. Add
$account
.Extra newline at the end of the file.
This looks like we are cloning the account proxy service. I think it would be cleaner to use
$user->getAccount()
instead.Comment #124
almaudoh CreditAttribution: almaudoh commented#123 fixed.
Comment #125
znerol CreditAttribution: znerol commentedThank 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.
Comment #126
znerol CreditAttribution: znerol commentedComment #127
znerol CreditAttribution: znerol commentedComment #129
alexpottThe 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.
Can we get a followup to do this?
Comment #131
mradcliffeYay!
I've updated the documentation page related to this: Safely Impersonating Another User.
Comment #132
hass CreditAttribution: hass commentedAdded one more D8 example to these doc page.
Comment #133
mradcliffeWe'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
Comment #134
almaudoh CreditAttribution: almaudoh commented#129: Raised follow up issue #2378329: Update AccountProxyInterface::setAccount() documentation to point people to the account_switcher service
Comment #136
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis 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.
Comment #137
hass CreditAttribution: hass commented@David: A backport is very trivial, but into what file(s) should the functions go?
Comment #138
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedNot 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.
Comment #139
hass CreditAttribution: hass commentedReroled #34 from Bedir. Not sure if the changes are all correct as a lot of code has changed.
Comment #140
almaudoh CreditAttribution: almaudoh commentedAt #115 we still had a
revertAll()
in the patch to do so, but was removed based on #116.4 reviews.We can always add that if there is a use case though.
Comment #141
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedDidn'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:
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.
Comment #147
jacob.embree CreditAttribution: jacob.embree at St. Louis Integration commentedThis 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.Comment #149
jacob.embree CreditAttribution: jacob.embree at St. Louis Integration commentedIf 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 thanTRUE
.The interdiff is against hass's #139.
Comment #150
jacob.embree CreditAttribution: jacob.embree at St. Louis Integration commentedComment #152
jacob.embree CreditAttribution: jacob.embree at St. Louis Integration commentedSo sorry. I missed a parenthesis.
Comment #154
jacob.embree CreditAttribution: jacob.embree at St. Louis Integration commentedTesting passed.