diff --git a/lib/private/Authentication/Token/PublicKeyTokenMapper.php b/lib/private/Authentication/Token/PublicKeyTokenMapper.php index e05325fec30aa..def21241bc9d7 100644 --- a/lib/private/Authentication/Token/PublicKeyTokenMapper.php +++ b/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(); + } } diff --git a/lib/private/Authentication/Token/PublicKeyTokenProvider.php b/lib/private/Authentication/Token/PublicKeyTokenProvider.php index 81dc3164ec141..a33d7eecc4bfc 100644 --- a/lib/private/Authentication/Token/PublicKeyTokenProvider.php +++ b/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); } } diff --git a/lib/public/AppFramework/Db/QBMapper.php b/lib/public/AppFramework/Db/QBMapper.php index 4d49ab9a442c7..afabc9ab387df 100644 --- a/lib/public/AppFramework/Db/QBMapper.php +++ b/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; diff --git a/tests/lib/Authentication/Token/PublicKeyTokenMapperTest.php b/tests/lib/Authentication/Token/PublicKeyTokenMapperTest.php index 2f2af38851dbe..12b3c587351fb 100644 --- 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 index a815025a50929..99129687b8c17 100644 --- a/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php +++ b/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); } diff --git a/tests/lib/User/SessionTest.php b/tests/lib/User/SessionTest.php index 8fd94ffc00495..4dc096477d66f 100644 --- a/tests/lib/User/SessionTest.php +++ b/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())