diff --git a/lib/AuthModule.php b/lib/AuthModule.php index 389f3bc..f2ce950 100644 --- a/lib/AuthModule.php +++ b/lib/AuthModule.php @@ -113,11 +113,7 @@ public function authToken($bearerToken): ?IUser { /** @var \OCP\IUserManager $userManager */ $userManager = $container->query('UserManager'); - $userId = $accessToken->getUserId(); - if (\strstr($userId, ':')) { - list(1 => $userId) = \explode(':', $userId, 2); - } - return $userManager->get($userId); + return $userManager->get($accessToken->getUserId()); } protected function tokenCanBeHandledByOpenIDConnect(): bool { diff --git a/lib/Controller/OAuthApiController.php b/lib/Controller/OAuthApiController.php index 13a8986..c459f7b 100644 --- a/lib/Controller/OAuthApiController.php +++ b/lib/Controller/OAuthApiController.php @@ -192,11 +192,6 @@ public function generateToken( $userId = $authorizationCode->getUserId(); - // strip off username if it exists - if (\strstr($userId, ':')) { - list(, $userId) = \explode(':', $userId, 2); - } - $this->authorizationCodeMapper->delete($authorizationCode); $userObj = $this->userManager->get($userId); @@ -235,11 +230,6 @@ public function generateToken( $userId = $refreshToken->getUserId(); - // strip off username if it exists - if (\strstr($userId, ':')) { - list(, $userId) = \explode(':', $userId, 2); - } - $userObj = $this->userManager->get($userId); if ($userObj === null || !$userObj->isEnabled()) { $this->logger->debug("the matching user is missing or disabled", ['app'=>__CLASS__]); diff --git a/tests/unit/AuthModuleTest.php b/tests/unit/AuthModuleTest.php index 3d35a2c..f347b95 100755 --- a/tests/unit/AuthModuleTest.php +++ b/tests/unit/AuthModuleTest.php @@ -37,9 +37,6 @@ class AuthModuleTest extends TestCase { /** @var String $userId */ private $userId = 'john'; - /** @var String $userIdConcat */ - private $userIdConcat = 'John Doe:john'; - /** @var ClientMapper $clientMapper */ private $clientMapper; @@ -118,18 +115,18 @@ public function testAuth() { $user = $this->authModule->auth($request); $this->assertNotNull($user); $this->assertEquals($this->userId, $user->getUID()); + } - // Valid request with ConcatUserID - $request = $this->getMockBuilder(IRequest::class)->getMock(); - $this->accessToken->setUserId($this->userIdConcat); - $this->accessToken = $this->accessTokenMapper->update($this->accessToken); - $request->expects($this->once()) - ->method('getHeader') - ->with($this->equalTo('Authorization')) - ->will($this->returnValue('Bearer ' . $this->accessToken->getToken())); - $user = $this->authModule->auth($request); - $this->assertNotNull($user); - $this->assertEquals($this->userId, $user->getUID()); + /** + * The user id stored on the token is opaque - a colon in it must not be + * treated as a "login name:user id" separator, otherwise the token + * authenticates a different account than the one it was issued for. + */ + public function testAuthTokenKeepsUserIdWithColon() { + $this->accessToken->setUserId('attacker:' . $this->userId); + $this->accessTokenMapper->update($this->accessToken); + + $this->assertNull($this->authModule->authToken($this->accessToken->getToken())); } /** diff --git a/tests/unit/Controller/OAuthApiControllerTest.php b/tests/unit/Controller/OAuthApiControllerTest.php index 5e36b20..ea2da0b 100755 --- a/tests/unit/Controller/OAuthApiControllerTest.php +++ b/tests/unit/Controller/OAuthApiControllerTest.php @@ -625,6 +625,76 @@ public function testGenerateTokenWithAuthorizationCodeAndPKCES256() { $this->assertNotEmpty($json->token_type); $this->assertEquals('Bearer', $json->token_type); } + /** + * The stored user id is opaque - a colon in it must not be treated as a + * "login name:user id" separator, otherwise the token is issued for a + * different account than the one which authorized the code. + */ + public function testGenerateTokenWithAuthorizationCodeKeepsUserIdWithColon() { + $_SERVER['PHP_AUTH_USER'] = $this->clientIdentifier1; + $_SERVER['PHP_AUTH_PW'] = $this->clientSecret; + + $userIdWithColon = 'attacker:' . $this->userId; + $this->authorizationCode->setUserId($userIdWithColon); + $this->authorizationCodeMapper->update($this->authorizationCode); + + $requestedUserIds = []; + $this->mockUserManagerFor($userIdWithColon, $requestedUserIds); + + $result = $this->controller->generateToken( + 'authorization_code', + $this->authorizationCode->getCode(), + $this->redirectUri + ); + $this->assertEquals(200, $result->getStatus()); + $json = \json_decode($result->render()); + $this->assertEquals([$userIdWithColon], $requestedUserIds); + $this->assertEquals($userIdWithColon, $json->user_id); + $this->assertEquals($userIdWithColon, $this->accessTokenMapper->findByToken($json->access_token)->getUserId()); + $this->assertEquals($userIdWithColon, $this->refreshTokenMapper->findByToken($json->refresh_token)->getUserId()); + } + + /** + * @see testGenerateTokenWithAuthorizationCodeKeepsUserIdWithColon + */ + public function testGenerateTokenWithRefreshTokenKeepsUserIdWithColon() { + $_SERVER['PHP_AUTH_USER'] = $this->clientIdentifier1; + $_SERVER['PHP_AUTH_PW'] = $this->clientSecret; + + $userIdWithColon = 'attacker:' . $this->userId; + $this->refreshToken->setUserId($userIdWithColon); + $this->refreshTokenMapper->update($this->refreshToken); + + $requestedUserIds = []; + $this->mockUserManagerFor($userIdWithColon, $requestedUserIds); + + $result = $this->controller->generateToken('refresh_token', null, null, $this->refreshToken->getToken()); + $this->assertEquals(200, $result->getStatus()); + $json = \json_decode($result->render()); + $this->assertEquals([$userIdWithColon], $requestedUserIds); + $this->assertEquals($userIdWithColon, $json->user_id); + $this->assertEquals($userIdWithColon, $this->accessTokenMapper->findByToken($json->access_token)->getUserId()); + $this->assertEquals($userIdWithColon, $this->refreshTokenMapper->findByToken($json->refresh_token)->getUserId()); + } + + /** + * Lets the user manager resolve $userId only, and records every requested id. + * + * @param string $userId The only user id which resolves to an enabled user. + * @param array $requestedUserIds Collects the requested user ids. + * @return void + */ + private function mockUserManagerFor($userId, array &$requestedUserIds) { + $userMock = $this->createMock(IUser::class); + $userMock->method('isEnabled')->willReturn(true); + + $this->userManager->method('get') + ->willReturnCallback(function ($requestedUserId) use ($userId, $userMock, &$requestedUserIds) { + $requestedUserIds[] = $requestedUserId; + return $requestedUserId === $userId ? $userMock : null; + }); + } + public function testGenerateTokenWithAuthorizationCodeAndPKCESInvalidChallengeMethod() { $_SERVER['PHP_AUTH_USER'] = $this->clientIdentifier1; $_SERVER['PHP_AUTH_PW'] = $this->clientSecret;