zendframework / zend-permissions-acl

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

[wip] Hotfix/alternative fix for order of parents seems backwards #32

Closed TomHAnderson closed 6 years ago

TomHAnderson commented 6 years ago

An alternative fix instead of https://github.com/zendframework/zend-permissions-acl/pull/31

TomHAnderson commented 6 years ago

This fix does not care about the order roles were added in. By ignoring which role returns false first and continuing to look for a role to return true every role gets evaluated without respect to the registration order.

TomHAnderson commented 6 years ago

This PR currently fails because after applying my fix a different test fails: https://github.com/zendframework/zend-permissions-acl/blob/master/test/AclTest.php#L997

I'll continue to investigate to find the cause of this failure.

TomHAnderson commented 6 years ago

This fix shows problems with the unit tests. This is the failing test: https://github.com/TomHAnderson/zend-permissions-acl/blob/a5f3dfff673071ae90466e386aba19afe76b8b71/test/AclTest.php#L997

$this->assertTrue($this->acl->isAllowed('staff', 'profiles', 'revise'));

The reason this is failing is because the 'profiles' resource was added just-above this line and no rules were set against it. That means the existing rules should take over. The existing rules say https://github.com/TomHAnderson/zend-permissions-acl/blob/a5f3dfff673071ae90466e386aba19afe76b8b71/test/AclTest.php#L962

$this->assertTrue($this->acl->isAllowed('staff', null, 'revise'));

This means that every resource of permission revise for staff should be true. However the ACL getRules only uses the allResources if the resource is null. **There is no check to allow all a permission to a role for all resources: https://github.com/TomHAnderson/zend-permissions-acl/blob/a5f3dfff673071ae90466e386aba19afe76b8b71/src/Acl.php#L1077

I tried correcting this by changing the do loop to:

        do {
            if (null === $resource) {
                $visitor =& $this->rules['allResources'];
                break;
            }
 // begin new code
            $resourceId = $resource->getResourceId();
            if (! isset($this->rules['byResourceId'][$resourceId]) || ! sizeof($this->rules['byResourceId'][$resourceId])) {
                $visitor =& $this->rules['allResources'];
                break;
            } else {
                print_r($this->rules['byResourceId'][$resourceId]);
            }
// end new code
            $resourceId = $resource->getResourceId();
            if (! isset($this->rules['byResourceId'][$resourceId])) {
                if (! $create) {
                    return $nullRef;
                }
                $this->rules['byResourceId'][$resourceId] = [];
                //asdf
            }
            $visitor =& $this->rules['byResourceId'][$resourceId];
        } while (false);

And this fixed the problem but it broke many other unit tests.

The first break comes here: https://github.com/zendframework/zend-permissions-acl/blob/master/test/AclTest.php#L880

        $this->assertFalse($this->acl->isAllowed(null, 'area'));

This fails possibly because the ACL does not add rules when a new resource is added.

This is my assessment; pick one:

  1. Something is very broken in my fix
  2. The ACL is broken because it does not evaluate every resource permission when a user has a null permission
  3. Perhaps the ACL needs to build rules whenever a new resource is added?
TomHAnderson commented 6 years ago

31, when merged with master, passes all unit tests correctly but I think this is because not all roles are being evaluated, just the first one to return false (or true).

TomHAnderson commented 6 years ago

This fix is invalid and does not follow the acl as it exists now.