[Security] Refactor logout listener to dispatch an event instead

[wouterj]

Q	A
Branch?	master
Bug fix?	yes (sort of...)
New feature?	yes
Deprecations?	yes
Tickets	Fix #25212, Fix #22473
License	MIT
Doc PR	tbd

The current LogoutListener has some extension points, but they are not really DX-friendly (ref #25212). It requires hacking a addMethodCall('addHandler') in the container builder to register a custom logout handler.
Also, it is impossible to overwrite the default logout functionality from a bundle (ref #22473).

This PR introduces a LogoutEvent that replaces both the LogoutSuccessHandlerInterface and LogoutHandlerInterface. This provides a DX-friendly extension point and also cleans up the authentication factories (no more addMethodCall()'s).

In order to allow different logout handlers for different firewalls, I created a specific event dispatcher for each firewall (as also shortly discussed in #33558). The dispatcher tag attribute allows you to specify which dispatcher it should be registered to (defaulting to the global dispatcher). The EventBubblingLogoutListener also dispatches logout events on the global dispatcher, to be used for listeners that should run on all firewalls.

@weaverryan and I discussed this feature while working on #33558, but figured it was unrelated and could be done while preservering BC. So that's why a separate PR is created.

[wouterj]

Please note that the fabbot exception message error is a false-positive

[nicolas-grekas]

Here is a superficial review.

[linaori]

I think this is a really nice change, and I hope that adding this to the event listener documentation will also make this behavior more accessible to developers 😄

[linaori]

I feel like the second part of the exception doesn't really belong to a run-time exception: "make sure at least the DefaultLogoutListener is registered."

What if we reword it to something in form of "Register the DefaultLogoutListener as LogoutEvent listener when no other listener is supposed to give a response"? Probably not the best either, though it does give less of a "you should've registered X" feeling.

[trsteel88]

+1

I've never understood why the security component uses Handlers instead of events. I'd love to see it using events for everything. For example:

LogoutEvent -> Replaces the need to override the logout handler
AuthenticationFailureEvent -> Replaces the need to override the authentication failure handler
AuthenticationSuccessEvent -> Replaces the need to override the authentication success handler

[fabpot]

if (!$dispatcherDefinitions) {

[fabpot]

Thank you @wouterj.

[Aenos85]

Hello, I have two small problems with the new functionality.

1. i have used the success_handler to get a json response and not a redirect (because of an API call). How can I realize this with the event listener?

2. when i implement the LogoutEvent i get an error in the UnitTest that "Symfony\Component\HttpFoundation\Request" would not be a service (autowiring via extended LogoutEvent)

[stof]

1. i have used the success_handler to get a json response and not a redirect (because of an API call).  How can I realize this with the event listener?

The LogoutEvent accepts any Response, not only RedirectResponse. You could set your own response

2\. when i implement the LogoutEvent i get an error in the UnitTest that "Symfony\Component\HttpFoundation\Request" would not be a service (autowiring via extended LogoutEvent)

What do you mean exactly ?
Btw, this looks like you are registering LogoutEvent as a service that gets autowired.That does not make sense. The event object is a value object, not a service.

[Aenos85]

I implemented all like it was written in "https://symfony.com/blog/new-in-symfony-5-1-simpler-logout-customization"

Reply 1.
I use the following code but the response is a 304 with a redirect

public function onSymfonyComponentSecurityHttpEventLogoutEvent()
{
    $this->setResponse(new JsonResponse(["success" => true]));
}

Reply 2.
In the UnitTest I get the following Message:

Symfony\Component\DependencyInjection\Exception\RuntimeException: Cannot autowire service "App\EventListener\LogoutListener": argument "$request" of method "Symfony\Component\Security\Http\Event\LogoutEvent::__construct()" references class "Symfony\Component\HttpFoundation\Request" but no such service exists.

I made a workaround for me

public function __construct(RequestStack $requestStack, ?TokenInterface $token)
{
    parent::__construct($requestStack->getCurrentRequest(), $token);
}

[stof]

without seeing your App\EventListener\LogoutListener, we cannot help you there.

Also, using $requestStack->getCurrentRequest() in the constructor of a service is the wrong way to use the RequestStack. It is totally possible that the constructor is called at the time where the stack is empty (and it will always be the case when using the CLI as there is no processed request there). And the current request will change over time.
$requestStack->getCurrentRequest() is meant to be called at the time you need to use the request. Introducing the RequestStack was done precisely to solve problems linked to defining the Request as a service in Symfony 2.x. Getting the request in the constructor reintroduces all these problems (and is even worse, as it does not provide any of the solutions which were implemented in Symfony 2.x for them).

[Aenos85]

I agree with you, and in my case I do not need the request. But the class "Symfony\Component\Security\Http\Event\LogoutEvent" is the problem here the constructor is made and it gets the request service. The RequestStack is just a workaround for the listener in the CLI to work at all.

I have already shown my "App\EventListener\LogoutListener" completely, but here again.

And unfortunately the setResponse does not work either.

<?php


namespace App\EventListener;

use Symfony\Component\HttpFoundation\JsonResponse;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Http\Event\LogoutEvent;


class LogoutListener extends LogoutEvent
{
    public function __construct(RequestStack $requestStack, ?TokenInterface $token)
    {
        parent::__construct($requestStack->getCurrentRequest(), $token);
    }

    public function onSymfonyComponentSecurityHttpEventLogoutEvent()
    {
        ...
        $this->setResponse(new JsonResponse(["success" => true]));
    }
}

The LogoutEvent class have the Problem, withot the following code I would delete me constructor.

class LogoutEvent extends Event
{
    private $request;
    private $response;
    private $token;

    public function __construct(Request $request, ?TokenInterface $token)
    {
        $this->request = $request;
        $this->token = $token;
    }
...

[stof]

LogoutListener should not extend LogoutEvent. the listener and the event are 2 different concepts.

LogoutEvent should be the type of the argument of the method in your listener.

[wouterj]

Please read the documentation of the event dispatcher for more information: https://symfony.com/doc/current/event_dispatcher.html

[Aenos85]

Okay, I got it. Sorry for the confusion, my bad.

[ndantonio]

Hi, I'm new with Symfony. I would like to ask for your help if you can give me an example of using this Logout Event on a listener or a subscriber? What I want to do is if my session expires then it will automatically logout and redirect to login page.

Thank you in advance!

[alexander-schranz]

@wouterj

I'm currently working on the FosHttpCacheBundle which has a LogoutHandler but since Symfony 5.1 this seems not longer be called in the test:
https://travis-ci.org/github/FriendsOfSymfony/FOSHttpCacheBundle/jobs/714724517

For me it looks like it has todo with this change, but I'm not 100% sure. For the Bundle its not important as that service is deprecated and the thing handled by another listener but maybe it could be important for other project when the LogoutHandler is not called.

[wouterj]

That's because it's registered in a weird way: (please note that there was no non-hacky way to add logout handlers)

$container->setAlias('security.logout.handler.session', 'fos_http_cache.user_context.session_logout_handler');

All internal handlers are now logout listeners, so "overriding" a core handler service ID no longer works. I'm not sure if internal core service IDs are covered by the BC promise (cc @nicolas-grekas please confirm).

[TomasVotruba]

@ndantonio Hi, you can use it like this: https://github.com/wouterj/symfony/blob/a9f096eb1f1fe687596cf62834d6c5efc003ec2a/src/Symfony/Component/Security/Http/EventListener/CsrfTokenClearingLogoutListener.php