Allow hasAllPermissions and hasAnyPermission to run on custom guard for wildcard permissions

[AlexandreBellas]

I'm using wildcard permissions. Nowadays, in the trait HasPermissions.php, there is the following code:

    /**
     * Determine if the model has all of the given permissions.
     *
     * @param  string|int|array|Permission|Collection|\BackedEnum  ...$permissions
     */
    public function hasAllPermissions(...$permissions): bool
    {
        $permissions = collect($permissions)->flatten();

        foreach ($permissions as $permission) {
            if (! $this->checkPermissionTo($permission)) {
                return false;
            }
        }

        return true;
    }

    // ...

    /**
     * An alias to hasPermissionTo(), but avoids throwing an exception.
     *
     * @param  string|int|Permission|\BackedEnum  $permission
     * @param  string|null  $guardName
     */
    public function checkPermissionTo($permission, $guardName = null): bool
    {
        try {
            return $this->hasPermissionTo($permission, $guardName);
        } catch (PermissionDoesNotExist $e) {
            return false;
        }
    }

    // ...

    /**
     * Determine if the model may perform the given permission.
     *
     * @param  string|int|Permission|\BackedEnum  $permission
     * @param  string|null  $guardName
     *
     * @throws PermissionDoesNotExist
     */
    public function hasPermissionTo($permission, $guardName = null): bool
    {
        if ($this->getWildcardClass()) {
            return $this->hasWildcardPermission($permission, $guardName);
        }

        $permission = $this->filterPermission($permission, $guardName);

        return $this->hasDirectPermission($permission) || $this->hasPermissionViaRole($permission);
    }

There isn't a way to define a custom guard for hasAllPermissions. By the way, in the method checkPermissionTo there is, as in hasPermissionTo.

My initial thought was "well, if hasAllPermissions doesn't have a way to define my custom guard, so sending the permission models (as they know their guard) would be enough, right?". In reality, this is not the case. The default guard is chosen in hasWildcardPermission.

    /**
     * Validates a wildcard permission against all permissions of a user.
     *
     * @param  string|int|Permission|\BackedEnum  $permission
     * @param  string|null  $guardName
     */
    protected function hasWildcardPermission($permission, $guardName = null): bool
    {
        $guardName = $guardName ?? $this->getDefaultGuardName();

        if ($permission instanceof \BackedEnum) {
            $permission = $permission->value;
        }

        if (is_int($permission) || PermissionRegistrar::isUid($permission)) {
            $permission = $this->getPermissionClass()::findById($permission, $guardName);
        }

        if ($permission instanceof Permission) {
            $permission = $permission->name;
        }

        if (! is_string($permission)) {
            throw WildcardPermissionInvalidArgument::create();
        }

        return app($this->getWildcardClass(), ['record' => $this])->implies(
            $permission,
            $guardName,
            app(PermissionRegistrar::class)->getWildcardPermissionIndex($this),
        );
    }

It's important to notice that hasAllPermissions has a spread-operator argument, which disallows to inform the desired guard as a function argument.

It would be nice to make hasWildcardPermission run the implies logic over the guard of each Permission model, and if it is not a model but a string/ID/etc, then it would use the default guard or any logic that would make sense.

The same applies to hasAnyPermission, in the same conditions (wildcard permissions, spread operator argument blocking new ones, usage of checkPermissionTo and hasPermissionTo).

Surely I could use a foreach using only checkPermissionTo with the custom guard or even override the hasAllPermissions method in the model, but the idea is to improve the developer experience when using the package.

What do you think? Does this make sense?

[drbyte]

Thanks for the detailed writeup.

I think your point about checking the guard only if it is not a model is probably the simplest and most logical fix.

Within hasWildcardPermission() could it even be as simple as:

        if ($permission instanceof Permission) {
            $permission = $permission->name;
+            $guard_name = $permission->guard_name;
        }

[drbyte]

I'm not seeing it as a breaking change. If anything, more of a bug fix, as in a welcome implementation of expected behavior.

Perhaps writing some tests for both the current operation and the change, and seeing what tests would need to be changed to support this change, might reveal how badly breaking anything might be.

[AlexandreBellas]

The possible breaking change I see is a code that uses hasAllPermissions with models but expects to assert them over the default guard (instead of the guard of each model). But as you said, I agree it is an implementation of expected behavior.

Is there a plan to include this fix in an already existing PR or may I create a PR for this?

[drbyte]

A PR is welcome.

Are you comfortable with including tests for it?

[AlexandreBellas]

Yes! Soon I'll submit the PR.
Thanks

[AlexandreBellas]

Done: #2608
Waiting for approval ;)