8000 bug #35065 [Security] Use supportsClass in addition to UnsupportedUse… · symfony/symfony@fb0be81 · GitHub
[go: up one dir, main page]
Skip to content

Commit fb0be81

Browse files
nicolas-grekasnicolas-grekas
committed
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 ------- d3942cb Use supportsClass where possible
2 parents ff174df + d3942cb commit fb0be81

File tree

4 files changed

+88
-12
lines changed

4 files changed

+88
-12
lines changed

‎src/Symfony/Component/Security/Core/Tests/User/ChainUserProviderTest.php

Lines changed: 58 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,31 +68,62 @@ public function testRefreshUser()
6868
$provider1 = $this->getProvider();
6969
$provider1
7070
->expects($this->once())
71-
->method('refreshUser')
72-
->willThrowException(new UnsupportedUserException('unsupported'))
71+
->method('supportsClass')
72+
->willReturn(false)
7373
;
7474

7575
$provider2 = $this->getProvider();
76+
$provider2
77+
->expects($this->once())
78+
->method('supportsClass')
79+
->willReturn(true)
80+
;
81+
7682
$provider2
83+
->expects($this->once())
84+
->method('refreshUser')
85+
->willThrowException(new UnsupportedUserException('unsupported'))
86+
;
87+
88+
$provider3 = $this->getProvider();
89+
$provider3
90+
->expects($this->once())
91+
->method('supportsClass')
92+
->willReturn(true)
93+
;
94+
95+
$provider3
7796
->expects($this->once())
7897
->method('refreshUser')
7998
->willReturn($account = $this->getAccount())
8099
;
81100

82-
$provider = new ChainUserProvider([$provider1, $provider2]);
101+
$provider = new ChainUserProvider([$provider1, $provider2, $provider3]);
83102
$this->assertSame($account, $provider->refreshUser($this->getAccount()));
84103
}
85104

86105
public function testRefreshUserAgain()
87106
{
88107
$provider1 = $this->getProvider();
108+
$provider1
109+
->expects($this->once())
110+
->method('supportsClass')
111+
->willReturn(true)
112+
;
113+
89114
$provider1
90115
->expects($this->once())
91116
->method('refreshUser')
92117
->willThrowException(new UsernameNotFoundException('not found'))
93118
;
94119

95120
$provider2 = $this->getProvider();
121+
$provider2
122+
->expects($this->once())
123+
->method('supportsClass')
124+
->willReturn(true)
125+
;
126+
96127
$provider2
97128
->expects($this->once())
98129
->method('refreshUser')
@@ -107,13 +138,25 @@ public function testRefreshUserThrowsUnsupportedUserException()
107138
{
108139
$this->expectException('Symfony\Component\Security\Core\Exception\UnsupportedUserException');
109140
$provider1 = $this->getProvider();
141+
$provider1
142+
->expects($this->once())
143+
->method('supportsClass')
144+
->willReturn(true)
145+
;
146+
110147
$provider1
111148
->expects($this->once())
112149
->method('refreshUser')
113150
->willThrowException(new UnsupportedUserException('unsupported'))
114151
;
115152

116153
$provider2 = $this->getProvider();
154+
$provider2
155+
->expects($this->once())
156+
->method('supportsClass')
157+
->willReturn(true)
158+
;
159+
117160
$provider2
118161
->expects($this->once())
119162
->method('refreshUser')
@@ -171,13 +214,25 @@ public function testSupportsClassWhenNotSupported()
171214
public function testAcceptsTraversable()
172215
{
173216
$provider1 = $this->getProvider();
217+
$provider1
218+
->expects($this->once())
219+
->method('supportsClass')
220+
->willReturn(true)
221+
;
222+
174223
$provider1
175224
->expects($this->once())
176225
->method('refreshUser')
177226
->willThrowException(new UnsupportedUserException('unsupported'))
178227
;
179228

180229
$provider2 = $this->getProvider();
230+
$provider2
231+
->expects($this->once())
232+
->method('supportsClass')
233+
->willReturn(true)
234+
;
235+
181236
$provider2
182237
->expects($this->once())
183238
->method('refreshUser')

‎src/Symfony/Component/Security/Core/User/ChainUserProvider.php

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,10 @@ public function refreshUser(UserInterface $user)
7373

7474
foreach ($this->providers as $provider) {
7575
try {
76+
if (!$provider->supportsClass(\get_class($user))) {
77+
continue;
78+
}
79+
7680
return $provider->refreshUser($user);
7781
} catch (UnsupportedUserException $e) {
7882
// try next one

‎src/Symfony/Component/Security/Http/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)

‎src/Symfony/Component/Security/Http/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)
0