merged branch asm89/refactor-authentication-success-handling (PR #4599)

Commits ------- bb138da [Security] Fix regression after rebase. Target url should be firewall dependent eb19f2c [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 f9d5606 [Security] Update AuthenticationFailureHandlerInterface docblock. Never return null 915704c [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 c6aa392 [Security] Move default authentication success handling strategy to seperate class [Security] Update configuration for changes regarding default success handler [Security] Fix + add AbstractFactory test Discussion ---------- [Security] Refactor authentication success handling Bug fix: no Feature addition: no Backwards compatibility break: yes Symfony2 tests pass: [![Build Status](https://secure.travis-ci.org/asm89/symfony.png?branch=refactor-authentication-success-handling)](http://travis-ci.org/asm89/symfony) License of the code: MIT This PR extracts the default authentication success handling to its own class as discussed in #4553. In the end the PR will basically revert #3183 (as suggested by @schmittjoh) and fix point one of #838. There are a few noticeable changes in this PR: - This implementation changes the constructor signature of the `AbstractAuthentictionListener` and `UsernamePasswordFormAuthenticationListener` by making the `AuthenticationSuccessHandler` mandatory (BC break). If this WIP is approved I will refactor the failure handling logic too and then this will also move one place in the constructor - This PR reverts the change of making the returning of a `Response` optional in the `AuthenticationSuccessHandlerInterface`. Developers can now extend the default behavior themselves @schmittjoh Any suggestions? Or a +1 to do the failure logic too? --------------------------------------------------------------------------- by schmittjoh at 2012-06-17T23:53:07Z +1 from me @fabpot, what so you think? --------------------------------------------------------------------------- by fabpot at 2012-06-19T08:15:48Z Can you add a note in the CHANGELOG? Thanks. --------------------------------------------------------------------------- by asm89 at 2012-06-19T10:22:20Z I will, but I'll first do the same for the failure logic. --------------------------------------------------------------------------- by travisbot at 2012-06-21T08:03:14Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1671555) (merged 17c8f66f into 55c6df9). --------------------------------------------------------------------------- by asm89 at 2012-06-21T08:45:38Z :+1: thank you @stof. I think this is good to go now. --------------------------------------------------------------------------- by travisbot at 2012-06-21T08:50:28Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1671817) (merged 8982c769 into 55c6df9). --------------------------------------------------------------------------- by asm89 at 2012-06-21T14:23:58Z @schmittjoh @fabpot The `LogoutListener` currently throws an exception when the successhandler doesn't return a `Response` ([link](https://github.com/symfony/symfony/blob/9e9519913d2c5e2bef96070bcb9106e1e389c3bd/src/Symfony/Component/Security/Http/Firewall/LogoutListener.php#L101)). Should this code check for this too? --------------------------------------------------------------------------- by schmittjoh at 2012-06-21T14:26:49Z Yes, this code was removed, but needs to be re-added here as well. --------------------------------------------------------------------------- by travisbot at 2012-06-21T15:08:59Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1674437) (merged 5afa240d into 55c6df9). --------------------------------------------------------------------------- by asm89 at 2012-06-26T06:01:02Z @fabpot Can you make a final decision on this? If you decide on point 3, this code can be merged. I agree with the arguments of @stof about the option handling and it 'only' being a BC break for direct users of the security component. I even think these direct users should be really careful anyway, since the behavior of the success and failurehandlers now change back to how they acted in 2.0. Now I am thinking about it, can't the optional parameters of this class move to setters anyway? That will make it cleaner to extend. --------------------------------------------------------------------------- by asm89 at 2012-06-28T10:29:50Z ping @fabpot --------------------------------------------------------------------------- by fabpot at 2012-06-28T17:23:02Z I'm ok with option 1 (the BC break). After doing the last changes, can you squash your commits before I merge? Thanks. --------------------------------------------------------------------------- by asm89 at 2012-07-06T21:59:54Z @fabpot I rebased the PR, added the authors and also ported the fix that was done in 8ffaafa to be contained in the default success handler. I also squashed all the CS and 'small blabla fix' commits. Is it ok now? Edit: travisbot will probably say that the tests in this PR fail, but that is because current master fails on form things --------------------------------------------------------------------------- by asm89 at 2012-07-08T18:53:05Z I rebased the PR, tests are green now: [![Build Status](https://secure.travis-ci.org/asm89/symfony.png?branch=refactor-authentication-success-handling)](http://travis-ci.org/asm89/symfony).
symfony · Jul 9, 2012 · 7f9fd11 · 7f9fd11
2 parents d100ffa + bb138da
commit 7f9fd11
Show file tree

Hide file tree

Showing 10 changed files with 312 additions and 114 deletions.
diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php
@@ -28,14 +28,21 @@ abstract class AbstractFactory implements SecurityFactoryInterface
 {
     protected $options = array(
         'check_path'                     => '/login_check',
-        'login_path'                     => '/login',
         'use_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)
@@ -71,7 +78,7 @@ public function addConfiguration(NodeDefinition $node)
             ->scalarNode('failure_handler')->end()
         ;
 
-        foreach ($this->options 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 {
@@ -149,21 +156,42 @@ protected function createListener($container, $id, $config, $userProvider)
         $listenerId = $this->getListenerId();
         $listener = new DefinitionDecorator($listenerId);
         $listener->replaceArgument(4, $id);
-        $listener->replaceArgument(5, array_intersect_key($config, $this->options));
+        $listener->replaceArgument(5, new Reference($this->createAuthenticationSuccessHandler($container, $id, $config)));
+        $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);
 
-        // success handler
+        return $listenerId;
+    }
+
+    protected function createAuthenticationSuccessHandler($container, $id, $config)
+    {
         if (isset($config['success_handler'])) {
-            $listener->replaceArgument(6, new Reference($config['success_handler']));
+            return $config['success_handler'];
         }
 
-        // failure handler
+        $successHandlerId = 'security.authentication.success_handler.'.$id;
+
+        $successHandler = $container->setDefinition($successHandlerId, new DefinitionDecorator('security.authentication.success_handler'));
+        $successHandler->replaceArgument(1, $id);
+        $successHandler->replaceArgument(2, array_intersect_key($config, $this->defaultSuccessHandlerOptions));
+
+        return $successHandlerId;
+    }
+
+    protected function createAuthenticationFailureHandler($container, $id, $config)
+    {
         if (isset($config['failure_handler'])) {
-            $listener->replaceArgument(7, new Reference($config['failure_handler']));
+            return $config['failure_handler'];
         }
 
-        $listenerId .= '.'.$id;
-        $container->setDefinition($listenerId, $listener);
+        $id = 'security.authentication.failure_handler.'.$id;
 
-        return $listenerId;
+        $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
@@ -37,6 +37,9 @@
         <parameter key="security.authentication.provider.pre_authenticated.class">Symfony\Component\Security\Core\Authentication\Provider\PreAuthenticatedAuthenticationProvider</parameter>
 
         <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>
@@ -98,13 +101,26 @@
             <argument type="service" id="security.authentication.session_strategy" />
             <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.success_handler" on-invalid="null" />
-            <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>
 
+        <service id="security.authentication.success_handler" class="%security.authentication.success_handler.class%" abstract="true" public="false">
+            <argument type="service" id="security.http_utils" />
+            <argument />
+            <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"

diff --git a/.../Bundle/SecurityBundle/Tests/DependencyInjection/Security/Factory/AbstractFactoryTest.php b/.../Bundle/SecurityBundle/Tests/DependencyInjection/Security/Factory/AbstractFactoryTest.php
@@ -18,26 +18,11 @@ class AbstractFactoryTest extends \PHPUnit_Framework_TestCase
 {
     public function testCreate()
     {
-        $factory = $this->getFactory();
-
-        $factory
-            ->expects($this->once())
-            ->method('createAuthProvider')
-            ->will($this->returnValue('auth_provider'))
-        ;
-        $factory
-            ->expects($this->atLeastOnce())
-            ->method('getListenerId')
-            ->will($this->returnValue('abstract_listener'))
-        ;
-
-        $container = new ContainerBuilder();
-        $container->register('auth_provider');
-
-        list($authProviderId,
+        list($container,
+             $authProviderId,
              $listenerId,
              $entryPointId
-        ) = $factory->create($container, 'foo', array('use_forward' => true, 'failure_path' => '/foo', 'success_handler' => 'foo', 'remember_me' => true), 'user_provider', 'entry_point');
+         ) = $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,19 +33,66 @@ public function testCreate()
         $definition = $container->getDefinition('abstract_listener.foo');
         $this->assertEquals(array(
             'index_4' => 'foo',
-            'index_5' => array(
+            'index_5' => new Reference('qux'),
+            'index_6' => new Reference('bar'),
+            'index_7' => 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()
+    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()
     {
-        return $this->getMockForAbstractClass('Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\AbstractFactory', array());
+        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())
+            ->method('createAuthProvider')
+            ->will($this->returnValue('auth_provider'))
+        ;
+        $factory
+            ->expects($this->atLeastOnce())
+            ->method('getListenerId')
+            ->will($this->returnValue('abstract_listener'))
+        ;
+
+        $container = new ContainerBuilder();
+        $container->register('auth_provider');
+
+        list($authProviderId,
+             $listenerId,
+             $entryPointId
+         ) = $factory->create($container, $id, $config, $userProviderId, $defaultEntryPointId);
+
+        return array($container, $authProviderId, $listenerId, $entryPointId);
     }
 }
diff --git 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/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
      */
     public function onAuthenticationFailure(Request $request, AuthenticationException $exception);
 }
diff --git 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
      */
     public function onAuthenticationSuccess(Request $request, TokenInterface $token);
 }
diff --git a/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationFailureHandler.php b/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationFailureHandler.php
@@ -0,0 +1,87 @@
+<?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 Fabien Potencier <fabien@symfony.com>
+ * @author Johannes M. Schmitt <schmittjoh@gmail.com>
+ * @author Alexander <iam.asm89@gmail.com>
+ */
+class DefaultAuthenticationFailureHandler implements AuthenticationFailureHandlerInterface
+{
+    protected $httpKernel;
+    protected $httpUtils;
+    protected $logger;
+    protected $options;
+
+    /**
+     * Constructor.
+     *
+     * @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)
+    {
+        $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']);
+    }
+}