[Security] ContextListener misbehavior with multiple UserProviders #4498
Comments
|
Perhaps someone should really look into this. |
|
I ran into this issue today too. This might be very problematic. |
|
FYI to the sport fans, I got this problem even if I use different classes for each provider. |
|
I have come across this behaviour when using two firewalls (eg. members and admin) with separate in memory user providers for each. When ContextListener refreshes a user after login it checks ALL user providers, not just the ones specific to the firewall, so it tries to load an admin user from the members user provider, fails, and logs out the admin. Not sure if this design is intentional, but it isn't documented that you can't use separate instances of the same class of provider. I have tested a quick bit of code which alters the problematic part of the ContextListener but not sure what effect it would have in the grand scheme of things. My new refreshUser method is: private function refreshUser(TokenInterface $token)
{
$user = $token->getUser();
if (!$user instanceof UserInterface) {
return $token;
}
if (null !== $this->logger) {
$this->logger->debug(sprintf('Reloading user from user provider.'));
}
foreach ($this->userProviders as $provider) {
try {
$token->setUser($provider->refreshUser($user));
if (null !== $this->logger) {
$this->logger->debug(sprintf('Username "%s" was reloaded from user provider.', $user->getUsername()));
}
return $token;
} catch (UnsupportedUserException $unsupported) {
// let's try the next user provider
} catch (UsernameNotFoundException $notFound) {
// user not found, try next provider
}
}
// If we had a user not found exception then we had a valid provider but could not find a user
if (isset($notFound)) {
if (null !== $this->logger) {
$this->logger->warn(sprintf('Username "%s" could not be found.', $user->getUsername()));
}
return null;
}
throw new \RuntimeException(sprintf('There is no user provider for user "%s".', get_class($user)));
} |
|
After running some tests, here is my findings. First of all, the The implementation of many authentication providers is not well aware of this change and eventually leads this situation unless each provider check if This makes me think that it might be better to add What do you guys think? |
|
I agree. In fact, is there any reason that all tokens should not have a |
|
Looking at this again it seems I'm getting confused between authentication provider and user provider. The user provider has no knowledge of which authentication provider it came from, particularly as it could potentially be used in multiple firewalls. The context listener has no knowledge of the authentication providers, only the user providers, user providers only accept/reject a user based on its class. I can see a few solutions:
|
|
I think @schmittjoh might have the better idea of the behavior. |
|
Why do you have different user providers for the same user objects? What is the difference between the two? |
|
The two different user providers access different user storages to have |
|
FYI, we used to have a |
|
@fabpot, could you confirm if the behavior described in the issue is intentional or not? If it is, could you provide some rationale behind this? |
|
@zeqfreed I need to investigate further the consequences of the current way it works vs the "better" way. |
|
Ran into this myself. This seems like less than ideal behavior. Is there an accepted stop gap solution? What are you guys doing to work around this issue? |
|
Hello. I agree with first @nyorai's solution : "Introduce a getUserProviderKey into UserInterface so each user instance knows exactly where it came from." Of course, Add this behavior will lead to break the BC, But it will fix this issue. |
|
Thanks @lyrixx. I found that since I was actually using different User classes that corresponded to my different UserProviders, I was able to modify the method "supportsClass" and be precise about which Users that provider is able to support. The checks in FOS UserBundle and the FR3D LDAPBundle are a little broad for my application, but by customizing those and narrowing down to the exact class I wanted each to support, I was able to get the existing setup to work... |
|
Any news on this one ? I just ran into the same issue today |
|
I am "stuck" with the same behavior. Nevertheless, I am unable to login. Stacktrace during ONE try : My questions are:
I bypass this behavior with "chain_provider" but it does not make sense for me.. |
|
I have the same problem with the following configuration: Both providers use the same class buth only return users with specific roles. It worked on my local development setup somehow, but on the production site it does not... |
|
👍 I think the implementation is really strange. Here is the difference between the The foreach ($this->userProviders as $provider) {
try {
// …
return $token;
} catch (UnsupportedUserException $e) {
// let's try the next user provider
} catch (UsernameNotFoundException $e) {
if (null !== $this->logger) {
$this->logger->warning('Username could not be found in the selected user provider.', array('username' => $e->getUsername(), 'provider' => get_class($provider)));
}
return;
}
}The foreach ($this->providers as $provider) {
try {
return $provider->refreshUser($user);
} catch (UnsupportedUserException $e) {
// try next one
} catch (UsernameNotFoundException $e) {
$supportedUserFound = true;
// try next one
}
}This is not only undocumented, it is also very disturbing. I just discovered this after 2 days trying to understand why my user had the correct token after login but then was again anonymous right after the redirection. We really need to fix this incoherent behaviour. Seems an old PR from 2014 proposed to solve this bug : #12465 |
|
This should be fixed by the patch proposed in #21791. |
This PR was merged into the 2.7 branch. Discussion ---------- [SecurityBundle] only pass relevant user provider | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #4498, #12465, #20401, #21737 | License | MIT | Doc PR | There is no need for the context listener to be aware of all the configured user providers. It must only use the provider for the current firewall (the one identified by the context key passed to the constructor) to refresh the user from the session. Commits ------- d97e07f [SecurityBundle] only pass relevant user provider
|
The fix in #21791 was not right. |
|
#21865 is my next attempt to solve this issue. Could you please try it with your projects? |
…ing (xabbuh) This PR was merged into the 2.7 branch. Discussion ---------- [Security] context listener: hardening user provider handling | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #4498 | License | MIT | Doc PR | After the wrong fix in #21791 this is the second attempt to solve #4498. If more than one user provider support the user for the current context, all of them will be applied instead of returning prematurely when the first user provider does not find the logged in user. Commits ------- 0fb0929 context listener: hardening user provider handling
|
@fabpot Hi, I have the same bug with Symfony 2.8 |
|
@omnilight Which patch version of Symfony 2.8 do you use? |
|
@xabbuh 2.8.16 |
|
2.8.19 was the first 2.8 release containing the fix. |
|
@xabbuh Thanks! |
I'm implementing an application that needs to have separate sets of users for its different sections. For instance, we create Admin and Front sections. For each of the sections I define a firewall rule in security config and a different user provider:
Now, the provider ids reference two different services of the same class that implements UserProviderInterface configured to use different user storages, but same user class.
This setup leads to a problem in ContextListener. After user logs in, the ContextListener attempts to validate user token and refresh user by iterating over every user provider and calling its refreshUser method. If current user provider does not support the user class it throws UnsupportedUserException and ContextListener continues to the next user provider, but if it does and can't actually find the user it throws UsernameNotFoundException which interrupts the iteration and returns null essentially forcing the user to log out.
One solution that would work here is to make sure a different user class used for every user provider, so the iteration always reaches the provider that can refresh user token. But this solution seems quirky, non-obvious and moreover is a kind of side effect depending on the internal implementation of ContextListener.
Another solution is obviously to implement my own ContextListener that will iterate through every user provider regardless of the exceptions thrown. This is the approach I think I will have to take if you confirm that this behavior is actually intended and my use case doesn't lie within symfony's design philosophy.
Please refer to the ContextListener source for the problematic code.
The text was updated successfully, but these errors were encountered: