From a17cdc33daa4f0db5b954393f64a40800730c7db Mon Sep 17 00:00:00 2001 From: Robin Chalas Date: Sun, 23 Feb 2020 15:53:58 +0100 Subject: [PATCH] [Security] Allow switching to another user when already switched --- .../Tests/Functional/SwitchUserTest.php | 6 ++-- src/Symfony/Component/Security/CHANGELOG.md | 1 + .../Http/Firewall/SwitchUserListener.php | 6 ++-- .../Tests/Firewall/SwitchUserListenerTest.php | 33 +++++++++++++++++++ 4 files changed, 39 insertions(+), 7 deletions(-) diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/SwitchUserTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/SwitchUserTest.php index 0ccacf583fe57..183b1ad8c4ef8 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/SwitchUserTest.php +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/SwitchUserTest.php @@ -29,15 +29,15 @@ public function testSwitchUser($originalUser, $targetUser, $expectedUser, $expec $this->assertEquals($expectedUser, $client->getProfile()->getCollector('security')->getUser()); } - public function testSwitchedUserCannotSwitchToOther() + public function testSwitchedUserCanSwitchToOther() { $client = $this->createAuthenticatedClient('user_can_switch'); $client->request('GET', '/profile?_switch_user=user_cannot_switch_1'); $client->request('GET', '/profile?_switch_user=user_cannot_switch_2'); - $this->assertEquals(500, $client->getResponse()->getStatusCode()); - $this->assertEquals('user_cannot_switch_1', $client->getProfile()->getCollector('security')->getUser()); + $this->assertEquals(200, $client->getResponse()->getStatusCode()); + $this->assertEquals('user_cannot_switch_2', $client->getProfile()->getCollector('security')->getUser()); } public function testSwitchedUserExit() diff --git a/src/Symfony/Component/Security/CHANGELOG.md b/src/Symfony/Component/Security/CHANGELOG.md index 4b255daf209ff..35414c68143b0 100644 --- a/src/Symfony/Component/Security/CHANGELOG.md +++ b/src/Symfony/Component/Security/CHANGELOG.md @@ -6,6 +6,7 @@ CHANGELOG * Added access decision strategy to override access decisions by voter service priority * Added `IS_ANONYMOUS`, `IS_REMEMBERED`, `IS_IMPERSONATOR` + * Added the ability to seamlessly switch to another user while being already switched 5.0.0 ----- diff --git a/src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php b/src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php index f6d4c8a587c0a..bbe94f348417a 100644 --- a/src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php +++ b/src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php @@ -94,8 +94,6 @@ public function supports(Request $request): ?bool /** * Handles the switch to another user. - * - * @throws \LogicException if switching to a user failed */ public function authenticate(RequestEvent $event) { @@ -131,7 +129,6 @@ public function authenticate(RequestEvent $event) /** * Attempts to switch to another user and returns the new token if successfully switched. * - * @throws \LogicException * @throws AccessDeniedException */ private function attemptSwitchUser(Request $request, string $username): ?TokenInterface @@ -144,7 +141,8 @@ private function attemptSwitchUser(Request $request, string $username): ?TokenIn return $token; } - throw new \LogicException(sprintf('You are already switched to "%s" user.', $token->getUsername())); + // User already switched, exit before seamlessly switching to another user + $token = $this->attemptExitUser($request); } $currentUsername = $token->getUsername(); diff --git a/src/Symfony/Component/Security/Http/Tests/Firewall/SwitchUserListenerTest.php b/src/Symfony/Component/Security/Http/Tests/Firewall/SwitchUserListenerTest.php index b0ab43efd1643..282cd441f544e 100644 --- a/src/Symfony/Component/Security/Http/Tests/Firewall/SwitchUserListenerTest.php +++ b/src/Symfony/Component/Security/Http/Tests/Firewall/SwitchUserListenerTest.php @@ -223,6 +223,39 @@ public function testSwitchUser() $this->assertInstanceOf('Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken', $this->tokenStorage->getToken()); } + public function testSwitchUserAlreadySwitched() + { + $originalToken = new UsernamePasswordToken('original', null, 'key', ['ROLE_FOO']); + $alreadySwitchedToken = new SwitchUserToken('switched_1', null, 'key', ['ROLE_BAR'], $originalToken); + + $tokenStorage = new TokenStorage(); + $tokenStorage->setToken($alreadySwitchedToken); + + $targetUser = new User('kuba', 'password', ['ROLE_FOO', 'ROLE_BAR']); + + $this->request->query->set('_switch_user', 'kuba'); + + $this->accessDecisionManager->expects($this->once()) + ->method('decide')->with($originalToken, ['ROLE_ALLOWED_TO_SWITCH'], $targetUser) + ->willReturn(true); + + $this->userProvider->expects($this->exactly(2)) + ->method('loadUserByUsername') + ->withConsecutive(['kuba']) + ->will($this->onConsecutiveCalls($targetUser, $this->throwException(new UsernameNotFoundException()))); + $this->userChecker->expects($this->once()) + ->method('checkPostAuth')->with($targetUser); + + $listener = new SwitchUserListener($tokenStorage, $this->userProvider, $this->userChecker, 'provider123', $this->accessDecisionManager, null, '_switch_user', 'ROLE_ALLOWED_TO_SWITCH', null, false, true); + $listener($this->event); + + $this->assertSame([], $this->request->query->all()); + $this->assertSame('', $this->request->server->get('QUERY_STRING')); + $this->assertInstanceOf(SwitchUserToken::class, $tokenStorage->getToken()); + $this->assertSame('kuba', $tokenStorage->getToken()->getUsername()); + $this->assertSame($originalToken, $tokenStorage->getToken()->getOriginalToken()); + } + public function testSwitchUserWorksWithFalsyUsernames() { $token = new UsernamePasswordToken('username', '', 'key', ['ROLE_FOO']);