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 55c6df99).

---------------------------------------------------------------------------

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 55c6df99).

---------------------------------------------------------------------------

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 55c6df99).

---------------------------------------------------------------------------

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 8ffaafa86741a03ecb2f91e3d67802f4c6baf36b 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).

4 files changed, 182 insertions, 2 deletions

diff --git a/Http/Authentication/AuthenticationFailureHandlerInterface.php b/Http/Authentication/AuthenticationFailureHandlerInterface.php
index 5bb00de..8dbd29a 100644
--- a/Http/Authentication/AuthenticationFailureHandlerInterface.php
+++ b/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/Http/Authentication/AuthenticationSuccessHandlerInterface.php b/Http/Authentication/AuthenticationSuccessHandlerInterface.php
index bf03cd6..5c08e73 100644
--- a/Http/Authentication/AuthenticationSuccessHandlerInterface.php
+++ b/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/Http/Authentication/DefaultAuthenticationFailureHandler.php b/Http/Authentication/DefaultAuthenticationFailureHandler.php
new file mode 100644
index 0000000..61d77a8
--- /dev/null
+++ b/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']);
+    }
+}
diff --git a/Http/Authentication/DefaultAuthenticationSuccessHandler.php b/Http/Authentication/DefaultAuthenticationSuccessHandler.php
new file mode 100644
index 0000000..deb7d45
--- /dev/null
+++ b/Http/Authentication/DefaultAuthenticationSuccessHandler.php
@@ -0,0 +1,93 @@
+<?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\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 Fabien Potencier <fabien@symfony.com>
+ * @author Johannes M. Schmitt <schmittjoh@gmail.com>
+ * @author Alexander <iam.asm89@gmail.com>
+ */
+class DefaultAuthenticationSuccessHandler implements AuthenticationSuccessHandlerInterface
+{
+    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, $providerKey, array $options)
+    {
+        $this->httpUtils   = $httpUtils;
+        $this->providerKey = $providerKey;
+
+        $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);
+    }
+
+    /**
+     * {@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.'.$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'];
+    }
+}