yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.23k stars 6.91k forks source link

ACF using RBAC roles - No automatic way to obtain params for `$user->can()` #13273

Closed sdlins closed 7 years ago

sdlins commented 7 years ago

Hi,

It would be useful to have an automatic way to pass post/get/any parameters to an ACF filter when it is checking rbac roles (please, look the comments):

    public function behaviors()
    {
        return [
            'acf' => [
                'class' => AccessControl::className(),
                'rules' => [
                    [
                        'allow' => true,
                        'actions' => ['view'],
                         // current implementation of roles:
                        'roles' => ['manageOwnOrganization'], 
                        // what I propose:
                        'roles' => [
                               'manageOwnOrganization' => ['id'], // or ['get' => 'id'] if I want a specific verb
                        ],
                    ],

And in https://github.com/yiisoft/yii2/blob/master/framework/filters/AccessRule.php#L151 those values could be get and sent to $user->can() as its second parameter... something like $user->can('manageOwnOrganization', ['id' => 9]). Where 9 is the $_REQUEST['id'] or $_GET['id'] if using the ['get' => 'id'] option. If nothing is found it would send a null value and the programmer would manage it.

Use case

I have a rbac rule that verify if the logged user organization is the same that he is trying to manage. The user organization I can obtain from $user->identity, but the organization id comes from get or post data. In so many actions I have to make my own Yii::$app->user->can('manageOwnOrg', ['id' => $id]) to perform the correct filtering but it turns acf rules virtually unuseful to those cases.

It seems a very common use case for rbac/acf. I must confess I am not sure if we have another automatic way to get those parameters, but I couldn't find anyone.

Additional info

Q A
Yii version 2.0.10
PHP version 7.0.1
Operating system Mint 18
arogachev commented 7 years ago

I'm against of this change.

Permissions checks with parameters is something more complicated, putting them inside a controller is a perfectly fine solution.

Here is an example.

Controller's action:

public function actionView($id)
{
    $model = $this->findModel($id);
    if (!Yii::$app->user->can('entry.view', ['model' => $model])) {
        throw new ForbiddenHttpException("Sorry, you don't have permission to view this entry.");
    }
    // The rest of the code
}

/**
 * @param integer $id
 * @return Entry
 * @throws NotFoundHttpException
 */
protected function findModel($id)
{
    $model = Entry::findOne($id);
    if ($model !== null) {
        return $model;
    } else {
        throw new NotFoundHttpException('This entry does not exist');
    }
}

RBAC Rule:

<?php

namespace frontend\rules\entries;

use yii\rbac\Rule;

class ViewRule extends Rule
{
    public $name = 'entry.view';

    public function execute($user, $item, $params)
    {
        /* @var $model \common\models\Entry */
        $model = $params['model'];

        return $model->is_active && !$model->isExpired();
    }
}

Early on I thought that writing checks for permissions with rules requiring parameters is inconsistent with ACF and therefore incorrect too.

But:

if (!Yii::$app->user->can('entry.view.all') && !Yii::$app->user->can('entry.view', ['model' => $model])) {
    throw new ForbiddenHttpException("Sorry, you don't have permission to view this entry.");
}

In this case if the user has admin role with according permission, we don't even need to further check the model to be accessible.

So, ACF is better to use without parameters when we need to reject request even without entering to the controllers's action on the early stage of request. Adding this feature does not bring any essential benefits. Also it can't be used in the way you wrote, I guess additional callbacks and customization will be needed and that's simply not worth implementing.

klimov-paul commented 7 years ago

You should either move access check to controller or use AccessRule::$matchCallback.

sdlins commented 7 years ago

Ok, thank you @arogachev and @klimov-paul.

I am already using access check in my controller, and this is what I want to avoid at the most since I have multiple roles and multiple permissions, some of them with rules, and it make me Repeat myself just because I have to get a parameter from post/get. I think I can do better.

However I will check the information you both provided and continue either using in controller or develop an extension to automatize this for web scenarios.

Thanks.

arogachev commented 7 years ago

By the way, this is a duplicate of #7643.