Skip to content

[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

Open
wants to merge 1 commit into
base: 7.4
Choose a base branch
from

Conversation

jvasseur
Copy link
Contributor

@jvasseur jvasseur commented May 11, 2022

Q A
Branch? 7.3
Bug fix? no
New feature yes
Deprecations no
License MIT
Doc PR

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:

security:
    firewalls:
        main:
            json_login:
                check_path: auth_login
            logout:
                path: auth_logout
                target: ~
class AuthController extends AbstractController
{
    #[Route("/login", name: "auth_login")]
    public function login(): Response
    {
        return $this->json($this->getUser());
    }

    #[Route("/logout", name: "auth_logout")]
    public function logout(): Response
    {
        return $this->json(null);
    }
}

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.

@jvasseur jvasseur requested review from wouterj and chalasr as code owners May 11, 2022 12:21
@carsonbot carsonbot added this to the 6.1 milestone May 11, 2022
@jvasseur jvasseur force-pushed the allow-disabling-logout-redirect branch from 1bbe1d9 to 799d8ae Compare May 11, 2022 12:22
@jvasseur jvasseur changed the title [Security] Allow disabling redirect on login [Security] Allow disabling redirect on logout May 11, 2022
@jvasseur jvasseur force-pushed the allow-disabling-logout-redirect branch from 799d8ae to 9a52fd4 Compare May 11, 2022 12:23
@carsonbot
Copy link

Hey!

I think @scheb has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@wouterj wouterj modified the milestones: 6.1, 6.2 May 12, 2022
@jvasseur jvasseur force-pushed the allow-disabling-logout-redirect branch from 9a52fd4 to 4077513 Compare July 6, 2022 10:35
@nicolas-grekas nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022
@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@xabbuh xabbuh added the Feature label May 15, 2024
@xabbuh xabbuh modified the milestones: 7.1, 7.2 May 15, 2024
@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
Copy link
Contributor

@Spomky Spomky left a 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
Copy link
Contributor

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

@chalasr
Copy link
Member

chalasr commented Mar 8, 2025

👍 from me also, once rebased and retargeted.

@fabpot fabpot modified the milestones: 7.3, 7.4 May 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants