Use existing config

[niekdemelker]

Hi there,

I am playing with your nova package, Really like the way permissions are code defined. like it should.

first thing I found was that you use some custom config to point to the user model, this is already defined in the original config, also used by Laravel and Nova for finding the user object.

The other idea i got is for the roles table: add a boolean field admin of full_control or something, with this boolean you can define a super user who always passes the policies and gates, no need to asign every permissions to the administrator group, example is as follow:

Gate::before(function ($user, $ability) {
    if ($user->isSuperAdmin()) {
        return true;
    }
});

Documentation: https://laravel.com/docs/5.7/authorization#intercepting-gate-checks

I will check later if i can find more improvements/fixes/whatever

keep up the good work, love the approach

[m2de]

Hi @niekdemelker . Thanks for the kind words and PR. I wasn't aware of the user model setting, much better than a custom config, thanks for that. I will definitely merge this for 2.0 (as it is technically a breaking change).

Also like the idea of a super admin setting. I'll have a play to see if I can somehow hide the permissions fields in nova when the super-admin option is checked, that would be pretty neat. There certainly is a valid use-case for an option like that to save having to assign new permissions to the main admin.

Just thinking about it though, it would mean the nobodyHasAccess method would allow anyone access, even if there is a super admin. But I guess I could include a check there to see if there is a super admin and deny access.

[niekdemelker]

Nice, glad you like it.

I guess the nobodyHasAccess method is something you wouldn't want in a production environment, if a user is removed and he was the last man with this rights everybody gets permitted to pass the gate, this is something you shouldn't want.

Maybe this entire check can be removed in favour of an artisan command, there can be two: one to create the admin group, and one to asign a user to the group (based on username/email or ID or something like that). I asume the true administrator has terminal access so he can accomplish this. the command makes sure the admin doesn't have to make changes directly in the database which is a good thing.

If you like i could spend some spare time to make something like this. Building the 2.0 ;).

If this works well we have some nice breaking changes which helps the developer creating the first (admin) group and asign a user to it by command, and never granting access unless explicitly given (removing the nobodyHasAccess method).

the hiding/showing in laravel nova seems like a nice to have, not a must have for these changes. Maybe i will get some more great ideas while making the suggested changes.

[m2de]

Thanks. Don't have anything against artisan commands, would make a nice addition.

I don't think there is any need to remove the nobodyHasAccess method, as people can use/not use it as they feel is suitable.

Appreciate you taking time to contribute.

[niekdemelker]

So, here i am again, As i started playing around in it i found out some things that don't seen right. like when opening the empty nova dashboard this package renders 8 query's to the database to check all permissions. i only can assume this will add up later when more policies get checked to render the menu.

also when you add an array of permissions to the database there is an query for every row added while this can be done in bulk,

I don't want you to feel attacked or something, I truly like the simpleness in this package, not to much of stuff you don't use anyway.

at the other hand, i really did go rampage on your code, so I don't dare to make a pull request of all i did to it haha. But please go check it out, i have made some comments in the code as well. maybe you like some parts of it and want to use it ;) (the commands for an example).

master...niekdemelker:playground

[m2de]

Hi @niekdemelker . Ha, no worries, no offence taken. That code was written some time ago and at the time my priority was solving my problem at the time, no so much the code quality. I will have a proper look through your suggestions (and get it running locally) when I have a bit of spare time but on first glance it looks great! You've clearly spend a bit of time on this and I really appreciate it.

From my point of view, as long as the main principle for separation of concerns in relation to roles and permissions is maintained, and it remains a top-level package so that users can basically just write their Gates and follow Laravel documentation, that is all good with me.