[Security] Logout success_handler is called **before** the actual logout happens

[ThomasLandauer]

Symfony version(s) affected: 4.4.5

Description
Logout success_handler is called before the actual logout happens.

Here's the problem:
https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/Security/Http/Firewall/LogoutListener.php#L95

The Response is created too early, since the actual logout is only happening below.

Reference at https://symfony.com/doc/4.4/reference/configuration/security.html#success-handler is saying that this is for "handling a successful logout." (Which is also in line with my understanding of a "success handler" and the semantics of onLogoutSuccess.) But this is not the case, cause until it has actually happened, there is no successful logout (but merely a pending logout).

So I'm suggesting to move $this->tokenStorage->setToken(null); https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/Security/Http/Firewall/LogoutListener.php#L107 above the $response = ... line.

[linaori]

What's the actual bug here? Changing this could break handlers relying on the token still existing when having the token storage as (implicit) dependency.

[wouterj]

To be honest, a logout is always a success. Calling TokenStorage#setToken(null) cannot fail. Calling it before the token is actual reset is quite nice, as it means the logout handlers know which user is now logging out.

I don't see, apart from naming semantics, how calling the LogoutSuccessHandler's behaviour is affected by being called before or after the token is reset. However, the side-effects are affected by changing the position. So this is a clear BC break.

Can you maybe share why in your situation this matters? Maybe that can help shed some light on why a BC break should be needed here.

[ThomasLandauer]

I have:

• An if in my base template, to display the user a link to their "personal" page:

  {% if app.user %}
  You are logged in as ...
  {% endif %}

• A success message in the Response generated by my onLogoutSuccess() ("You were logged out."). (Displaying such a message is the core functionality of a success handler, I would say)

Result: Since the Response is created when the token is still there, the final page says:

You are logged in as ...
You were logged out.

Can it be that this "bug" is the very reason why the target: option was invented? https://symfony.com/doc/4.4/security.html#logging-out Cause I can't see any benefit in redirecting the user, other than postponing the creation of the final Response until the token is gone for sure.
If I'm right about this, the target: option could be dropped then.

Maybe the token could be passed as second (optional) argument to onLogoutSuccess() in https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/Security/Http/Firewall/LogoutListener.php#L95 ? This would make everybody happy :-)

[chalasr]

Maybe the token could be passed as second (optional) argument to onLogoutSuccess() in https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/Security/Http/Firewall/LogoutListener.php#L95 ? This would make everybody happy :-)

onLogoutSuccess($) is replaced by LogoutEvent in 5.1, which exposes the token.
See #36243