-
Notifications
You must be signed in to change notification settings - Fork 3
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.
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
ImpersonateUser fix #10
Conversation
@@ -125,7 +125,7 @@ | |||
} | |||
}, | |||
'switch' => function ($url, $model) { | |||
if($model->id != Yii::$app->user->id && Yii::$app->getModule('user')->enableImpersonateUser) { | |||
if($model->isAdmin && $model->id != Yii::$app->user->id && Yii::$app->getModule('user')->enableImpersonateUser) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, why actually adding this $model->isAdmin check here?
Usually $model will be the user cycled in the grid row: so $model->isAdmin will be true only for admin users.
Result: the button will not be displayed if the gridrow contains a normal user (read: the button "impersonate" will not be shown).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Result: the button will not be displayed if the gridrow contains a normal user (read: the button "impersonate" will not be shown).
this is correct behavior. Impersonate available only on "admin". See origin thread discussion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I insist: "The impersonate should be available to admin with access to all other users."
So: $model will be the user cycled in the grid row: so $model->isAdmin will be true only for admin users.
This will break the behavior you said.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i.e.
$model->isAdmin
should be replace to
\Yii::$app->user->identity->isAdmin
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isAdmin
after
dektrium@b7180b2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before this commit $someuser->isAdmin has used wrong behavior (correct for this case)
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So... but in this fork I can see old code
Line 125 in 4805971
\Yii::$app->user->can($this->module->adminPermission) : false) |
\Yii::$app->user->can($this->module->adminPermission) : false)
instead of
\Yii::$app->authManager->checkAccess($this->id, $this->module->adminPermission) : false)
@d4rkstar you sure use this fork?
After a composer update all went fine :) Thank you! |
dektrium#893