Complexity using the library for multiple "contexts" for the same application.

[illambo]

Hello everyone, first of all I would like to thank you and congratulate you for the package,
I wanted to try to describe for a moment the difficulties I am encountering (hoping to findsome useful ideas) using the library in multiple "contexts" for the same application.

I try to represent in a concise way that I think I express myself better:

Case 1
Connection “1” on db1 -> User has roles and permissions teams disable (users / roles / permissions all in db1)
Connection “2” on db2 -> Customer has roles and permissions teams enable (customers / roles / permissions all in db2)

UserRole extend SpatieRole on connection “1”
UserPermission extend SpatiePermission on Connection “1”

CustomerRole extend SpatieRole on connection “2”
CustomerPermission extend SpatiePermission on connection “2”

Case 2 (multitenancy)
Landlord -> Connection “landlord” on db1 -> User has roles and permissions teams disable (users / roles / permissions all in db1)
Tenant n -> Connection “tenant” on db-n -> Profile has roles and permissions teams enable (profiles / roles / permissions all in db-n)

Landlord/Role extend SpatieRole on connection “landlord”
Landlord/Permission extend SpatiePermission with Connection “landlord”

Tenant/Role extend SpatieRole on connection “tenant”
Tenant/Permission extend SpatiePermission on connection “tenant”

I have found that in both cases it is not enough to overwrite the traits or class methods of roles and permissions, but rather it is necessary to "work" on the configuration (and PermissionRegistrar::class) at runtime, modifying the behaviors from time to time and restoring them.

Basically, for example, you cannot call in succession (because it generates exceptions):
Case 1 : $user->roles; […] $customer->roles;
Case 2 : $user->roles; […] $profile->roles;

Basically it fails to verify the permissions correctly in succession (see test).

Eg with spatie/laravel-multitenancy the solution that I found "functional" is the following:

class SwitchPermissionTask implements SwitchTenantTask
{
    protected string $originalCacheKey;
    protected string $originalPermissionClass;
    protected string $originalRoleClass;
    protected bool $originalTeams;

    public function __construct(
        protected PermissionRegistrar $permissionRegistrar
    ) {
        $this->originalCacheKey = $this->permissionRegistrar->cacheKey;
        $this->originalPermissionClass = $this->permissionRegistrar->getPermissionClass();
        $this->originalRoleClass = $this->permissionRegistrar->getRoleClass();
        $this->originalTeams = $this->permissionRegistrar->teams;
    }

    public function makeCurrent(\Spatie\Multitenancy\Models\Tenant $tenant): void
    {
        $this->permissionRegistrar->teams = true;
        $this->permissionRegistrar->setRoleClass(\App\Models\Tenant\Role::class);
        $this->permissionRegistrar->setPermissionClass(\App\Models\Tenant\Permission::class);
        //$this->permissionRegistrar->cacheKey = $this->originalCacheKey.'.tenant.'.$tenant->getKey();
        $this->permissionRegistrar->forgetCachedPermissions();
    }

    public function forgetCurrent(): void
    {
        $this->permissionRegistrar->teams = $this->originalTeams;
        $this->permissionRegistrar->setRoleClass($this->originalRoleClass);
        $this->permissionRegistrar->setPermissionClass($this->originalPermissionClass);
        //$this->permissionRegistrar->cacheKey = $this->originalCacheKey;
        $this->permissionRegistrar->forgetCachedPermissions();
    }
}

Note for example:

laravel-permission/src/Traits/HasRoles.php

Line 51 in b6b9f4f

config('permission.models.role'),

it relies on reading the configuration rather than on the getRoleClass trait method (same for permissions)

laravel-permission/src/Traits/HasRoles.php

Line 36 in b6b9f4f

public function getRoleClass(): string

There are also numerous other points of similar behavior.

I understand the importance and opportunity of isolating contexts but I have the feeling that this "forcing" makes the management of some use cases complex / expensive / limiting.

I forked a small example in order to test, debug and understand some problems I'm encountering,

I hope I have explained myself, I probably don't have enough experience and if so I apologize if the assessments and considerations are incorrect, I ask you for advice if there are alternative solutions.

Thank you

[drbyte]

it relies on reading the configuration rather than on the getRoleClass trait method (same for permissions)

I agree: we could probably safely switch that to use what the Registrar has resolved.

[illambo]

Hi, I forked a small example in order to test, debug and understand some problems I'm encountering, when you have time, if you feel like it, could you take a look? Thank you.

[illambo]

Update: I updated the test battery a bit and fixed some points where the values were retrieved from the configuration rather than class/trait method. Now it seems to me that the problem remains with cache management which does not provide a "namespace" solution per connection.
For now I find it very complicated to get my hands on it, perhaps you have more confidence on the matter.

[illambo]

Maybe the most interesting approach would be the ability to resolve different PermissionRegistrar based on the "context", so instead of having a singleton have the ability to configure additional PermissionRegistrar with in input your own configuration and resolve them based on the context (of this way there would be no problem related caching, team and more as they would be isolated).

E.g. something like:

$permissionRegistrarApp1 = new \Spatie\Permission\PermissionRegistrar([/*...configApp1...*/]);

$this->app->when([App1\Role::class, App1\Permission::class, App1\User::class])
  ->needs(\Spatie\Permission\PermissionRegistrar::class)
  ->give(fn () => $permissionRegistrarApp1);


$permissionRegistrarApp2 = new \Spatie\Permission\PermissionRegistrar([/*...configApp2...*/]);

$this->app->when([App2\Role::class, App2\Permission::class, App2\Customer::class])
  ->needs(\Spatie\Permission\PermissionRegistrar::class)
  ->give(fn () => $permissionRegistrarApp2);

However, it seems very complex to me to act in both imagined directions (or at least, for me), I think I'll close the discussion if I don't find any ideas or help on the matter.

[drbyte]

Yes, I follow your thinking in terms of resolving different instances of the Registrar based on context.
As you're saying, as a singleton, having it namespace itself by context is a bit of a complex undertaking.

The caching part that uses the Cache infrastructure (as opposed to data memoized in properties) does let you "namespace" things by changing the cache-key. Your earlier code post had that commented out. Were you finding that it wasn't necessary/useful?

[illambo]

Your earlier code post had that commented out. Were you finding that it wasn't necessary/useful?

Was commented as it was extrapolated from a small poc project with multitenancy management via spatie package, this package already includes one of the tasks to adapt the cache and I consequently assumed that it would also adapt the permission cache for the same driver (but I haven't checked carefully if this is actually the case).

[drbyte]

You wrote:

you cannot call in succession (because it generates exceptions):
Case 1 : $user->roles; […] $customer->roles;
Case 2 : $user->roles; […] $profile->roles;

What were the exceptions? Was it just "permission not found for guard"? (which would signify wrong model or wrong connection)

[illambo]

Unfortunately I can't replicate the role problem (I will correct the discussion of what is described as soon as I can), I don't want to have extended the classes badly (or it was referring to the different configuration of the team functionality per "context").
I would remain more focused as an investigation on the failing test (permissions related) and on the things of a possible attempt to clean up the interaction between configuration and PermissionRegistar and overriding class methods.
The thought remains that if we were able to instantiate the PermissionRegistrar once per "context" and if we cleaned up the data recovery from configuration issue, we could operate on multiple contexts without switching the configuration at runtime.
I'll try to work on it a little to see if I can do anything, in the meantime, thank you.

[drbyte]

Interestingly, in your new tests, if you replace the lookup on permission by "name", with the model instance of the created permission, the test passes.

For example:

    public function it_can_manage_roles_and_permissions_on_multiple_schemas_2()
    {
        /*
        ray()->clearAll()->showApp();
        DB::listen(function($query) {
            ray($query)->orange();
        });
        */

        $roleApp1Name = 'testRoleApp1InWebGuard';
        $roleApp2Name = 'testRoleApp2InWebGuard';
        $permissionApp1Name = 'testPermissionApp1InWebGuard';
        $permissionApp2Name = 'testPermissionApp2InWebGuard';

        $this->assertFalse($this->testUser->hasRole($roleApp1Name));
        $this->assertFalse($this->testCustomer->hasRole($roleApp2Name));

        $roleApp1 = App1\Role::findOrCreate($roleApp1Name, 'web');
        $roleApp2 = App2\Role::findOrCreate($roleApp2Name, 'web');

        App1\Permission::findOrCreate($permissionApp1Name, 'web');
-        App2\Permission::findOrCreate($permissionApp2Name, 'web');
+        $p2 = App2\Permission::findOrCreate($permissionApp2Name, 'web');

        $roleApp1->givePermissionTo([$permissionApp1Name]);
        $roleApp2->givePermissionTo([$permissionApp2Name]);

        /*
        ray([
           'roles in app1' => App1\Role::all()->pluck('name')->toArray(),
           'permissions in app1' => App1\Permission::all()->pluck('name')->toArray(),
           'roles in app2' => App2\Role::all()->pluck('name')->toArray(),
           'permissions in app2' => App2\Permission::all()->pluck('name')->toArray(),
           'permissions in roleApp1' => $roleApp1->permissions->pluck('name')->toArray(),
           'permissions in roleApp2' => $roleApp2->permissions->pluck('name')->toArray(),
        ]);
        */

        $this->assertTrue($roleApp1->hasPermissionTo($permissionApp1Name));
-        $this->assertTrue($roleApp2->hasPermissionTo($permissionApp2Name)); // NOTE: -> Tests failed, problem related to PermissionRegistrar method loadPermissions, the feeling is that the entire cache must reason per connection (something like namespace)
+        $this->assertTrue($roleApp2->hasPermissionTo($p2));

        $this->assertFalse($this->testUser->hasRole($roleApp1Name));
        $this->assertFalse($this->testCustomer->hasRole($roleApp2Name));

        $this->testUser->assignRole($roleApp1Name);
        $this->assertTrue($this->testUser->hasRole($roleApp1Name));

        $this->testCustomer->assignRole($roleApp2Name);
        $this->assertTrue($this->testCustomer->hasRole($roleApp2Name));

        $this->testUser->unsetRelation('roles');
        $this->testCustomer->unsetRelation('roles');

        $this->assertTrue($this->testUser->hasRole($roleApp1Name));
        $this->assertTrue($this->testCustomer->hasRole($roleApp2Name));

        $this->assertTrue($this->testUser->hasPermissionTo($permissionApp1Name));
        $this->assertTrue($this->testUser->can($permissionApp1Name));
        $this->assertFalse($this->testUser->checkPermissionTo($permissionApp2Name));
-        $this->assertTrue($this->testCustomer->hasPermissionTo($permissionApp2Name));
+        $this->assertTrue($this->testCustomer->hasPermissionTo($p2));
        $this->assertFalse($this->testCustomer->checkPermissionTo($permissionApp1Name));
    }

[illambo]

Yes, because bypass cache (PermissionRegistrar -> loadPermissions).

[illambo]

Hi, I'm working on something in this branch, I'd like to understand what you think and whether you think it's a portable approach later in the package (maybe major release) or whether it's too complicated.
I would then like to move all the configuration stuff as properties in the PermissionRegistrar so as to remove the array and make retrieving/modifying the values more immediate.
I don't understand the binding contract for roles and permissions (see also here and here), I think it should be removed as I find it inconsistent if you have more than one schema (and it is also redundant compared to the methods present in the PermissionRegistrar).
Thanks

[illambo]

I'm going to close the conversation (and the ongoing implementation on the fork) as too much maintenance (major changes) is needed to clean up the situation. I leave the fork as a reference in case it could be useful as a starting point, I hope to have made the focus and motivation of the discussion clear and reasonable. Thank you.

[illambo]

Ehi @drbyte I push #2623 as a possible starting point and solution.