$guarded value on Role/Permission class causes failure in latest laravel 7.x and 6.x #1542
Comments
By removing |
|
@drbyte sorry, my mistake, I actually did us fillable on my class that extends the role and permission class and it works as expected. |
|
While I much prefer using |
|
@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 :) |
|
[edit: sorry, wrong key combo on enter 🤦] @drbyte from what I can tell if I think though that if anyone has extended Role or Permission class extensively they would only be affected if they have added extra columns to the table. From what I could tell just extending the class and not adding It would maybe be a good idea then to note in the documentation that if a class is extended or extra columns added to the tables they should be accounted for in their model that is extending the packages class? I just did a search, although it probably isn't really representative of what the use of the package is but there are quite a few repos that extend the spate permission models, but most of them don't define any new columns or methods. One did add new fillable columns to the model this one (it also seems a recent project) Most though seem to be based on this template repo which extends the base Role/Permission classes and doesn't add anything: https://github.com/rappasoft/laravel-boilerplate/blob/master/app/Domains/Auth/Models/Role.php I think most people should be able to figure out what to add if there is something amiss? |
|
I also think though that for each time that a user does add columns the tables in the migration, and they haven't extended the classes, they may face issues. Unless there is a configurable value in the permissions config file, there is already a key Not sure on the performance effect it would have on instantiating the class each time, although the config helper is used in the Could be an alternative approach to not 'break' anything. |
|
Hmmm.... The last thing I want to do is break anything for everyone! I also don't want to introduce a lot of complexity. |
|
Yeah, breaking things are not really going to be any good, especially as it works as long as a class is extended. Could maybe let those who read the 'readme' know to make that change for now if they have the problem? Maybe this specific things isn't such a big problem at the current moment? I don't seem to see any other people having this issue so maybe from they are setting a If the change is made to use fillable instead of guarded, could it be set as a pre-release and see if other users are able to test it? But also transitioning towards the |
We're having it, too. We're not extending Permission ourselves, so this breaks it for us. Thanks for looking into it! |
|
I think this is the same issue we're seeing with the following code that works in Laravel 7.23.3 but not in 7.24.0 and later: We have this code in set setup method of a test class And from what I can tell, inside the actual query builder actions encapsulated by a create, somewhere the values being passed in the array here are dropped and result in a sql error: |
@jimbojsb from 7.24.0 I think would be correct as that is the version from when the change in the framework came in. The behavior you are seeing is the same I got. So it is the same bug |
|
@drbyte do you think it would be possible to make the needed changes that makes use of the |
|
Latest thoughts:
|
|
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." |
|
@BastianErler wrote:
Yes, and I agree at the "app" level. |
|
@ReeceM ... Maybe this would work on the models? (Unfortunately my initial testing is leaving It sets $fillable to this package's expected fieldnames, but then if the model is extended and $fillable is set in the extended model (which replaces this default) but forgets to keep the name/guard_name entries, then this override of getFillable will reinstate them. /src/Models/Role.php and Permission.php protected $fillable = ['name', 'guard_name'];
public function getFillable()
{
$this->mergeFillable(['name', 'guard_name']);
return $this->fillable;
} |
@drbyte, that is along the lines of what I was thinking could be done to account for the extended class. I have been testing again on my side and have found some other odd things related to the earlier comment you posted today, just wanting to finish checking before saying anything, there seems to be a bug when extending a class with guarded and fillable, but not on the base class?? But that idea looks like it would be easy to maintain in the future and basically self-explanatory |
|
Yes, and this hopefully avoids adding more stuff to the config file. |
|
LOL, I think I found the problem that would make it possible not to worry about adding a new function. But will just check, as it is interesting bug that is maybe a implementation detail on the framework function too 🤷♂️ |
|
re: #1542 (comment) by @drbyte From what I understand in looking at the code and running it when I first found the bug I found this:
It does appear that having Maybe it is the addition of the
What I have found in testing is that the one problem is that because the Role and Permission class don't use the normal So what happens is you get this: Internally of the model class you get this in the guarded columns thing
It is querying the schema before the table name from the config files are set
Because of this it basically gets nothing as the column names and kicks all data
And the problem is even worse, if the table is named something else, or if the class that extends is called something else.
$guardableColumns = array:2 [
"Spatie\Permission\Models\Role" => []
];What I noticed is that the 'simple' fix is to define a This way it makes use of the Laravel feature to get the table columns, but still guard the id. This means that if the user does extend the class nothing actually changes and they wouldn't notice even if they had an old version of laravel. This would make sense as then whenever eloquent was looking for the table it is a central method and is also matched? As it stands even in my App level logic, I have to get the table name from the config file to adjust for the Rule validation because the name isn't just roles/permissions. So the change can be two ways: /**
* Get the table associated with the model.
*
* @return string
*/
public function getTable()
{
return config('permission.table_names.roles');
// or
return config('permission.table_names.permissions');
}^ Setting it this way would mean that even if the class is extended it would work. Or we set the table before calling the public function __construct(array $attributes = [])
{
$attributes['guard_name'] = $attributes['guard_name'] ?? config('auth.defaults.guard');
$this->setTable(config('permission.table_names.roles'));
parent::__construct($attributes);
}But I tested, and you can keep the Please let me know if this came across clear :) |
|
Yes, I had a similar hunch when I was digging as well: ie: is there a chicken/egg situation where the lookup of table columns for guarded verification is firing too late? Declaring Haven't had a chance to test it myself though. Cases I want to test: base setup without extension; extended models; extended with added custom fields; declaring those custom fields as fillable (does it break anything); custom tablenames; changing config settings at runtime (eg: multitenancy dynamic update/set of config values such as db-connection, tablenames, etc). |
|
Of the things you want to test, I currently have the following going on my app that is using the package
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 🤣 |
Those are the best ones to know about! It's the wild and crazy uses that are hard to anticipate! |
|
@ReeceM So, to be sure that I'm reading your posts completely, are you saying that in your app merely declaring |
@drbyte yip, I added that snippet that’s a few comments up into the base Role |
|
Thanks. That helps me determine where to focus some of my tests when I can focus on it later today. |
|
Pleasure to do so 👍😁 |
|
Oh ... and remove the |
|
In my test I removed it in the __constructor |
|
Thinking aloud: Given the default |
|
(You can tell I'd rather be working on this instead of my project deadline! LOL) |
@drbyte I think keeping the fallback would be good, I was think originally when changing it locally to use parent::getTable() incase something in the framework changes, but I doubt that line of code has changed since early days :D |
|
Agreed. I'm thinking: return config('permission.table_names.roles', parent::getTable()); |
|
That looks good, that is very clear too which is cool :) |
|
👏🥳 Awesome @drbyte, thanks for keeping the package working :) |
|
Thanks @ReeceM for your research and testing. Greatly appreciated. |
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 laravel/framework#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:
This gives the error:
It seems the solution is to just set the
$guardedproperty 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
The text was updated successfully, but these errors were encountered: