8000 [Security] Logout `success_handler` is called **before** the actual logout happens · Issue #36227 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
ThomasLandauer opened this issue Mar 26, 2020 · 4 comments
Closed

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

ThomasLandauer opened this issue Mar 26, 2020 · 4 comments
Labels
Feature Security

Comments

@ThomasLandauer
Copy link
Contributor

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
Copy link
Contributor
linaori commented Mar 26, 2020

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
Copy link
Member
wouterj commented Mar 27, 2020

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
Copy link
Contributor Author

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
Copy link
Member
chalasr commented Apr 13, 2020

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

@chalasr chalasr closed this as completed Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Feature Security
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@wouterj @ThomasLandauer @linaori @xabbuh @chalasr @carsonbot
0