Design flaw in RBAC bizrules (user is not available)

[cebe]

When trying to check access for a user that is not the one currently logged in (his Id is not Yii::app()->user->getId()) and a bizrule comes in to check if a role assignment applies for this user I am not able to check things with this users id.

Bizrules should not check for Yii::app()->user->... but for $user-> which has to be made available in the scope where bizrule is executed:

http://www.yiiframework.com/doc/api/1.1/CAuthManager#executeBizRule-detail

[mdomba]

I'm not sure I understand your issue... check the code of executeBizRule(), it just eval()'s the code you provide....

[cebe]

yep, but the only variables available are $params and $data, there is no chance to get the userId for which the bizrule is checked.

[mdomba]

The idea of the access checking is to check if the current logged user can execute the requested action.

Please explain your usage - why would you check access for another user?

[cebe]

Example: You have users stored in DB, User is an active record class. My role rTest has a bizrule Yii::app()->user->property == 5 This role is assigned to my User with id 1.

<?php
$user = User::model()->findByPk(1);
Yii::app()->authManager->checkAccess('rTest', $user->id);
Bizrule is executed, but checks for Yii::app()->user which is not the one with id=1
There is no way to check something for the `$user` whos Id is given to checkAccess.
Hope it is clear now.

[cebe]

My use case is to for hide links from users which they are not allowed to access. For example when sending an email to a user to notify him about some action the currently logged in user is different from the one I want to check access for to decide if I want to display a link or not. If he is allowed to perform an action, he gets different options in the mail than if he was not allowed.

[mdomba]

You are checking if a user has access to some action to decide on the content of the email.... I'm not sure this use case belong in the access checking...

The idea behind access checking is to check the current user when he try to access a method. That's even explained in the guide - http://www.yiiframework.com/doc/guide/1.1/en/topics.auth#using-business-rules

The returning value of the code is used to determine if the role or assignment applies to the current user.

I would like to hear what @qiangxue and @samdark think about this...

@cebe Do you have a solution in mind for your use case?

[kidol]

@mdomba What you quoted refers to CWebUser::checkAccess(). He uses CAuthManager::checkAccess(), i.e. he checks access for a random user.

@cebe In your bizrule you can use $params['userId'], but you have to pass it to checkAccess() manually. That would look like this:

Yii::app()->authManager->checkAccess('rTest', $user->id, array('userId' => $user->id));

So the question seems to be: Why do we need to pass the $userId 2 times? Every time we checkAccess() we provide a user id already, so why does the signature of executeBizRule() have no $userId?

[cebe]

So the question seems to be: Why do we need to pass the $userId 2 times? Every time we checkAccess() we provide a user id already, so why does the signature of executeBizRule() have no $userId?

ACK, thats the point, thanks :-)

[mdomba]

Oh, seems I need a good nap today :D

I got your point... yes looking your way it seems there is a bit of inconsistency as the checkAcess receives the userId but it's not passed to the bizRule...

Not sure how to solve this without breaking BC... maybe by doing what @kidol suggested in the checkAcess when calling evaluateBizRule.

[samdark]

Affected methods are CDbAuthManager::checkAccessRecursive and CPhpAuthManager::checkAccess. Fix is as simple as adding the following after Yii::trace:

if(!isset($params['userId']))
    $params['userId'] = $userId;

Of course we'll need to update docs after doing that.

What do you think?

[cebe]

@samdark for BC compatibility this would be the best solution I think. In Yii2 it would be much nicer if the user object was available to be able to call methods on it, like we did with Yii::app()->user->... I'd like to call $user->... instead.

[intel352]

Agreed with @cebe, as it's convention elsewhere within Yii for $user to be available and already populated, so at least for Yii 2, would make quite a bit of sense.

[kidol]

@cebe @intel352 Where to take the CWebUser object from when you check a random user? In your use case you only have $user->id. That's all the information there is. So $userId only would be fine I think since it's always required when calling checkAccess().

Even though samdarks solution is good, I have the feeling the current implementation has a reason. For example there is CAuthItem::checkAccess() as well which has a different signature. Without fully understanding the CAuth* stuff I can't tell whether it's a good idea to implement this.

Maybe @qiangxue can tell us if there is a reason for not having $userId in the signature.

[intel352]

@kidol, CAuthItem is more of a generic API. CAuthItem::checkAccess() is concerned with checking access for a role, task, or operation. It has a different purpose and concern than CDbAuth and CPhpAuth, which concerned with user-specific access.

I will say, it is quite annoying to not have access to the passed $userId in CDbAuth and CPhpAuth's checkAccess methods. I don't even see it used in CDbAuth.

[cebe]

@kidol: Where to take the CWebUser object from when you check a random user? In your use case you only have $user->id. That's all the information there is. So $userId only would be fine I think since it's always required when calling checkAccess().

Thats a problem I am facing by making it generic for everyone. I in my case have an AR User model which I bind to CWebUser by extending CWebUser and overwriting get + set. So I'd like to do something like this:

<?php
$webUser = new WebUser(); // extends CWebUser and adds property $model
$webUser->model = User::model()->findByPk(1);
$webUser->checkAccess('rTest');

the checkAccess() method could then call CDbAuthManager like this (passing the CWebUser object instead of users ID):

Yii::app()->authManager->checkAccess($authItem, $this, $params);

[kidol]

@intel352 Thanks for explanation.

@cebe

the checkAccess() method could then call CDbAuthManager like this (passing the CWebUser object instead of users ID):

If CAuthManager::checkAccess() requires a CWebUser object, how does it work without CWebUser? We could include the user object in the $params for the CWebUser::checkAccess() method, but does it make sense? Yii::app()->user is available everywhere...

[cebe]

@kidol If CAuthManager::checkAccess() requires a CWebUser object, how does it work without CWebUser?

Thats what I ment with "problem I am facing by making it generic for everyone". At least a dependency on a User object that implements IWebUser has to be there. But that should not be a big problem since it only requires getName(), getId(), getIsGuest() and checkAccess() which you normally always have for a user.

[...] but does it make sense? Yii::app()->user is available everywhere...

Thats the root cause why I opened this ticket, Yii::app()->user is not the one I want, when checking for someone who is not logged in in my current request.

[qiangxue]

How about renaming userId to be 'user' in $params, which makes the typing less error prone? The reason that userId was not exposed to biz rule was because checkAccess() only checks the roles that are assigned to the specified user. I guess you met this issue because you are using default roles which relies on bizrule to define the role assignment. This was a feature added after a few releases of the initial RBAC implementation.

[cebe]

How about renaming userId to be 'user' in $params, which makes the typing less error prone?

Would not do this since I'd expect 'user' to be an instance of CWebUser as described above and 'userId' is explicit about what it is: an ID.

The reason that userId was not exposed to biz rule was because checkAccess() only checks the roles that are assigned to the specified user. I guess you met this issue because you are using default roles which relies on bizrule to define the role assignment.

No, I am using bizrules to check if a role assigned to a user applies in the current context. Lets take the editOwnPost-example of Definitive Guide:

The bizrule described in that example is: return Yii::app()->user->id==$params["post"]->authID; This one will work, when I check if the currently logged in user can edit his own post with Yii::app()->user->checkAccess('editOwnPost', array('post'=>$post)).

What I needed is to be able to check if an other user could access an action to edit his own post. For example in an email or in a cronjob where I generate a task for a user to review the post. In these cases Yii::app()->user is not available in the bizrule and would not give the expected result.

---

Just came up with a better explaination: It is okay, if Yii::app()->user->checkAccess(...) relies on Yii::app()->user to be present which is the normal case in WebApplication. But why should Yii::app()->authManager->checkAccess(..., $userId) rely on that Yii::app()->user is the same user as $userId. A better design would provide a $user object inside.

[qiangxue]

RBAC doesn't rely on Web application. We use userID instead of user object because we try to make it independent of any specific user object design. So it's not a good idea to expose CWebUser as a $user object in bizrule as CWebUser is mainly used in Web application and represents user session information.

[cebe]

RBAC doesn't rely on Web application.

I know that, but all the bizRule examples use Yii::app()->user which is not independent anymore. RBAC could rely on a user object that implements IWebUser or even an other interface that only needs to require getId(). Then it would be able to expose the user object as $user in the bizRule.

btw: for Yii 1.1 I am okay with the current solution, my thoughts about the User object are mainly about how it could be in Yii2.