Merge pull request #47896 from nextcloud/fix/resiliant-user-removal

fix: Make user removal more resilient

8 files changed, 255 insertions, 71 deletions

diff --git a/lib/private/Authentication/Listeners/UserDeletedFilesCleanupListener.php b/lib/private/Authentication/Listeners/UserDeletedFilesCleanupListener.php
index 8523fb6abc7..a619021d192 100644
--- a/lib/private/Authentication/Listeners/UserDeletedFilesCleanupListener.php
+++ b/lib/private/Authentication/Listeners/UserDeletedFilesCleanupListener.php
@@ -16,27 +16,31 @@ use OCP\Files\Config\IMountProviderCollection;
 use OCP\Files\Storage\IStorage;
 use OCP\User\Events\BeforeUserDeletedEvent;
 use OCP\User\Events\UserDeletedEvent;
+use Psr\Log\LoggerInterface;
 
 /** @template-implements IEventListener<BeforeUserDeletedEvent|UserDeletedEvent> */
 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();
+
 		// 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('Account has no home storage');
@@ -51,12 +55,14 @@ class UserDeletedFilesCleanupListener implements IEventListener {
 			$this->homeStorageCache[$event->getUser()->getUID()] = $storage;
 		}
 		if ($event instanceof UserDeletedEvent) {
-			if (!isset($this->homeStorageCache[$event->getUser()->getUID()])) {
+			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 {
diff --git a/lib/private/Repair.php b/lib/private/Repair.php
index 42d3cfb4910..b1a824ba5e3 100644
--- a/lib/private/Repair.php
+++ b/lib/private/Repair.php
@@ -11,6 +11,7 @@ use OC\DB\Connection;
 use OC\DB\ConnectionAdapter;
 use OC\Repair\AddAppConfigLazyMigration;
 use OC\Repair\AddBruteForceCleanupJob;
+use OC\Repair\AddCleanupDeletedUsersBackgroundJob;
 use OC\Repair\AddCleanupUpdaterBackupsJob;
 use OC\Repair\AddMetadataGenerationJob;
 use OC\Repair\AddRemoveOldTasksBackgroundJob;
@@ -192,6 +193,7 @@ class Repair implements IOutput {
 			\OCP\Server::get(AddAppConfigLazyMigration::class),
 			\OCP\Server::get(RepairLogoDimension::class),
 			\OCP\Server::get(RemoveLegacyDatadirFile::class),
+			\OCP\Server::get(AddCleanupDeletedUsersBackgroundJob::class),
 		];
 	}
 
diff --git a/lib/private/Repair/AddCleanupDeletedUsersBackgroundJob.php b/lib/private/Repair/AddCleanupDeletedUsersBackgroundJob.php
new file mode 100644
index 00000000000..9713d8595e7
--- /dev/null
+++ b/lib/private/Repair/AddCleanupDeletedUsersBackgroundJob.php
@@ -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);
+	}
+}
diff --git a/lib/private/Setup.php b/lib/private/Setup.php
index e64f6806b87..f286c3698cf 100644
--- a/lib/private/Setup.php
+++ b/lib/private/Setup.php
@@ -17,6 +17,7 @@ use OC\Authentication\Token\TokenCleanupJob;
 use OC\Log\Rotate;
 use OC\Preview\BackgroundCleanupJob;
 use OC\TextProcessing\RemoveOldTasksBackgroundJob;
+use OC\User\BackgroundJobs\CleanupDeletedUsers;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\BackgroundJob\IJobList;
 use OCP\Defaults;
@@ -415,6 +416,7 @@ class Setup {
 		$jobList->add(Rotate::class);
 		$jobList->add(BackgroundCleanupJob::class);
 		$jobList->add(RemoveOldTasksBackgroundJob::class);
+		$jobList->add(CleanupDeletedUsers::class);
 	}
 
 	/**
diff --git a/lib/private/User/Backend.php b/lib/private/User/Backend.php
index 98b291e5dee..9b6a9a890ef 100644
--- a/lib/private/User/Backend.php
+++ b/lib/private/User/Backend.php
@@ -107,9 +107,9 @@ abstract class Backend implements UserInterface {
 	/**
 	 * 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;
 	}
 
diff --git a/lib/private/User/BackgroundJobs/CleanupDeletedUsers.php b/lib/private/User/BackgroundJobs/CleanupDeletedUsers.php
new file mode 100644
index 00000000000..3c1b73637ac
--- /dev/null
+++ b/lib/private/User/BackgroundJobs/CleanupDeletedUsers.php
@@ -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]);
+			}
+		}
+	}
+}
diff --git a/lib/private/User/PartiallyDeletedUsersBackend.php b/lib/private/User/PartiallyDeletedUsersBackend.php
new file mode 100644
index 00000000000..298ddaff6c6
--- /dev/null
+++ b/lib/private/User/PartiallyDeletedUsersBackend.php
@@ -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');
+	}
+
+}
diff --git a/lib/private/User/User.php b/lib/private/User/User.php
index f2c38825338..6e1085b4fe3 100644
--- a/lib/private/User/User.php
+++ b/lib/private/User/User.php
@@ -19,6 +19,8 @@ use OCP\Group\Events\BeforeUserRemovedEvent;
 use OCP\Group\Events\UserRemovedEvent;
 use OCP\IAvatarManager;
 use OCP\IConfig;
+use OCP\IDBConnection;
+use OCP\IGroupManager;
 use OCP\IImage;
 use OCP\IURLGenerator;
 use OCP\IUser;
@@ -37,30 +39,27 @@ use OCP\User\Events\UserChangedEvent;
 use OCP\User\Events\UserDeletedEvent;
 use OCP\User\GetQuotaEvent;
 use OCP\UserInterface;
+use Psr\Log\LoggerInterface;
+
 use function json_decode;
 use function json_encode;
 
 class User implements IUser {
 	private const CONFIG_KEY_MANAGERS = 'manager';
 
+	private IConfig $config;
+	private IURLGenerator $urlGenerator;
+
 	/** @var IAccountManager */
 	protected $accountManager;
-	/** @var string */
-	private $uid;
 
 	/** @var string|null */
 	private $displayName;
 
-	/** @var UserInterface|null */
-	private $backend;
-
-	/** @var IEventDispatcher */
-	private $dispatcher;
-
 	/** @var bool|null */
 	private $enabled;
 
-	/** @var Emitter|Manager */
+	/** @var Emitter|Manager|null */
 	private $emitter;
 
 	/** @var string */
@@ -69,28 +68,20 @@ class User implements IUser {
 	/** @var int|null */
 	private $lastLogin;
 
-	/** @var \OCP\IConfig */
-	private $config;
-
 	/** @var IAvatarManager */
 	private $avatarManager;
 
-	/** @var IURLGenerator */
-	private $urlGenerator;
-
-	public function __construct(string $uid, ?UserInterface $backend, IEventDispatcher $dispatcher, $emitter = null, ?IConfig $config = null, $urlGenerator = null) {
-		$this->uid = $uid;
-		$this->backend = $backend;
+	public function __construct(
+		private string $uid,
+		private ?UserInterface $backend,
+		private IEventDispatcher $dispatcher,
+		$emitter = null,
+		?IConfig $config = null,
+		$urlGenerator = null,
+	) {
 		$this->emitter = $emitter;
-		if (is_null($config)) {
-			$config = \OC::$server->getConfig();
-		}
-		$this->config = $config;
-		$this->urlGenerator = $urlGenerator;
-		if (is_null($this->urlGenerator)) {
-			$this->urlGenerator = \OC::$server->getURLGenerator();
-		}
-		$this->dispatcher = $dispatcher;
+		$this->config = $config ?? \OCP\Server::get(IConfig::class);
+		$this->urlGenerator = $urlGenerator ?? \OCP\Server::get(IURLGenerator::class);
 	}
 
 	/**
@@ -244,50 +235,85 @@ class User implements IUser {
 	 * @return bool
 	 */
 	public function delete() {
+		if ($this->backend === null) {
+			\OCP\Server::get(LoggerInterface::class)->error('Cannot delete user: No backend set');
+			return false;
+		}
+
 		if ($this->emitter) {
 			/** @deprecated 21.0.0 use BeforeUserDeletedEvent event with the IEventDispatcher instead */
 			$this->emitter->emit('\OC\User', 'preDelete', [$this]);
 		}
 		$this->dispatcher->dispatchTyped(new BeforeUserDeletedEvent($this));
+
+		// Set delete flag on the user - this is needed to ensure that the user data is removed if there happen any exception in the backend
+		// because we can not restore the user meaning we could not rollback to any stable state otherwise.
+		$this->config->setUserValue($this->uid, 'core', 'deleted', 'true');
+		// We also need to backup the home path as this can not be reconstructed later if the original backend uses custom home paths
+		$this->config->setUserValue($this->uid, 'core', 'deleted.home-path', $this->getHome());
+
+		// Try to delete the user on the backend
 		$result = $this->backend->deleteUser($this->uid);
-		if ($result) {
-			// FIXME: Feels like an hack - suggestions?
-
-			$groupManager = \OC::$server->getGroupManager();
-			// We have to delete the user from all groups
-			foreach ($groupManager->getUserGroupIds($this) as $groupId) {
-				$group = $groupManager->get($groupId);
-				if ($group) {
-					$this->dispatcher->dispatchTyped(new BeforeUserRemovedEvent($group, $this));
-					$group->removeUser($this);
-					$this->dispatcher->dispatchTyped(new UserRemovedEvent($group, $this));
-				}
+		if ($result === false) {
+			// The deletion was aborted or something else happened, we are in a defined state, so remove the delete flag
+			$this->config->deleteUserValue($this->uid, 'core', 'deleted');
+			return false;
+		}
+
+		// We have to delete the user from all groups
+		$groupManager = \OCP\Server::get(IGroupManager::class);
+		foreach ($groupManager->getUserGroupIds($this) as $groupId) {
+			$group = $groupManager->get($groupId);
+			if ($group) {
+				$this->dispatcher->dispatchTyped(new BeforeUserRemovedEvent($group, $this));
+				$group->removeUser($this);
+				$this->dispatcher->dispatchTyped(new UserRemovedEvent($group, $this));
 			}
-			// Delete the user's keys in preferences
-			\OC::$server->getConfig()->deleteAllUserValues($this->uid);
+		}
 
-			\OC::$server->get(ICommentsManager::class)->deleteReferencesOfActor('users', $this->uid);
-			\OC::$server->get(ICommentsManager::class)->deleteReadMarksFromUser($this);
+		$commentsManager = \OCP\Server::get(ICommentsManager::class);
+		$commentsManager->deleteReferencesOfActor('users', $this->uid);
+		$commentsManager->deleteReadMarksFromUser($this);
 
-			/** @var AvatarManager $avatarManager */
-			$avatarManager = \OCP\Server::get(AvatarManager::class);
-			$avatarManager->deleteUserAvatar($this->uid);
+		$avatarManager = \OCP\Server::get(AvatarManager::class);
+		$avatarManager->deleteUserAvatar($this->uid);
 
-			$notification = \OC::$server->get(INotificationManager::class)->createNotification();
-			$notification->setUser($this->uid);
-			\OC::$server->get(INotificationManager::class)->markProcessed($notification);
+		$notificationManager = \OCP\Server::get(INotificationManager::class);
+		$notification = $notificationManager->createNotification();
+		$notification->setUser($this->uid);
+		$notificationManager->markProcessed($notification);
 
-			/** @var AccountManager $accountManager */
-			$accountManager = \OCP\Server::get(AccountManager::class);
-			$accountManager->deleteUser($this);
+		$accountManager = \OCP\Server::get(AccountManager::class);
+		$accountManager->deleteUser($this);
 
-			if ($this->emitter) {
-				/** @deprecated 21.0.0 use UserDeletedEvent event with the IEventDispatcher instead */
-				$this->emitter->emit('\OC\User', 'postDelete', [$this]);
-			}
-			$this->dispatcher->dispatchTyped(new UserDeletedEvent($this));
+		$database = \OCP\Server::get(IDBConnection::class);
+		try {
+			// We need to create a transaction to make sure we are in a defined state
+			// because if all user values are removed also the flag is gone, but if an exception happens (e.g. database lost connection on the set operation)
+			// exactly here we are in an undefined state as the data is still present but the user does not exist on the system anymore.
+			$database->beginTransaction();
+			// Remove all user settings
+			$this->config->deleteAllUserValues($this->uid);
+			// But again set flag that this user is about to be deleted
+			$this->config->setUserValue($this->uid, 'core', 'deleted', 'true');
+			$this->config->setUserValue($this->uid, 'core', 'deleted.home-path', $this->getHome());
+			// Commit the transaction so we are in a defined state: either the preferences are removed or an exception occurred but the delete flag is still present
+			$database->commit();
+		} catch (\Throwable $e) {
+			$database->rollback();
+			throw $e;
 		}
-		return !($result === false);
+
+		if ($this->emitter !== null) {
+			/** @deprecated 21.0.0 use UserDeletedEvent event with the IEventDispatcher instead */
+			$this->emitter->emit('\OC\User', 'postDelete', [$this]);
+		}
+		$this->dispatcher->dispatchTyped(new UserDeletedEvent($this));
+
+		// Finally we can unset the delete flag and all other states
+		$this->config->deleteAllUserValues($this->uid);
+
+		return true;
 	}
 
 	/**
@@ -344,10 +370,8 @@ class User implements IUser {
 			/** @psalm-suppress UndefinedInterfaceMethod Once we get rid of the legacy implementsActions, psalm won't complain anymore */
 			if (($this->backend instanceof IGetHomeBackend || $this->backend->implementsActions(Backend::GET_HOME)) && $home = $this->backend->getHome($this->uid)) {
 				$this->home = $home;
-			} elseif ($this->config) {
-				$this->home = $this->config->getSystemValueString('datadirectory', \OC::$SERVERROOT . '/data') . '/' . $this->uid;
 			} else {
-				$this->home = \OC::$SERVERROOT . '/data/' . $this->uid;
+				$this->home = $this->config->getSystemValueString('datadirectory', \OC::$SERVERROOT . '/data') . '/' . $this->uid;
 			}
 		}
 		return $this->home;