Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[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.

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

Closed
wants to merge 5 commits into from
Closed

[Security] Fix user role restriction. #35533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4cf925b
Allowed to restrict user roles.
gorshkov-ag Jan 31, 2020
2478205
Update user role restriction.
gorshkov-ag Feb 3, 2020
4f84a99
Add test cases to user role restriction.
gorshkov-ag Feb 3, 2020
429dee7
Update user roles comparison.
gorshkov-ag Feb 13, 2020
9de09f0
Update user role restriction test cases.
gorshkov-ag Feb 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion 2 src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))) {
Copy link
Contributor

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

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));

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@gorshkov-ag gorshkov-ag Feb 13, 2020 •
edited
Loading

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

return true;
}

Expand Down
82 changes: 82 additions & 0 deletions 82 src/Symfony/Component/Security/Core/Tests/Authentication/Token/SwitchUserTokenTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,4 +73,86 @@ public function getSalt()
$token->setUser($impersonated);
$this->assertTrue($token->isAuthenticated());
}

public function testRestrictUserRolesDoesNotDeauthenticate()
{
$impersonator = new class() implements UserInterface {
Copy link
Contributor

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

public function getUsername()
{
return 'impersonator';
}

public function getPassword()
{
return null;
}

public function eraseCredentials()
{
}

public function getRoles()
{
return ['ROLE_USER', 'ROLE_ADMIN', 'ROLE_ALLOWED_TO_SWITCH'];
}

public function getSalt()
{
return null;
}
};

$originalToken = new UsernamePasswordToken($impersonator, 'foo', 'provider-key', ['ROLE_USER', 'ROLE_ADMIN', 'ROLE_ALLOWED_TO_SWITCH']);

$token = new SwitchUserToken($impersonator, 'foo', 'provider-key', ['ROLE_USER', 'ROLE_ADMIN', 'ROLE_ALLOWED_TO_SWITCH', 'ROLE_PREVIOUS_ADMIN'], $originalToken);
$token->setUser($impersonator);
$this->assertTrue($token->isAuthenticated());

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

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

public function testAddUserRolesDeauthenticate()
{
$impersonator = new class() implements UserInterface {
public function getUsername()
{
return 'impersonator';
}

public function getPassword()
{
return null;
}

public function eraseCredentials()
{
}

public function getRoles()
{
return ['ROLE_USER', 'ROLE_ADMIN', 'ROLE_ALLOWED_TO_SWITCH'];
}

public function getSalt()
{
return null;
}
};

$originalToken = new UsernamePasswordToken($impersonator, 'foo', 'provider-key', ['ROLE_USER', 'ROLE_ADMIN', 'ROLE_ALLOWED_TO_SWITCH']);

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

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 name testSetUserDoesNotDeauthenticate

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


$token = new SwitchUserToken($impersonator, 'foo', 'provider-key', ['ROLE_USER', 'ROLE_TEST', 'ROLE_PREVIOUS_ADMIN', 'ROLE_ALLOWED_TO_SWITCH'], $originalToken);
$token->setUser($impersonator);
$this->assertFalse($token->isAuthenticated());
}
}
4 changes: 2 additions & 2 deletions 4 src/Symfony/Component/Security/Core/User/User.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,8 @@ public function isEqualTo(UserInterface $user): bool

$currentRoles = array_map('strval', (array) $this->getRoles());
$newRoles = array_map('strval', (array) $user->getRoles());
$rolesChanged = \count($currentRoles) !== \count($newRoles) || \count($currentRoles) !== \count(array_intersect($currentRoles, $newRoles));
if ($rolesChanged) {

if (\count($newRoles) !== \count(array_intersect($newRoles, $currentRoles))) {
return false;
}

Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.