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

[Security] Fix user role restriction. #35533

Open
wants to merge 5 commits into
base: master
from

Conversation

@gorshkov-ag
Copy link

gorshkov-ag commented Jan 31, 2020

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.
@gorshkov-ag gorshkov-ag requested review from dunglas, lyrixx, sroze and xabbuh as code owners Jan 31, 2020
@nicolas-grekas nicolas-grekas added this to the 4.4 milestone Jan 31, 2020
@nicolas-grekas nicolas-grekas changed the base branch from 4.4 to master Jan 31, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 31, 2020

Can you add a test case please?

@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Jan 31, 2020

It was impossible to restrict user roles on the fly

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

@gorshkov-ag

This comment has been minimized.

Copy link
Author

gorshkov-ag commented Feb 3, 2020

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

This comment has been minimized.

Copy link
Author

gorshkov-ag commented Feb 3, 2020

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.

@gorshkov-ag gorshkov-ag closed this Feb 3, 2020
@gorshkov-ag gorshkov-ag reopened this Feb 3, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 4, 2020

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

@@ -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))) {

This comment has been minimized.

Copy link
@oleg-andreyev

oleg-andreyev Feb 5, 2020

Contributor

Can you please update also

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

This comment has been minimized.

Copy link
@gorshkov-ag

This comment has been minimized.

Copy link
@gorshkov-ag

gorshkov-ag Feb 13, 2020

Author

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

@@ -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 {

This comment has been minimized.

Copy link
@oleg-andreyev

oleg-andreyev Feb 5, 2020

Contributor

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


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

This comment has been minimized.

Copy link
@oleg-andreyev

oleg-andreyev Feb 5, 2020

Contributor

assertFalse contradicts with the unit name testSetUserDoesNotDeauthenticate

This comment has been minimized.

Copy link
@gorshkov-ag
@chalasr

This comment has been minimized.

Copy link
Member

chalasr commented Feb 15, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.