[Security] Fix user role restriction. #35533

gorshkov-ag · 2020-01-31T05:59:31Z

It was impossible to restrict user roles on the fly without deauthentication.

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

Allowed to restrict current user roles without changing roles for user object.

It was impossible to restrict user roles on the fly without deauthentication.

nicolas-grekas · 2020-01-31T09:06:03Z

Can you add a test case please?

xabbuh · 2020-01-31T09:08:26Z

It was impossible to restrict user roles on the fly

@gorshkov-ag Can you explain a bit what you mean with that?

gorshkov-ag · 2020-02-03T09:26:01Z

It was impossible to restrict user roles on the fly

@gorshkov-ag Can you explain a bit what you mean with that?

I have some user roles in application and need a possibility to switch them without changing user or user roles.

gorshkov-ag · 2020-02-03T13:05:39Z

Can you add a test case please?

Added test cases. Updated rule don't compare count of current user roles with count of applicable roles.

nicolas-grekas · 2020-02-04T13:25:04Z

@oleg-andreyev could you please have a look, since this changes the logic from #31177?

oleg-andreyev · 2020-02-05T12:28:10Z

src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php

@@ -276,7 +276,7 @@ private function hasUserChanged(UserInterface $user): bool
            $userRoles[] = 'ROLE_PREVIOUS_ADMIN';
        }

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


Can you please update also

symfony/src/Symfony/Component/Security/Core/User/User.php

Line 148 in 4f4c30d

$rolesChanged = \count($currentRoles) !== \count($newRoles) || \count($currentRoles) !== \count(array_intersect($currentRoles, $newRoles));

Done https://github.com/gorshkov-ag/symfony/commit/429dee7efb5d018a67d0654bfbc956cb08d1b86c

Now is it problem in User equals test: User without role now is equivalent with user with role "ROLE", but opposite is not true.
https://github.com/gorshkov-ag/symfony/blob/9de09f0cfbfed2af1f7ae0a63597171615a51ac3/src/Symfony/Component/Security/Core/Tests/User/UserTest.php#L120

oleg-andreyev · 2020-02-05T12:29:38Z

src/Symfony/Component/Security/Core/Tests/Authentication/Token/SwitchUserTokenTest.php

@@ -72,5 +72,53 @@ public function getSalt()
        $token = new SwitchUserToken($impersonated, 'bar', 'provider-key', ['ROLE_USER', 'ROLE_PREVIOUS_ADMIN'], $originalToken);
        $token->setUser($impersonated);
        $this->assertTrue($token->isAuthenticated());
+
+        $impersonator = new class() implements UserInterface {


I think it's better to create a separate unit for this

oleg-andreyev · 2020-02-05T12:30:13Z

src/Symfony/Component/Security/Core/Tests/Authentication/Token/SwitchUserTokenTest.php

+
+        $token = new SwitchUserToken($impersonator, 'foo', 'provider-key', ['ROLE_TEST', 'ROLE_PREVIOUS_ADMIN'], $originalToken);
+        $token->setUser($impersonator);
+        $this->assertFalse($token->isAuthenticated());


assertFalse contradicts with the unit name testSetUserDoesNotDeauthenticate

Updated test cases https://github.com/gorshkov-ag/symfony/commit/9de09f0cfbfed2af1f7ae0a63597171615a51ac3

chalasr · 2020-02-15T15:34:29Z

The current behavior is the expected one. Removing a role must trigger deauthentication.
Looking at the linked issue, it seems like you are manually changing the session token from a controller, which is probably not a good idea.
👎 for this change.

gorshkov-ag · 2020-02-17T05:34:16Z

Not removing role for user, just for token to use one or some user roles, not all of them.

chalasr · 2020-02-17T06:20:15Z

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.

dmaicher · 2020-02-17T08:11:11Z

I think this default behavior of de-authenticating on any role change seems good. So not sure we should change anything here as @chalasr said.

@gorshkov-ag maybe for your usecase its enough to make your user implement EquatableInterface and customize the logic when to consider a user changed? This would by-pass the default behavior.

fabpot · 2020-02-17T08:20:10Z

Closing as I agree with @chalasr

jmalinens · 2020-02-19T09:01:35Z

@fabpot this has bitten us after symfony4.4 upgrade because by default we give ROLE_USER role but when user accesses admin panel (separate symfony instance which shares session with main website) and he is in the list of admins we add ROLE_ADMIN for him.

After symfony upgrade this does not work anymore as adding ROLE_ADMIN deauthenticates user.

This is one of the multiple breaking changes in symfony authentication in minor versions we have to deal with :/

dmaicher · 2020-02-19T09:53:11Z

@jmalinens its hard (impossible) to anticipate every possible way people are using symfony and rely on behavior that was never intended 😉

So see the positive side here: now this makes things more secure by default. If you need to restore the previous behavior its quite simple: make your user implement EquatableInterface and do not check the roles at all.

oleg-andreyev · 2020-02-19T10:30:44Z

@jmalinens I'm not aware of your implementation but you need to make sure that you ADD not REPLACE your ROLE_USER.

Probably \count($currentRoles) !== \count($newRoles) is too strict and here is an example that @gorshkov-ag and @jmalinens faced:
http://sandbox.onlinephpfunctions.com/code/0bedfc6e52a54307fa06c91154a9854a81d0cecc

jmalinens · 2020-02-19T10:39:29Z

Thanks guys.
I went with implementing EquatableInterface and at the same time got rid of AdvancedUserInterface.

Allowed to restrict user roles.

4cf925b

It was impossible to restrict user roles on the fly without deauthentication.

gorshkov-ag requested review from dunglas, lyrixx, sroze and xabbuh as code owners January 31, 2020 05:59

carsonbot added Status: Needs Review Security Bug labels Jan 31, 2020

nicolas-grekas added this to the 4.4 milestone Jan 31, 2020

nicolas-grekas changed the base branch from 4.4 to master January 31, 2020 09:05

gorshkov-ag added 2 commits February 3, 2020 15:56

Update user role restriction.

2478205

Add test cases to user role restriction.

4f84a99

gorshkov-ag closed this Feb 3, 2020

gorshkov-ag reopened this Feb 3, 2020

oleg-andreyev suggested changes Feb 5, 2020

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Feb 5, 2020

gorshkov-ag added 2 commits February 13, 2020 16:25

Update user roles comparison.

429dee7

Update user role restriction test cases.

9de09f0

fabpot closed this Feb 17, 2020

wouterj mentioned this pull request Mar 5, 2020

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

Closed

gorshkov-ag deleted the patch-1 branch June 10, 2020 11:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] Fix user role restriction. #35533

[Security] Fix user role restriction. #35533

gorshkov-ag commented Jan 31, 2020

nicolas-grekas commented Jan 31, 2020

xabbuh commented Jan 31, 2020

gorshkov-ag commented Feb 3, 2020

gorshkov-ag commented Feb 3, 2020

nicolas-grekas commented Feb 4, 2020

oleg-andreyev Feb 5, 2020

gorshkov-ag Feb 13, 2020

gorshkov-ag Feb 13, 2020 •

edited

Loading

oleg-andreyev Feb 5, 2020

oleg-andreyev Feb 5, 2020

gorshkov-ag Feb 13, 2020

chalasr commented Feb 15, 2020 •

edited

Loading

gorshkov-ag commented Feb 17, 2020

chalasr commented Feb 17, 2020 •

edited

Loading

dmaicher commented Feb 17, 2020

fabpot commented Feb 17, 2020

jmalinens commented Feb 19, 2020

dmaicher commented Feb 19, 2020

oleg-andreyev commented Feb 19, 2020

jmalinens commented Feb 19, 2020

[Security] Fix user role restriction. #35533

[Security] Fix user role restriction. #35533

Conversation

gorshkov-ag commented Jan 31, 2020

nicolas-grekas commented Jan 31, 2020

xabbuh commented Jan 31, 2020

gorshkov-ag commented Feb 3, 2020

gorshkov-ag commented Feb 3, 2020

nicolas-grekas commented Feb 4, 2020

oleg-andreyev Feb 5, 2020

Choose a reason for hiding this comment

gorshkov-ag Feb 13, 2020

Choose a reason for hiding this comment

gorshkov-ag Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

oleg-andreyev Feb 5, 2020

Choose a reason for hiding this comment

oleg-andreyev Feb 5, 2020

Choose a reason for hiding this comment

gorshkov-ag Feb 13, 2020

Choose a reason for hiding this comment

chalasr commented Feb 15, 2020 • edited Loading

gorshkov-ag commented Feb 17, 2020

chalasr commented Feb 17, 2020 • edited Loading

dmaicher commented Feb 17, 2020

fabpot commented Feb 17, 2020

jmalinens commented Feb 19, 2020

dmaicher commented Feb 19, 2020

oleg-andreyev commented Feb 19, 2020

jmalinens commented Feb 19, 2020

gorshkov-ag Feb 13, 2020 •

edited

Loading

chalasr commented Feb 15, 2020 •

edited

Loading

chalasr commented Feb 17, 2020 •

edited

Loading