Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security] Fix user role restriction.#35533
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
It was impossible to restrict user roles on the fly without deauthentication.
nicolas-grekas commentedJan 31, 2020
Can you add a test case please? |
xabbuh commentedJan 31, 2020
@gorshkov-ag Can you explain a bit what you mean with that? |
gorshkov-ag commentedFeb 3, 2020
I have some user roles in application and need a possibility to switch them without changing user or user roles. |
gorshkov-ag commentedFeb 3, 2020
Added test cases. Updated rule don't compare count of current user roles with count of applicable roles. |
nicolas-grekas commentedFeb 4, 2020
@oleg-andreyev could you please have a look, since this changes the logic from#31177? |
| } | ||
| if (\count($userRoles) !==\count($this->getRoleNames())||\count($userRoles)!==\count(array_intersect($userRoles,$this->getRoleNames()))) { | ||
| if (\count($this->getRoleNames()) !==\count(array_intersect($this->getRoleNames(),$userRoles))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you please update also
| $rolesChanged =\count($currentRoles) !==\count($newRoles) ||\count($currentRoles) !==\count(array_intersect($currentRoles,$newRoles)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
gorshkov-agFeb 13, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
| $token->setUser($impersonated); | ||
| $this->assertTrue($token->isAuthenticated()); | ||
| $impersonator =newclass()implements UserInterface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it's better to create a separate unit for this
| $token =newSwitchUserToken($impersonator,'foo','provider-key', ['ROLE_TEST','ROLE_PREVIOUS_ADMIN'],$originalToken); | ||
| $token->setUser($impersonator); | ||
| $this->assertFalse($token->isAuthenticated()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
assertFalse contradicts with the unit nametestSetUserDoesNotDeauthenticate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
chalasr commentedFeb 15, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The current behavior is the expected one. Removing a role must trigger deauthentication. |
gorshkov-ag commentedFeb 17, 2020
Not removing role for user, just for token to use one or some user roles, not all of them. |
chalasr commentedFeb 17, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The token roles must contain all the roles of the user it holds. 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 commentedFeb 17, 2020
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 |
fabpot commentedFeb 17, 2020
Closing as I agree with@chalasr |
jmalinens commentedFeb 19, 2020
@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 commentedFeb 19, 2020
@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 |
oleg-andreyev commentedFeb 19, 2020
@jmalinens I'm not aware of your implementation but you need to make sure that you ADD not REPLACE your ROLE_USER. Probably |
jmalinens commentedFeb 19, 2020
Thanks guys. |
It was impossible to restrict user roles on the fly without deauthentication.
Allowed to restrict current user roles without changing roles for user object.