Core User class returning roles as strings instead of Role objects #13037

turneliusz · 2014-12-19T14:34:33Z

I'm using Symfony 2.6

system_users:
  memory:
    users:
      my_admin: { password: "xxx", roles: 'ROLE_ADMIN' }

for that the container looks like:

    /**
     * Gets the 'security.user.provider.concrete.system_users' service.
     *
     * This service is shared.
     * This method always returns the same instance of the service.
     *
     * This service is private.
     * If you want to be able to request this service from the container directly,
     * make it public, otherwise you might end up with broken code.
     *
     * @param bool    $lazyLoad whether to try lazy-loading the service with a proxy
     *
     * @return \Symfony\Component\Security\Core\User\InMemoryUserProvider A Symfony\Component\Security\Core\User\InMemoryUserProvider instance.
     */
    public function getSecurity_User_Provider_Concrete_SystemUsersService($lazyLoad = true)
    {
        // (...)
        $instance = new \Symfony\Component\Security\Core\User\InMemoryUserProvider();

        $instance->createUser(new \Symfony\Component\Security\Core\User\User('my_admin', 'xxx', array(0 => 'ROLE_ADMIN')));

        return $instance;
    }

which is triggering an exception Call to a member function getRole() on a non-object:

(...)
Dec 19 15:28:54 damian-ThinkPad-T440p [3861]: PHP 18. XXX\Bundle\AdminBundle\Security\Voter\AdminAccessVoter->isGranted($attribute = uninitialized, $object = uninitialized, $user = uninitialized) /home/damian/PhpstormProjects/vendor-backend/vendor/symfony/symfony/src/Symfony/Component/Security/Core/Authorization/Voter/AbstractVoter.php:76
Dec 19 15:28:54 damian-ThinkPad-T440p [3861]: PHP 19. Symfony\Component\Security\Core\Role\RoleHierarchy->getReachableRoles($roles = uninitialized) /home/damian/PhpstormProjects/vendor-backend/src/XXX/Bundle/AdminBundle/Security/Voter/AdminAccessVoter.php:65
Dec 19 15:28:54 damian-ThinkPad-T440p [3861]: request.CRITICAL: Uncaught PHP Exception Symfony\Component\Debug\Exception\FatalErrorException: "Error: Call to a member function getRole() on a non-object" at /home/damian/PhpstormProjects/vendor-backend/vendor/symfony/symfony/src/Symfony/Component/Security/Core/Role/RoleHierarchy.php line 43 {"exception":"[object](Symfony\Component\Debug\Exception\FatalErrorException: Error: Call to a member function getRole%28%29 on a non-object at /home/damian/PhpstormProjects/vendor-backend/vendor/symfony/symfony/src/Symfony/Component/Security/Core/Role/RoleHierarchy.php:43)"} []

as Core User code looks like:

    private $roles;

    public function __construct($username, $password, array $roles = array(), $enabled = true, $userNonExpired = true, $credentialsNonExpired = true, $userNonLocked = true)
    {
        // ...

        $this->roles = $roles;
    }

    /**
     * {@inheritdoc}
     */
    public function getRoles()
    {
        return $this->roles;
    }

UserInterface::getRoles() defines:

    /**
     * Returns the roles granted to the user.
     * (...)
     * @return Role[] The user roles
     */
    public function getRoles();

Am I doing something wrong or it's a bug?

The text was updated successfully, but these errors were encountered:

inmarelibero · 2014-12-19T21:46:28Z

hi, where in your code are you calling getRoles()?

turneliusz · 2014-12-19T22:01:30Z

As in stack trace, I'm not calling it:

Symfony\Component\Security\Core\Role\RoleHierarchy->getReachableRoles

EDIT: Anyway, it doesn't even matter, the DIC code created in the security extension seems wrong or I am missing something?

inmarelibero · 2014-12-19T22:04:52Z

I would exclude code is wrong, but.. can you explain more your setup please?
like:

version of symfony
your security.yml
are you trying to login?
is it a fresh symfony installation?

turneliusz · 2014-12-19T22:14:46Z

I don't have the source right now but I can give some copy-paste from Security component

The extension code:

$definition = $container->setDefinition($id, new DefinitionDecorator('security.user.provider.in_memory'));
foreach ($config['users'] as $username => $user) {
  $userId = $id.'_'.$username;
  $container
  ->setDefinition($userId, new DefinitionDecorator('security.user.provider.in_memory.user'))
  ->setArguments(array($username, (string) $user['password'], $user['roles']))
  ;
  $definition->addMethodCall('createUser', array(new Reference($userId)));
}

Core User class defined:

<parameter key="security.user.provider.in_memory.user.class">Symfony\Component\Security\Core\User\User</parameter>

constructor just setts that array of strings while the interface requires array of Role instances.

I will try to create a test for that but from the code it's obvious that it's a bug, correct me if I'm wrong.

johnkary · 2014-12-22T07:30:15Z

Please post a copy of your AdminAccessVoter. It looks like that class is calling something on Line 65 that's invoking the RoleHierarchy. The RoleHierarchy object stores the hierarchy as all role strings, but expects the $roles array argument via ->getReachableRoles($roles) to all be RoleInterface objects, which they should be if that list is coming from the Security Token.

src/XXX/Bundle/AdminBundle/Security/Voter/AdminAccessVoter.php:65

When each Voter goes to make a voting decision in its vote() method, it has arguments of the Security Token, an object being voted on and a list of attributes being voted on. If a Voter needs the list of roles for the current user, it should only use Token->getRoles(), which returns an array of objects implementing RoleInterface. So how do those RoleInterface (Role class) objects get created, you ask?

When a Token is created (at least if it extends AbstractToken), it is supplied the User object. The User->getRoles() list is iterated, each converted into RoleInterface objects then stored with the Token, which is then stored in the TokenStorage (previously SecurityContext < 2.6).

turneliusz · 2014-12-29T08:24:07Z

It's quite simple:

protected function isGranted($attribute, $object, $user = null)
{
    $decision = self::ACCESS_ABSTAIN;
    if ($user instanceof UserInterface) {
        $roles = $this->roleHierarchy->getReachableRoles($user->getRoles());
        foreach ($roles as $role) {
            if ($role->getRole() === Role::ROLE_ADMIN) {
                $decision = self::ACCESS_GRANTED;
                break;
            }
        }
    }
    return $decision;
}

Still I don't understand why created Symfony\Component\Security\Core\User\User (in DIC) instance which requires roles to be RoleInterface objects has them as strings and returns them as strings.

turneliusz · 2014-12-29T09:45:06Z

What I've found in documentation is:

This is because Symfony's security system requires that the User::getRoles method returns an array of either role strings or objects that implement this interface. If Role didn't implement this interface, then User::getRoles would need to iterate over all the Role objects, call getRole on each, and create an array of strings to return. Both approaches are valid and equivalent.

So the issue seems to be the docblock of UserInterface::getRoles() that has @return Role[] The user roles and should be @return string[]|Role[], right?

jakzal · 2014-12-29T11:11:31Z

So the issue seems to be the docblock of UserInterface::getRoles() that has @return Role[] The user roles and should be @return string[]|Role[], right?

Yes. Would you mind sending a PR fixing the docblock for the UserInterface::getRoles()?

turneliusz · 2014-12-29T11:13:20Z

IMO it would be nice to create some TODO to unify access to roles across the codebase, as everywhere we require to use RoleInterface instances and UserInterface doesn't make any constraints. Nice to have for 3.0 IMHO

xelaris · 2014-12-29T11:19:12Z

@turneliusz have a look at #11742 it's on the list

Remove the RoleInterface and the Role class in favor of using strings only for roles

stof · 2014-12-29T11:19:35Z

The user is allowed to return strings since the very beginning of Symfony actually. This is a mistake in the phpdoc.
For 3.0, there was a discussion about dropping Role objects altogether in favor of using strings

CMatias · 2015-09-21T19:03:21Z

When executing this piece of code:

$this->roleHierarchy->getReachableRoles($user->getRoles());

I get the following error: "Error: Call to a member function getRole() on string". Because Symfony is treating strings as role objects on Symfony/Component/Security/Core/Role/RoleHierarchy.php.

$reachableRoles = $roles;
    foreach ($roles as $role) {
        if (!isset($this->map[$role->getRole()])) {
            continue;
        }

Shouldn't the code be changed to something like

$reachableRoles = $roles;
    foreach ($roles as $role) {
        if(is_string($role)){
            if (!isset($this->map[$role])) {
                continue;
            }

            foreach ($this->map[$role] as $r) {
                $reachableRoles[] = $r;
            }
        }
        else {
            if (!isset($this->map[$role->getRole()])) {
                continue;
            }

            foreach ($this->map[$role->getRole()] as $r) {
                $reachableRoles[] = new Role($r);
            }
        }
    }

This PR was merged into the 2.3 branch. Discussion ---------- Fixed the phpDoc of UserInterface | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #13037 | License | MIT | Doc PR | - #13037 is the original issue ... which was tried to be fixed in #13146 ... which was closed without merging it. Commits ------- 8e2a33e Fixed the phpDoc of UserInterface

CMatias · 2016-01-25T16:50:31Z

Hey, this commit does not solve the issue as role objects are still being treated as strings in the RoleHierarchy.php file.

I've implemented the patch on my side (the phpDoc change) and the "Error: Call to a member function getRole() on string" persists.

xabbuh · 2016-01-30T08:58:15Z

@CMatias Can you please show the complete stack trace of that error and a way to reproduce your issue?

xabbuh · 2016-01-31T07:55:55Z

@CMatias Closing here as I now understand your issue which is related to how you use the RoleHierarchy class. The RoleHierarchyInterface clearly states that the passed arguments must be objects implementing the RoleInterface: https://github.com/symfony/symfony/blob/2.3/src/Symfony/Component/Security/Core/Role/RoleHierarchyInterface.php#L27 If your User class can return strings in getRoles(), you need to make sure to convert them yourself before passing them to getReachableRoles().

CMatias · 2016-01-31T14:00:43Z

This issue stood with us for a long time. As we upgraded to 2.8 the new way of voting with decisionManager no longer required us to do that fix. Sorry and thanks for the help.

turneliusz mentioned this issue Dec 29, 2014

[Security] Missing return type in "UserInterface" #13146

Closed

fabpot added this to the 3.0 milestone Jan 2, 2015

fabpot added the Security label Jan 2, 2015

xabbuh mentioned this issue Jan 26, 2015

Better getRoles() annotation in UserInterface #13530

Closed

javiereguiluz mentioned this issue Jan 25, 2016

Fixed the phpDoc of UserInterface #17525

Merged

fabpot closed this as completed Jan 25, 2016

xabbuh added the Unconfirmed label Jan 30, 2016

xabbuh reopened this Jan 30, 2016

xabbuh closed this as completed Jan 31, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Core User class returning roles as strings instead of Role objects #13037

Core User class returning roles as strings instead of Role objects #13037

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Core User class returning roles as strings instead of Role objects #13037

Core User class returning roles as strings instead of Role objects #13037

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!