JosephSilber / bouncer

Laravel Eloquent roles and abilities.
MIT License
3.43k stars 330 forks source link

Columns 'restricted_to_id' and 'restricted_to_type', 'options' - can they be safely deleted? #613

Closed ievgen-klymenko-uvoteam closed 1 year ago

ievgen-klymenko-uvoteam commented 1 year ago

Hello! Thank you for the best Laravel permission package!

I have a question, there are 2 additional columns on assigned_roles table: restricted_to_id and restricted_to_type.

        Schema::create(Models::table('assigned_roles'), function (Blueprint $table) {
...
            $table->bigInteger('restricted_to_id')->unsigned()->nullable();
            $table->string('restricted_to_type')->nullable();
            $table->integer('scope')->nullable()->index();
...
        });

They seem to be always null, and I can't find any usage of them in the code.

I tried to delete these columns and it doesn't look like anything changed. Are these two leftovers from the past or reserved for the future features? Can they be safely deleted?

edit: Same thing about abilities table, json column options - there is get/set attribute for it, but looks like nothing else is using it inside the package. I guess this one was meant for userland from the beginning. I deleted it and looks like nothing broke, too


As a bonus request: Would be very nice if scope columns and indexes could be deleted too (through config, maybe). This isn't currently possible for how they are integrated into the code; (and I fully realize how hard this might be to implement); But I'm quite sure there are many projects that will never use scopes for permissions.

Thank you once again!

JosephSilber commented 1 year ago

About restricted_to_id and restricted_to_type

This was added in preparation for being able to grant a role only for a given modal or record (see number one in my post on releasing Bouncer):

// Note: this is not implemented yet
Bouncer::allow('editor')->to(['view', 'create', 'update'])->everything();

Bouncer::assign('editor')->to($user)->on(Podcast::class);

$user->can('create', Podcast::class); // true
$user->can('create', Post::class); // false

Its implementation ran into complications, so for the time being Bouncer does not support this. But the DB columns are there to support it in the future without requiring a breaking change.

About scope

There's no real way to completely remove it. If you're concerned about the space, and know that you'll never need it, you could probably change it to a Boolean.

bezanis commented 7 months ago

Hi @JosephSilber Thanks for all the work done on this package, really helpful for our project. Just wondering if any progress has been made on the restricted_to_id and restricted_to_type feature. Would be great to grant roles to users on specific models or records. It looks like the alternative is to set abilities, but I imagine the abilities table would quickly get large and messy.

ievgen-klymenko-uvoteam commented 7 months ago

Hello! That would be very cool indeed!

For now we ended up with abilities table like this:

id     name       title                                    entity_id      entity_type
20  view         View field category #10                10          field_category
21  view         View field #1                       1           field  
22  view         View field #2                       2           field  

Like @bezanis sais, it gets little bloated with time, because this entities ('field_category', 'field') of ours are user-filled... But only admin-user-filled, fortunately :smirk: So in our case we will only have 150-200 of them. But other projects might be not that lucky.. :grin:

JosephSilber commented 7 months ago

Just wondering if any progress has been made on the restricted_to_id and restricted_to_type feature.

Nope. Didn't get the time yet. It turns out to be much trickier than I had initially anticipated, so will require quite a bit of my time.

It looks like the alternative is to set abilities

I wouldn't say so. I'd say the alternative is to create model-specific roles:

Bouncer::allow('podcast-editor')->to(['view', 'create', 'update'], Podcast::class);

Bouncer::assign('podcast-editor')->to($user);

$user->can('create', Podcast::class); // true
$user->can('create', Post::class); // false
bezanis commented 7 months ago

On our current systems, we are using a simplified users/user_role(user_id,role_id,entity_id)/roles(key,label,has_entity,entity_type_id) structure for roles, which works because roles are only on entities(company, dealer, door) and not on abstract models (posts, users, etc)

For our project in development, we like the flexibility that having abilities affords us. We would also like to be able to assign a single role to a user restricted to a single door/dealer/whatever, or assign a role to a user for all doors.

In the code you provided above, Bouncer::assign('podcast-editor')->to($user); would assign the user as a podcast-editor to all Podcasts. In our situation, we would like to assign the user as a podcast-editor on a subset of Podcasts. It looks like assigned_roles.restricted_to_id/assigned_roles.restricted_to_type would allow that functionality.

Do you have an outline of your plans / progress you would like to share or is it ok if I take a look at adding the functionality?

Thanks again for this excellent package.

JosephSilber commented 7 months ago

In our situation, we would like to assign the user as a podcast-editor on a subset of Podcasts.

At that point, how is it different than just straight-up abilities?

bezanis commented 6 months ago

@JosephSilber I've started to add the ability to assign a role to a user but restrict it to a set of entities (in this test I've set restrictions for the user to access other users, but in my real word use, it will be on entities like doors, dealers, companies, etc) https://github.com/bezanis/bouncer/blob/feature/role-restricted-to-entity/tests/QueryScopes/RoleScopesTest.php Going to look into caching next