zendframework / zend-permissions-acl

BSD 3-Clause "New" or "Revised" License
60 stars 23 forks source link

Added test to show order of multiple parents reads backwards #31

Closed TomHAnderson closed 6 years ago

TomHAnderson commented 6 years ago

This fix covers every possible way of entering roles because it evaluates all roles and/or tree of roles every time and if any role returns true then the ACL returns true.

Should no role return true and any role return false the ACL returns false.

Should all roles return null then the ACL returns null.

TomHAnderson commented 6 years ago

I think #32 is a better fix.

TomHAnderson commented 6 years ago

Closing in favor of #32

TomHAnderson commented 6 years ago

Reopening; #32 causes many side affects.

ezimuel commented 6 years ago

@TomHAnderson the unit test case that you proposed contains an "inconsistent" ACL configuration. You basically created an admin role that inherits from guest and user roles. Then you allowed user to access resource with privilege assert, and you denied guest to access the same resource with the same privilege assert, as follows:

use Zend\Permissions\Acl;
use Zend\Permissions\Acl\Role;
use Zend\Permissions\Acl\Resource;

$acl = new Acl();
$acl->addRole(new Role\GenericRole('guest'));
$acl->addRole(new Role\GenericRole('user'), 'guest');
$acl->addRole(new Role\GenericRole('admin'), ['user', 'guest']);

$acl->addResource(new Resource\GenericResource('resource'));
$acl->allow('user', 'resource', 'assert');
$acl->deny('guest', 'resource', 'assert');

What do you expect from asking if admin is allowed to access resource with privilege assert?

$acl->isAllowed('admin', 'resource', 'assert'); // should returns true or false?

The response can be true (if you use the user children) or false (if you use the guest children) at the same time.

The actual implementation on zend-permissions-acl returns false because it checks Role inheritance using a depth-first traversal. The highest priority parent (i.e., the parent most recently added) is checked first (guest in our example). If you invert the order of parents for admin, as follows:

$acl->addRole(new Role\GenericRole('admin'), ['guest', 'user']);

The response will be true this time. IMHO, this is not a issue of zend-permissions-acl but a configuration problem of the ACL example.

TomHAnderson commented 6 years ago

Thanks, this answers my question. To restate, my assumption would be the highest priority parent would be the parent added first, not last. So if you're sure that the highest priority parent is the last element in the passed array then I'll change api-skeletons/zf-oauth2-doctrine-permissions-acl to order roles least to most. Please give me that assurance and I'll close this ticket.

TomHAnderson commented 6 years ago

The original complaint here was the roles read backwards but it is clear in the documentation that roles by priority read right to left.