Description
Symfony version(s) affected: 3.4, 4.3, 4.4, 5.0
Description
Report to clarify #29653, which was closed due to lack of activity.
There's no check whether a user provider supports a given user during the refreshUser()
process, thus will attempt to refresh the user against every provider, rather than just the supported providers:
symfony/src/Symfony/Component/Security/Http/Firewall/ContextListener.php
Lines 185 to 196 in 4d064a2
I've found a case where:
- The first the correct provider is hit (
WebsiteUserProvider
), this de-authenticates the token because the user has changed - The second provider is hit (
ApiUserProvider
), this one does not support this user class, and immediately returns the User object
public function refreshUser(UserInterface $user)
{
return $user;
}
Because the ApiUserProvider
gives the same user object back, the token is not de-authenticated after the setUser()
is called.
$refreshedUser = $provider->refreshUser($user); // $user = WebsiteUser instance 1
$newToken = clone $token;
$newToken->setUser($refreshedUser); // $refreshedUser = WebsiteUser instance 1
When the token receives a setUser($user)
where the instance of $user
is identical to the instance it contains internally, the token's authentication state will not be changed:
Because the second token will not hit the continue;
, it will return the token and be considered "authenticated", thus the user won't be authenticated. This breaks the de-authentication flow and could result in accounts being logged in while they shouldn't.
Edit:
- The
isSupported()
is not called in therefreshUser()
for theChainUserProvider
, would have to be changed in a similar fashion.
How to reproduce
n/a - will have to make a failing test to prove this
Possible Solution
While refreshUser()
indicates it can throw a UnsupportedUserException
, this is not intuitive and is easy to miss, especially as supportsUser()
exists. I think the solution of #29653 is the right direction. There was some feedback that had not been processed yet and is missing some tests.
Additional Context
Does the UnsupportedUserException
even make sense at this point? It duplicates a feature in two different ways. Some core classes actually throw this exception, which would no longer be required if the supportsClass()
would be used.