onury / accesscontrol

Role and Attribute based Access Control for Node.js
https://onury.io/accesscontrol
MIT License
2.21k stars 178 forks source link

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

Open scandinave opened 6 years ago

scandinave commented 6 years ago

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 :

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

onury commented 6 years ago

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.

scandinave commented 6 years ago

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

Logic-Bits commented 5 years ago

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!

anodynos commented 5 years ago

Any updates on this issue?

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

simoami commented 4 years ago

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)