Oauth2: Prevent redeeming an unused authorization code via the refresh token grant

apps/oauth2/lib/Controller/OauthApiController.php

@@ -227,7 +227,7 @@ public function getToken(
 				$code,
 				$newCode,
 				$newEncryptedToken,
-				$grant_type === 'authorization_code',
+				$grant_type,
 			);
 
 			if ($updatedRows !== 1) {

apps/oauth2/lib/Db/AccessTokenMapper.php

@@ -66,12 +66,14 @@ public function cleanupExpiredAuthorizationCode(ITimeFactory $timeFactory): void
 	}
 
 	/**
-	 * Rotate an access token only if it still matches the caller's previously-read state.
+	 * Rotate an access token only if it still matches the caller's previously-read state
+	 * and its token count matches the requested grant type.
 	 *
-	 * @param bool $expectAuthorizationCodeState Require the token to still be unused
+	 * @param 'authorization_code'|'refresh_token' $grantType Require the row to still be unused for 'authorization_code'
+	 *                                                        or to have already delivered one token for 'refresh_token'
 	 * @return int Number of updated rows
 	 */
-	public function rotateToken(int $id, string $oldCode, string $newCode, string $encryptedToken, bool $expectAuthorizationCodeState): int {
+	public function rotateToken(int $id, string $oldCode, string $newCode, string $encryptedToken, string $grantType): int {
 		$qb = $this->connection->getQueryBuilder();
 		$qb
 			->update($this->getTableName())
@@ -81,8 +83,12 @@ public function rotateToken(int $id, string $oldCode, string $newCode, string $e
 			->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT)))
 			->andWhere($qb->expr()->eq('hashed_code', $qb->createNamedParameter(hash('sha512', $oldCode))));
 
-		if ($expectAuthorizationCodeState) {
+		if ($grantType === 'authorization_code') {
 			$qb->andWhere($qb->expr()->eq('token_count', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)));
+		} else {
+			// an unused (possibly expired) authorization code
+			// must not be redeemed through the refresh token grant
+			$qb->andWhere($qb->expr()->gt('token_count', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)));
 		}
 
 		return $qb->executeStatement();

apps/oauth2/tests/Controller/OauthApiControllerTest.php

@@ -425,7 +425,7 @@ public function testRefreshTokenValidAppToken(): void {
 				'validrefresh',
 				'random128',
 				'newEncryptedToken',
-				false,
+				'refresh_token',
 			)->willReturn(1);
 
 		$expected = new JSONResponse([
@@ -531,7 +531,7 @@ public function testRefreshTokenValidAppTokenBasicAuth(): void {
 				'validrefresh',
 				'random128',
 				'newEncryptedToken',
-				false,
+				'refresh_token',
 			)->willReturn(1);
 
 		$expected = new JSONResponse([
@@ -640,7 +640,7 @@ public function testRefreshTokenExpiredAppToken(): void {
 				'validrefresh',
 				'random128',
 				'newEncryptedToken',
-				false,
+				'refresh_token',
 			)->willReturn(1);
 
 		$expected = new JSONResponse([
@@ -739,7 +739,7 @@ public function testRefreshTokenRedeemedConcurrently(): void {
 				'validrefresh',
 				'random128',
 				'newEncryptedToken',
-				false,
+				'refresh_token',
 			)->willReturn(0);
 
 		$this->throttler->expects($this->never())

apps/oauth2/tests/Db/AccessTokenMapperTest.php

@@ -54,4 +54,70 @@ public function testDeleteByClientId(): void {
 		$this->accessTokenMapper->deleteByClientId(1234);
 		$this->accessTokenMapper->getByCode('MyAwesomeToken');
 	}
+
+	private function insertToken(int $clientId, int $tokenCount): AccessToken {
+		$token = new AccessToken();
+		$token->clientId = $clientId;
+		$token->tokenId = time();
+		$token->encryptedToken = 'MyEncryptedToken';
+		$token->hashedCode = hash('sha512', 'MyAwesomeToken');
+		$token->tokenCount = $tokenCount;
+
+		$this->accessTokenMapper->insert($token);
+		return $token;
+	}
+
+	public function testRotateTokenInAuthorizationCodeState(): void {
+		$this->accessTokenMapper->deleteByClientId(1235);
+		$token = $this->insertToken(1235, 0);
+
+		$rotated = $this->accessTokenMapper->rotateToken($token->id, 'MyAwesomeToken', 'rotatedcode', 'OtherEncryptedToken', 'authorization_code');
+		$this->assertEquals(1, $rotated);
+
+		$row = $this->accessTokenMapper->getByCode('rotatedcode');
+		$this->assertEquals('OtherEncryptedToken', $row->encryptedToken);
+		$this->assertEquals(1, $row->tokenCount);
+
+		$this->accessTokenMapper->deleteByClientId(1235);
+	}
+
+	public function testRotateTokenWithRefreshTokenGrantOnUnusedToken(): void {
+		$this->accessTokenMapper->deleteByClientId(1235);
+		$token = $this->insertToken(1235, 0);
+
+		// an unused authorization code is not a valid refresh token
+		$rotated = $this->accessTokenMapper->rotateToken($token->id, 'MyAwesomeToken', 'rotatedcode', 'OtherEncryptedToken', 'refresh_token');
+		$this->assertEquals(0, $rotated);
+
+		$row = $this->accessTokenMapper->getByCode('MyAwesomeToken');
+		$this->assertEquals('MyEncryptedToken', $row->encryptedToken);
+		$this->assertEquals(0, $row->tokenCount);
+
+		$this->accessTokenMapper->deleteByClientId(1235);
+	}
+
+	public function testRotateTokenInRefreshTokenState(): void {
+		$this->accessTokenMapper->deleteByClientId(1235);
+		$token = $this->insertToken(1235, 1);
+
+		$rotated = $this->accessTokenMapper->rotateToken($token->id, 'MyAwesomeToken', 'rotatedcode', 'OtherEncryptedToken', 'refresh_token');
+		$this->assertEquals(1, $rotated);
+
+		$row = $this->accessTokenMapper->getByCode('rotatedcode');
+		$this->assertEquals('OtherEncryptedToken', $row->encryptedToken);
+		$this->assertEquals(2, $row->tokenCount);
+
+		$this->accessTokenMapper->deleteByClientId(1235);
+	}
+
+	public function testRotateTokenWithAuthorizationCodeGrantOnUsedToken(): void {
+		$this->accessTokenMapper->deleteByClientId(1235);
+		$token = $this->insertToken(1235, 1);
+
+		// an already used token can't be processed through the authorization code grant
+		$rotated = $this->accessTokenMapper->rotateToken($token->id, 'MyAwesomeToken', 'rotatedcode', 'OtherEncryptedToken', 'authorization_code');
+		$this->assertEquals(0, $rotated);
+
+		$this->accessTokenMapper->deleteByClientId(1235);
+	}
 }