-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge pull request #50985 from nextcloud/fix/account-property-validation
fix: validate account properties as a repair step
- Loading branch information
Showing
8 changed files
with
271 additions
and
75 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| /** | ||
| * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors | ||
| * SPDX-License-Identifier: AGPL-3.0-or-later | ||
| */ | ||
| namespace OC\Repair\NC29; | ||
|
|
||
| use OCP\BackgroundJob\IJobList; | ||
| use OCP\Migration\IOutput; | ||
| use OCP\Migration\IRepairStep; | ||
|
|
||
| class SanitizeAccountProperties implements IRepairStep { | ||
|
|
||
| public function __construct( | ||
| private IJobList $jobList, | ||
| ) { | ||
| } | ||
|
|
||
| public function getName(): string { | ||
| return 'Validate account properties and store phone numbers in a known format for search'; | ||
| } | ||
|
|
||
| public function run(IOutput $output): void { | ||
| $this->jobList->add(SanitizeAccountPropertiesJob::class, null); | ||
| $output->info('Queued background to validate account properties.'); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| /** | ||
| * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors | ||
| * SPDX-License-Identifier: AGPL-3.0-or-later | ||
| */ | ||
| namespace OC\Repair\NC29; | ||
|
|
||
| use InvalidArgumentException; | ||
| use OCP\Accounts\IAccountManager; | ||
| use OCP\AppFramework\Utility\ITimeFactory; | ||
| use OCP\BackgroundJob\QueuedJob; | ||
| use OCP\IUser; | ||
| use OCP\IUserManager; | ||
| use Psr\Log\LoggerInterface; | ||
|
|
||
| class SanitizeAccountPropertiesJob extends QueuedJob { | ||
|
|
||
| private const PROPERTIES_TO_CHECK = [ | ||
| IAccountManager::PROPERTY_PHONE, | ||
| IAccountManager::PROPERTY_WEBSITE, | ||
| IAccountManager::PROPERTY_TWITTER, | ||
| IAccountManager::PROPERTY_FEDIVERSE, | ||
| ]; | ||
|
|
||
| public function __construct( | ||
| ITimeFactory $timeFactory, | ||
| private IUserManager $userManager, | ||
| private IAccountManager $accountManager, | ||
| private LoggerInterface $logger, | ||
| ) { | ||
| parent::__construct($timeFactory); | ||
| $this->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'); | ||
| } | ||
| } | ||
| } |
116 changes: 116 additions & 0 deletions
116
tests/lib/Repair/NC29/SanitizeAccountPropertiesJobTest.php
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| /** | ||
| * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors | ||
| * SPDX-License-Identifier: AGPL-3.0-or-later | ||
| */ | ||
| namespace OC\Repair\NC29; | ||
|
|
||
| use InvalidArgumentException; | ||
| use OCP\Accounts\IAccount; | ||
| use OCP\Accounts\IAccountManager; | ||
| use OCP\Accounts\IAccountProperty; | ||
| use OCP\AppFramework\Utility\ITimeFactory; | ||
| use OCP\IUser; | ||
| use OCP\IUserManager; | ||
| use PHPUnit\Framework\MockObject\MockObject; | ||
| use Psr\Log\LoggerInterface; | ||
| use Test\TestCase; | ||
|
|
||
| class SanitizeAccountPropertiesJobTest extends TestCase { | ||
|
|
||
| private IUserManager&MockObject $userManager; | ||
| private IAccountManager&MockObject $accountManager; | ||
| private LoggerInterface&MockObject $logger; | ||
|
|
||
| private SanitizeAccountPropertiesJob $job; | ||
|
|
||
| protected function setUp(): void { | ||
| $this->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]); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| /** | ||
| * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors | ||
| * SPDX-License-Identifier: AGPL-3.0-or-later | ||
| */ | ||
| namespace OC\Repair\NC29; | ||
|
|
||
| use OCP\BackgroundJob\IJobList; | ||
| use OCP\Migration\IOutput; | ||
| use PHPUnit\Framework\MockObject\MockObject; | ||
| use Test\TestCase; | ||
|
|
||
| class SanitizeAccountPropertiesTest extends TestCase { | ||
|
|
||
| private IJobList&MockObject $jobList; | ||
| private SanitizeAccountProperties $repairStep; | ||
|
|
||
| protected function setUp(): void { | ||
| $this->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); | ||
| } | ||
| } |