From 9fdd48dd2d784f22675021445f253defc059c075 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Sun, 23 Feb 2025 19:08:10 +0100 Subject: [PATCH] fix: validate account properties as a repair step Replace `ValidatePhoneNumber` from Nextcloud 21 with a new repair step, `ValidateAccountProperties` which validates and sanitizes all account properties. Signed-off-by: Ferdinand Thiessen --- lib/composer/composer/autoload_classmap.php | 2 +- lib/composer/composer/autoload_static.php | 2 +- lib/private/Repair.php | 4 +- .../Repair/NC21/ValidatePhoneNumber.php | 70 --------------- .../Repair/NC29/ValidateAccountProperties.php | 58 ++++++++++++ .../NC29/ValidateAccountPropertiesTest.php | 89 +++++++++++++++++++ 6 files changed, 151 insertions(+), 74 deletions(-) delete mode 100644 lib/private/Repair/NC21/ValidatePhoneNumber.php create mode 100644 lib/private/Repair/NC29/ValidateAccountProperties.php create 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 b6b479028d6..913962d88c0 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1815,10 +1815,10 @@ return array( 'OC\\Repair\\NC20\\EncryptionMigration' => $baseDir . '/lib/private/Repair/NC20/EncryptionMigration.php', 'OC\\Repair\\NC20\\ShippedDashboardEnable' => $baseDir . '/lib/private/Repair/NC20/ShippedDashboardEnable.php', 'OC\\Repair\\NC21\\AddCheckForUserCertificatesJob' => $baseDir . '/lib/private/Repair/NC21/AddCheckForUserCertificatesJob.php', - 'OC\\Repair\\NC21\\ValidatePhoneNumber' => $baseDir . '/lib/private/Repair/NC21/ValidatePhoneNumber.php', '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\\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 b8ca8eeaa15..f5846d8e91e 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1848,10 +1848,10 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Repair\\NC20\\EncryptionMigration' => __DIR__ . '/../../..' . '/lib/private/Repair/NC20/EncryptionMigration.php', 'OC\\Repair\\NC20\\ShippedDashboardEnable' => __DIR__ . '/../../..' . '/lib/private/Repair/NC20/ShippedDashboardEnable.php', 'OC\\Repair\\NC21\\AddCheckForUserCertificatesJob' => __DIR__ . '/../../..' . '/lib/private/Repair/NC21/AddCheckForUserCertificatesJob.php', - 'OC\\Repair\\NC21\\ValidatePhoneNumber' => __DIR__ . '/../../..' . '/lib/private/Repair/NC21/ValidatePhoneNumber.php', '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\\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 63a55c81cdb..0a67dd91fc2 100644 --- a/lib/private/Repair.php +++ b/lib/private/Repair.php @@ -37,10 +37,10 @@ use OC\Repair\NC20\EncryptionLegacyCipher; use OC\Repair\NC20\EncryptionMigration; use OC\Repair\NC20\ShippedDashboardEnable; use OC\Repair\NC21\AddCheckForUserCertificatesJob; -use OC\Repair\NC21\ValidatePhoneNumber; use OC\Repair\NC22\LookupServerSendCheck; use OC\Repair\NC24\AddTokenCleanupJob; use OC\Repair\NC25\AddMissingSecretJob; +use OC\Repair\NC29\ValidateAccountProperties; use OC\Repair\NC30\RemoveLegacyDatadirFile; use OC\Repair\OldGroupMembershipShares; use OC\Repair\Owncloud\CleanPreviews; @@ -205,7 +205,7 @@ 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(ValidatePhoneNumber::class), + \OC::$server->get(ValidateAccountProperties::class), \OC::$server->get(DeleteSchedulingObjects::class), ]; } diff --git a/lib/private/Repair/NC21/ValidatePhoneNumber.php b/lib/private/Repair/NC21/ValidatePhoneNumber.php deleted file mode 100644 index 3a6ace37bd2..00000000000 --- a/lib/private/Repair/NC21/ValidatePhoneNumber.php +++ /dev/null @@ -1,70 +0,0 @@ -config = $config; - $this->userManager = $userManager; - $this->accountManager = $accountManager; - } - - public function getName(): string { - return 'Validate the phone number and store it in a known format for search'; - } - - public function run(IOutput $output): void { - if ($this->config->getSystemValueString('default_phone_region', '') === '') { - $output->warning('Can not validate phone numbers without `default_phone_region` being set in the config file'); - return; - } - - $numUpdated = 0; - $numRemoved = 0; - - $this->userManager->callForSeenUsers(function (IUser $user) use (&$numUpdated, &$numRemoved) { - $account = $this->accountManager->getAccount($user); - $property = $account->getProperty(IAccountManager::PROPERTY_PHONE); - - if ($property->getValue() !== '') { - $this->accountManager->updateAccount($account); - $updatedAccount = $this->accountManager->getAccount($user); - $updatedProperty = $updatedAccount->getProperty(IAccountManager::PROPERTY_PHONE); - - if ($property->getValue() !== $updatedProperty->getValue()) { - if ($updatedProperty->getValue() === '') { - $numRemoved++; - } else { - $numUpdated++; - } - } - } - }); - - if ($numRemoved > 0 || $numUpdated > 0) { - $output->info('Updated ' . $numUpdated . ' entries and cleaned ' . $numRemoved . ' invalid phone numbers'); - } - } -} diff --git a/lib/private/Repair/NC29/ValidateAccountProperties.php b/lib/private/Repair/NC29/ValidateAccountProperties.php new file mode 100644 index 00000000000..266266c8a1c --- /dev/null +++ b/lib/private/Repair/NC29/ValidateAccountProperties.php @@ -0,0 +1,58 @@ +userManager->callForSeenUsers(function (IUser $user) use (&$numRemoved) { + $account = $this->accountManager->getAccount($user); + while (true) { + try { + $this->accountManager->updateAccount($account); + break; + } 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()]); + break; + } + } + } + }); + + if ($numRemoved > 0) { + $output->info('Cleaned ' . $numRemoved . ' invalid account property entries'); + } + } +} diff --git a/tests/lib/Repair/NC29/ValidateAccountPropertiesTest.php b/tests/lib/Repair/NC29/ValidateAccountPropertiesTest.php new file mode 100644 index 00000000000..cc43a63ca19 --- /dev/null +++ b/tests/lib/Repair/NC29/ValidateAccountPropertiesTest.php @@ -0,0 +1,89 @@ +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->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); + $account2 = $this->createMock(IAccount::class); + $account2->expects(self::never()) + ->method('getProperty'); + $this->accountManager + ->expects(self::exactly(2)) + ->method('getAccount') + ->willReturnMap([ + [$users[0], $account1], + [$users[1], $account2], + ]); + $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); + } +}