Skip to content

Commit 4c6c1e3

Browse files
blizzzblizzz
authored
Merge pull request #51005 from nextcloud/backport/50985/stable31
[stable31] fix: validate account properties as a repair step
2 parents a1f7480 + 06ddb7a commit 4c6c1e3

File tree

8 files changed

+271
-75
lines changed

8 files changed

+271
-75
lines changed

‎lib/composer/composer/autoload_classmap.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1882,10 +1882,11 @@
18821882
'OC\\Repair\\NC20\\EncryptionMigration' => $baseDir . '/lib/private/Repair/NC20/EncryptionMigration.php',
18831883
'OC\\Repair\\NC20\\ShippedDashboardEnable' => $baseDir . '/lib/private/Repair/NC20/ShippedDashboardEnable.php',
18841884
'OC\\Repair\\NC21\\AddCheckForUserCertificatesJob' => $baseDir . '/lib/private/Repair/NC21/AddCheckForUserCertificatesJob.php',
1885-
'OC\\Repair\\NC21\\ValidatePhoneNumber' => $baseDir . '/lib/private/Repair/NC21/ValidatePhoneNumber.php',
18861885
'OC\\Repair\\NC22\\LookupServerSendCheck' => $baseDir . '/lib/private/Repair/NC22/LookupServerSendCheck.php',
18871886
'OC\\Repair\\NC24\\AddTokenCleanupJob' => $baseDir . '/lib/private/Repair/NC24/AddTokenCleanupJob.php',
18881887
'OC\\Repair\\NC25\\AddMissingSecretJob' => $baseDir . '/lib/private/Repair/NC25/AddMissingSecretJob.php',
1888+
'OC\\Repair\\NC29\\SanitizeAccountProperties' => $baseDir . '/lib/private/Repair/NC29/SanitizeAccountProperties.php',
1889+
'OC\\Repair\\NC29\\SanitizeAccountPropertiesJob' => $baseDir . '/lib/private/Repair/NC29/SanitizeAccountPropertiesJob.php',
18891890
'OC\\Repair\\NC30\\RemoveLegacyDatadirFile' => $baseDir . '/lib/private/Repair/NC30/RemoveLegacyDatadirFile.php',
18901891
'OC\\Repair\\OldGroupMembershipShares' => $baseDir . '/lib/private/Repair/OldGroupMembershipShares.php',
18911892
'OC\\Repair\\Owncloud\\CleanPreviews' => $baseDir . '/lib/private/Repair/Owncloud/CleanPreviews.php',

‎lib/composer/composer/autoload_static.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1931,10 +1931,11 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
19311931
'OC\\Repair\\NC20\\EncryptionMigration' => __DIR__ . '/../../..' . '/lib/private/Repair/NC20/EncryptionMigration.php',
19321932
'OC\\Repair\\NC20\\ShippedDashboardEnable' => __DIR__ . '/../../..' . '/lib/private/Repair/NC20/ShippedDashboardEnable.php',
19331933
'OC\\Repair\\NC21\\AddCheckForUserCertificatesJob' => __DIR__ . '/../../..' . '/lib/private/Repair/NC21/AddCheckForUserCertificatesJob.php',
1934-
'OC\\Repair\\NC21\\ValidatePhoneNumber' => __DIR__ . '/../../..' . '/lib/private/Repair/NC21/ValidatePhoneNumber.php',
19351934
'OC\\Repair\\NC22\\LookupServerSendCheck' => __DIR__ . '/../../..' . '/lib/private/Repair/NC22/LookupServerSendCheck.php',
19361935
'OC\\Repair\\NC24\\AddTokenCleanupJob' => __DIR__ . '/../../..' . '/lib/private/Repair/NC24/AddTokenCleanupJob.php',
19371936
'OC\\Repair\\NC25\\AddMissingSecretJob' => __DIR__ . '/../../..' . '/lib/private/Repair/NC25/AddMissingSecretJob.php',
1937+
'OC\\Repair\\NC29\\SanitizeAccountProperties' => __DIR__ . '/../../..' . '/lib/private/Repair/NC29/SanitizeAccountProperties.php',
1938+
'OC\\Repair\\NC29\\SanitizeAccountPropertiesJob' => __DIR__ . '/../../..' . '/lib/private/Repair/NC29/SanitizeAccountPropertiesJob.php',
19381939
'OC\\Repair\\NC30\\RemoveLegacyDatadirFile' => __DIR__ . '/../../..' . '/lib/private/Repair/NC30/RemoveLegacyDatadirFile.php',
19391940
'OC\\Repair\\OldGroupMembershipShares' => __DIR__ . '/../../..' . '/lib/private/Repair/OldGroupMembershipShares.php',
19401941
'OC\\Repair\\Owncloud\\CleanPreviews' => __DIR__ . '/../../..' . '/lib/private/Repair/Owncloud/CleanPreviews.php',

‎lib/private/Repair.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,10 @@
3737
use OC\Repair\NC20\EncryptionMigration;
3838
use OC\Repair\NC20\ShippedDashboardEnable;
3939
use OC\Repair\NC21\AddCheckForUserCertificatesJob;
40-
use OC\Repair\NC21\ValidatePhoneNumber;
4140
use OC\Repair\NC22\LookupServerSendCheck;
4241
use OC\Repair\NC24\AddTokenCleanupJob;
4342
use OC\Repair\NC25\AddMissingSecretJob;
43+
use OC\Repair\NC29\SanitizeAccountProperties;
4444
use OC\Repair\NC30\RemoveLegacyDatadirFile;
4545
use OC\Repair\OldGroupMembershipShares;
4646
use OC\Repair\Owncloud\CleanPreviews;
@@ -194,6 +194,7 @@ public static function getRepairSteps(): array {
194194
\OCP\Server::get(RepairLogoDimension::class),
195195
\OCP\Server::get(RemoveLegacyDatadirFile::class),
196196
\OCP\Server::get(AddCleanupDeletedUsersBackgroundJob::class),
197+
\OCP\Server::get(SanitizeAccountProperties::class),
197198
];
198199
}
199200

@@ -212,8 +213,7 @@ public static function getExpensiveRepairSteps() {
212213
\OCP\Server::get(IAppConfig::class),
213214
\OCP\Server::get(IDBConnection::class)
214215
),
215-
\OC::$server->get(ValidatePhoneNumber::class),
216-
\OC::$server->get(DeleteSchedulingObjects::class),
216+
\OCP\Server::get(DeleteSchedulingObjects::class),
217217
];
218218
}
219219

‎lib/private/Repair/NC21/ValidatePhoneNumber.php‎

Lines changed: 0 additions & 70 deletions
This file was deleted.

‎lib/private/Repair/NC29/SanitizeAccountProperties.php‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
namespace OC\Repair\NC29;
10+
11+
use OCP\BackgroundJob\IJobList;
12+
use OCP\Migration\IOutput;
13+
use OCP\Migration\IRepairStep;
14+
15+
class SanitizeAccountProperties implements IRepairStep {
16+
17+
public function __construct(
18+
private IJobList $jobList,
19+
) {
20+
}
21+
22+
public function getName(): string {
23+
return 'Validate account properties and store phone numbers in a known format for search';
24+
}
25+
26+
public function run(IOutput $output): void {
27+
$this->jobList->add(SanitizeAccountPropertiesJob::class, null);
28+
$output->info('Queued background to validate account properties.');
29+
}
30+
}

‎lib/private/Repair/NC29/SanitizeAccountPropertiesJob.php‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
namespace OC\Repair\NC29;
10+
11+
use InvalidArgumentException;
12+
use OCP\Accounts\IAccountManager;
13+
use OCP\AppFramework\Utility\ITimeFactory;
14+
use OCP\BackgroundJob\QueuedJob;
15+
use OCP\IUser;
16+
use OCP\IUserManager;
17+
use Psr\Log\LoggerInterface;
18+
19+
class SanitizeAccountPropertiesJob extends QueuedJob {
20+
21+
private const PROPERTIES_TO_CHECK = [
22+
IAccountManager::PROPERTY_PHONE,
23+
IAccountManager::PROPERTY_WEBSITE,
24+
IAccountManager::PROPERTY_TWITTER,
25+
IAccountManager::PROPERTY_FEDIVERSE,
26+
];
27+
28+
public function __construct(
29+
ITimeFactory $timeFactory,
30+
private IUserManager $userManager,
31+
private IAccountManager $accountManager,
32+
private LoggerInterface $logger,
33+
) {
34+
parent::__construct($timeFactory);
35+
$this->setAllowParallelRuns(false);
36+
}
37+
38+
protected function run(mixed $argument): void {
39+
$numRemoved = 0;
40+
41+
$this->userManager->callForSeenUsers(function (IUser $user) use (&$numRemoved) {
42+
$account = $this->accountManager->getAccount($user);
43+
$properties = array_keys($account->jsonSerialize());
44+
45+
// Check if there are some properties we can sanitize - reduces number of db queries
46+
if (empty(array_intersect($properties, self::PROPERTIES_TO_CHECK))) {
47+
return;
48+
}
49+
50+
// Limit the loop to the properties we check to ensure there are no infinite loops
51+
// we add one additional loop (+ 1) as we need 1 loop for checking + 1 for update.
52+
$iteration = count(self::PROPERTIES_TO_CHECK) + 1;
53+
while ($iteration-- > 0) {
54+
try {
55+
$this->accountManager->updateAccount($account);
56+
return;
57+
} catch (InvalidArgumentException $e) {
58+
if (in_array($e->getMessage(), IAccountManager::ALLOWED_PROPERTIES)) {
59+
$numRemoved++;
60+
$property = $account->getProperty($e->getMessage());
61+
$account->setProperty($property->getName(), '', $property->getScope(), IAccountManager::NOT_VERIFIED);
62+
} else {
63+
$this->logger->error('Error while sanitizing account property', ['exception' => $e, 'user' => $user->getUID()]);
64+
return;
65+
}
66+
}
67+
}
68+
$this->logger->error('Iteration limit exceeded while cleaning account properties', ['user' => $user->getUID()]);
69+
});
70+
71+
if ($numRemoved > 0) {
72+
$this->logger->info('Cleaned ' . $numRemoved . ' invalid account property entries');
73+
}
74+
}
75+
}

‎tests/lib/Repair/NC29/SanitizeAccountPropertiesJobTest.php‎

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
namespace OC\Repair\NC29;
10+
11+
use InvalidArgumentException;
12+
use OCP\Accounts\IAccount;
13+
use OCP\Accounts\IAccountManager;
14+
use OCP\Accounts\IAccountProperty;
15+
use OCP\AppFramework\Utility\ITimeFactory;
16+
use OCP\IUser;
17+
use OCP\IUserManager;
18+
use PHPUnit\Framework\MockObject\MockObject;
19+
use Psr\Log\LoggerInterface;
20+
use Test\TestCase;
21+
22+
class SanitizeAccountPropertiesJobTest extends TestCase {
23+
24+
private IUserManager&MockObject $userManager;
25+
private IAccountManager&MockObject $accountManager;
26+
private LoggerInterface&MockObject $logger;
27+
28+
private SanitizeAccountPropertiesJob $job;
29+
30+
protected function setUp(): void {
31+
$this->userManager = $this->createMock(IUserManager::class);
32+
$this->accountManager = $this->createMock(IAccountManager::class);
33+
$this->logger = $this->createMock(LoggerInterface::class);
34+
35+
$this->job = new SanitizeAccountPropertiesJob(
36+
$this->createMock(ITimeFactory::class),
37+
$this->userManager,
38+
$this->accountManager,
39+
$this->logger,
40+
);
41+
}
42+
43+
public function testParallel() {
44+
self::assertFalse($this->job->getAllowParallelRuns());
45+
}
46+
47+
public function testRun(): void {
48+
$users = [
49+
$this->createMock(IUser::class),
50+
$this->createMock(IUser::class),
51+
$this->createMock(IUser::class),
52+
];
53+
$this->userManager
54+
->expects(self::once())
55+
->method('callForSeenUsers')
56+
->willReturnCallback(fn ($fn) => array_map($fn, $users));
57+
58+
$property = $this->createMock(IAccountProperty::class);
59+
$property->expects(self::once())->method('getName')->willReturn(IAccountManager::PROPERTY_PHONE);
60+
$property->expects(self::once())->method('getScope')->willReturn(IAccountManager::SCOPE_LOCAL);
61+
62+
$account1 = $this->createMock(IAccount::class);
63+
$account1->expects(self::once())
64+
->method('getProperty')
65+
->with(IAccountManager::PROPERTY_PHONE)
66+
->willReturn($property);
67+
$account1->expects(self::once())
68+
->method('setProperty')
69+
->with(IAccountManager::PROPERTY_PHONE, '', IAccountManager::SCOPE_LOCAL, IAccountManager::NOT_VERIFIED);
70+
$account1->expects(self::once())
71+
->method('jsonSerialize')
72+
->willReturn([
73+
IAccountManager::PROPERTY_DISPLAYNAME => [],
74+
IAccountManager::PROPERTY_PHONE => [],
75+
]);
76+
77+
$account2 = $this->createMock(IAccount::class);
78+
$account2->expects(self::never())
79+
->method('getProperty');
80+
$account2->expects(self::once())
81+
->method('jsonSerialize')
82+
->willReturn([
83+
IAccountManager::PROPERTY_DISPLAYNAME => [],
84+
IAccountManager::PROPERTY_PHONE => [],
85+
]);
86+
87+
$account3 = $this->createMock(IAccount::class);
88+
$account3->expects(self::never())
89+
->method('getProperty');
90+
$account3->expects(self::once())
91+
->method('jsonSerialize')
92+
->willReturn([
93+
IAccountManager::PROPERTY_DISPLAYNAME => [],
94+
]);
95+
96+
$this->accountManager
97+
->expects(self::exactly(3))
98+
->method('getAccount')
99+
->willReturnMap([
100+
[$users[0], $account1],
101+
[$users[1], $account2],
102+
[$users[2], $account3],
103+
]);
104+
$valid = false;
105+
$this->accountManager->expects(self::exactly(3))
106+
->method('updateAccount')
107+
->willReturnCallback(function (IAccount $account) use (&$account1, &$valid) {
108+
if (!$valid && $account === $account1) {
109+
$valid = true;
110+
throw new InvalidArgumentException(IAccountManager::PROPERTY_PHONE);
111+
}
112+
});
113+
114+
self::invokePrivate($this->job, 'run', [null]);
115+
}
116+
}

‎tests/lib/Repair/NC29/SanitizeAccountPropertiesTest.php‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
namespace OC\Repair\NC29;
10+
11+
use OCP\BackgroundJob\IJobList;
12+
use OCP\Migration\IOutput;
13+
use PHPUnit\Framework\MockObject\MockObject;
14+
use Test\TestCase;
15+
16+
class SanitizeAccountPropertiesTest extends TestCase {
17+
18+
private IJobList&MockObject $jobList;
19+
private SanitizeAccountProperties $repairStep;
20+
21+
protected function setUp(): void {
22+
$this->jobList = $this->createMock(IJobList::class);
23+
24+
$this->repairStep = new SanitizeAccountProperties($this->jobList);
25+
}
26+
27+
public function testGetName(): void {
28+
self::assertStringContainsString('Validate account properties', $this->repairStep->getName());
29+
}
30+
31+
public function testRun(): void {
32+
$this->jobList->expects(self::once())
33+
->method('add')
34+
->with(SanitizeAccountPropertiesJob::class, null);
35+
36+
$output = $this->createMock(IOutput::class);
37+
$output->expects(self::once())
38+
->method('info')
39+
->with(self::matchesRegularExpression('/queued background/i'));
40+
41+
$this->repairStep->run($output);
42+
}
43+
}

0 commit comments

Comments
 (0)