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.

Sign up for GitHub

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

check preventAccessingMissingAttributes user_id default owned #620

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

check preventAccessingMissingAttributes user_id default owned #620

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/Database/Models.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,16 +186,18 @@ public static function ownedVia($model, $attribute = null)
public static function isOwnedBy(Model $authority, Model $model)
{
$type = get_class($model);
$isDefaultAttribute = false;

if (isset(static::$ownership[$type])) {
$attribute = static::$ownership[$type];
} elseif (isset(static::$ownership['*'])) {
$attribute = static::$ownership['*'];
} else {
$attribute = strtolower(static::basename($authority)).'_id';
$isDefaultAttribute = true;
}

return static::isOwnedVia($attribute, $authority, $model);
return static::isOwnedVia($attribute, $authority, $model, $isDefaultAttribute);
}

/**
Expand All @@ -204,14 +206,19 @@ public static function isOwnedBy(Model $authority, Model $model)
* @param string|\Closure $attribute
* @param \Illuminate\Database\Eloquent\Model $authority
* @param \Illuminate\Database\Eloquent\Model $model
* @param bool $isDefaultAttribute
* @return bool
*/
protected static function isOwnedVia($attribute, Model $authority, Model $model)
protected static function isOwnedVia($attribute, Model $authority, Model $model, $isDefaultAttribute = false)
{
if ($attribute instanceof Closure) {
return $attribute($model, $authority);
}

if ($isDefaultAttribute && !array_key_exists($attribute, $model->getAttributes())) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

@JosephSilber JosephSilber Feb 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

return false;
}
Comment on lines +218 to +220
Copy link
Owner

@JosephSilber JosephSilber Feb 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return $authority->getKey() == $model->{$attribute};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

Expand Down