From 229deb04971bec89495f4765073462875b935eac Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Tue, 10 Sep 2024 22:50:16 +0200 Subject: [PATCH 1/3] 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 --- lib/composer/composer/autoload_classmap.php | 3 + lib/composer/composer/autoload_static.php | 3 + .../UserDeletedFilesCleanupListener.php | 24 +-- lib/private/Repair.php | 2 + .../AddCleanupDeletedUsersBackgroundJob.php | 30 ++++ lib/private/Setup.php | 2 + lib/private/User/Backend.php | 4 +- .../BackgroundJobs/CleanupDeletedUsers.php | 55 +++++++ lib/private/User/FailedUsersBackend.php | 46 ++++++ lib/private/User/User.php | 147 +++++++++++------- tests/lib/User/UserTest.php | 81 ++++++++-- 11 files changed, 319 insertions(+), 78 deletions(-) create mode 100644 lib/private/Repair/AddCleanupDeletedUsersBackgroundJob.php create mode 100644 lib/private/User/BackgroundJobs/CleanupDeletedUsers.php create mode 100644 lib/private/User/FailedUsersBackend.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 2b7b5b6e652b1..f695a8467c3d3 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -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', @@ -1804,8 +1805,10 @@ '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\\FailedUsersBackend' => $baseDir . '/lib/private/User/FailedUsersBackend.php', 'OC\\User\\LazyUser' => $baseDir . '/lib/private/User/LazyUser.php', 'OC\\User\\Listeners\\BeforeUserDeletedListener' => $baseDir . '/lib/private/User/Listeners/BeforeUserDeletedListener.php', 'OC\\User\\Listeners\\UserChangedListener' => $baseDir . '/lib/private/User/Listeners/UserChangedListener.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 2dd883a794b19..10058ec3a32cb 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -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', @@ -1837,8 +1838,10 @@ 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\\FailedUsersBackend' => __DIR__ . '/../../..' . '/lib/private/User/FailedUsersBackend.php', 'OC\\User\\LazyUser' => __DIR__ . '/../../..' . '/lib/private/User/LazyUser.php', 'OC\\User\\Listeners\\BeforeUserDeletedListener' => __DIR__ . '/../../..' . '/lib/private/User/Listeners/BeforeUserDeletedListener.php', 'OC\\User\\Listeners\\UserChangedListener' => __DIR__ . '/../../..' . '/lib/private/User/Listeners/UserChangedListener.php', diff --git a/lib/private/Authentication/Listeners/UserDeletedFilesCleanupListener.php b/lib/private/Authentication/Listeners/UserDeletedFilesCleanupListener.php index 5e657be0763b9..888ac30d3214c 100644 --- a/lib/private/Authentication/Listeners/UserDeletedFilesCleanupListener.php +++ b/lib/private/Authentication/Listeners/UserDeletedFilesCleanupListener.php @@ -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 */ 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("User has no home storage"); @@ -67,12 +71,14 @@ public function handle(Event $event): void { $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 { diff --git a/lib/private/Repair.php b/lib/private/Repair.php index fe1925bddfe51..80fc78661359f 100644 --- a/lib/private/Repair.php +++ b/lib/private/Repair.php @@ -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; @@ -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), ]; } diff --git a/lib/private/Repair/AddCleanupDeletedUsersBackgroundJob.php b/lib/private/Repair/AddCleanupDeletedUsersBackgroundJob.php new file mode 100644 index 0000000000000..9713d8595e774 --- /dev/null +++ b/lib/private/Repair/AddCleanupDeletedUsersBackgroundJob.php @@ -0,0 +1,30 @@ +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 3cd3716c19598..48cf3fa8e07bd 100644 --- a/lib/private/Setup.php +++ b/lib/private/Setup.php @@ -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; @@ -463,6 +464,7 @@ public static function installBackgroundJobs() { $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 b68e4c2e541ee..061ad11516145 100644 --- a/lib/private/User/Backend.php +++ b/lib/private/User/Backend.php @@ -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; } diff --git a/lib/private/User/BackgroundJobs/CleanupDeletedUsers.php b/lib/private/User/BackgroundJobs/CleanupDeletedUsers.php new file mode 100644 index 0000000000000..46ca2175c168d --- /dev/null +++ b/lib/private/User/BackgroundJobs/CleanupDeletedUsers.php @@ -0,0 +1,55 @@ +setInterval(3600); + } + + protected function run($argument): void { + $backend = new FailedUsersBackend($this->config); + $users = $backend->getUsers(); + + if (empty($users)) { + $this->logger->debug('No failed deleted users found.'); + return; + } + + foreach ($users as $userId) { + try { + $user = new User( + $userId, + $backend, + \OCP\Server::get(IEventDispatcher::class), + config: $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/FailedUsersBackend.php b/lib/private/User/FailedUsersBackend.php new file mode 100644 index 0000000000000..934ceea17dabd --- /dev/null +++ b/lib/private/User/FailedUsersBackend.php @@ -0,0 +1,46 @@ +config->getUserValue($uid, 'core', 'deleted') === 'true'; + } + + public function getHome(string $uid): string|false { + return $this->config->getUserValue($uid, 'core', 'deleted.backup-home') ?: false; + } + + public function getUsers($search = '', $limit = null, $offset = null) { + return $this->config->getUsersForUserValue('core', 'deleted', 'true'); + } + +} diff --git a/lib/private/User/User.php b/lib/private/User/User.php index 580c590e6eb54..c803615dc3b51 100644 --- a/lib/private/User/User.php +++ b/lib/private/User/User.php @@ -40,15 +40,19 @@ use OC\Hooks\Emitter; use OC_Helper; use OCP\Accounts\IAccountManager; +use OCP\Comments\ICommentsManager; use OCP\EventDispatcher\IEventDispatcher; 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; use OCP\IUserBackend; +use OCP\Notification\IManager as INotificationManager; use OCP\User\Backend\IGetHomeBackend; use OCP\User\Backend\IProvideAvatarBackend; use OCP\User\Backend\IProvideEnabledStateBackend; @@ -61,6 +65,8 @@ use OCP\User\Events\UserDeletedEvent; use OCP\User\GetQuotaEvent; use OCP\UserInterface; +use Psr\Log\LoggerInterface; + use function json_decode; use function json_encode; @@ -69,6 +75,7 @@ class User implements IUser { /** @var IAccountManager */ protected $accountManager; + /** @var string */ private $uid; @@ -84,7 +91,7 @@ class User implements IUser { /** @var bool|null */ private $enabled; - /** @var Emitter|Manager */ + /** @var Emitter|Manager|null */ private $emitter; /** @var string */ @@ -93,28 +100,29 @@ 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) { + /** @var IConfig */ + private $config; + + public function __construct( + string $uid, + ?UserInterface $backend, + IEventDispatcher $dispatcher, + $emitter = null, + ?IConfig $config = null, + $urlGenerator = null, + ) { $this->uid = $uid; $this->backend = $backend; - $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->emitter = $emitter; + $this->config = $config ?? \OCP\Server::get(IConfig::class); + $this->urlGenerator = $urlGenerator ?? \OCP\Server::get(IURLGenerator::class); } /** @@ -240,9 +248,9 @@ private function ensureAccountManager() { */ public function getLastLogin() { if ($this->lastLogin === null) { - $this->lastLogin = (int) $this->config->getUserValue($this->uid, 'login', 'lastLogin', 0); + $this->lastLogin = (int)$this->config->getUserValue($this->uid, 'login', 'lastLogin', 0); } - return (int) $this->lastLogin; + return (int)$this->lastLogin; } /** @@ -268,50 +276,85 @@ public function updateLastLoginTimestamp() { * @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)); - $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)); - } - } - // Delete the user's keys in preferences - \OC::$server->getConfig()->deleteAllUserValues($this->uid); - \OC::$server->getCommentsManager()->deleteReferencesOfActor('users', $this->uid); - \OC::$server->getCommentsManager()->deleteReadMarksFromUser($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.backup-home', $this->getHome()); - /** @var AvatarManager $avatarManager */ - $avatarManager = \OCP\Server::get(AvatarManager::class); - $avatarManager->deleteUserAvatar($this->uid); + // Try to delete the user on the backend + $result = $this->backend->deleteUser($this->uid); + 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; + } - $notification = \OC::$server->getNotificationManager()->createNotification(); - $notification->setUser($this->uid); - \OC::$server->getNotificationManager()->markProcessed($notification); + // 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)); + } + } - /** @var AccountManager $accountManager */ - $accountManager = \OCP\Server::get(AccountManager::class); - $accountManager->deleteUser($this); + $commentsManager = \OCP\Server::get(ICommentsManager::class); + $commentsManager->deleteReferencesOfActor('users', $this->uid); + $commentsManager->deleteReadMarksFromUser($this); + + $avatarManager = \OCP\Server::get(AvatarManager::class); + $avatarManager->deleteUserAvatar($this->uid); + + $notificationManager = \OCP\Server::get(INotificationManager::class); + $notification = $notificationManager->createNotification(); + $notification->setUser($this->uid); + $notificationManager->markProcessed($notification); + + $accountManager = \OCP\Server::get(AccountManager::class); + $accountManager->deleteUser($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.backup-home', $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; + } - 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)); + if ($this->emitter) { + /** @deprecated 21.0.0 use UserDeletedEvent event with the IEventDispatcher instead */ + $this->emitter->emit('\OC\User', 'postDelete', [$this]); } - return !($result === false); + $this->dispatcher->dispatchTyped(new UserDeletedEvent($this)); + + // Finally we can unset the delete flag and all other states + $this->config->deleteAllUserValues($this->uid); + + return true; } /** @@ -354,10 +397,8 @@ public function getHome() { /** @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; @@ -534,7 +575,7 @@ public function setQuota($quota) { if ($quota !== 'none' and $quota !== 'default') { $bytesQuota = OC_Helper::computerFileSize($quota); if ($bytesQuota === false) { - throw new InvalidArgumentException('Failed to set quota to invalid value '.$quota); + throw new InvalidArgumentException('Failed to set quota to invalid value ' . $quota); } $quota = OC_Helper::humanFileSize($bytesQuota); } diff --git a/tests/lib/User/UserTest.php b/tests/lib/User/UserTest.php index 806cb094066cf..965dd65aeabbe 100644 --- a/tests/lib/User/UserTest.php +++ b/tests/lib/User/UserTest.php @@ -514,14 +514,25 @@ public function testDeleteHooks($result, $expectedHooks) { $test = $this; /** - * @var Backend | MockObject $backend + * @var UserInterface&MockObject $backend */ $backend = $this->createMock(\Test\Util\User\Dummy::class); $backend->expects($this->once()) ->method('deleteUser') ->willReturn($result); + + $config = $this->createMock(IConfig::class); + $config->method('getSystemValue') + ->willReturnArgument(1); + $config->method('getSystemValueString') + ->willReturnArgument(1); + $config->method('getSystemValueBool') + ->willReturnArgument(1); + $config->method('getSystemValueInt') + ->willReturnArgument(1); + $emitter = new PublicEmitter(); - $user = new User('foo', $backend, $this->dispatcher, $emitter); + $user = new User('foo', $backend, $this->dispatcher, $emitter, $config); /** * @param User $user @@ -534,21 +545,11 @@ public function testDeleteHooks($result, $expectedHooks) { $emitter->listen('\OC\User', 'preDelete', $hook); $emitter->listen('\OC\User', 'postDelete', $hook); - $config = $this->createMock(IConfig::class); $commentsManager = $this->createMock(ICommentsManager::class); $notificationManager = $this->createMock(INotificationManager::class); - $config->method('getSystemValue') - ->willReturnArgument(1); - $config->method('getSystemValueString') - ->willReturnArgument(1); - $config->method('getSystemValueBool') - ->willReturnArgument(1); - $config->method('getSystemValueInt') - ->willReturnArgument(1); - if ($result) { - $config->expects($this->once()) + $config->expects($this->atLeastOnce()) ->method('deleteAllUserValues') ->with('foo'); @@ -587,7 +588,6 @@ public function testDeleteHooks($result, $expectedHooks) { $this->overwriteService(\OCP\Notification\IManager::class, $notificationManager); $this->overwriteService(\OCP\Comments\ICommentsManager::class, $commentsManager); - $this->overwriteService(AllConfig::class, $config); $this->assertSame($result, $user->delete()); @@ -598,6 +598,59 @@ public function testDeleteHooks($result, $expectedHooks) { $this->assertEquals($expectedHooks, $hooksCalled); } + public function testDeleteRecoverState() { + $backend = $this->createMock(\Test\Util\User\Dummy::class); + $backend->expects($this->once()) + ->method('deleteUser') + ->willReturn(true); + + $config = $this->createMock(IConfig::class); + $config->method('getSystemValue') + ->willReturnArgument(1); + $config->method('getSystemValueString') + ->willReturnArgument(1); + $config->method('getSystemValueBool') + ->willReturnArgument(1); + $config->method('getSystemValueInt') + ->willReturnArgument(1); + + $userConfig = []; + $config->expects(self::atLeast(2)) + ->method('setUserValue') + ->willReturnCallback(function () { + $userConfig[] = func_get_args(); + }); + + $commentsManager = $this->createMock(ICommentsManager::class); + $commentsManager->expects($this->once()) + ->method('deleteReferencesOfActor') + ->willThrowException(new \Error('Test exception')); + + $this->overwriteService(\OCP\Comments\ICommentsManager::class, $commentsManager); + $this->expectException(\Error::class); + + $user = $this->getMockBuilder(User::class) + ->onlyMethods(['getHome']) + ->setConstructorArgs(['foo', $backend, $this->dispatcher, null, $config]) + ->getMock(); + + $user->expects(self::atLeastOnce()) + ->method('getHome') + ->willReturn('/home/path'); + + $user->delete(); + + $this->assertEqualsCanonicalizing( + [ + ['foo', 'core', 'deleted', 'true', null], + ['foo', 'core', 'deleted.backup-home', '/home/path', null], + ], + $userConfig, + ); + + $this->restoreService(\OCP\Comments\ICommentsManager::class); + } + public function dataGetCloudId(): array { return [ ['https://localhost:8888/nextcloud', 'foo@localhost:8888/nextcloud'], From 1b76925aed75d94481c1230c6c32b7f7ff57e18e Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Wed, 11 Sep 2024 19:33:53 +0200 Subject: [PATCH 2/3] fix: Skip users that still exist in backend Signed-off-by: Ferdinand Thiessen --- lib/composer/composer/autoload_classmap.php | 2 +- lib/composer/composer/autoload_static.php | 2 +- .../User/BackgroundJobs/CleanupDeletedUsers.php | 17 +++++++++++++---- ...end.php => PartiallyDeletedUsersBackend.php} | 14 ++++++++++++-- lib/private/User/User.php | 6 +++--- 5 files changed, 30 insertions(+), 11 deletions(-) rename lib/private/User/{FailedUsersBackend.php => PartiallyDeletedUsersBackend.php} (63%) diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index f695a8467c3d3..972bd36e31a74 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1808,7 +1808,6 @@ '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\\FailedUsersBackend' => $baseDir . '/lib/private/User/FailedUsersBackend.php', 'OC\\User\\LazyUser' => $baseDir . '/lib/private/User/LazyUser.php', 'OC\\User\\Listeners\\BeforeUserDeletedListener' => $baseDir . '/lib/private/User/Listeners/BeforeUserDeletedListener.php', 'OC\\User\\Listeners\\UserChangedListener' => $baseDir . '/lib/private/User/Listeners/UserChangedListener.php', @@ -1816,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', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 10058ec3a32cb..806cb8d30a542 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1841,7 +1841,6 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 '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\\FailedUsersBackend' => __DIR__ . '/../../..' . '/lib/private/User/FailedUsersBackend.php', 'OC\\User\\LazyUser' => __DIR__ . '/../../..' . '/lib/private/User/LazyUser.php', 'OC\\User\\Listeners\\BeforeUserDeletedListener' => __DIR__ . '/../../..' . '/lib/private/User/Listeners/BeforeUserDeletedListener.php', 'OC\\User\\Listeners\\UserChangedListener' => __DIR__ . '/../../..' . '/lib/private/User/Listeners/UserChangedListener.php', @@ -1849,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', diff --git a/lib/private/User/BackgroundJobs/CleanupDeletedUsers.php b/lib/private/User/BackgroundJobs/CleanupDeletedUsers.php index 46ca2175c168d..3c1b73637acdb 100644 --- a/lib/private/User/BackgroundJobs/CleanupDeletedUsers.php +++ b/lib/private/User/BackgroundJobs/CleanupDeletedUsers.php @@ -8,10 +8,11 @@ */ namespace OC\User\BackgroundJobs; -use OC\User\FailedUsersBackend; 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; @@ -25,11 +26,12 @@ public function __construct( private LoggerInterface $logger, ) { parent::__construct($time); - $this->setInterval(3600); + $this->setTimeSensitivity(IJob::TIME_INSENSITIVE); + $this->setInterval(24 * 3600); } protected function run($argument): void { - $backend = new FailedUsersBackend($this->config); + $backend = new PartiallyDeletedUsersBackend($this->config); $users = $backend->getUsers(); if (empty($users)) { @@ -38,12 +40,19 @@ protected function run($argument): void { } 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), - config: $this->config, + $this->userManager, + $this->config, ); $user->delete(); $this->logger->info('Cleaned up deleted user {userId}', ['userId' => $userId]); diff --git a/lib/private/User/FailedUsersBackend.php b/lib/private/User/PartiallyDeletedUsersBackend.php similarity index 63% rename from lib/private/User/FailedUsersBackend.php rename to lib/private/User/PartiallyDeletedUsersBackend.php index 934ceea17dabd..298ddaff6c6bc 100644 --- a/lib/private/User/FailedUsersBackend.php +++ b/lib/private/User/PartiallyDeletedUsersBackend.php @@ -15,7 +15,7 @@ * 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 FailedUsersBackend extends Backend implements IGetHomeBackend, IUserBackend { +class PartiallyDeletedUsersBackend extends Backend implements IGetHomeBackend, IUserBackend { public function __construct( private IConfig $config, @@ -36,11 +36,21 @@ public function userExists($uid) { } public function getHome(string $uid): string|false { - return $this->config->getUserValue($uid, 'core', 'deleted.backup-home') ?: 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 c803615dc3b51..3b0172d915164 100644 --- a/lib/private/User/User.php +++ b/lib/private/User/User.php @@ -291,7 +291,7 @@ public function delete() { // 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.backup-home', $this->getHome()); + $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); @@ -337,7 +337,7 @@ public function delete() { $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.backup-home', $this->getHome()); + $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) { @@ -345,7 +345,7 @@ public function delete() { throw $e; } - if ($this->emitter) { + if ($this->emitter !== null) { /** @deprecated 21.0.0 use UserDeletedEvent event with the IEventDispatcher instead */ $this->emitter->emit('\OC\User', 'postDelete', [$this]); } From c42ec8d0d26276afd214e5891a422c4c11e493aa Mon Sep 17 00:00:00 2001 From: provokateurin Date: Tue, 8 Oct 2024 11:12:01 +0200 Subject: [PATCH 3/3] fix(UserTrait): Fix backend initialization Signed-off-by: provokateurin --- .../Listeners/UserDeletedFilesCleanupListener.php | 1 + tests/lib/Traits/UserTrait.php | 3 +++ 2 files changed, 4 insertions(+) diff --git a/lib/private/Authentication/Listeners/UserDeletedFilesCleanupListener.php b/lib/private/Authentication/Listeners/UserDeletedFilesCleanupListener.php index 888ac30d3214c..bd639a3a37c8b 100644 --- a/lib/private/Authentication/Listeners/UserDeletedFilesCleanupListener.php +++ b/lib/private/Authentication/Listeners/UserDeletedFilesCleanupListener.php @@ -35,6 +35,7 @@ use OCP\User\Events\UserDeletedEvent; use Psr\Log\LoggerInterface; +/** @template-implements IEventListener */ class UserDeletedFilesCleanupListener implements IEventListener { /** @var array */ private $homeStorageCache = []; diff --git a/tests/lib/Traits/UserTrait.php b/tests/lib/Traits/UserTrait.php index 3f7cfe419db45..d2c1444c94377 100644 --- a/tests/lib/Traits/UserTrait.php +++ b/tests/lib/Traits/UserTrait.php @@ -9,13 +9,16 @@ namespace Test\Traits; use OC\User\User; +use OCP\EventDispatcher\IEventDispatcher; use OCP\IUser; +use OCP\Server; class DummyUser extends User { private string $uid; public function __construct(string $uid) { $this->uid = $uid; + parent::__construct($uid, null, Server::get(IEventDispatcher::class)); } public function getUID(): string {