-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Security] Allow disabling redirect on logout #46320
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
base: 7.4
Are you sure you want to change the base?
Conversation
1bbe1d9
to
799d8ae
Compare
799d8ae
to
9a52fd4
Compare
Hey! I think @scheb has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
9a52fd4
to
4077513
Compare
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.
Hi @jvasseur,
Thank you for the proposed changes. This looks good to me.
Just a few changes rebase and we are good to go.
@@ -8,6 +8,7 @@ CHANGELOG | |||
* Deprecate the `Symfony\Component\Security\Core\Security` service alias, use `Symfony\Bundle\SecurityBundle\Security\Security` instead | |||
* Add `Security::getFirewallConfig()` to help to get the firewall configuration associated to the Request | |||
* Add `Security::login()` to login programmatically | |||
* Allow disabling the redirection on successful logout by passing `null` to the `target` option |
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.
Should move to 7.3
👍 from me also, once rebased and retargeted. |
When using JSON authentication, customizing the success response can be done easily in the corresponding controller, but customizing the logout success is more tiresome and requires to create a listener for it (while still requiring to create a route for it to work).
This PR propose to add the possibility to disable the redirect on successful logout and let the request fallback to the defined route in this case.
This allow customizing both login and logout response in the same place like this:
It means I had to remove the Exception in case no logout listener set a response but I don't think it should be considered a BC break since it's an error case that is removed and the corresponding exception was just the base
RuntimeException
and thus wasn't specifically catchable.The only downside is that other logout listeners can't modify the response anymore because it doesn't exists in the event but since this is an opt-in feature I don't think it's a problem.