Skip to content

[Security][SecurityBundle] Enhance automatic logout url generation #20516

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.

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
Mar 22, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions UPGRADE-3.3.md
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,9 @@ Security
* The `RoleInterface` has been deprecated. Extend the `Symfony\Component\Security\Core\Role\Role`
class in your custom role implementations instead.

* The `LogoutUrlGenerator::registerListener()` method will expect a 6th `$context = null` argument in 4.0.
Define the argument when overriding this method.

SecurityBundle
--------------

Expand Down
2 changes: 2 additions & 0 deletions UPGRADE-4.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,8 @@ Security

* The `RoleInterface` has been removed. Extend the `Symfony\Component\Security\Core\Role\Role`
class instead.

* The `LogoutUrlGenerator::registerListener()` method expects a 6th `$context = null` argument.

SecurityBundle
--------------
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,7 @@ private function createFirewall(ContainerBuilder $container, $id, $firewall, &$a
$firewall['logout']['csrf_token_id'],
$firewall['logout']['csrf_parameter'],
isset($firewall['logout']['csrf_token_generator']) ? new Reference($firewall['logout']['csrf_token_generator']) : null,
false === $firewall['stateless'] && isset($firewall['context']) ? $firewall['context'] : null,
))
;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
<?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\EventListener;

use Symfony\Bundle\SecurityBundle\Security\FirewallMap;
use Symfony\Component\EventDispatcher\EventDispatcherInterface;
use Symfony\Component\HttpKernel\Event\FinishRequestEvent;
use Symfony\Component\HttpKernel\Event\GetResponseEvent;
use Symfony\Component\Security\Http\Firewall;
use Symfony\Component\Security\Http\FirewallMapInterface;
use Symfony\Component\Security\Http\Logout\LogoutUrlGenerator;

/**
* @author Maxime Steinhausser <maxime.steinhausser@gmail.com>
*/
class FirewallListener extends Firewall
{
private $map;
private $logoutUrlGenerator;

public function __construct(FirewallMapInterface $map, EventDispatcherInterface $dispatcher, LogoutUrlGenerator $logoutUrlGenerator)
{
$this->map = $map;
$this->logoutUrlGenerator = $logoutUrlGenerator;

parent::__construct($map, $dispatcher);
}

public function onKernelRequest(GetResponseEvent $event)
{
if (!$event->isMasterRequest()) {
return;
}

if ($this->map instanceof FirewallMap && $config = $this->map->getFirewallConfig($event->getRequest())) {
$this->logoutUrlGenerator->setCurrentFirewall($config->getName(), $config->getContext());
}

parent::onKernelRequest($event);
}

public function onKernelFinishRequest(FinishRequestEvent $event)
{
if ($event->isMasterRequest()) {
$this->logoutUrlGenerator->setCurrentFirewall(null);
Copy link
Contributor

@ro0NL ro0NL Nov 20, 2016

Choose a reason for hiding this comment

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

This sets array(null, null) though, lets allow it and unset $currentFirewall in that case (yes, ignoring $context if so.. perhaps throw ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplified by 0c5628c?w=1

Copy link
Contributor

Choose a reason for hiding this comment

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

0bb5dd7 was good? Now $currentFirewall can be null, an array, an array with nulls. Which isnt checked for accordingly (see list and the upcoming isset (where $key can be null again ^^).

Copy link
Contributor Author

@ogizanagi ogizanagi Nov 20, 2016

Choose a reason for hiding this comment

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

IMHO, it doesn't matter much how the current firewall informations are stored internally (in this case, it aims to simplify the code. The multiple possible values aren't an issue, as we're expecting to use it with list), and isset($this->listeners[$key]) wouldn't be an issue even with $key as null.

Copy link
Contributor

@ro0NL ro0NL Nov 20, 2016

Choose a reason for hiding this comment

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

Ok, list($a, $b) = null doesnt crashes :) Fair enough. I'd prefer the previous commit as it was explicit.

Just saying we run code that's not actually needed. And we implicitly allow setCurrentFirewall(null, 'context') which im not sure is intended / should be allowed.

}

parent::onKernelFinishRequest($event);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,11 @@


<!-- Firewall related services -->
<service id="security.firewall" class="Symfony\Component\Security\Http\Firewall">
<service id="security.firewall" class="Symfony\Bundle\SecurityBundle\EventListener\FirewallListener">
<tag name="kernel.event_subscriber" />
<argument type="service" id="security.firewall.map" />
<argument type="service" id="event_dispatcher" />
<argument type="service" id="security.logout_url_generator" />
</service>

<service id="security.firewall.map" class="Symfony\Bundle\SecurityBundle\Security\FirewallMap" public="false">
Expand Down
109 changes: 83 additions & 26 deletions src/Symfony/Component/Security/Http/Logout/LogoutUrlGenerator.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
use Symfony\Component\Security\Core\Authentication\Token\AnonymousToken;
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface;

Expand All @@ -28,6 +29,7 @@ class LogoutUrlGenerator
private $router;
private $tokenStorage;
private $listeners = array();
private $currentFirewall;

public function __construct(RequestStack $requestStack = null, UrlGeneratorInterface $router = null, TokenStorageInterface $tokenStorage = null)
{
Expand All @@ -39,15 +41,29 @@ public function __construct(RequestStack $requestStack = null, UrlGeneratorInter
/**
* Registers a firewall's LogoutListener, allowing its URL to be generated.
*
* @param string $key The firewall key
* @param string $logoutPath The path that starts the logout process
* @param string $csrfTokenId The ID of the CSRF token
* @param string $csrfParameter The CSRF token parameter name
* @param CsrfTokenManagerInterface $csrfTokenManager A CsrfTokenManagerInterface instance
* @param string $key The firewall key
* @param string $logoutPath The path that starts the logout process
* @param string $csrfTokenId The ID of the CSRF token
* @param string $csrfParameter The CSRF token parameter name
* @param CsrfTokenManagerInterface|null $csrfTokenManager A CsrfTokenManagerInterface instance
* @param string|null $context The listener context
*/
public function registerListener($key, $logoutPath, $csrfTokenId, $csrfParameter, CsrfTokenManagerInterface $csrfTokenManager = null)
public function registerListener($key, $logoutPath, $csrfTokenId, $csrfParameter, CsrfTokenManagerInterface $csrfTokenManager = null/*, $context = null*/)
{
$this->listeners[$key] = array($logoutPath, $csrfTokenId, $csrfParameter, $csrfTokenManager);
if (func_num_args() >= 6) {
$context = func_get_arg(5);
} else {
if (__CLASS__ !== get_class($this)) {
$r = new \ReflectionMethod($this, __FUNCTION__);
if (__CLASS__ !== $r->getDeclaringClass()->getName()) {
@trigger_error(sprintf('Method %s() will have a sixth `$context = null` argument in version 4.0. Not defining it is deprecated since 3.3.', get_class($this), __FUNCTION__), E_USER_DEPRECATED);
Copy link
Contributor

Choose a reason for hiding this comment

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

Method %s::%s() ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should even use __METHOD__ instead. (671694d#diff-98543741515f0201292fdfa41ea3c412R97)

}
}

$context = null;
}

$this->listeners[$key] = array($logoutPath, $csrfTokenId, $csrfParameter, $csrfTokenManager, $context);
}

/**
Expand All @@ -74,35 +90,26 @@ public function getLogoutUrl($key = null)
return $this->generateLogoutUrl($key, UrlGeneratorInterface::ABSOLUTE_URL);
}

/**
* @param string|null $key The current firewall key
* @param string|null $context The current firewall context
*/
public function setCurrentFirewall($key, $context = null)
{
$this->currentFirewall = array($key, $context);
}

/**
* Generates the logout URL for the firewall.
*
* @param string|null $key The firewall key or null to use the current firewall key
* @param int $referenceType The type of reference (one of the constants in UrlGeneratorInterface)
*
* @return string The logout URL
*
* @throws \InvalidArgumentException if no LogoutListener is registered for the key or the key could not be found automatically.
*/
private function generateLogoutUrl($key, $referenceType)
{
// Fetch the current provider key from token, if possible
if (null === $key && null !== $this->tokenStorage) {
$token = $this->tokenStorage->getToken();
if (null !== $token && method_exists($token, 'getProviderKey')) {
$key = $token->getProviderKey();
}
}

if (null === $key) {
throw new \InvalidArgumentException('Unable to find the current firewall LogoutListener, please provide the provider key manually.');
}

if (!array_key_exists($key, $this->listeners)) {
throw new \InvalidArgumentException(sprintf('No LogoutListener found for firewall key "%s".', $key));
}

list($logoutPath, $csrfTokenId, $csrfParameter, $csrfTokenManager) = $this->listeners[$key];
list($logoutPath, $csrfTokenId, $csrfParameter, $csrfTokenManager) = $this->getListener($key);

$parameters = null !== $csrfTokenManager ? array($csrfParameter => (string) $csrfTokenManager->getToken($csrfTokenId)) : array();

Expand All @@ -128,4 +135,54 @@ private function generateLogoutUrl($key, $referenceType)

return $url;
}

/**
* @param string|null $key The firewall key or null use the current firewall key
*
* @return array The logout listener found
*
* @throws \InvalidArgumentException if no LogoutListener is registered for the key or could not be found automatically.
*/
private function getListener($key)
{
if (null !== $key) {
if (isset($this->listeners[$key])) {
return $this->listeners[$key];
}

throw new \InvalidArgumentException(sprintf('No LogoutListener found for firewall key "%s".', $key));
}

// Fetch the current provider key from token, if possible
if (null !== $this->tokenStorage) {
$token = $this->tokenStorage->getToken();

if ($token instanceof AnonymousToken) {
throw new \InvalidArgumentException('Unable to generate a logout url for an anonymous token.');
}

if (null !== $token && method_exists($token, 'getProviderKey')) {
$key = $token->getProviderKey();

if (isset($this->listeners[$key])) {
return $this->listeners[$key];
}
}
}

// Fetch from injected current firewall information, if possible
list($key, $context) = $this->currentFirewall;

if (isset($this->listeners[$key])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add null !== $key to be explicit.

return $this->listeners[$key];
}

foreach ($this->listeners as $listener) {
if (isset($listener[4]) && $context === $listener[4]) {
return $listener;
}
}

throw new \InvalidArgumentException('Unable to find the current firewall LogoutListener, please provide the provider key manually.');
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
<?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\Tests\Logout;

use PHPUnit\Framework\TestCase;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\Security\Core\Authentication\Token\AnonymousToken;
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
use Symfony\Component\Security\Http\Logout\LogoutUrlGenerator;

/**
* @author Maxime Steinhausser <maxime.steinhausser@gmail.com>
*/
class LogoutUrlGeneratorTest extends TestCase
{
/** @var TokenStorage */
private $tokenStorage;
/** @var LogoutUrlGenerator */
private $generator;

protected function setUp()
{
$requestStack = $this->getMockBuilder(RequestStack::class)->getMock();
$request = $this->getMockBuilder(Request::class)->getMock();
$requestStack->method('getCurrentRequest')->willReturn($request);

$this->tokenStorage = new TokenStorage();
$this->generator = new LogoutUrlGenerator($requestStack, null, $this->tokenStorage);
}

public function testGetLogoutPath()
{
$this->generator->registerListener('secured_area', '/logout', null, null);

$this->assertSame('/logout', $this->generator->getLogoutPath('secured_area'));
}

/**
* @expectedException \InvalidArgumentException
* @expectedExceptionMessage No LogoutListener found for firewall key "unregistered_key".
*/
public function testGetLogoutPathWithoutLogoutListenerRegisteredForKeyThrowsException()
{
$this->generator->registerListener('secured_area', '/logout', null, null, null);

$this->generator->getLogoutPath('unregistered_key');
}

public function testGuessFromToken()
{
$this->tokenStorage->setToken(new UsernamePasswordToken('user', 'password', 'secured_area'));
$this->generator->registerListener('secured_area', '/logout', null, null);

$this->assertSame('/logout', $this->generator->getLogoutPath());
}

/**
* @expectedException \InvalidArgumentException
* @expectedExceptionMessage Unable to generate a logout url for an anonymous token.
*/
public function testGuessFromAnonymousTokenThrowsException()
{
$this->tokenStorage->setToken(new AnonymousToken('default', 'anon.'));

$this->generator->getLogoutPath();
}

public function testGuessFromCurrentFirewallKey()
{
$this->generator->registerListener('secured_area', '/logout', null, null);
$this->generator->setCurrentFirewall('secured_area');

$this->assertSame('/logout', $this->generator->getLogoutPath());
}

public function testGuessFromCurrentFirewallContext()
{
$this->generator->registerListener('secured_area', '/logout', null, null, null, 'secured');
$this->generator->setCurrentFirewall('admin', 'secured');

$this->assertSame('/logout', $this->generator->getLogoutPath());
}

public function testGuessFromTokenWithoutProviderKeyFallbacksToCurrentFirewall()
{
$this->tokenStorage->setToken($this->getMockBuilder(TokenInterface::class)->getMock());
$this->generator->registerListener('secured_area', '/logout', null, null);
$this->generator->setCurrentFirewall('secured_area');

$this->assertSame('/logout', $this->generator->getLogoutPath());
}

/**
* @expectedException \InvalidArgumentException
* @expectedExceptionMessage Unable to find the current firewall LogoutListener, please provide the provider key manually
*/
public function testUnableToGuessThrowsException()
{
$this->generator->registerListener('secured_area', '/logout', null, null);

$this->generator->getLogoutPath();
}
}