JosephSilber / bouncer

Laravel Eloquent roles and abilities.
MIT License
3.43k stars 330 forks source link

check preventAccessingMissingAttributes user_id default owned #620

Open gpibarra opened 1 year ago

gpibarra commented 1 year ago

According to this article, it is recommended to use these checks to avoid errors during the development of an application in Laravel. https://planetscale.com/blog/laravels-safety-mechanisms

When trying to add \Illuminate\Database\Eloquent\Model::preventAccessingMissingAttributes(); an exception is thrown when trying to check some permission of a specific model and it does not have an owned callback or column defined. When undefined it tries to look up the attribute ({model}_id, for example user_id). In the vast majority of models that attribute (column) does not exist and the isOwnedVia function returns false, but activating the check option, when trying to access user_id an exception is thrown because the attribute does not exist. This PR checks if the attribute exists only when the attribute has not been defined correctly (ie default).

JosephSilber commented 1 year ago

Can you explain this a little more? Why do you have owned abilities that are owned via a column that doesn't exist?

Are you saying you have the column on some models, and you're granting a blanket ability to all models, and you're relying that it'll only apply to the models that actually have that column?

gpibarra commented 1 year ago

Sorry for my English.

According to the documentation https://github.com/JosephSilber/bouncer#allowing-a-user-or-role-to-own-a-model and https://github.com/JosephSilber/bouncer#ownership

When checking permission, the owner check is always run (method isOwnedVia), whether or not the column exists in the model. I am doing a check on a model that does NOT have an owner, so the user is compared with the user_id column, which is null in the model because the column does not exist.

But according to the article mentioned, it can be risky since the attribute is null, it may be because:

JosephSilber commented 1 year ago

Oh I see. We're always calling it, regardless of whether you set it it up that way.

I now see the need for this change 👍

lrljoe commented 1 year ago

Maybe I'm being dense, but you could just create the mapping for isOwnedVia on those models where user_id is missing and have it do something else to determine ownership or just return true if you don't care about ownership of that model