bug #35065 [Security] Use supportsClass in addition to UnsupportedUserException (linaori)
This PR was merged into the 3.4 branch.
Discussion
----------
[Security] Use supportsClass in addition to UnsupportedUserException
| Q | A
| ------------- | ---
| Branch? | 3.4+
| Bug fix? | yes
| New feature? | no
| Deprecations? | no
| Tickets | Fix #35045
| License | MIT
| Doc PR | ~
This PR fixes the issue where user providers rely on just the UnsupportedUserException from `refreshUser()`, causing a flow where users are wrongfully re-authenticated.
There's one issue where `refreshUser()` can do far more sophisticated checks on the user class, which it will never reach if the class is not supported. As far as I know it was never intended to support instances that are rejected by `supportsClass()`, though people could've implemented this (by accident). So the question is more if we should add a BC layer for this; for example:
```php
try {
$refreshedUser = $provider->refreshUser($user);
$newToken = clone $token;
$newToken->setUser($refreshedUser);
if (!$provider->supportsClass($userClass)) {
if ($this->shouldCheckSupportsClass) {
continue;
}
// have to think of a proper deprecation here for 6.0
@trigger_error('Provider %s does not support user class %s via supportsClass() while it does support it via refreshUser .. please set option X and fix %s::supportsUser() ', E_USER_DEPRECATED);
}
```
This would prevent behavior from breaking but also means we can't fix this on anything less than 5.1.
Commits
-------
d3942cbe17
Use supportsClass where possible
This commit is contained in:
commit
fb0be81b7a
@ -68,24 +68,49 @@ class ChainUserProviderTest extends TestCase
|
||||
$provider1 = $this->getProvider();
|
||||
$provider1
|
||||
->expects($this->once())
|
||||
->method('refreshUser')
|
||||
->willThrowException(new UnsupportedUserException('unsupported'))
|
||||
->method('supportsClass')
|
||||
->willReturn(false)
|
||||
;
|
||||
|
||||
$provider2 = $this->getProvider();
|
||||
$provider2
|
||||
->expects($this->once())
|
||||
->method('supportsClass')
|
||||
->willReturn(true)
|
||||
;
|
||||
|
||||
$provider2
|
||||
->expects($this->once())
|
||||
->method('refreshUser')
|
||||
->willThrowException(new UnsupportedUserException('unsupported'))
|
||||
;
|
||||
|
||||
$provider3 = $this->getProvider();
|
||||
$provider3
|
||||
->expects($this->once())
|
||||
->method('supportsClass')
|
||||
->willReturn(true)
|
||||
;
|
||||
|
||||
$provider3
|
||||
->expects($this->once())
|
||||
->method('refreshUser')
|
||||
->willReturn($account = $this->getAccount())
|
||||
;
|
||||
|
||||
$provider = new ChainUserProvider([$provider1, $provider2]);
|
||||
$provider = new ChainUserProvider([$provider1, $provider2, $provider3]);
|
||||
$this->assertSame($account, $provider->refreshUser($this->getAccount()));
|
||||
}
|
||||
|
||||
public function testRefreshUserAgain()
|
||||
{
|
||||
$provider1 = $this->getProvider();
|
||||
$provider1
|
||||
->expects($this->once())
|
||||
->method('supportsClass')
|
||||
->willReturn(true)
|
||||
;
|
||||
|
||||
$provider1
|
||||
->expects($this->once())
|
||||
->method('refreshUser')
|
||||
@ -93,6 +118,12 @@ class ChainUserProviderTest extends TestCase
|
||||
;
|
||||
|
||||
$provider2 = $this->getProvider();
|
||||
$provider2
|
||||
->expects($this->once())
|
||||
->method('supportsClass')
|
||||
->willReturn(true)
|
||||
;
|
||||
|
||||
$provider2
|
||||
->expects($this->once())
|
||||
->method('refreshUser')
|
||||
@ -107,6 +138,12 @@ class ChainUserProviderTest extends TestCase
|
||||
{
|
||||
$this->expectException('Symfony\Component\Security\Core\Exception\UnsupportedUserException');
|
||||
$provider1 = $this->getProvider();
|
||||
$provider1
|
||||
->expects($this->once())
|
||||
->method('supportsClass')
|
||||
->willReturn(true)
|
||||
;
|
||||
|
||||
$provider1
|
||||
->expects($this->once())
|
||||
->method('refreshUser')
|
||||
@ -114,6 +151,12 @@ class ChainUserProviderTest extends TestCase
|
||||
;
|
||||
|
||||
$provider2 = $this->getProvider();
|
||||
$provider2
|
||||
->expects($this->once())
|
||||
->method('supportsClass')
|
||||
->willReturn(true)
|
||||
;
|
||||
|
||||
$provider2
|
||||
->expects($this->once())
|
||||
->method('refreshUser')
|
||||
@ -171,6 +214,12 @@ class ChainUserProviderTest extends TestCase
|
||||
public function testAcceptsTraversable()
|
||||
{
|
||||
$provider1 = $this->getProvider();
|
||||
$provider1
|
||||
->expects($this->once())
|
||||
->method('supportsClass')
|
||||
->willReturn(true)
|
||||
;
|
||||
|
||||
$provider1
|
||||
->expects($this->once())
|
||||
->method('refreshUser')
|
||||
@ -178,6 +227,12 @@ class ChainUserProviderTest extends TestCase
|
||||
;
|
||||
|
||||
$provider2 = $this->getProvider();
|
||||
$provider2
|
||||
->expects($this->once())
|
||||
->method('supportsClass')
|
||||
->willReturn(true)
|
||||
;
|
||||
|
||||
$provider2
|
||||
->expects($this->once())
|
||||
->method('refreshUser')
|
||||
|
@ -73,6 +73,10 @@ class ChainUserProvider implements UserProviderInterface
|
||||
|
||||
foreach ($this->providers as $provider) {
|
||||
try {
|
||||
if (!$provider->supportsClass(\get_class($user))) {
|
||||
continue;
|
||||
}
|
||||
|
||||
return $provider->refreshUser($user);
|
||||
} catch (UnsupportedUserException $e) {
|
||||
// try next one
|
||||
|
@ -168,12 +168,17 @@ class ContextListener implements ListenerInterface
|
||||
|
||||
$userNotFoundByProvider = false;
|
||||
$userDeauthenticated = false;
|
||||
$userClass = \get_class($user);
|
||||
|
||||
foreach ($this->userProviders as $provider) {
|
||||
if (!$provider instanceof UserProviderInterface) {
|
||||
throw new \InvalidArgumentException(sprintf('User provider "%s" must implement "%s".', \get_class($provider), UserProviderInterface::class));
|
||||
}
|
||||
|
||||
if (!$provider->supportsClass($userClass)) {
|
||||
continue;
|
||||
}
|
||||
|
||||
try {
|
||||
$refreshedUser = $provider->refreshUser($user);
|
||||
$newToken = clone $token;
|
||||
@ -233,7 +238,7 @@ class ContextListener implements ListenerInterface
|
||||
return null;
|
||||
}
|
||||
|
||||
throw new \RuntimeException(sprintf('There is no user provider for user "%s".', \get_class($user)));
|
||||
throw new \RuntimeException(sprintf('There is no user provider for user "%s".', $userClass));
|
||||
}
|
||||
|
||||
private function safelyUnserialize($serializedToken)
|
||||
|
@ -256,7 +256,7 @@ class ContextListenerTest extends TestCase
|
||||
{
|
||||
$tokenStorage = new TokenStorage();
|
||||
$refreshedUser = new User('foobar', 'baz');
|
||||
$this->handleEventWithPreviousSession($tokenStorage, [new NotSupportingUserProvider(), new SupportingUserProvider($refreshedUser)]);
|
||||
$this->handleEventWithPreviousSession($tokenStorage, [new NotSupportingUserProvider(true), new NotSupportingUserProvider(false), new SupportingUserProvider($refreshedUser)]);
|
||||
|
||||
$this->assertSame($refreshedUser, $tokenStorage->getToken()->getUser());
|
||||
}
|
||||
@ -265,7 +265,7 @@ class ContextListenerTest extends TestCase
|
||||
{
|
||||
$tokenStorage = new TokenStorage();
|
||||
$refreshedUser = new User('foobar', 'baz');
|
||||
$this->handleEventWithPreviousSession($tokenStorage, [new NotSupportingUserProvider(), new SupportingUserProvider($refreshedUser)], null, true);
|
||||
$this->handleEventWithPreviousSession($tokenStorage, [new NotSupportingUserProvider(true), new NotSupportingUserProvider(false), new SupportingUserProvider($refreshedUser)], null, true);
|
||||
|
||||
$this->assertNull($tokenStorage->getToken());
|
||||
}
|
||||
@ -287,7 +287,7 @@ class ContextListenerTest extends TestCase
|
||||
$rememberMeServices = $this->createMock(RememberMeServicesInterface::class);
|
||||
$rememberMeServices->expects($this->once())->method('loginFail');
|
||||
|
||||
$this->handleEventWithPreviousSession($tokenStorage, [new NotSupportingUserProvider(), new SupportingUserProvider($refreshedUser)], null, true, $rememberMeServices);
|
||||
$this->handleEventWithPreviousSession($tokenStorage, [new NotSupportingUserProvider(true), new NotSupportingUserProvider(false), new SupportingUserProvider($refreshedUser)], null, true, $rememberMeServices);
|
||||
|
||||
$this->assertNull($tokenStorage->getToken());
|
||||
}
|
||||
@ -296,7 +296,7 @@ class ContextListenerTest extends TestCase
|
||||
{
|
||||
$tokenStorage = new TokenStorage();
|
||||
$refreshedUser = new User('foobar', 'baz');
|
||||
$this->handleEventWithPreviousSession($tokenStorage, [new NotSupportingUserProvider(), new SupportingUserProvider($refreshedUser)], $refreshedUser);
|
||||
$this->handleEventWithPreviousSession($tokenStorage, [new NotSupportingUserProvider(true), new NotSupportingUserProvider(false), new SupportingUserProvider($refreshedUser)], $refreshedUser);
|
||||
|
||||
$this->assertSame($refreshedUser, $tokenStorage->getToken()->getUser());
|
||||
}
|
||||
@ -313,7 +313,7 @@ class ContextListenerTest extends TestCase
|
||||
public function testTokenIsSetToNullIfNoUserWasLoadedByTheRegisteredUserProviders()
|
||||
{
|
||||
$tokenStorage = new TokenStorage();
|
||||
$this->handleEventWithPreviousSession($tokenStorage, [new NotSupportingUserProvider(), new SupportingUserProvider()]);
|
||||
$this->handleEventWithPreviousSession($tokenStorage, [new NotSupportingUserProvider(true), new NotSupportingUserProvider(false), new SupportingUserProvider()]);
|
||||
|
||||
$this->assertNull($tokenStorage->getToken());
|
||||
}
|
||||
@ -321,14 +321,14 @@ class ContextListenerTest extends TestCase
|
||||
public function testRuntimeExceptionIsThrownIfNoSupportingUserProviderWasRegistered()
|
||||
{
|
||||
$this->expectException('RuntimeException');
|
||||
$this->handleEventWithPreviousSession(new TokenStorage(), [new NotSupportingUserProvider(), new NotSupportingUserProvider()]);
|
||||
$this->handleEventWithPreviousSession(new TokenStorage(), [new NotSupportingUserProvider(false), new NotSupportingUserProvider(true)]);
|
||||
}
|
||||
|
||||
public function testAcceptsProvidersAsTraversable()
|
||||
{
|
||||
$tokenStorage = new TokenStorage();
|
||||
$refreshedUser = new User('foobar', 'baz');
|
||||
$this->handleEventWithPreviousSession($tokenStorage, new \ArrayObject([new NotSupportingUserProvider(), new SupportingUserProvider($refreshedUser)]), $refreshedUser);
|
||||
$this->handleEventWithPreviousSession($tokenStorage, new \ArrayObject([new NotSupportingUserProvider(true), new NotSupportingUserProvider(false), new SupportingUserProvider($refreshedUser)]), $refreshedUser);
|
||||
|
||||
$this->assertSame($refreshedUser, $tokenStorage->getToken()->getUser());
|
||||
}
|
||||
@ -383,6 +383,14 @@ class ContextListenerTest extends TestCase
|
||||
|
||||
class NotSupportingUserProvider implements UserProviderInterface
|
||||
{
|
||||
/** @var bool */
|
||||
private $throwsUnsupportedException;
|
||||
|
||||
public function __construct($throwsUnsupportedException)
|
||||
{
|
||||
$this->throwsUnsupportedException = $throwsUnsupportedException;
|
||||
}
|
||||
|
||||
public function loadUserByUsername($username)
|
||||
{
|
||||
throw new UsernameNotFoundException();
|
||||
@ -390,7 +398,11 @@ class NotSupportingUserProvider implements UserProviderInterface
|
||||
|
||||
public function refreshUser(UserInterface $user)
|
||||
{
|
||||
throw new UnsupportedUserException();
|
||||
if ($this->throwsUnsupportedException) {
|
||||
throw new UnsupportedUserException();
|
||||
}
|
||||
|
||||
return $user;
|
||||
}
|
||||
|
||||
public function supportsClass($class)
|
||||
|
Reference in New Issue
Block a user