From eb19f2c9e38d6ac638d09ed7d3f026e007643f30 Mon Sep 17 00:00:00 2001 From: Alexander Date: Thu, 21 Jun 2012 09:49:20 +0200 Subject: [PATCH] [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); }