impersonate method security

[assghard]

Hello.
Please consider adding verifications to impersonate method.

    /**
     * Impersonate the given user.
     *
     * @param Model       $user
     * @param string|null $guardName
     * @return  bool
     */
    public function impersonate(Model $user, $guardName = null)
    {
        if ($this->canImpersonate() === false) {
            return false;
        }

        if ($user->canBeImpersonated() === false) {
            return false;
        }

        return app(ImpersonateManager::class)->take($this, $user, $guardName);
    }

After that in User model override just 2 methods:

// User.php model
    /**
     * Only admin can impersonate another user
     * 
     * @return bool
     */
    public function canImpersonate()
    {
        return $this->is_admin == 1; // or check user permissions/group here
    }

    /**
     * Admins can't be impersonated for security reason
     * 
     * @return bool
     */
    public function canBeImpersonated()
    {
        return $this->is_admin == 0; // or check user permissions/group here
    }

No need additional backend verification in controller or service.
This change is compatible with existing code and no need any changes in projects using this library

[drbyte]

You used $this-> here, but $user-> below.

Was that intentional?

I'm not convinced that this is the best place to implement any security rules. This is the "action" method, and I'd prefer that it only handle "doing", not "checking".
The canXxxxxxx() methods above are the better places to "check" security, or in your own app/model.

[assghard]

I fully agree with you. The "checking" logic should be in canImpersonate() and canBeImpersonated() methods and it will still there.
An impersonate() method will return a true if successfully impersonated and false if not and still "doing" method.

From the beginning. The Impersonate.php is a trait which is using in User model:

use Lab404\Impersonate\Models\Impersonate;
class User extends Authenticatable implements MustVerifyEmail
{
    use Impersonate;

after use you can override (in User model) just 2 methods from trait (with your own custom verification logic) :

// every project will have custom logic to verificate user canImpersonate and canBeImpersonated

    /**
     * Only admin can impersonate another user
     * 
     * @return bool
     */
    public function canImpersonate()
    {
        return $this->is_admin == 1; // or check user permissions/group here
    }

    /**
     * Admins can't be impersonated for security reason
     * 
     * @return bool
     */
    public function canBeImpersonated()
    {
        return $this->is_admin == 0; // or check user permissions/group here
    }

I use $this-> because the impersonate method will be called on User model, not on a trait and it looks like:
Auth::user()->impersonate($other_user);

so in trait $this-> - it's impersonating user (admin for example): Auth::user()
and in trait $user - it's $other_user being impersonated (not admin) setted from param: public function impersonate(User $user, $guardName = null)

I proposed this change in Trait to avoid additional verification in controller or service like:

if(Auth::user()->canImpersonate() === false){
 return false; // or ERROR
}
if($other_user->canBeImpersonated() === false){
 return false; // or ERROR
}

Instead of that you can call Auth::user()->impersonate($other_user); and impersonate() will return true if both users pass verification and false if not. In my opinion one line of code in controller/service is better that x2 of if.
And methods canXxxxxxx() will still only "check" security methods

[MarceauKa]

In my opinion, the verification logic must be made in your controller. The impersonate method on models are just helpers. You are free to make your logic in your controller or in canImpersonate and canBeImpersonated as mentionned by @drbyte