8000 merged branch jmikola/logout-csrf (PR #3007) · symfony/symfony@294b57e · GitHub
[go: up one dir, main page]
Skip to content

Commit 294b57e

Browse files
fabpotfabpot
committed
merged branch jmikola/logout-csrf (PR #3007)
Commits ------- 49a8654 [Security] Use LogoutException for invalid CSRF token in LogoutListener a96105e [SecurityBundle] Use assertCount() in tests 4837407 [SecurityBundle] Fix execution of functional tests with different names 66722b3 [SecurityBundle] Templating helpers to generate logout URL's with CSRF tokens aaaa040 [Security] Allow LogoutListener to validate CSRF tokens b1f545b [Security] Refactor LogoutListener constructor to take options c48c775 [SecurityBundle] Add functional test for form login with CSRF token Discussion ---------- [Security] Implement support for CSRF tokens in logout URL's ``` Bug fix: no Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: - Todo: - ``` [![Build Status](https://secure.travis-ci.org/jmikola/symfony.png?branch=logout-csrf)](http://travis-ci.org/jmikola/symfony) This derived from #3006 but properly targeting on the master branch. This exposes new configuration options to the logout listener to enable CSRF protection, as already exists for the form login listener. The individual commits and their extended messages should suffice for explaining the logical changes of the PR. In addition to changing LogoutListener, I also created a templating helper to generate logout URL's, which includes a CSRF token if necessary. This may or may not using routing, depending on how the listener is configured since both route names or hard-coded paths are valid options. Additionally, I added unit tests for LogoutListener and functional tests for both CSRF-enabled form logins and the new logout listener work. Kudo's to @henrikbjorn for taking the time to document CSRF validation for form login listeners (see [here](http://henrik.bjrnskov.dk/symfony2-cross-site-request-forgery/)). The [Logout CSRF Protection](http://www.yiiframework.com/wiki/190/logout-csrf-protection/) article on the Yii Framework wiki was also helpful in drafting this. --------------------------------------------------------------------------- by jmikola at 2011-12-31T07:50:31Z Odd that Travis CI reported a build failure for PHP 5.3.2, but both 5.3 and 5.4 passed: http://travis-ci.org/#!/jmikola/symfony/builds/463356 My local machine passes as well. --------------------------------------------------------------------------- by jmikola at 2012-02-06T20:05:30Z @schmittjoh: Please let me know your thoughts on the last commit. I think it would be overkill to add support for another handler service and/or error page just for logout exceptions. Perhaps as an alternative, we might just want to consider an invalid CSRF token on logout imply a false return value for `LogoutListener::requiresLogout()`. That would sacrifice the ability to handle the error separately (which a 403 response allows us), although we could still add logging (currently done in ExceptionListener). --------------------------------------------------------------------------- by jmikola at 2012-02-13T17:41:33Z @schmittjoh: ping --------------------------------------------------------------------------- by fabpot at 2012-02-14T23:36:22Z @jmikola: Instead of merging symfony/master, can you rebase? --------------------------------------------------------------------------- by jmikola at 2012-02-15T00:00:49Z Will do. --------------------------------------------------------------------------- by jmikola at 2012-02-15T00:05:48Z ``` [avocado: symfony] logout-csrf (+9/-216) $ git rebase master First, rewinding head to replay your work on top of it... Applying: [SecurityBundle] Add functional test for form login with CSRF token Applying: [Security] Refactor LogoutListener constructor to take options Applying: [Security] Allow LogoutListener to validate CSRF tokens Applying: [SecurityBundle] Templating helpers to generate logout URL's with CSRF tokens Applying: [SecurityBundle] Fix execution of functional tests with different names Applying: [SecurityBundle] Use assertCount() in tests Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Applying: [Security] Use LogoutException for invalid CSRF token in LogoutListener [avocado: symfony] logout-csrf (+7) $ git st # On branch logout-csrf # Your branch and 'origin/logout-csrf' have diverged, # and have 223 and 9 different commit(s) each, respectively. # nothing to commit (working directory clean) [avocado: symfony] logout-csrf (+7) $ ``` After rebasing, my merge commits disappeared. Is this normal? --------------------------------------------------------------------------- by stof at 2012-02-15T00:15:07Z Are you sure they disappeared ? Diverging from the remote branch is logical (you rewrote the history and so changed the commit id) but are you sure it does not have the commits on top of master ? Try ``git log master..logout-scrf`` If your commut are there, you simply need to force the push for the logout-csrf branch (take care to push only this branch during the force push to avoid messing all others as git won't warn you when asking to force) --------------------------------------------------------------------------- by stof at 2012-02-15T00:17:09Z ah sorry, you talked only about the merge commit. Yeah it is normal. When reapplying your commits on top of master, the merge commit are not kept as you are reapplying the changes linearly on top of the other branch (and deleting the merge commit was the reason why @fabpot asked you to rebase instead of merging btw) --------------------------------------------------------------------------- by jmikola at 2012-02-15T00:18:00Z The merge commits are not present in `git log master..logout-csrf`. Perhaps it used those merge commits when rebasing, as there were definitely conflicts resolved when I originally merged in symfony/master (@fabpot had made his own changes to LogoutListener). I'll force-push the changes to my PR brange. IIRC, GitHub is smart enough to preserve inline diff comments, provided they were made through the PR and not on the original commits. --------------------------------------------------------------------------- by jmikola at 2012-02-15T00:19:38Z That worked well. In the future, I think I'll stick to merging upstream in and then rebasing afterwards. Resolving conflicts is much easier during a merge than interactive rebase. --------------------------------------------------------------------------- by jmikola at 2012-02-23T18:46:13Z @fabpot @schmittjoh: Is there anything else I can do for this PR? I believe the exception was the only outstanding question (see: [this comment](#3007 (comment))).
2 parents af52362 + 49a8654 commit 294b57e

File tree

23 files changed

+951
-20
lines changed

23 files changed

+951
-20
lines changed

‎src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,9 @@ private function addFirewallsSection(ArrayNodeDefinition $rootNode, array $facto
200200
->treatTrueLike(array())
201201
->canBeUnset()
202202
->children()
203+
->scalarNode('csrf_parameter')->defaultValue('_csrf_token')->end()
204+
->scalarNode('csrf_provider')->cannotBeEmpty()->end()
205+
->scalarNode('intention')->defaultValue('logout')->end()
203206
->scalarNode('path')->defaultValue('/logout')->end()
204207
->scalarNode('target')->defaultValue('/')->end()
205208
->scalarNode('success_handler')->end()

‎src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -277,15 +277,24 @@ private function createFirewall(ContainerBuilder $container, $id, $firewall, &$a
277277
if (isset($firewall['logout'])) {
278278
$listenerId = 'security.logout_listener.'.$id;
279279
$listener = $container->setDefinition($listenerId, new DefinitionDecorator('security.logout_listener'));
280-
$listener->replaceArgument(2, $firewall['logout']['path']);
281-
$listener->replaceArgument(3, $firewall['logout']['target']);
280+
$listener->replaceArgument(2, array(
281+
'csrf_parameter' => $firewall['logout']['csrf_parameter'],
282+
'intention' => $firewall['logout']['intention'],
283+
'logout_path' => $firewall['logout']['path'],
284+
'target_url' => $firewall['logout']['target'],
285+
));
282286
$listeners[] = new Reference($listenerId);
283287

284288
// add logout success handler
285289
if (isset($firewall['logout']['success_handler'])) {
286290
$listener->replaceArgument(4, new Reference($firewall['logout']['success_handler']));
287291
}
288292

293+
// add CSRF provider
294+
if (isset($firewall['logout']['csrf_provider'])) {
295+
$listener->addArgument(new Reference($firewall['logout']['csrf_provider']));
296+
}
297+
289298
// add session logout handler
290299
if (true === $firewall['logout']['invalidate_session'] && false === $firewall['stateless']) {
291300
$listener->addMethodCall('addHandler', array(new Reference('security.logout.handler.session')));
@@ -304,6 +313,18 @@ private function createFirewall(ContainerBuilder $container, $id, $firewall, &$a
304313
foreach ($firewall['logout']['handlers'] as $handlerId) {
305314
$listener->addMethodCall('addHandler', array(new Reference($handlerId)));
306315
}
316+
317+
// register with LogoutUrlHelper
318+
$container
319+
->getDefinition('templating.helper.logout_url')
320+
->addMethodCall('registerListener', array(
321+
$id,
322+
$firewall['logout']['path'],
323+
$firewall['logout']['intention'],
324+
$firewall['logout']['csrf_parameter'],
325+
isset($firewall['logout']['csrf_provider']) ? new Reference($firewall['logout']['csrf_provider']) : null,
326+
))
327+
;
307328
}
308329

309330
// Authentication listeners

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,8 +78,7 @@
7878
<service id="security.logout_listener" class="%security.logout_listener.class%" public="false" abstract="true">
7979
<argument type="service" id="security.context" />
8080
<argument type="service" id="security.http_utils" />
81-
<argument /> <!-- Logout Path -->
82-
<argument /> <!-- Target-URL Path -->
81+
<argument /> <!-- Options -->
8382
<argument type="service" id="security.logout.success_handler" on-invalid="null" />
8483
</service>
8584
<service id="security.logout.handler.session" class="%security.logout.handler.session.class%" public="false" />

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,17 @@
55
xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">
66

77
<parameters>
8+
<parameter key="templating.helper.logout_url.class">Symfony\Bundle\SecurityBundle\Templating\Helper\LogoutUrlHelper</parameter>
89
<parameter key="templating.helper.security.class">Symfony\Bundle\SecurityBundle\Templating\Helper\SecurityHelper</parameter>
910
</parameters>
1011

1112
<services>
13+
<service id="templating.helper.logout_url" class="%templating.helper.logout_url.class%">
14+
<tag name="templating.helper" alias="logout_url" />
15+
<argument type="service" id="request" strict="false" />
16+
<argument type="service" id="router" />
17+
</service>
18+
1219
<service id="templating.helper.security" class="%templating.helper.security.class%">
1320
<tag name="templating.helper" alias="security" />
1421
<argument type="service" id="security.context" on-invalid="ignore" />

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,16 @@
55
xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">
66

77
<parameters>
8+
<parameter key="twig.extension.logout_url.class">Symfony\Bundle\SecurityBundle\Twig\Extension\LogoutUrlExtension</parameter>
89
<parameter key="twig.extension.security.class">Symfony\Bundle\SecurityBundle\Twig\Extension\SecurityExtension</parameter>
910
</parameters>
11+
1012
<services>
13+
<service id="twig.extension.logout_url" class="%twig.extension.logout_url.class%" public="false">
14+
<tag name="twig.extension" />
15+
<argument type="service" id="templating.helper.logout_url" />
16+
</service>
17+
1118
<service id="twig.extension.security" class="%twig.extension.security.class%" public="false">
1219
<tag name="twig.extension" />
1320
<argument type="service" id="security.context" on-invalid="ignore" />

‎src/Symfony/Bundle/SecurityBundle/Templating/Helper/LogoutUrlHelper.php

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
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\Bundle\SecurityBundle\Templating\Helper;
13+
14+
use Symfony\Component\Form\Extension\Csrf\CsrfProvider\CsrfProviderInterface;
15+
use Symfony\Component\HttpFoundation\Request;
16+
use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
17+
use Symfony\Component\Templating\Helper\Helper;
18+
19+
/**
20+
* LogoutUrlHelper provides generator functions for the logout URL.
21+
*
22+
* @author Jeremy Mikola <jmikola@gmail.com>
23+
*/
24+
class LogoutUrlHelper extends Helper
25+
{
26+
private $listeners;
27+
private $request;
28+
private $router;
29+
30+
/**
31+
* Constructor.
32+
*
33+
* @param Request $request A request instance
34+
* @param UrlGeneratorInterface $router A Router instance
35+
*/
36+
public function __construct(Request $request, UrlGeneratorInterface $router)
37+
{
38+
$this->request = $request;
39+
$this->router = $router;
40+
$this->listeners = array();
41+
}
42+
43+
/**
44+
* Registers a firewall's LogoutListener, allowing its URL to be generated.
45+
*
46+
* @param string $key The firewall key
47+
* @param string $logoutPath The path that starts the logout process
48+
* @param string $intention The intention for CSRF token generation
49+
* @param string $csrfParameter The CSRF token parameter name
50+
* @param CsrfProviderInterface $csrfProvider A CsrfProviderInterface instance
51+
*/
52+
public function registerListener($key, $logoutPath, $intention, $csrfParameter, CsrfProviderInterface $csrfProvider = null)
53+
{
54+
$this->listeners[$key] = array($logoutPath, $intention, $csrfParameter, $csrfProvider);
55+
}
56+
57+
/**
58+
* Generate the relative logout URL for the firewall.
59+
*
60+
* @param string $key The firewall key
61+
* @return string The relative logout URL
62+
*/
63+
public function getLogoutPath($key)
64+
{
65+
return $this->generateLogoutUrl($key, false);
66+
}
67+
68+
/**
69+
* Generate the absolute logout URL for the firewall.
70+
*
71+
* @param string $key The firewall key
72+
* @return string The absolute logout URL
73+
*/
74+
public function getLogoutUrl($key)
75+
{
76+
return $this->generateLogoutUrl($key, true);
77+
}
78+
79+
/**
80+
* Generate the logout URL for the firewall.
81+
*
82+
* @param string $key The firewall key
83+
* @param Boolean $absolute Whether to generate an absolute URL
84+
* @return string The logout URL
85+
* @throws InvalidArgumentException if no LogoutListener is registered for the key
86+
*/
87+
private function generateLogoutUrl($key, $absolute)
88+
{
89+
if (!array_key_exists($key, $this->listeners)) {
90+
throw new \InvalidArgumentException(sprintf('No LogoutListener found for firewall key "%s".', $key));
91+
}
92+
93+
list($logoutPath, $intention, $csrfParameter, $csrfProvider) = $this->listeners[$key];
94+
95+
$parameters = null !== $csrfProvider ? array($csrfParameter => $csrfProvider->generateCsrfToken($intention)) : array();
96+
97+
if ('/' === $logoutPath[0]) {
98+
$url = ($absolute ? $this->request->getUriForPath($logoutPath) : $this->request->getBasePath() . $logoutPath);
99+
100+
if (!empty($parameters)) {
101+
$url .= '?' . http_build_query($parameters);
102+
}
103+
} else {
104+
$url = $this->router->generate($logoutPath, $parameters, $absolute);
105+
}
106+
107+
return $url;
108+
}
109+
110+
/**
111+
* Returns the canonical name of this helper.
112+
*
113+
* @return string The canonical name
114+
*/
115+
public function getName()
116+
{
117+
return 'logout_url';
118+
}
119+
}

‎src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/CsrfFormLoginBundle/Controller/LoginController.php

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony framework.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* This source file is subject to the MIT license that is bundled
9+
* with this source code in the file LICENSE.
10+
*/
11+
12+
namespace Symfony\Bundle\SecurityBundle\Tests\Functional\Bundle\CsrfFormLoginBundle\Controller;
13+
14+
use Symfony\Component\DependencyInjection\ContainerAware;
15+
use Symfony\Component\HttpFoundation\Response;
16+
use Symfony\Component\Security\Core\SecurityContextInterface;
17+
use Symfony\Component\Security\Core\Exception\AccessDeniedException;
18+
19+
class LoginController extends ContainerAware
20+
{
21+
public function loginAction()
22+
{
23+
$form = $this->container->get('form.factory')->create('user_login');
24+
25+
return $this->container->get('templating')->renderResponse('CsrfFormLoginBundle:Login:login.html.twig', array(
26+
'form' => $form->createView(),
27+
));
28+
}
29+
30+
public function afterLoginAction()
31+
{
32+
return $this->container->get('templating')->renderResponse('CsrfFormLoginBundle:Login:after_login.html.twig');
33+
}
34+
35+
public function loginCheckAction()
36+
{
37+
return new Response('', 400);
38+
}
39+
40+
public function secureAction()
41+
{
42+
throw new \Exception('Wrapper', 0, new \Exception('Another Wrapper', 0, new AccessDeniedException()));
43+
}
44+
}

‎src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/CsrfFormLoginBundle/CsrfFormLoginBundle.php

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony framework.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* This source file is subject to the MIT license that is bundled
9+
* with this source code in the file LICENSE.
10+
*/
11+
12+
namespace Symfony\Bundle\SecurityBundle\Tests\Functional\Bundle\CsrfFormLoginBundle;
13+
14+
use Symfony\Component\HttpKernel\Bundle\Bundle;
15+
16+
class CsrfFormLoginBundle extends Bundle
17+
{
18+
}

‎src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/CsrfFormLoginBundle/Form/UserLoginFormType.php

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony framework.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* This source file is subject to the MIT license that is bundled
9+
* with this source code in the file LICENSE.
10+
*/
11+
12+
namespace Symfony\Bundle\SecurityBundle\Tests\Functional\Bundle\CsrfFormLoginBundle\Form;
13+
14+
use Symfony\Component\Form\AbstractType;
15+
use Symfony\Component\Form\FormBuilder;
16+
use Symfony\Component\Form\FormError;
17+
use Symfony\Component\Form\FormEvents;
18+
use Symfony\Component\Form\Event\FilterDataEvent;
19+
use Symfony\Component\HttpFoundation\Request;
20+
use Symfony\Component\Security\Core\SecurityContextInterface;
21+
22+
/**
23+
* Form type for use with the Security component's form-based authentication
24+
* listener.
25+
*
26+
* @author Henrik Bjornskov <henrik@bjrnskov.dk>
27+
* @author Jeremy Mikola <jmikola@gmail.com>
28+
*/
29+
class UserLoginFormType extends AbstractType
30+
{
31+
private $reqeust;
32+
33+
/**
34+
* @param Request $request A request instance
35+
*/
36+
public function __construct(Request $request)
37+
{
38+
$this->request = $request;
39+
}
40+
41+
/**
42+
* @see Symfony\Component\Form\AbstractType::buildForm()
43+
*/
44+
public function buildForm(FormBuilder $builder, array $options)
45+
{
46+
$builder
47+
->add('username', 'text')
48+
->add('password', 'password')
49+
->add('_target_path', 'hidden')
50+
;
51+
52+
$request = $this->request;
53+
54+
/* Note: since the Security component's form login listener intercepts
55+
* the POST request, this form will never really be bound to the
56+
* request; however, we can match the expected behavior by checking the
57+
* session for an authentication error and last username.
58+
*/
59+
$builder->addEventListener(FormEvents::SET_DATA, function (FilterDataEvent $event) use ($request) {
60+
if ($request->attributes->has(SecurityContextInterface::AUTHENTICATION_ERROR)) {
61+
$error = $request->attributes->get(SecurityContextInterface::AUTHENTICATION_ERROR);
62+
} else {
63+
$error = $request->getSession()->get(SecurityContextInterface::AUTHENTICATION_ERROR);
64+
}
65+
66+
if ($error) {
67+
$event->getForm()->addError(new FormError($error->getMessage()));
68+
}
69+
70+
$event->setData(array_replace((array) $event->getData(), array(
71+
'username' => $request->getSession()->get(SecurityContextInterface::LAST_USERNAME),
72+
)));
73+
});
74+
}
75+
76+
/**
77+
* @see Symfony\Component\Form\AbstractType::getDefaultOptions()
78+
*/
79+
public function getDefaultOptions(array $options)
80+
{
81+
/* Note: the form's intention must correspond to that for the form login
82+
* listener in order for the CSRF token to validate successfully.
83+
*/
84+
return array(
85+
'intention' => 'authenticate',
86+
);
87+
}
88+
89+
/**
90+
* @see Symfony\Component\Form\FormTypeInterface::getName()
91+
*/
92+
public function getName()
93+
{
94+
return 'user_login';
95+
}
96+
}

0 commit comments

Comments
 (0)
0