[20] Auth token update improvements

lib/private/Authentication/Token/PublicKeyTokenMapper.php

@@ -187,4 +187,47 @@ public function hasExpiredTokens(string $uid): bool {
 
 		return count($data) === 1;
 	}
+
+	/**
+	 * Update the last activity timestamp and save all saved fields
+	 *
+	 * 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 2 (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 PublicKeyToken $token
+	 * @param int $now
+	 */
+	public function updateActivity(PublicKeyToken $token, int $now): void {
+		$token->setLastActivity($now);
+		$update = $this->createUpdateQuery($token);
+
+		$updatedFields = $token->getUpdatedFields();
+		unset($updatedFields['lastActivity']);
+
+		// if no other fields are updated, we add the extra filter to prevent duplicate updates
+		if (count($updatedFields) === 0) {
+			$update->andWhere($update->expr()->lt('last_activity', $update->createNamedParameter($now - 15, IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT));
+		}
+
+		$update->execute();
+	}
 }

lib/private/Authentication/Token/PublicKeyTokenProvider.php

@@ -219,12 +219,13 @@ public function updateTokenActivity(IToken $token) {
 		$activityInterval = $this->config->getSystemValueInt('token_auth_activity_update', 60);
 		$activityInterval = min(max($activityInterval, 0), 300);
 
-		/** @var DefaultToken $token */
+		$updatedFields = $token->getUpdatedFields();
+		unset($updatedFields['lastActivity']);
+
+		/** @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);
+		if ($token->getLastActivity() < ($now - $activityInterval) || count($updatedFields)) {
+			$this->mapper->updateActivity($token, $now);
 		}
 	}
 

lib/public/AppFramework/Db/QBMapper.php

@@ -163,20 +163,15 @@ public function insertOrUpdate(Entity $entity): Entity {
 	}
 
 	/**
-	 * Updates an entry in the db from an entity
-	 * @throws \InvalidArgumentException if entity has no id
-	 * @param Entity $entity the entity that should be created
-	 * @psalm-param T $entity the entity that should be created
-	 * @return Entity the saved entity with the set id
-	 * @psalm-return T the saved entity with the set id
-	 * @since 14.0.0
+	 * Create an update query that saves all updated fields
+	 *
+	 * @param Entity $entity the entity that should be updated
+	 * @psalm-param T $entity the entity that should be updated
+	 * @return IQueryBuilder
+	 * @since 25.0.0
 	 */
-	public function update(Entity $entity): Entity {
-		// if entity wasn't changed it makes no sense to run a db query
+	protected function createUpdateQuery(Entity $entity): IQueryBuilder {
 		$properties = $entity->getUpdatedFields();
-		if (\count($properties) === 0) {
-			return $entity;
-		}
 
 		// entity needs an id
 		$id = $entity->getId();
@@ -208,6 +203,29 @@ public function update(Entity $entity): Entity {
 		$qb->where(
 			$qb->expr()->eq('id', $qb->createNamedParameter($id, $idType))
 		);
+
+		return $qb;
+	}
+
+	/**
+	 * Updates an entry in the db from an entity
+	 *
+	 * @param Entity $entity the entity that should be created
+	 * @psalm-param T $entity the entity that should be created
+	 * @return Entity the saved entity with the set id
+	 * @psalm-return T the saved entity with the set id
+	 * @throws \Exception
+	 * @throws \InvalidArgumentException if entity has no id
+	 * @since 14.0.0
+	 */
+	public function update(Entity $entity): Entity {
+		// if entity wasn't changed it makes no sense to run a db query
+		$properties = $entity->getUpdatedFields();
+		if (\count($properties) === 0) {
+			return $entity;
+		}
+
+		$qb = $this->createUpdateQuery($entity);
 		$qb->execute();
 
 		return $entity;

tests/lib/Authentication/Token/PublicKeyTokenMapperTest.php

@@ -266,4 +266,53 @@ public function testHasExpiredTokens() {
 		$this->assertFalse($this->mapper->hasExpiredTokens('user1'));
 		$this->assertTrue($this->mapper->hasExpiredTokens('user3'));
 	}
+
+	public function testUpdateTokenActivity() {
+		$token = '6d9a290d239d09f2cc33a03cc54cccd46f7dc71630dcc27d39214824bd3e093f1feb4e2b55eb159d204caa15dee9556c202a5aa0b9d67806c3f4ec2cde11af67';
+		$dbToken = $this->mapper->getToken($token);
+
+		$this->assertEquals($dbToken->getLastActivity(), $this->time - 120);
+		$this->assertEquals($this->time - 60 * 10, $dbToken->getLastCheck());
+
+		$this->mapper->updateActivity($dbToken, $this->time);
+
+		$updatedDbToken = $this->mapper->getToken($token);
+
+		$this->assertEquals($this->time, $updatedDbToken->getLastActivity());
+		$this->assertEquals($this->time - 60 * 10, $dbToken->getLastCheck());
+		$this->assertEquals($this->time, $dbToken->getLastActivity());
+	}
+
+	public function testUpdateTokenActivityDebounce() {
+		$token = '6d9a290d239d09f2cc33a03cc54cccd46f7dc71630dcc27d39214824bd3e093f1feb4e2b55eb159d204caa15dee9556c202a5aa0b9d67806c3f4ec2cde11af67';
+		$dbToken = $this->mapper->getToken($token);
+
+		$this->assertEquals($dbToken->getLastActivity(), $this->time - 120);
+		$this->assertEquals($this->time - 60 * 10, $dbToken->getLastCheck());
+
+		$this->mapper->updateActivity($dbToken, $this->time - 110);
+
+		$updatedDbToken = $this->mapper->getToken($token);
+
+		$this->assertEquals($this->time - 120, $updatedDbToken->getLastActivity());
+		$this->assertEquals($this->time - 60 * 10, $dbToken->getLastCheck());
+		$this->assertEquals($this->time - 110, $dbToken->getLastActivity());
+	}
+
+	public function testUpdateTokenActivityDebounceUpdate() {
+		$token = '6d9a290d239d09f2cc33a03cc54cccd46f7dc71630dcc27d39214824bd3e093f1feb4e2b55eb159d204caa15dee9556c202a5aa0b9d67806c3f4ec2cde11af67';
+		$dbToken = $this->mapper->getToken($token);
+
+		$this->assertEquals($this->time - 120, $dbToken->getLastActivity());
+		$this->assertEquals($this->time - 60 * 10, $dbToken->getLastCheck());
+
+		$dbToken->setLastCheck($this->time - 100);
+		$this->mapper->updateActivity($dbToken, $this->time - 110);
+
+		$updatedDbToken = $this->mapper->getToken($token);
+
+		$this->assertEquals($this->time - 110, $updatedDbToken->getLastActivity());
+		$this->assertEquals($this->time - 100, $dbToken->getLastCheck());
+		$this->assertEquals($this->time - 110, $dbToken->getLastActivity());
+	}
 }

tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php

@@ -100,28 +100,41 @@ 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);
+		$tk->setLastActivity($this->time - 200);
 
 		$this->tokenProvider->updateTokenActivity($tk);
-
-		$this->assertEquals($this->time, $tk->getLastActivity());
 	}
 
 	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);
+	}
+
+	public function testUpdateTokenDebounceWithUpdatedFields() {
+		$tk = new PublicKeyToken();
+		$this->config->method('getSystemValueInt')
+			->willReturnCallback(function ($value, $default) {
+				return $default;
+			});
+		$tk->setLastActivity($this->time - 30);
+		$tk->setLastCheck($this->time - 30);
+
+		$this->mapper->expects($this->once())
+			->method('updateActivity')
+			->with($tk, $this->time);
 
 		$this->tokenProvider->updateTokenActivity($tk);
 	}

tests/lib/User/SessionTest.php

@@ -1255,7 +1255,7 @@ public function testUpdateAuthTokenLastCheck() {
 		$mapper->expects($this->any())
 			->method('getToken')
 			->willReturn($token);
-		$mapper->expects($this->once())
+		$mapper->expects($this->exactly(2))
 			->method('update');
 		$request
 			->expects($this->any())
@@ -1305,7 +1305,7 @@ public function testNoUpdateAuthTokenLastCheckRecent() {
 		$mapper->expects($this->any())
 			->method('getToken')
 			->willReturn($token);
-		$mapper->expects($this->never())
+		$mapper->expects($this->once())
 			->method('update');
 		$request
 			->expects($this->any())