laravel-permission: $guarded value on Role/Permission class causes failure in latest laravel 7.x and 6.x

With regards the latest laravel update that is documented here: Security Release: Laravel 6.18.35, 7.24.0

And also the code change happened here in the 6.x branch https://github.com/laravel/framework/pull/33777

These two updates have broken the functionality when you try to perform a with() query on the Roles model.

For example I have the code:

Role::orderBy('type')
    ->withCount('users')
    ->paginate();

This gives the error:

PHP Notice:  Undefined index: guard_name in app/vendor/spatie/laravel-permission/src/Models/Role.php on line 62
TypeError: Argument 1 passed to getModelForGuard() must be of the type string, null given, called in app/vendor/spatie/laravel-permission/src/Models/Role.php on line 62

It seems the solution is to just set the $guarded property to [] in the model classes.

If someone is extending the class this is easy to fix, but unfortunately it isn’t very clear immediately what error caused this.

If it is alright with the maintainers I don’t mind making the change to the model files to the code that will make it functional.

System Info:

Laravel: 6.18.35 PHP: 7.3.8 Permissions: 3.13

About this issue

Original URL
State: closed
Created 4 years ago
Reactions: 1
Comments: 36 (17 by maintainers)

Most upvoted comments

@BastianErler wrote:

Taylor Otwell described the reason for this change in this Blogpost

https://blog.laravel.com/security-release-laravel-61835-7240

and yes in fact he recommends “As a personal recommendation, I recommend always using $fillable instead of $guarded.”

Yes, and I agree at the “app” level. But packages are complicated: we can set default $fillable properties, but then if you extend the model and set your own but forget to include the defaults in your own then it wipes them out and then you come asking for support about why it’s not working… It’s hard to find the right balance between making it easy for you to use and also “protecting you from yourself!”

drbyte on Aug 14, 2020

👏🥳

Awesome @drbyte, thanks for keeping the package working 😃

ReeceM on Aug 15, 2020

(You can tell I’d rather be working on this instead of my project deadline! LOL)

drbyte on Aug 14, 2020

In my test I removed it in the __constructor

ReeceM on Aug 14, 2020

Thanks. That helps me determine where to focus some of my tests when I can focus on it later today. I appreciate your interactive exploration here!

drbyte on Aug 14, 2020

So, to be sure that I’m reading your posts completely, are you saying that in your app merely declaring getTable in the base model is enough to meet the needs for accommodating this security patch?

@drbyte yip, I added that snippet that’s a few comments up into the base Role ~table~ model and it sorted it. Didn’t set id either if $guarded was left in the base model

ReeceM on Aug 14, 2020

Of the things you want to test, I currently have the following going on my app that is using the package

extended the base class,
added column names,
$fillable on the extended model,
dynamic connections for the DB (database switch in Service Providers).
I have changed the table names in the config file, but not the extended model as it was more practical to adjust it in the config.

But as you said, a batch of tests would be useful to go through with different implementations, for my case it is probably a rather ‘crazy’ end user modifications possibility covered 🤣

ReeceM on Aug 14, 2020

Yes, and this hopefully avoids adding more stuff to the config file.

drbyte on Aug 14, 2020

I don’t seem to see any other people having this issue

We’re having it, too. We’re not extending Permission ourselves, so this breaks it for us. Thanks for looking into it!

josephsong on Aug 11, 2020

@drbyte I am just going to test what those changes would do too, as I have extended the Role and Permission classes to add extra stuff and columns. That would give me a better position to give my thoughts if that is alright 😃

ReeceM on Aug 11, 2020