bug #33799 [Security]: Don't let falsy usernames slip through impersonation (j4nr6n)

chalasr · chalasr · commit 8622c8c95e2b · 2019-10-03T14:19:04.000+02:00
This PR was merged into the 3.4 branch. Discussion ---------- [Security]: Don't let falsy usernames slip through impersonation | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | When you try to impersonate users with a falsy username, `SwitchUserListener::handle()` would `return;` and impersonation would fail. I'm using a third party OAuth provider that allows users to change their usernames with no guaranteed protection against re-use. To overcome that issue, I implemented `UserLoaderInterface::loadUserByUsername()` and query by a `providerId`. After loading development fixtures, One user has `0` as it's `providerId`. Commits ------- 64aecab Don't let falsey usernames slip through
diff --git a/src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php b/src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php
@@ -77,9 +77,16 @@ public function __construct(TokenStorageInterface $tokenStorage, UserProviderInt
     public function handle(GetResponseEvent $event)
     {
         $request = $event->getRequest();
-        $username = $request->get($this->usernameParameter) ?: $request->headers->get($this->usernameParameter);
 
-        if (!$username) {
+        // usernames can be falsy
+        $username = $request->get($this->usernameParameter);
+
+        if (null === $username || '' === $username) {
+            $username = $request->headers->get($this->usernameParameter);
+        }
+
+        // if it's still "empty", nothing to do.
+        if (null === $username || '' === $username) {
             return;
         }
 
diff --git a/src/Symfony/Component/Security/Http/Tests/Firewall/SwitchUserListenerTest.php b/src/Symfony/Component/Security/Http/Tests/Firewall/SwitchUserListenerTest.php
@@ -191,6 +191,32 @@ public function testSwitchUser()
         $this->assertInstanceOf('Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken', $this->tokenStorage->getToken());
     }
 
+    public function testSwitchUserWorksWithFalsyUsernames()
+    {
+        $token = new UsernamePasswordToken('username', '', 'key', ['ROLE_FOO']);
+        $user = new User('username', 'password', []);
+
+        $this->tokenStorage->setToken($token);
+        $this->request->query->set('_switch_user', '0');
+
+        $this->accessDecisionManager->expects($this->once())
+            ->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'])
+            ->willReturn(true);
+
+        $this->userProvider->expects($this->once())
+            ->method('loadUserByUsername')->with('0')
+            ->willReturn($user);
+        $this->userChecker->expects($this->once())
+            ->method('checkPostAuth')->with($user);
+
+        $listener = new SwitchUserListener($this->tokenStorage, $this->userProvider, $this->userChecker, 'provider123', $this->accessDecisionManager);
+        $listener->handle($this->event);
+
+        $this->assertSame([], $this->request->query->all());
+        $this->assertSame('', $this->request->server->get('QUERY_STRING'));
+        $this->assertInstanceOf('Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken', $this->tokenStorage->getToken());
+    }
+
     public function testSwitchUserKeepsOtherQueryStringParameters()
     {
         $token = new UsernamePasswordToken('username', '', 'key', ['ROLE_FOO']);
