Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[11.x] Auth User Impersonation #51031

Draft
wants to merge 7 commits into
base: 11.x
Choose a base branch
from
+184 −0
Draft

[11.x] Auth User Impersonation #51031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7bde396
Add ability to impersonate users
stevebauman Apr 11, 2024
b964824
Add tests
stevebauman Apr 11, 2024
5580480
Logout user if impersonator cannot be retrieved
stevebauman Apr 11, 2024
565851b
Add test
stevebauman Apr 11, 2024
a737274
Prevent nested impersonation
stevebauman Apr 12, 2024
44c7c66
Merge condition
stevebauman Apr 12, 2024
388255c
Implement session regeneration
stevebauman Apr 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 79 additions & 0 deletions src/Illuminate/Auth/SessionGuard.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,75 @@ protected function userFromRecaller($recaller)
return $user;
}

/**
* Impersonate the given user.
*
* @param \Illuminate\Contracts\Auth\Authenticatable $user
* @return void
*
* @throws \Illuminate\Auth\AuthenticationException
*/
public function impersonate(AuthenticatableContract $user)
{
if ($this->impersonating()) {
throw new AuthenticationException('Cannot impersonate while already impersonating.');
}

if (! $authenticated = $this->user()) {
throw new AuthenticationException('Cannot impersonate without a currently authenticated user.');
stevebauman marked this conversation as resolved.
Show resolved Hide resolved
}

$this->session->put($this->getImpersonationName(), $authenticated->getAuthIdentifier());

$this->session->regenerate();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regenerate() doesn't remove any data from the session, it just rotates the ID, so this won't stop session attributes from bleeding across sessions.

You can pass regenerate($destroy = true) to force it to wipe the session data when regenerating the session. I haven't tested it, but you should be able to destroy the session, start a new one, and write to it in the same request?

There is still the risk that something in the current request bleeds through to the session after this though. Either due to Laravel persisting session data somewhere and rewriting it, or maybe through some middleware that manipulates the session after the controller builds a response.

Given I'm paranoid, I'd prefer a full bounce through the browser, i.e. kill the current session, redirect to a unique signed URL, build a new session, etc. It'd eliminate session bleed, but would also add implementation complexity. So maybe this is an acceptable risk that needs to be carefully outlined in the docs? Similar to how they'll need to advise devs to do proper authorisation checks before allowing a user to impersonate another.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the session will be completely destroyed, I'd recommend we store the attributes in the session before impersonating into a key that can be reinstated (for example _old.impersonated) so that we don't loose the context of the original user when they leave the impersonation, otherwise important info may be lost.

Although the way I have seen this implemented in Nova and other packages, the session is never destroyed nor regenerated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also rarely seen that impersonation regenerates session or data. It's not like impersonating grants you the users session data, it's still yours so there's nothing to actually leak. And also, if you can impersonate, you already see that users data so what are you actually leaking here in the session? Maybe all this needs is just to allow clearing current session data impersonate($user, $clearData = true)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also rarely seen that impersonation regenerates session or data. It's not like impersonating grants you the users session data, it's still yours so there's nothing to actually leak. And also, if you can impersonate, you already see that users data so what are you actually leaking here in the session?

The risk is if there is sensitive information in the admin's session that gets stored when impersonating a user, as it'll be stored under the user's account - not the admin's account.

As an example, consider a form that accepts personal or sensitive information which stores draft responses in a session variable. The admin user fills in the form, submitting sensitive information which is stored in a session variable. The admin impersonates the user, visits the form, and the admin's sensitive information is stored in the user's account.

It's a pretty contrived example, but it's definitely possible if session data is persisted.

Maybe all this needs is just to allow clearing current session data impersonate($user, $clearData = true)?

I can't see a reason why you'd want to persist session data when impersonating a user, so why make it optional? What data would you want to persist that would apply across both?

Also, it's best practice to regenerate sessions during login and logout to prevent session fixation, etc, so I think it makes sense to do the same for impersonation too. It's a change of security context, so session IDs and data shouldn't be persisted.

If the session will be completely destroyed, I'd recommend we store the attributes in the session before impersonating into a key that can be reinstated (for example _old.impersonated) so that we don't loose the context of the original user when they leave the impersonation, otherwise important info may be lost.

My recommendation is to force a logout to end impersonation, which removes the need for storing the old session. But if you want to end impersonation without logout, then storing the old session like this through regenerations makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an example, consider a form that accepts personal or sensitive information which stores draft responses in a session variable. The admin user fills in the form, submitting sensitive information which is stored in a session variable. The admin impersonates the user, visits the form, and the admin's sensitive information is stored in the user's account.

I can see that being a problem, but I also think it's an implementation detail for the developer to decide whether they want to clear all or some of the data or not on impersonation.

I can't see a reason why you'd want to persist session data when impersonating a user, so why make it optional? What data would you want to persist that would apply across both?

We have a case where the user has an active_company_id and the id is not available via url - when impersonating we do not want to modify it for the user so the id is stored in the session for specificly targeted company as to not magically make the user start creating entries for different company he's not working with at the time.
We also usually have a previous url stored in the session when stopping impersonation to redirect back to as most time it's being tested with many different users at the same time and it's extremely annoying having to filter the list over and over again.


$this->login($user);
}

/**
* Stop impersonating a user and resume the original authentication state.
*
* @return void
*/
public function unpersonate()
Copy link

@Braunson Braunson Apr 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any interest in stopImpersonating() or endImpersonation?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any interest in stopImpersonating() or endImpersonation?

I would love to choose stopImpersonating() instead of unpersonate() because I don’t see the vocabulary for unpersonate(). Hmm, I come with another option: stopImpersonate(). It saves more characters than stopImpersonating() although stopImpersonate() is same with stopImpersonating().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll wait for additional feedback on this as well. Chose this for its character length being the same, but maybe startImpersonating() stopImpersonating() would work better like you guys mention 🙏

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about impersonateAs() and impersonateOff ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There’s already precedence for stopImpersonating as that’s what the classic version of Spark used: https://github.com/laravel/spark-aurelius/blob/9db0edc5fe1d067305797a46e9e45f116890d476/src/Http/Controllers/Kiosk/ImpersonationController.php#L53

It also makes sense as a method name since it clearly explains the action being performed.

{
if (! $id = $this->session->pull($this->getImpersonationName())) {
stevebauman marked this conversation as resolved.
Show resolved Hide resolved
return;
}

if ($user = $this->provider->retrieveById($id)) {
$this->login($user);

$this->session->regenerate();

return;
}

$this->logout();
}

/**
* Get the underlying impersonator user.
*
* @return \Illuminate\Contracts\Auth\Authenticatable|null
*/
public function impersonator()
{
if ($id = $this->session->get($this->getImpersonationName())) {
return $this->provider->retrieveById($id);
}
}

/**
* Determine if the current user is impersonating another user.
*
* @return bool
*/
public function impersonating()
{
return $this->session->has($this->getImpersonationName());
}

/**
* Get the decrypted recaller cookie for the request.
*
Expand Down Expand Up @@ -839,6 +908,16 @@ public function getRecallerName()
return 'remember_'.$this->name.'_'.sha1(static::class);
}

/**
* Get a unique identifier for the impersonator session value.
*
* @return string
*/
public function getImpersonationName()
{
return 'impersonator_'.$this->name.'_'.sha1(static::class);
}

/**
* Determine if the user was authenticated via "remember me" cookie.
*
Expand Down
105 changes: 105 additions & 0 deletions tests/Auth/AuthGuardTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,111 @@ public function testForgetUserSetsUserToNull()
$this->assertNull($guard->getUser());
}

public function testImpersonate()
{
$mock = $this->getGuard();

$impersonator = m::mock(Authenticatable::class);
$impersonator->shouldReceive('getAuthIdentifier')->once()->andReturn('foo');

$impersonated = m::mock(Authenticatable::class);
$impersonated->shouldReceive('getAuthIdentifier')->once()->andReturn('bar');

$mock->getSession()->shouldReceive('has')->with($mock->getImpersonationName())->once()->andReturn(false);
$mock->getSession()->shouldReceive('get')->with($mock->getName())->once()->andReturn('foo');
$mock->getProvider()->shouldReceive('retrieveById')->once()->with('foo')->andReturn($impersonator);
$mock->getSession()->shouldReceive('put')->with($mock->getImpersonationName(), 'foo')->once();
$mock->getSession()->shouldReceive('put')->with($mock->getName(), 'bar')->once();
$mock->getSession()->shouldReceive('regenerate')->once();
$mock->getSession()->shouldReceive('migrate')->once();

$mock->impersonate($impersonated);
}

public function testImpersonateThrowsWhenAlreadyImpersonating()
{
$this->expectException(AuthenticationException::class);
$this->expectExceptionMessage('Cannot impersonate while already impersonating.');

$mock = $this->getGuard();

$mock->getSession()->shouldReceive('has')->with($mock->getImpersonationName())->once()->andReturn(true);

$mock->impersonate(m::mock(Authenticatable::class));
}

public function testImpersonateThrowsWhenUserIsNotAuthenticated()
{
$this->expectException(AuthenticationException::class);
$this->expectExceptionMessage('Cannot impersonate without a currently authenticated user.');

$mock = $this->getGuard();

$mock->getSession()->shouldReceive('has')->with($mock->getImpersonationName())->once()->andReturn(false);
$mock->getSession()->shouldReceive('get')->with($mock->getName())->once()->andReturn(null);

$mock->impersonate(m::mock(Authenticatable::class));
}

public function testUnpersonate()
{
$mock = $this->getGuard();

$impersonator = m::mock(Authenticatable::class);
$impersonator->shouldReceive('getAuthIdentifier')->once()->andReturn('foo');

$mock->getSession()->shouldReceive('pull')->with($mock->getImpersonationName())->once()->andReturn('foo');
$mock->getProvider()->shouldReceive('retrieveById')->once()->with('foo')->andReturn($impersonator);
$mock->getSession()->shouldReceive('put')->with($mock->getName(), 'foo')->once();
$mock->getSession()->shouldReceive('regenerate')->once();
$mock->getSession()->shouldReceive('migrate')->once();

$mock->unpersonate();
}

public function testUnpersonateLogsOutWhenImpersonatorIsNotAuthenticated()
{
[$session, $provider, $request] = $this->getMocks();

$mock = $this->getMockBuilder(SessionGuard::class)->onlyMethods(['clearUserDataFromStorage'])->setConstructorArgs(['default', $provider, $session, $request])->getMock();
$mock->expects($this->once())->method('clearUserDataFromStorage');

$mock->setDispatcher($events = m::mock(Dispatcher::class));
$events->shouldReceive('dispatch')->once()->with(m::type(Authenticated::class));

$session->shouldReceive('pull')->with($mock->getImpersonationName())->once()->andReturn('foo');
$provider->shouldReceive('retrieveById')->once()->with('foo')->andReturn(null);

$user = m::mock(Authenticatable::class);
$user->shouldReceive('getRememberToken')->andReturn(null);
$mock->setUser($user);

$events->shouldReceive('dispatch')->once()->with(m::type(Logout::class));

$mock->unpersonate();
}

public function testImpersonator()
{
$mock = $this->getGuard();

$impersonator = m::mock(Authenticatable::class);

$mock->getSession()->shouldReceive('get')->with($mock->getImpersonationName())->once()->andReturn('foo');
$mock->getProvider()->shouldReceive('retrieveById')->once()->with('foo')->andReturn($impersonator);

$this->assertSame($impersonator, $mock->impersonator());
}

public function testImpersonating()
{
$mock = $this->getGuard();

$mock->getSession()->shouldReceive('has')->with($mock->getImpersonationName())->once()->andReturnTrue();

$this->assertTrue($mock->impersonating());
}

protected function getGuard()
{
[$session, $provider, $request, $cookie, $timebox] = $this->getMocks();
Expand Down