Skip to content
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

ACL inherit order should not matter #13870

Closed
tempcke opened this issue Feb 28, 2019 · 3 comments

Comments

@tempcke
Copy link

commented Feb 28, 2019

As I was about to create this issue I found this related issue: #10294

I wrote tests to reproduce my situation and then another test to reproduce the situation in issue #10294

Using PHP 7.1.26 with Phalcon 3.4.2 the first test passes and the rest fail

class PhalconAclTest extends \PHPUnit\Framework\TestCase {

  public function testInheritedAllow() { // Test passes
    $acl = new \Phalcon\Acl\Adapter\Memory;
    $acl->setDefaultAction(\Phalcon\Acl::DENY);
    $acl->addResource('widget','read');
    $acl->addRole('A');
    $acl->addRole('B');
    $acl->addRole('C');
    $acl->addInherit('B', 'C');
    $acl->addInherit('A', 'B');
    $acl->allow('C', 'widget', 'read');
    $this->assertTrue($acl->isAllowed('A', 'widget', 'read'));
  }
  
  public function testInheritedAllow_reversedOrder() {
    $acl = new \Phalcon\Acl\Adapter\Memory;
    $acl->setDefaultAction(\Phalcon\Acl::DENY);
    $acl->addResource('widget','read');
    $acl->addRole('A');
    $acl->addRole('B');
    $acl->addRole('C');
    $acl->addInherit('A', 'B');
    $acl->addInherit('B', 'C');
    $acl->allow('C', 'widget', 'read');
    $this->assertTrue($acl->isAllowed('A', 'widget', 'read'));
  }

  public function testOrderOfInheritShouldNotMatter() {
    $acl = new \Phalcon\Acl\Adapter\Memory;
    $acl->setDefaultAction(\Phalcon\Acl::DENY);
    $acl->addRole('A');
    $acl->addRole('B');
    $acl->addRole('C');
    $acl->addResource('widget',['read','write','delete']);
    $acl->allow('A', 'widget', 'read');
    $acl->allow('B', 'widget', 'write');
    $acl->allow('C', 'widget', 'delete');
    $acl->addInherit('B','A');
    $acl->addInherit('C','B');
    $acl->addInherit('A','C');
    $this->assertTrue($acl->isAllowed('A','widget','read'));
    $this->assertTrue($acl->isAllowed('A','widget','write'));
    $this->assertTrue($acl->isAllowed('A','widget','delete'));
    $this->assertTrue($acl->isAllowed('B','widget','read'));
    $this->assertTrue($acl->isAllowed('B','widget','write'));
    $this->assertTrue($acl->isAllowed('B','widget','delete'));
    $this->assertTrue($acl->isAllowed('C','widget','read'));
    $this->assertTrue($acl->isAllowed('C','widget','write'));
    $this->assertTrue($acl->isAllowed('C','widget','delete'));
  }

  public function testGithubIssue10294() {
    $acl = new \Phalcon\Acl\Adapter\Memory();
    $acl->setDefaultAction(\Phalcon\Acl::DENY);

    $acl->addRole(new \Phalcon\Acl\Role('Jane'));
    $acl->addRole(new \Phalcon\Acl\Role('Admin'));
    $acl->addRole(new \Phalcon\Acl\Role('User'));

    $acl->addResource('Contact', ['ping', 'info', 'getAll']);

    $acl->allow('Admin', 'Contact', 'ping');
    $acl->allow('User', 'Contact', 'getAll');
    $acl->allow('Jane', 'Contact', 'info');

    $acl->addInherit('Jane', 'Admin');
    $acl->addInherit('Admin', 'User');

    $this->assertTrue($acl->isAllowed('Jane','Contact', 'ping'));
    $this->assertTrue($acl->isAllowed('Jane','Contact', 'getAll'));
    $this->assertTrue($acl->isAllowed('Jane','Contact', 'info'));
  }
}

@niden niden added the Bug - Low label Mar 11, 2019

@niden niden added this to To do in 4.0 Release via automation Mar 11, 2019

@niden

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

@tempcke I spent a couple of days trying to figure this one out. I do have a couple of fixes that need to be introduced to the Acl and will do that shortly. All supplied tests pass on my testbed app apart from the last one.

The use case on the last test will not be fixed because we have an infinite loop there - which is what the test reports back.

niden added a commit to niden/cphalcon that referenced this issue Jun 16, 2019

@niden niden referenced this issue Jun 16, 2019

Merged

T13870 acl inherit order #14184

4 of 5 tasks complete

@niden niden moved this from To do to In progress in 4.0 Release Jun 16, 2019

niden added a commit to niden/cphalcon that referenced this issue Jun 16, 2019

niden added a commit that referenced this issue Jun 16, 2019

niden added a commit that referenced this issue Jun 16, 2019

@niden

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

Closing this - #14184 and also circular inheritance is not allowed

@niden niden closed this Jun 16, 2019

4.0 Release automation moved this from In progress to Done Jun 16, 2019

SidRoberts added a commit to SidRoberts/cphalcon that referenced this issue Jun 19, 2019

SidRoberts added a commit to SidRoberts/cphalcon that referenced this issue Jun 19, 2019

@niden niden added the 4.0 label Jun 21, 2019

@tempcke

This comment has been minimized.

Copy link
Author

commented Jul 12, 2019

All supplied tests pass on my testbed app apart from the last one.

The use case on the last test will not be fixed because we have an infinite loop there - which is what the test reports back.

Thanks @niden

the last test is testGithubIssue10294. I do not have the setup to re-execute it right now but reading it I do not see why it would cause an infinite loop, it is really a just a subset of testOrderOfInheritShouldNotMatter except as there was another issue related to this I used the resource and role names they used in their example.

shrugs as long as testOrderOfInheritShouldNotMatter passes I'm not sure what more we can ask for though ... I just don't understand if testGithubIssue10294 results in an infinite loop how testOrderOfInheritShouldNotMatter passes lol.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2 participants
You can’t perform that action at this time.