Skip to content

Commit 2d9796e

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 f5185df + ac16d32 commit 2d9796e

File tree

2 files changed

+62
-3
lines changed

2 files changed

+62
-3
lines changed

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')

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

0 commit comments

Comments
 (0)