Change of behaviour after version 0.0.20

[szydan]

Hi

On version 0.0.20 these tests passed

   describe('one role, different rule order', function () {

      it('one role, allow, deny', function(done) {
        this.acl.addResource('type:dashboard', 'type:*')
        this.acl.addResource('dashboard:A', 'type:dashboard')
        this.acl.addRole('user');

        this.acl.allow('user', 'type:dashboard', 'view');
        this.acl.deny('user', 'dashboard:A', 'view');

        this.acl.query('user', 'dashboard:A', 'view', function(err, allowed) {
          allowed.should.equal(false);
          done();
        });
      });

      it('one role, deny, allow', function(done) {
        this.acl.addResource('type:dashboard', 'type:*')
        this.acl.addResource('dashboard:A', 'type:dashboard')
        this.acl.addRole('user');

        this.acl.deny('user', 'dashboard:A', 'view');
        this.acl.allow('user', 'type:dashboard', 'view');

        this.acl.query('user', 'dashboard:A', 'view', function(err, allowed) {
          allowed.should.equal(false);
          done();
        });
      });

    });

After upgrading to 0.0.22 the second test will FAIL The resource for which I ask for permission to view dashboard:A it is a child of type:dashboard so as far as I can understand if I allow view access for a parent but deny on a child the access for a child should be denied regardless of the order of the rules

[djvirgen]

Hi @szydan , thanks for providing these tests. I don't believe this is a bug, because VirgenACL checks the rules in last-in, first-out order. This means that changing the order of the rules may, by design, have an impact on the result. This also applies to resources that inherit from other resources. If the first rule that Virgen ACL checks doesn't match the resource but matches the resource's parent, then that rule will determine the result. By checking the rules in LIFO order, this allows you to "override" previous rules by applying a new rule last.

I hope this helps. Let me know if you have any questions or think this should behave differently.

[szydan]

@djvirgen thanks for the clarification, I'll investigate if I can solve my use case by ordering the rules in the correct order. It might be worth to mention the LIFO order in the REAME.md, I've seen it in the code and in some issue comment but it is not stated in the readme so some people (like myself ;_)) may get confused. I'll come back here after doing some more tests.

[szydan]

@djvirgen I have a question. Was the LIFO order always there? from the start? If yes then the second test should also fail in version 0.0.20

it('one role, deny, allow', function(done) {
        this.acl.addResource('type:dashboard', 'type:*')
        this.acl.addResource('dashboard:A', 'type:dashboard')
        this.acl.addRole('user');

        this.acl.deny('user', 'dashboard:A', 'view');
        this.acl.allow('user', 'type:dashboard', 'view');

        this.acl.query('user', 'dashboard:A', 'view', function(err, allowed) {
          allowed.should.equal(false);
          done();
        });
});

as the last rule is allow What worries me is that the same test is passing in 0.0.20 and failing in > 0.0.20

[djvirgen]

Good point about the README, I'll update it to make it clearer what the expectation is. The LIFO behavior was expected from the very start, but it may have been a bug in previous releases. I'll add some unit tests to verify the expected behavior -- I don't think your particular use case was tested.

[djvirgen]

I've updated the README and added two new unit tests to cover the behavior you mentioned. Thanks for the feedback!

https://github.com/djvirgen/virgen-acl/commit/4a0b7a0e4caf091e528b6d36742820a466e4b60e