merge last_activity and last_check updates

the debounce for updating last_activity is changed so it always updates if another field of the token has been updated, this ensures that last_check if updated even if last_activity is still in the debounce period. Signed-off-by: Robin Appelman <[email protected]>
nextcloud · Sep 14, 2022 · f012969 · f012969
1 parent 7ea015f
commit f012969
Show file tree

Hide file tree

Showing 6 changed files with 113 additions and 30 deletions.
diff --git a/lib/private/Authentication/Token/PublicKeyTokenMapper.php b/lib/private/Authentication/Token/PublicKeyTokenMapper.php
@@ -192,7 +192,7 @@ public function hasExpiredTokens(string $uid): bool {
 	}
 
 	/**
-	 * Update the last activity timestamp
+	 * 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
@@ -202,7 +202,7 @@ public function hasExpiredTokens(string $uid): bool {
 	 *
 	 * Example:
 	 * - process 1 (P1) reads the token at timestamp 1500
-	 * - process 1 (P2) reads the token at timestamp 1501
+	 * - process 2 (P2) reads the token at timestamp 1501
 	 * - activity update interval is 100
 	 *
 	 * This means
@@ -216,17 +216,21 @@ public function hasExpiredTokens(string $uid): bool {
 	 *   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 PublicKeyToken $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)
-			);
+	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->executeStatement();
 	}
 }
diff --git a/lib/private/Authentication/Token/PublicKeyTokenProvider.php b/lib/private/Authentication/Token/PublicKeyTokenProvider.php
@@ -227,10 +227,12 @@ public function updateTokenActivity(IToken $token) {
 		$activityInterval = $this->config->getSystemValueInt('token_auth_activity_update', 60);
 		$activityInterval = min(max($activityInterval, 0), 300);
 
+		$updatedFields = $token->getUpdatedFields();
+		unset($updatedFields['lastActivity']);
+
 		/** @var PublicKeyToken $token */
 		$now = $this->time->getTime();
-		if ($token->getLastActivity() < ($now - $activityInterval)) {
-			$token->setLastActivity($now);
+		if ($token->getLastActivity() < ($now - $activityInterval) || count($updatedFields)) {
 			$this->mapper->updateActivity($token, $now);
 		}
 	}

diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php
@@ -738,7 +738,6 @@ private function checkTokenCredentials(IToken $dbToken, $token) {
 			}
 
 			$dbToken->setLastCheck($now);
-			$this->tokenProvider->updateToken($dbToken);
 			return true;
 		}
 
@@ -756,7 +755,6 @@ private function checkTokenCredentials(IToken $dbToken, $token) {
 		}
 
 		$dbToken->setLastCheck($now);
-		$this->tokenProvider->updateToken($dbToken);
 		return true;
 	}
 

diff --git a/lib/public/AppFramework/Db/QBMapper.php b/lib/public/AppFramework/Db/QBMapper.php
@@ -171,22 +171,15 @@ public function insertOrUpdate(Entity $entity): Entity {
 	}
 
 	/**
-	 * Updates an entry in the db from an entity
+	 * Create an update query that saves all updated fields
 	 *
-	 * @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
+	 * @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();
@@ -218,6 +211,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->executeStatement();
 
 		return $entity;

diff --git a/tests/lib/Authentication/Token/PublicKeyTokenMapperTest.php b/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());
+	}
 }
diff --git a/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php b/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php
@@ -177,8 +177,6 @@ public function testUpdateToken() {
 			]);
 
 		$this->tokenProvider->updateTokenActivity($tk);
-
-		$this->assertEquals($this->time, $tk->getLastActivity());
 	}
 
 	public function testUpdateTokenDebounce() {
@@ -196,6 +194,22 @@ public function testUpdateTokenDebounce() {
 		$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);
+	}
+
 	public function testGetTokenByUser() {
 		$this->mapper->expects($this->once())
 			->method('getTokenByUser')