8000 [Security] Call logout handlers even if token is null by MatTheCat · Pull Request #24489 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Security] Call logout handlers even if token is null #24489

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

Closed
wants to merge 1 commit into from
Closed

[Security] Call logout handlers even if token is null #24489

wants to merge 1 commit into from

Conversation

MatTheCat
Copy link
Contributor
@MatTheCat MatTheCat commented Oct 8, 2017
Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #7104
License MIT

When listeners are registered in SecurityExtension first ones always are ChannelListener, ContextListener (if stateful) and LogoutListener. This means only ContextListener can set a token to trigger the logout handlers. This means the LogoutListener is useless

  • if we don't have any token in the session
  • if the firewall is stateless

As said in #7104 (comment) we cannot register the LogoutListener last so a quick solution is to call the logout handlers wether a token is present or not in which case I pass a DummyToken instead (see #24489 (comment)).

@MatTheCat MatTheCat changed the title call logout handlers even if token is null [Security] call logout handlers even if token is null Oct 8, 2017
@linaori
Copy link
Contributor
linaori commented Oct 8, 2017

I'm not entirely sure what the actual issue is, but calling out logout listeners when you don't have a token feels weird

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Oct 8, 2017
@MatTheCat
Copy link
Contributor Author

The issue is listeners cannot set any token before the logout listener as it is registered before, so it is wrong to call the handlers based on the token being set.

@MatTheCat MatTheCat changed the title [Security] call logout handlers even if token is null [Security] Call logout handlers even if token is null Oct 9, 2017
@MatTheCat
Copy link
Contributor Author

I edited the PR description, sorry but I was on my phone.

*/
public function logout(Request $request, Response $response, TokenInterface $token)
public function logout(Request $request, Response $response, TokenInterface $token = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the signature here is a BC break, same for all other.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, for applications implementing their own logout handler using $token in a case it is not set. I doubt anyone is concerned and I don't want to wait for Symfony flex (we won't be able to use it anyways) for a bug filed 4 years ago.

Copy link
Contributor Author
@MatTheCat MatTheCat Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake I didn't realize updating the interface force every implementation to update its signature. So is there any way to fix this before Symfony flex? Maybe by setting a dummy token instead of null?

@MatTheCat
Copy link
Contributor Author

Closed in favor of #24769

@MatTheCat MatTheCat closed this Oct 31, 2017
@MatTheCat MatTheCat deleted the 2.7 branch October 31, 2017 12:18
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.

5 participants
0