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

Deny abilities #45

Closed
antoniopaisfernandes opened this issue Jan 28, 2016 · 15 comments
Closed

Deny abilities #45

antoniopaisfernandes opened this issue Jan 28, 2016 · 15 comments
Labels

Comments

@antoniopaisfernandes
Copy link

There should be an option to deny an ability to a user. That way, a user that belongs to a Role with X permissions, could have a specific permission(s) denied from that X without the need to fork the Role.

Best regards,
António Fernandes

@Arcesilas
Copy link
Contributor

In my opinion, if you give a role to a user and denies this user a specific ability, then, in reality, this user does not have the given role.
I could also say that the role is not correctly designed. You should then redesign it by splitting it in more than one role, or remove some abilities from the role and grant them individually to users who specifically need them (which should be the exception, not the rule).

@antoniopaisfernandes
Copy link
Author

I understand that it's a point of view.

Let's just see an example:

There is a Role called 'PostsManager' with 14 permissions assigned
10 users have the Role 'PostsManager'
8 of those shouldn't be able to 'change-the-timestamp'
2 of those 8 cannot also 'change-the-posts-logo'

I understand it could be tackled with 3 different roles but then when a new permission get's added to the 'PostsManager' role, it has to be added the the remaining 2 roles.

View this through the deny feature it's, in this example, as viewing an hierarchical role system.

@JosephSilber
Copy link
Owner

I understand the use case, but I'm not sure we need this in Bouncer.

I'm leaning towards agreeing with @Arcesilas.

I'll keep this open for now while I mull it over.

@Arcesilas
Copy link
Contributor

I understand it could be tackled with 3 different roles but then when a new permission get's added to the 'PostsManager' role, it has to be added the the remaining 2 roles.

Not necessarily. All 10 users have the common PostManager role, which gives them 12 common abilities. 2 other roles give the specific abilities 'change-the-timestamp' and 'change-the-post-logo'. If you want all your users to have a new ability, just add it to PostManager role.

Actually, I am completly and totally aware that what I'm saying is a little like what a French humorist said once: "Tell us what you need, we will tell you how to deal without it".

More seriously, it may become difficult to understand how roles and abilities are organized, since there are already 2 levels of abilities: abilities and roles. If you want to retract a specific ability to a user, what happens if one day you grant the user a role that should normally grant him the ability you denied him?

Today, user John Doe is PostManager, but he cannot edit the timestamp nor the post logo. And you have dozens of users, roles, abilities. Tomorrow, John Doe is in charge of updating all images to fit a new design (it's maybe not realistic, just for the example). It is then absolutely obvious he needs to be able to change the post logo. You grant him a new role, to allow him various abilities for his new mission, among which, abilitiy to edit the post logo. Which one prevails over the other ? You have to remove the ability denial, but when the mission ends, you remove the new role you added and have to deny him again the 'change-the-post-logo' ability.

Looks messy to me.

Just make roles for what is common to users: what cannot be shared needs a specific role.

@antoniopaisfernandes
Copy link
Author

More seriously, it may become difficult to understand how roles and abilities are organized, since there are already 2 levels of abilities: abilities and roles. If you want to retract a specific ability to a user, what happens if one day you grant the user a role that should normally grant him the ability you denied him?

The user get's denied. But that is OK. For prevention sake, the order should be: 'denies/omits', 'allows'. If the gate hits the firsts, the later should not be addressed.

And the main problem, in my point of view, in what relates to having a lot of Roles is managing them. Because we have a Role for each combination of permissions... That is what user permission is all about.

@Arcesilas
Copy link
Contributor

Or you could want the behaviour to be configurable, like in Apache, with the directive Order:

Order deny,allow

You want Deny to prevail over Allow, but tomorrow someone will ask for Allow to prevail.
I think adding such a functionnality doesn't suit KISS principle.

@antoniopaisfernandes
Copy link
Author

Order deny,allow

Nice idea!

@Gummibeer
Copy link

I don't see the real problem. If you have different role setups than use them like this.

In your PostsManager you have all roles that have every PostManager and than add PostUpdateTimestamp, PostDoOtherThings and assign all the roles like needed.

You can assign multiple roles to a single User. And why should it be easier to deny an ability on a User compared with multiple roles?

In my opinion there is rly no reason to add deny functionality. Everything can be solved with no more code and the same results.

@renanBritz
Copy link

Hey guys!

I'm building a user groups (roles) and permissions (abilities) system using bouncer.

I just stumbled onto this "issue" where I can't disallow an ability from a user in a specific role.

Using an example from my project (an MMO control panel): Let's say you hired a new game master, and you assign him the "Game Master" role. But for the first month, until you can really trust him, you'd probably disable permissions like browse accounts, browse character inventories, etc.

Creating a new role, like a "Newbie Game Master" is a good idea in our minds (programmers). The problem is: for the end user of our application / product, it probably makes more sence to retract an ability with a simple mouse click than to create a completely new role and assign it to the user.

If you could reconsider this proposal it would help me a lot.

Thank you for your attention and for this awesome laravel package!

Best Regards!

@JosephSilber
Copy link
Owner

@renanBritz I am considering this, especially now that we have wildcard abilities.

That said, it probably won't happen until after we launch 1.0.

@renanBritz
Copy link

@JosephSilber Great!

Looking forward to 1.0 then.

Best regards!

@devonzara
Copy link

This could also be useful if one wanted to add a 'Banned' role that they could apply without removing the user's existing roles. That way you don't have to add any extra logic to your code to check another table or anything to determine if the user is banned, but also if you ever decide to unban the user (or perhaps it was a suspension), then you wouldn't have to worry about reassigning all of the proper roles as they would still exist.

As far as which allow/deny takes precedence over the other, I would add a level to the roles to form a sort of hierarchy... and then assign user-level allows/denies an arbitrary value (perhaps customizable in the configs) so that you can create a role that may or may not override them.

I know there is some distaste towards config files, and you could always make it something else to go in the AppServiceProvider... Though personally, I'd much rather see an organized config file with all of the customizable options in one place than having to refer to the documentation to find a method to call and cluttering the AppServiceProvider. This would be rather cumbersome if all packages began doing it and we had to start creating a bunch of separate methods or service providers at the App level to keep things organized.

@JosephSilber
Copy link
Owner

JosephSilber commented Nov 29, 2016

Just implemented this in 865227b

Here's how to use it:

$post = Post::where('title', 'Very important post')->first();

Bouncer::allow($user)->to('delete', Post::class);

Bouncer::forbid($user)->to('delete', $post);

Bouncer::allows('delete', $post); // false

Here's another example:

$bouncer->allow('superadmin')->everything();

$bouncer->allow('admin')->everything();
$bouncer->forbid('admin')->toManage(User::class);

Now the superadmin and admin roles can do everything, but the admin role cannot manage (create/update/delete) users.


Finally for @devonzara's example:

$bouncer->forbid('banned')->everything();

Now when you want to ban a user, simply assign them the banned role. When you're ready to unban them, remove that role from them, and they'll get back all their previous abilities.

@boynet
Copy link

boynet commented May 21, 2017

@JosephSilber Hi does it still true? in the readme it's clearly say:

Note: if the user has a role that allows them to ban-users they will still have that ability. To disallow it, either remove the ability from the role or retract the role from the user.

nothing about forbid()

@JosephSilber
Copy link
Owner

@boynet all of the above is still true. forbid hasn't been documented yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants