-
Notifications
You must be signed in to change notification settings - Fork 331
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
How should wildcards work? #56
Comments
1st option is giving more flexibility, but it's wouldn't be so obvious behaviour. Maybe there should be alias methods which should do the trick and will be more verbose? |
Well of course there will be alias methods. This discussion is about the core functionality.
The alias methods will probably only go in the Bouncer itself. I'm not sure I want to pollute the trait too much.
|
It think no 1 makes sense, it retains a easy way to give only simple abilities for any project that requires that. //in my mind
//this
Bouncer::allow($user)->to('*');
//translates to
Bouncer::allow($user)->to('*', null);
//i.e. allow these permissions which are not constrained to a particular model. |
So I guess we'll leave it the way I have it now, which is option 1. Here are the aliases I've been thinking of: Bouncer::allow($user)->to('*', '*');
Bouncer::allow($user)->all();
$user->allowAll();
Bouncer::allow($user)->to('*', Post::class);
Bouncer::allow($user)->allOn(Post::class);
$user->allowAllOn(Post::class);
Bouncer::allow($user)->to('create', '*');
Bouncer::allow($user)->onAllTo('create');
$user->allowOnAllTo(Post::class);
Bouncer::allow($user)->to('*');
Bouncer::allow($user)->allSimple();
$user->allowAllSimple(); I'm not really excited about all of these. I'm open to suggestions. |
In my opinion there is no usecase for this - it's something like But I will never use total wildcard abilities. |
Hmmm. I don't think I'd use it directly very often on a user, but perhaps on a role - such as 'admin'. Which would avoid the need for me to have a separate check for If we allow wildcard abilities, then I guess In terms of naming, I ran along the lines of |
Hi, is there option to revoke all permissions for user. |
@ognjenm have you tried? $user->abilities()->sync([]);
$user->roles()->sync([]); This should work, if it does you also can do: $user->abilities()->sync($abilityIDsArray);
$user->roles()->sync($roleIDsArray); But I don't know if it will conflict with the Bouncer Cache. |
Thanks @Gummibeer I can clear cache after this command. |
Hi @JosephSilber, Will there be a way to retract a role from all users in one go using wildcards? So something like: Bouncer::retract('admin'); \\retract admin role from all users instead of only having per user: Bouncer::retract('admin')->from($user); |
@taiyeoguns you can do it like this: Bouncer::retract('admin')->from(User::pluck('id')->all()); If you want it to be more efficient, then only select users with that role: Bouncer::retract('admin')->from(User::whereIs('admin')->pluck('id')->all()); I think this is not a common enough action to warrant a dedicated way to do it in the package. Maybe in the future we can add more helpers for such things, but for now this'll suffice. |
Thanks, I'll try this. |
Wildcards without exceptions messes up things a bit. I would have a role This would be the case to give everything to the role or to a user. $user->allow('*'); Then make this user super admin or take away those abilities. $user->allow('super-admin'); It will create 2 rules. Bouncer::denies('super-admin'); Can remove only this tag as a record in database if it was added. But can not remove this ability from Maybe this is the case with |
Second variant is more logical and it'll be better. There's no point of keeping two levels of ROOT access - it's ROOT and it would be stupid, if there was another ROOT with another permissions. |
I am building a system where we only allow one role per user. Abilities will be inherited by the role. If a user's role is changed, i.e. from admin to reviewer, I want to make sure that the user is only a reviewer, and no longer an admin, after the change. Something like
would be helpful, such that when the user is then updated with |
@OnceWas you can currently accomplish this very easily: $user->roles->each(function ($role) {
$user->retract($role);
}); Update: we now have a Bouncer::sync($user)->roles([$request->new_user_role]); |
Here's my current thinking with regards to the method aliasing: Bouncer::allow($user)->everything();
// Equivalent to
Bouncer::allow($user)->to('*', '*');
Bouncer::allow($user)->toAdministrate(Post::class);
// Equivalent to
Bouncer::allow($user)->to('*', Post::class);
Bouncer::allow($user)->toAlways('view');
// Equivalent to
Bouncer::allow($user)->to('view', '*');
// Only allowing simple abilities will not get an alias and will not even be documented:
Bouncer::allow($user)->to('*'); Thoughts? |
We are thinking about an alias for the wildcard, right? 😉 So, lets stick to it and focus only on wildcard. If I translate a If we take into consideration that people are lazy (like me) and like to write less code, read less documentation, remember fewer specific variations of possible namings, then for me there is only one alias left - Bouncer::allow($user)->toAll();
// Equivalent to
Bouncer::allow($user)->to('*', '*');
Bouncer::allow($user)->toAll(Post::class);
// Equivalent to
Bouncer::allow($user)->to('*', Post::class);
Bouncer::allow($user)->toAll('view');
// Equivalent to
Bouncer::allow($user)->to('view', '*'); To complicate things and give more power and freedom to users, then there is a possibility on going Laravel way. This is just a quick thought. Bouncer::allow($user)->toAll();
// Equivalent to
Bouncer::allow($user)->to('*', '*');
Bouncer::allow($user)->toAll(Post::class);
// Equivalent to
Bouncer::allow($user)->to('*', Post::class);
Bouncer::allow($user)->toView();
// or splitting into 3 keywords - to view all
// where view - name of the ability
// and all - wildcard
Bouncer::allow($user)->toViewAll();
// Equivalent to
Bouncer::allow($user)->to('view', '*');
Bouncer::allow($user)->toView(Post::class);
// Equivalent to
Bouncer::allow($user)->to('view', Post::class);
Bouncer::allow($user)->toAdminister(Post::class);
// Equivalent to
Bouncer::allow($user)->to('administer', Post::class); |
@coolynx thanks for your comments. Always appreciated. Let me first address the second half of your proposal, and state quite simply that I do not want magic methods. So let's put that to rest. With regards to the first half of your proposal I see two problems:
|
Hi, I like these: Bouncer::allow($user)->everything();
// Equivalent to
Bouncer::allow($user)->to('*', '*');
Bouncer::allow($user)->toAdminister(Post::class);
// Equivalent to
Bouncer::allow($user)->to('*', Post::class);
Bouncer::allow($user)->toAlways('view');
// Equivalent to
Bouncer::allow($user)->to('view', '*');
// Only allowing simple abilities will not get an alias and will not even be documented:
Bouncer::allow($user)->to('*'); Perhaps Bouncer::allow($user)->everything();
// would be more consistent as
Bouncer::allow($user)->toAdministerEverything(); Would you consider applying the same to roles? Part of me feels like being able to say the following would be nice, but a nightmare/impossible to implement alongside the simple abilities Bouncer::allow($user)->to('view')->everything();
// instead of
Bouncer::allow($user)->toAlways('view'); |
@Keoghan For the other one: I truly wish we could do something like this: Bouncer::allow($user)->to('view')->everything(); But I don't think it's possible... |
Hi, One possibility would be to have objects for abilities. But yeah, it's a little heavy.
Makes it possible. Of course, since method Not sure it's relevant. Don't know if anyone may like it, it's just a way to make it possible. It's not necessary to create hundreds of |
I don't think the complexity is worth it. |
For giggles I put together a rough piece of code that would allow a method to 'lookahead' to see if it's being chained. Then |
@Keoghan cool proof of concept! Not that I'd ever ship anything like that 😆 |
I'm wondering if wildcards as they are currently planned allow developper/admin to grant abilities on a user's own items.
But this means many database entries: one for each pair user/post. If we consider we have to do it for each post for each user, it may grow fast. Could something like:
be possible? If current user belongs to |
@Arcesilas The scope of a database-bound ACL system (or at least Bouncer's scope) is to record static permissions. Custom logic like that belongs in the application code. Bouncer allows you to define regular callbacks on the Gate::define('edit', function ($user, $post) {
if ( ! $post instanceof Post) {
return null;
}
return $user->id == $post->user_id;
}); If you want to allow all actions on a post, use a policy: class PostPolicy
{
public function before($user, $ability, $arguments)
{
return $arguments[0]->user_id == $user->id;
}
} |
That's right! Thanks for your answer, which reminds me that Policies are still useful! :) |
Just pushed up wildcard aliases with the following syntax: Bouncer::allow($user)->everything();
Bouncer::allow($user)->toManage($post);
Bouncer::allow($user)->toManage(Post::class);
Bouncer::allow($user)->toAlways('view'); |
@Arcesilas with the new alpha 3 release, you don't need that policy anymore. You can replace it with this simple call: Bouncer::allow($user)->toOwn(App\Post::class); If you only want to allow a single ability on owned models, you can pass that as a second argument: Bouncer::allow($user)->toOwn(App\Post::class, 'view'); You can also pass an array of abilities you want to allow on owned models: Bouncer::allow($user)->toOwn(App\Post::class, ['view', 'update']); |
Hey ! That is really great ! Thanks a lot for your work ! |
@Keoghan I actually got this to work: Bouncer::allow($user)->to('view')->everything(); |
@JosephSilber awesome stuff. You’ve put a whole lot of work into bouncer over the last year! |
I'm currently working on allowing wildcard abilities. Here's how it works:
You can also allow all actions on a model:
You can also allow a specific action on all models:
So far so good. What I'm not sure about is the following:
As you can see, a wildcard ability does not allow model abilities. To also allow model abilities you need two wildcards:
All of this is already implemented. Now onto the question:
Which one of these two makes more sense?
Option 1 is the way it works now.
Thoughts?
The text was updated successfully, but these errors were encountered: