ImpersonateUser fix

[bscheshirwork]

* Tokens enclosed within curly brackets are treated as controller action IDs (also called *button names*
* in the context of action column). They will be replaced by the corresponding button rendering callbacks
* specified in [[buttons]]. For example, the token `{view}` will be replaced by the result of
* the callback `buttons['view']`. If a callback cannot be found, the token will be replaced with an empty string.

Q	A
Is bugfix?	yes
New feature?	no
Breaks BC?	yes/no
Fixed issues	#890

[thyseus]

+1 - please merge :)

[thyseus]

@bscheshirwork Hey - could you please copy your PR over to https://github.com/dabilite/yii2-user/pulls ? We currently have an fork of dektrium/yii2-user ongoing. Thanks !

[bscheshirwork]

You are clearly not doing the right thing.

17 opened PR.

3 new Member... 759 stars!

Can resolve problem in this project.

[thyseus]

The Idea is to do a "temporary fork" of the dektrium module collection that can be merged back into dektrium once @dmeroff or @thiagotalma have enough resources again to manage the project.

[bscheshirwork]

Sure? A nightmare for combining and verifying which of the PRs were combined to you branch and which are not.

[thyseus]

Git is decentral by design, i don´t see any problems with it.

[YiiRocks]

I don't feel much for moving over to another fork for the moment and having to move back later in my projects. You are not only dealing with a decentral system, but also with user implementations.

[thiagotalma]

Merged. Thanks!

[schmunk42]

Sorry I haven't checked this in detail, but it looks to me like only the button is hidden. There should be additional checks in the controller or not?

[thiagotalma]

@schmunk42 Can you create a new PR with the changes that you think necessary?

[bscheshirwork]

I can't remember...

So.. this action allow to all for design reason (one action to masquerade and to go out)

yii2-user/controllers/AdminController.php

Lines 183 to 193 in 120ab9e

	'rules' => [
	[
	'allow' => true,
	'actions' => ['switch'],
	'roles' => ['@'],
	],
	[
	'allow' => true,
	'roles' => ['admin'],
	],
	],

and check access inside

yii2-user/controllers/AdminController.php

Lines 344 to 346 in 120ab9e

	if (!Yii::$app->user->identity->isAdmin) {
	throw new ForbiddenHttpException;
	}

[bscheshirwork]

@schmunk42 look at this ^

[schmunk42]

Ah, I remember :) Actually, we have this permission set.

Our problem comes from the fact that we have ie. three roles for users: admin, developer and editor. A developer should be able to use the user-module, but if we set the adminPermission he can also become an admin.

So giving the adminPermission of the user-module to a user is equivalent to giving him full access to the application, even though he has no access to RBAC.
Thinking about this, there would have to be also an additional check for assignments.

I guess we need to implement this via events or is this something you would like to address in the user-module?

[bscheshirwork]

@schmunk42 yes

Yii::$app->user->identity->isAdmin

will be customize throw adminPermission property of this module. But adminPermission property is not role only.
For example we can create permission administrateUser and assign it to admin role only

    'modules' => [
        'user' => [
            'class' => 'dektrium\user\Module',
            'enableRegistration' => false,
            'adminPermission' => 'administrateUser',
...

[bscheshirwork]

or we have developer who can masquerade into user?

[thyseus]

Using a 'developer' role could be misleading. We use the impersonate user function also on the live system, for the administrators to see what the user has done. We use a own role 'can-impersonate' for this to fine-grain which administrators should get this (very powerful!) access right.

[schmunk42]

We use a own role 'can-impersonate' ...

I thought about a similar thing.

Moreover, although I haven't check this in detail ... a user should only be able to assign permissions/roles he has Yii::$app->user->can(), maybe in conjunction with a property can-assign-revoke?

[bscheshirwork]

So... This is argument for redesign it.

[thyseus]

Big +1 for the can-assign-revoke idea. Being able to fine-grain the administrator access rights is really important. One should also not be able to impersonate into any other administrator user ! Currently this is possible, so every admin could do everything, when he knows how. We definitely need some huge redesign for this feature.

I am still not sure if all our work and love should better go into yii2-usuario though... will this fork ever be merged into dektrium again?

[bscheshirwork]

@thyseus @schmunk42 how about separate administrate and impersonate?

    'modules' => [
        'user' => [
            'class' => 'dektrium\user\Module',
            'adminPermission' => 'administrateUser',
            'impersonatePermission' => 'impersonateUser',

[bscheshirwork]

will this fork ever be merged into dektrium again?

I can see some merged PR

[thyseus]

I mean: in the long term its bad to have dektrium/yii2-user and yii2-usuario in parallel with the same codebase but different features... there should be one, great user module for yii2.

[schmunk42]

I'd be also very interested in a common code-base.

@thiagotalma Are you a maintainer of this project now? Looks to me like this is the case from commits and releases.

@tonydspaniard Will you or someone of your team be able to support yii2-usuario in the long term?

[thyseus]

Furthermore (just collecting ideas): all impersonating actions should be logged very precisely (login+logout time) so one can determine which action was done by the "real" user, and which one by the "admin".

For example, we use yii\behaviors\BlameableBehavior a lot, and the created_by column would be set to the user id in Yii::app->user->id - the "real" user, even when impersonated through the admin. We should think about how we handle this.

What happens when the real user and the impersonating administrator are logged in simultaneously (not uncommon when the admin tries to support the user in real-time) ? How do we avoid conflicts (row lock, ...)

[thiagotalma]

I'd be also very interested in a common code-base.

@thiagotalma Are you a maintainer of this project now? Looks to me like this is the case from commits and releases.

@tonydspaniard Will you or someone of your team be able to support yii2-usuario in the long term?

@schmunk42 Please, join here

[thyseus]

I just found the time to add some security improvements to the impersonating feature:

#1014

@thiagotalma @schmunk42 @bscheshirwork @tonydspaniard