Skip to content

Commit b714280

Browse files
susnuxsusnux
committed
fix: Make user removal more resilient
Currently there is a problem if an exception is thrown in `User::delete`, because at that point the user is already removed from the backend, but not all data is deleted. There is no way to recover from this state, as the user is gone no information is available anymore. This means the data is still available on the server but can not removed by any API anymore. The solution here is to first set a flag and backup the user home, this can be used to recover failed user deletions in a way the delete can be re-tried. Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
1 parent 5fc5148 commit b714280

File tree

11 files changed

+301
-82
lines changed

11 files changed

+301
-82
lines changed

‎lib/composer/composer/autoload_classmap.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1791,6 +1791,7 @@
17911791
'OC\\RepairException' => $baseDir . '/lib/private/RepairException.php',
17921792
'OC\\Repair\\AddAppConfigLazyMigration' => $baseDir . '/lib/private/Repair/AddAppConfigLazyMigration.php',
17931793
'OC\\Repair\\AddBruteForceCleanupJob' => $baseDir . '/lib/private/Repair/AddBruteForceCleanupJob.php',
1794+
'OC\\Repair\\AddCleanupDeletedUsersBackgroundJob' => $baseDir . '/lib/private/Repair/AddCleanupDeletedUsersBackgroundJob.php',
17941795
'OC\\Repair\\AddCleanupUpdaterBackupsJob' => $baseDir . '/lib/private/Repair/AddCleanupUpdaterBackupsJob.php',
17951796
'OC\\Repair\\AddMetadataGenerationJob' => $baseDir . '/lib/private/Repair/AddMetadataGenerationJob.php',
17961797
'OC\\Repair\\AddRemoveOldTasksBackgroundJob' => $baseDir . '/lib/private/Repair/AddRemoveOldTasksBackgroundJob.php',
@@ -1989,8 +1990,10 @@
19891990
'OC\\UserStatus\\Manager' => $baseDir . '/lib/private/UserStatus/Manager.php',
19901991
'OC\\User\\AvailabilityCoordinator' => $baseDir . '/lib/private/User/AvailabilityCoordinator.php',
19911992
'OC\\User\\Backend' => $baseDir . '/lib/private/User/Backend.php',
1993+
'OC\\User\\BackgroundJobs\\CleanupDeletedUsers' => $baseDir . '/lib/private/User/BackgroundJobs/CleanupDeletedUsers.php',
19921994
'OC\\User\\Database' => $baseDir . '/lib/private/User/Database.php',
19931995
'OC\\User\\DisplayNameCache' => $baseDir . '/lib/private/User/DisplayNameCache.php',
1996+
'OC\\User\\FailedUsersBackend' => $baseDir . '/lib/private/User/FailedUsersBackend.php',
19941997
'OC\\User\\LazyUser' => $baseDir . '/lib/private/User/LazyUser.php',
19951998
'OC\\User\\Listeners\\BeforeUserDeletedListener' => $baseDir . '/lib/private/User/Listeners/BeforeUserDeletedListener.php',
19961999
'OC\\User\\Listeners\\UserChangedListener' => $baseDir . '/lib/private/User/Listeners/UserChangedListener.php',

‎lib/composer/composer/autoload_static.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1824,6 +1824,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
18241824
'OC\\RepairException' => __DIR__ . '/../../..' . '/lib/private/RepairException.php',
18251825
'OC\\Repair\\AddAppConfigLazyMigration' => __DIR__ . '/../../..' . '/lib/private/Repair/AddAppConfigLazyMigration.php',
18261826
'OC\\Repair\\AddBruteForceCleanupJob' => __DIR__ . '/../../..' . '/lib/private/Repair/AddBruteForceCleanupJob.php',
1827+
'OC\\Repair\\AddCleanupDeletedUsersBackgroundJob' => __DIR__ . '/../../..' . '/lib/private/Repair/AddCleanupDeletedUsersBackgroundJob.php',
18271828
'OC\\Repair\\AddCleanupUpdaterBackupsJob' => __DIR__ . '/../../..' . '/lib/private/Repair/AddCleanupUpdaterBackupsJob.php',
18281829
'OC\\Repair\\AddMetadataGenerationJob' => __DIR__ . '/../../..' . '/lib/private/Repair/AddMetadataGenerationJob.php',
18291830
'OC\\Repair\\AddRemoveOldTasksBackgroundJob' => __DIR__ . '/../../..' . '/lib/private/Repair/AddRemoveOldTasksBackgroundJob.php',
@@ -2022,8 +2023,10 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
20222023
'OC\\UserStatus\\Manager' => __DIR__ . '/../../..' . '/lib/private/UserStatus/Manager.php',
20232024
'OC\\User\\AvailabilityCoordinator' => __DIR__ . '/../../..' . '/lib/private/User/AvailabilityCoordinator.php',
20242025
'OC\\User\\Backend' => __DIR__ . '/../../..' . '/lib/private/User/Backend.php',
2026+
'OC\\User\\BackgroundJobs\\CleanupDeletedUsers' => __DIR__ . '/../../..' . '/lib/private/User/BackgroundJobs/CleanupDeletedUsers.php',
20252027
'OC\\User\\Database' => __DIR__ . '/../../..' . '/lib/private/User/Database.php',
20262028
'OC\\User\\DisplayNameCache' => __DIR__ . '/../../..' . '/lib/private/User/DisplayNameCache.php',
2029+
'OC\\User\\FailedUsersBackend' => __DIR__ . '/../../..' . '/lib/private/User/FailedUsersBackend.php',
20272030
'OC\\User\\LazyUser' => __DIR__ . '/../../..' . '/lib/private/User/LazyUser.php',
20282031
'OC\\User\\Listeners\\BeforeUserDeletedListener' => __DIR__ . '/../../..' . '/lib/private/User/Listeners/BeforeUserDeletedListener.php',
20292032
'OC\\User\\Listeners\\UserChangedListener' => __DIR__ . '/../../..' . '/lib/private/User/Listeners/UserChangedListener.php',

‎lib/private/Authentication/Listeners/UserDeletedFilesCleanupListener.php‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,27 +16,31 @@
1616
use OCP\Files\Storage\IStorage;
1717
use OCP\User\Events\BeforeUserDeletedEvent;
1818
use OCP\User\Events\UserDeletedEvent;
19+
use Psr\Log\LoggerInterface;
1920

2021
/** @template-implements IEventListener<BeforeUserDeletedEvent|UserDeletedEvent> */
2122
class UserDeletedFilesCleanupListener implements IEventListener {
2223
/** @var array<string,IStorage> */
2324
private $homeStorageCache = [];
2425

25-
/** @var IMountProviderCollection */
26-
private $mountProviderCollection;
27-
28-
public function __construct(IMountProviderCollection $mountProviderCollection) {
29-
$this->mountProviderCollection = $mountProviderCollection;
26+
public function __construct(
27+
private IMountProviderCollection $mountProviderCollection,
28+
private LoggerInterface $logger,
29+
) {
3030
}
3131

3232
public function handle(Event $event): void {
33+
$user = $event->getUser();
34+
3335
// since we can't reliably get the user home storage after the user is deleted
3436
// but the user deletion might get canceled during the before event
3537
// we only cache the user home storage during the before event and then do the
3638
// action deletion during the after event
3739

3840
if ($event instanceof BeforeUserDeletedEvent) {
39-
$userHome = $this->mountProviderCollection->getHomeMountForUser($event->getUser());
41+
$this->logger->debug('Prepare deleting storage for user {userId}', ['userId' => $user->getUID()]);
42+
43+
$userHome = $this->mountProviderCollection->getHomeMountForUser($user);
4044
$storage = $userHome->getStorage();
4145
if (!$storage) {
4246
throw new \Exception('Account has no home storage');
@@ -51,12 +55,14 @@ public function handle(Event $event): void {
5155
$this->homeStorageCache[$event->getUser()->getUID()] = $storage;
5256
}
5357
if ($event instanceof UserDeletedEvent) {
54-
if (!isset($this->homeStorageCache[$event->getUser()->getUID()])) {
58+
if (!isset($this->homeStorageCache[$user->getUID()])) {
5559
throw new \Exception('UserDeletedEvent fired without matching BeforeUserDeletedEvent');
5660
}
57-
$storage = $this->homeStorageCache[$event->getUser()->getUID()];
61+
$storage = $this->homeStorageCache[$user->getUID()];
5862
$cache = $storage->getCache();
5963
$storage->rmdir('');
64+
$this->logger->debug('Deleted storage for user {userId}', ['userId' => $user->getUID()]);
65+
6066
if ($cache instanceof Cache) {
6167
$cache->clear();
6268
} else {

‎lib/private/Repair.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use OC\DB\ConnectionAdapter;
1212
use OC\Repair\AddAppConfigLazyMigration;
1313
use OC\Repair\AddBruteForceCleanupJob;
14+
use OC\Repair\AddCleanupDeletedUsersBackgroundJob;
1415
use OC\Repair\AddCleanupUpdaterBackupsJob;
1516
use OC\Repair\AddMetadataGenerationJob;
1617
use OC\Repair\AddRemoveOldTasksBackgroundJob;
@@ -189,6 +190,7 @@ public static function getRepairSteps(): array {
189190
\OCP\Server::get(AddAppConfigLazyMigration::class),
190191
\OCP\Server::get(RepairLogoDimension::class),
191192
\OCP\Server::get(RemoveLegacyDatadirFile::class),
193+
\OCP\Server::get(AddCleanupDeletedUsersBackgroundJob::class),
192194
];
193195
}
194196

‎lib/private/Repair/AddCleanupDeletedUsersBackgroundJob.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: 2024 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
namespace OC\Repair;
10+
11+
use OC\User\BackgroundJobs\CleanupDeletedUsers;
12+
use OCP\BackgroundJob\IJobList;
13+
use OCP\Migration\IOutput;
14+
use OCP\Migration\IRepairStep;
15+
16+
class AddCleanupDeletedUsersBackgroundJob implements IRepairStep {
17+
private IJobList $jobList;
18+
19+
public function __construct(IJobList $jobList) {
20+
$this->jobList = $jobList;
21+
}
22+
23+
public function getName(): string {
24+
return 'Add cleanup-deleted-users background job';
25+
}
26+
27+
public function run(IOutput $output) {
28+
$this->jobList->add(CleanupDeletedUsers::class);
29+
}
30+
}

‎lib/private/Setup.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
use OC\Log\Rotate;
1818
use OC\Preview\BackgroundCleanupJob;
1919
use OC\TextProcessing\RemoveOldTasksBackgroundJob;
20+
use OC\User\BackgroundJobs\CleanupDeletedUsers;
2021
use OCP\AppFramework\Utility\ITimeFactory;
2122
use OCP\BackgroundJob\IJobList;
2223
use OCP\Defaults;
@@ -414,6 +415,7 @@ public static function installBackgroundJobs(): void {
414415
$jobList->add(Rotate::class);
415416
$jobList->add(BackgroundCleanupJob::class);
416417
$jobList->add(RemoveOldTasksBackgroundJob::class);
418+
$jobList->add(CleanupDeletedUsers::class);
417419
}
418420

419421
/**

‎lib/private/User/Backend.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,9 +107,9 @@ public function userExists($uid) {
107107
/**
108108
* get the user's home directory
109109
* @param string $uid the username
110-
* @return boolean
110+
* @return string|boolean
111111
*/
112-
public function getHome($uid) {
112+
public function getHome(string $uid) {
113113
return false;
114114
}
115115

‎lib/private/User/BackgroundJobs/CleanupDeletedUsers.php‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
namespace OC\User\BackgroundJobs;
10+
11+
use OC\User\FailedUsersBackend;
12+
use OC\User\Manager;
13+
use OC\User\User;
14+
use OCP\AppFramework\Utility\ITimeFactory;
15+
use OCP\BackgroundJob\TimedJob;
16+
use OCP\EventDispatcher\IEventDispatcher;
17+
use OCP\IConfig;
18+
use Psr\Log\LoggerInterface;
19+
20+
class CleanupDeletedUsers extends TimedJob {
21+
public function __construct(
22+
ITimeFactory $time,
23+
private Manager $userManager,
24+
private IConfig $config,
25+
private LoggerInterface $logger,
26+
) {
27+
parent::__construct($time);
28+
$this->setInterval(3600);
29+
}
30+
31+
protected function run($argument): void {
32+
$backend = new FailedUsersBackend($this->config);
33+
$users = $backend->getUsers();
34+
35+
if (empty($users)) {
36+
$this->logger->debug('No failed deleted users found.');
37+
return;
38+
}
39+
40+
foreach ($users as $userId) {
41+
try {
42+
$user = new User(
43+
$userId,
44+
$backend,
45+
\OCP\Server::get(IEventDispatcher::class),
46+
config: $this->config,
47+
);
48+
$user->delete();
49+
$this->logger->info('Cleaned up deleted user {userId}', ['userId' => $userId]);
50+
} catch (\Throwable $error) {
51+
$this->logger->warning('Could not cleanup deleted user {userId}', ['userId' => $userId, 'exception' => $error]);
52+
}
53+
}
54+
}
55+
}

‎lib/private/User/FailedUsersBackend.php‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
<?php
2+
3+
/**
4+
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
5+
* SPDX-License-Identifier: AGPL-3.0-or-later
6+
*/
7+
namespace OC\User;
8+
9+
use OCP\IConfig;
10+
use OCP\IUserBackend;
11+
use OCP\User\Backend\IGetHomeBackend;
12+
13+
/**
14+
* This is a "fake" backend for users that were deleted,
15+
* but not properly removed from Nextcloud (e.g. an exception occurred).
16+
* This backend is only needed because some APIs in user-deleted-events require a "real" user with backend.
17+
*/
18+
class FailedUsersBackend extends Backend implements IGetHomeBackend, IUserBackend {
19+
20+
public function __construct(
21+
private IConfig $config,
22+
) {
23+
}
24+
25+
public function deleteUser($uid): bool {
26+
// fake true, deleting failed users is automatically handled by User::delete()
27+
return true;
28+
}
29+
30+
public function getBackendName(): string {
31+
return 'deleted users';
32+
}
33+
34+
public function userExists($uid) {
35+
return $this->config->getUserValue($uid, 'core', 'deleted') === 'true';
36+
}
37+
38+
public function getHome(string $uid): string|false {
39+
return $this->config->getUserValue($uid, 'core', 'deleted.backup-home') ?: false;
40+
}
41+
42+
public function getUsers($search = '', $limit = null, $offset = null) {
43+
return $this->config->getUsersForUserValue('core', 'deleted', 'true');
44+
}
45+
46+
}

0 commit comments

Comments
 (0)