fix(oauth): rotate the auth token only if the access token rotation was successful

fix(oauth): rotate the auth token only if the access token rotation was successful

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>

[skip ci]

Author: julien-nc

apps/oauth2/lib/Controller/OauthApiController.php

@@ -184,21 +184,9 @@ public function getToken(
 		$redeemedThrottleReason = $grant_type === 'authorization_code'
 			? 'authorization_code_already_redeemed'
 			: 'refresh_token_already_redeemed';
-		$tokenRotated = false;
 
 		$this->db->beginTransaction();
 		try {
-			$appToken = $this->tokenProvider->rotate(
-				$appToken,
-				$decryptedToken,
-				$newToken
-			);
-			$tokenRotated = true;
-
-			// Expiration is in 1 hour again
-			$appToken->setExpires($this->time->getTime() + 3600);
-			$this->tokenProvider->updateToken($appToken);
-
 			$updatedRows = $this->accessTokenMapper->rotateToken(
 				$accessToken->getId(),
 				$code,
@@ -209,25 +197,31 @@ public function getToken(
 
 			if ($updatedRows !== 1) {
 				$this->db->rollBack();
-				// tokenProvider->rotate() updates the auth token cache, so we have to clear the new token on rollback
-				$this->tokenProvider->invalidateToken($newToken);
 				$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();
 			}
-
-			if ($tokenRotated) {
-				// tokenProvider->rotate() updates the auth token cache, so we have to clear the new token on rollback
-				$this->tokenProvider->invalidateToken($newToken);
-			}
+			// 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;
 		}

apps/oauth2/tests/Controller/OauthApiControllerTest.php

@@ -692,20 +692,14 @@ public function testRefreshTokenRedeemedConcurrently(): void {
 				return 'random' . $len;
 			});
 
-		$this->tokenProvider->expects($this->once())
-			->method('rotate')
-			->with(
-				$appToken,
-				'decryptedToken',
-				'random72'
-			)->willReturn($appToken);
+		$this->tokenProvider->expects($this->never())
+			->method('rotate');
 
 		$this->time->method('getTime')
 			->willReturn(1000);
 
-		$this->tokenProvider->expects($this->once())
-			->method('updateToken')
-			->with($this->isInstanceOf(PublicKeyToken::class));
+		$this->tokenProvider->expects($this->never())
+			->method('updateToken');
 
 		$this->crypto->method('encrypt')
 			->with('random72', 'random128')
@@ -717,16 +711,11 @@ public function testRefreshTokenRedeemedConcurrently(): void {
 		$this->db->expects($this->never())
 			->method('commit');
 
-		$this->db->expects($this->exactly(2))
-			->method('inTransaction')
-			->willReturnOnConsecutiveCalls(true, false);
-
 		$this->db->expects($this->once())
 			->method('rollBack');
 
-		$this->tokenProvider->expects($this->once())
-			->method('invalidateToken')
-			->with('random72');
+		$this->tokenProvider->expects($this->never())
+			->method('invalidateToken');
 
 		$this->accessTokenMapper->expects($this->once())
 			->method('rotateToken')