Skip to content

Commit a10ae32

Browse files
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
2 parents eb3db77 + f7d49a0 commit a10ae32

File tree

2 files changed

+26
-9
lines changed

2 files changed

+26
-9
lines changed

Firewall/ContextListener.php

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -168,12 +168,17 @@ protected function refreshUser(TokenInterface $token)
168168

169169
$userNotFoundByProvider = false;
170170
$userDeauthenticated = false;
171+
$userClass = \get_class($user);
171172

172173
foreach ($this->userProviders as $provider) {
173174
if (!$provider instanceof UserProviderInterface) {
174175
throw new \InvalidArgumentException(sprintf('User provider "%s" must implement "%s".', \get_class($provider), UserProviderInterface::class));
175176
}
176177

178+
if (!$provider->supportsClass($userClass)) {
179+
continue;
180+
}
181+
177182
try {
178183
$refreshedUser = $provider->refreshUser($user);
179184
$newToken = clone $token;
@@ -233,7 +238,7 @@ protected function refreshUser(TokenInterface $token)
233238
return null;
234239
}
235240

236-
throw new \RuntimeException(sprintf('There is no user provider for user "%s".', \get_class($user)));
241+
throw new \RuntimeException(sprintf('There is no user provider for user "%s".', $userClass));
237242
}
238243

239244
private function safelyUnserialize($serializedToken)

Tests/Firewall/ContextListenerTest.php

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,7 @@ public function testIfTokenIsDeauthenticatedTriggersDeprecations()
256256
{
257257
$tokenStorage = new TokenStorage();
258258
$refreshedUser = new User('foobar', 'baz');
259-
$this->handleEventWithPreviousSession($tokenStorage, [new NotSupportingUserProvider(), new SupportingUserProvider($refreshedUser)]);
259+
$this->handleEventWithPreviousSession($tokenStorage, [new NotSupportingUserProvider(true), new NotSupportingUserProvider(false), new SupportingUserProvider($refreshedUser)]);
260260

261261
$this->assertSame($refreshedUser, $tokenStorage->getToken()->getUser());
262262
}
@@ -265,7 +265,7 @@ public function testIfTokenIsDeauthenticated()
265265
{
266266
$tokenStorage = new TokenStorage();
267267
$refreshedUser = new User('foobar', 'baz');
268-
$this->handleEventWithPreviousSession($tokenStorage, [new NotSupportingUserProvider(), new SupportingUserProvider($refreshedUser)], null, true);
268+
$this->handleEventWithPreviousSession($tokenStorage, [new NotSupportingUserProvider(true), new NotSupportingUserProvider(false), new SupportingUserProvider($refreshedUser)], null, true);
269269

270270
$this->assertNull($tokenStorage->getToken());
271271
}
@@ -287,7 +287,7 @@ public function testRememberMeGetsCanceledIfTokenIsDeauthenticated()
287287
$rememberMeServices = $this->createMock(RememberMeServicesInterface::class);
288288
$rememberMeServices->expects($this->once())->method('loginFail');
289289

290-
$this->handleEventWithPreviousSession($tokenStorage, [new NotSupportingUserProvider(), new SupportingUserProvider($refreshedUser)], null, true, $rememberMeServices);
290+
$this->handleEventWithPreviousSession($tokenStorage, [new NotSupportingUserProvider(true), new NotSupportingUserProvider(false), new SupportingUserProvider($refreshedUser)], null, true, $rememberMeServices);
291291

292292
$this->assertNull($tokenStorage->getToken());
293293
}
@@ -296,7 +296,7 @@ public function testTryAllUserProvidersUntilASupportingUserProviderIsFound()
296296
{
297297
$tokenStorage = new TokenStorage();
298298
$refreshedUser = new User('foobar', 'baz');
299-
$this->handleEventWithPreviousSession($tokenStorage, [new NotSupportingUserProvider(), new SupportingUserProvider($refreshedUser)], $refreshedUser);
299+
$this->handleEventWithPreviousSession($tokenStorage, [new NotSupportingUserProvider(true), new NotSupportingUserProvider(false), new SupportingUserProvider($refreshedUser)], $refreshedUser);
300300

301301
$this->assertSame($refreshedUser, $tokenStorage->getToken()->getUser());
302302
}
@@ -313,22 +313,22 @@ public function testNextSupportingUserProviderIsTriedIfPreviousSupportingUserPro
313313
public function testTokenIsSetToNullIfNoUserWasLoadedByTheRegisteredUserProviders()
314314
{
315315
$tokenStorage = new TokenStorage();
316-
$this->handleEventWithPreviousSession($tokenStorage, [new NotSupportingUserProvider(), new SupportingUserProvider()]);
316+
$this->handleEventWithPreviousSession($tokenStorage, [new NotSupportingUserProvider(true), new NotSupportingUserProvider(false), new SupportingUserProvider()]);
317317

318318
$this->assertNull($tokenStorage->getToken());
319319
}
320320

321321
public function testRuntimeExceptionIsThrownIfNoSupportingUserProviderWasRegistered()
322322
{
323323
$this->expectException('RuntimeException');
324-
$this->handleEventWithPreviousSession(new TokenStorage(), [new NotSupportingUserProvider(), new NotSupportingUserProvider()]);
324+
$this->handleEventWithPreviousSession(new TokenStorage(), [new NotSupportingUserProvider(false), new NotSupportingUserProvider(true)]);
325325
}
326326

327327
public function testAcceptsProvidersAsTraversable()
328328
{
329329
$tokenStorage = new TokenStorage();
330330
$refreshedUser = new User('foobar', 'baz');
331-
$this->handleEventWithPreviousSession($tokenStorage, new \ArrayObject([new NotSupportingUserProvider(), new SupportingUserProvider($refreshedUser)]), $refreshedUser);
331+
$this->handleEventWithPreviousSession($tokenStorage, new \ArrayObject([new NotSupportingUserProvider(true), new NotSupportingUserProvider(false), new SupportingUserProvider($refreshedUser)]), $refreshedUser);
332332

333333
$this->assertSame($refreshedUser, $tokenStorage->getToken()->getUser());
334334
}
@@ -383,14 +383,26 @@ private function handleEventWithPreviousSession(TokenStorageInterface $tokenStor
383383

384384
class NotSupportingUserProvider implements UserProviderInterface
385385
{
386+
/** @var bool */
387+
private $throwsUnsupportedException;
388+
389+
public function __construct($throwsUnsupportedException)
390+
{
391+
$this->throwsUnsupportedException = $throwsUnsupportedException;
392+
}
393+
386394
public function loadUserByUsername($username)
387395
{
388396
throw new UsernameNotFoundException();
389397
}
390398

391399
public function refreshUser(UserInterface $user)
392400
{
393-
throw new UnsupportedUserException();
401+
if ($this->throwsUnsupportedException) {
402+
throw new UnsupportedUserException();
403+
}
404+
405+
return $user;
394406
}
395407

396408
public function supportsClass($class)

0 commit comments

Comments
 (0)