8000 [Security] Call logout handlers even if token is null by MatTheCat · Pull Request #24769 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Security] Call logout handlers even if token is null #24769

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

Closed
wants to merge 1 commit into from
Closed

[Security] Call logout handlers even if token is null #24769

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
7 changes: 3 additions & 4 deletions src/Symfony/Component/Security/Http/Firewall/LogoutListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,9 @@ public function handle(GetResponseEvent $event)
}

// handle multiple logout attempts gracefully
if ($token = $this->tokenStorage->getToken()) {
foreach ($this->handlers as $handler) {
$handler->logout($request, $response, $token);
}
$token = $this->tokenStorage->getToken();
foreach ($this->handlers as $handler) {
$handler->logout($request, $response, $token);
}

$this->tokenStorage->setToken(null);
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Security/Http/Logout/CookieClearingLogoutHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public function __construct(array $cookies)
/**
* Implementation for the LogoutHandlerInterface. Deletes all requested cookies.
*/
public function logout(Request $request, Response $response, TokenInterface $token)
public function logout(Request $request, Response $response, TokenInterface $token = null)
{
foreach ($this->cookies as $cookieName => $cookieData) {
$response->headers->clearCookie($cookieName, $cookieData['path'], $cookieData['domain']);
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Security/Http/Logout/LogoutHandlerInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,5 @@ interface LogoutHandlerInterface
* to be logged out. Usually, you would unset session variables, or remove
* cookies, etc.
*/
public function logout(Request $request, Response $response, TokenInterface $token);
public function logout(Request $request, Response $response, TokenInterface $token = null);
Copy link
Member

Choose a reason for hiding this comment

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

That would fatally break any existing logout handler, not doable.
Although it's not very sexy, passing a dummy token is better as it doesn't break (I would not call it DummyToken though, we need a meaningful name), and it seems to be the best alternative we have for fixing this bug.

Copy link
Member

Choose a reason for hiding this comment

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

Please don't close :) While it aims to achieve the same goal, it's fine to rework a PR.

Copy link
Contributor Author
@MatTheCat MatTheCat Oct 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I thought we could have BC breaks between major versions? That's why I closed #24489 which was based on 2.7

Copy link
Member

Choose a reason for hiding this comment

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

Not without proper upgrade path, i.e. noticing about the breaking change, that is not possible from an interface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I presume it's too late for an upgrade path now?

}
2 changes: 1 addition & 1 deletion src/Symfony/Component/Security/Http/Logout/SessionLogoutHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class SessionLogoutHandler implements LogoutHandlerInterface
/**
* Invalidate the current session.
*/
public function logout(Request $request, Response $response, TokenInterface $token)
public function logout(Request $request, Response $response, TokenInterface $token = null)
{
$request->getSession()->invalidate();
}
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Security/Http/RememberMe/AbstractRememberMeServices.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ final public function autoLogin(Request $request)
/**
* Implementation for LogoutHandlerInterface. Deletes the cookie.
*/
public function logout(Request $request, Response $response, TokenInterface $token)
public function logout(Request $request, Response $response, TokenInterface $token = null)
{
$this->cancelCookie($request);
}
Expand Down
0