diff --git a/lib/private/Repair/NC29/ValidateAccountProperties.php b/lib/private/Repair/NC29/ValidateAccountProperties.php index 266266c8a1c93..640cd678ba07a 100644 --- a/lib/private/Repair/NC29/ValidateAccountProperties.php +++ b/lib/private/Repair/NC29/ValidateAccountProperties.php @@ -18,6 +18,13 @@ class ValidateAccountProperties implements IRepairStep { + private const PROPERTIES_TO_CHECK = [ + IAccountManager::PROPERTY_PHONE, + IAccountManager::PROPERTY_WEBSITE, + IAccountManager::PROPERTY_TWITTER, + IAccountManager::PROPERTY_FEDIVERSE, + ]; + public function __construct( private IUserManager $userManager, private IAccountManager $accountManager, @@ -34,10 +41,20 @@ public function run(IOutput $output): void { $this->userManager->callForSeenUsers(function (IUser $user) use (&$numRemoved) { $account = $this->accountManager->getAccount($user); - while (true) { + $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); - break; + return; } catch (InvalidArgumentException $e) { if (in_array($e->getMessage(), IAccountManager::ALLOWED_PROPERTIES)) { $numRemoved++; @@ -45,10 +62,11 @@ public function run(IOutput $output): void { $account->setProperty($property->getName(), '', $property->getScope(), IAccountManager::NOT_VERIFIED); } else { $this->logger->error('Error while sanitizing account property', ['exception' => $e, 'user' => $user->getUID()]); - break; + return; } } } + $this->logger->error('Iteration limit exceeded while cleaning account properties', ['user' => $user->getUID()]); }); if ($numRemoved > 0) { diff --git a/tests/lib/Repair/NC29/ValidateAccountPropertiesTest.php b/tests/lib/Repair/NC29/ValidateAccountPropertiesTest.php index cc43a63ca19c3..e113f1809980e 100644 --- a/tests/lib/Repair/NC29/ValidateAccountPropertiesTest.php +++ b/tests/lib/Repair/NC29/ValidateAccountPropertiesTest.php @@ -43,6 +43,7 @@ public function testRun(): void { $users = [ $this->createMock(IUser::class), $this->createMock(IUser::class), + $this->createMock(IUser::class), ]; $this->userManager ->expects(self::once()) @@ -61,15 +62,39 @@ public function testRun(): void { $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(2)) + ->expects(self::exactly(3)) ->method('getAccount') ->willReturnMap([ [$users[0], $account1], [$users[1], $account2], + [$users[2], $account3], ]); $valid = false; $this->accountManager->expects(self::exactly(3))