laminas / laminas-permissions-rbac

Provides a role-based access control management
https://docs.laminas.dev/laminas-permissions-rbac/
BSD 3-Clause "New" or "Revised" License
30 stars 16 forks source link

Child roles not added #3

Open weierophinney opened 4 years ago

weierophinney commented 4 years ago

In v3.x, if you create a role, add a child role too it, then add the parent role to the Rbac instance, the child role is not added. Using your own example code:

use Zend\Permissions\Rbac\Rbac;
use Zend\Permissions\Rbac\Role;

$rbac = new Rbac();
$foo  = new Role('foo');
$bar  = new Role('bar');

$foo->addChild($bar);
$rbac->addRole($foo);

echo $rbac->hasRole("bar") ? "works" : "doesn't work";

This displays "doesn't work". Similarly, attempting a permission check against role "bar" raises a "No role with name 'bar' could be found" exception.

This worked in 2.x


Originally posted by @jmaybury at https://github.com/zendframework/zend-permissions-rbac/issues/45

weierophinney commented 4 years ago

@jmaybury I've checked the issue and I think it is a valid one. I mean I am not sure if it is going to be added back to v3 or not, but if not - the change is not documented. The problem was introduced in PR #34 and that change was never noted. I was looking also on changes in the documentation: https://github.com/zendframework/zend-permissions-rbac/pull/34/files#diff-fb6293f79cf977d58bdcbddfe2ab7c94 and there is said:

hasRole - Recursively queries the RBAC for the given role, returning `true` if found, `false` otherwise.
getRole - Get the role specified by name, raising an exception if not found.

Before getRole was also recursive. Of course it has no sense, that one method is recursive and the other not, because we can finish up with case when: hasRole(xyz)==true and getRole(xyz) does not return the role (this is not the implementation either, thankfully).

I would ping here the author of the change - @ezimuel, so maybe he can explain if it was desired change for v3, or not. If so, probably we would need update the migration guide and the documentation, also add some tests to cover that behaviour (as now there is no any).


Originally posted by @michalbundyra at https://github.com/zendframework/zend-permissions-rbac/issues/45#issuecomment-517913807

weierophinney commented 4 years ago

Jus to link PR: #46


Originally posted by @michalbundyra at https://github.com/zendframework/zend-permissions-rbac/issues/45#issuecomment-518509070

weierophinney commented 4 years ago

@webimpress I agree this is a bug. The #46 it doesn't fix the issue in my opinion. We need to fix this is addRole() and not in hasRole(). I'll send a PR in a while.


Originally posted by @ezimuel at https://github.com/zendframework/zend-permissions-rbac/issues/45#issuecomment-518560827

weierophinney commented 4 years ago

@webimpress, @jmaybury Just send the PR #48 to fix this issue.


Originally posted by @ezimuel at https://github.com/zendframework/zend-permissions-rbac/issues/45#issuecomment-518571779

badmansan commented 4 years ago

So, where is the fix commit?