[Security] Allow switching to another user when already switched

symfony · Feb 23, 2020 · ee219ef · ee219ef
1 parent b3b368b
commit ee219ef
Show file tree

Hide file tree

Showing 13 changed files with 109 additions and 11 deletions.
diff --git a/src/Symfony/Bundle/SecurityBundle/CHANGELOG.md b/src/Symfony/Bundle/SecurityBundle/CHANGELOG.md
@@ -5,6 +5,7 @@ CHANGELOG
 -----
 
  * Added security configuration for priority-based access decision strategy
+ * Added `switch_user.allow_already_switched` option to allow switching seamlessly when already switched (default  `false`)
 
 5.0.0
 -----

diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php
@@ -238,6 +238,7 @@ private function addFirewallsSection(ArrayNodeDefinition $rootNode, array $facto
                     ->scalarNode('provider')->end()
                     ->scalarNode('parameter')->defaultValue('_switch_user')->end()
                     ->scalarNode('role')->defaultValue('ROLE_ALLOWED_TO_SWITCH')->end()
+                    ->booleanNode('allow_already_switched')->defaultFalse()->end()
                 ->end()
             ->end()
         ;

diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php
@@ -684,6 +684,10 @@ private function createSwitchUserListener(ContainerBuilder $container, string $i
             throw new InvalidConfigurationException(sprintf('Not configuring explicitly the provider for the "switch_user" listener on "%s" firewall is ambiguous as there is more than one registered provider.', $id));
         }
 
+        if ($stateless && $config['allow_already_switched']) {
+            throw new InvalidConfigurationException(sprintf('Cannot set "allow_already_switched" to true for the "switch_user" listener on firewall "%s" as it is stateless.', $id));
+        }
+
         $switchUserListenerId = 'security.authentication.switchuser_listener.'.$id;
         $listener = $container->setDefinition($switchUserListenerId, new ChildDefinition('security.authentication.switchuser_listener'));
         $listener->replaceArgument(1, new Reference($userProvider));
@@ -692,6 +696,7 @@ private function createSwitchUserListener(ContainerBuilder $container, string $i
         $listener->replaceArgument(6, $config['parameter']);
         $listener->replaceArgument(7, $config['role']);
         $listener->replaceArgument(9, $stateless);
+        $listener->replaceArgument(10, $config['allow_already_switched']);
 
         return $switchUserListenerId;
     }

diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml b/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml
@@ -202,6 +202,7 @@
             <argument>ROLE_ALLOWED_TO_SWITCH</argument>
             <argument type="service" id="event_dispatcher" on-invalid="null"/>
             <argument>false</argument> <!-- Stateless -->
+            <argument>false</argument> <!-- Consecutive Switching -->
         </service>
 
         <service id="security.access_listener" class="Symfony\Component\Security\Http\Firewall\AccessListener">

diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php
@@ -111,6 +111,7 @@ public function testFirewalls()
                 [
                     'parameter' => '_switch_user',
                     'role' => 'ROLE_ALLOWED_TO_SWITCH',
+                    'allow_already_switched' => false,
                 ],
             ],
             [

diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/SwitchUserTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/SwitchUserTest.php
@@ -12,6 +12,7 @@
 namespace Symfony\Bundle\SecurityBundle\Tests\Functional;
 
 use Symfony\Component\HttpFoundation\JsonResponse;
+use Symfony\Component\Security\Core\Exception\AlreadySwitchedException;
 use Symfony\Component\Security\Http\Firewall\SwitchUserListener;
 
 class SwitchUserTest extends AbstractWebTestCase
@@ -36,8 +37,20 @@ public function testSwitchedUserCannotSwitchToOther()
         $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->assertSame(403, $client->getResponse()->getStatusCode());
+        $this->assertSame(AlreadySwitchedException::class, $client->getProfile()->getCollector('exception')->getException()->getPrevious()->getPrevious()->getClass());
+        $this->assertSame('user_cannot_switch_1', $client->getProfile()->getCollector('security')->getUser());
+    }
+
+    public function testAlreadySwitchedUserCanSwitch()
+    {
+        $client = $this->createAuthenticatedClient('user_can_switch', 'switchuser_already_switched.yml');
+
+        $client->request('GET', '/profile?_switch_user=user_cannot_switch_1');
+        $client->request('GET', '/profile?_switch_user=user_cannot_switch_2');
+
+        $this->assertSame(200, $client->getResponse()->getStatusCode());
+        $this->assertSame('user_cannot_switch_2', $client->getProfile()->getCollector('security')->getUser());
     }
 
     public function testSwitchedUserExit()
@@ -73,9 +86,9 @@ public function getTestParameters()
         ];
     }
 
-    protected function createAuthenticatedClient($username)
+    protected function createAuthenticatedClient($username, string $rootConfig = 'switchuser.yml')
     {
-        $client = $this->createClient(['test_case' => 'StandardFormLogin', 'root_config' => 'switchuser.yml']);
+        $client = $this->createClient(['test_case' => 'StandardFormLogin', 'root_config' => $rootConfig]);
         $client->followRedirects(true);
 
         $form = $client->request('GET', '/login')->selectButton('login')->form();

diff --git a/...dle/SecurityBundle/Tests/Functional/app/StandardFormLogin/switchuser_already_switched.yml b/...dle/SecurityBundle/Tests/Functional/app/StandardFormLogin/switchuser_already_switched.yml
@@ -0,0 +1,14 @@
+imports:
+    - { resource: ./config.yml }
+
+security:
+    providers:
+        in_memory:
+            memory:
+                users:
+                    user_can_switch:      { password: test, roles: [ROLE_USER, ROLE_ALLOWED_TO_SWITCH] }
+                    user_cannot_switch_1: { password: test, roles: [ROLE_USER] }
+                    user_cannot_switch_2: { password: test, roles: [ROLE_USER] }
+    firewalls:
+        default:
+            switch_user: { allow_already_switched: true }
diff --git a/src/Symfony/Bundle/SecurityBundle/composer.json b/src/Symfony/Bundle/SecurityBundle/composer.json
@@ -21,7 +21,7 @@
         "symfony/config": "^4.4|^5.0",
         "symfony/dependency-injection": "^4.4|^5.0",
         "symfony/http-kernel": "^5.0",
-        "symfony/security-core": "^4.4|^5.0",
+        "symfony/security-core": "^5.1",
         "symfony/security-csrf": "^4.4|^5.0",
         "symfony/security-guard": "^4.4|^5.0",
         "symfony/security-http": "^5.1"

diff --git a/src/Symfony/Component/Security/CHANGELOG.md b/src/Symfony/Component/Security/CHANGELOG.md
@@ -5,6 +5,7 @@ CHANGELOG
 -----
 
  * Added access decision strategy to override access decisions by voter service priority
+ * Added `bool $allowAlreadySwitched` argument to the `SwitchUserListener` constructor (default `false`)
 
 5.0.0
 -----

diff --git a/src/Symfony/Component/Security/Core/Exception/AlreadySwitchedException.php b/src/Symfony/Component/Security/Core/Exception/AlreadySwitchedException.php
@@ -0,0 +1,21 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Security\Core\Exception;
+
+/**
+ * Thrown when trying to switch to another user while being already switched.
+ *
+ * @author Robin Chalas <robin.chalas@gmail.com>
+ */
+final class AlreadySwitchedException extends AuthenticationException
+{
+}
diff --git a/src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php b/src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php
@@ -20,6 +20,7 @@
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface;
 use Symfony\Component\Security\Core\Exception\AccessDeniedException;
+use Symfony\Component\Security\Core\Exception\AlreadySwitchedException;
 use Symfony\Component\Security\Core\Exception\AuthenticationCredentialsNotFoundException;
 use Symfony\Component\Security\Core\Exception\AuthenticationException;
 use Symfony\Component\Security\Core\User\UserCheckerInterface;
@@ -51,8 +52,9 @@ class SwitchUserListener extends AbstractListener
     private $logger;
     private $dispatcher;
     private $stateless;
+    private $allowAlreadySwitched;
 
-    public function __construct(TokenStorageInterface $tokenStorage, UserProviderInterface $provider, UserCheckerInterface $userChecker, string $providerKey, AccessDecisionManagerInterface $accessDecisionManager, LoggerInterface $logger = null, string $usernameParameter = '_switch_user', string $role = 'ROLE_ALLOWED_TO_SWITCH', EventDispatcherInterface $dispatcher = null, bool $stateless = false)
+    public function __construct(TokenStorageInterface $tokenStorage, UserProviderInterface $provider, UserCheckerInterface $userChecker, string $providerKey, AccessDecisionManagerInterface $accessDecisionManager, LoggerInterface $logger = null, string $usernameParameter = '_switch_user', string $role = 'ROLE_ALLOWED_TO_SWITCH', EventDispatcherInterface $dispatcher = null, bool $stateless = false, bool $allowAlreadySwitched = false)
     {
         if (empty($providerKey)) {
             throw new \InvalidArgumentException('$providerKey must not be empty.');
@@ -68,6 +70,7 @@ public function __construct(TokenStorageInterface $tokenStorage, UserProviderInt
         $this->logger = $logger;
         $this->dispatcher = $dispatcher;
         $this->stateless = $stateless;
+        $this->allowAlreadySwitched = $allowAlreadySwitched;
     }
 
     /**
@@ -94,8 +97,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 +132,7 @@ public function authenticate(RequestEvent $event)
     /**
      * Attempts to switch to another user and returns the new token if successfully switched.
      *
-     * @throws \LogicException
+     * @throws AlreadySwitchedException
      * @throws AccessDeniedException
      */
     private function attemptSwitchUser(Request $request, string $username): ?TokenInterface
@@ -142,9 +143,15 @@ private function attemptSwitchUser(Request $request, string $username): ?TokenIn
         if (null !== $originalToken) {
             if ($token->getUsername() === $username) {
                 return $token;
+            } elseif (!$this->allowAlreadySwitched) {
+                $e = new AlreadySwitchedException(sprintf('You are already switched to "%s" user.', $token->getUsername()));
+                $e->setToken($token);
+
+                throw $e;
             }
 
-            throw new \LogicException(sprintf('You are already switched to "%s" user.', $token->getUsername()));
+            // Seamlessly exit from already switched 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
@@ -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']);

diff --git a/src/Symfony/Component/Security/Http/composer.json b/src/Symfony/Component/Security/Http/composer.json
@@ -17,7 +17,7 @@
     ],
     "require": {
         "php": "^7.2.5",
-        "symfony/security-core": "^4.4|^5.0",
+        "symfony/security-core": "^5.1",
         "symfony/http-foundation": "^4.4|^5.0",
         "symfony/http-kernel": "^4.4|^5.0",
         "symfony/property-access": "^4.4|^5.0"