[11.x] Auth User Impersonation

[stevebauman]

This PR provides capability of first-party impersonation built into the default session guard, allowing developers to easily implement impersonating other users, while tracking the source user who initiated the impersonation:

Impersonating:

// Initial impersonator session (an admin, for example)
Auth::login($impersonator);

// Impersonating another user
Auth::impersonate($user); // void

// Authenticated user is now the given user
Auth::user(); // $user

// Impersonator is the the initial session owner
Auth::impersonator(); // $impersonator

Determining Impersonation State:

Auth::impersonating(); // bool

Unpersonating:

Auth::unpersonate(); // void

Let me know your thoughts on this feature and if you'd like anything changed or adjusted.

No hard feelings on closure. Thanks so much for your time! 🙏

[Braunson]

Any interest in stopImpersonating() or endImpersonation?

[kresnasatya]

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

[stevebauman]

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 🙏

[CoolGoose]

what about impersonateAs() and impersonateOff ?

[martinbean]

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.

[Braunson]

Somewhat related, this edge-case may be not actually relevant to this MR but:

1. User A impersonates User B
2. User B impersonates User C
3. When you stop impersonating you are put back to User B (not user A).

Of course this is crazy and should be taken care of by the developer but wouldn't this MR allow this? Should we throw an exception if we're already impersonating a user?

[devajmeireles]

Although I find this useful, I believe it should not be part of the framework - maybe this is a good one for a Fortify feature. Still, if Taylor decides it's useful to take this forward I would recommend thinking about a few things, things that I usually deal with when I implement impersonation in my projects:

1. Prevent self-impersonate.
2. Dispatch events when impersonating or log out the impersonation.
3. Like Fortify, offers the possibility of touching and interacting with the impersonate logic.

EDIT: 4. As mentioned below, it would be very nice to have the full logout feature as well.

[ankurk91]

Does it support multiple guards?

[stevebauman]

@Braunson

Somewhat related, this edge-case may be not actually relevant to this MR but:

1. User A impersonates User B
2. User B impersonates User C
3. When you stop impersonating you are put back to User B (not user A).

Of course this is crazy and should be taken care of by the developer but wouldn't this MR allow this? Should we throw an exception if we're already impersonating a user?

Fixed! 👍 I've implemented this, thanks 🙏

[valorin]

While I like the idea of having solid impersonation support within the Auth system, this implementation suffers from session bleed. The original user's session is maintained when they impersonate a user, so any sensitive session data from the original user will be available to the impersonated user's account. Likewise, session data from the impersonated user will persist back to the original user's session when they stop impersonating.

For security reasons you need to completely wipe and regenerate the session when impersonating. I'd suggest tracking the impersonation information in a cookie instead, as that is separate to a user session.

I've also seen apps require a full logout when ending impersonation. It's not as user friendly, but it does significantly reduce the risk of impersonated accounts serving XSS that can return to the original (admin) account for PrivEsc.

[devajmeireles]

While I like the idea of having solid impersonation support within the Auth system, this implementation suffers from session bleed. The original user's session is maintained when they impersonate a user, so any sensitive session data from the original user will be available to the impersonated user's account. Likewise, session data from the impersonated user will persist back to the original user's session when they stop impersonating.

For security reasons you need to completely wipe and regenerate the session when impersonating. I'd suggest tracking the impersonation information in a cookie instead, as that is separate to a user session.

I've also seen apps require a full logout when ending impersonation. It's not as user friendly, but it does significantly reduce the risk of impersonated accounts serving XSS that can return to the original (admin) account for PrivEsc.

Full logout would be a very good addition too.

[moisish]

I have my own implementation for impersonating users because i need to control which user can use impersonation.

I think if this will be added on the framework it will need some kind of authorization for security

Maybe something similar to what horizon does eg only admins can impersonate users

Gate::define('canImpersonate', function (User $user) {
    return in_array($user->email, [
        'taylor@laravel.com',
    ]);
});

And maybe even further to who can be impersonated eg you can impersonate only non admin users

Gate::define('canBeImpersonated', function (User $user) {
    return !in_array($user->email, [
        'taylor@laravel.com',
    ]);
});

I can contribute to the PR if needed

[peterfox]

I'd suggest adding pluggable events to this. E.g. one to check before impersonation has occurred that can block it by returning false in the same way Send events allow. Another afterwards etc.

Also, there should probably be some kind of wrapper with a contract to the way the ID is stored. From my brief look if you have multiple guards and impersonate one guard it will affect all guards. That might be intentional but equally a security issue for others. Having a way to customise that mechanism makes sense.

[jorqensen]

While I think impersonation is a great feature to have, I am not a complete fan of this implementation. I am also questioning if this should be a concern of the framework, as impersonation is a layer on top of authentication. Where does it stop then?

I would much rather be in favor of an implementation with the likes of how Nova does it, using a trait to opt into this functionality being available.

[Wirone]

As I said here, I also see this implementation as too simple. When you compare this with Symfony's impersonation feature, which is fully configurable, supports several ways of triggering impersonation entrypoint (GET parameter, HTTP header), and most importantly: handles session properly and requires specific role to be able to impersonate other users, then you can clearly see that complex domain was approached in too optimistic way. I am not against impersonation being a framework concept, as I am used to it as a part of Symfony's security layer, I just think many pieces are missing here and were already pointed out earlier (like events etc).

[donnysim]

I'd think you could just wipe the session data clean before setting impersonation data which should remove the data leak problem if regenerating is a bit problematic. For the cookie, it's kind of a tough approach too - having a separate cookie for impersonation on poor implementation can allow the wrong user becoming the impersonated user on session expiration as the cookie is not tied to the actual user session. I've tried both in the past and storing it directly in a session worked better than a cookie from security standpoint.

[valorin]

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.

[newtonjob]

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.

[donnysim]

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

[valorin]

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.

[donnysim]

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.

[calebdw]

We currently use the laravel-impersonate package for this purpose, but would love to see first party support for it in Laravel.

Some of the niceties offered in the package that I would like to see here are:

• Events (we use a listener/subscriber to capture these events in our audit log)
• Blade directives