Skip to content

Commit 290b23a

Browse files
nicolas-grekasnicolas-grekas
committed
[Security\Core] throw AccessDeniedException when switch user fails
1 parent a2f67df commit 290b23a

File tree

2 files changed

+67
-17
lines changed

2 files changed

+67
-17
lines changed

‎Firewall/SwitchUserListener.php

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,8 @@ public function __invoke(RequestEvent $event)
105105
try {
106106
$this->tokenStorage->setToken($this->attemptSwitchUser($request, $username));
107107
} catch (AuthenticationException $e) {
108-
throw new \LogicException(sprintf('Switch User failed: "%s"', $e->getMessage()));
108+
// Generate 403 in any conditions to prevent user enumeration vulnerabilities
109+
throw new AccessDeniedException('Switch User failed: '.$e->getMessage(), $e);
109110
}
110111
}
111112

@@ -142,7 +143,24 @@ private function attemptSwitchUser(Request $request, $username)
142143
throw new \LogicException(sprintf('You are already switched to "%s" user.', $token->getUsername()));
143144
}
144145

145-
$user = $this->provider->loadUserByUsername($username);
146+
$currentUsername = $token->getUsername();
147+
$nonExistentUsername = '_'.md5(random_bytes(8).$username);
148+
149+
// To protect against user enumeration via timing measurements
150+
// we always load both successfully and unsuccessfully
151+
try {
152+
$user = $this->provider->loadUserByUsername($username);
153+
154+
try {
155+
$this->provider->loadUserByUsername($nonExistentUsername);
156+
throw new \LogicException('AuthenticationException expected');
157+
} catch (AuthenticationException $e) {
158+
}
159+
} catch (AuthenticationException $e) {
160+
$this->provider->loadUserByUsername($currentUsername);
161+
162+
throw $e;
163+
}
146164

147165
if (false === $this->accessDecisionManager->decide($token, [$this->role], $user)) {
148166
$exception = new AccessDeniedException();

‎Tests/Firewall/SwitchUserListenerTest.php

Lines changed: 47 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage;
1919
use Symfony\Component\Security\Core\Authentication\Token\SwitchUserToken;
2020
use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
21+
use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
2122
use Symfony\Component\Security\Core\Role\SwitchUserRole;
2223
use Symfony\Component\Security\Core\User\User;
2324
use Symfony\Component\Security\Http\Event\SwitchUserEvent;
@@ -174,6 +175,7 @@ public function testSwitchUserIsDisallowed()
174175
{
175176
$this->expectException('Symfony\Component\Security\Core\Exception\AccessDeniedException');
176177
$token = new UsernamePasswordToken('username', '', 'key', ['ROLE_FOO']);
178+
$user = new User('username', 'password', []);
177179

178180
$this->tokenStorage->setToken($token);
179181
$this->request->query->set('_switch_user', 'kuba');
@@ -182,6 +184,31 @@ public function testSwitchUserIsDisallowed()
182184
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'])
183185
->willReturn(false);
184186

187+
$this->userProvider->expects($this->exactly(2))
188+
->method('loadUserByUsername')
189+
->withConsecutive(['kuba'])
190+
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
191+
192+
$listener = new SwitchUserListener($this->tokenStorage, $this->userProvider, $this->userChecker, 'provider123', $this->accessDecisionManager);
193+
$listener($this->event);
194+
}
195+
196+
public function testSwitchUserTurnsAuthenticationExceptionTo403()
197+
{
198+
$this->expectException('Symfony\Component\Security\Core\Exception\AccessDeniedException');
199+
$token = new UsernamePasswordToken('username', '', 'key', ['ROLE_ALLOWED_TO_SWITCH']);
200+
201+
$this->tokenStorage->setToken($token);
202+
$this->request->query->set('_switch_user', 'kuba');
203+
204+
$this->accessDecisionManager->expects($this->never())
205+
->method('decide');
206+
207+
$this->userProvider->expects($this->exactly(2))
208+
->method('loadUserByUsername')
209+
->withConsecutive(['kuba'], ['username'])
210+
->will($this->onConsecutiveCalls($this->throwException(new UsernameNotFoundException())));
211+
185212
$listener = new SwitchUserListener($this->tokenStorage, $this->userProvider, $this->userChecker, 'provider123', $this->accessDecisionManager);
186213
$listener($this->event);
187214
}
@@ -198,9 +225,10 @@ public function testSwitchUser()
198225
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user)
199226
->willReturn(true);
200227

201-
$this->userProvider->expects($this->once())
202-
->method('loadUserByUsername')->with('kuba')
203-
->willReturn($user);
228+
$this->userProvider->expects($this->exactly(2))
229+
->method('loadUserByUsername')
230+
->withConsecutive(['kuba'])
231+
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
204232
$this->userChecker->expects($this->once())
205233
->method('checkPostAuth')->with($user);
206234

@@ -224,9 +252,10 @@ public function testSwitchUserWorksWithFalsyUsernames()
224252
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'])
225253
->willReturn(true);
226254

227-
$this->userProvider->expects($this->once())
228-
->method('loadUserByUsername')->with('0')
229-
->willReturn($user);
255+
$this->userProvider->expects($this->exactly(2))
256+
->method('loadUserByUsername')
257+
->withConsecutive(['0'])
258+
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
230259
$this->userChecker->expects($this->once())
231260
->method('checkPostAuth')->with($user);
232261

@@ -254,9 +283,10 @@ public function testSwitchUserKeepsOtherQueryStringParameters()
254283
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user)
255284
->willReturn(true);
256285

257-
$this->userProvider->expects($this->once())
258-
->method('loadUserByUsername')->with('kuba')
259-
->willReturn($user);
286+
$this->userProvider->expects($this->exactly(2))
287+
->method('loadUserByUsername')
288+
->withConsecutive(['kuba'])
289+
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
260290
$this->userChecker->expects($this->once())
261291
->method('checkPostAuth')->with($user);
262292

@@ -282,9 +312,10 @@ public function testSwitchUserWithReplacedToken()
282312
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user)
283313
->willReturn(true);
284314

285-
$this->userProvider->expects($this->any())
286-
->method('loadUserByUsername')->with('kuba')
287-
->willReturn($user);
315+
$this->userProvider->expects($this->exactly(2))
316+
->method('loadUserByUsername')
317+
->withConsecutive(['kuba'])
318+
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
288319

289320
$dispatcher = $this->getMockBuilder(EventDispatcherInterface::class)->getMock();
290321
$dispatcher
@@ -329,9 +360,10 @@ public function testSwitchUserStateless()
329360
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user)
330361
->willReturn(true);
331362

332-
$this->userProvider->expects($this->once())
333-
->method('loadUserByUsername')->with('kuba')
334-
->willReturn($user);
363+
$this->userProvider->expects($this->exactly(2))
364+
->method('loadUserByUsername')
365+
->withConsecutive(['kuba'])
366+
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
335367
$this->userChecker->expects($this->once())
336368
->method('checkPostAuth')->with($user);
337369

0 commit comments

Comments
 (0)