fix: treat the OAuth2 user id as opaque

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 {

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__]);

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()));
 	}
 
 	/**

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;