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#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.
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
| * cookies, etc. | ||
| */ | ||
| 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.
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.
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.
Please don't close :) While it aims to achieve the same goal, it's fine to rework a PR.
MatTheCatOct 31, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
I thought we could have BC breaks between major versions? That's why I closed#24489 which was based on 2.7
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.
Not without proper upgrade path, i.e. noticing about the breaking change, that is not possible from an interface
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.
I presume it's too late for an upgrade path now?
linaori commentedNov 1, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There will be a lot of code that makes assumptions of always having a valid & authenticated token in here. I think this will be a BC break with behavior that people have not anticipated due to those assumptions, and can potentially break a lot. Would require a carefully crafted BC layer. If it's a bug fix, you might be able to get it in, but it seems to be a new feature, which requires a very clear notice about the changes in behavior (and possibly won't make it into 3.4). |
MatTheCat commentedNov 1, 2017
Well I opened this PR and#24489 in order to have a quick fix for 3.4 or 4.0 but if it's not possible I'll try to fix the root issue which is the listeners registration order. |
ebuildy commentedDec 21, 2017
What about create a dummy Token if $token is null? |
MatTheCat commentedDec 21, 2017
ebuildy commentedDec 22, 2017
Thanks you very much for your work, I hope this one will be merged. I must release a project tonight and find a quick/dirty workaround, do you have any recommandation? thanks you. |
My previous attempt tofix#7104 without any BC break was quite ugly so here we are.
As said before the logout handlers are currently not invoked if the security token is null. Problem is, the firewall listeners registration order only allows
ContextListenerto set a token beforeLogoutListener. This means any stateless firewall cannot benefit from thelogoutoption, which is quite ironic as we have aCookieClearingLogoutHandler.None of the Symfony logout handlers use
logout's$tokenparameter so I thought about removing it but in the meantime I just allowed it to benullin order to mitigate possible BC breaks.I really would like this to be fixed with Symfony 4!