Removing the session migration on SimplePreAuthenticationListener #27427
Conversation
This authentication mechanism should not be used for session-based authentication. Migrating the session caused some regressions in legacy authentication systems.
|
At least, I would add a comment about the session fixation issue when used incorrectly. |
|
Don't we need to do the same in |
|
After some thought, there is a generic issue/incompatibility across all security listeners. Here it is: If you create an auth mechanism that you expect will be used by a browser, then your auth mechanism must be smart enough to not re-authenticate the user when they are already authenticated This is at the root of any bugs that were caused by the new behavior change. Here's a real-world example of the bad behavior. Suppose you have a Guard authenticator that authenticates entirely by IP address (i.e. look up the So, I actually now think this is an annoying documentation issue: we must make sure that auth mechanisms intended for browser use do not re-auth on every request. I'm going to create a docs PR right now. |
|
See symfony/symfony-docs#9860 for the 2.8 version with Guard. I believe that we should not merge this PR - that it's entirely a documentation issue. |
|
Closing then |
|
I think there is a generic misinterpretation. As the HTTP protocol is entirely stateless, a user is always authenticated on every request. In the Symfony firewall this can be any listener, the
Just because a session does exist, does not mean the user is authenticated through it. There can be plenty of other sources. For example if using Shibboleth as SSO solution, the To me, a stateless firewall cannot not handle session migration. This can only be done reliably if the firewall listener is stateful. |
|
oh well, just ignore my comment, I didn't see #27395 (comment) 🙈 |
…averryan) This PR was squashed before being merged into the 2.8 branch (closes #9860). Discussion ---------- Avoiding authentication on every request with Guard Hi guys! See symfony/symfony#27395 and symfony/symfony#27427 When we're happy with this, I can merge and handle the changes on 3.4, because it will be almost entirely different (with `supports()`, autowiring & the `Security` class - booya for progress!) Commits ------- d2f062a Avoiding authentication on every request with Guard
This partially reverts the session migration patches applied recently. Here are the details:
ContaoTokenon every request:https://github.com/contao/core-bundle/blob/504d8ed7c14a8e135def5722e47ffa1dc7c4d6ed/src/Security/ContaoAuthenticator.php#L88
When that token (
ContaoToken) is instantiated, it actually does a legacy, cookie-based authentication where a cookie is set by using the session id as a hash.But, then, because the
ContaoAuthenticatorreturns theContaoToken, the session is migrated, which effectively invalidates the cookie set by their legacy auth system (because the hash for the cookie uses the session id).This is a legacy system that does some crazy stuff (their WIP newer version apparently already makes a lot of nice changes), but the point is that the patch causes side effects. We decided to migrate the session in
SimplePreAuthenticationListenerjust to be extra safe: this authentication mechanism is not meant to be used with session-based authentication.The downside of this revert is that, it makes session fixation a possibility with
SimplePreAuthenticationListener. But, I believe this would only be possible if someone creates a "SimplePreAuthenticator" in order to make a traditional, session-based authentication system - e.g. a login form.