-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Role Demo - Possible incorrect behavior #51
Comments
Why in the RbacManager.php after checking the direct permissions and assertions, the code follows checking of the parentRoles? I think that if the role has the direct permissions, it's not necessary to check whether the permission which I'm testing belongs to any its parent roles? public function isGranted($user, $permission, $params = null)
|
Just out of curiosity, should you not also check for assertions when going thru the parent roles? |
Hello Oleg, Although it's been some time :) i can confirm this issue. The problem is your implementation of inheritance assumes Children roles inherit from Parrent roles, and zend-permission-rbac asssumes parrents inherit all permissions from children. i have fixxed this by fiddling with the Role entity, RoleManager and RoleController if i remember correctly. Basicly, anywhere you "add parrent", you need to "addChild" and vice-versa :) awsome book by the way |
That is indeed the problem.
to this:
Or you could rename everything in this implementation to reflect the requirements. |
Fixxed that one :).
The problem i seem to have now is that the inheritance work only one level
down. Parent inherit from direct children, and only the right the children
have, not the ones they inherit from their own children. Hope that makes
sense.
…On Sat, Jul 20, 2019, 17:30 Sebastiaan ***@***.***> wrote:
Hello Oleg,
Although it's been some time :) i can confirm this issue.
The problem is your implementation of inheritance assumes Children roles
inherit from Parrent roles, and zend-permission-rbac asssumes parrents
inherit all permissions from children.
i have fixxed this by fiddling with the Role entity, RoleManager and
RoleController if i remember correctly. Basicly, anywhere you "add
parrent", you need to "addChild" and vice-versa :)
awsome book by the way
That is indeed the problem.
Least effort way of fixings this is by changing rbacManager init function
from this:
$parentRoleNames = [];
foreach ($role->getParentRoles() as $parentRole) {
$parentRoleNames[] = $parentRole->getName();
}
to this:
$parentRoleNames = [];
// Our inheritance code assumes childRoles inherit from parentRoles (sounds logical)
// Zends inheritance code assumes parentRoles inherit from childRoles (unlikely irl)
// Can't expect Zend to change their code and break existing implementations
// So this is where we swap the expected getParentRoles() with getChildRoles()
foreach ($role->getChildRoles() as $parentRole) {
$parentRoleNames[] = $parentRole->getName();
}
Or you could rename everything in this implementation to reflect the
requirements.
Either way *if you do not fix this you will end up with serious security
issues* where in situations when admins inherit read permissions from
guests, guests end up with inheriting write permissions from admins.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#51?email_source=notifications&email_token=ADJ3CV2MVCZUGB5PEBP7U7LQAMOR5A5CNFSM4FPRMGCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2NPOTY#issuecomment-513472335>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADJ3CVYWZRNN3ECKAHUPVT3QAMOR5ANCNFSM4FPRMGCA>
.
|
ok, fixxed that one too. Apparently, the way Zend creates the Rbac Object is rather wiered. fixed it by going through the roles in reverse order. RbacManager.php lines 83-84 $roles = $this->entityManager->getRepository(Role::class) |
@x-dragos
Note: I work with a similar setup where admins inherit from managers and managers inherit from clients. |
I'll have another look but at the moment ... it looks to me like Rbac is not creating the proper object. it overwrites the roles that it created earlyer in the process with the "last" inheritance... and it seems to miss relationships defined early in the process... at least that is how it looks in the debugger. i'll keep looking and get back to you. (chaning the order it iterated through the ids made it work for 3 roles inheriting from eachother... but when you add a 4th in between them :) it stops working again) |
I've not been in this code for a while but i do not think the sorting order is supposed to have have any impact on that. |
Well i don't know how much code you have modified.
This config is read when default permissions, roles and accounts are created. (in that specific order) I also added a level definition to the roles so that managers can add roles to clients but never a role that is of equal or lower level than their own role. And instead of checking for default accounts on every login attempt I have a SetupController that listens to /setup/ route that is only available if no accounts exist, otherwise returns notFound or notAuthorized. I chose to work with a predefined set of permissions and roles because adding permissions using the interface doesn't make a lot of sense since adding a permission that way does not magically add the code to check for any of them. |
Hello Oleg, I figured it out. The problem was generated by the init() in RbacManager.php, specifically the fact that it had "$rbac->setCreateMissingRoles(true);" and later it was adding a role even if it was allready created, from a previous iteration. This ment that if a role was created early as a parent for another role (as it was missing at that point), it would get overwritten when the foreach loop was reaching it through iteration, loosing the child that was previously added on it. i replaced the single line that was adding roles in the init():
with this block that is handling the creation properly (specifically, if the role allready exists, it modifies that role in the rbac object, not overwriting it):
I haven't modified your code too much except chainging the parrent <-> child thing that i talked about earlyer :) This module saved me a lot of work, so thanks again for creating it. i may at some point actually redownload the repo version and create a pull request with a few fixxes. (not a big git guru... so not sure how to submit them otherwise... or even if it is possible to create the pull request from my own project) |
@x-dragos Try the follwing.. In RbacManager->init:
In RbacManager add a method that can be called recursively:
Tested with up to 5 levels of inheritance with database records in random order. |
I see what you did, but i think it is the same. Can you give me an example where my code would fail. As you said, the iteration order should not matter and it doesn't with the block in my last example. If the parrent has parrents it will go on the else branch when the main iteration gets to it and add them to the existing object:
There is only a diffrence in presentation in my opinion :) both pieces of code do the same thing. The only functional diffrence i can see is that you dont need
as you recursivly add the full parent objects. |
PS: infinite recursion is beeing tanken care of my Zend Rbac as it doesnt add circular references (it does this check for us :) ), but it would be safe in my example. If you add a circular reference, with your example it might actually loop infinitely as it doesn't check for it. As in my example it doesn't recursively call itself, it will just exit the loop when it finishes one run of all roles and all the circular references are being handled by Zend\Permission\Rbac |
@x-dragos
Children should not be empty. |
hmmm ... can you post the full Manager Object role? want to check who is child for that one and who is parrent |
The code i use now is: (RbacManager->init)
This sets up roles with proper inheritance:
Once the roles are setup with proper inheritance you no longer need the code at the bottom of RbacManager->isGranted
[EDITED]
|
well.. it apears to be in order. The only diffrence from my code is that i do the second loop inside the fist one :):
both solutions work |
@x-dragos nope.
|
that is not true. for the sake fo this example lets asume Agent -> Manager -> Administrator as 3 roles , inheriting eachother, Admin beeing the top parent. in the first loop, when adding agent with this parent (Manager) , due to : $rbac->setCreateMissingRoles(true); it will add the Manager role also. At that point there will be 2 roles already in the rbac object (Agent with parent Manager, and Manager with child Agent). Manager will not have it's parrent yet. on the second loop, it will go to the else branch, as Manager already exists, and only add Admin as it's parrent. Hope i have made this as clear as i can :) as i can post a recording of the debuger, but if you put a breakpoint in init, and delete the cache... add a watch on $rbac and see it's memebers change while looping. |
My bad, it's clear, i just assumed the setCreateMissingRoles option was set to false. |
I have never used Doctrine so I could be completely wrong, but I think this may be caused by a mix-up in annotations for the /**
* @ORM\ManyToMany(targetEntity="User\Entity\Role", inversedBy="childRoles")
* @ORM\JoinTable(name="role_hierarchy",
* joinColumns={@ORM\JoinColumn(name="child_role_id", referencedColumnName="id")},
* inverseJoinColumns={@ORM\JoinColumn(name="parent_role_id", referencedColumnName="id")}
* )
*/
private $parentRoles;
/**
* @ORM\ManyToMany(targetEntity="User\Entity\Role", mappedBy="parentRoles")
* @ORM\JoinTable(name="role_hierarchy",
* joinColumns={@ORM\JoinColumn(name="parent_role_id", referencedColumnName="id")},
* inverseJoinColumns={@ORM\JoinColumn(name="child_role_id", referencedColumnName="id")}
* )
*/
protected $childRoles; Shouldn't the joinColumn for the If this is true, you won't have to confuse yourself in the RbacManager with code like |
the definition looks ok to me.
…On Fri, Aug 16, 2019 at 4:45 PM billmbillson ***@***.***> wrote:
I have never used Doctrine so I could be completely wrong, but I think
this may be caused by a mix-up in annotations for the $parentRoles and
$childRoles properties in the Role entity
(roledemo/module/User/src/Entity/Role.php):
/** * @Orm\ManyToMany(targetEntity="User\Entity\Role", inversedBy="childRoles") * @Orm\JoinTable(name="role_hierarchy", * ***@***.***\JoinColumn(name="child_role_id", referencedColumnName="id")}, * ***@***.***\JoinColumn(name="parent_role_id", referencedColumnName="id")} * ) */ private $parentRoles; /** * @Orm\ManyToMany(targetEntity="User\Entity\Role", mappedBy="parentRoles") * @Orm\JoinTable(name="role_hierarchy", * ***@***.***\JoinColumn(name="parent_role_id", referencedColumnName="id")}, * ***@***.***\JoinColumn(name="child_role_id", referencedColumnName="id")} * ) */ protected $childRoles;
Shouldn't the joinColumn for the $parentRoles be "parent_rold_id" and the
joinColumn for $childRoles be "child_role_id"?
If this is true, you won't have to confuse yourself in the RbacManager
with code like
foreach ($role->getChildRoles() as $parentRole).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#51?email_source=notifications&email_token=ADJ3CVZPB75MNBCWG5G63YTQE2VRDA5CNFSM4FPRMGCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4OU4GA#issuecomment-522014232>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADJ3CV3F3DJDHJ755GPGOITQE2VRDANCNFSM4FPRMGCA>
.
|
Yes, it works for inheritance for roles |
As reported here https://olegkrivtsov.github.io/using-zend-framework-3-book/html/en/Role_Based_Access_Control/Summary.html#comment-4038175770
The text was updated successfully, but these errors were encountered: