diff --git a/apps/dav/composer/composer/autoload_static.php b/apps/dav/composer/composer/autoload_static.php index 6901889f7294f..4d59f1e26733e 100644 --- a/apps/dav/composer/composer/autoload_static.php +++ b/apps/dav/composer/composer/autoload_static.php @@ -153,6 +153,7 @@ class ComposerStaticInitDAV 'OCA\\DAV\\CardDAV\\MultiGetExportPlugin' => __DIR__ . '/..' . '/../lib/CardDAV/MultiGetExportPlugin.php', 'OCA\\DAV\\CardDAV\\PhotoCache' => __DIR__ . '/..' . '/../lib/CardDAV/PhotoCache.php', 'OCA\\DAV\\CardDAV\\Plugin' => __DIR__ . '/..' . '/../lib/CardDAV/Plugin.php', + 'OCA\\DAV\\CardDAV\\Security\\CardDavRateLimitingPlugin' => __DIR__ . '/..' . '/../lib/CardDAV/Security/CardDavRateLimitingPlugin.php', 'OCA\\DAV\\CardDAV\\Sharing\\Backend' => __DIR__ . '/..' . '/../lib/CardDAV/Sharing/Backend.php', 'OCA\\DAV\\CardDAV\\Sharing\\Service' => __DIR__ . '/..' . '/../lib/CardDAV/Sharing/Service.php', 'OCA\\DAV\\CardDAV\\SyncService' => __DIR__ . '/..' . '/../lib/CardDAV/SyncService.php', @@ -160,7 +161,6 @@ class ComposerStaticInitDAV 'OCA\\DAV\\CardDAV\\UserAddressBooks' => __DIR__ . '/..' . '/../lib/CardDAV/UserAddressBooks.php', 'OCA\\DAV\\CardDAV\\Validation\\CardDavValidatePlugin' => __DIR__ . '/..' . '/../lib/CardDAV/Validation/CardDavValidatePlugin.php', 'OCA\\DAV\\CardDAV\\Xml\\Groups' => __DIR__ . '/..' . '/../lib/CardDAV/Xml/Groups.php', - 'OCA\\DAV\\CardDAV\\Security\\CardDavRateLimitingPlugin' => __DIR__ . '/..' . '/../lib/CardDAV/Security/CardDavRateLimitingPlugin.php', 'OCA\\DAV\\Command\\CreateAddressBook' => __DIR__ . '/..' . '/../lib/Command/CreateAddressBook.php', 'OCA\\DAV\\Command\\CreateCalendar' => __DIR__ . '/..' . '/../lib/Command/CreateCalendar.php', 'OCA\\DAV\\Command\\DeleteCalendar' => __DIR__ . '/..' . '/../lib/Command/DeleteCalendar.php', diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index b8f27474abf91..2eb0f2b6028d9 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1702,10 +1702,11 @@ '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\\SanitizeAccountProperties' => $baseDir . '/lib/private/Repair/NC29/SanitizeAccountProperties.php', + 'OC\\Repair\\NC29\\SanitizeAccountPropertiesJob' => $baseDir . '/lib/private/Repair/NC29/SanitizeAccountPropertiesJob.php', 'OC\\Repair\\OldGroupMembershipShares' => $baseDir . '/lib/private/Repair/OldGroupMembershipShares.php', 'OC\\Repair\\Owncloud\\CleanPreviews' => $baseDir . '/lib/private/Repair/Owncloud/CleanPreviews.php', 'OC\\Repair\\Owncloud\\CleanPreviewsBackgroundJob' => $baseDir . '/lib/private/Repair/Owncloud/CleanPreviewsBackgroundJob.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index bb0d4516c58d2..99af25c80142f 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1735,10 +1735,11 @@ 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\\SanitizeAccountProperties' => __DIR__ . '/../../..' . '/lib/private/Repair/NC29/SanitizeAccountProperties.php', + 'OC\\Repair\\NC29\\SanitizeAccountPropertiesJob' => __DIR__ . '/../../..' . '/lib/private/Repair/NC29/SanitizeAccountPropertiesJob.php', 'OC\\Repair\\OldGroupMembershipShares' => __DIR__ . '/../../..' . '/lib/private/Repair/OldGroupMembershipShares.php', 'OC\\Repair\\Owncloud\\CleanPreviews' => __DIR__ . '/../../..' . '/lib/private/Repair/Owncloud/CleanPreviews.php', 'OC\\Repair\\Owncloud\\CleanPreviewsBackgroundJob' => __DIR__ . '/../../..' . '/lib/private/Repair/Owncloud/CleanPreviewsBackgroundJob.php', diff --git a/lib/private/Repair.php b/lib/private/Repair.php index e1ceceafed7b7..f453f3e50de6f 100644 --- a/lib/private/Repair.php +++ b/lib/private/Repair.php @@ -65,10 +65,10 @@ 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\SanitizeAccountProperties; use OC\Repair\OldGroupMembershipShares; use OC\Repair\Owncloud\CleanPreviews; use OC\Repair\Owncloud\DropAccountTermsTable; @@ -215,6 +215,7 @@ public static function getRepairSteps(): array { \OCP\Server::get(AddAppConfigLazyMigration::class), \OCP\Server::get(RepairLogoDimension::class), \OCP\Server::get(AddCleanupDeletedUsersBackgroundJob::class), + \OCP\Server::get(SanitizeAccountProperties::class), ]; } @@ -229,7 +230,6 @@ public static function getExpensiveRepairSteps() { 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(DeleteSchedulingObjects::class), ]; } diff --git a/lib/private/Repair/NC21/ValidatePhoneNumber.php b/lib/private/Repair/NC21/ValidatePhoneNumber.php deleted file mode 100644 index 51120c9d1399d..0000000000000 --- a/lib/private/Repair/NC21/ValidatePhoneNumber.php +++ /dev/null @@ -1,88 +0,0 @@ - - * - * @author Arthur Schiwon - * @author Joas Schilling - * - * @license GNU AGPL version 3 or any later version - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as - * published by the Free Software Foundation, either version 3 of the - * License, or (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . - * - */ -namespace OC\Repair\NC21; - -use OCP\Accounts\IAccountManager; -use OCP\IConfig; -use OCP\IUser; -use OCP\IUserManager; -use OCP\Migration\IOutput; -use OCP\Migration\IRepairStep; - -class ValidatePhoneNumber implements IRepairStep { - /** @var IConfig */ - protected $config; - /** @var IUserManager */ - protected $userManager; - /** @var IAccountManager */ - private $accountManager; - - public function __construct(IUserManager $userManager, - IAccountManager $accountManager, - IConfig $config) { - $this->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/SanitizeAccountProperties.php b/lib/private/Repair/NC29/SanitizeAccountProperties.php new file mode 100644 index 0000000000000..412570ba71dec --- /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 0000000000000..55ec445e9daec --- /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/tests/lib/Repair/NC29/SanitizeAccountPropertiesJobTest.php b/tests/lib/Repair/NC29/SanitizeAccountPropertiesJobTest.php new file mode 100644 index 0000000000000..8d54175770e36 --- /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 0000000000000..778281885d889 --- /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); + } +}