[security-core] Wrong roles comparison in hasUserChanged method

[thlbaut]

Symfony version(s) affected: 4.4 / 5.0

Description

It seems there is mistake here :

https://github.com/symfony/security-core/blob/6251c8e432640106e6f2f045ac3b365f1af36d44/Authentication/Token/AbstractToken.php#L326

How to reproduce

When simulating authentication, roles passed to UsernamePasswordToken have to be identical to user roles retrieve by user_provider. It doesn't make any sense. The fourth parameter of UsernamePasswordToken is useless in this case.

private function logIn()
    {
        $session = self::$container->get('session');

        $firewallName = 'secure_area';
        // if you don't define multiple connected firewalls, the context defaults to the firewall name
        // See https://symfony.com/doc/current/reference/configuration/security.html#firewall-context
        $firewallContext = 'secured_area';

        // you may need to use a different token class depending on your application.
        // for example, when using Guard authentication you must instantiate PostAuthenticationGuardToken
        $token = new UsernamePasswordToken('admin', null, $firewallName, ['ROLE_ADMIN']);
        $session->set('_security_'.$firewallContext, serialize($token));
        $session->save();

        $cookie = new Cookie($session->getName(), $session->getId());
        $this->client->getCookieJar()->set($cookie);
    }

Possible Solution

It should be :

if (\count($userRoles) !== \count($this->user->getRoles()) || \count($userRoles) !== \count(array_intersect($userRoles, $this->user->getRoles()))) {

instead of

if (\count($userRoles) !== \count($this->getRoleNames()) || \count($userRoles) !== \count(array_intersect($userRoles, $this->getRoleNames()))) {

[thlbaut]

Sorry for multiple pull requests, it's my first PR

[xabbuh]

This looks similar to #35509.

[pevdh]

This behavior was added in 4f4c30d (as mentioned in #35509). Isn't that commit a BC break technically?

[wouterj]

See also #35533

[ajgarlag]

I'm with @pevdh. It's technically a BC break.

I think that the intended purpose of the AbstractToken::hasUserChanged private method is to check if the user set to the token has changed, so that you can write:

if ($token->isAuthenticated()) {
    assert($token->setUser($token->getUser())->isAuthenticated());
}

But 4f4c30d broke the assertion.

@chalasr mentioned in #35533 (comment) that:

The token roles must contain all the roles of the user it holds.
The only exception to this rule is ROLE_PREVIOUS_ADMIN(impersonation) as it is added on the fly, the logic for this case is hardcoded in AbstractToken.

There is no valid use case for having a token that knows only part of the user roles, as it does not reflect the real state of the user.

With #35944 that hardcoded exception to the rule can (and should) be removed.

The token roles and the user roles are (currently) two different things, and the ROLE_PREVIOUS_ADMIN role known by the SwitchUserToken is the best demonstration of it. If they must be equal (which I think can be discussed in other issue), we should detect when they are different throwing a deprecation error in symfony/security-core >5.2 <6.0

[javiereguiluz]

@wouterj in case it helps debug this issue, @ajgarlag shared this example showing the BC break: https://gist.github.com/ajgarlag/43ec84816d72bdd513dc238c0dba6de4 Thanks.

[wouterj]

The fix for this issue has already been provided: #35944

But another PR that tried to fix this was rejected: #35533 So I'm not sure what should be done here.

From my point of view, this is a BC break and we should do the following things:

1. Fix the comparison while still checking if user roles changed (see [Security/Core] Fix wrong roles comparison #35944)
2. Maybe deprecate having different token roles and user roles in 5.1/5.2 and remove this in 6.0

@chalasr @fabpot would you agree?

[chalasr]

@wouterj Works for me.

[ajgarlag]

@thlbaut This issue is not fixed anymore after merging #37008. Could you reopen it?