Skip to content
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.

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

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

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

Comments

Projects
None yet
10 participants
@javiereguiluz
Copy link
Member

commented Mar 25, 2015

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

This comment has been minimized.

Copy link
Member

commented Mar 25, 2015

providing it where ?

@javiereguiluz

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2015

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

@Mx-Glitter

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2015

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

This comment has been minimized.

Copy link
Member

commented Mar 25, 2015

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

@linaori

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented Mar 27, 2015

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2015

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']

@xabbuh

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2015

👍 for @iltar's idea

@linaori

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented Aug 6, 2015

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

This comment has been minimized.

Copy link
Member

commented Sep 26, 2015

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.