cms
cms copied to clipboard
statamic
Allow roles and groups to be database driven
This PR follows on from the eloquent all models PR by allowing the user roles and groups to be stored in the database.
I appreciate that not every install would want this as they may be using Spatie permissions or some other roles library, but it feels like the option should be there at least for the roles and groups to be eloquent driven on a vanilla Statamic install.
If you'd be happy with the approach I've taken and open to merging it in principle then I'll start into writing the tests required to ensure coverage.
Yep, absolutely a good idea. However it should be an option, not a requirement.
That’s fair, should I put it behind a config flag? So if the roles/groups tables are falsey then it reverts to how it was before?
Yeah I think that'll work. Just gotta be aware of how it works if the keys are missing from the tables array. Existing users will not have them.
'tables' => [
'users' => 'users',
'role_user' => 'role_user',
'roles' => 'roles', // wont be there at all
'group_user' => 'group_user',
'groups' => 'groups', // wont be there at all
],
I've pushed a couple of updates that make it opt-in based on the config value. Do you mind checking them over to make sure this approach works for you?
I'll have a go at the tests tomorrow.
Ok I've added some tests here to cover adding groups/roles to users and removing them again. I had to make some changes to the existing eloquent user test, and the auth migration.
So you'll see I've added a new param to the auth migration (--test) which if present fixes the timestamp generated so it overwrites the files each time. Without that I was getting a class name already exists error as every migration generation was creating a new file.
Then I've made the eloquent user test (and groups/roles) run the migration as there was a todo mentioning that needed changed.
I also modified the down() migration to support sqlite.
~~Tests are working fine now for me locally, but failing here in GitHub as doctrine/dbal needs added to the test runner.~~
Can you take a look and see what you think of these changes?
edit: I added doctrine/dbal to require-dev and as you can see tests are now passing.
If we use this PR, will the roles.yaml file gets updated when we change the role permissions?
In my users.php I used eloquent as a repository. but even my roles.php gets updated while adding permissions to roles
You need to specify tables for your roles/groups here: https://github.com/statamic/cms/blob/22ab6996c860f31abd65b06f40a2850d4bb7c9e5/config/users.php#L109-L111
Remember to add doctrine/dbal to normal dependencies for it to work, otherwise migrate will blow up.
I guess it should be added to docs somewhere if/when this gets merged.
There’s no merge conflicts so it’s not needed.
On 14 Jul 2022, at 15:25, Frederik Sauer @.***> wrote:
@ryanmitchell can you merge 3.3 into this one as well?
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.
@ryanmitchell made a few performance changes, as I saw roles and groups being fetched 10 times each - ryanmitchell/cms#2
@ryanmitchell I missed the $table->id('id') => $table->id() cleanup in src/Console/Commands/stubs/auth/statamic_auth_tables.php.stub - but it's really just a visual thing.
@jasonvarga got any additional feedback on this?
I currently need a composer patch to get this functionality working, which is not quite optimal, so if you could move it along it would be very much appreciated.
Shouldn't it be possible to import users, groups and roles from files to database?
Any updates on this? Seems ready to merge or is there something left to do?
What is this waiting on?
Time to review. Will hopefully get to this soon.
FYI when I run the patch and do the auth migration the roles migration to modify that table stubbed as follows:
public function up()
{
Schema::create('1', function (Blueprint $table) {
$table->id();
$table->string('handle')->unique();
$table->string('title');
$table->json('permissions')->nullable();
$table->json('preferences')->nullable();
$table->timestamps();
});
}
/**
* Reverse the migrations.
*/
public function down()
{
Schema::dropIfExists('1');
}
```
Not sure why the table name is `1` but I changed that roles as I believe that's what it's suppose to be.
@icemancast im guessing you added ‘true’ to the config values rather than the name of the table.
@icemancast im guessing you added ‘true’ to the config values rather than the name of the table.
@ryanmitchell 🤦 son... saw false there and was like oh let me make this true because I do have a roles table. LOL
The recent fixes seemed to work, there are some small areas of things I notice in the control panel area but seem small/costmetic.
I have access to the control panel! :)
@ryanmitchell, do you think you would have time to rebase this on top of the 4.x branch? I'm trying to use the diff of this PR as a patch via Composer Patches, but it fails because this PR is 74 commits behind.
@faustbrian I've merged it in.
There are some test failures now but they are related to forms untouched by this PR so I think its a Github failure.
We found an issue in the User Groups list. It's trying to request the user group with the handle instead of the id. You should add
in Statamic\Auth\Eloquent\UserGroup.php a function id that will return the ID instead of the handle.
