8000 [security-core] Wrong roles comparison in hasUserChanged method · Issue #35941 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[security-core] Wrong roles comparison in hasUserChanged method #35941

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
thlbaut opened this issue Mar 3, 2020 · 9 comments
Closed

[security-core] Wrong roles comparison in hasUserChanged method #35941

thlbaut opened this issue Mar 3, 2020 · 9 comments
Labels
Bug Security Status: Needs Review

Comments

@thlbaut
Copy link
Contributor
thlbaut commented Mar 3, 2020 •
edited
Loading

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 thlbaut added the Bug label Mar 3, 2020
thlbaut added a commit to thlbaut/security-core that referenced this issue Mar 3, 2020
@thlbaut
Update AbstractToken.php
bbf6f49 
Issue : symfony/symfony#35941
@thlbaut thlbaut mentioned this issue Mar 3, 2020
Closed
thlbaut added a commit to thlbaut/symfony that referenced this issue Mar 3, 2020
@thlbaut
Fix wrong roles comparison
8949516 
Issue : symfony#35941
@thlbaut thlbaut mentioned this issue Mar 3, 2020
Closed
thlbaut added a commit to thlbaut/symfony that referenced this issue Mar 3, 2020
@thlbaut
Fix wrong roles comparison
e041dec 
Issue : symfony#35941
@thlbaut thlbaut mentioned this issue Mar 3, 2020
Closed
@thlbaut
Copy link
Contributor Author
thlbaut commented Mar 3, 2020

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

@xabbuh xabbuh added the Security label Mar 3, 2020
@thlbaut thlbaut mentioned this issue Mar 3, 2020
Merged
@xabbuh
Copy link
Member
xabbuh commented Mar 5, 2020

This looks similar to #35509.

@pevdh
Copy link
pevdh commented Mar 5, 2020

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

@wouterj
Copy link
Member
wouterj commented Mar 5, 2020

See also #35533

@ajgarlag
Copy link
Contributor
ajgarlag commented Mar 6, 2020 •
edited
Loading

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

@ajgarlag ajgarlag mentioned this issue May 4, 2020
Closed
@javiereguiluz
Copy link
Member

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

@wouterj
Copy link
Member
wouterj commented May 22, 2020 •
edited
Loading

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
Copy link
Member
chalasr commented May 22, 2020

@wouterj Works for me.

@fabpot fabpot closed this as completed May 22, 2020
fabpot added a commit that referenced this issue May 22, 2020
@fabpot
bug #35944 [Security/Core] Fix wrong roles comparison (thlbaut)
2e46c63 
This PR was merged into the 4.4 branch.

Discussion
----------

[Security/Core] Fix wrong roles comparison

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #35941
| License       | MIT

Fix wrong roles comparison.

Commits
-------

7d2ad4b Fix wrong roles comparison
@wouterj wouterj mentioned this issue May 29, 2020
Merged
nicolas-grekas added a commit that referenced this issue May 30, 2020
@nicolas-grekas
bug #37008 [Security] Fixed AbstractToken::hasUserChanged() (wouterj)
bdb01db 
This PR was squashed before being merged into the 4.4 branch.

Discussion
----------

[Security] Fixed AbstractToken::hasUserChanged()

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #36989
| License       | MIT
| Doc PR        | -

This PR completely reverts #35944.

That PR tried to fix a BC break (ref #35941, #35509) introduced by #31177. However, this broke many authentications (ref #36989), as the User is serialized in the session (as hinted by @stof). Many applications don't include the `roles` property in the serialization (at least, the MakerBundle doesn't include it).

In 5.2, we should probably deprecate having different roles in token and user, which fixes the BC breaks all together.

Commits
-------

f297beb [Security] Fixed AbstractToken::hasUserChanged()
@ajgarlag
Copy link
Contributor
ajgarlag commented Jun 1, 2020

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

@ajgarlag ajgarlag mentioned this issue Jun 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Security Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@fabpot @javiereguiluz @ajgarlag @wouterj @xabbuh @thlbaut @pevdh @chalasr @carsonbot
0