From e7a5803e2ed6ade6d39d83f0b768aa2d67783eea Mon Sep 17 00:00:00 2001 From: Robin Chalas Date: Thu, 28 Sep 2017 13:56:55 +0200 Subject: [PATCH] [Security] Add user impersonation support for stateless authentication --- UPGRADE-3.4.md | 3 +++ UPGRADE-4.0.md | 2 ++ .../Bundle/SecurityBundle/CHANGELOG.md | 1 + .../DependencyInjection/MainConfiguration.php | 1 + .../DependencyInjection/SecurityExtension.php | 10 +++++-- .../Resources/config/security_listeners.xml | 1 + .../CompleteConfigurationTest.php | 2 ++ .../Fixtures/php/container1.php | 2 +- .../Fixtures/php/container1_with_acl.php | 2 +- .../Fixtures/php/container1_with_digest.php | 2 +- .../Fixtures/php/no_custom_user_checker.php | 2 +- .../Fixtures/xml/container1.xml | 2 +- .../Fixtures/xml/container1_with_acl.xml | 2 +- .../Fixtures/xml/container1_with_digest.xml | 2 +- .../Fixtures/xml/no_custom_user_checker.xml | 2 +- .../Fixtures/yml/container1.yml | 3 ++- .../Fixtures/yml/container1_with_acl.yml | 3 ++- .../Fixtures/yml/container1_with_digest.yml | 3 ++- .../Fixtures/yml/no_custom_user_checker.yml | 3 ++- .../SecurityExtensionTest.php | 26 +++++++++++++++++++ .../Tests/Functional/SwitchUserTest.php | 13 ++++++++++ .../app/JsonLogin/switchuser_stateless.yml | 13 ++++++++++ .../Http/Firewall/SwitchUserListener.php | 15 ++++++----- .../Tests/Firewall/SwitchUserListenerTest.php | 25 ++++++++++++++++++ 24 files changed, 120 insertions(+), 20 deletions(-) create mode 100644 src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/JsonLogin/switchuser_stateless.yml diff --git a/UPGRADE-3.4.md b/UPGRADE-3.4.md index 6561c15a0b..09e0a8a002 100644 --- a/UPGRADE-3.4.md +++ b/UPGRADE-3.4.md @@ -316,6 +316,9 @@ SecurityBundle * Deprecated the HTTP digest authentication: `HttpDigestFactory` will be removed in 4.0. Use another authentication system like `http_basic` instead. + + * Deprecated setting the `switch_user.stateless` option to false when the firewall is `stateless`. + Setting it to false will have no effect in 4.0. Translation ----------- diff --git a/UPGRADE-4.0.md b/UPGRADE-4.0.md index 688a309466..9631609958 100644 --- a/UPGRADE-4.0.md +++ b/UPGRADE-4.0.md @@ -693,6 +693,8 @@ SecurityBundle * Removed the HTTP digest authentication system. The `HttpDigestFactory` class has been removed. Use another authentication system like `http_basic` instead. + + * The `switch_user.stateless` option is now always true if the firewall is stateless. Serializer ---------- diff --git a/src/Symfony/Bundle/SecurityBundle/CHANGELOG.md b/src/Symfony/Bundle/SecurityBundle/CHANGELOG.md index 4d5a1b8f86..12c8fa8a0c 100644 --- a/src/Symfony/Bundle/SecurityBundle/CHANGELOG.md +++ b/src/Symfony/Bundle/SecurityBundle/CHANGELOG.md @@ -17,6 +17,7 @@ CHANGELOG * deprecated command `acl:set` along with `SetAclCommand` class * deprecated command `init:acl` along with `InitAclCommand` class * Added support for the new Argon2i password encoder + * added `stateless` option to the `switch_user` listener 3.3.0 ----- diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php index 533b52cd4b..e3c3fcef12 100644 --- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php +++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php @@ -304,6 +304,7 @@ class MainConfiguration implements ConfigurationInterface ->scalarNode('provider')->end() ->scalarNode('parameter')->defaultValue('_switch_user')->end() ->scalarNode('role')->defaultValue('ROLE_ALLOWED_TO_SWITCH')->end() + ->booleanNode('stateless')->defaultValue(false)->end() ->end() ->end() ; diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php index 45ab00ac47..f0e9abf0a7 100644 --- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php +++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php @@ -456,7 +456,7 @@ class SecurityExtension extends Extension // Switch user listener if (isset($firewall['switch_user'])) { $listenerKeys[] = 'switch_user'; - $listeners[] = new Reference($this->createSwitchUserListener($container, $id, $firewall['switch_user'], $defaultProvider)); + $listeners[] = new Reference($this->createSwitchUserListener($container, $id, $firewall['switch_user'], $defaultProvider, $firewall['stateless'])); } // Access listener @@ -699,10 +699,15 @@ class SecurityExtension extends Extension return $exceptionListenerId; } - private function createSwitchUserListener($container, $id, $config, $defaultProvider) + private function createSwitchUserListener($container, $id, $config, $defaultProvider, $stateless) { $userProvider = isset($config['provider']) ? $this->getUserProviderId($config['provider']) : $defaultProvider; + // in 4.0, ignore the `switch_user.stateless` key if $stateless is `true` + if ($stateless && false === $config['stateless']) { + @trigger_error(sprintf('Firewall "%s" is configured as "stateless" but the "switch_user.stateless" key is set to false. Both should have the same value, the firewall\'s "stateless" value will be used as default value for the "switch_user.stateless" key in 4.0.', $id), E_USER_DEPRECATED); + } + $switchUserListenerId = 'security.authentication.switchuser_listener.'.$id; $listener = $container->setDefinition($switchUserListenerId, new ChildDefinition('security.authentication.switchuser_listener')); $listener->replaceArgument(1, new Reference($userProvider)); @@ -710,6 +715,7 @@ class SecurityExtension extends Extension $listener->replaceArgument(3, $id); $listener->replaceArgument(6, $config['parameter']); $listener->replaceArgument(7, $config['role']); + $listener->replaceArgument(9, $config['stateless']); return $switchUserListenerId; } diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml b/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml index ab9a587ad7..73c3eed722 100644 --- a/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml +++ b/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml @@ -241,6 +241,7 @@ _switch_user ROLE_ALLOWED_TO_SWITCH + false diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php index cb58104844..ad87bcb112 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php +++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php @@ -130,6 +130,7 @@ abstract class CompleteConfigurationTest extends TestCase array( 'parameter' => '_switch_user', 'role' => 'ROLE_ALLOWED_TO_SWITCH', + 'stateless' => true, ), ), array( @@ -256,6 +257,7 @@ abstract class CompleteConfigurationTest extends TestCase array( 'parameter' => '_switch_user', 'role' => 'ROLE_ALLOWED_TO_SWITCH', + 'stateless' => true, ), ), array( diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/php/container1.php b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/php/container1.php index a80f880f80..1395aa5db4 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/php/container1.php +++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/php/container1.php @@ -65,7 +65,7 @@ $container->loadFromExtension('security', array( 'http_basic' => true, 'form_login' => true, 'anonymous' => true, - 'switch_user' => true, + 'switch_user' => array('stateless' => true), 'x509' => true, 'remote_user' => true, 'logout' => true, diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/php/container1_with_acl.php b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/php/container1_with_acl.php index fc9b07c4f1..f6c1561014 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/php/container1_with_acl.php +++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/php/container1_with_acl.php @@ -67,7 +67,7 @@ $container->loadFromExtension('security', array( 'http_digest' => array('secret' => 'TheSecret'), 'form_login' => true, 'anonymous' => true, - 'switch_user' => true, + 'switch_user' => array('stateless' => true), 'x509' => true, 'remote_user' => true, 'logout' => true, diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/php/container1_with_digest.php b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/php/container1_with_digest.php index 581407fcc0..d35a5743b6 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/php/container1_with_digest.php +++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/php/container1_with_digest.php @@ -67,7 +67,7 @@ $container->loadFromExtension('security', array( 'http_digest' => array('secret' => 'TheSecret'), 'form_login' => true, 'anonymous' => true, - 'switch_user' => true, + 'switch_user' => array('stateless' => true), 'x509' => true, 'remote_user' => true, 'logout' => true, diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/php/no_custom_user_checker.php b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/php/no_custom_user_checker.php index 3889752f8f..2724be3e28 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/php/no_custom_user_checker.php +++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/php/no_custom_user_checker.php @@ -17,7 +17,7 @@ $container->loadFromExtension('security', array( 'http_basic' => true, 'form_login' => true, 'anonymous' => true, - 'switch_user' => true, + 'switch_user' => array('stateless' => true), 'x509' => true, 'remote_user' => true, 'logout' => true, diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/xml/container1.xml b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/xml/container1.xml index 01a5940d8c..ac85132ffe 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/xml/container1.xml +++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/xml/container1.xml @@ -49,7 +49,7 @@ - + diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/xml/container1_with_acl.xml b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/xml/container1_with_acl.xml index 6d43fcdc4f..30b24f8d8e 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/xml/container1_with_acl.xml +++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/xml/container1_with_acl.xml @@ -51,7 +51,7 @@ - + diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/xml/container1_with_digest.xml b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/xml/container1_with_digest.xml index e5049f2033..b0af7529ca 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/xml/container1_with_digest.xml +++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/xml/container1_with_digest.xml @@ -52,7 +52,7 @@ - + diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/xml/no_custom_user_checker.xml b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/xml/no_custom_user_checker.xml index b97d39adb5..996c8a7de2 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/xml/no_custom_user_checker.xml +++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/xml/no_custom_user_checker.xml @@ -17,7 +17,7 @@ - + diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/yml/container1.yml b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/yml/container1.yml index d9489abca1..48a572ba71 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/yml/container1.yml +++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/yml/container1.yml @@ -47,7 +47,8 @@ security: http_basic: true form_login: true anonymous: true - switch_user: true + switch_user: + stateless: true x509: true remote_user: true logout: true diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/yml/container1_with_acl.yml b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/yml/container1_with_acl.yml index e8ed61ef03..9a3d902bb5 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/yml/container1_with_acl.yml +++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/yml/container1_with_acl.yml @@ -50,7 +50,8 @@ security: secret: TheSecret form_login: true anonymous: true - switch_user: true + switch_user: + stateless: true x509: true remote_user: true logout: true diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/yml/container1_with_digest.yml b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/yml/container1_with_digest.yml index a2b57201bf..77a4f9b0a3 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/yml/container1_with_digest.yml +++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/yml/container1_with_digest.yml @@ -50,7 +50,8 @@ security: secret: TheSecret form_login: true anonymous: true - switch_user: true + switch_user: + stateless: true x509: true remote_user: true logout: true diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/yml/no_custom_user_checker.yml b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/yml/no_custom_user_checker.yml index 6a196597c5..05ee906237 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/yml/no_custom_user_checker.yml +++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Fixtures/yml/no_custom_user_checker.yml @@ -12,7 +12,8 @@ security: http_basic: true form_login: true anonymous: true - switch_user: true + switch_user: + stateless: true x509: true remote_user: true logout: true diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/SecurityExtensionTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/SecurityExtensionTest.php index 1055e4afd4..9ad7bfb3f8 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/SecurityExtensionTest.php +++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/SecurityExtensionTest.php @@ -148,6 +148,32 @@ class SecurityExtensionTest extends TestCase $container->compile(); } + /** + * @group legacy + * @expectedDeprecation Firewall "some_firewall" is configured as "stateless" but the "switch_user.stateless" key is set to false. Both should have the same value, the firewall's "stateless" value will be used as default value for the "switch_user.stateless" key in 4.0. + */ + public function testSwitchUserNotStatelessOnStatelessFirewall() + { + $container = $this->getRawContainer(); + + $container->loadFromExtension('security', array( + 'providers' => array( + 'default' => array('id' => 'foo'), + ), + + 'firewalls' => array( + 'some_firewall' => array( + 'stateless' => true, + 'http_basic' => null, + 'switch_user' => array('stateless' => false), + 'logout_on_user_change' => true, + ), + ), + )); + + $container->compile(); + } + protected function getRawContainer() { $container = new ContainerBuilder(); diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/SwitchUserTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/SwitchUserTest.php index 697829b1cb..d89c24f123 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/SwitchUserTest.php +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/SwitchUserTest.php @@ -11,6 +11,7 @@ namespace Symfony\Bundle\SecurityBundle\Tests\Functional; +use Symfony\Component\HttpFoundation\JsonResponse; use Symfony\Component\Security\Http\Firewall\SwitchUserListener; class SwitchUserTest extends WebTestCase @@ -50,6 +51,18 @@ class SwitchUserTest extends WebTestCase $this->assertEquals('user_can_switch', $client->getProfile()->getCollector('security')->getUser()); } + public function testSwitchUserStateless() + { + $client = $this->createClient(array('test_case' => 'JsonLogin', 'root_config' => 'switchuser_stateless.yml')); + $client->request('POST', '/chk', array('_switch_user' => 'dunglas'), array(), array('CONTENT_TYPE' => 'application/json'), '{"user": {"login": "user_can_switch", "password": "test"}}'); + $response = $client->getResponse(); + + $this->assertInstanceOf(JsonResponse::class, $response); + $this->assertSame(200, $response->getStatusCode()); + $this->assertSame(array('message' => 'Welcome @dunglas!'), json_decode($response->getContent(), true)); + $this->assertSame('dunglas', $client->getProfile()->getCollector('security')->getUser()); + } + public function getTestParameters() { return array( diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/JsonLogin/switchuser_stateless.yml b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/JsonLogin/switchuser_stateless.yml new file mode 100644 index 0000000000..29789a4caa --- /dev/null +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/JsonLogin/switchuser_stateless.yml @@ -0,0 +1,13 @@ +imports: + - { resource: ./config.yml } + +security: + providers: + in_memory: + memory: + users: + user_can_switch: { password: test, roles: [ROLE_USER, ROLE_ALLOWED_TO_SWITCH] } + firewalls: + main: + switch_user: + stateless: true diff --git a/src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php b/src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php index 2fc06140cf..d659ffc258 100644 --- a/src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php +++ b/src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php @@ -49,8 +49,9 @@ class SwitchUserListener implements ListenerInterface private $role; private $logger; private $dispatcher; + private $stateless; - public function __construct(TokenStorageInterface $tokenStorage, UserProviderInterface $provider, UserCheckerInterface $userChecker, $providerKey, AccessDecisionManagerInterface $accessDecisionManager, LoggerInterface $logger = null, $usernameParameter = '_switch_user', $role = 'ROLE_ALLOWED_TO_SWITCH', EventDispatcherInterface $dispatcher = null) + public function __construct(TokenStorageInterface $tokenStorage, UserProviderInterface $provider, UserCheckerInterface $userChecker, $providerKey, AccessDecisionManagerInterface $accessDecisionManager, LoggerInterface $logger = null, $usernameParameter = '_switch_user', $role = 'ROLE_ALLOWED_TO_SWITCH', EventDispatcherInterface $dispatcher = null, $stateless = false) { if (empty($providerKey)) { throw new \InvalidArgumentException('$providerKey must not be empty.'); @@ -65,6 +66,7 @@ class SwitchUserListener implements ListenerInterface $this->role = $role; $this->logger = $logger; $this->dispatcher = $dispatcher; + $this->stateless = $stateless; } /** @@ -92,12 +94,13 @@ class SwitchUserListener implements ListenerInterface } } - $request->query->remove($this->usernameParameter); - $request->server->set('QUERY_STRING', http_build_query($request->query->all())); + if (!$this->stateless) { + $request->query->remove($this->usernameParameter); + $request->server->set('QUERY_STRING', http_build_query($request->query->all())); + $response = new RedirectResponse($request->getUri(), 302); - $response = new RedirectResponse($request->getUri(), 302); - - $event->setResponse($response); + $event->setResponse($response); + } } /** diff --git a/src/Symfony/Component/Security/Http/Tests/Firewall/SwitchUserListenerTest.php b/src/Symfony/Component/Security/Http/Tests/Firewall/SwitchUserListenerTest.php index c97eda7593..0e61ee208e 100644 --- a/src/Symfony/Component/Security/Http/Tests/Firewall/SwitchUserListenerTest.php +++ b/src/Symfony/Component/Security/Http/Tests/Firewall/SwitchUserListenerTest.php @@ -266,4 +266,29 @@ class SwitchUserListenerTest extends TestCase $this->assertSame($replacedToken, $this->tokenStorage->getToken()); } + + public function testSwitchUserStateless() + { + $token = new UsernamePasswordToken('username', '', 'key', array('ROLE_FOO')); + $user = new User('username', 'password', array()); + + $this->tokenStorage->setToken($token); + $this->request->query->set('_switch_user', 'kuba'); + + $this->accessDecisionManager->expects($this->once()) + ->method('decide')->with($token, array('ROLE_ALLOWED_TO_SWITCH')) + ->will($this->returnValue(true)); + + $this->userProvider->expects($this->once()) + ->method('loadUserByUsername')->with('kuba') + ->will($this->returnValue($user)); + $this->userChecker->expects($this->once()) + ->method('checkPostAuth')->with($user); + + $listener = new SwitchUserListener($this->tokenStorage, $this->userProvider, $this->userChecker, 'provider123', $this->accessDecisionManager, null, '_switch_user', 'ROLE_ALLOWED_TO_SWITCH', null, true); + $listener->handle($this->event); + + $this->assertInstanceOf('Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken', $this->tokenStorage->getToken()); + $this->assertFalse($this->event->hasResponse()); + } }