From 4e33af65fd0609808e64115a686c6cc3138d0575 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Thu, 21 Oct 2021 14:12:36 +0200 Subject: [PATCH 1/4] 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 --- .../Token/PublicKeyTokenMapper.php | 39 +++++++++++++++++++ .../Token/PublicKeyTokenProvider.php | 3 +- .../Token/PublicKeyTokenProviderTest.php | 13 +++---- 3 files changed, 46 insertions(+), 9 deletions(-) diff --git a/lib/private/Authentication/Token/PublicKeyTokenMapper.php b/lib/private/Authentication/Token/PublicKeyTokenMapper.php index e05325fec30aa..5c7d9114318d4 100644 --- a/lib/private/Authentication/Token/PublicKeyTokenMapper.php +++ b/lib/private/Authentication/Token/PublicKeyTokenMapper.php @@ -187,4 +187,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(), IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT), + $qb->expr()->lt('last_activity', $qb->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 index 81dc3164ec141..d9936f29447b5 100644 --- a/lib/private/Authentication/Token/PublicKeyTokenProvider.php +++ b/lib/private/Authentication/Token/PublicKeyTokenProvider.php @@ -222,9 +222,8 @@ public function updateTokenActivity(IToken $token) { /** @var DefaultToken $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 a815025a50929..c4d9b0667c2b7 100644 --- a/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php +++ b/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php @@ -100,10 +100,10 @@ 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); @@ -112,16 +112,15 @@ public function testUpdateToken() { 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); } From 375010f3c0966b41740c7fcb935704eb03c604d9 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 12 Nov 2021 14:43:23 +0100 Subject: [PATCH 2/4] Fix missing token update Signed-off-by: Joas Schilling --- lib/private/User/Session.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index e553cdb448bb9..25118f5e64e81 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -749,6 +749,7 @@ private function checkTokenCredentials(IToken $dbToken, $token) { } $dbToken->setLastCheck($now); + $this->tokenProvider->updateToken($dbToken); return true; } @@ -766,6 +767,7 @@ private function checkTokenCredentials(IToken $dbToken, $token) { } $dbToken->setLastCheck($now); + $this->tokenProvider->updateToken($dbToken); return true; } From 3ebb319898eebdd47f4f120171c0065256b1a155 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 12 Nov 2021 15:01:56 +0100 Subject: [PATCH 3/4] Fix unit tests Signed-off-by: Joas Schilling --- tests/lib/User/SessionTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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()) From d5f14e995a2c2890a942676c17b2e6a7cc578969 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 13 Sep 2022 16:52:44 +0200 Subject: [PATCH 4/4] 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 --- .../Token/PublicKeyTokenMapper.php | 28 ++++++----- .../Token/PublicKeyTokenProvider.php | 8 +-- lib/private/User/Session.php | 2 - lib/public/AppFramework/Db/QBMapper.php | 42 +++++++++++----- .../Token/PublicKeyTokenMapperTest.php | 49 +++++++++++++++++++ .../Token/PublicKeyTokenProviderTest.php | 18 ++++++- 6 files changed, 116 insertions(+), 31 deletions(-) diff --git a/lib/private/Authentication/Token/PublicKeyTokenMapper.php b/lib/private/Authentication/Token/PublicKeyTokenMapper.php index 5c7d9114318d4..def21241bc9d7 100644 --- a/lib/private/Authentication/Token/PublicKeyTokenMapper.php +++ b/lib/private/Authentication/Token/PublicKeyTokenMapper.php @@ -189,7 +189,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 @@ -199,7 +199,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 @@ -213,17 +213,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) - ); - $update->executeStatement(); + 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 d9936f29447b5..a33d7eecc4bfc 100644 --- a/lib/private/Authentication/Token/PublicKeyTokenProvider.php +++ b/lib/private/Authentication/Token/PublicKeyTokenProvider.php @@ -219,10 +219,12 @@ 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)) { - $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 index 25118f5e64e81..e553cdb448bb9 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -749,7 +749,6 @@ private function checkTokenCredentials(IToken $dbToken, $token) { } $dbToken->setLastCheck($now); - $this->tokenProvider->updateToken($dbToken); return true; } @@ -767,7 +766,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 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 c4d9b0667c2b7..99129687b8c17 100644 --- a/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php +++ b/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php @@ -106,8 +106,6 @@ public function testUpdateToken() { $tk->setLastActivity($this->time - 200); $this->tokenProvider->updateTokenActivity($tk); - - $this->assertEquals($this->time, $tk->getLastActivity()); } public function testUpdateTokenDebounce() { @@ -125,6 +123,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')