8000 [DX] Provide an easy way to check if a user has a security role · Issue #14048 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DX] Provide an easy way to check if a user has a security role #14048

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Closed
javiereguiluz opened this issue Mar 25, 2015 · 14 comments
Closed

[DX] Provide an easy way to check if a user has a security role #14048

javiereguiluz opened this issue Mar 25, 2015 · 14 comments

Comments

@javiereguiluz
Copy link
Member

The problem

As many of you know, the getRoles() method of the UserInterface doesn't take into account the role hierarchy that you may have defined in your application.

That's why usually you must end up with a code similar to the following:

use Symfony\Component\Security\Core\Role\RoleHierarchyInterface;
use Symfony\Component\Security\Core\User\UserInterface;

private $roleHierarchy;

public function __construct(RoleHierarchyInterface $roleHierarchy = null)
{
    $this->roleHierarchy = $roleHierarchy;
}

private function userHasRole(UserInterface $user, $roleName)
{
    if (null === $this->roleHierarchy) {
        return in_array($roleName, $user->getRoles(), true);
    }

    foreach ($this->roleHierarchy->getReachableRoles($user->getRoles()) as $role) {
        if ($roleName === $role->getRole()) {
            return true;
        }
    }

    return false;
}

The solution

The obvious solution would be to provide a method such as ->hasRole(string $role_name). What do you think?
@stof
Copy link
Member
stof commented Mar 25, 2015

providing it where ?

@javiereguiluz
Copy link
Member Author

@stof quite honestly I have no idea :) That's why I was so vague in the solution details 8000 . Let's wait for the Symfony community to think about some amazing solution.

@Mx-Glitter
Copy link
Contributor

It's a need I had in several situations, so 👍 for this helper. As to where to put it, the first place I'd look for such a method would be directly in UserInterface as I'm wondering whether a User has a Role, but it would couple the implementation User to RoleHierarchyInterface the way you wrote it.

A RoleChecker class maybe?

@wouterj
Copy link
Member
wouterj commented Mar 25, 2015

Given that we already have an AuthenticationUtils class, what about adding an AuthorizationUtils class with this method?

@linaori
Copy link
Contributor
linaori commented Mar 25, 2015

Isn't this just something that should be handled by a voter via isGranted? I don't like the idea of creating a second entry point to push roles in.

@stof
Copy link
Member
stof commented Mar 27, 2015
< 8000 /h3>

@Triiistan it cannot be in the UserInterface. your user cannot be aware of the role hierarchy (and it would force any user implementation to duplicate the logic).

@iltar it is already handled by the voter (RoleVoter or RoleHierarchyVoter depending on whether you configure the hierarchy or no). this is why I'm asking where it should be provided

@Sander-Toonen
Copy link
Contributor

I was confronted with the same issue a while back. I needed to query all users from the database with a specific role (not for anything security related), but since this role could be assigned trough inheritance I could not simply query the database for users having the role. Instead I created an inverted variant of the RoleHierarchy. It returns all roles that lead to a specific role. You can then query the database for users having one of these roles.

Don't know whether it is useful but just in case, here is a gist:

https://gist.github.com/Sander-Toonen/1e03b4e729bc1d3c982d

Example:
$inversedRoleHierarchy->getParentRoles(['ROLE_MAY_EDIT_ARTICLE']); --> ['ROLE_ADMIN', 'ROLE_EDITOR']

@javiereguiluz javiereguiluz mentioned this issue Jul 29, 2015
Closed
@xabbuh
Copy link
Member
xabbuh commented Aug 2, 2015

Something like a RoleChecker or an AuthorizationUtils class sounds like a good idea to me. The role voters could simply delegate decisions to this new class.

@linaori
Copy link
Contributor
linaori commented Aug 2, 2015

What about flattening the roles instead when logging in? That would make it run-time even better.

In my custom authentication implementation, I've created a RoleProvider. Using this to flatten the hierarchy and optionally provide extra roles based on logic (or maybe even expression language) would be a nice alternative. Would make the run-time checks as simple as in_array($role, $roles) and no need for additional complexity.

@sstok
Copy link
Contributor
sstok commented Aug 2, 2015

👍 for @iltar's idea

@linaori
Copy link
Contributor
linaori commented Aug 2, 2015

/ping @weaverryan what do you think about what I propose here, would that be something you could easily implement in your Guard Authenticator?

@apfelbox
Copy link
Contributor
apfelbox commented Aug 2, 2015

I think this is the current idea, just wanted to be sure: wouldn't the easiest (from a DX pov) solution be to just extend isGranted that it doesn't just work on the currently logged in user but on every user?

$o->isGranted("ROLE_ADMIN", null);

$o->isGrantedOnUser("ROLE_ADMIN", null, $user);
// or even just
$o->isGranted("ROLE_ADMIN", null, $someUser);

Where in the case of $someUser === null the user from the security context would be used.

@weaverryan
Copy link
Member

I second @apfelbox's thoughts. And this is already possible via AccessDecisionManager::decide(). The downside is that this is a private service. The good thing is that usually you're doing this work in a voter, so you have no problems injecting the security.access.decision_manager service (actually, this is only possible in 2.8 due to a circular dependency problem we fixed).

If you want to know if a user has a role, I think you should always go through the security system. If you use the AccessDecisionManager, you can even check for other users. If you need to be able to query for all users with a role, that's not covered - you should handle that in your persistence logic (i.e. don't use role hierarchy, or tweak your query to take into account role hierarchy).

My vote is to do nothing: use the security.access.decision_manager service or normal security.authorization_checker in all cases to check roles.

@weaverryan
Copy link
Member

Since #15870 was accepted to 2.8, you will now be able to use the security.access.decision_manager service in your voter to accomplish this (or to call any other voters):

class SomeVoter extends AbstractVoter
{
    private $decisionManager;

    // inject the security.access.decision_manager service
    public function __construct(AccessDecisionManagerInterface $decisionManager)
    {
         $this->decisionManager = $decisionManager;
    }

    protected function voteOnAttribute($attribute, $object, TokenInterface $token)
    {
        if ($this->decisionManager->decide($token, array('ROLE_SUPER_ADMIN'))) {
            return true;
        }
    }
}

So, this is a documentation issue now - see symfony/symfony-docs#4389

@javiereguiluz I'm closing this issue - re-open it if you disagree.

@javiereguiluz javiereguiluz mentioned this issue Apr 22, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
0