Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[stable28] fix: Make user removal more resilient #48587

Open
wants to merge 3 commits into
base: stable28
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -1613,6 +1613,7 @@
'OC\\Repair' => $baseDir . '/lib/private/Repair.php',
'OC\\RepairException' => $baseDir . '/lib/private/RepairException.php',
'OC\\Repair\\AddBruteForceCleanupJob' => $baseDir . '/lib/private/Repair/AddBruteForceCleanupJob.php',
'OC\\Repair\\AddCleanupDeletedUsersBackgroundJob' => $baseDir . '/lib/private/Repair/AddCleanupDeletedUsersBackgroundJob.php',
'OC\\Repair\\AddCleanupUpdaterBackupsJob' => $baseDir . '/lib/private/Repair/AddCleanupUpdaterBackupsJob.php',
'OC\\Repair\\AddMetadataGenerationJob' => $baseDir . '/lib/private/Repair/AddMetadataGenerationJob.php',
'OC\\Repair\\AddRemoveOldTasksBackgroundJob' => $baseDir . '/lib/private/Repair/AddRemoveOldTasksBackgroundJob.php',
Expand Down Expand Up @@ -1804,6 +1805,7 @@
'OC\\UserStatus\\Manager' => $baseDir . '/lib/private/UserStatus/Manager.php',
'OC\\User\\AvailabilityCoordinator' => $baseDir . '/lib/private/User/AvailabilityCoordinator.php',
'OC\\User\\Backend' => $baseDir . '/lib/private/User/Backend.php',
'OC\\User\\BackgroundJobs\\CleanupDeletedUsers' => $baseDir . '/lib/private/User/BackgroundJobs/CleanupDeletedUsers.php',
'OC\\User\\Database' => $baseDir . '/lib/private/User/Database.php',
'OC\\User\\DisplayNameCache' => $baseDir . '/lib/private/User/DisplayNameCache.php',
'OC\\User\\LazyUser' => $baseDir . '/lib/private/User/LazyUser.php',
Expand All @@ -1813,6 +1815,7 @@
'OC\\User\\Manager' => $baseDir . '/lib/private/User/Manager.php',
'OC\\User\\NoUserException' => $baseDir . '/lib/private/User/NoUserException.php',
'OC\\User\\OutOfOfficeData' => $baseDir . '/lib/private/User/OutOfOfficeData.php',
'OC\\User\\PartiallyDeletedUsersBackend' => $baseDir . '/lib/private/User/PartiallyDeletedUsersBackend.php',
'OC\\User\\Session' => $baseDir . '/lib/private/User/Session.php',
'OC\\User\\User' => $baseDir . '/lib/private/User/User.php',
'OC_API' => $baseDir . '/lib/private/legacy/OC_API.php',
Expand Down
3 changes: 3 additions & 0 deletions lib/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -1646,6 +1646,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
'OC\\Repair' => __DIR__ . '/../../..' . '/lib/private/Repair.php',
'OC\\RepairException' => __DIR__ . '/../../..' . '/lib/private/RepairException.php',
'OC\\Repair\\AddBruteForceCleanupJob' => __DIR__ . '/../../..' . '/lib/private/Repair/AddBruteForceCleanupJob.php',
'OC\\Repair\\AddCleanupDeletedUsersBackgroundJob' => __DIR__ . '/../../..' . '/lib/private/Repair/AddCleanupDeletedUsersBackgroundJob.php',
'OC\\Repair\\AddCleanupUpdaterBackupsJob' => __DIR__ . '/../../..' . '/lib/private/Repair/AddCleanupUpdaterBackupsJob.php',
'OC\\Repair\\AddMetadataGenerationJob' => __DIR__ . '/../../..' . '/lib/private/Repair/AddMetadataGenerationJob.php',
'OC\\Repair\\AddRemoveOldTasksBackgroundJob' => __DIR__ . '/../../..' . '/lib/private/Repair/AddRemoveOldTasksBackgroundJob.php',
Expand Down Expand Up @@ -1837,6 +1838,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
'OC\\UserStatus\\Manager' => __DIR__ . '/../../..' . '/lib/private/UserStatus/Manager.php',
'OC\\User\\AvailabilityCoordinator' => __DIR__ . '/../../..' . '/lib/private/User/AvailabilityCoordinator.php',
'OC\\User\\Backend' => __DIR__ . '/../../..' . '/lib/private/User/Backend.php',
'OC\\User\\BackgroundJobs\\CleanupDeletedUsers' => __DIR__ . '/../../..' . '/lib/private/User/BackgroundJobs/CleanupDeletedUsers.php',
'OC\\User\\Database' => __DIR__ . '/../../..' . '/lib/private/User/Database.php',
'OC\\User\\DisplayNameCache' => __DIR__ . '/../../..' . '/lib/private/User/DisplayNameCache.php',
'OC\\User\\LazyUser' => __DIR__ . '/../../..' . '/lib/private/User/LazyUser.php',
Expand All @@ -1846,6 +1848,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
'OC\\User\\Manager' => __DIR__ . '/../../..' . '/lib/private/User/Manager.php',
'OC\\User\\NoUserException' => __DIR__ . '/../../..' . '/lib/private/User/NoUserException.php',
'OC\\User\\OutOfOfficeData' => __DIR__ . '/../../..' . '/lib/private/User/OutOfOfficeData.php',
'OC\\User\\PartiallyDeletedUsersBackend' => __DIR__ . '/../../..' . '/lib/private/User/PartiallyDeletedUsersBackend.php',
'OC\\User\\Session' => __DIR__ . '/../../..' . '/lib/private/User/Session.php',
'OC\\User\\User' => __DIR__ . '/../../..' . '/lib/private/User/User.php',
'OC_API' => __DIR__ . '/../../..' . '/lib/private/legacy/OC_API.php',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,26 +33,30 @@
use OCP\Files\Storage\IStorage;
use OCP\User\Events\BeforeUserDeletedEvent;
use OCP\User\Events\UserDeletedEvent;
use Psr\Log\LoggerInterface;

class UserDeletedFilesCleanupListener implements IEventListener {
/** @var array<string,IStorage> */
private $homeStorageCache = [];

/** @var IMountProviderCollection */
private $mountProviderCollection;

public function __construct(IMountProviderCollection $mountProviderCollection) {
$this->mountProviderCollection = $mountProviderCollection;
public function __construct(
private IMountProviderCollection $mountProviderCollection,
private LoggerInterface $logger,
) {
}

public function handle(Event $event): void {
$user = $event->getUser();

Check failure on line 49 in lib/private/Authentication/Listeners/UserDeletedFilesCleanupListener.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

UndefinedMethod

lib/private/Authentication/Listeners/UserDeletedFilesCleanupListener.php:49:19: UndefinedMethod: Method OCP\EventDispatcher\Event::getUser does not exist (see https://psalm.dev/022)

Check failure

Code scanning / Psalm

UndefinedMethod Error

Method OCP\EventDispatcher\Event::getUser does not exist

// since we can't reliably get the user home storage after the user is deleted
// but the user deletion might get canceled during the before event
// we only cache the user home storage during the before event and then do the
// action deletion during the after event

if ($event instanceof BeforeUserDeletedEvent) {
$userHome = $this->mountProviderCollection->getHomeMountForUser($event->getUser());
$this->logger->debug('Prepare deleting storage for user {userId}', ['userId' => $user->getUID()]);

$userHome = $this->mountProviderCollection->getHomeMountForUser($user);
$storage = $userHome->getStorage();
if (!$storage) {
throw new \Exception("User has no home storage");
Expand All @@ -67,12 +71,14 @@
$this->homeStorageCache[$event->getUser()->getUID()] = $storage;
}
if ($event instanceof UserDeletedEvent) {
if (!isset($this->homeStorageCache[$event->getUser()->getUID()])) {
throw new \Exception("UserDeletedEvent fired without matching BeforeUserDeletedEvent");
if (!isset($this->homeStorageCache[$user->getUID()])) {
throw new \Exception('UserDeletedEvent fired without matching BeforeUserDeletedEvent');
}
$storage = $this->homeStorageCache[$event->getUser()->getUID()];
$storage = $this->homeStorageCache[$user->getUID()];
$cache = $storage->getCache();
$storage->rmdir('');
$this->logger->debug('Deleted storage for user {userId}', ['userId' => $user->getUID()]);

if ($cache instanceof Cache) {
$cache->clear();
} else {
Expand Down
2 changes: 2 additions & 0 deletions lib/private/Repair.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
use OC\DB\Connection;
use OC\DB\ConnectionAdapter;
use OC\Repair\AddBruteForceCleanupJob;
use OC\Repair\AddCleanupDeletedUsersBackgroundJob;
use OC\Repair\AddCleanupUpdaterBackupsJob;
use OC\Repair\AddMetadataGenerationJob;
use OC\Repair\AddRemoveOldTasksBackgroundJob;
Expand Down Expand Up @@ -215,6 +216,7 @@ public static function getRepairSteps(): array {
\OCP\Server::get(AddRemoveOldTasksBackgroundJob::class),
\OCP\Server::get(AddMetadataGenerationJob::class),
\OCP\Server::get(RepairLogoDimension::class),
\OCP\Server::get(AddCleanupDeletedUsersBackgroundJob::class),
];
}

Expand Down
30 changes: 30 additions & 0 deletions lib/private/Repair/AddCleanupDeletedUsersBackgroundJob.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/
namespace OC\Repair;

use OC\User\BackgroundJobs\CleanupDeletedUsers;
use OCP\BackgroundJob\IJobList;
use OCP\Migration\IOutput;
use OCP\Migration\IRepairStep;

class AddCleanupDeletedUsersBackgroundJob implements IRepairStep {
private IJobList $jobList;

public function __construct(IJobList $jobList) {
$this->jobList = $jobList;
}

public function getName(): string {
return 'Add cleanup-deleted-users background job';
}

public function run(IOutput $output) {
$this->jobList->add(CleanupDeletedUsers::class);
}
}
2 changes: 2 additions & 0 deletions lib/private/Setup.php
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
use OC\Log\Rotate;
use OC\Preview\BackgroundCleanupJob;
use OC\TextProcessing\RemoveOldTasksBackgroundJob;
use OC\User\BackgroundJobs\CleanupDeletedUsers;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\Defaults;
use OCP\IGroup;
Expand Down Expand Up @@ -463,6 +464,7 @@ public static function installBackgroundJobs() {
$jobList->add(Rotate::class);
$jobList->add(BackgroundCleanupJob::class);
$jobList->add(RemoveOldTasksBackgroundJob::class);
$jobList->add(CleanupDeletedUsers::class);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions lib/private/User/Backend.php
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,9 @@ public function userExists($uid) {
/**
* get the user's home directory
* @param string $uid the username
* @return boolean
* @return string|boolean
*/
public function getHome($uid) {
public function getHome(string $uid) {
return false;
}

Expand Down
64 changes: 64 additions & 0 deletions lib/private/User/BackgroundJobs/CleanupDeletedUsers.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/
namespace OC\User\BackgroundJobs;

use OC\User\Manager;
use OC\User\PartiallyDeletedUsersBackend;
use OC\User\User;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\BackgroundJob\IJob;
use OCP\BackgroundJob\TimedJob;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\IConfig;
use Psr\Log\LoggerInterface;

class CleanupDeletedUsers extends TimedJob {
public function __construct(
ITimeFactory $time,
private Manager $userManager,
private IConfig $config,
private LoggerInterface $logger,
) {
parent::__construct($time);
$this->setTimeSensitivity(IJob::TIME_INSENSITIVE);
$this->setInterval(24 * 3600);
}

protected function run($argument): void {
$backend = new PartiallyDeletedUsersBackend($this->config);
$users = $backend->getUsers();

if (empty($users)) {
$this->logger->debug('No failed deleted users found.');
return;
}

foreach ($users as $userId) {
if ($this->userManager->userExists($userId)) {
$this->logger->info('Skipping user {userId}, marked as deleted, as they still exists in user backend.', ['userId' => $userId]);
$backend->unmarkUser($userId);
continue;
}

try {
$user = new User(
$userId,
$backend,
\OCP\Server::get(IEventDispatcher::class),
$this->userManager,
$this->config,
);
$user->delete();
$this->logger->info('Cleaned up deleted user {userId}', ['userId' => $userId]);
} catch (\Throwable $error) {
$this->logger->warning('Could not cleanup deleted user {userId}', ['userId' => $userId, 'exception' => $error]);
}
}
}
}
56 changes: 56 additions & 0 deletions lib/private/User/PartiallyDeletedUsersBackend.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
<?php

/**
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/
namespace OC\User;

use OCP\IConfig;
use OCP\IUserBackend;
use OCP\User\Backend\IGetHomeBackend;

/**
* This is a "fake" backend for users that were deleted,
* but not properly removed from Nextcloud (e.g. an exception occurred).
* This backend is only needed because some APIs in user-deleted-events require a "real" user with backend.
*/
class PartiallyDeletedUsersBackend extends Backend implements IGetHomeBackend, IUserBackend {

public function __construct(
private IConfig $config,
) {
}

public function deleteUser($uid): bool {
// fake true, deleting failed users is automatically handled by User::delete()
return true;
}

public function getBackendName(): string {
return 'deleted users';
}

public function userExists($uid) {
return $this->config->getUserValue($uid, 'core', 'deleted') === 'true';
}

public function getHome(string $uid): string|false {
return $this->config->getUserValue($uid, 'core', 'deleted.home-path') ?: false;
}

public function getUsers($search = '', $limit = null, $offset = null) {
return $this->config->getUsersForUserValue('core', 'deleted', 'true');
}

/**
* Unmark a user as deleted.
* This typically the case if the user deletion failed in the backend but before the backend deleted the user,
* meaning the user still exists so we unmark them as it still can be accessed (and deleted) normally.
*/
public function unmarkUser(string $userId): void {
$this->config->deleteUserValue($userId, 'core', 'deleted');
$this->config->deleteUserValue($userId, 'core', 'deleted.home-path');
}

}
Loading
Loading