-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Security] Do not overwrite already stored tokens for REMOTE_USER authentication #43992
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
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 |
0c32783
to
243937a
Compare
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍 . We can always improve things as a feature in an upcoming version.
src/Symfony/Component/Security/Http/Authenticator/AbstractPreAuthenticatedAuthenticator.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
76c52f5
to
ce1ee74
Compare
Thank you @stlrnz. |
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