JosephSilber / bouncer

Laravel Eloquent roles and abilities.
MIT License
3.45k stars 332 forks source link

How to check ownership? #569

Closed Synchro closed 3 years ago

Synchro commented 3 years ago

The Bouncer docs describe how a model can be owned by another model, and how to assign that ownership, via Bouncer::allow('user')->toOwn(Thing::class);, and then says that this ownership should be "checked at the gate", however, I can't see how to do this, nor can I find any mention of it in the Laravel docs. My policies currently have things like:

    if ($user->isAn('admin')) {
        return true;
    }

    return $user->can('see any thing');

but I don't know how to check ownership in this situation. I tried $thing->isOwnedBy($user); and $user->owns($thing), but neither worked. How should I check ownership?

JosephSilber commented 3 years ago

then says that this ownership should be "checked at the gate"

That seems like the docs are a little misleading, since you don't have to check anything yourself. Can you please point me to where in the docs you saw that?

I don't know how to check ownership in this situation

You don't have to check it yourself. Bouncer will automatically check it for you.

Using your example, if you've set ownership with:

Bouncer::allow($user)->toOwn(Thing::class);

...then any check at the gate, such as:

Gate::can('view', $thing);

...will automatically check if $thing->user_id == $user->id.

There's nothing else you need to do.

JosephSilber commented 3 years ago

Side question: why do you have a policy in the first place?

Synchro commented 3 years ago

It was this bit I was confused by. After your explanation it makes more sense though. A related thing I'm not clear on is the to list:

Bouncer::allow($user)->toOwn(Post::class)->to(['view', 'update']);

these look like method names rather than bouncer abilities, which I call things like can view post. I'm not sure when I should use which name.

Synchro commented 3 years ago

I'm using a policy because it's useful to have things like the before method to provide admin access, provide default access for apiResources, and generally to keep all the authorization in one place. Are you saying I should prefer embedding these checks in controller methods?

JosephSilber commented 3 years ago

these look like method names rather than bouncer abilities, which I call things like can view post. I'm not sure when I should use which name.

You are of course free to name your abilities however you like. The standard convention is not to call them can view post, but just the verb itself: view.

You might find this article useful here: https://josephsilber.com/posts/2016/08/03/authorization-improvements-in-laravel-5-3

JosephSilber commented 3 years ago

Are you saying I should prefer embedding these checks in controller methods?

That's not what I'm saying.

I'm saying you should prefer adding these with Bouncer:

Bouncer::allow('admin')->everything();
Synchro commented 3 years ago

Hm. I have a bad feeling I've got a lot of things wrong... I used that naming scheme because at the point of defining an ability, there is no model:

    Bouncer::ability()->firstOrCreate(['name' => 'can view post']);

so I don't see how you would differentiate between view on a post model vs the same action on some other model.

Do I gather that if I've called Bouncer::allow('admin')->everything(); then I don't need to bother with the is('admin') check in before? I was under the impression that the "allow" definition defined the authorization, but that you still had to check it in a policy or controller action, not realising that Bouncer would do this "magically", in which case I think I have a lot of duplicate checking going on, which may have conflicts :(

Thanks very much for your help.

JosephSilber commented 3 years ago

I used that naming scheme because at the point of defining an ability, there is no model

Did you read the linked article?

Bouncer follows Laravel's conventions, so the correct way to do that is to use the class name:

Bouncer::ability()->firstOrCreate([
    'name' => 'view',
    'entity_id' => '*',
    'entity_type' => Post::class,
]);

Do I gather that if I've called Bouncer::allow('admin')->everything(); then I don't need to bother with the is('admin') check in before?

Correct.

I was under the impression that the "allow" definition defined the authorization, but that you still had to check it in a policy or controller action

You no longer need a policy, but you would definitely still need to check it from either a can middleware or a controller action.

Again, the linked article explains Laravel's authorization in detail :+1: