sonata-project / SonataAdminBundle

The missing Symfony Admin Generator
https://docs.sonata-project.org/projects/SonataAdminBundle
MIT License
2.11k stars 1.26k forks source link

[Question] _ALL Role return true for ROLE_ALLOWED_TO_SWITCH and something weird about isGranted() #5992

Closed quazardous closed 3 years ago

quazardous commented 4 years ago

Hi !

the current isGranted($attribute) returns true for any attribute in a specific admin if user has the _ALL role.

https://github.com/sonata-project/SonataAdminBundle/blob/b16427b024f3399f2c3d725a79ad0667de509592/src/Security/Handler/RoleSecurityHandler.php#L64-L66

It's convenient for the _EDIT, _LIST, etc roles but it feels strange for ROLE_ALLOWED_TO_SWITCH (ie).

maybe something like:

        $allRole = sprintf($this->getBaseRole($admin), 'ALL');
        // decide if $attributes contains only base roles
        $onlyBaseRoles = true;
        ...

        try {
            return $this->isAnyGranted($this->superAdminRoles)
                || $this->isAnyGranted($attributes, $object)
                || ($onlyBaseRoles && $this->isAnyGranted([$allRole], $object));
        } catch (AuthenticationCredentialsNotFoundException $e) {
            return false;
        }

I feel something is weird with the current isGranted().

https://github.com/sonata-project/SonataUserBundle/blob/194bfa24cffb0d1982f18efe35e056ec64d779f9/src/Admin/Model/UserAdmin.php#L101-L105

The user-bundle calls a isGranted(ROLE_ALLOWED_TO_SWITCH) and the following code turns it into ROLE_SONATA_USER_ADMIN_USER_ROLE_ALLOWED_TO_SWITCH. It's at least confusing...

        foreach ($attributes as $pos => $attribute) {
            $attributes[$pos] = sprintf($this->getBaseRole($admin), $attribute);
        }

and in this specific case it seams to be not working if you put ROLE_ALLOWED_TO_SWITCH to a non admin role (with the role_hierarchy).

github-actions[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

VincentLanglet commented 4 years ago

I'm not sure to understand the issue @OskarStark. Should we really keep this ? And isn't a SonataUser-related issue instead ?

VincentLanglet commented 3 years ago

@quazardous I'm not sure to understand the issue (especially why you talk about SonataUserBundle). Can you provide a PR ?