From 5c9ab5d098f2318db026698269b94b0984c21d9e Mon Sep 17 00:00:00 2001
From: Christoph Wurst <christoph@winzerhof-wurst.at>
Date: Thu, 21 Oct 2021 14:12:36 +0200
Subject: [PATCH] Prevent duplicate auth token activity updates

The auth token activity logic works as follows
* Read auth token
* Compare last activity time stamp to current time
* Update auth token activity if it's older than x seconds

This works fine in isolation but with concurrency that means that
occasionally the same token is read simultaneously by two processes and
both of these processes will trigger an update of the same row.
Affectively the second update doesn't add much value. It might set the
time stamp to the exact same time stamp or one a few seconds later. But
the last activity is no precise science, we don't need this accuracy.

This patch changes the UPDATE query to include the expected value in a
comparison with the current data. This results in an affected row when
the data in the DB still has an old time stamp, but won't affect a row
if the time stamp is (nearly) up to date.

This is a micro optimization and will possibly not show any significant
performance improvement. Yet in setups with a DB cluster it means that
the write node has to send fewer changes to the read nodes due to the
lower number of actual changes.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
---
 .../Token/PublicKeyTokenMapper.php            | 40 +++++++++++++++++++
 .../Token/PublicKeyTokenProvider.php          |  3 +-
 .../Token/PublicKeyTokenProviderTest.php      | 10 ++---
 3 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/lib/private/Authentication/Token/PublicKeyTokenMapper.php b/lib/private/Authentication/Token/PublicKeyTokenMapper.php
index 8aab7daf62308..5db342f524d8b 100644
--- a/lib/private/Authentication/Token/PublicKeyTokenMapper.php
+++ b/lib/private/Authentication/Token/PublicKeyTokenMapper.php
@@ -29,6 +29,7 @@
 
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Db\QBMapper;
+use OCP\DB\Exception;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\IDBConnection;
 
@@ -190,4 +191,43 @@ public function hasExpiredTokens(string $uid): bool {
 
 		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())),
+				$qb->expr()->lt('last_activity', $qb->createNamedParameter($now - 15))
+			);
+		$update->executeStatement();
+	}
 }
diff --git a/lib/private/Authentication/Token/PublicKeyTokenProvider.php b/lib/private/Authentication/Token/PublicKeyTokenProvider.php
index 22c62d82fac05..b9cfce6c86930 100644
--- a/lib/private/Authentication/Token/PublicKeyTokenProvider.php
+++ b/lib/private/Authentication/Token/PublicKeyTokenProvider.php
@@ -221,9 +221,8 @@ public function updateTokenActivity(IToken $token) {
 		/** @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);
 		}
 	}
 
diff --git a/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php b/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php
index f27100b5d781c..fb366cb9975bd 100644
--- a/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php
+++ b/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php
@@ -100,10 +100,9 @@ public function testGenerateToken() {
 
 	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);
 
 		$this->tokenProvider->updateTokenActivity($tk);
 
@@ -118,10 +117,9 @@ public function testUpdateTokenDebounce() {
 				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);
 	}