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
[Security] Do not overwrite already stored tokens for REMOTE_USER authentication #43992
Conversation
This looks like a correct change, although I'm wondering if we should apply this always (i.e. never check if authenticators support the request if there already is a token) instead of letting the authenticator decide. But I'm not sure if this would be the correct fix (happy to hear other ideas on this topic). Is it possible for you to add a test for this behavior? (e.g. in the |
I've done that. The test is inspired by symfony/src/Symfony/Component/Security/Http/Tests/Authenticator/RememberMeAuthenticatorTest.php Line 39 in 07a891f
I think this is the right question to ask. Looking at symfony/src/Symfony/Component/Security/Http/Authenticator/HttpBasicAuthenticator.php Line 57 in 07a891f
It's easy to forget that scenario when writing an Authenticator. In my opinion the Authenticator is lying when returning false for the question "do you support this request?". It can support/authenticate this request, but we dont want it to do. |
Thank you for the PR issue and the reproducer. Great job! Im also hesitant that this is the correct fix. But Im not sure what else is. Im wondering what a system like kerberos would do on logout and if the user changes without logging out first. Then this fix will break applications (depending on kerberos or others behavior). Im not sure who would like to read this but one can do a workaround by creating your own RemoteUserAuthentication and override the |
Thank you for the feedback, @Nyholm!
That's a really good question. In our environment, this cannot happen. But I mocked this scenario in my demo application and found a flaw in the original PR. So I pushed an updated version which checks for the user identifier before making a decision. But this implementation does not invalidate the session of the first user. In consequence the second user may have access to sensible data and/or the application will break anyway.
I think this is still a really good idea, @wouterj! It may solve the problem with the session, too? |
Thank you for working on this!
I've created a minimal app and checked your findings. The legacy security system indeed did not load the user if the token contained the same user (checked by username and firewall name).
If we add the precise checks of the old system, let's merge this
src/Symfony/Component/Security/Http/Authenticator/AbstractPreAuthenticatedAuthenticator.php
Outdated
Show resolved
Hide resolved
Thank you @stlrnz. |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
As described in #43648 the user is currently loaded on every request for REMOTE_USER authentication.
Thanks to @wouterj for confirming me on Slack that this seems weird. So, I looked deeper into this.
I found out that other Authenticators tell the AuthenticatorManager only under special conditions (like matching route etc.) that they support the current request. However, the
AbstractPreAuthenticatedAuthenticator
is not so picky. In consequence, the user is authenticated again on every request.Inspired by
RememberMeAuthenticator
, this PR adds an addition check toAbstractPreAuthenticatedAuthenticator
to solve this issue.symfony/src/Symfony/Component/Security/Http/Authenticator/RememberMeAuthenticator.php
Line 63 in 07a891f