-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Security] Call logout handlers even if token is null #24489
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
Conversation
I'm not entirely sure what the actual issue is, but calling out logout listeners when you don't have a token feels weird |
The issue is listeners cannot set any token before the logout listener as it is registered before, so it is wrong to call the handlers based on the token being set. |
I edited the PR description, sorry but I was on my phone. |
*/ | ||
public function logout(Request $request, Response $response, TokenInterface $token) | ||
public function logout(Request $request, Response $response, TokenInterface $token = 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.
Changing the signature here is a BC break, same for all other.
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.
Right, for applications implementing their own logout handler using $token
in a case it is not set. I doubt anyone is concerned and I don't want to wait for Symfony flex (we won't be able to use it anyways) for a bug filed 4 years ago.
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.
My mistake I didn't realize updating the interface force every implementation to update its signature. So is there any way to fix this before Symfony flex? Maybe by setting a dummy token instead of null
?
Closed in favor of #24769 |
When listeners are registered in
SecurityExtension
first ones always areChannelListener
,ContextListener
(if stateful) andLogoutListener
. This means onlyContextListener
can set a token to trigger the logout handlers. This means theLogoutListener
is uselessAs said in #7104 (comment) we cannot register the
LogoutListener
last so a quick solution is to call the logout handlers wether a token is present or not in which case I pass aDummyToken
instead (see #24489 (comment)).