Laravel-permission: Next major release -- Features/Requests for v3

[AlexVanderbist]

It feels like the last couple of months a lot of issues were related to the multi-guard functionality introduced in v2. Apart from that there have been some vague caching issues as well. Time to do something about that in v3 of the package.

The multi-guard issue

Supporting multiple guards has added a lot of complexity to the package and made it feel quite "heavy". The multi-guard functionality is definitely a selling point for this package but it also seems to confuse some users that are new to the framework and haven't learned about guards yet. Not to mention the countless times a misconfiguration in auth.php causes the wrong guard to be used.

Some possible solutions:

• completely dropping support for guards - as @drbyte and I discussed at Laracon removing all support for multiple guards would make the package a lot easier to maintain, simpler and lighter.
• my personal favourite: drop guard support but keep Permissions and Roles polymorphic relations. This means any model can have roles and permissions but there's no extra functionality related to guards.
• maintaining two branches: one with- and one without support for guards.

Caching issue

There's no interface or CLI tool that comes with the package to create and manage roles/permissions. This means a lot of users revert to editing the DB to manage permissions causing the cache to be rendered invalid. Even though it's well documented this seems to be an issue that keeps coming back. Maybe caching should be limited to a single request lifecycle by default and configurable for anyone that needs more anything else?

---

Any thoughts on these two issues or suggestions for other features for v3 of the package are very welcome!

[freekmurze]

👍👍👍 for simplifying this package.

maintaining two branches: one with- and one without support for guards.
It'll be too much work, so let's don't do that.

completely dropping support for guards

The nuclear option, but given that not so much users need multi-guard support, this is certainly an option.

This means any model can have roles and permissions

How would that look like in the db? I'm guessing not many people are going to need this, so if we go this route it has to be really really simple.

Maybe caching should be limited to a single request lifecycle by default and configurable for anyone that needs more anything else

Good idea, let's do that!

Let's also drop support for anything below Laravel 5.5

[shirshak55]

@freekmurze Thanks. Its very hard to develop the app for mobile device using this package and that is due to name spacing issue. I guess we can drop guards so that it will work on both. But we cannot ignore the fact that using guards have some use case. I guess we can decide using guard or not according to null value on table etc..

And I do agree dropping support below laravel 5.5 is good as Laravel 5.5 is LTS

[barryvanveen]

Although I cannot easily estimate the effort of maintaining support for multiple guards, I would consider it a missed opportunity if support is dropped.

The Laravel authorization system includes the ability to use multiple guards. Many new applications now include an "api" guard (or something similar). Personally, I think the usage of multiple guards is not going to lessen. it might only increase in the future.

Is there a way to maintain support and still make life easier for users and maintainers of this package?

For me the problem with using multiple guards only arose during database seeding and testing. Those are the moments that I work in a "web-guarded environment" and want to assign "api-guarded" roles and permissions. Maybe the docs could be clearer about these situations.

If you try to do this

$user = new User();
$user->name = 'API user';
$user->save();
$user->assignRole('MyApiRole');

and get a RoleDoesNotExist exception, then try this:

$role = Role::findByName('MyApiRole', 'api');

$user = new User();
$user->name = 'API user';
$user->save();
$user->assignRole($role);

Same thing goes for the PermissionDoesNotExist exception.

[shirshak55]

Exactly I also face problem . I wanted to make mobile apps where admin can control many thing. But due to this namespacing issue I also fell in this trap.

I guess there should be easy way without duplication of such roles and permissions.

[AlexVanderbist]

Thanks for the feedback!

So the way support for guards works right now is mainly through polymorphic relations. A permission/role can simply belong to any other model. On top of that there's some logic dealing with guards specifically.

My idea is to keep the polymorphic relationships on the Permission and Role models but drop all logic related to guards. Any model can simply have it's own set of permissions/roles and that's it.

In the PermissionRegistrar all permissions will still be registered as gates so @can('permission name', 'guard') and user()->can('permission name') will still work.

For creating Permissions and Roles I'd propose using User::createPermission('name') instead of Permission::create('name', 'guard'). This way we no longer need to specify a guard or model the permission belongs to.

When using multiple guards users will need to have a different model for each guard to properly use the package. This can be easily accomplished by extending for example User to an ApiUser and WebUser.
This way WebUser::givePermissionTo('do something') will work just like before, without having to assume what guard is being used.

Hopefully this makes sense. This feels like the easiest way to support multiple guards but also keep the package simple enough for anyone that doesn't use guards. Any remarks/suggestions welcome 🙂

[drbyte]

These are great discussion points!

Some additional features I've seen requested semi-repeatedly are:

• heirarchical/cascading permissions (see https://github.com/spatie/laravel-permission/issues/382#issuecomment-332627484 and #440 and #512)
• inheritance (a variation on the above, and part of https://github.com/ultraware/roles )

I'd like to see us resolve https://github.com/spatie/laravel-permission/issues/480 either before v3, or as part of v3.
And, related, would be preferable to allow devs to not have to manually trigger the PermissionRegistrar after creating permissions during unit tests.

EDIT: #480 has been resolved via #505

[AlexVanderbist]

I agree on #480, if I've got some free time I'll start with that one and tag it before starting on v3.

For the testing issue I think it would suffice to define a new gate when the Permission::created event is fired. (Maybe we can even extend Laravel's Gate and check for permissions there?) I'll test that and add it in v3 as well.

[benyanke]

Would love some sort of wildcard implementation for granting permissions to groups, ala #382

[shirshak55]

yea those additional feature can take this package to another level :)

[hakimihamdan88]

Support giving permissions by id number, #450

[shirshak55]

Is therw any advantage of using id? Although laravel provide such things in
ecosystem

On Oct 30, 2017 2:53 PM, "hakimihamdan88" [e-mail] wrote:

Support giving permissions by id number, #450
https://github.com/spatie/laravel-permission/issues/450

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/spatie/laravel-permission/issues/479#issuecomment-340383818,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHuOYZ8reSWeza0BqPfnwk6DsDp3YTlgks5sxZIBgaJpZM4Pnc7B
.

[benyanke]

Depending on the context, using the ID allows you to skip a needless DB query. I'll put it this way - if this library is inserting the ID into the database in the end, and you already have the ID in your app from another task, why wouldn't you want to juse use the ID?

[pedzed]

_(I wanted to create a new issue for this feature request, but it seems that it is more welcome to be thrown into this issue)_

Feature: wildcard permissions

Current way:

$superadmin->givePermissionTo('create clients');
$superadmin->givePermissionTo('read clients');
$superadmin->givePermissionTo('update clients');
$superadmin->givePermissionTo('delete clients');

$superadmin->givePermissionTo('create campaigns');
$superadmin->givePermissionTo('read campaigns');
$superadmin->givePermissionTo('update campaigns');
$superadmin->givePermissionTo('delete campaigns');

$developer->givePermissionTo('read clients');

$developer->givePermissionTo('create campaigns');
$developer->givePermissionTo('read campaigns');
$developer->givePermissionTo('update campaigns');
$developer->givePermissionTo('delete campaigns');

New wildcard way:

$superadmin->givePermissionTo('*');

$developer->givePermissionTo('read clients');
$developer->givePermissionTo('* campaigns');

Wildcards should be able to be used alone, before, after or in the middle of a permission.

$role->givePermissionTo('*'); // alone
$role->givePermissionTo('* users'); // before
$role->givePermissionTo('read *'); // after
$role->givePermissionTo('create * users'); // middle

Perhaps for consistency, wildcard roles should be added too, but I cannot quickly think of a real world example of why one would want it.

[benyanke]

Request:

Create Permissions -> No Error if Not Exists (Make idempotent)

Currently, a permission creation call fails if the permission already exists. I would propose an option to allow this to be an idempotent call, which would silently continue instead of throw an error if it already exists. This allows much easier use in provisioning scripts.

Currently, the syntax is:

Permission::create(['name' => 'permission.namespace.name.here']);

I would propose an optional option, such as:

Permission::create(['name' => 'permission.namespace.name.here' , 'no_error_if_exists' => true]);

Such an option would allow the permission create function to be called multiple times, with no error.

If the maintainers are open to this, I might even be able to work on the patch myself, as this will be a major plus to my application. In my case, this functionality would allow me to refresh permissions from an Active Directory periodically, based on some rules.

[pedzed]

~@benabbottnz~ @benyanke I stumbled upon it upon seeding the database as well. In that case, it should apply to roles and other tables as well.

My opinion about your example however, is that it is bad practice to use another argument to implement this. I would create another method, which is better for those nifty IDE features like autocomplete and error checking. Something like this, but perhaps with a better name:

Permission::createIdempotently(['name' => 'permission.namespace.name.here');

Or perhaps switch the way how it works and use your feature request as default, but leave room for those who want to catch errors on such occasions.

Permission::createOrFail(['name' => 'permission.namespace.name.here');
// Eventually throws PermissionAlreadyExistsException

[benyanke]

I like the createOrFail method, as it more clearly explains the function, but if the maintainers prefer to keep the compatibility, something like createIdempotently would well work too.

[hrabbit]

createOrFail works although sounds like an exception should be fired (IMO).
createOrSkip sounds closer to what I would want to be achieved (of course, this is all semantics).

[pedzed]

createOrFail works although sounds like an exception should be fired (IMO).

Thank you for confirming what I already wrote :)

(Read the comment in the example code, two comment posts above)

[hrabbit]

@pedzed

Thank you for confirming what I already wrote :)

Sorry about that, was just purely stating an observation from my own view.

At present, to get around this behaviour, I use the following (which also matches your Permission::createIdempotently method, but leverages existing Eloquent methods).

$permission = Permission::firstOrCreate(['name' => 'my_permission', 'guard_name' => 'my_guard]);

I realise this is a feature/future request thread but I can't help but feel that there is a little re-invention around this area. (Of course, I could have missed the point somewhere in here).

[IlCallo]

My enhancement, restrict roles and permissions given to a certain model to only work with a second given model #292, was rejected some time ago because not needed and API breaking, but I saw someone else had my same use case (#576).
Can it be inserted in v3?
It works by adding a nullableMorph field on roles and permissions pivot tables, all methods signature have been changed to take an array instead of a ...$args because the second parameter would be an instance of the model to which the role/permission has been restricted to.
If no restrictable method is specified, the package behaves like it does now

[benyanke]

Another request: Give an option to delete all roles and permissions.

I built a frontend which dynamically creates and assigns permissions to users - it would be very nice to be able to flush all the permissions so I can rebuild from a clean slate.

[Epistol]

A request : making an artisan command for joining role & permission, as we do to create a role or a permission.

[drbyte]

@Epistol wrote:

A request : making an artisan command for joining role & permission, as we do to create a role or a permission.

Can you please explain your intended use of this? Are you planning to write batch files full of artisan commands? Why do you want it on the CLI instead of being part of your app?

[Epistol]

For testing purpose, being able to add and remove permission join without having to write the entire system for it (I suppose it could be something people would need)

[Neve12ende12]

I would love to see a "key / tenant" column added to roles and permissions so that you can store multiple roles with the same name & guard in the same table, but that kind of dream is probably just pie in the sky.

Example:

    Guard  Key             Permission
    Web     Project X    Create User
    Web     Project Y    Create User

[benyanke]

Doesn't sound that hard to do, but what would be a real life use case of that feature?

[benyanke]

Also, another proposal:

An option to load a user's full permissionset into cache once per request.

Currently, I'm doing some advanced use of this, for displaying elements on pages and such, and this can sometimes result in things like 10-20 queries (one to check each permission as the view is loaded).

If there was a call which could load all of a user's permission, or even just an array of permissions you know you'll need later, and store the boolean (user has permission / user does not have permission) in ram during the request, this could massively speed up some of my requests.

This could, of course, be built on top of spatie/blink.

[pedzed]

I would love to see a "key / tenant" column

I am strongly against this one, because this package is laravel-permissions. It is completely unrelated to tenants.

Don't get me wrong, I use tenants as well for some of my projects, but my point is that the package should focus on what the package is for.

If you want an extra column, just write your own class and inherit from Role and Permission.

[greggilbert]

I was just read #660, and it seems like a deeper version of that is something that could be built into the next version. For example, perhaps adding another layer of polymorphism on model_has_permissions to have target_type and target_type, and then be able to assign permissions to a specific target for a specific user (or whatever).

[sixlive]

Looking at this today, I'm personally for

drop guard support but keep Permissions and Roles polymorphic relations. This means any model can have roles and permissions but there's no extra functionality related to guards.

I think this would simplify things a lot.

[drbyte]

@benyanke wrote:

Another request: Give an option to delete all roles and permissions.
I built a frontend which dynamically creates and assigns permissions to users - it would be very nice to be able to flush all the permissions so I can rebuild from a clean slate.

To clarify: did you mean a command to purge all roles/permissions for a specified model/user? Or a complete reset of everything (ALL roles/permissions/cache, a total purge)?

[benyanke]

@drbyte

Both, ideally. I know I can't be the only one who needs to programmatically create permissions. And if you are programmatically creating them based on a ruleset, it's likely an implementation like that would also want to start with a clean slate.

[drbyte]

@barryvanveen ... you wrote:

Although I cannot easily estimate the effort of maintaining support for multiple guards, I would consider it a missed opportunity if support is dropped.
The Laravel authorization system includes the ability to use multiple guards. Many new applications now include an "api" guard (or something similar). Personally, I think the usage of multiple guards is not going to lessen. it might only increase in the future.
Is there a way to maintain support and still make life easier for users and maintainers of this package?

Alex and I will be talking more about multiple guard support over the next few weeks.
Would you (and others) mind listing here some scenarios where an app's business rules require very different permissions for each guard?

I find a lot of apps can happily use identical permissions for both 'web' and 'api' guards, especially when using model policies to manage guard-specific filtering. So a list of cases we should consider would be helpful so we're not shortsighted as we consider the future path here! It will also help justify retaining the feature if we decide to do so.

[barryvanveen]

@drbyte, thanks for remembering that remark and for asking for my input. It is much appreciated :)

My opinion on this has changed since my previous comment. As you mention, checking permissions is done through model policies. So, even if different types of users (API-users vs web-users) require a different set of permissions, these will still be checked using the same policies.

The reason I like multiple guards is that you can (try to) completely split both types of users and their associated permissions. This works for the user models (i.e. \AppUser vs \AppApiUser) and for the roles and permissions, but as you note these permissions will probably still apply to the same domain models, and hence be using the same policies.

So, in effect the multiple guard system has only allowed me to split permissions and roles. In our applications roles can be configured using a CMS-like page. API-permissions can be assigned to API-roles and web-permissions to web-roles. API-users can be given API-roles, web-users can be given web-roles. They can't be mixed. This was easier because all necessary information and methods were already available in the package.

I'd call that a minor advantage if the cost of maintaining such functionality is high. Also, it can be recreated by extending the Role- and Permissions-models.

But, and this has not changed, we do use the guard-functionality and would have to find a workaround if you would decide to remove it :)

Hope this helps your discussion. If there is anything I can clarify, please don't hesitate to ask!

[drbyte]

Thanks @barryvanveen - your feedback is appreciated!

Your last comment leads me to another question:
If this package became guard-agnostic, how would you have to re-architect those apps that are using that feature now?

By guard-agnostic I mean: middleware/gate/policies would respect guards as part of the normal Laravel execution chain, but this package wouldn't make decisions or cause any influence one way or another in relation to which guard is active during execution.

(I'm asking for 2 reasons: first, to consider/understand the larger impact and possibly strategize ways around the biggest complexities, and second to ascertain how to explain to "upgraders" what they would need to consider/change in their app.)

[shirshak55]

@drbyte Hi,

was there any reason for namespacing roles and permissions for API and WEB guards? I think it only creates duplication like I need to have same permissions for API and WEB. And think today most people use SPA for coding so namespacing will be useless .

[drbyte]

@shirshak55 That's kinda my point.
I don't call it "namespacing" per sé, but the whole "multiple-guards" bit feels like bloat, and that's what I'm hoping to see removed.

But in doing so I'm trying to understand the ways in which people are currently using that "namespaced" or "multiple guard" feature, in order to assist with an ideal migration path to a new approach, or at least be able to clearly define what people will lose if they upgrade, etc.

[barryvanveen]

@drbyte Users from previous versions already have the correct database information. This means that permissions and roles are linked to a specific guard. What you would lose after upgrading is a way to select a permission or role by its name/id _and its guard_.

To fix that users would have to extend the Permission and Role classes and for example add a local scope to each. Something like this:

public function scopeGuard(Builder $query, string $guard_name)
{
    return $query->where('guard_name', $guard_name);
}

In my application the guard-system was used to separate (or namespace if you will) API-users from web-users. Since an API-user can never have web-roles or web-permissions, there is no need for checking guard_names during each request. That is why I wouldn't miss that functionality and no further upgrade actions would be required.

It might be interesting to know if other users have assigned data of multiple guards to a single user, because that would lead to a different problem.

[bjuppa]

This is a very interesting discussion, thanks for taking your time to investigate the question of multiple guards.

Myself, I've only used multiple guards to separate admin users for the backend from web users. In addition to the standard web guard I've had an "admin" guard that also uses an eloquent user provider, but with an admin user model.

[drbyte]

Awhile back in this thread, @benyanke @pedzed @hrabbit mentioned wishing for some variation of:

Create Permissions -> No Error if Not Exists (Make idempotent)
Permission::createIdempotently(['name' => 'permission.namespace.name.here');
Permission::createOrFail(['name' => 'permission.namespace.name.here');
createOrFail works although sounds like an exception should be fired (IMO).
createOrSkip sounds closer to what I would want to be achieved (of course, this is all semantics).

Of course if you've been using this package since then you've probably already seen this, but ...
v2.9.2 added findOrCreate which basically accomplishes the same.

Just posting this to close the loop on what's been requested vs completed.

[drbyte]

Closing this Issue, as it covers too many things at once, and we're starting work on v3 shortly.

If you've got comments for one of the questions I've posed in the last couple weeks, feel free to reply here.
New thoughts should be posted as new issues.