[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

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));
+        $listener->replaceArgument(6, new Reference($this->createAuthenticationFailureHandler($container, $id, $config)));
-
+        $listener->replaceArgument(7, array_intersect_key($config, $this->options));
-        // failure handler
-        if (isset($config['failure_handler'])) {
-            $listener->replaceArgument(7, new Reference($config['failure_handler']));
-        }
 
         $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;
+    }
 }

src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml

@@ -39,6 +39,7 @@
         <parameter key="security.authentication.provider.anonymous.class">Symfony\Component\Security\Core\Authentication\Provider\AnonymousAuthenticationProvider</parameter>
 
         <parameter key="security.authentication.success_handler.class">Symfony\Component\Security\Http\Authentication\DefaultAuthenticationSuccessHandler</parameter>
+        <parameter key="security.authentication.failure_handler.class">Symfony\Component\Security\Http\Authentication\DefaultAuthenticationFailureHandler</parameter>
     </parameters>
 
     <services>
@@ -101,8 +102,8 @@
             <argument type="service" id="security.http_utils" />
             <argument />
             <argument type="service" id="security.authentication.success_handler" />
+            <argument type="service" id="security.authentication.failure_handler" />
             <argument type="collection"></argument>
-            <argument type="service" id="security.authentication.failure_handler" on-invalid="null" />
             <argument type="service" id="logger" on-invalid="null" />
             <argument type="service" id="event_dispatcher" on-invalid="null" />
         </service>
@@ -112,6 +113,13 @@
             <argument />
         </service>
 
+        <service id="security.authentication.failure_handler" class="%security.authentication.failure_handler.class%" abstract="true" public="false">
+            <argument type="service" id="http_kernel" />
+            <argument type="service" id="security.http_utils" />
+            <argument />
+            <argument type="service" id="logger" on-invalid="null" />
+        </service>
+
         <service id="security.authentication.listener.form"
                  class="%security.authentication.listener.form.class%"
                  parent="security.authentication.listener.abstract"

src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Security/Factory/AbstractFactoryTest.php

@@ -21,7 +21,7 @@ class AbstractFactoryTest extends \PHPUnit_Framework_TestCase
         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');
+             $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_5' => new Reference('qux'),
-            'index_6' => array(
+            '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,

src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationFailureHandler.php

@@ -0,0 +1,100 @@
+<?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\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 <iam.asm89@gmail.com>
+ */
+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']);
+    }
+}

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);

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) {
+        return $this->failureHandler->onAuthenticationFailure($request, $failed);
-            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']);
     }
 
     private function onSuccess(GetResponseEvent $event, Request $request, TokenInterface $token)

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;
     }