8000 merged branch asm89/refactor-authentication-success-handling (PR #4599) · symfony/symfony@7f9fd11 · GitHub
[go: up one dir, main page]
8000
Skip to content

Commit 7f9fd11

Browse files
fabpotfabpot
committed
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).
2 parents d100ffa + bb138da commit 7f9fd11

File tree

10 files changed

+312
-114
lines changed

10 files changed

+312
-114
lines changed

‎src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php

Lines changed: 38 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -28,14 +28,21 @@ abstract class AbstractFactory implements SecurityFactoryInterface
2828
{
2929
protected $options = array(
3030
'check_path' => '/login_check',
31-
'login_path' => '/login',
3231
'use_forward' => false,
32+
);
33+
34+
protected $defaultSuccessHandlerOptions = array(
3335
'always_use_default_target_path' => false,
3436
'default_target_path' => 9E12 '/',
37+
'login_path' => '/login',
3538
'target_path_parameter' => '_target_path',
3639
'use_referer' => false,
40+
);
41+
42+
protected $defaultFailureHandlerOptions = array(
3743
'failure_path' => null,
3844
'failure_forward' => false,
45+
'login_path' => '/login',
3946
);
4047

4148
public function create(ContainerBuilder $container, $id, $config, $userProviderId, $defaultEntryPointId)
@@ -71,7 +78,7 @@ public function addConfiguration(NodeDefinition $node)
7178
->scalarNode('failure_handler')->end()
7279
;
7380

74-
foreach ($this->options as $name => $default) {
81+
foreach (array_merge($this->options, $this->defaultSuccessHandlerOptions, $this->defaultFailureHandlerOptions) as $name => $default) {
7582
if (is_bool($default)) {
7683
$builder->booleanNode($name)->defaultValue($default);
7784
} else {
@@ -149,21 +156,42 @@ protected function createListener($container, $id, $config, $userProvider)
149156
$listenerId = $this->getListenerId();
150157
$listener = new DefinitionDecorator($listenerId);
151158
$listener->replaceArgument(4, $id);
152-
$listener->replaceArgument(5, array_intersect_key($config, $this->options));
159+
$listener->replaceArgument(5, new Reference($this->createAuthenticationSuccessHandler($container, $id, $config)));
160+
$listener->replaceArgument(6, new Reference($this->createAuthenticationFailureHandler($container, $id, $config)));
161+
$listener->replaceArgument(7, array_intersect_key($config, $this->options));
162+
163+
$listenerId .= '.'.$id;
164+
$container->setDefinition($listenerId, $listener);
153165

154-
// success handler
166+
return $listenerId;
167+
}
168+
169+
protected function createAuthenticationSuccessHandler($container, $id, $config)
170+
{
155171
if (isset($config['success_handler'])) {
156-
$listener->replaceArgument(6, new Reference($config['success_handler']));
172+
return $config['success_handler'];
157173
}
158174

159-
// failure handler
175+
$successHandlerId = 'security.authentication.success_handler.'.$id;
176+
177+
$successHandler = $container->setDefinition($successHandlerId, new DefinitionDecorator('security.authentication.success_handler'));
178+
$successHandler->replaceArgument(1, $id);
179+
$successHandler->replaceArgument(2, array_intersect_key($config, $this->defaultSuccessHandlerOptions));
180+
181+
return $successHandlerId;
182+
}
183+
184+
protected function createAuthenticationFailureHandler($container, $id, $config)
185+
{
160186
if (isset($config['failure_handler'])) {
161-
$listener->replaceArgument(7, new Reference($config['failure_handler']));
187+
return $config['failure_handler'];
162188
}
163189

164-
$listenerId .= '.'.$id;
165-
$container->setDefinition($listenerId, $listener);
190+
$id = 'security.authentication.failure_handler.'.$id;
166191

167-
return $listenerId;
192+
$failureHandler = $container->setDefinition($id, new DefinitionDecorator('security.authentication.failure_handler'));
193+
$failureHandler->replaceArgument(2, array_intersect_key($config, $this->defaultFailureHandlerOptions));
194+
195+
return $id;
168196
}
169197
}

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

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,9 @@
3737
<parameter key="security.authentication.provider.pre_authenticated.class">Symfony\Component\Security\Core\Authentication\Provider\PreAuthenticatedAuthenticationProvider</parameter>
3838

3939
<parameter key="security.authentication.provider.anonymous.class">Symfony\Component\Security\Core\Authentication\Provider\AnonymousAuthenticationProvider</parameter>
40+
41+
<parameter key="security.authentication.success_handler.class">Symfony\Component\Security\Http\Authentication\DefaultAuthenticationSuccessHandler</parameter>
42+
<parameter key="security.authentication.failure_handler.class">Symfony\Component\Security\Http\Authentication\DefaultAuthenticationFailureHandler</parameter>
4043
</parameters>
4144

4245
<services>
@@ -98,13 +101,26 @@
98101
<argument type="service" id="security.authentication.session_strategy" />
99102
<argument type="service" id="security.http_utils" />
100103
<argument />
104+
<argument type="service" id="security.authentication.success_handler" />
105+
<argument type="service" id="security.authentication.failure_handler" />
101106
<argument type="collection"></argument>
102-
<argument type="service" id="security.authentication.success_handler" on-invalid="null" />
103-
<argument type="service" id="security.authentication.failure_handler" on-invalid="null" />
104107
<argument type="service" id="logger" on-invalid="null" />
105108
<argument type="service" id="event_dispatcher" on-invalid="null" />
106109
</service>
107110

111+
<service id="security.authentication.success_handler" class="%security.authentication.success_handler.class%" abstract="true" public="false">
112+
<argument type="service" id="security.http_utils" />
113+
<argument />
114+
<argument />
115+
</service>
116+
117+
<service id="security.authentication.failure_handler" class="%security.authentication.failure_handler.class%" abstract="true" public="false">
118+
<argument type="service" id="http_kernel" />
119+
<argument type="service" id="security.http_utils" />
120+
<argument />
121+
<argument type="service" id="logger" on-invalid="null" />
122+
</service>
123+
108124
<service id="security.authentication.listener.form"
109125
class="%security.authentication.listener.form.class%"
110126
parent="security.authentication.listener.abstract"

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

Lines changed: 55 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -18,26 +18,11 @@ class AbstractFactoryTest extends \PHPUnit_Framework_TestCase
1818
{
1919
public function testCreate()
2020
{
21-
$factory = $this->getFactory();
22-
23-
$factory
24-
->expects($this->once())
25-
->method('createAuthProvider')
26-
->will($this->returnValue('auth_provider'))
27-
;
28-
$factory
29-
->expects($this->atLeastOnce())
30-
->method('getListenerId')
31-
->will($this->returnValue('abstract_listener'))
32-
;
33-
34-
$container = new ContainerBuilder();
35-
$container->register('auth_provider');
36-
37-
list($authProviderId,
21+
list($container,
22+
$authProviderId,
3823
$listenerId,
3924
$entryPointId
40-
) = $factory->create($container, 'foo', array('use_forward' => true, 'failure_path' => '/foo', 'success_handler' => 'foo', 'remember_me' => true), 'user_provider', 'entry_point');
25+
) = $this->callFactory('foo', array('use_forward' => true, 'failure_path' => '/foo', 'success_handler' => 'qux', 'failure_handler' => 'bar', 'remember_me' => true), 'user_provider', 'entry_point');
4126

4227
// auth provider
4328
$this->assertEquals('auth_provider', $authProviderId);
@@ -48,19 +33,66 @@ public function testCreate()
4833
$definition = $container->getDefinition('abstract_listener.foo');
4934
$this->assertEquals(array(
5035
'index_4' => 'foo',
51-
'index_5' => array(
36+
'index_5' => new Reference('qux'),
37+
'index_6' => new Reference('bar'),
38+
'index_7' => array(
5239
'use_forward' => true,
53-
'failure_path' => '/foo',
5440
),
55-
'index_6' => new Reference('foo'),
5641
), $definition->getArguments());
5742

5843
// entry point
5944
$this->assertEquals('entry_point', $entryPointId, '->create() does not change the default entry point.');
6045
}
6146

62-
protected function getFactory()
47+
public function testDefaultFailureHandler()
48+
{
49+
list($container,
50+
$authProviderId,
51+
$listenerId,
52+
$entryPointId
53+
) = $this->callFactory('foo', array('remember_me' => true), 'user_provider', 'entry_point');
54+
55+
$definition = $container->getDefinition('abstract_listener.foo');
56+
$arguments = $definition->getArguments();
57+
$this->assertEquals(new Reference('security.authentication.failure_handler.foo'), $arguments['index_6']);
58+
}
59+
60+
public function testDefaultSuccessHandler()
6361
{
64-
return $this->getMockForAbstractClass('Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\AbstractFactory', array());
62+
list($container,
63+
$authProviderId,
64+
$listenerId,
65+
$entryPointId
66+
) = $this->callFactory('foo', array('remember_me' => true), 'user_provider', 'entry_point');
67+
68+
$definition = $container->getDefinition('abstract_listener.foo');
69+
$arguments = $definition->getArguments();
70+
$this->assertEquals(new Reference('security.authentication.success_handler.foo'), $arguments['index_5']);
71+
}
72+
73+
protected function callFactory($id, $config, $userProviderId, $defaultEntryPointId)
74+
{
75+
$factory = $this->getMockForAbstractClass('Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\AbstractFactory', array());
76+
77+
$factory
78+
->expects($this->once())
79+
->method('createAuthProvider')
80+
->will($this->returnValue('auth_provider'))
81+
;
82+
$factory
83+
->expects($this->atLeastOnce())
84+
->method('getListenerId')
85+
->will($this->returnValue('abstract_listener'))
86+
;
87+
88+
$container = new ContainerBuilder();
89+
$container->register('auth_provider');
90+
91+
list($authProviderId,
92+
$listenerId,
93+
$entryPointId
94+
) = $factory->create($container, $id, $config, $userProviderId, $defaultEntryPointId);
95+
96+
return array($container, $authProviderId, $listenerId, $entryPointId);
6597
}
6698
}

‎src/Symfony/Component/Security/CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,3 +20,6 @@ CHANGELOG
2020
* `ObjectIdentity::fromDomainObject`, `UserSecurityIdentity::fromAccount` and
2121
`UserSecurityIdentity::fromToken` now return correct identities for proxies
2222
objects (e.g. Doctrine proxies)
23+
* [BC BREAK] moved the default authentication success and failure handling to
24+
seperate classes. The order of arguments in the constructor of the
25+
`AbstractAuthenticationListener` has changed.

‎src/Symfony/Component/Security/Http/Authentication/AuthenticationFailureHandlerInterface.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ interface AuthenticationFailureHandlerInterface
3333
* @param Request $request
3434
* @param AuthenticationException $exception
3535
*
36-
* @return Response|null the response to return
36+
* @return Response The response to return, never null
3737
*/
3838
public function onAuthenticationFailure(Request $request, AuthenticationException $exception);
3939
}

‎src/Symfony/Component/Security/Http/Authentication/AuthenticationSuccessHandlerInterface.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ interface AuthenticationSuccessHandlerInterface
3333
* @param Request $request
3434
* @param TokenInterface $token
3535
*
36-
* @return Response|null the response to return
36+
* @return Response never null
3737
*/
3838
public function onAuthenticationSuccess(Request $request, TokenInterface $token);
3939
}

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

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Component\Security\Http\Authentication;
13+
14+
use Symfony\Component\HttpFoundation\Request;
15+
use Symfony\Component\HttpKernel\HttpKernelInterface;
16+
use Symfony\Component\HttpKernel\Log\LoggerInterface;
17+
use Symfony\Component\Security\Core\Exception\AuthenticationException;
18+
use Symfony\Component\Security\Core\SecurityContextInterface;
19+
use Symfony\Component\Security\Http\HttpUtils;
20+
21+
/**
22+
* Class with the default authentication failure handling logic.
23+
*
24+
* Can be optionally be extended from by the developer to alter the behaviour
25+
* while keeping the default behaviour.
26+
*
27+
* @author Fabien Potencier <fabien@symfony.com>
28+
* @author Johannes M. Schmitt <schmittjoh@gmail.com>
29+
* @author Alexander <iam.asm89@gmail.com>
30+
*/
31+
class DefaultAuthenticationFailureHandler implements AuthenticationFailureHandlerInterface
32+
{
33+
protected $httpKernel;
34+
protected $httpUtils;
35+
protected $logger;
36+
protected $options;
37+
38+
/**
39+
* Constructor.
40+
*
41+
* @param HttpKernelInterface $httpKernel
42+
* @param HttpUtils $httpUtils
43+
* @param array $options Options for processing a failed authentication attempt.
44+
* @param LoggerInterface $logger Optional logger
45+
*/
46+
public function __construct(HttpKernelInterface $httpKernel, HttpUtils $httpUtils, array $options, LoggerInterface $logger = null)
47+
{
48+
$this->httpKernel = $httpKernel;
49+
$this->httpUtils = $httpUtils;
50+
$this->logger = $logger;
51+
52+
$this->options = array_merge(array(
53+
'failure_path' => null,
54+
'failure_forward' => false,
55+
'login_path' => '/login',
56+
), $options);
57+
}
58+
59+
/**
60+
* {@inheritDoc}
61+
*/
62+
public function onAuthenticationFailure(Request $request, AuthenticationException $exception)
63+
{
64+
if (null === $this->options['failure_path']) {
65+
$this->options['failure_path'] = $this->options['login_path'];
66+
}
67+
68+
if ($this->options['failure_forward']) {
69+
if (null !== $this->logger) {
70+
$this->logger->debug(sprintf('Forwarding to %s', $this->options['failure_path']));
71+
}
72+
73+
$subRequest = $this->httpUtils->createRequest($request, $this->options['failure_path']);
74+
$subRequest->attributes->set(SecurityContextInterface::AUTHENTICATION_ERROR, $exception);
75+
76+
return $this->httpKernel->handle($subRequest, HttpKernelInterface::SUB_REQUEST);
77+
}
78+
79+
if (null !== $this->logger) {
80+
$this->logger->debug(sprintf('Redirecting to %s', $this->options['failure_path']));
81+
}
82+
83+
$request->getSession()->set(SecurityContextInterface::AUTHENTICATION_ERROR, $exception);
84+
85+
return $this->httpUtils->createRedirectResponse($request, $this->options['failure_path']);
86+
}
87+
}

0 commit comments

Comments
 (0)
0