AccessRule ошибка в обработке allows

[arkhamvm]

Класс AccessRule:

    public function allows($action, $user, $request)
    {
        if ($this->matchAction($action)
            && $this->matchRole($user)
            && $this->matchIP($request->getUserIP())
            && $this->matchVerb($request->getMethod())
            && $this->matchController($action->controller)
            && $this->matchCustom($action)
        ) {
            return $this->allow ? true : false;
        } else {
            return null;
        }
    }

Получается, что в случае если хоть одна функция в условии вернет false, на выходе получим null. В следствие этого, в AccessControl::beforeAction вызовется denyCallback не конкретного rule, а behavior'а.

Например:

            'access' => [
                'class'  => AccessControl::class,
                'rules'  => [
                    [
                        'actions'   => ['index'],
                        'allow'          => true,
                        'matchCallback'  => function($rule, $action) {
                            return false;
                        },
                                'denyCallback'   => function($rule, $action) {
                                         throw new ExceptionFOO;
                                }
                    ],
                ],
                'denyCallback'   => function($rule, $action) {
                    throw new ExceptionBAR;
                }
            ]

Вызовется ExceptionBAR.

[arkhamvm]

Возможное решение:

    public function allows($action, $user, $request)
    {
        return $this->allow
            && $this->matchAction($action)
            && $this->matchRole($user)
            && $this->matchIP($request->getUserIP())
            && $this->matchVerb($request->getMethod())
            && $this->matchController($action->controller)
            && $this->matchCustom($action);
    }

[githubjeka]

Красивое решение.

А PR можете и тестов парочку? Было бы здорово!

[SilverFire]

Мне кажется, что такое условие было сделано не случайно. Нужно проверить историю коммитов чтобы понять, зачем так сделали и что может сломаться

[githubjeka]

и null пропал в решении, а он же нужен был для чего то. Нужны тесты!

[cebe]

Not sure I understand correctly but null has a meaning here:

@return boolean|null true if the user is allowed, false if the user is denied, null if the rule does not apply to the user

null is used as neither allow or deny here: https://github.com/yiisoft/yii2/blob/7ba631b4ae3a40206e752fded8775b65491d1bcb/framework/filters/AccessControl.php#L118-L129

[samdark]

Right. So rule wasn't applied and "deny" is a result of no rules match. Then everything works as it was designed.

[arkhamvm]

@samdark, @cebe, So if "matchCallback" is only used to determine if the rule should be applied, сan I suggest additional callback to check is the rule allowed to user? Something like this:

            'access' => [
                'class'  => AccessControl::class,
                'rules'  => [
                    [
                        'actions'   => ['index'],
                        'allow'          => true,
                        'allowCallback'  => function($rule, $action) {
                            return (1 === rand(0,1));
                        },
                    ],
                ],
            ]

    public function allows($action, $user, $request)
    {
        if ($this->matchAction($action)
            && $this->matchRole($user)
            && $this->matchIP($request->getUserIP())
            && $this->matchVerb($request->getMethod())
            && $this->matchController($action->controller)
            && $this->matchCustom($action)
        ) {
            return ($this->allow && $this->allowCustom) ? true : false;
        } else {
            return null;
        }
    }

    protected function allowCustom($action)
    {
        return empty($this->allowCallback) || call_user_func($this->allowCallback, $this, $action);
    }

If this option is acceptable, i can create pull request.

[arkhamvm]

In additional, it would be nice, if "allow" variable can be closure (function ($rule, $action)) too, "allowCustom" is would not be necessary. Sample:

            'access' => [
                'class'  => AccessControl::class,
                'rules'  => [
                    [
                        'actions'   => ['index'],
                        'allow'          =>  function($rule, $action) {
                            return (1 === rand(0,1));
                        },
                    ],
                ],
            ]

    public function allows($action, $user, $request)
    {
        if ($this->matchAction($action)
            && $this->matchRole($user)
            && $this->matchIP($request->getUserIP())
            && $this->matchVerb($request->getMethod())
            && $this->matchController($action->controller)
            && $this->matchCustom($action)
        ) {
            if (is_object($this->allow) && ($this->allow instanceof Closure)) {
                return call_user_func($this->allow, $this, $action) ? true : false;
            }
            return $this->allow ? true : false;
        } else {
            return null;
        }
    }

[samdark]

Isn't "is the rule allowed" === "if the rule should be applied"?

i.e. Rule is attached to permission. If permission is on the path from what's checked to user role in the RBAC hierarchy then Rule will be executed.

[arkhamvm]

@samdark, please, can you explain, do i understand correctly: there is no way to call "denyCallback" if "matchCallback" return "false", and this option does not planned?

[cebe]

you can use a rule that denies if it matches:

 'access' => [
                'class'  => AccessControl::class,
                'rules'  => [
                    [
                        'allow'          => false,
                        'matchCallback'  => function($rule, $action) {
                            return (1 === rand(0,1));
                        },
                        'denyCallback' => function() { ... }
                    ],
                ],
            ]

[samdark]

@arkhamvm yes. If permission wasn't matched, rule isn't executed. That's by design.

[arkhamvm]

@samdark, @cebe, thanks for explanation.