framework icon indicating copy to clipboard operation
framework copied to clipboard

[11.x] Auth User Impersonation

Open stevebauman opened this issue 1 year ago • 20 comments

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! 🙏

stevebauman avatar Apr 11 '24 23:04 stevebauman

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?

Braunson avatar Apr 12 '24 00:04 Braunson

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.

devajmeireles avatar Apr 12 '24 00:04 devajmeireles

Does it support multiple guards?

ankurk91 avatar Apr 12 '24 01:04 ankurk91

@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 🙏

stevebauman avatar Apr 12 '24 01:04 stevebauman

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.

valorin avatar Apr 12 '24 04:04 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.

Full logout would be a very good addition too.

devajmeireles avatar Apr 12 '24 04:04 devajmeireles

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, [
        '[email protected]',
    ]);
});

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, [
        '[email protected]',
    ]);
});

I can contribute to the PR if needed

moisish avatar Apr 12 '24 07:04 moisish

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.

peterfox avatar Apr 12 '24 08:04 peterfox

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.

jorqensen avatar Apr 12 '24 11:04 jorqensen

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

Wirone avatar Apr 12 '24 11:04 Wirone

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.

donnysim avatar Apr 12 '24 11:04 donnysim

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

calebdw avatar Apr 26 '24 19:04 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

I second this, I heavily use this in all of my projects and it works perfectly and does everything I see others suggest in this thread. My vote would be that the setup for fortify/jetstream asks if you want to install and use impersonate. Next, does Taylor just include this awesome package as the option or makes this package first party with the repo owners permission. I don't see why this PR needs to reinvent something that we already have a solid package for. Just my thoughts. @taylorotwell

CodyPChristian avatar Sep 17 '24 12:09 CodyPChristian