From 568c4b49650794bb85409470688e373b2990d10a 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 | 1 - lib/public/AppFramework/Db/QBMapper.php | 42 +++++++++++----- .../Token/PublicKeyTokenMapperTest.php | 49 +++++++++++++++++++ .../Token/PublicKeyTokenProviderTest.php | 18 ++++++- 6 files changed, 113 insertions(+), 29 deletions(-) diff --git a/lib/private/Authentication/Token/PublicKeyTokenMapper.php b/lib/private/Authentication/Token/PublicKeyTokenMapper.php index 0db5c4f53e7ea..5b97e557219b3 100644 --- a/lib/private/Authentication/Token/PublicKeyTokenMapper.php +++ b/lib/private/Authentication/Token/PublicKeyTokenMapper.php @@ -182,7 +182,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 @@ -192,7 +192,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 @@ -206,17 +206,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 767ece1e55121..809c39474e2ab 100644 --- a/lib/private/Authentication/Token/PublicKeyTokenProvider.php +++ b/lib/private/Authentication/Token/PublicKeyTokenProvider.php @@ -299,10 +299,12 @@ public function updateTokenActivity(OCPIToken $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); $this->cacheToken($token); } diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index e5d2717251970..8226e3c95f59f 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -757,7 +757,6 @@ private function checkTokenCredentials(IToken $dbToken, $token) { if ($dbToken instanceof PublicKeyToken) { $dbToken->setLastActivity($now); } - $this->tokenProvider->updateToken($dbToken); return true; } diff --git a/lib/public/AppFramework/Db/QBMapper.php b/lib/public/AppFramework/Db/QBMapper.php index badd2483b58e9..ad9f772095a8c 100644 --- a/lib/public/AppFramework/Db/QBMapper.php +++ b/lib/public/AppFramework/Db/QBMapper.php @@ -146,22 +146,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(); @@ -193,6 +186,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 4b87f7101b5b5..8ff7ec70bf743 100644 --- a/tests/lib/Authentication/Token/PublicKeyTokenMapperTest.php +++ b/tests/lib/Authentication/Token/PublicKeyTokenMapperTest.php @@ -261,4 +261,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 3c81eade7007a..5f0f65e2cc948 100644 --- a/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php +++ b/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php @@ -191,8 +191,6 @@ public function testUpdateToken() { ]); $this->tokenProvider->updateTokenActivity($tk); - - $this->assertEquals($this->time, $tk->getLastActivity()); } public function testUpdateTokenDebounce() { @@ -210,6 +208,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')