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

impersonate method security #131

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

impersonate method security #131

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions src/Models/Impersonate.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,14 @@ public function canBeImpersonated()
*/
public function impersonate(Model $user, $guardName = null)
{
if ($this->canImpersonate() === false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

return false;
}

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

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

Expand Down