Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
linaori commentedOct 8, 2017
I'm not entirely sure what the actual issue is, but calling out logout listeners when you don't have a token feels weird |
MatTheCat commentedOct 8, 2017
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. |
MatTheCat commentedOct 9, 2017
I edited the PR description, sorry but I was on my phone. |
| * @param TokenInterface|null $token | ||
| */ | ||
| publicfunctionlogout(Request$request,Response$response,TokenInterface$token) | ||
| publicfunctionlogout(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 ofnull?
MatTheCat commentedOct 31, 2017
Closed in favor of#24769 |
Uh oh!
There was an error while loading.Please reload this page.
When listeners are registered in
SecurityExtensionfirst ones always areChannelListener,ContextListener(if stateful) andLogoutListener. This means onlyContextListenercan set a token to trigger the logout handlers. This means theLogoutListeneris uselessAs said in#7104 (comment) we cannot register the
LogoutListenerlast so a quick solution is to call the logout handlers wether a token is present or not in which case I pass aDummyTokeninstead (see#24489 (comment)).