Merge pull request #29357 from nextcloud/fix/concurrent-duplicate-auth-token-updates

lib/private/Authentication/Token/PublicKeyTokenMapper.php

@@ -190,4 +190,43 @@ class PublicKeyTokenMapper extends QBMapper {
 
 		return count($data) === 1;
 	}
+
+	/**
+	 * Update the last activity timestamp
+	 *
+	 * In highly concurrent setups it can happen that two parallel processes
+	 * trigger the update at (nearly) the same time. In that special case it's
+	 * not necessary to hit the database with two actual updates. Therefore the
+	 * target last activity is included in the WHERE clause with a few seconds
+	 * of tolerance.
+	 *
+	 * Example:
+	 * - process 1 (P1) reads the token at timestamp 1500
+	 * - process 1 (P2) reads the token at timestamp 1501
+	 * - activity update interval is 100
+	 *
+	 * This means
+	 *
+	 * - P1 will see a last_activity smaller than the current time and update
+	 *   the token row
+	 * - If P2 reads after P1 had written, it will see 1600 as last activity
+	 *   and the comparison on last_activity won't be truthy. This means no rows
+	 *   need to be updated a second time
+	 * - If P2 reads before P1 had written, it will see 1501 as last activity,
+	 *   but the comparison on last_activity will still not be truthy and the
+	 *   token row is not updated a second time
+	 *
+	 * @param IToken $token
+	 * @param int $now
+	 */
+	public function updateActivity(IToken $token, int $now): void {
+		$qb = $this->db->getQueryBuilder();
+		$update = $qb->update($this->getTableName())
+			->set('last_activity', $qb->createNamedParameter($now, IQueryBuilder::PARAM_INT))
+			->where(
+				$qb->expr()->eq('id', $qb->createNamedParameter($token->getId(), IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT),
+				$qb->expr()->lt('last_activity', $qb->createNamedParameter($now - 15, IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT)
+			);
+		$update->executeStatement();
+	}
 }

lib/private/Authentication/Token/PublicKeyTokenProvider.php

@@ -221,9 +221,8 @@ class PublicKeyTokenProvider implements IProvider {
 		/** @var PublicKeyToken $token */
 		$now = $this->time->getTime();
 		if ($token->getLastActivity() < ($now - $activityInterval)) {
-			// Update token only once per minute
 			$token->setLastActivity($now);
-			$this->mapper->update($token);
+			$this->mapper->updateActivity($token, $now);
 		}
 	}
 

tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php

@@ -100,10 +100,10 @@ class PublicKeyTokenProviderTest extends TestCase {
 
 	public function testUpdateToken() {
 		$tk = new PublicKeyToken();
-		$tk->setLastActivity($this->time - 200);
 		$this->mapper->expects($this->once())
-			->method('update')
-			->with($tk);
+			->method('updateActivity')
+			->with($tk, $this->time);
+		$tk->setLastActivity($this->time - 200);
 
 		$this->tokenProvider->updateTokenActivity($tk);
 
@@ -112,16 +112,15 @@ class PublicKeyTokenProviderTest extends TestCase {
 
 	public function testUpdateTokenDebounce() {
 		$tk = new PublicKeyToken();
-
 		$this->config->method('getSystemValueInt')
 			->willReturnCallback(function ($value, $default) {
 				return $default;
 			});
-
 		$tk->setLastActivity($this->time - 30);
+
 		$this->mapper->expects($this->never())
-			->method('update')
-			->with($tk);
+			->method('updateActivity')
+			->with($tk, $this->time);
 
 		$this->tokenProvider->updateTokenActivity($tk);
 	}