diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/SecuredPageBundle/Controller/AdminController.php b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/SecuredPageBundle/Controller/AdminController.php new file mode 100644 index 0000000000..db494ed936 --- /dev/null +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/SecuredPageBundle/Controller/AdminController.php @@ -0,0 +1,17 @@ +users[$user->getUsername()] = $user; + } + + public function setUser($username, UserInterface $user) + { + $this->users[$username] = $user; + } + + public function getUser($username) + { + return $this->users[$username]; + } + + public function loadUserByUsername($username) + { + $user = $this->getUser($username); + + if (null === $user) { + throw new UsernameNotFoundException(sprintf('User "%s" not found.', $username)); + } + + return $user; + } + + public function refreshUser(UserInterface $user) + { + if (!$user instanceof UserInterface) { + throw new UnsupportedUserException(sprintf('Instances of "%s" are not supported.', \get_class($user))); + } + + $storedUser = $this->getUser($user->getUsername()); + $class = \get_class($storedUser); + + return new $class($storedUser->getUsername(), $storedUser->getPassword(), $storedUser->getRoles(), $storedUser->isEnabled(), $storedUser->isAccountNonExpired(), $storedUser->isCredentialsNonExpired() && $storedUser->getPassword() === $user->getPassword(), $storedUser->isAccountNonLocked()); + } + + public function supportsClass($class) + { + return 'Symfony\Component\Security\Core\User\User' === $class; + } +} diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/SecurityTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/SecurityTest.php index eb83e75d8c..ab39d637c3 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/SecurityTest.php +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/SecurityTest.php @@ -11,8 +11,10 @@ namespace Symfony\Bundle\SecurityBundle\Tests\Functional; +use Symfony\Bundle\SecurityBundle\Tests\Functional\Bundle\SecuredPageBundle\Security\Core\User\ArrayUserProvider; use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken; use Symfony\Component\Security\Core\User\User; +use Symfony\Component\Security\Core\User\UserInterface; class SecurityTest extends AbstractWebTestCase { @@ -31,4 +33,148 @@ class SecurityTest extends AbstractWebTestCase $this->assertTrue($security->isGranted('ROLE_USER')); $this->assertSame($token, $security->getToken()); } + + public function userWillBeMarkedAsChangedIfRolesHasChangedProvider() + { + return [ + [ + new User('user1', 'test', ['ROLE_ADMIN']), + new User('user1', 'test', ['ROLE_USER']), + ], + [ + new UserWithoutEquatable('user1', 'test', ['ROLE_ADMIN']), + new UserWithoutEquatable('user1', 'test', ['ROLE_USER']), + ], + ]; + } + + /** + * @dataProvider userWillBeMarkedAsChangedIfRolesHasChangedProvider + */ + public function testUserWillBeMarkedAsChangedIfRolesHasChanged(UserInterface $userWithAdminRole, UserInterface $userWithoutAdminRole) + { + $client = $this->createClient(['test_case' => 'AbstractTokenCompareRoles', 'root_config' => 'config.yml']); + $client->disableReboot(); + + /** @var ArrayUserProvider $userProvider */ + $userProvider = static::$kernel->getContainer()->get('security.user.provider.array'); + $userProvider->addUser($userWithAdminRole); + + $client->request('POST', '/login', [ + '_username' => 'user1', + '_password' => 'test', + ]); + + // user1 has ROLE_ADMIN and can visit secure page + $client->request('GET', '/admin'); + $this->assertEquals(200, $client->getResponse()->getStatusCode()); + + // updating user provider with same user but revoked ROLE_ADMIN from user1 + $userProvider->setUser('user1', $userWithoutAdminRole); + + // user1 has lost ROLE_ADMIN and MUST be redirected away from secure page + $client->request('GET', '/admin'); + $this->assertEquals(302, $client->getResponse()->getStatusCode()); + } +} + +final class UserWithoutEquatable implements UserInterface +{ + private $username; + private $password; + private $enabled; + private $accountNonExpired; + private $credentialsNonExpired; + private $accountNonLocked; + private $roles; + + public function __construct(?string $username, ?string $password, array $roles = [], bool $enabled = true, bool $userNonExpired = true, bool $credentialsNonExpired = true, bool $userNonLocked = true) + { + if ('' === $username || null === $username) { + throw new \InvalidArgumentException('The username cannot be empty.'); + } + + $this->username = $username; + $this->password = $password; + $this->enabled = $enabled; + $this->accountNonExpired = $userNonExpired; + $this->credentialsNonExpired = $credentialsNonExpired; + $this->accountNonLocked = $userNonLocked; + $this->roles = $roles; + } + + public function __toString() + { + return $this->getUsername(); + } + + /** + * {@inheritdoc} + */ + public function getRoles() + { + return $this->roles; + } + + /** + * {@inheritdoc} + */ + public function getPassword() + { + return $this->password; + } + + /** + * {@inheritdoc} + */ + public function getSalt() + { + } + + /** + * {@inheritdoc} + */ + public function getUsername() + { + return $this->username; + } + + /** + * {@inheritdoc} + */ + public function isAccountNonExpired() + { + return $this->accountNonExpired; + } + + /** + * {@inheritdoc} + */ + public function isAccountNonLocked() + { + return $this->accountNonLocked; + } + + /** + * {@inheritdoc} + */ + public function isCredentialsNonExpired() + { + return $this->credentialsNonExpired; + } + + /** + * {@inheritdoc} + */ + public function isEnabled() + { + return $this->enabled; + } + + /** + * {@inheritdoc} + */ + public function eraseCredentials() + { + } } diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/AbstractTokenCompareRoles/bundles.php b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/AbstractTokenCompareRoles/bundles.php new file mode 100644 index 0000000000..bedfbb1bd8 --- /dev/null +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/AbstractTokenCompareRoles/bundles.php @@ -0,0 +1,20 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +use Symfony\Bundle\FrameworkBundle\FrameworkBundle; +use Symfony\Bundle\SecurityBundle\SecurityBundle; +use Symfony\Bundle\SecurityBundle\Tests\Functional\Bundle\SecuredPageBundle\SecuredPageBundle; + +return [ + new FrameworkBundle(), + new SecurityBundle(), + new SecuredPageBundle(), +]; diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/AbstractTokenCompareRoles/config.yml b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/AbstractTokenCompareRoles/config.yml new file mode 100644 index 0000000000..5c86da6252 --- /dev/null +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/AbstractTokenCompareRoles/config.yml @@ -0,0 +1,32 @@ +imports: + - { resource: ./../config/framework.yml } + +services: + _defaults: { public: true } + + security.user.provider.array: + class: Symfony\Bundle\SecurityBundle\Tests\Functional\Bundle\SecuredPageBundle\Security\Core\User\ArrayUserProvider + +security: + + encoders: + \Symfony\Component\Security\Core\User\UserInterface: plaintext + + providers: + array: + id: security.user.provider.array + + firewalls: + default: + form_login: + check_path: login + remember_me: true + require_previous_session: false + logout: ~ + anonymous: ~ + stateless: false + + access_control: + - { path: ^/admin$, roles: ROLE_ADMIN } + - { path: ^/login$, roles: IS_AUTHENTICATED_ANONYMOUSLY } + - { path: .*, roles: IS_AUTHENTICATED_FULLY } diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/AbstractTokenCompareRoles/routing.yml b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/AbstractTokenCompareRoles/routing.yml new file mode 100644 index 0000000000..988fa8b63e --- /dev/null +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/AbstractTokenCompareRoles/routing.yml @@ -0,0 +1,8 @@ +login: + path: /login + +logout: + path: /logout + +admin_bundle: + resource: '@SecuredPageBundle/Resources/config/routing.yml' diff --git a/src/Symfony/Bundle/SecurityBundle/composer.json b/src/Symfony/Bundle/SecurityBundle/composer.json index 5d7a120f23..a155930b4d 100644 --- a/src/Symfony/Bundle/SecurityBundle/composer.json +++ b/src/Symfony/Bundle/SecurityBundle/composer.json @@ -21,7 +21,7 @@ "symfony/config": "^4.2|^5.0", "symfony/dependency-injection": "^4.2|^5.0", "symfony/http-kernel": "^4.4", - "symfony/security-core": "^4.3", + "symfony/security-core": "^4.4", "symfony/security-csrf": "^4.2|^5.0", "symfony/security-guard": "^4.2|^5.0", "symfony/security-http": "^4.3" diff --git a/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php b/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php index 6c9903c780..3a71bff2b0 100644 --- a/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php +++ b/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php @@ -166,6 +166,7 @@ abstract class AbstractToken implements TokenInterface * @return string * * @final since Symfony 4.3, use __serialize() instead + * * @internal since Symfony 4.3, use __serialize() instead */ public function serialize() @@ -316,6 +317,13 @@ abstract class AbstractToken implements TokenInterface return true; } + $userRoles = array_map('strval', (array) $user->getRoles()); + $rolesChanged = \count($userRoles) !== \count($this->getRoleNames()) || \count($userRoles) !== \count(array_intersect($userRoles, $this->getRoleNames())); + + if ($rolesChanged) { + return true; + } + if ($this->user->getUsername() !== $user->getUsername()) { return true; } diff --git a/src/Symfony/Component/Security/Core/Tests/User/UserTest.php b/src/Symfony/Component/Security/Core/Tests/User/UserTest.php index c0f47b555e..7468e95244 100644 --- a/src/Symfony/Component/Security/Core/Tests/User/UserTest.php +++ b/src/Symfony/Component/Security/Core/Tests/User/UserTest.php @@ -117,8 +117,8 @@ class UserTest extends TestCase { return [ [true, new User('username', 'password'), new User('username', 'password')], - [true, new User('username', 'password', ['ROLE']), new User('username', 'password')], - [true, new User('username', 'password', ['ROLE']), new User('username', 'password', ['NO ROLE'])], + [false, new User('username', 'password', ['ROLE']), new User('username', 'password')], + [false, new User('username', 'password', ['ROLE']), new User('username', 'password', ['NO ROLE'])], [false, new User('diff', 'diff'), new User('username', 'password')], [false, new User('diff', 'diff', [], false), new User('username', 'password')], [false, new User('diff', 'diff', [], false, false), new User('username', 'password')], diff --git a/src/Symfony/Component/Security/Core/User/User.php b/src/Symfony/Component/Security/Core/User/User.php index ce41c02a1c..31196abc79 100644 --- a/src/Symfony/Component/Security/Core/User/User.php +++ b/src/Symfony/Component/Security/Core/User/User.php @@ -143,6 +143,13 @@ final class User implements UserInterface, EquatableInterface, AdvancedUserInter return false; } + $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) { + return false; + } + if ($this->getUsername() !== $user->getUsername()) { return false; }