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

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

Open
scandinave opened this issue Feb 27, 2018 · 5 comments
Labels
revision A change rather than a bug or feature. v3 Will be implemented/fixed in version 3.

Comments

@scandinave
Copy link

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 )

@onury
Copy link
Owner

onury commented Feb 27, 2018

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 onury added revision A change rather than a bug or feature. v3 Will be implemented/fixed in version 3. labels Feb 27, 2018
@onury onury changed the title Provide a possibility to change the way permissions are computed. Explicit .grant() and .deny() should override any inherited permissions Feb 27, 2018
@scandinave
Copy link
Author

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

@Logic-Bits
Copy link

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
Copy link

Any updates on this issue?

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

@simoami
Copy link

simoami commented Feb 11, 2020

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
revision A change rather than a bug or feature. v3 Will be implemented/fixed in version 3.
Projects
None yet
Development

No branches or pull requests

5 participants