-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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); | ||
} | ||
|
||
parent::onKernelFinishRequest($event); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
|
@@ -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) | ||
{ | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should even use |
||
} | ||
} | ||
|
||
$context = null; | ||
} | ||
|
||
$this->listeners[$key] = array($logoutPath, $csrfTokenId, $csrfParameter, $csrfTokenManager, $context); | ||
} | ||
|
||
/** | ||
|
@@ -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(); | ||
|
||
|
@@ -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])) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps add |
||
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(); | ||
} | ||
} |
There was a problem hiding this comment.
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 ).There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 (seelist
and the upcomingisset
(where$key
can be null again ^^).There was a problem hiding this comment.
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
), andisset($this->listeners[$key])
wouldn't be an issue even with$key
as null.There was a problem hiding this comment.
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.