From c6aa392df7b1b68d7f5be0b858bd93e97a5a6cdc Mon Sep 17 00:00:00 2001 From: Alexander Date: Sun, 17 Jun 2012 21:47:16 +0200 Subject: [PATCH 1/5] [Security] Move default authentication success handling strategy to seperate class [Security] Update configuration for changes regarding default success handler [Security] Fix + add AbstractFactory test --- .../Security/Factory/AbstractFactory.php | 33 ++++++-- .../Resources/config/security_listeners.xml | 9 +- .../Security/Factory/AbstractFactoryTest.php | 67 +++++++++------ .../AuthenticationSuccessHandlerInterface.php | 2 +- .../DefaultAuthenticationSuccessHandler.php | 84 +++++++++++++++++++ .../AbstractAuthenticationListener.php | 45 +--------- ...namePasswordFormAuthenticationListener.php | 6 +- 7 files changed, 164 insertions(+), 82 deletions(-) create mode 100644 src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php index 611b0285c0..e3711a9b25 100644 --- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php +++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php @@ -30,12 +30,15 @@ abstract class AbstractFactory implements SecurityFactoryInterface 'check_path' => '/login_check', 'login_path' => '/login', 'use_forward' => false, + 'failure_path' => null, + 'failure_forward' => false, + ); + + protected $defaultSuccessHandlerOptions = array( 'always_use_default_target_path' => false, 'default_target_path' => '/', 'target_path_parameter' => '_target_path', 'use_referer' => false, - 'failure_path' => null, - 'failure_forward' => false, ); public function create(ContainerBuilder $container, $id, $config, $userProviderId, $defaultEntryPointId) @@ -71,7 +74,7 @@ abstract class AbstractFactory implements SecurityFactoryInterface ->scalarNode('failure_handler')->end() ; - foreach ($this->options as $name => $default) { + foreach (array_merge($this->options, $this->defaultSuccessHandlerOptions) as $name => $default) { if (is_bool($default)) { $builder->booleanNode($name)->defaultValue($default); } else { @@ -149,12 +152,8 @@ abstract class AbstractFactory implements SecurityFactoryInterface $listenerId = $this->getListenerId(); $listener = new DefinitionDecorator($listenerId); $listener->replaceArgument(4, $id); - $listener->replaceArgument(5, array_intersect_key($config, $this->options)); - - // success handler - if (isset($config['success_handler'])) { - $listener->replaceArgument(6, new Reference($config['success_handler'])); - } + $listener->replaceArgument(5, new Reference($this->createAuthenticationSuccessHandler($container, $id, $config))); + $listener->replaceArgument(6, array_intersect_key($config, $this->options)); // failure handler if (isset($config['failure_handler'])) { @@ -166,4 +165,20 @@ abstract class AbstractFactory implements SecurityFactoryInterface return $listenerId; } + + protected function createAuthenticationSuccessHandler($container, $id, $config) + { + // success handler + if (isset($config['success_handler'])) { + + return $config['success_handler']; + } + + $id = 'security.authentication.success_handler.'.$id; + + $successHandler = $container->setDefinition($id, new DefinitionDecorator('security.authentication.success_handler')); + $successHandler->replaceArgument(1, array_intersect_key($config, $this->defaultSuccessHandlerOptions)); + + return $id; + } } diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml b/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml index b6cd02007a..f3a4f37858 100644 --- a/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml +++ b/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml @@ -37,6 +37,8 @@ Symfony\Component\Security\Core\Authentication\Provider\PreAuthenticatedAuthenticationProvider Symfony\Component\Security\Core\Authentication\Provider\AnonymousAuthenticationProvider + + Symfony\Component\Security\Http\Authentication\DefaultAuthenticationSuccessHandler @@ -98,13 +100,18 @@ + - + + + + + getFactory(); + list($container, + $authProviderId, + $listenerId, + $entryPointId) = $this->callFactory('foo', array('use_forward' => true, 'failure_path' => '/foo', 'success_handler' => 'foo', 'remember_me' => true), 'user_provider', 'entry_point'); + + // auth provider + $this->assertEquals('auth_provider', $authProviderId); + + // listener + $this->assertEquals('abstract_listener.foo', $listenerId); + $this->assertTrue($container->hasDefinition('abstract_listener.foo')); + $definition = $container->getDefinition('abstract_listener.foo'); + $this->assertEquals(array( + 'index_4' => 'foo', + 'index_5' => new Reference('foo'), + 'index_6' => array( + 'use_forward' => true, + 'failure_path' => '/foo', + ), + ), $definition->getArguments()); + + // entry point + $this->assertEquals('entry_point', $entryPointId, '->create() does not change the default entry point.'); + } + + public function testDefaultSuccessHandler() + { + list($container, + $authProviderId, + $listenerId, + $entryPointId) = $this->callFactory('foo', array('remember_me' => true), 'user_provider', 'entry_point'); + + $definition = $container->getDefinition('abstract_listener.foo'); + $arguments = $definition->getArguments(); + $this->assertEquals(new Reference('security.authentication.success_handler.foo'), $arguments['index_5']); + } + + protected function callFactory($id, $config, $userProviderId, $defaultEntryPointId) + { + $factory = $this->getMockForAbstractClass('Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\AbstractFactory', array()); $factory ->expects($this->once()) @@ -37,30 +76,8 @@ class AbstractFactoryTest extends \PHPUnit_Framework_TestCase list($authProviderId, $listenerId, $entryPointId - ) = $factory->create($container, 'foo', array('use_forward' => true, 'failure_path' => '/foo', 'success_handler' => 'foo', 'remember_me' => true), 'user_provider', 'entry_point'); + ) = $factory->create($container, $id, $config, $userProviderId, $defaultEntryPointId); - // auth provider - $this->assertEquals('auth_provider', $authProviderId); - - // listener - $this->assertEquals('abstract_listener.foo', $listenerId); - $this->assertTrue($container->hasDefinition('abstract_listener.foo')); - $definition = $container->getDefinition('abstract_listener.foo'); - $this->assertEquals(array( - 'index_4' => 'foo', - 'index_5' => array( - 'use_forward' => true, - 'failure_path' => '/foo', - ), - 'index_6' => new Reference('foo'), - ), $definition->getArguments()); - - // entry point - $this->assertEquals('entry_point', $entryPointId, '->create() does not change the default entry point.'); - } - - protected function getFactory() - { - return $this->getMockForAbstractClass('Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\AbstractFactory', array()); + return array($container, $authProviderId, $listenerId, $entryPointId); } } diff --git a/src/Symfony/Component/Security/Http/Authentication/AuthenticationSuccessHandlerInterface.php b/src/Symfony/Component/Security/Http/Authentication/AuthenticationSuccessHandlerInterface.php index 8917f3e7ea..99b1ae6c24 100644 --- a/src/Symfony/Component/Security/Http/Authentication/AuthenticationSuccessHandlerInterface.php +++ b/src/Symfony/Component/Security/Http/Authentication/AuthenticationSuccessHandlerInterface.php @@ -33,7 +33,7 @@ interface AuthenticationSuccessHandlerInterface * @param Request $request * @param TokenInterface $token * - * @return Response|null the response to return + * @return Response never null */ function onAuthenticationSuccess(Request $request, TokenInterface $token); } diff --git a/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php b/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php new file mode 100644 index 0000000000..5d414c6df2 --- /dev/null +++ b/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php @@ -0,0 +1,84 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Http\Authentication; + +use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\Security\Http\HttpUtils; + +/** + * Class with the default authentication success handling logic. + * + * Can be optionally be extended from by the developer to alter the behaviour + * while keeping the default behaviour. + * + * @author Alexander + */ +class DefaultAuthenticationSuccessHandler implements AuthenticationSuccessHandlerInterface +{ + /** + * Constructor. + * + * @param HttpUtils $httpUtils HttpUtils + * @param array $options Options for processing a successful authentication attempt. + */ + public function __construct(HttpUtils $httpUtils, array $options) + { + $this->httpUtils = $httpUtils; + + $this->options = array_merge(array( + 'always_use_default_target_path' => false, + 'default_target_path' => '/', + 'target_path_parameter' => '_target_path', + 'use_referer' => false, + ), $options); + } + + /** + * {@inheritDoc} + */ + public function onAuthenticationSuccess(Request $request, TokenInterface $token) + { + return $this->httpUtils->createRedirectResponse($request, $this->determineTargetUrl($request)); + } + + /** + * Builds the target URL according to the defined options. + * + * @param Request $request + * + * @return string + */ + protected function determineTargetUrl(Request $request) + { + if ($this->options['always_use_default_target_path']) { + return $this->options['default_target_path']; + } + + if ($targetUrl = $request->get($this->options['target_path_parameter'], null, true)) { + return $targetUrl; + } + + $session = $request->getSession(); + if ($targetUrl = $session->get('_security.target_path')) { + $session->remove('_security.target_path'); + + return $targetUrl; + } + + if ($this->options['use_referer'] && ($targetUrl = $request->headers->get('Referer')) && $targetUrl !== $request->getUriForPath($this->options['login_path'])) { + return $targetUrl; + } + + return $this->options['default_target_path']; + } +} diff --git a/src/Symfony/Component/Security/Http/Firewall/AbstractAuthenticationListener.php b/src/Symfony/Component/Security/Http/Firewall/AbstractAuthenticationListener.php index 1caaf0af0b..1e27f89af7 100644 --- a/src/Symfony/Component/Security/Http/Firewall/AbstractAuthenticationListener.php +++ b/src/Symfony/Component/Security/Http/Firewall/AbstractAuthenticationListener.php @@ -77,7 +77,7 @@ abstract class AbstractAuthenticationListener implements ListenerInterface * @param LoggerInterface $logger A LoggerInterface instance * @param EventDispatcherInterface $dispatcher An EventDispatcherInterface instance */ - public function __construct(SecurityContextInterface $securityContext, AuthenticationManagerInterface $authenticationManager, SessionAuthenticationStrategyInterface $sessionStrategy, HttpUtils $httpUtils, $providerKey, array $options = array(), AuthenticationSuccessHandlerInterface $successHandler = null, AuthenticationFailureHandlerInterface $failureHandler = null, LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null) + public function __construct(SecurityContextInterface $securityContext, AuthenticationManagerInterface $authenticationManager, SessionAuthenticationStrategyInterface $sessionStrategy, HttpUtils $httpUtils, $providerKey, AuthenticationSuccessHandlerInterface $successHandler, array $options = array(), AuthenticationFailureHandlerInterface $failureHandler = null, LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null) { if (empty($providerKey)) { throw new \InvalidArgumentException('$providerKey must not be empty.'); @@ -92,10 +92,6 @@ abstract class AbstractAuthenticationListener implements ListenerInterface $this->options = array_merge(array( 'check_path' => '/login_check', 'login_path' => '/login', - 'always_use_default_target_path' => false, - 'default_target_path' => '/', - 'target_path_parameter' => '_target_path', - 'use_referer' => false, 'failure_path' => null, 'failure_forward' => false, ), $options); @@ -238,13 +234,7 @@ abstract class AbstractAuthenticationListener implements ListenerInterface $this->dispatcher->dispatch(SecurityEvents::INTERACTIVE_LOGIN, $loginEvent); } - $response = null; - if (null !== $this->successHandler) { - $response = $this->successHandler->onAuthenticationSuccess($request, $token); - } - if (null === $response) { - $response = $this->httpUtils->createRedirectResponse($request, $this->determineTargetUrl($request)); - } + $response = $this->successHandler->onAuthenticationSuccess($request, $token); if (null !== $this->rememberMeServices) { $this->rememberMeServices->loginSuccess($request, $response, $token); @@ -252,35 +242,4 @@ abstract class AbstractAuthenticationListener implements ListenerInterface return $response; } - - /** - * Builds the target URL according to the defined options. - * - * @param Request $request - * - * @return string - */ - private function determineTargetUrl(Request $request) - { - if ($this->options['always_use_default_target_path']) { - return $this->options['default_target_path']; - } - - if ($targetUrl = $request->get($this->options['target_path_parameter'], null, true)) { - return $targetUrl; - } - - $session = $request->getSession(); - if ($targetUrl = $session->get('_security.' . $this->providerKey . '.target_path')) { - $session->remove('_security.' . $this->providerKey . '.target_path'); - - return $targetUrl; - } - - if ($this->options['use_referer'] && ($targetUrl = $request->headers->get('Referer')) && $targetUrl !== $request->getUriForPath($this->options['login_path'])) { - return $targetUrl; - } - - return $this->options['default_target_path']; - } } diff --git a/src/Symfony/Component/Security/Http/Firewall/UsernamePasswordFormAuthenticationListener.php b/src/Symfony/Component/Security/Http/Firewall/UsernamePasswordFormAuthenticationListener.php index 35b0b11c35..c0fb79eef2 100644 --- a/src/Symfony/Component/Security/Http/Firewall/UsernamePasswordFormAuthenticationListener.php +++ b/src/Symfony/Component/Security/Http/Firewall/UsernamePasswordFormAuthenticationListener.php @@ -37,15 +37,15 @@ class UsernamePasswordFormAuthenticationListener extends AbstractAuthenticationL /** * {@inheritdoc} */ - public function __construct(SecurityContextInterface $securityContext, AuthenticationManagerInterface $authenticationManager, SessionAuthenticationStrategyInterface $sessionStrategy, HttpUtils $httpUtils, $providerKey, array $options = array(), AuthenticationSuccessHandlerInterface $successHandler = null, AuthenticationFailureHandlerInterface $failureHandler = null, LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null, CsrfProviderInterface $csrfProvider = null) + public function __construct(SecurityContextInterface $securityContext, AuthenticationManagerInterface $authenticationManager, SessionAuthenticationStrategyInterface $sessionStrategy, HttpUtils $httpUtils, $providerKey, AuthenticationSuccessHandlerInterface $successHandler = null, array $options = array(), AuthenticationFailureHandlerInterface $failureHandler = null, LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null, CsrfProviderInterface $csrfProvider = null) { - parent::__construct($securityContext, $authenticationManager, $sessionStrategy, $httpUtils, $providerKey, array_merge(array( + parent::__construct($securityContext, $authenticationManager, $sessionStrategy, $httpUtils, $providerKey, $successHandler, array_merge(array( 'username_parameter' => '_username', 'password_parameter' => '_password', 'csrf_parameter' => '_csrf_token', 'intention' => 'authenticate', 'post_only' => true, - ), $options), $successHandler, $failureHandler, $logger, $dispatcher); + ), $options), $failureHandler, $logger, $dispatcher); $this->csrfProvider = $csrfProvider; } From 915704c0717c1b13f44468e51638761e71682da5 Mon Sep 17 00:00:00 2001 From: Alexander Date: Wed, 20 Jun 2012 08:50:59 +0200 Subject: [PATCH 2/5] [Security] Move default authentication failure handling strategy to seperate class [Security] Update configuration for changes regarding default failure handler [Security] Fixes + add AbstractFactory test for failure handler --- .../Security/Factory/AbstractFactory.php | 35 ++++-- .../Resources/config/security_listeners.xml | 10 +- .../Security/Factory/AbstractFactoryTest.php | 20 +++- .../DefaultAuthenticationFailureHandler.php | 100 ++++++++++++++++++ .../DefaultAuthenticationSuccessHandler.php | 11 ++ .../AbstractAuthenticationListener.php | 34 +----- ...namePasswordFormAuthenticationListener.php | 6 +- 7 files changed, 166 insertions(+), 50 deletions(-) create mode 100644 src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationFailureHandler.php diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php index e3711a9b25..99e17420eb 100644 --- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php +++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php @@ -28,19 +28,23 @@ abstract class AbstractFactory implements SecurityFactoryInterface { protected $options = array( 'check_path' => '/login_check', - 'login_path' => '/login', 'use_forward' => false, - 'failure_path' => null, - 'failure_forward' => false, ); protected $defaultSuccessHandlerOptions = array( 'always_use_default_target_path' => false, 'default_target_path' => '/', + 'login_path' => '/login', 'target_path_parameter' => '_target_path', 'use_referer' => false, ); + protected $defaultFailureHandlerOptions = array( + 'failure_path' => null, + 'failure_forward' => false, + 'login_path' => '/login', + ); + public function create(ContainerBuilder $container, $id, $config, $userProviderId, $defaultEntryPointId) { // authentication provider @@ -74,7 +78,7 @@ abstract class AbstractFactory implements SecurityFactoryInterface ->scalarNode('failure_handler')->end() ; - foreach (array_merge($this->options, $this->defaultSuccessHandlerOptions) as $name => $default) { + foreach (array_merge($this->options, $this->defaultSuccessHandlerOptions, $this->defaultFailureHandlerOptions) as $name => $default) { if (is_bool($default)) { $builder->booleanNode($name)->defaultValue($default); } else { @@ -153,12 +157,8 @@ abstract class AbstractFactory implements SecurityFactoryInterface $listener = new DefinitionDecorator($listenerId); $listener->replaceArgument(4, $id); $listener->replaceArgument(5, new Reference($this->createAuthenticationSuccessHandler($container, $id, $config))); - $listener->replaceArgument(6, array_intersect_key($config, $this->options)); - - // failure handler - if (isset($config['failure_handler'])) { - $listener->replaceArgument(7, new Reference($config['failure_handler'])); - } + $listener->replaceArgument(6, new Reference($this->createAuthenticationFailureHandler($container, $id, $config))); + $listener->replaceArgument(7, array_intersect_key($config, $this->options)); $listenerId .= '.'.$id; $container->setDefinition($listenerId, $listener); @@ -181,4 +181,19 @@ abstract class AbstractFactory implements SecurityFactoryInterface return $id; } + + protected function createAuthenticationFailureHandler($container, $id, $config) + { + if (isset($config['failure_handler'])) { + + return $config['failure_handler']; + } + + $id = 'security.authentication.failure_handler.'.$id; + + $failureHandler = $container->setDefinition($id, new DefinitionDecorator('security.authentication.failure_handler')); + $failureHandler->replaceArgument(2, array_intersect_key($config, $this->defaultFailureHandlerOptions)); + + return $id; + } } diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml b/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml index f3a4f37858..a0d01a33d1 100644 --- a/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml +++ b/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml @@ -39,6 +39,7 @@ Symfony\Component\Security\Core\Authentication\Provider\AnonymousAuthenticationProvider Symfony\Component\Security\Http\Authentication\DefaultAuthenticationSuccessHandler + Symfony\Component\Security\Http\Authentication\DefaultAuthenticationFailureHandler @@ -101,8 +102,8 @@ + - @@ -112,6 +113,13 @@ + + + + + + + callFactory('foo', array('use_forward' => true, 'failure_path' => '/foo', 'success_handler' => 'foo', 'remember_me' => true), 'user_provider', 'entry_point'); + $entryPointId) = $this->callFactory('foo', array('use_forward' => true, 'failure_path' => '/foo', 'success_handler' => 'qux', 'failure_handler' => 'bar', 'remember_me' => true), 'user_provider', 'entry_point'); // auth provider $this->assertEquals('auth_provider', $authProviderId); @@ -32,10 +32,10 @@ class AbstractFactoryTest extends \PHPUnit_Framework_TestCase $definition = $container->getDefinition('abstract_listener.foo'); $this->assertEquals(array( 'index_4' => 'foo', - 'index_5' => new Reference('foo'), - 'index_6' => array( + 'index_5' => new Reference('qux'), + 'index_6' => new Reference('bar'), + 'index_7' => array( 'use_forward' => true, - 'failure_path' => '/foo', ), ), $definition->getArguments()); @@ -43,6 +43,18 @@ class AbstractFactoryTest extends \PHPUnit_Framework_TestCase $this->assertEquals('entry_point', $entryPointId, '->create() does not change the default entry point.'); } + public function testDefaultFailureHandler() + { + list($container, + $authProviderId, + $listenerId, + $entryPointId) = $this->callFactory('foo', array('remember_me' => true), 'user_provider', 'entry_point'); + + $definition = $container->getDefinition('abstract_listener.foo'); + $arguments = $definition->getArguments(); + $this->assertEquals(new Reference('security.authentication.failure_handler.foo'), $arguments['index_6']); + } + public function testDefaultSuccessHandler() { list($container, diff --git a/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationFailureHandler.php b/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationFailureHandler.php new file mode 100644 index 0000000000..71a0057dd1 --- /dev/null +++ b/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationFailureHandler.php @@ -0,0 +1,100 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Http\Authentication; + +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpKernel\HttpKernelInterface; +use Symfony\Component\HttpKernel\Log\LoggerInterface; +use Symfony\Component\Security\Core\Exception\AuthenticationException; +use Symfony\Component\Security\Core\SecurityContextInterface; +use Symfony\Component\Security\Http\HttpUtils; + +/** + * Class with the default authentication failure handling logic. + * + * Can be optionally be extended from by the developer to alter the behaviour + * while keeping the default behaviour. + * + * @author Alexander + */ +class DefaultAuthenticationFailureHandler implements AuthenticationFailureHandlerInterface +{ + /** + * @var HttpKernel + */ + private $httpKernel; + + /** + * @var HttpUtils + */ + protected $httpUtils; + + /** + * @var LoggerInterface + */ + private $logger; + + /** + * @var array + */ + protected $options; + + /** + * Constructor. + * + * @param HttpKernelInterface $httpKernel Kernel + * @param HttpUtils $httpUtils HttpUtils + * @param array $options Options for processing a successful authentication attempt. + * @param LoggerInterface $logger Optional logger + */ + public function __construct(HttpKernelInterface $httpKernel, HttpUtils $httpUtils, array $options, LoggerInterface $logger = null) + { + $this->httpKernel = $httpKernel; + $this->httpUtils = $httpUtils; + $this->logger = $logger; + + $this->options = array_merge(array( + 'failure_path' => null, + 'failure_forward' => false, + 'login_path' => '/login', + ), $options); + } + + /** + * {@inheritDoc} + */ + public function onAuthenticationFailure(Request $request, AuthenticationException $exception) + { + if (null === $this->options['failure_path']) { + $this->options['failure_path'] = $this->options['login_path']; + } + + if ($this->options['failure_forward']) { + if (null !== $this->logger) { + $this->logger->debug(sprintf('Forwarding to %s', $this->options['failure_path'])); + } + + $subRequest = $this->httpUtils->createRequest($request, $this->options['failure_path']); + $subRequest->attributes->set(SecurityContextInterface::AUTHENTICATION_ERROR, $exception); + + return $this->httpKernel->handle($subRequest, HttpKernelInterface::SUB_REQUEST); + } + + if (null !== $this->logger) { + $this->logger->debug(sprintf('Redirecting to %s', $this->options['failure_path'])); + } + + $request->getSession()->set(SecurityContextInterface::AUTHENTICATION_ERROR, $exception); + + return $this->httpUtils->createRedirectResponse($request, $this->options['failure_path']); + } +} diff --git a/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php b/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php index 5d414c6df2..8139786369 100644 --- a/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php +++ b/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php @@ -25,6 +25,16 @@ use Symfony\Component\Security\Http\HttpUtils; */ class DefaultAuthenticationSuccessHandler implements AuthenticationSuccessHandlerInterface { + /** + * @var HttpUtils + */ + protected $httpUtils; + + /** + * @var array + */ + protected $options; + /** * Constructor. * @@ -38,6 +48,7 @@ class DefaultAuthenticationSuccessHandler implements AuthenticationSuccessHandle $this->options = array_merge(array( 'always_use_default_target_path' => false, 'default_target_path' => '/', + 'login_path' => '/login', 'target_path_parameter' => '_target_path', 'use_referer' => false, ), $options); diff --git a/src/Symfony/Component/Security/Http/Firewall/AbstractAuthenticationListener.php b/src/Symfony/Component/Security/Http/Firewall/AbstractAuthenticationListener.php index 1e27f89af7..377639c086 100644 --- a/src/Symfony/Component/Security/Http/Firewall/AbstractAuthenticationListener.php +++ b/src/Symfony/Component/Security/Http/Firewall/AbstractAuthenticationListener.php @@ -77,7 +77,7 @@ abstract class AbstractAuthenticationListener implements ListenerInterface * @param LoggerInterface $logger A LoggerInterface instance * @param EventDispatcherInterface $dispatcher An EventDispatcherInterface instance */ - public function __construct(SecurityContextInterface $securityContext, AuthenticationManagerInterface $authenticationManager, SessionAuthenticationStrategyInterface $sessionStrategy, HttpUtils $httpUtils, $providerKey, AuthenticationSuccessHandlerInterface $successHandler, array $options = array(), AuthenticationFailureHandlerInterface $failureHandler = null, LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null) + public function __construct(SecurityContextInterface $securityContext, AuthenticationManagerInterface $authenticationManager, SessionAuthenticationStrategyInterface $sessionStrategy, HttpUtils $httpUtils, $providerKey, AuthenticationSuccessHandlerInterface $successHandler, AuthenticationFailureHandlerInterface $failureHandler, array $options = array(), LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null) { if (empty($providerKey)) { throw new \InvalidArgumentException('$providerKey must not be empty.'); @@ -91,9 +91,6 @@ abstract class AbstractAuthenticationListener implements ListenerInterface $this->failureHandler = $failureHandler; $this->options = array_merge(array( 'check_path' => '/login_check', - 'login_path' => '/login', - 'failure_path' => null, - 'failure_forward' => false, ), $options); $this->logger = $logger; $this->dispatcher = $dispatcher; @@ -187,34 +184,7 @@ abstract class AbstractAuthenticationListener implements ListenerInterface $this->securityContext->setToken(null); - if (null !== $this->failureHandler) { - if (null !== $response = $this->failureHandler->onAuthenticationFailure($request, $failed)) { - return $response; - } - } - - if (null === $this->options['failure_path']) { - $this->options['failure_path'] = $this->options['login_path']; - } - - if ($this->options['failure_forward']) { - if (null !== $this->logger) { - $this->logger->debug(sprintf('Forwarding to %s', $this->options['failure_path'])); - } - - $subRequest = $this->httpUtils->createRequest($request, $this->options['failure_path']); - $subRequest->attributes->set(SecurityContextInterface::AUTHENTICATION_ERROR, $failed); - - return $event->getKernel()->handle($subRequest, HttpKernelInterface::SUB_REQUEST); - } - - if (null !== $this->logger) { - $this->logger->debug(sprintf('Redirecting to %s', $this->options['failure_path'])); - } - - $request->getSession()->set(SecurityContextInterface::AUTHENTICATION_ERROR, $failed); - - return $this->httpUtils->createRedirectResponse($request, $this->options['failure_path']); + return $this->failureHandler->onAuthenticationFailure($request, $failed); } private function onSuccess(GetResponseEvent $event, Request $request, TokenInterface $token) diff --git a/src/Symfony/Component/Security/Http/Firewall/UsernamePasswordFormAuthenticationListener.php b/src/Symfony/Component/Security/Http/Firewall/UsernamePasswordFormAuthenticationListener.php index c0fb79eef2..87a4cf6ec5 100644 --- a/src/Symfony/Component/Security/Http/Firewall/UsernamePasswordFormAuthenticationListener.php +++ b/src/Symfony/Component/Security/Http/Firewall/UsernamePasswordFormAuthenticationListener.php @@ -37,15 +37,15 @@ class UsernamePasswordFormAuthenticationListener extends AbstractAuthenticationL /** * {@inheritdoc} */ - public function __construct(SecurityContextInterface $securityContext, AuthenticationManagerInterface $authenticationManager, SessionAuthenticationStrategyInterface $sessionStrategy, HttpUtils $httpUtils, $providerKey, AuthenticationSuccessHandlerInterface $successHandler = null, array $options = array(), AuthenticationFailureHandlerInterface $failureHandler = null, LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null, CsrfProviderInterface $csrfProvider = null) + public function __construct(SecurityContextInterface $securityContext, AuthenticationManagerInterface $authenticationManager, SessionAuthenticationStrategyInterface $sessionStrategy, HttpUtils $httpUtils, $providerKey, AuthenticationSuccessHandlerInterface $successHandler = null, AuthenticationFailureHandlerInterface $failureHandler, array $options = array(), LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null, CsrfProviderInterface $csrfProvider = null) { - parent::__construct($securityContext, $authenticationManager, $sessionStrategy, $httpUtils, $providerKey, $successHandler, array_merge(array( + parent::__construct($securityContext, $authenticationManager, $sessionStrategy, $httpUtils, $providerKey, $successHandler, $failureHandler, array_merge(array( 'username_parameter' => '_username', 'password_parameter' => '_password', 'csrf_parameter' => '_csrf_token', 'intention' => 'authenticate', 'post_only' => true, - ), $options), $failureHandler, $logger, $dispatcher); + ), $options), $logger, $dispatcher); $this->csrfProvider = $csrfProvider; } From f9d5606f3f066932ef31e67a099b8dee8746e1d9 Mon Sep 17 00:00:00 2001 From: Alexander Date: Thu, 21 Jun 2012 09:44:37 +0200 Subject: [PATCH 3/5] [Security] Update AuthenticationFailureHandlerInterface docblock. Never return null --- .../Authentication/AuthenticationFailureHandlerInterface.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Symfony/Component/Security/Http/Authentication/AuthenticationFailureHandlerInterface.php b/src/Symfony/Component/Security/Http/Authentication/AuthenticationFailureHandlerInterface.php index 9cd74964c5..7cc4da8980 100644 --- a/src/Symfony/Component/Security/Http/Authentication/AuthenticationFailureHandlerInterface.php +++ b/src/Symfony/Component/Security/Http/Authentication/AuthenticationFailureHandlerInterface.php @@ -33,7 +33,7 @@ interface AuthenticationFailureHandlerInterface * @param Request $request * @param AuthenticationException $exception * - * @return Response|null the response to return + * @return Response The response to return, never null */ function onAuthenticationFailure(Request $request, AuthenticationException $exception); } From eb19f2c9e38d6ac638d09ed7d3f026e007643f30 Mon Sep 17 00:00:00 2001 From: Alexander Date: Thu, 21 Jun 2012 09:49:20 +0200 Subject: [PATCH 4/5] [Security] Add note to CHANGELOG about refactored authentication failure/success handling [Security] Various CS + doc fixes [Security] Exception when authentication failure/success handlers do not return a response [Security] Add authors + fix docblock --- .../Security/Factory/AbstractFactory.php | 3 -- .../Security/Factory/AbstractFactoryTest.php | 9 +++-- src/Symfony/Component/Security/CHANGELOG.md | 3 ++ .../DefaultAuthenticationFailureHandler.php | 33 ++++++------------- .../DefaultAuthenticationSuccessHandler.php | 11 ++----- .../AbstractAuthenticationListener.php | 20 ++++++++--- 6 files changed, 37 insertions(+), 42 deletions(-) diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php index 99e17420eb..eae720e52d 100644 --- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php +++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php @@ -168,9 +168,7 @@ abstract class AbstractFactory implements SecurityFactoryInterface protected function createAuthenticationSuccessHandler($container, $id, $config) { - // success handler if (isset($config['success_handler'])) { - return $config['success_handler']; } @@ -185,7 +183,6 @@ abstract class AbstractFactory implements SecurityFactoryInterface protected function createAuthenticationFailureHandler($container, $id, $config) { if (isset($config['failure_handler'])) { - return $config['failure_handler']; } diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Security/Factory/AbstractFactoryTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Security/Factory/AbstractFactoryTest.php index 4e9d457121..61863b87ef 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Security/Factory/AbstractFactoryTest.php +++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Security/Factory/AbstractFactoryTest.php @@ -21,7 +21,8 @@ class AbstractFactoryTest extends \PHPUnit_Framework_TestCase list($container, $authProviderId, $listenerId, - $entryPointId) = $this->callFactory('foo', array('use_forward' => true, 'failure_path' => '/foo', 'success_handler' => 'qux', 'failure_handler' => 'bar', 'remember_me' => true), 'user_provider', 'entry_point'); + $entryPointId + ) = $this->callFactory('foo', array('use_forward' => true, 'failure_path' => '/foo', 'success_handler' => 'qux', 'failure_handler' => 'bar', 'remember_me' => true), 'user_provider', 'entry_point'); // auth provider $this->assertEquals('auth_provider', $authProviderId); @@ -48,7 +49,8 @@ class AbstractFactoryTest extends \PHPUnit_Framework_TestCase list($container, $authProviderId, $listenerId, - $entryPointId) = $this->callFactory('foo', array('remember_me' => true), 'user_provider', 'entry_point'); + $entryPointId + ) = $this->callFactory('foo', array('remember_me' => true), 'user_provider', 'entry_point'); $definition = $container->getDefinition('abstract_listener.foo'); $arguments = $definition->getArguments(); @@ -60,7 +62,8 @@ class AbstractFactoryTest extends \PHPUnit_Framework_TestCase list($container, $authProviderId, $listenerId, - $entryPointId) = $this->callFactory('foo', array('remember_me' => true), 'user_provider', 'entry_point'); + $entryPointId + ) = $this->callFactory('foo', array('remember_me' => true), 'user_provider', 'entry_point'); $definition = $container->getDefinition('abstract_listener.foo'); $arguments = $definition->getArguments(); diff --git a/src/Symfony/Component/Security/CHANGELOG.md b/src/Symfony/Component/Security/CHANGELOG.md index 2389ac3f50..797462b4e8 100644 --- a/src/Symfony/Component/Security/CHANGELOG.md +++ b/src/Symfony/Component/Security/CHANGELOG.md @@ -20,3 +20,6 @@ CHANGELOG * `ObjectIdentity::fromDomainObject`, `UserSecurityIdentity::fromAccount` and `UserSecurityIdentity::fromToken` now return correct identities for proxies objects (e.g. Doctrine proxies) + * [BC BREAK] moved the default authentication success and failure handling to + seperate classes. The order of arguments in the constructor of the + `AbstractAuthenticationListener` has changed. diff --git a/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationFailureHandler.php b/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationFailureHandler.php index 71a0057dd1..61d77a8ba8 100644 --- a/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationFailureHandler.php +++ b/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationFailureHandler.php @@ -24,36 +24,23 @@ use Symfony\Component\Security\Http\HttpUtils; * Can be optionally be extended from by the developer to alter the behaviour * while keeping the default behaviour. * + * @author Fabien Potencier + * @author Johannes M. Schmitt * @author Alexander */ class DefaultAuthenticationFailureHandler implements AuthenticationFailureHandlerInterface { - /** - * @var HttpKernel - */ - private $httpKernel; - - /** - * @var HttpUtils - */ + protected $httpKernel; protected $httpUtils; - - /** - * @var LoggerInterface - */ - private $logger; - - /** - * @var array - */ + protected $logger; protected $options; /** * Constructor. * - * @param HttpKernelInterface $httpKernel Kernel - * @param HttpUtils $httpUtils HttpUtils - * @param array $options Options for processing a successful authentication attempt. + * @param HttpKernelInterface $httpKernel + * @param HttpUtils $httpUtils + * @param array $options Options for processing a failed authentication attempt. * @param LoggerInterface $logger Optional logger */ public function __construct(HttpKernelInterface $httpKernel, HttpUtils $httpUtils, array $options, LoggerInterface $logger = null) @@ -63,9 +50,9 @@ class DefaultAuthenticationFailureHandler implements AuthenticationFailureHandle $this->logger = $logger; $this->options = array_merge(array( - 'failure_path' => null, - 'failure_forward' => false, - 'login_path' => '/login', + 'failure_path' => null, + 'failure_forward' => false, + 'login_path' => '/login', ), $options); } diff --git a/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php b/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php index 8139786369..88dcf68080 100644 --- a/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php +++ b/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php @@ -21,24 +21,19 @@ use Symfony\Component\Security\Http\HttpUtils; * Can be optionally be extended from by the developer to alter the behaviour * while keeping the default behaviour. * + * @author Fabien Potencier + * @author Johannes M. Schmitt * @author Alexander */ class DefaultAuthenticationSuccessHandler implements AuthenticationSuccessHandlerInterface { - /** - * @var HttpUtils - */ protected $httpUtils; - - /** - * @var array - */ protected $options; /** * Constructor. * - * @param HttpUtils $httpUtils HttpUtils + * @param HttpUtils $httpUtils * @param array $options Options for processing a successful authentication attempt. */ public function __construct(HttpUtils $httpUtils, array $options) diff --git a/src/Symfony/Component/Security/Http/Firewall/AbstractAuthenticationListener.php b/src/Symfony/Component/Security/Http/Firewall/AbstractAuthenticationListener.php index 377639c086..3324ba96fa 100644 --- a/src/Symfony/Component/Security/Http/Firewall/AbstractAuthenticationListener.php +++ b/src/Symfony/Component/Security/Http/Firewall/AbstractAuthenticationListener.php @@ -70,12 +70,12 @@ abstract class AbstractAuthenticationListener implements ListenerInterface * @param SessionAuthenticationStrategyInterface $sessionStrategy * @param HttpUtils $httpUtils An HttpUtilsInterface instance * @param string $providerKey + * @param AuthenticationSuccessHandlerInterface $successHandler + * @param AuthenticationFailureHandlerInterface $failureHandler * @param array $options An array of options for the processing of a * successful, or failed authentication attempt - * @param AuthenticationSuccessHandlerInterface $successHandler - * @param AuthenticationFailureHandlerInterface $failureHandler - * @param LoggerInterface $logger A LoggerInterface instance - * @param EventDispatcherInterface $dispatcher An EventDispatcherInterface instance + * @param LoggerInterface $logger A LoggerInterface instance + * @param EventDispatcherInterface $dispatcher An EventDispatcherInterface instance */ public function __construct(SecurityContextInterface $securityContext, AuthenticationManagerInterface $authenticationManager, SessionAuthenticationStrategyInterface $sessionStrategy, HttpUtils $httpUtils, $providerKey, AuthenticationSuccessHandlerInterface $successHandler, AuthenticationFailureHandlerInterface $failureHandler, array $options = array(), LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null) { @@ -184,7 +184,13 @@ abstract class AbstractAuthenticationListener implements ListenerInterface $this->securityContext->setToken(null); - return $this->failureHandler->onAuthenticationFailure($request, $failed); + $response = $this->failureHandler->onAuthenticationFailure($request, $failed); + + if (!$response instanceof Response) { + throw new \RuntimeException('Authentication Failure Handler did not return a Response.'); + } + + return $response; } private function onSuccess(GetResponseEvent $event, Request $request, TokenInterface $token) @@ -206,6 +212,10 @@ abstract class AbstractAuthenticationListener implements ListenerInterface $response = $this->successHandler->onAuthenticationSuccess($request, $token); + if (!$response instanceof Response) { + throw new \RuntimeException('Authentication Success Handler did not return a Response.'); + } + if (null !== $this->rememberMeServices) { $this->rememberMeServices->loginSuccess($request, $response, $token); } From bb138dadb3ad5a0b5850fc25bd2f9ec8d47880ed Mon Sep 17 00:00:00 2001 From: Alexander Date: Fri, 6 Jul 2012 23:57:26 +0200 Subject: [PATCH 5/5] [Security] Fix regression after rebase. Target url should be firewall dependent --- .../Security/Factory/AbstractFactory.php | 9 +++++---- .../Resources/config/security_listeners.xml | 1 + .../DefaultAuthenticationSuccessHandler.php | 11 +++++++---- .../UsernamePasswordFormAuthenticationListener.php | 2 +- 4 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php index eae720e52d..210252f69b 100644 --- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php +++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php @@ -172,12 +172,13 @@ abstract class AbstractFactory implements SecurityFactoryInterface return $config['success_handler']; } - $id = 'security.authentication.success_handler.'.$id; + $successHandlerId = 'security.authentication.success_handler.'.$id; - $successHandler = $container->setDefinition($id, new DefinitionDecorator('security.authentication.success_handler')); - $successHandler->replaceArgument(1, array_intersect_key($config, $this->defaultSuccessHandlerOptions)); + $successHandler = $container->setDefinition($successHandlerId, new DefinitionDecorator('security.authentication.success_handler')); + $successHandler->replaceArgument(1, $id); + $successHandler->replaceArgument(2, array_intersect_key($config, $this->defaultSuccessHandlerOptions)); - return $id; + return $successHandlerId; } protected function createAuthenticationFailureHandler($container, $id, $config) diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml b/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml index a0d01a33d1..6c39faceff 100644 --- a/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml +++ b/src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml @@ -111,6 +111,7 @@ + diff --git a/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php b/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php index 88dcf68080..deb7d4574a 100644 --- a/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php +++ b/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php @@ -29,16 +29,19 @@ class DefaultAuthenticationSuccessHandler implements AuthenticationSuccessHandle { protected $httpUtils; protected $options; + protected $providerKey; /** * Constructor. * * @param HttpUtils $httpUtils + * @param string $providerKey * @param array $options Options for processing a successful authentication attempt. */ - public function __construct(HttpUtils $httpUtils, array $options) + public function __construct(HttpUtils $httpUtils, $providerKey, array $options) { - $this->httpUtils = $httpUtils; + $this->httpUtils = $httpUtils; + $this->providerKey = $providerKey; $this->options = array_merge(array( 'always_use_default_target_path' => false, @@ -75,8 +78,8 @@ class DefaultAuthenticationSuccessHandler implements AuthenticationSuccessHandle } $session = $request->getSession(); - if ($targetUrl = $session->get('_security.target_path')) { - $session->remove('_security.target_path'); + if ($targetUrl = $session->get('_security.'.$this->providerKey.'.target_path')) { + $session->remove('_security.'.$this->providerKey.'.target_path'); return $targetUrl; } diff --git a/src/Symfony/Component/Security/Http/Firewall/UsernamePasswordFormAuthenticationListener.php b/src/Symfony/Component/Security/Http/Firewall/UsernamePasswordFormAuthenticationListener.php index 87a4cf6ec5..22330a8c43 100644 --- a/src/Symfony/Component/Security/Http/Firewall/UsernamePasswordFormAuthenticationListener.php +++ b/src/Symfony/Component/Security/Http/Firewall/UsernamePasswordFormAuthenticationListener.php @@ -37,7 +37,7 @@ class UsernamePasswordFormAuthenticationListener extends AbstractAuthenticationL /** * {@inheritdoc} */ - public function __construct(SecurityContextInterface $securityContext, AuthenticationManagerInterface $authenticationManager, SessionAuthenticationStrategyInterface $sessionStrategy, HttpUtils $httpUtils, $providerKey, AuthenticationSuccessHandlerInterface $successHandler = null, AuthenticationFailureHandlerInterface $failureHandler, array $options = array(), LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null, CsrfProviderInterface $csrfProvider = null) + public function __construct(SecurityContextInterface $securityContext, AuthenticationManagerInterface $authenticationManager, SessionAuthenticationStrategyInterface $sessionStrategy, HttpUtils $httpUtils, $providerKey, AuthenticationSuccessHandlerInterface $successHandler, AuthenticationFailureHandlerInterface $failureHandler, array $options = array(), LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null, CsrfProviderInterface $csrfProvider = null) { parent::__construct($securityContext, $authenticationManager, $sessionStrategy, $httpUtils, $providerKey, $successHandler, $failureHandler, array_merge(array( 'username_parameter' => '_username',