Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

It does not detect the model guard if you use it from the constructor #1923

Closed
mcolominas opened this issue Nov 10, 2021 · 14 comments
Closed

Comments

@mcolominas
Copy link

I was doing several tests, and I have discovered if you use multiple guards and try to tell it to be a specific guard from the constructor it doesn't work, but it does work if it is set as an attribute.

This does not work:

class YourCustomPermission extends SpatiePermission
{
    public function __construct(array $attributes = [])
    {
        $this->guard_name = config('xxx.guard');
        parent::__construct($attributes);
    }
}

This works:

class YourCustomPermission extends SpatiePermission
{
    public $guard_name = 'custom_guard';

    public function __construct(array $attributes = [])
    {
        parent::__construct($attributes);
    }
}

Reviewing the project code, I have seen that in this line comes the problem, it only obtains the attributes of the class, but not those that are initialized in the constructor:

$guardName = (new \ReflectionClass($class))->getDefaultProperties()['guard_name'] ?? null;

Therefore, if you have the default web guard, and another that is admin, everything you create will be created with the web guard, and when you try to assign it to someone from the admin guard, it will tell you that the permission or role does not exist

@erikn69
Copy link
Contributor

erikn69 commented Nov 10, 2021

parent::__construct($attributes); replace $this->guard_name = config('xxx.guard'); but using public $guard_name = 'custom_guard'; makes it a model propertie, not an attribute from db

public function __construct(array $attributes = [])
{
$attributes['guard_name'] = $attributes['guard_name'] ?? config('auth.defaults.guard');
parent::__construct($attributes);
}

For create a new permission YourCustomPermission::create(['name'=>'name','guard_name'=>'guard_name']); and now guard_name is setted

Maybe related #1793

could you create a minimal Laravel app that demonstrates the issue? Or maybe create a failing tests for us to look at? example

@mcolominas
Copy link
Author

The user class is a standard one, making use of the traits

use Spatie\Permission\Traits\HasPermissions;
use Spatie\Permission\Traits\HasRoles;

The guards that I have are web and admin.
In the path I use as middleware auth:admin
And I use the classes mentioned above to create the admin roles and permissions

@erikn69
Copy link
Contributor

erikn69 commented Nov 10, 2021

For create a new permission or role set guard_name YourCustomPermission::create(['name'=>'name','guard_name'=>'guard_name']);

@mcolominas
Copy link
Author

mcolominas commented Nov 10, 2021

Yes, this would work, but I had it on, it is that the guard admin is put if or if without passing it by parameter.

In other words, when using that model, the default guard is admin and not web.
But if the model provided by this package is used, the default guard is web.

@erikn69
Copy link
Contributor

erikn69 commented Nov 10, 2021

It's no a bug, it's a expected behaivor, i don't understand why you wanna do that, but

class YourCustomPermission extends SpatiePermission
{
    public function __construct(array $attributes = [])
    {
         $attributes['guard_name'] = config('xxx.guard');
         parent::__construct($attributes);         
    }
    public static function create(array $attributes = [])
    {
        $attributes['guard_name'] = $attributes['guard_name'] ?? config('xxx.guard');;

        $permission = static::getPermission(['name' => $attributes['name'], 'guard_name' => $attributes['guard_name']]);

        if ($permission) {
            throw PermissionAlreadyExists::create($attributes['name'], $attributes['guard_name']);
        }

        return static::query()->create($attributes);
    }
}

@mcolominas
Copy link
Author

If you want to close this bug, or mark it as an improvement, but ideally, it is to obtain the guard, either by attribute or constructor

@mcolominas
Copy link
Author

mcolominas commented Nov 10, 2021

Ideally this code will work, since config cannot be used in an attribute.

class AdminPermission extends SpatiePermission
{
    public function __construct(array $attributes = [])
    {
        $this->guard_name = $attributes['guard_name '] ?? config('xxx.guard');
         parent::__construct($attributes);
    }
}

What I want to use, is this class, to create the admin permissions, without passing any additional parameters.

I have also tried this, but it doesn't work either

class AdminPermission extends SpatiePermission
{
    public function __construct(array $attributes = [])
    {
         $this->guard_name = $attributes['guard_name '] ?? config('xxx.guard');
         $attributes['guard_name'] = $this->guard_name;
         parent::__construct($attributes);
    }
}

config('xxx.guard') is admin, but in bbdd is stored with web

@erikn69
Copy link
Contributor

erikn69 commented Nov 10, 2021

There is, you forget create, read laravel documentation

class YourCustomPermission extends SpatiePermission
{
    public function __construct(array $attributes = [])
    {
         $attributes['guard_name'] = config('xxx.guard');
         parent::__construct($attributes);         
    }
    public static function create(array $attributes = [])
    {
        $attributes['guard_name'] = $attributes['guard_name'] ?? config('xxx.guard');;

        $permission = static::getPermission(['name' => $attributes['name'], 'guard_name' => $attributes['guard_name']]);

        if ($permission) {
            throw PermissionAlreadyExists::create($attributes['name'], $attributes['guard_name']);
        }

        return static::query()->create($attributes);
    }
}

@mcolominas
Copy link
Author

mcolominas commented Nov 10, 2021

1.- In this line, $attributes['guard_name'] not exists, so it gets the guard name of 'Guard::getDefaultName(static::class)'

$attributes['guard_name'] = $attributes['guard_name'] ?? Guard::getDefaultName(static::class);

2.- In Guard::getDefaultName(static::class) call this, where in model, is a string of the class

public static function getNames($model): Collection

3.- Therefore execute this line, but since the guard_name has not been defined in the class, but in the constructor, this returns empty collection

$guardName = (new \ReflectionClass($class))->getDefaultProperties()['guard_name'] ?? null;

4.- Since the collection is empty, it returns the default guard that is usually web config('auth.defaults.guard')

public static function getDefaultName($class): string

So, wouldn't it be possible for it to also take into account what is initialized in the constructor?

If it is not possible, close the ticket or mark it as an improvement for some later update of the package.

While following your advice, I have added this method and works.

class YourCustomPermission extends SpatiePermission
{
    public static function create(array $attributes = [])
    {
        $attributes['guard_name'] = $attributes['guard_name'] ?? config('xxx.guard');
        return parent::create($attributes);
    }
}    

EDIT:
For example, with this it would take into account the constructor in case it does not have the attribute:
Change

$guardName = (new \ReflectionClass($class))->getDefaultProperties()['guard_name'] ?? null;

To

$guardName = (new \ReflectionClass($class))->getDefaultProperties()['guard_name'] ?? (new $class)->guard_name ?? null;

@erikn69
Copy link
Contributor

erikn69 commented Nov 11, 2021

$guardName = (new \ReflectionClass($class))->getDefaultProperties()['guard_name'] ?? null;
$guardName = (new \ReflectionClass($class))->getDefaultProperties()['guard_name'] ?? (new $class)->guard_name ?? null;

It's the same

@mcolominas
Copy link
Author

It's not the same, with (new $class)->guard_name ?? null; I instantiate the class and I can get the attributes that are started from the constructor, with (new \ReflectionClass($class))->getDefaultProperties()['guard_name'] you only get the attributes defined in the class, ignoring the constructor.

I'm going to close this, if you want to add it or do something else, go ahead, at the moment I have the code that works, although it is duplicated (constructor and create method)

I put what I was doing:
I have 2 guards, web and admin, the web would not have to use any permission or role, only the admin, therefore, create the class mentioned above, where the guard admin was assigned by the builder, once created, just modify config/permission.php to change the Role and Permission class to my own.

@erikn69
Copy link
Contributor

erikn69 commented Nov 11, 2021

I test it

class YourCustomPermission extends SpatiePermission
{
    public $guard_name = 'custom_guard';
}

dd(\Spatie\Permission\Guard::getDefaultName(\App\Models\YourCustomPermission ::class));
// displays "custom_guard"

@mcolominas
Copy link
Author

mcolominas commented Nov 11, 2021

The code that you have given me that works, because the attribute is in the class, but I use the config() to obtain the guard, therefore I cannot do what you use, I have to do:

class YourCustomPermission  extends Permission
{
    public function __construct()
    {
        $this->guard_name = config('xxx.guard');
    }
}

dd(\Spatie\Permission\Guard::getDefaultName(\App\Models\YourCustomPermission::class));
// displays "web"

and with

$guardName = (new \ReflectionClass($class))->getDefaultProperties()['guard_name'] ?? (new $class)->guard_name ?? null;

dd(\Spatie\Permission\Guard::getDefaultName(\App\Models\YourCustomPermission::class));
// displays "custom_guard"

@andrekutianski
Copy link

andrekutianski commented Nov 13, 2022

In my scenario I need that User Model can be associate with roles for multiple guards, to accomplish that I just define $guard_name on Model as array with all guard_names that can this can be associated.

namespace App\Models;

Class User extends UserModel
{

// ...

protected array $guard_name = ['api', 'web'];

// ...

}

With that, I can assign model to multiple guards.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants