8000 Security Voter not aware of role hierarchy in Symfony Best Practises · Issue #4389 · symfony/symfony-docs · GitHub
[go: up one dir, main page]
Skip to content

Security Voter not aware of role hierarchy in Symfony Best Practises #4389

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
richard-keller opened this issue Oct 28, 2014 · 8 comments
Closed

Security Voter not aware of role hierarchy in Symfony Best Practises #4389

richard-keller opened this issue Oct 28, 2014 · 8 comments
Labels
actionable Clear and specific issues ready for anyone to take them. hasPR A Pull Request has already been submitted for this issue.
Milestone
2.8

Comments

@richard-keller
Copy link

The section on Security suggests using Security Voters to implement complex security logic. The provided code example makes use of the getRoles() function to determine whether the user has the given role. However, it should be noted that this won't work if role hierarchies are used, since the User entity is not aware of the full role hierarchy.

Code snippet in question:

protected function isGranted($attribute, $post, $user = null)
{
    if ($attribute == self::CREATE && in_array(ROLE_ADMIN, $user->getRoles())) {
      return true;
    }

    return false;
}

I imagine that the best way to handle this would be to use something like $securityContext->isGranted('ROLE_ADMIN') but then we should be injecting security.context, and not the User.

Edit: It would appear that using security.context isn't a great idea, since it works using the token for the currently logged-in user. Regardless, I think the documentation should note that the solution provided will not work for hierarchical role structures.
@richard-keller
Copy link
Author

So it turns out that injecting security.context so that we can use isGranted() will not work, since it results in a circular dependency.

Update: the following does not work. $container->getParameter('security.role_hierarchy.roles') returns all the roles, not just the ones assigned to the user.

The following is a working solution

Inject the Service Container into the voter via the constructor:

services:
    call_voter:
        class:  Acme\FooBundle\Security\CustomVoter
        public: false
        arguments: ['@service_container']
        tags:
            - { name: security.voter }

The the voter class would look something like this:

class CustomVoter extends AbstractVoter
{
    const DELETE = 'delete'; 
    private $roles;

    function __construct(ContainerInterface $container)
    {
        $this->roles = array_keys($container->getParameter('security.role_hierarchy.roles'));
    }

    protected function isGranted($attribute, $object, $user = null)
    {
        if (!$user instanceof UserInterface)
        {
            return false;
        }

        if ($attribute == self::DELETE)
        {
            if (in_array('ROLE_USER', $this->roles))
                return true;
        }

        return false;
    }
}

@xabbuh
Copy link
Member
xabbuh commented Oct 29, 2014

@richard-keller Your code always returns true when ROLE_USER is in the role hierarchy.

@jdachtera
Copy link

First inject @security.role_hierarchy into the voter. Then use this code:

protected function hasRole($roleName, TokenInterface $token)
{
    foreach ($this->roleHierarchy->getReachableRoles($token->getRoles()) as $role) {
        if ($roleName === $role->getRole()) {
            return TRUE;
        }
    }
    return FALSE;
}

@javiereguiluz javiereguiluz mentioned this issue Feb 19, 2015
Closed
@wouterj wouterj added the hasPR A Pull Request has already been submitted for this issue. label Feb 19, 2015
@wouterj
Copy link
Member
wouterj commented Jul 29, 2015

If I understand things correctly, this can't be fixed at the moment. @javiereguiluz can you please confirm?

@javiereguiluz
Copy link
Member

@wouterj sadly this problem cannot be solved yet. There is a pending PR symfony/symfony#14048 which unfortunately didn't get much attention.

@weaverryan weaverryan added this to the 2.8 milestone Sep 26, 2015
@weaverryan weaverryan removed the hasPR A Pull Request has already been submitted for this issue. label Sep 26, 2015
@weaverryan weaverryan mentioned this issue Sep 26, 2015
Closed
@weaverryan
Copy link
Member

See symfony/symfony#14048 (comment) - this is now actionable in 2.8 and we really should have a cookbook article or section about this.

@weaverryan weaverryan added the actionable Clear and specific issues ready for anyone to take them. label Sep 26, 2015
@weaverryan
Copy link
Member

this is now fixable in Symfony 2.8: see #5908 for the example. Someone needs to update best_practices/security.rst to close this issue and #5728

@wouterj wouterj mentioned this issue Nov 28, 2015
Merged
@wouterj wouterj added the hasPR A Pull Request has already been submitted for this issue. label Nov 28, 2015
weaverryan added a commit that referenced this issue Nov 30, 2015
@weaverryan
feature #5926 Update voter section of best practices (WouterJ)
757e159 
This PR was merged into the 2.8 branch.

Discussion
----------

Update voter section of best practices

| Q | A
| --- | ---
| Doc fix? | yes
| New docs? | yes
| Applies to | 2.8+
| Fixed tickets | #4389, #5728

Commits
-------

68da041 Update voter section of best practices
@weaverryan
Copy link
Member

Fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
actionable Clear and specific issues ready for anyone to take them. hasPR A Pull Request has already been submitted for this issue.
Projects
None yet
Milestone
2.8
Development

No branches or pull requests
6 participants
@javiereguiluz @weaverryan @wouterj @jdachtera @richard-keller @xabbuh
0