accesscontrol icon indicating copy to clipboard operation
accesscontrol copied to clipboard

Explicit .grant() and .deny() should override any inherited permissions

Open scandinave opened this issue 6 years ago • 5 comments

Currently permissions are computed with a Union when there is inheritance. Tell me if i'm wrong. :) .

It will be awesome if in V3, we could have a switch, to choose the algorithm that make the computation. For example, I will really appreciate to have a algorithm that make the children permissions override the parents permissions.

Something like this :

  • Parent1 (grant, video, createOwn )

  • Parent2 (grant, post, createAny/Own)

  • Chid1 extends Parent1, Parent2 ( deny, post, createAny)

Child permissions == ( video:createOwn, post:createOwn )

scandinave avatar Feb 27 '18 13:02 scandinave

I'll keep this a bit detailed, as an implementation note.

It will be awesome if in V3, we could have a switch, to choose the algorithm that make the computation.

Changing the algorithm (as an option or not) is not the solution here. Besides, that will make the user code prone to mistakes and hard to maintain.

Currently permissions are computed with a Union when there is inheritance. Tell me if i'm wrong. :) .

Yes. When a role is extended with another or more roles; corresponding resource attributes are union'ed optimistically (meaning more privileges will win over less). So in its simplest form; when roleA has permitted attributes ['*'] (all) and roleB has only ['title'] but extends roleA; the resulting permitted attributes for that resource will be union'ed as ['*'].

ac.grant('user')
     .create('video', ['title'])
     ... // other grants
  .grant('admin')
    .extend('user')
    .create('video', ['*'])

const permission = ac.can('admin').create('video');
console.log(permission.granted); // true
console.log(permission.attributes); // ['title'] UNION ['*'] => ['*']

I will really appreciate to have a algorithm that make the children permissions override the parents permissions.

I partially agree with this part.

For instance, .deny() is just a convenience method that essentially sets the resource attributes to []. i.e. ac.deny(role).create('resource') is equivalent to ac.grant(role).create('resource', []).

Now, explicit .grant() or .deny() already override themselves;

ac.grant('user').create('video')
  .deny('user').create('video');

const permission = ac.can('user').create('video');
console.log(permission.granted); // false
console.log(permission.attributes); // []

... but (currently) this is not the case for inherited grants; which are union'ed:

ac.grant('user').create('video')
  .grant('admin').extend('user')
  .deny('admin').create('video');

const permission = ac.can('user').create('video');
console.log(permission.granted); // true
console.log(permission.attributes); // ['*']

This is intended and not a bug but; logically, I agree that explicit .grant() or .deny() should also override any inherited permissions.

This may break some user code and should be implemented in a major version.

onury avatar Feb 27 '18 18:02 onury

Ok. I'm fine with your solution. I will waiting for V3 :)

scandinave avatar Feb 27 '18 18:02 scandinave

Hi, just wanted to say that this feature would be huge! We will also have the "problem" that down the chain will have a disabled feature / attribute for a role but all others can still have access to it.

Best!

Logic-Bits avatar Jan 04 '19 15:01 Logic-Bits

Any updates on this issue?

This is a much needed fix IMO - it renders "extend" almost useless in the 2.x behavior.

anodynos avatar Feb 22 '19 12:02 anodynos

Rather than thinking about a parent/child relationship, it's advisable to think in terms of security. In OOP, inheritance means extending the capabilities of the parent, not restricting them. You can tell from the lib examples that an admin extends a user.

The example listed in this issue is the other way around (restrictive inheritance?).

user extends admin + user can't have x,y,z powers

Many security experts favor whitelisting as a pattern for acl, which is what this library aims to provide. The main issue with blacklisting is that you gotta think about all the possible scenarios where the root grant is permissible and there is room for mistakes. This doesn't mean the library shouldn't allow blacklisting. But this is rather a call to avoid blacklisting as much as possible.

To further expand on the example provided here, a safer pattern is:

Role1 ( grant, video, createOwn )
Role2 ( grant, post, createOwn )
Child extends Role1, Role2 == ( video:createOwn, post:createOwn )
Parent1 extends Role1 == (video, createOwn )
Parent2 extends Role2 + (grant, post, createAny) == (grant, post, createAny/Own)

simoami avatar Feb 11 '20 16:02 simoami