From d3dbe3ab2c14760d4801c1167dc78d55943b9ebe Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Mon, 24 Feb 2025 14:52:24 +0100 Subject: refactor: convert sanitize account properties repair step to background job Signed-off-by: Ferdinand Thiessen --- lib/composer/composer/autoload_classmap.php | 3 +- lib/composer/composer/autoload_static.php | 3 +- lib/private/Repair.php | 4 +- .../Repair/NC29/SanitizeAccountProperties.php | 30 ++++++ .../Repair/NC29/SanitizeAccountPropertiesJob.php | 75 +++++++++++++ .../Repair/NC29/ValidateAccountProperties.php | 76 -------------- .../NC29/SanitizeAccountPropertiesJobTest.php | 116 +++++++++++++++++++++ .../Repair/NC29/SanitizeAccountPropertiesTest.php | 43 ++++++++ .../Repair/NC29/ValidateAccountPropertiesTest.php | 114 -------------------- 9 files changed, 270 insertions(+), 194 deletions(-) create mode 100644 lib/private/Repair/NC29/SanitizeAccountProperties.php create mode 100644 lib/private/Repair/NC29/SanitizeAccountPropertiesJob.php delete mode 100644 lib/private/Repair/NC29/ValidateAccountProperties.php create mode 100644 tests/lib/Repair/NC29/SanitizeAccountPropertiesJobTest.php create mode 100644 tests/lib/Repair/NC29/SanitizeAccountPropertiesTest.php delete mode 100644 tests/lib/Repair/NC29/ValidateAccountPropertiesTest.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 913962d88c0..4acb7bd7ce6 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1818,7 +1818,8 @@ return array( 'OC\\Repair\\NC22\\LookupServerSendCheck' => $baseDir . '/lib/private/Repair/NC22/LookupServerSendCheck.php', 'OC\\Repair\\NC24\\AddTokenCleanupJob' => $baseDir . '/lib/private/Repair/NC24/AddTokenCleanupJob.php', 'OC\\Repair\\NC25\\AddMissingSecretJob' => $baseDir . '/lib/private/Repair/NC25/AddMissingSecretJob.php', - 'OC\\Repair\\NC29\\ValidateAccountProperties' => $baseDir . '/lib/private/Repair/NC29/ValidateAccountProperties.php', + 'OC\\Repair\\NC29\\SanitizeAccountProperties' => $baseDir . '/lib/private/Repair/NC29/SanitizeAccountProperties.php', + 'OC\\Repair\\NC29\\SanitizeAccountPropertiesJob' => $baseDir . '/lib/private/Repair/NC29/SanitizeAccountPropertiesJob.php', 'OC\\Repair\\NC30\\RemoveLegacyDatadirFile' => $baseDir . '/lib/private/Repair/NC30/RemoveLegacyDatadirFile.php', 'OC\\Repair\\OldGroupMembershipShares' => $baseDir . '/lib/private/Repair/OldGroupMembershipShares.php', 'OC\\Repair\\Owncloud\\CleanPreviews' => $baseDir . '/lib/private/Repair/Owncloud/CleanPreviews.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index f5846d8e91e..c6f32ac6247 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1851,7 +1851,8 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Repair\\NC22\\LookupServerSendCheck' => __DIR__ . '/../../..' . '/lib/private/Repair/NC22/LookupServerSendCheck.php', 'OC\\Repair\\NC24\\AddTokenCleanupJob' => __DIR__ . '/../../..' . '/lib/private/Repair/NC24/AddTokenCleanupJob.php', 'OC\\Repair\\NC25\\AddMissingSecretJob' => __DIR__ . '/../../..' . '/lib/private/Repair/NC25/AddMissingSecretJob.php', - 'OC\\Repair\\NC29\\ValidateAccountProperties' => __DIR__ . '/../../..' . '/lib/private/Repair/NC29/ValidateAccountProperties.php', + 'OC\\Repair\\NC29\\SanitizeAccountProperties' => __DIR__ . '/../../..' . '/lib/private/Repair/NC29/SanitizeAccountProperties.php', + 'OC\\Repair\\NC29\\SanitizeAccountPropertiesJob' => __DIR__ . '/../../..' . '/lib/private/Repair/NC29/SanitizeAccountPropertiesJob.php', 'OC\\Repair\\NC30\\RemoveLegacyDatadirFile' => __DIR__ . '/../../..' . '/lib/private/Repair/NC30/RemoveLegacyDatadirFile.php', 'OC\\Repair\\OldGroupMembershipShares' => __DIR__ . '/../../..' . '/lib/private/Repair/OldGroupMembershipShares.php', 'OC\\Repair\\Owncloud\\CleanPreviews' => __DIR__ . '/../../..' . '/lib/private/Repair/Owncloud/CleanPreviews.php', diff --git a/lib/private/Repair.php b/lib/private/Repair.php index 0a67dd91fc2..cfcdfc22d0d 100644 --- a/lib/private/Repair.php +++ b/lib/private/Repair.php @@ -40,7 +40,7 @@ use OC\Repair\NC21\AddCheckForUserCertificatesJob; use OC\Repair\NC22\LookupServerSendCheck; use OC\Repair\NC24\AddTokenCleanupJob; use OC\Repair\NC25\AddMissingSecretJob; -use OC\Repair\NC29\ValidateAccountProperties; +use OC\Repair\NC29\SanitizeAccountProperties; use OC\Repair\NC30\RemoveLegacyDatadirFile; use OC\Repair\OldGroupMembershipShares; use OC\Repair\Owncloud\CleanPreviews; @@ -191,6 +191,7 @@ class Repair implements IOutput { \OCP\Server::get(RepairLogoDimension::class), \OCP\Server::get(RemoveLegacyDatadirFile::class), \OCP\Server::get(AddCleanupDeletedUsersBackgroundJob::class), + \OCP\Server::get(SanitizeAccountProperties::class), ]; } @@ -205,7 +206,6 @@ class Repair implements IOutput { new OldGroupMembershipShares(\OC::$server->getDatabaseConnection(), \OC::$server->getGroupManager()), new RemoveBrokenProperties(\OC::$server->getDatabaseConnection()), new RepairMimeTypes(\OC::$server->getConfig(), \OC::$server->getDatabaseConnection()), - \OC::$server->get(ValidateAccountProperties::class), \OC::$server->get(DeleteSchedulingObjects::class), ]; } diff --git a/lib/private/Repair/NC29/SanitizeAccountProperties.php b/lib/private/Repair/NC29/SanitizeAccountProperties.php new file mode 100644 index 00000000000..412570ba71d --- /dev/null +++ b/lib/private/Repair/NC29/SanitizeAccountProperties.php @@ -0,0 +1,30 @@ +jobList->add(SanitizeAccountPropertiesJob::class, null); + $output->info('Queued background to validate account properties.'); + } +} diff --git a/lib/private/Repair/NC29/SanitizeAccountPropertiesJob.php b/lib/private/Repair/NC29/SanitizeAccountPropertiesJob.php new file mode 100644 index 00000000000..55ec445e9da --- /dev/null +++ b/lib/private/Repair/NC29/SanitizeAccountPropertiesJob.php @@ -0,0 +1,75 @@ +setAllowParallelRuns(false); + } + + protected function run(mixed $argument): void { + $numRemoved = 0; + + $this->userManager->callForSeenUsers(function (IUser $user) use (&$numRemoved) { + $account = $this->accountManager->getAccount($user); + $properties = array_keys($account->jsonSerialize()); + + // Check if there are some properties we can sanitize - reduces number of db queries + if (empty(array_intersect($properties, self::PROPERTIES_TO_CHECK))) { + return; + } + + // Limit the loop to the properties we check to ensure there are no infinite loops + // we add one additional loop (+ 1) as we need 1 loop for checking + 1 for update. + $iteration = count(self::PROPERTIES_TO_CHECK) + 1; + while ($iteration-- > 0) { + try { + $this->accountManager->updateAccount($account); + return; + } catch (InvalidArgumentException $e) { + if (in_array($e->getMessage(), IAccountManager::ALLOWED_PROPERTIES)) { + $numRemoved++; + $property = $account->getProperty($e->getMessage()); + $account->setProperty($property->getName(), '', $property->getScope(), IAccountManager::NOT_VERIFIED); + } else { + $this->logger->error('Error while sanitizing account property', ['exception' => $e, 'user' => $user->getUID()]); + return; + } + } + } + $this->logger->error('Iteration limit exceeded while cleaning account properties', ['user' => $user->getUID()]); + }); + + if ($numRemoved > 0) { + $this->logger->info('Cleaned ' . $numRemoved . ' invalid account property entries'); + } + } +} diff --git a/lib/private/Repair/NC29/ValidateAccountProperties.php b/lib/private/Repair/NC29/ValidateAccountProperties.php deleted file mode 100644 index 640cd678ba0..00000000000 --- a/lib/private/Repair/NC29/ValidateAccountProperties.php +++ /dev/null @@ -1,76 +0,0 @@ -userManager->callForSeenUsers(function (IUser $user) use (&$numRemoved) { - $account = $this->accountManager->getAccount($user); - $properties = array_keys($account->jsonSerialize()); - - // Check if there are some properties we can sanitize - reduces number of db queries - if (empty(array_intersect($properties, self::PROPERTIES_TO_CHECK))) { - return; - } - - // Limit the loop to the properties we check to ensure there are no infinite loops - // we add one additional loop (+ 1) as we need 1 loop for checking + 1 for update. - $iteration = count(self::PROPERTIES_TO_CHECK) + 1; - while ($iteration-- > 0) { - try { - $this->accountManager->updateAccount($account); - return; - } catch (InvalidArgumentException $e) { - if (in_array($e->getMessage(), IAccountManager::ALLOWED_PROPERTIES)) { - $numRemoved++; - $property = $account->getProperty($e->getMessage()); - $account->setProperty($property->getName(), '', $property->getScope(), IAccountManager::NOT_VERIFIED); - } else { - $this->logger->error('Error while sanitizing account property', ['exception' => $e, 'user' => $user->getUID()]); - return; - } - } - } - $this->logger->error('Iteration limit exceeded while cleaning account properties', ['user' => $user->getUID()]); - }); - - if ($numRemoved > 0) { - $output->info('Cleaned ' . $numRemoved . ' invalid account property entries'); - } - } -} diff --git a/tests/lib/Repair/NC29/SanitizeAccountPropertiesJobTest.php b/tests/lib/Repair/NC29/SanitizeAccountPropertiesJobTest.php new file mode 100644 index 00000000000..e0f4eb3cbc1 --- /dev/null +++ b/tests/lib/Repair/NC29/SanitizeAccountPropertiesJobTest.php @@ -0,0 +1,116 @@ +userManager = $this->createMock(IUserManager::class); + $this->accountManager = $this->createMock(IAccountManager::class); + $this->logger = $this->createMock(LoggerInterface::class); + + $this->job = new SanitizeAccountPropertiesJob( + $this->createMock(ITimeFactory::class), + $this->userManager, + $this->accountManager, + $this->logger, + ); + } + + public function testParallel() { + self::assertFalse($this->job->getAllowParallelRuns()); + } + + public function testRun(): void { + $users = [ + $this->createMock(IUser::class), + $this->createMock(IUser::class), + $this->createMock(IUser::class), + ]; + $this->userManager + ->expects(self::once()) + ->method('callForSeenUsers') + ->willReturnCallback(fn ($fn) => array_map($fn, $users)); + + $property = $this->createMock(IAccountProperty::class); + $property->expects(self::once())->method('getName')->willReturn(IAccountManager::PROPERTY_PHONE); + $property->expects(self::once())->method('getScope')->willReturn(IAccountManager::SCOPE_LOCAL); + + $account1 = $this->createMock(IAccount::class); + $account1->expects(self::once()) + ->method('getProperty') + ->with(IAccountManager::PROPERTY_PHONE) + ->willReturn($property); + $account1->expects(self::once()) + ->method('setProperty') + ->with(IAccountManager::PROPERTY_PHONE, '', IAccountManager::SCOPE_LOCAL, IAccountManager::NOT_VERIFIED); + $account1->expects(self::once()) + ->method('jsonSerialize') + ->willReturn([ + IAccountManager::PROPERTY_DISPLAYNAME => [], + IAccountManager::PROPERTY_PHONE => [], + ]); + + $account2 = $this->createMock(IAccount::class); + $account2->expects(self::never()) + ->method('getProperty'); + $account2->expects(self::once()) + ->method('jsonSerialize') + ->willReturn([ + IAccountManager::PROPERTY_DISPLAYNAME => [], + IAccountManager::PROPERTY_PHONE => [], + ]); + + $account3 = $this->createMock(IAccount::class); + $account3->expects(self::never()) + ->method('getProperty'); + $account3->expects(self::once()) + ->method('jsonSerialize') + ->willReturn([ + IAccountManager::PROPERTY_DISPLAYNAME => [], + ]); + + $this->accountManager + ->expects(self::exactly(3)) + ->method('getAccount') + ->willReturnMap([ + [$users[0], $account1], + [$users[1], $account2], + [$users[2], $account3], + ]); + $valid = false; + $this->accountManager->expects(self::exactly(3)) + ->method('updateAccount') + ->willReturnCallback(function (IAccount $account) use (&$account1, &$valid) { + if (!$valid && $account === $account1) { + $valid = true; + throw new InvalidArgumentException(IAccountManager::PROPERTY_PHONE); + } + }); + + self::invokePrivate($this->job, 'run', [null]); + } +} diff --git a/tests/lib/Repair/NC29/SanitizeAccountPropertiesTest.php b/tests/lib/Repair/NC29/SanitizeAccountPropertiesTest.php new file mode 100644 index 00000000000..d0d33eb2817 --- /dev/null +++ b/tests/lib/Repair/NC29/SanitizeAccountPropertiesTest.php @@ -0,0 +1,43 @@ +jobList = $this->createMock(IJobList::class); + + $this->repairStep = new SanitizeAccountProperties($this->jobList); + } + + public function testGetName(): void { + self::assertStringContainsString('Validate account properties', $this->repairStep->getName()); + } + + public function testRun(): void { + $this->jobList->expects(self::once()) + ->method('add') + ->with(SanitizeAccountPropertiesJob::class, null); + + $output = $this->createMock(IOutput::class); + $output->expects(self::once()) + ->method('info') + ->with(self::matchesRegularExpression('/queued background/i')); + + $this->repairStep->run($output); + } +} diff --git a/tests/lib/Repair/NC29/ValidateAccountPropertiesTest.php b/tests/lib/Repair/NC29/ValidateAccountPropertiesTest.php deleted file mode 100644 index e113f180998..00000000000 --- a/tests/lib/Repair/NC29/ValidateAccountPropertiesTest.php +++ /dev/null @@ -1,114 +0,0 @@ -userManager = $this->createMock(IUserManager::class); - $this->accountManager = $this->createMock(IAccountManager::class); - $this->logger = $this->createMock(LoggerInterface::class); - - $this->repairStep = new ValidateAccountProperties($this->userManager, $this->accountManager, $this->logger); - } - - public function testGetName(): void { - self::assertStringContainsString('Validate account properties', $this->repairStep->getName()); - } - - public function testRun(): void { - $users = [ - $this->createMock(IUser::class), - $this->createMock(IUser::class), - $this->createMock(IUser::class), - ]; - $this->userManager - ->expects(self::once()) - ->method('callForSeenUsers') - ->willReturnCallback(fn ($fn) => array_map($fn, $users)); - - $property = $this->createMock(IAccountProperty::class); - $property->expects(self::once())->method('getName')->willReturn(IAccountManager::PROPERTY_PHONE); - $property->expects(self::once())->method('getScope')->willReturn(IAccountManager::SCOPE_LOCAL); - - $account1 = $this->createMock(IAccount::class); - $account1->expects(self::once()) - ->method('getProperty') - ->with(IAccountManager::PROPERTY_PHONE) - ->willReturn($property); - $account1->expects(self::once()) - ->method('setProperty') - ->with(IAccountManager::PROPERTY_PHONE, '', IAccountManager::SCOPE_LOCAL, IAccountManager::NOT_VERIFIED); - $account1->expects(self::once()) - ->method('jsonSerialize') - ->willReturn([ - IAccountManager::PROPERTY_DISPLAYNAME => [], - IAccountManager::PROPERTY_PHONE => [], - ]); - - $account2 = $this->createMock(IAccount::class); - $account2->expects(self::never()) - ->method('getProperty'); - $account2->expects(self::once()) - ->method('jsonSerialize') - ->willReturn([ - IAccountManager::PROPERTY_DISPLAYNAME => [], - IAccountManager::PROPERTY_PHONE => [], - ]); - - $account3 = $this->createMock(IAccount::class); - $account3->expects(self::never()) - ->method('getProperty'); - $account3->expects(self::once()) - ->method('jsonSerialize') - ->willReturn([ - IAccountManager::PROPERTY_DISPLAYNAME => [], - ]); - - $this->accountManager - ->expects(self::exactly(3)) - ->method('getAccount') - ->willReturnMap([ - [$users[0], $account1], - [$users[1], $account2], - [$users[2], $account3], - ]); - $valid = false; - $this->accountManager->expects(self::exactly(3)) - ->method('updateAccount') - ->willReturnCallback(function (IAccount $account) use (&$account1, &$valid) { - if (!$valid && $account === $account1) { - $valid = true; - throw new InvalidArgumentException(IAccountManager::PROPERTY_PHONE); - } - }); - - $output = $this->createMock(IOutput::class); - $output->expects(self::once())->method('info')->with('Cleaned 1 invalid account property entries'); - - $this->repairStep->run($output); - } -} -- cgit v1.2.3