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

SwitchUserRole gets lost while reloading user #3085

Closed
craue opened this issue Jan 10, 2012 · 31 comments
Closed

SwitchUserRole gets lost while reloading user #3085

craue opened this issue Jan 10, 2012 · 31 comments

Comments

@craue
Copy link
Contributor

craue commented Jan 10, 2012

When, after switching to another user, the user is reloaded, the SwitchUserRole gets lost and it's not possible to switch back to the originating user.

@manuelkiessling
Copy link
Contributor

Could you write a bit more about the context and setup, so I can try to recreate the bug (and try to fix it)?

@craue
Copy link
Contributor Author

craue commented Jan 20, 2012

Given user entity:

<?php

use FOS\UserBundle\Entity\User as BaseUser;
use Symfony\Component\Security\Core\User\EquatableInterface;
use Symfony\Component\Security\Core\User\UserInterface;

class MyUser extends BaseUser implements EquatableInterface, UserInterface {

    public function isEqualTo(UserInterface $user) {
        return false;
    }

}

User A is granted role ROLE_ALLOWED_TO_SWITCH and switches into user B. I guess the firewall now reloads the current user B and drops ROLE_PREVIOUS_ADMIN (programmatically, an instance of Symfony\Component\Security\Core\Role\SwitchUserRole is used) so that one cannot switch back to A.

@stof
Copy link
Member

stof commented Jan 20, 2012

@craue if the method always return false, the token will never be considered as authenticated (as the user is considered as having changed since last login) and so will require logging in again before accessing a restricted area

@craue
Copy link
Contributor Author

craue commented Jan 20, 2012

@stof: I know. ;) That just serves as an example. Even though, the SwitchUserRole should be preserved.

@craue
Copy link
Contributor Author

craue commented Jan 24, 2012

The actual use case is this: User A switches into B, who then performs actions to gain a new role, which requires the user to be reloaded. But he still needs to be able to switch back to A at a later time. Or am I just missing something here?

@craue
Copy link
Contributor Author

craue commented Feb 8, 2012

@schmittjoh: Do you have any idea on how to solve this maybe?

@stof
Copy link
Member

stof commented Apr 4, 2012

@schmittjoh ping

@michelsalib
Copy link

Any news on the subject ?

@asm89
Copy link
Contributor

asm89 commented Jul 12, 2012

@craue What does your code look like for gaining a new role?

I don't think this is a bug btw, but rather a missing feature?

@TheDevilOnLine
Copy link

I had the same issue and it has to do with the way you serialize your user. I actually wrote a blog about it recently: http://12wiki.blogspot.nl/2012/09/symfony-2-and-case-of-missing.html

@craue
Copy link
Contributor Author

craue commented Dec 12, 2012

@asm89: Let's just say that the new role is added in the DB directly. Now the user entity needs to be reloaded to gain the new role. But when doing this, the SwitchUserRole gets lost.

@TheDevilOnLine: I tried your approach of modifying serialization of the user entity, but it doesn't solve the issue.

@TheDevilOnLine
Copy link

@craue If you send me your user entity class, I'll take a look ;-)

@craue
Copy link
Contributor Author

craue commented Dec 12, 2012

I found out what's wrong in my code. It's indeed about serialization. Thank you for the hint, @TheDevilOnLine.

What pointed me to the solution is https://github.com/FriendsOfSymfony/FOSUserBundle/blob/6d1fe1bad7621e618a379aebc45b36e080e7eca8/Model/User.php#L152, although the comment itself seems to be outdated by the time EquatableInterface has been introduced.

I had some additional checks in the isEqualTo method of my user entity for fields which were not serialized. I just added these and now switching back to the originating user is working, finally. Whoo-hoo!

@craue
Copy link
Contributor Author

craue commented Dec 12, 2012

Maybe I was a bit too excited. 😒

SwitchUserRole still gets lost when adding $this->roles to serialize()/unserialize() and

if ($this->getRoles() != $user->getRoles()) {
    return false;
}

to isEqualTo(), while the user is reloaded after its roles changed in the DB.

@craue
Copy link
Contributor Author

craue commented Dec 14, 2012

Just an idea, but would it be a better approach to not save this special role in the user roles (which will be overwritten when reloading the user), but in a different session variable instead? Alternatively, this role has to be preserved while the user is reloaded. Any other ideas? /cc @schmittjoh

@ureimers
Copy link
Contributor

ureimers commented Jan 8, 2013

I'm currently merging my system from Symfony 2.0 to Symfony 2.1(.6).
Impersoniating in 2.0 worked flawlessly but in 2.1 it doesn't anymore. The impersionated user never gets the ROLE_PREVIOUS_ADMIN assigned. If I impersionate using the _switch_user URI and click on the impersionated user in the Symfony Profiler it never shows the ROLE_PREVIOUS_ADMIN role whereas the same code does in Symfony 2.0 (@TheDevilOnLine: I too have the log entry "Username "" was reloaded from user provider.". But I had it in 2.0 too and had no problem with it there).

Just wanted to confirm that under some conditions (which need to be found) ROLE_PREVIOUS_ADMIN is not saved into the impersionated user.

@ureimers
Copy link
Contributor

ureimers commented Jan 8, 2013

Turns out I overlooked to use Symfony\Component\Security\Core\User\EquatableInterface::isEqualTo() instead of my old Symfony\Component\Security\Core\User\UserInterface::equals() as stated under https://github.com/symfony/symfony/blob/master/UPGRADE-2.1.md#security.

After implementing the EquatablerInterface, user impersionation now works.

@frosas
Copy link
Contributor

frosas commented Feb 8, 2013

I could solve it by serializing the username (thanks @TheDevilOnLine!)

But the documentation states that:

Only the id needs to be serialized, because the refreshUser() method reloads the user on each request by using the id.

This should be corrected right?

@craue do you really need to compare roles? According to EquatableInterface:

You do not need to compare every attribute, but only those that are relevant for assessing whether re-authentication is required.

@TheDevilOnLine
Copy link

@frosas
The bug was caused because in the 'old' documentation, the refreshUser function looked like:

public function refreshUser(UserInterface $user)
    {
        $class = get_class($user);
        if (!$this->supportsClass($class)) {
            throw new UnsupportedUserException(sprintf('Instances of "%s" are not supported.', $class));
        }

        return $this->loadUserByUsername($user->getUsername());
    }

This is recently changed to:

public function refreshUser(UserInterface $user)
    {
        $class = get_class($user);
        if (!$this->supportsClass($class)) {
            throw new UnsupportedUserException(sprintf('Instances of "%s" are not supported.', $class));
        }

        return $this->find($user->getId());
    }

@frosas
Copy link
Contributor

frosas commented Feb 8, 2013

@TheDevilOnLine where did you get this implementation from? All I've seen use getUsername() (e.g.)

@TheDevilOnLine
Copy link

@frosas
It's currently like that in the documentation on http://symfony.com/doc/current/cookbook/security/entity_provider.html#authenticating-someone-with-a-custom-entity-provider

I can relate to it being like that for the In-Memory users, as these don't have an id but do have a username!

@frosas
Copy link
Contributor

frosas commented Feb 8, 2013

@TheDevilOnLine oh, you are right, sorry.

But still ROLE_PREVIOUS_ADMIN is lost somehow when username is not serialized.

@frosas
Copy link
Contributor

frosas commented Feb 9, 2013

Okay, after some digging I've found this:

  • Not serializing the properties checked at AbstractToken::hasUserChanged() makes the token become unauthenticated.
  • When this token is reauthenticated, a new token is created and previous token roles (including our beloved SwitchUserRole) are lost.

So I would properly document how a user has to be serialized and compared both in documentation and code. What do you think?

@craue
Copy link
Contributor Author

craue commented Feb 19, 2013

@frosas: Thank you for the hint about EquatableInterface. That did it.

With some further testing, I eventually just removed the methods isEqualTo, serialize, and unserialize from my user entity completely.

I can neither tell if I had it this way already nor if it worked as this issue is quite old now. But I can finally consider it as solved from my POV.

@craue
Copy link
Contributor Author

craue commented May 2, 2013

I have to come back to this (again). The issue is still not solved. In my project, I can either

  • let a new role, which is e.g. added in the DB directly, being taken into account when reloading the page (with implementing EquatableInterface for my user entity) or
  • being able to return to the originating user after switching into one (without implementing EquatableInterface),

but not both. I've chosen to have the former working.

@seiffert
Copy link
Contributor

👍
I am experiencing the same symptoms... After refreshing the user, the ROLE_PREVIOUS_ADMIN role is not present in the active token anymore (although it was persisted to session in the previous request!).

@lucabartoli
Copy link

Just solved in few steps, thanks to previous comments:

  1. Add EquatableInterface
    use Symfony\Component\Security\Core\User\EquatableInterface;
  2. For those who implements AdvancedUserInterface instead of UserInterface, remember that EquatableInterface needs UserInterface, so don't forget to add
    use Symfony\Component\Security\Core\User\UserInterface;
  3. Implements isEqualTo
    class User implements AdvancedUserInterface, EquatableInterface, \Serializable
    {
    //...
        public function isEqualTo(UserInterface $user){
            return $this->username === $user->getUsername();
        }
    //...
    }
  4. Add username to serialize and unserialize functions
    class User implements AdvancedUserInterface, EquatableInterface, \Serializable
    {
    //...
        /**
         * @see \Serializable::serialize()
         */
        public function serialize()
        {
            return serialize(array(
                $this->id,$this->username
            ));
        }
        /**
         * @see \Serializable::unserialize()
         */
        public function unserialize($serialized)
        {
            list (
                $this->id,$this->username
            ) = unserialize($serialized);
        }
    //...
    }
    

@jtreminio
Copy link

The solution provided by @lucabartoli solution works 100%.

If you're using email instead of username, your isEqualTo() would look like

public function isEqualTo(UserInterface $user){
    return $this->email === $user->getUsername();
}

and replace $this->username with $this->email in the two serialize methods.

@vtuz
Copy link

vtuz commented Sep 6, 2013

+1 for @lucabartoli
was confused because use AdvancedUserInterface

@ghost
Copy link

ghost commented Dec 15, 2013

@lucabartoli You, sir, are the man. I've spent countless hours trying to figure out this problem. I wish I would have stumbled on this thread sooner.

@ghost
Copy link

ghost commented Dec 15, 2013

Upon further inspection, I determined what my problem was. All of my User properties are private. Additionally, I'm implementing the AdvancedUserInterface, which requires the isEnabled() method that returns the property 'isActive'. Since this property is private in my User document, the expression below was evaluating to true because $this->user->isEnabled() was returning 'null', while $user->isEnabled() returned 'true'.

abstract class AbstractToken implements TokenInterface
{

...

if ($this->user->isEnabled() !== $user->isEnabled()) {
    return true;
}

...

Because of this, the AbstractToken object determined the user changed on every request, which resulted in the need for the current token instance to be re-authenticated. My custom AuthenticationProvider would then authenticate the token, replacing the roles with new ones, eliminating the ROLE_PREVIOUS_ADMIN.

class ShibbolethAuthenticationProvider implements AuthenticationProviderInterface
{

...

public function authenticate(TokenInterface $token)
{
    $user = $this->userProvider->loadUserByUsername($token->getUsername());

    if ($user) {            
        $authenticatedToken = new ShibbolethUserToken($user->getRoles());
        $authenticatedToken->setUser($user);

        return $authenticatedToken;
    }

    throw new AuthenticationException('Shib authentication failed.');
}

...

Simply adding 'isActive' to my Serializable methods in my User document resolved my issue.

class User implements AdvancedUserInterface, \Serializable
{

...

public function serialize()
{
    return serialize(array(
        $this->id, $this->username, $this->isActive,
    ));
}

public function unserialize($serialized)
{
    list (
        $this->id, $this->username, $this->isActive,
    ) = unserialize($serialized);
}

...

Either serializing the 'isActive' property OR implementing @lucabartoli's solution worked for me.

fabpot added a commit that referenced this issue Dec 29, 2013
…le. (pawaclawczyk)

This PR was squashed before being merged into the 2.3 branch (closes #8997).

Discussion
----------

[Security] Fixed problem with losing ROLE_PREVIOUS_ADMIN role.

<table>
  <tr>
    <td><b>Q</b></td>
    <td><b>A</b></td>
  </tr>
  <tr>
    <td>Bug fix?</td>
    <td>yes</td>
  </tr>
  <tr>
    <td>New feature</td>
    <td>no</td>
  </tr>
  <tr>
    <td>BC breaks?</td>
    <td>no</td>
  </tr>
  <tr>
    <td>Deprecations?</td>
    <td>no</td>
  </tr>
  <tr>
    <td>Tests pass?</td>
    <td>yes</td>
  </tr>
  <tr>
    <td>Fixed tickets</td>
    <td>#3085, #8974</td>
  </tr>
  <tr>
    <td>License</td>
    <td>MIT</td>
  </tr>
  <tr>
    <td>Doc PR</td>
    <td>n/a</td>
  </tr>
</table>

Problem occurs while user is impersonated. Authentication process generates new token and doeas not preserve role ```ROLE_PREVIOUS_ADMIN```. Ex. when parameter ```security.always_authenticate_before_granting``` is enabled.

Commits
-------

a7baa3b [Security] Fixed problem with losing ROLE_PREVIOUS_ADMIN role.
@fabpot fabpot closed this as completed Dec 29, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests