8000 [Security] Fix logout by MatTheCat · Pull Request #24805 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Security] Fix logout #24805

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 15, 2018
Merged

[Security] Fix logout #24805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 13 additions & 12 deletions src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,13 +222,14 @@ private function createFirewalls($config, ContainerBuilder $container)
$mapDef = $container->getDefinition('security.firewall.map');
$map = $authenticationProviders = array();
foreach ($firewalls as $name => $firewall) {
list($matcher, $listeners, $exceptionListener) = $this->createFirewall($container, $name, $firewall, $authenticationProviders, $providerIds);
list($matcher, $listeners, $exceptionListener, $logoutListener) = $this->createFirewall($container, $name, $firewall, $authenticationProviders, $providerIds);

$contextId = 'security.firewall.map.context.'.$name;
$context = $container->setDefinition($contextId, new DefinitionDecorator('security.firewall.context'));
$context
->replaceArgument(0, $listeners)
->replaceArgument(1, $exceptionListener)
->replaceArgument(2, $logoutListener)
;
$map[$contextId] = $matcher;
}
Expand Down Expand Up @@ -259,7 +260,7 @@ private function createFirewall(ContainerBuilder $container, $id, $firewall, &$a

// Security disabled?
if (false === $firewall['security']) {
return array($matcher, array(), null);
return array($matcher, array(), null, null);
}

// Provider id (take the first registered provider if none defined)
Expand All @@ -286,15 +287,15 @@ private function createFirewall(ContainerBuilder $container, $id, $firewall, &$a
}

// Logout listener
$logoutListenerId = null;
if (isset($firewall['logout'])) {
$listenerId = 'security.logout_listener.'.$id;
$listener = $container->setDefinition($listenerId, new DefinitionDecorator('security.logout_listener'));
$listener->replaceArgument(3, array(
$logoutListenerId = 'security.logout_listener.'.$id;
$logoutListener = $container->setDefinition($logoutListenerId, new DefinitionDecorator('security.logout_listener'));
$logoutListener->replaceArgument(3, array(
'csrf_parameter' => $firewall['logout']['csrf_parameter'],
'intention' => $firewall['logout']['csrf_token_id'],
'logout_path' => $firewall['logout']['path'],
));
$listeners[] = new Reference($listenerId);

// add logout success handler
if (isset($firewall['logout']['success_handler'])) {
Expand All @@ -304,16 +305,16 @@ private function createFirewall(ContainerBuilder $container, $id, $firewall, &$a
$logoutSuccessHandler = $container->setDefinition($logoutSuccessHandlerId, new DefinitionDecorator('security.logout.success_handler'));
$logoutSuccessHandler->replaceArgument(1, $firewall['logout']['target']);
}
$listener->replaceArgument(2, new Reference($logoutSuccessHandlerId));
$logoutListener->replaceArgument(2, new Reference($logoutSuccessHandlerId));

// add CSRF provider
if (isset($firewall['logout']['csrf_token_generator'])) {
$listener->addArgument(new Reference($firewall['logout']['csrf_token_generator']));
$logoutListener->addArgument(new Reference($firewall['logout']['csrf_token_generator']));
}

// add session logout handler
if (true === $firewall['logout']['invalidate_session'] && false === $firewall['stateless']) {
$listener->addMethodCall('addHandler', array(new Reference('security.logout.handler.session')));
$logoutListener->addMethodCall('addHandler', array(new Reference('security.logout.handler.session')));
}

// add cookie logout handler
Expand All @@ -322,12 +323,12 @@ private function createFirewall(ContainerBuilder $container, $id, $firewall, &$a
$cookieHandler = $container->setDefinition($cookieHandlerId, new DefinitionDecorator('security.logout.handler.cookie_clearing'));
$cookieHandler->addArgument($firewall['logout']['delete_cookies']);

$listener->addMethodCall('addHandler', array(new Reference($cookieHandlerId)));
$logoutListener->addMethodCall('addHandler', array(new Reference($cookieHandlerId)));
}

// add custom handlers
foreach ($firewall['logout']['handlers'] as $handlerId) {
$listener->addMethodCall('addHandler', array(new Reference($handlerId)));
$logoutListener->addMethodCall('addHandler', array(new Reference($handlerId)));
}

// register with LogoutUrlGenerator
Expand Down Expand Up @@ -362,7 +363,7 @@ private function createFirewall(ContainerBuilder $container, $id, $firewall, &$a
// Exception listener
$exceptionListener = new Reference($this->createExceptionListener($container, $firewall, $id, $configuredEntryPoint ?: $defaultEntryPoint, $firewall['stateless']));

return array($matcher, $listeners, $exceptionListener);
return array($matcher, $listeners, $exceptionListener, null !== $logoutListenerId ? new Reference($logoutListenerId) : null);
}

private function createContextListener($container, $contextKey)
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Bundle/SecurityBundle/Resources/config/security.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@
<service id="security.firewall.context" class="%security.firewall.context.class%" abstract="true">
<argument type="collection" />
<argument type="service" id="security.exception_listener" />
<argument /> <!-- LogoutListener -->
</service>

<service id="security.logout_url_generator" class="Symfony\Component\Security\Http\Logout\LogoutUrlGenerator" public="false">
Expand Down
7 changes: 5 additions & 2 deletions src/Symfony/Bundle/SecurityBundle/Security/FirewallContext.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Bundle\SecurityBundle\Security;

use Symfony\Component\Security\Http\Firewall\ExceptionListener;
use Symfony\Component\Security\Http\Firewall\LogoutListener;

/**
* This is a wrapper around the actual firewall configuration which allows us
Expand All @@ -23,15 +24,17 @@ class FirewallContext
{
private $listeners;
private $exceptionListener;
private $logoutListener;

public function __construct(array $listeners, ExceptionListener $exceptionListener = null)
public function __construct(array $listeners, ExceptionListener $exceptionListener = null, LogoutListener $logoutListener = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fun fact: on 3.4 & up, this class already has a 3rd argument: FirewallConfig $config
To prevent any funnier things, I suggest adding this argument in 2.7 (but ignoring its value).
WDYT? Any better idea?

Copy link
Contributor Author
@MatTheCat MatTheCat May 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do this on concerned branches only?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could change for a setter on 2.7, not sure it's better. Here is what you propose #27280

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I finally managed to merge, see 86a9c73#diff-cf0390616c24425d612bcf9ee1555111
@chalasr when doing your PR against master, please also deprecate passing a FirewallConfig as 3rd arg there.

{
$this->listeners = $listeners;
$this->exceptionListener = $exceptionListener;
$this->logoutListener = $logoutListener;
}

public function getContext()
{
return array($this->listeners, $this->exceptionListener);
return array($this->listeners, $this->exceptionListener, $this->logoutListener);
}
}
2 changes: 1 addition & 1 deletion src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,6 @@ public function getListeners(Request $request)
}
}

return array(array(), null);
return array(array(), null, null);
}
}
1 change: 0 additions & 1 deletion src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ public function testFirewalls()
array(),
array(
'security.channel_listener',
'security.logout_listener.secure',
'security.authentication.listener.x509.secure',
'security.authentication.listener.remote_user.secure',
'security.authentication.listener.form.secure',
Expand Down
34 changes: 34 additions & 0 deletions src/Symfony/Bundle/SecurityBundle/Tests/Functional/LogoutTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?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\Bundle\SecurityBundle\Tests\Functional;

class LogoutTest extends WebTestCase
{
public function testSessionLessRememberMeLogout()
{
$client = $this->createClient(array('test_case' => 'RememberMeLogout', 'root_config' => 'config.yml'));

$client->request('POST', '/login', array(
'_username' => 'johannes',
'_password' => 'test',
));

$cookieJar = $client->getCookieJar();
$cookieJar->expire(session_name());

$this->assertNotNull($cookieJar->get('REMEMBERME'));

$client->request('GET', '/logout');

$this->assertNull($cookieJar->get('REMEMBERME'));
}
}

This comment was marked as resolved.

Sign in to view

18 changes: 18 additions & 0 deletions src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/RememberMeLogout/bundles.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?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.
*/

use Symfony\Bundle\SecurityBundle\SecurityBundle;
use Symfony\Bundle\FrameworkBundle\FrameworkBundle;

return array(
new FrameworkBundle(),
new SecurityBundle(),
< 10000 span class='blob-code-inner blob-code-marker ' data-code-marker="+">);
25 changes: 25 additions & 0 deletions src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/RememberMeLogout/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
imports:
- { resource: ./../config/framework.yml }

security:
encoders:
Symfony\Component\Security\Core\User\User: plaintext

providers:
in_memory:
memory:
users:
johannes: { password: test, roles: [ROLE_USER] }

firewalls:
default:
form_login:
check_path: login
remember_me: true
require_previous_session: false
remember_me:
always_remember_me: true
key: key
logout: ~
anonymous: ~
stateless: true
5 changes: 5 additions & 0 deletions src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/RememberMeLogout/routing.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
login:
path: /login

logout:
path: /logout
2 changes: 1 addition & 1 deletion src/Symfony/Bundle/SecurityBundle/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"require": {
"php": ">=5.3.9",
"ext-xml": "*",
"symfony/security": "~2.7.38|~2.8.31",
"symfony/security": "~2.7.47|~2.8.40",
"symfony/security-acl": "~2.7",
"symfony/http-kernel": "~2.7"
},
Expand Down
13 changes: 11 additions & 2 deletions src/Symfony/Component/Security/Http/Firewall.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,20 +47,29 @@ public function onKernelRequest(GetResponseEvent $event)
}

// register listeners for this firewall
list($listeners, $exceptionListener) = $this->map->getListeners($event->getRequest());
$listeners = $this->map->getListeners($event->getRequest());

$authenticationListeners = $listeners[0];
$exceptionListener = $listeners[1];
$logoutListener = isset($listeners[2]) ? $listeners[2] : null;

if (null !== $exceptionListener) {
$this->exceptionListeners[$event->getRequest()] = $exceptionListener;
$exceptionListener->register($this->dispatcher);
}

// initiate the listener chain
foreach ($listeners as $listener) {
foreach ($authenticationListeners as $listener) {
$listener->handle($event);

if ($event->hasResponse()) {
break;
}
}

if (null !== $logoutListener) {
$logoutListener->handle($event);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍

}
}

public function onKernelFinishRequest(FinishRequestEvent $event)
Expand Down
9 changes: 5 additions & 4 deletions src/Symfony/Component/Security/Http/FirewallMap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use Symfony\Component\HttpFoundation\RequestMatcherInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Security\Http\Firewall\ExceptionListener;
use Symfony\Component\Security\Http\Firewall\LogoutListener;

/**
* FirewallMap allows configuration of different firewalls for specific parts
Expand All @@ -25,9 +26,9 @@ class FirewallMap implements FirewallMapInterface
{
private $map = array();

public function add(RequestMatcherInterface $requestMatcher = null, array $listeners = array(), ExceptionListener $exceptionListener = null)
public function add(RequestMatcherInterface $requestMatcher = null, array $listeners = array(), ExceptionListener $exceptionListener = null, LogoutListener $logoutListener = null)
{
$this->map[] = array($requestMatcher, $listeners, $exceptionListener);
$this->map[] = array($requestMatcher, $listeners, $exceptionListener, $logoutListener);
}

/**
Expand All @@ -37,10 +38,10 @@ public function getListeners(Request $request)
{
foreach ($this->map as $elements) {
if (null === $elements[0] || $elements[0]->matches($request)) {
return array($elements[1], $elements[2]);
return array($elements[1], $elements[2], $elements[3]);
}
}

return array(array(), null);
return array(array(), null, null);
}
}
0