check preventAccessingMissingAttributes user_id default owned #620
Conversation
|
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? |
|
Sorry for my English. According to the documentation When checking permission, the owner check is always run (method isOwnedVia), whether or not the column exists in the model. But according to the article mentioned, it can be risky since the attribute is null, it may be because:
|
|
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 👍 |
| if ($isDefaultAttribute && !array_key_exists($attribute, $model->getAttributes())) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Why do we need to add $isDefaultAttribute to this check, and pass it all the way down? It makes the whole thing messy. Couldn't we just always do this array_key_exists check?
Oh I see now. You do want it to throw when it's not there 🤔
But do you really? Imagine this piece of code:
Bouncer::allow($user)->toOwnEverything();Would you want it to throw for all models that don't have a user_id column. I'd think not.
| { | ||
| if ($attribute instanceof Closure) { | ||
| return $attribute($model, $authority); | ||
| } | ||
|
|
||
| if ($isDefaultAttribute && !array_key_exists($attribute, $model->getAttributes())) { |
There was a problem hiding this comment.
I wish there were a better solution to this. $model->getAttributes() is quite expensive.
Maybe we could instead toggle the preventAccessingMissingAttributes back and forth before and after checking 🤔
There was a problem hiding this comment.
That would also be nice.
Anyway, it's something I thought. I think if you want to implement it in some way that you think is best (I mean the entire PR), I'll just close this PR. If this PR served to show this behavior, I consider myself satisfied.
There was a problem hiding this comment.
The only thing I'm not sure about is the global nature of it. The setting in the Eloquent\Model class sets a property on static::, which I guess means you can override it per model type. Not sure how switching it back and forth could work 🤔
| if ($isDefaultAttribute && !array_key_exists($attribute, $model->getAttributes())) { | ||
| return false; | ||
| } | ||
|
|
||
| return $authority->getKey() == $model->{$attribute}; |
There was a problem hiding this comment.
Maybe its possible to just change this to return $authority->getKey() == $model->getAttributeValue($attribute);?
This gets around the missing attribute exception. Atleast im looking at laravel 10 and this "fixes" the issue.
Im only trying to get rid of the barrage of Missing attribute logs im getting, so no idea if this impacts any other functionality as im not using ownedVia functionality.
|
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 |
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 exampleuser_id). In the vast majority of models that attribute (column) does not exist and theisOwnedViafunction returns false, but activating the check option, when trying to accessuser_idan 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).