8000 [Security] Multiple user providers on 1 authenticator can wrongfully re-authenticate · Issue #35045 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content
[Security] Multiple user providers on 1 authenticator can wrongfully re-authenticate #35045
Closed
@linaori

Description

@linaori

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:

foreach ($this->userProviders as $provider) {
if (!$provider instanceof UserProviderInterface) {
throw new \InvalidArgumentException(sprintf('User provider "%s" must implement "%s".', \get_class($provider), UserProviderInterface::class));
}
try {
$refreshedUser = $provider->refreshUser($user);
$newToken = clone $token;
$newToken->setUser($refreshedUser);
// tokens can be deauthenticated if the user has been changed.
if (!$newToken->isAuthenticated()) {

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:

} else {
$changed = (string) $this->user !== (string) $user;
}
if ($changed) {
$this->setAuthenticated(false);
}

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 the refreshUser() for the ChainUserProvider, 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.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      0