Merge pull request #59877 from nextcloud/backport/59767/stable31

apps/oauth2/lib/Controller/OauthApiController.php

@@ -24,6 +24,7 @@ use OCP\AppFramework\Utility\ITimeFactory;
 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;
@@ -47,6 +48,7 @@ class OauthApiController extends Controller {
 		private LoggerInterface $logger,
 		private IThrottler $throttler,
 		private ITimeFactory $timeFactory,
+		private IDBConnection $db,
 	) {
 		parent::__construct($appName, $request);
 	}
@@ -177,27 +179,52 @@ class OauthApiController extends Controller {
 
 		// Rotate the apptoken (so the old one becomes invalid basically)
 		$newToken = $this->secureRandom->generate(72, ISecureRandom::CHAR_ALPHANUMERIC);
-
-		$appToken = $this->tokenProvider->rotate(
-			$appToken,
-			$decryptedToken,
-			$newToken
-		);
-
-		// Expiration is in 1 hour again
-		$appToken->setExpires($this->time->getTime() + 3600);
-		$this->tokenProvider->updateToken($appToken);
-
-		// 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);
+		$newEncryptedToken = $this->crypto->encrypt($newToken, $newCode);
+		$redeemedThrottleReason = $grant_type === 'authorization_code'
+			? 'authorization_code_already_redeemed'
+			: 'refresh_token_already_redeemed';
+
+		$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;
+			}
+
+			$appToken = $this->tokenProvider->rotate(
+				$appToken,
+				$decryptedToken,
+				$newToken
+			);
+
+			// 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

@@ -82,4 +82,31 @@ class AccessTokenMapper extends QBMapper {
 			->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

@@ -19,6 +19,7 @@ use OCA\OAuth2\Exceptions\ClientNotFoundException;
 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;
@@ -52,6 +53,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;
 
@@ -68,6 +71,7 @@ class OauthApiControllerTest extends TestCase {
 		$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',
@@ -80,7 +84,8 @@ class OauthApiControllerTest extends TestCase {
 			$this->time,
 			$this->logger,
 			$this->throttler,
-			$this->timeFactory
+			$this->timeFactory,
+			$this->db,
 		);
 	}
 
@@ -315,6 +320,7 @@ class OauthApiControllerTest extends TestCase {
 
 	public function testRefreshTokenValidAppToken(): void {
 		$accessToken = new AccessToken();
+		$accessToken->setId(21);
 		$accessToken->setClientId(42);
 		$accessToken->setTokenId(1337);
 		$accessToken->setEncryptedToken('encryptedToken');
@@ -366,6 +372,18 @@ class OauthApiControllerTest extends TestCase {
 		$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(
@@ -379,13 +397,14 @@ class OauthApiControllerTest extends TestCase {
 			->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',
@@ -411,6 +430,7 @@ class OauthApiControllerTest extends TestCase {
 
 	public function testRefreshTokenValidAppTokenBasicAuth(): void {
 		$accessToken = new AccessToken();
+		$accessToken->setId(21);
 		$accessToken->setClientId(42);
 		$accessToken->setTokenId(1337);
 		$accessToken->setEncryptedToken('encryptedToken');
@@ -462,6 +482,18 @@ class OauthApiControllerTest extends TestCase {
 		$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(
@@ -475,13 +507,14 @@ class OauthApiControllerTest extends TestCase {
 			->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',
@@ -510,6 +543,7 @@ class OauthApiControllerTest extends TestCase {
 
 	public function testRefreshTokenExpiredAppToken(): void {
 		$accessToken = new AccessToken();
+		$accessToken->setId(21);
 		$accessToken->setClientId(42);
 		$accessToken->setTokenId(1337);
 		$accessToken->setEncryptedToken('encryptedToken');
@@ -561,6 +595,18 @@ class OauthApiControllerTest extends TestCase {
 		$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(
@@ -574,13 +620,14 @@ class OauthApiControllerTest extends TestCase {
 			->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',
@@ -603,4 +650,89 @@ class OauthApiControllerTest extends TestCase {
 
 		$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'));
+	}
 }