From f012969ad20eb8691b4bc9fe3bf4c39cf3c7094a Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 13 Sep 2022 16:52:44 +0200 Subject: [PATCH] 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 | 26 +++++----- .../Token/PublicKeyTokenProvider.php | 6 ++- lib/private/User/Session.php | 2 - lib/public/AppFramework/Db/QBMapper.php | 42 +++++++++++----- .../Token/PublicKeyTokenMapperTest.php | 49 +++++++++++++++++++ .../Token/PublicKeyTokenProviderTest.php | 18 ++++++- 6 files changed, 113 insertions(+), 30 deletions(-) diff --git a/lib/private/Authentication/Token/PublicKeyTokenMapper.php b/lib/private/Authentication/Token/PublicKeyTokenMapper.php index 7b11ef8adf3b3..ee96645e7df2a 100644 --- 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 index 0f1767e845b75..8e75205bd31a8 100644 --- 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 index 65a213d4bf808..56a027b04e3ff 100644 --- 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 index 2491fd83f4a64..434c2081310a8 100644 --- 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 index bfb92932e8119..5da0ed8329996 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 1ef0aa80817b3..70c8aa7431204 100644 --- 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')