yiisoft / yii2

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

AccessRule behavior and rbac\Rule $params #4302

Closed antokara closed 10 years ago

antokara commented 10 years ago

As I see on this line https://github.com/yiisoft/yii2/blob/a6136e1b16b293513bb873e0904bb50d2198ed69/framework/filters/AccessRule.php#L151 the AccessRule behavior calls the $user->can($role) without an ability to pass any $params which may be required on a defined yii\rbac\Rule.

Is there another way for this that I am missing? (besides calling the user->can() directly from the action)

samdark commented 10 years ago

Where do you want to get these parameters from in your access filter config?

antokara commented 10 years ago

in my case, I think I can use \Yii::$app->request->get() for the parameter in question which I believe it does not need to be passed through $params...

But a need may arise where controller variables could be needed;

samdark commented 10 years ago

Are you sure the parameter in question will be available for all actions of the controller? Because filters are initialized for every action.

Overall when one needs to pass parameters most probably these are action-specific parameters so the check has to be made in the action itself and not in the global controller filter.

antokara commented 10 years ago

In this scenario there is a Controller where all my Controllers extend from and have common actions declared. (REST service).

I need the same RBAC Rule to be applied on each set of actions that extends my Controller which defines the Access Filter for them.

In the case I do not want that Access Filter Rule, I can declare the Behaviors and change the Rules.

The specific action is "Does the X user has Access to the Y module for the Z project?" I have the X user... I have the Y module (as the permission itself that the user has inherited from a Role) and I have a table in database that contains which Z projects the X user has access to.

So when I get the request, I need the Rule to check those information and provide a result...

zelenin commented 10 years ago

maybe you should use this snippet

    public function behaviors()
    {
        return [
            'access' => [
                'class' => AccessControl::className(),
                'rules' => [
                    [
                        'actions' => ['update', 'delete'],
                        'allow' => true,
                        'matchCallback' => function ($rule, $action) {
                                $model = $this->getModels(Yii::$app->getRequest()->get('id'));
                                return Yii::$app->getUser()->can('updateUser', ['model' => $model]);
                            }
                    ],
                ]
            ]
        ];
    }
antokara commented 10 years ago

That could work but perhaps there is a better way to achieve the same functionality;

Thank you.

zelenin commented 10 years ago

@antokara no better way with using of params

antokara commented 10 years ago

@zelenin I mean by adding a new class property in AccessRule or something like that.

qiangxue commented 10 years ago

What @zelenin suggested is the right way to go because you do not want to evaluate the parameter each time behaviors() is called or you may be able to get the parameter at that time.

antokara commented 10 years ago

It would be nice to add this snippet somewhere accessible by everyone, in the docs maybe?

samdark commented 10 years ago

I don't think it's a good idea to use the same permission for both update and delete. @qiangxue isn't it better to do the check directly in the action method in this case?

qiangxue commented 10 years ago

Yes, it's easier to write code directly in actions in this case.

dmill-bz commented 9 years ago

I know this is closed but I wanted to think out loud on this matter. Is the case of having a get/post param passed to a rule not a frequent enough occurrence that it would be worth it to offer syntactic sugar for it ? For example:

    public function behaviors()
    {
        return [
            'access' => [
                'class' => AccessControl::className(),
                'rules' => [
                    [
                        'actions' => ['update', 'delete'],
                        'allow' => true,
                        'matchCallback' => function ($rule, $action) {
                                $params = [
                                      'modelId' => Yii::$app->getRequest()->get('id'),
                                      'otherParam' => 'somefixedstring'
                                ];
                                return Yii::$app->getUser()->can('updateUser', $params);
                            }
                    ],
                ]
            ]
        ];
    }

could become :

    public function behaviors()
    {
        return [
            'access' => [
                'class' => AccessControl::className(),
                'rules' => [
                    [
                        'actions' => ['update', 'delete'],
                        'allow' => true,
                        'role' => [
                              'updateUser' => [
                                    'modelId' => ['get' => 'id']],
                                    'otherParam' => "somefixedstring"
                        ]
                    ],
                ]
            ]
        ];
    }

I'm asking this because in our case every single access rule is dependent on a context. So we basically have matchCallback everywhere. It is 1) a pain to set up 2) incredibly redundant 3) not all that readable.

With all that said, our case is quite more complex than probably a majority of projects, and the curent situation is functional and still better than handling access rules directly in actions which is hard to maintain. (And we still have the option of extending the native classes anyways)

Like I said, just thinking out loud.

cebe commented 9 years ago

this will be lost now, as it is posted under a closed issue and I have no time to read it now. If you think it matters, please open a new issue.

SamMousa commented 8 years ago

I know this is old, but I'd like this feature as well. While I agree that checking URL params might not be the best practice, there are other arguments one might want to pass as a parameter.

What is the downside of allowing developers to pass in arguments by using a string key and an array value?

--> Note that this could be implemented without breaking BC in ~ 3 lines: Current:

protected function matchRole($user)
    {
        if (empty($this->roles)) {
            return true;
        }
        foreach ($this->roles as $role) {
            if ($role === '?') {
                if ($user->getIsGuest()) {
                    return true;
                }
            } elseif ($role === '@') {
                if (!$user->getIsGuest()) {
                    return true;
                }
            } elseif ($user->can($role)) {
                return true;
            }
        }

        return false;
    }

Proposed:

protected function matchRole($user)
    {
        if (empty($this->roles)) {
            return true;
        }
        foreach ($this->roles as $i => $role) {
            if ($role === '?') {
                if ($user->getIsGuest()) {
                    return true;
                }
            } elseif ($role === '@') {
                if (!$user->getIsGuest()) {
                    return true;
                }
            } 
            else {
                return is_array($role) ? $user->can($i, $role) : $user->can($role);
            }
        }

        return false;
    }