Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Possible RBAC design problem #742

Closed
boazrymland opened this Issue · 15 comments

5 participants

@boazrymland

General

I've written a forum post that received no replies that explains the issue a bit more forum post. I think this is an important issue and that's why I'm raising this bug.

Description

The initial RBAC setup is simple - have two roles - guest and authenticated. authenticated inherits all from guest so it adds a few more operations on top of the several operations that guest can do. guest and authenticated roles has a nice bizrule that can make the "defaultRoles", meaning no need to attach specifically to every new user on the system.
This should be quite a powerful setup. It can be summed up in the following diagram:

Authenticated (do D, do E)  [bizrule=return !Yii::app()->getUser()->getIsGuest();]
  ^
  |
Guest (do A, do B, do C) [bizrule=return Yii::app()->getUser()->getIsGuest();

Now, all good and promising. But:
CDbAuthManager::checkAccessRecursive() runs in a recursive loop bubbling upward with the "parents" of this authorization item, and runs the bizrule if one is found. If the bizrule returned true - access is granted. If false - an immediate reply of "no access" is determined.
When run on the scheme above, if the authenticated user tries to execute an operation who is allowed to guest (e.g. "do B"), it will be denied access. Why? Because the bizrule for the guest will deny fail, and a negative answer will be returned.

So in short, what are you saying Boaz?

Now if I understood it all correctly, here's my conclusions in the form of "the essence of the problem" and a "possible design modification to resolve the problem":

Essence of the problem

Essentially, the problem is that there are only two types of "answers" that are defined in the interface between the bizrule and checkAccessRecursive(). The two answers are "yes - grant access" and "no - deny access". Actually, its not the bizrule that say this, but since only bool is allowed to be returned from the bizrule, those are in fact the only two possible interpretations of the answers.
The problem is a missing third option "I don't know". And in a more verbose anwser: "l don't know - please continue the recursive check. Maybe someone above me (my parents) will either specifically grant the permission, or specifically deny it".

Possible design modification

If, for example, a value of NULL was allowed to be returned from the bizrule (that is, one can return it but it will be cast as false... :) ), we could have the bizrule that is assigned to Guest in the above example be "if (Yii::app()->getUser()->getIsGuest()) {return true;} else {return NULL;}", this "null" value could serve as the third type of answer "I don't know".
For that, checkAccessRecursive() will need to to be updated as well, naturally. The "if" clause on the bizrule execution line will need to be expanded to evaluate the bizrule, check for true, false, or null. In case of null, probably a mere "continue;" will do. In case of true of false, current behavior should be retained.

Whee, that was a long one, wasn't it? I tried to be precise and well-phrased but couldn't make it shorter :)

@boazrymland

Hmm... . In continuation to my suggestion above, we can also consider the following design:
have the same two answers as now: true and false. true will stand for "permission granted", false will mark "I don't know", which will result in continuation of recursive loop until either a "yes, permission granted" auth item will be found, or, until tree is exhausted, which will result in the default negative answer.
The difference between this option and the one in the original issue description, above, is that this will traverse the full tree more often. In other words, the tradeoff is simpler code and simpler coding needed from the developer (this option), vs. better performance (original option).

@mdomba
Collaborator

First of all the forum has a separate section for possible bug discussion... you posted in the general section, so your post got unnoticed by the dev team (at least from me)...

I don't see this as a problem at all. You clearly set the biz rule of action A to allow only guests, and authenticated users are not guests...

I wonder why would you set that rule at all... if a guest is allowed to do those actions... just remove that entry... keep only those for the authenticated users...

@boazrymland

Its your second paragraph that I'll focus on: one of the greatest things about RBAC is that its "inheritance" concept is similar to that used object oriented programming. This means its easy for OO developers to grasp it and harness its power.

Your statement that "authenticated users are not guest" contradicts this valuable advantage of inheritance. On the contrary: authenticated user are guest and should be so. As such, they add just a few more operations to what this user can do: D and E, which are added on top of what Guest could do: A, B and C.
Designing as I just explained makes authenticated user "a" guest, in terms of RBAC inheritance, and for good reasons.

I know that on the face of it, this issue indicates a not-a-minor flaw in how the RBAC tree is checked and traversed in Yii (but only when bizrules are present).

@mdomba
Collaborator

So by inheritance you would like that bizrules gets overriden by the childs ?

IMO inheritance means accumulation... so in your case an authenticated user should be non-guest and guest at the same time... but as you use the method isGuest() specifically.. this one does return true for authenticated users and false for guests... so your bizrule is the main problem...

Again - why set the guest bizrule at all when this actions can be accessed from everyone (guests and non-guests)

@boazrymland

So by inheritance you would like that bizrules gets overriden by the childs ?

Nope, I would like by inheritance that bizrule could (also) say - "neither" (don't allow, don't disallow).

BTW, I'm thinking now - maybe this whole thing should apply to bizrule that are attached to roles only? (because I guess not many are using inheritance between operations and tasks. IMHO its just complicating stuff while flexibility and power of inheritance between roles only is no more than I need).

IMO inheritance means accumulation... so in your case an authenticated user should be non-guest and guest at the same time... but as you use the method isGuest() specifically.. this one does return true for authenticated users and false for guests... so your bizrule is the main problem...

I'm not sure I understood you: Note that the bizrule for guest and authenticated in the example I gave above are different. The delta is the simple little "!" that negate the whole thing :) .
so the bizrule I used above will work as they supposed to. Using isGuest one can tell precisely if the user is Guest (and only guest since the 'id' property of non guest is supposed to be non-null) or if its logged in (and thus Authenticated user).

Again - why set the guest bizrule at all when this actions can be accessed from everyone (guests and non-guests)

That correct, guest role actions require no explicit auth items. Still, for the sake of completeness and easr of maintenance its better to have those in the first place: if tomorrow my client would like that "do A" is applicable to non-guest users (not necessarily authenticated - we have lots of roles). This would be a simple matter of configuration he could do alone as I utilize the nice RBAM module/extension. No developer will need to enter "checkAccess()" statements in the relevant actions. They would be already in.

@mdomba
Collaborator

I know that they are different... that's the point of my writing... as in the end you want that "authenticated" can execute action A that by your rules is "isGuest() && !isGuest()"

So to "again" make a summary of all this... if the action is available to guests... just remove the bizrules completely... why would you set a rule there ?

@boazrymland

Ok, I think I got your first point. True that if you mix the rules they look as if they negate each other but that's fine as indeed Guest is not Authenticated (they are kind of opposites).

Its also true that I could remove the bizrule for the Guest. This will solve my problem (as I already did, BTW...) but the generic problem still remains. It could happen for higher level roles in someone's project. That's a hard to find bug once it manifests. The problem in a nutshell: An operation belongs to role X but the checked user is of role Y which is a parent of X. If the bizrule of X will return false for the user of role Y the bug will occur.

BTW, I already for a PM from another user that (probably) bumped on the same issue so its not only me. See this forum thread.

@mdomba
Collaborator

I again say that this is not a bug... it's a logic problem created by the developer...

@mdomba mdomba was assigned
@mdomba mdomba closed this
@boazrymland

I was just referred to the official docs about RBAC business rules. The official documentation recommend on the business rule that I gave above (for Guest). The only thing missing there from having a the same complete recipe as I gave above is the hierarchy/inheritance between Authenticated and Guest.

@mdomba
Collaborator

I see your point...

@qiangxue, @samdark please check the last section "default roles"... there is the example with the guest and authenticated users... would be better to change this part as it can lead to troubles explained on this issue?

maybe make a check for authenticated and admin ?

@qiangxue
Owner

Yes, I agree using "authenticated" and "admin" would be a better example.

@samdark samdark reopened this
@samdark
Owner

Reopened, tagged as documentation issue.

@mdomba
Collaborator

No need to keep this issue open as the description would let others think there is a bug (if they do not read all the comments)...

@qiangxue, @samdark Please review the new description I added

@mdomba mdomba closed this
@qiangxue
Owner

Looks fine to me. Thanks!

@zodimo

Hi, I ran into the same problem. (I use yii 1.1.14 ) I made the following change in CDAuthManager.

in checkAccessRecursive replaced if($this->checkAccessRecursive($parent,$userId,$params,$assignments)) with if($this->checkAccessRecursiveParent($parent,$userId,$params,$assignments)).

and created checkAccessRecursiveParent as a copy of checkAccessRecursive without the bizrule check. What this does is it will inherit the permissions correctly while only applying the bizrule that directly applied to the authitem. I hope this helps someone.

/**
 * Performs access check for the specified user.
 * This method is internally called by {@link checkAccess}.
 * @param string $itemName the name of the operation that need access check
 * @param mixed $userId the user ID. This should can be either an integer and a string representing
 * the unique identifier of a user. See {@link IWebUser::getId}.
 * @param array $params name-value pairs that would be passed to biz rules associated
 * with the tasks and roles assigned to the user.
 * Since version 1.1.11 a param with name 'userId' is added to this array, which holds the value of <code>$userId</code>.
 * @param array $assignments the assignments to the specified user
 * @return boolean whether the operations can be performed by the user.
 * @since 1.1.3
 */
protected function checkAccessRecursive($itemName,$userId,$params,$assignments)
{
    if(($item=$this->getAuthItem($itemName))===null)
        return false;
    Yii::trace('Checking permission "'.$item->getName().'"','system.web.auth.CDbAuthManager');
    if(!isset($params['userId']))
        $params['userId'] = $userId;
    if($this->executeBizRule($item->getBizRule(),$params,$item->getData()))
    {
        if(in_array($itemName,$this->defaultRoles))
            return true;
        if(isset($assignments[$itemName]))
        {
            $assignment=$assignments[$itemName];
            if($this->executeBizRule($assignment->getBizRule(),$params,$assignment->getData()))
                return true;
        }
        $parents=$this->db->createCommand()
            ->select('parent')
            ->from($this->itemChildTable)
            ->where('child=:name', array(':name'=>$itemName))
            ->queryColumn();
        foreach($parents as $parent)
        {
            if($this->checkAccessRecursiveParent($parent,$userId,$params,$assignments))
                return true;
        }
    }
    return false;
}

/**
 * Performs access check for the specified user.
 * This method is internally called by {@link checkAccessRecursive}.
 * The function is to keep parent permissions while only applying the bizrule directly related to the authitem
 * @param string $itemName the name of the operation that need access check
 * @param mixed $userId the user ID. This should can be either an integer and a string representing
 * the unique identifier of a user. See {@link IWebUser::getId}.
 * @param array $params name-value pairs that would be passed to biz rules associated
 * with the tasks and roles assigned to the user.
 * Since version 1.1.11 a param with name 'userId' is added to this array, which holds the value of <code>$userId</code>.
 * @param array $assignments the assignments to the specified user
 * @return boolean whether the operations can be performed by the user.
 */

protected function checkAccessRecursiveParent($itemName,$userId,$params,$assignments)
{
    if(($item=$this->getAuthItem($itemName))===null)
        return false;
    Yii::trace('Checking permission "'.$item->getName().'"','system.web.auth.CDbAuthManager');
    if(!isset($params['userId']))
        $params['userId'] = $userId;

        if(in_array($itemName,$this->defaultRoles))
            return true;
        if(isset($assignments[$itemName]))
        {
            $assignment=$assignments[$itemName];
            if($this->executeBizRule($assignment->getBizRule(),$params,$assignment->getData()))
                return true;
        }
        $parents=$this->db->createCommand()
            ->select('parent')
            ->from($this->itemChildTable)
            ->where('child=:name', array(':name'=>$itemName))
            ->queryColumn();
        foreach($parents as $parent)
        {
            if($this->checkAccessRecursive($parent,$userId,$params,$assignments))
                return true;
        }

}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.