[stable29] Wrap oauth2 token rotation in a transaction

apps/oauth2/lib/Controller/OauthApiController.php

@@ -40,6 +40,7 @@
 use OCP\Authentication\Exceptions\ExpiredTokenException;
 use OCP\Authentication\Exceptions\InvalidTokenException;
 use OCP\DB\Exception;
+use OCP\IDBConnection;
 use OCP\IRequest;
 use OCP\Security\Bruteforce\IThrottler;
 use OCP\Security\ICrypto;
@@ -62,6 +63,7 @@ public function __construct(
 		private LoggerInterface $logger,
 		private IThrottler $throttler,
 		private ITimeFactory $timeFactory,
+		private IDBConnection $db,
 	) {
 		parent::__construct($appName, $request);
 	}
@@ -193,27 +195,52 @@ public function getToken(
 
 		// Rotate the apptoken (so the old one becomes invalid basically)
 		$newToken = $this->secureRandom->generate(72, ISecureRandom::CHAR_ALPHANUMERIC);
+		$newCode = $this->secureRandom->generate(128, ISecureRandom::CHAR_ALPHANUMERIC);
+		$newEncryptedToken = $this->crypto->encrypt($newToken, $newCode);
+		$redeemedThrottleReason = $grant_type === 'authorization_code'
+			? 'authorization_code_already_redeemed'
+			: 'refresh_token_already_redeemed';
 
-		$appToken = $this->tokenProvider->rotate(
-			$appToken,
-			$decryptedToken,
-			$newToken
-		);
+		$this->db->beginTransaction();
+		try {
+			$updatedRows = $this->accessTokenMapper->rotateToken(
+				$accessToken->getId(),
+				$code,
+				$newCode,
+				$newEncryptedToken,
+				$grant_type === 'authorization_code',
+			);
+
+			if ($updatedRows !== 1) {
+				$this->db->rollBack();
+				$response = new JSONResponse([
+					'error' => 'invalid_request',
+				], Http::STATUS_BAD_REQUEST);
+				$response->throttle(['invalid_request' => $redeemedThrottleReason]);
+				return $response;
+			}
 
-		// Expiration is in 1 hour again
-		$appToken->setExpires($this->time->getTime() + 3600);
-		$this->tokenProvider->updateToken($appToken);
+			$appToken = $this->tokenProvider->rotate(
+				$appToken,
+				$decryptedToken,
+				$newToken
+			);
 
-		// Generate a new refresh token and encrypt the new apptoken in the DB
-		$newCode = $this->secureRandom->generate(128, ISecureRandom::CHAR_ALPHANUMERIC);
-		$accessToken->setHashedCode(hash('sha512', $newCode));
-		$accessToken->setEncryptedToken($this->crypto->encrypt($newToken, $newCode));
-		// increase the number of delivered oauth token
-		// this helps with cleaning up DB access token when authorization code has expired
-		// and it never delivered any oauth token
-		$tokenCount = $accessToken->getTokenCount();
-		$accessToken->setTokenCount($tokenCount + 1);
-		$this->accessTokenMapper->update($accessToken);
+			// Expiration is in 1 hour again
+			$appToken->setExpires($this->time->getTime() + 3600);
+			$this->tokenProvider->updateToken($appToken);
+
+			$this->db->commit();
+		} catch (\Throwable $e) {
+			if ($this->db->inTransaction()) {
+				$this->db->rollBack();
+			}
+			// rotate() and updateToken() write the auth token to the cache,
+			// so if we are past rotate() we must invalidate the new token
+			$this->tokenProvider->invalidateToken($newToken);
+
+			throw $e;
+		}
 
 		$this->throttler->resetDelay($this->request->getRemoteAddress(), 'login', ['user' => $appToken->getUID()]);
 

apps/oauth2/lib/Db/AccessTokenMapper.php

@@ -102,4 +102,31 @@ public function cleanupExpiredAuthorizationCode(): void {
 			->andWhere($qb->expr()->lt('code_created_at', $qb->createNamedParameter($maxTokenCreationTs, IQueryBuilder::PARAM_INT)));
 		$qb->executeStatement();
 	}
+
+	/**
+	 * Rotate an access token only if it still matches the caller's previously-read state.
+	 *
+	 * @param int $id
+	 * @param string $oldCode
+	 * @param string $newCode
+	 * @param string $encryptedToken
+	 * @param bool $expectAuthorizationCodeState Require the token to still be unused
+	 * @return int Number of updated rows
+	 */
+	public function rotateToken(int $id, string $oldCode, string $newCode, string $encryptedToken, bool $expectAuthorizationCodeState): int {
+		$qb = $this->db->getQueryBuilder();
+		$qb
+			->update($this->tableName)
+			->set('hashed_code', $qb->createNamedParameter(hash('sha512', $newCode)))
+			->set('encrypted_token', $qb->createNamedParameter($encryptedToken))
+			->set('token_count', $qb->createFunction('token_count + 1'))
+			->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT)))
+			->andWhere($qb->expr()->eq('hashed_code', $qb->createNamedParameter(hash('sha512', $oldCode))));
+
+		if ($expectAuthorizationCodeState) {
+			$qb->andWhere($qb->expr()->eq('token_count', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)));
+		}
+
+		return $qb->executeStatement();
+	}
 }

apps/oauth2/tests/Controller/OauthApiControllerTest.php

@@ -39,6 +39,7 @@
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\JSONResponse;
 use OCP\AppFramework\Utility\ITimeFactory;
+use OCP\IDBConnection;
 use OCP\IRequest;
 use OCP\Security\Bruteforce\IThrottler;
 use OCP\Security\ICrypto;
@@ -72,6 +73,8 @@ class OauthApiControllerTest extends TestCase {
 	private $logger;
 	/** @var ITimeFactory|\PHPUnit\Framework\MockObject\MockObject */
 	private $timeFactory;
+	/** @var IDBConnection|\PHPUnit\Framework\MockObject\MockObject */
+	private $db;
 	/** @var OauthApiController */
 	private $oauthApiController;
 
@@ -88,6 +91,7 @@ protected function setUp(): void {
 		$this->throttler = $this->createMock(IThrottler::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
 		$this->timeFactory = $this->createMock(ITimeFactory::class);
+		$this->db = $this->createMock(IDBConnection::class);
 
 		$this->oauthApiController = new OauthApiController(
 			'oauth2',
@@ -100,7 +104,8 @@ protected function setUp(): void {
 			$this->time,
 			$this->logger,
 			$this->throttler,
-			$this->timeFactory
+			$this->timeFactory,
+			$this->db,
 		);
 	}
 
@@ -335,6 +340,7 @@ public function testRefreshTokenInvalidAppToken() {
 
 	public function testRefreshTokenValidAppToken() {
 		$accessToken = new AccessToken();
+		$accessToken->setId(21);
 		$accessToken->setClientId(42);
 		$accessToken->setTokenId(1337);
 		$accessToken->setEncryptedToken('encryptedToken');
@@ -386,6 +392,18 @@ public function testRefreshTokenValidAppToken() {
 		$this->time->method('getTime')
 			->willReturn(1000);
 
+		$this->db->expects($this->once())
+			->method('beginTransaction');
+
+		$this->db->expects($this->once())
+			->method('commit');
+
+		$this->db->expects($this->never())
+			->method('rollBack');
+
+		$this->tokenProvider->expects($this->never())
+			->method('invalidateToken');
+
 		$this->tokenProvider->expects($this->once())
 			->method('updateToken')
 			->with(
@@ -399,13 +417,14 @@ public function testRefreshTokenValidAppToken() {
 			->willReturn('newEncryptedToken');
 
 		$this->accessTokenMapper->expects($this->once())
-			->method('update')
+			->method('rotateToken')
 			->with(
-				$this->callback(function (AccessToken $token) {
-					return $token->getHashedCode() === hash('sha512', 'random128') &&
-						$token->getEncryptedToken() === 'newEncryptedToken';
-				})
-			);
+				21,
+				'validrefresh',
+				'random128',
+				'newEncryptedToken',
+				false,
+			)->willReturn(1);
 
 		$expected = new JSONResponse([
 			'access_token' => 'random72',
@@ -431,6 +450,7 @@ public function testRefreshTokenValidAppToken() {
 
 	public function testRefreshTokenValidAppTokenBasicAuth() {
 		$accessToken = new AccessToken();
+		$accessToken->setId(21);
 		$accessToken->setClientId(42);
 		$accessToken->setTokenId(1337);
 		$accessToken->setEncryptedToken('encryptedToken');
@@ -482,6 +502,18 @@ public function testRefreshTokenValidAppTokenBasicAuth() {
 		$this->time->method('getTime')
 			->willReturn(1000);
 
+		$this->db->expects($this->once())
+			->method('beginTransaction');
+
+		$this->db->expects($this->once())
+			->method('commit');
+
+		$this->db->expects($this->never())
+			->method('rollBack');
+
+		$this->tokenProvider->expects($this->never())
+			->method('invalidateToken');
+
 		$this->tokenProvider->expects($this->once())
 			->method('updateToken')
 			->with(
@@ -495,13 +527,14 @@ public function testRefreshTokenValidAppTokenBasicAuth() {
 			->willReturn('newEncryptedToken');
 
 		$this->accessTokenMapper->expects($this->once())
-			->method('update')
+			->method('rotateToken')
 			->with(
-				$this->callback(function (AccessToken $token) {
-					return $token->getHashedCode() === hash('sha512', 'random128') &&
-						$token->getEncryptedToken() === 'newEncryptedToken';
-				})
-			);
+				21,
+				'validrefresh',
+				'random128',
+				'newEncryptedToken',
+				false,
+			)->willReturn(1);
 
 		$expected = new JSONResponse([
 			'access_token' => 'random72',
@@ -530,6 +563,7 @@ public function testRefreshTokenValidAppTokenBasicAuth() {
 
 	public function testRefreshTokenExpiredAppToken() {
 		$accessToken = new AccessToken();
+		$accessToken->setId(21);
 		$accessToken->setClientId(42);
 		$accessToken->setTokenId(1337);
 		$accessToken->setEncryptedToken('encryptedToken');
@@ -581,6 +615,18 @@ public function testRefreshTokenExpiredAppToken() {
 		$this->time->method('getTime')
 			->willReturn(1000);
 
+		$this->db->expects($this->once())
+			->method('beginTransaction');
+
+		$this->db->expects($this->once())
+			->method('commit');
+
+		$this->db->expects($this->never())
+			->method('rollBack');
+
+		$this->tokenProvider->expects($this->never())
+			->method('invalidateToken');
+
 		$this->tokenProvider->expects($this->once())
 			->method('updateToken')
 			->with(
@@ -594,13 +640,14 @@ public function testRefreshTokenExpiredAppToken() {
 			->willReturn('newEncryptedToken');
 
 		$this->accessTokenMapper->expects($this->once())
-			->method('update')
+			->method('rotateToken')
 			->with(
-				$this->callback(function (AccessToken $token) {
-					return $token->getHashedCode() === hash('sha512', 'random128') &&
-						$token->getEncryptedToken() === 'newEncryptedToken';
-				})
-			);
+				21,
+				'validrefresh',
+				'random128',
+				'newEncryptedToken',
+				false,
+			)->willReturn(1);
 
 		$expected = new JSONResponse([
 			'access_token' => 'random72',
@@ -623,4 +670,89 @@ public function testRefreshTokenExpiredAppToken() {
 
 		$this->assertEquals($expected, $this->oauthApiController->getToken('refresh_token', null, 'validrefresh', 'clientId', 'clientSecret'));
 	}
+
+	public function testRefreshTokenRedeemedConcurrently(): void {
+		$expected = new JSONResponse([
+			'error' => 'invalid_request',
+		], Http::STATUS_BAD_REQUEST);
+		$expected->throttle(['invalid_request' => 'refresh_token_already_redeemed']);
+
+		$accessToken = new AccessToken();
+		$accessToken->setId(21);
+		$accessToken->setClientId(42);
+		$accessToken->setTokenId(1337);
+		$accessToken->setEncryptedToken('encryptedToken');
+
+		$this->accessTokenMapper->method('getByCode')
+			->with('validrefresh')
+			->willReturn($accessToken);
+
+		$client = new Client();
+		$client->setClientIdentifier('clientId');
+		$client->setSecret(bin2hex('hashedClientSecret'));
+		$this->clientMapper->method('getByUid')
+			->with(42)
+			->willReturn($client);
+
+		$this->crypto
+			->method('decrypt')
+			->with('encryptedToken')
+			->willReturn('decryptedToken');
+
+		$this->crypto
+			->method('calculateHMAC')
+			->with('clientSecret')
+			->willReturn('hashedClientSecret');
+
+		$appToken = new PublicKeyToken();
+		$appToken->setUid('userId');
+		$this->tokenProvider->method('getTokenById')
+			->with(1337)
+			->willReturn($appToken);
+
+		$this->secureRandom->method('generate')
+			->willReturnCallback(function ($len) {
+				return 'random' . $len;
+			});
+
+		$this->tokenProvider->expects($this->never())
+			->method('rotate');
+
+		$this->time->method('getTime')
+			->willReturn(1000);
+
+		$this->tokenProvider->expects($this->never())
+			->method('updateToken');
+
+		$this->crypto->method('encrypt')
+			->with('random72', 'random128')
+			->willReturn('newEncryptedToken');
+
+		$this->db->expects($this->once())
+			->method('beginTransaction');
+
+		$this->db->expects($this->never())
+			->method('commit');
+
+		$this->db->expects($this->once())
+			->method('rollBack');
+
+		$this->tokenProvider->expects($this->never())
+			->method('invalidateToken');
+
+		$this->accessTokenMapper->expects($this->once())
+			->method('rotateToken')
+			->with(
+				21,
+				'validrefresh',
+				'random128',
+				'newEncryptedToken',
+				false,
+			)->willReturn(0);
+
+		$this->throttler->expects($this->never())
+			->method('resetDelay');
+
+		$this->assertEquals($expected, $this->oauthApiController->getToken('refresh_token', null, 'validrefresh', 'clientId', 'clientSecret'));
+	}
 }