From fe1be72702e0cafeba12f48ede2320ebadf4878a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcel=20M=C3=BCller?= Date: Mon, 12 Oct 2026 17:45:57 +0200 Subject: [PATCH] fix: Remove avatar and credentials only after user removal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Marcel Müller --- lib/composer/composer/autoload_classmap.php | 1 - lib/composer/composer/autoload_static.php | 1 - lib/private/Server.php | 3 -- .../Listeners/BeforeUserDeletedListener.php | 54 ------------------- lib/private/User/User.php | 4 ++ tests/lib/User/UserTest.php | 54 +++++++++++++++++++ 6 files changed, 58 insertions(+), 59 deletions(-) delete mode 100644 lib/private/User/Listeners/BeforeUserDeletedListener.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index aaf5602ba47e4..14087b82077ab 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -2461,7 +2461,6 @@ 'OC\\User\\DisplayNameCache' => $baseDir . '/lib/private/User/DisplayNameCache.php', 'OC\\User\\LastInteractiveLogin' => $baseDir . '/lib/private/User/LastInteractiveLogin.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', 'OC\\User\\Listeners\\UserLoggedInWithCookieListener' => $baseDir . '/lib/private/User/Listeners/UserLoggedInWithCookieListener.php', 'OC\\User\\LoginException' => $baseDir . '/lib/private/User/LoginException.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index d07e0aaa6b3bd..cef42516a207d 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -2502,7 +2502,6 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\User\\DisplayNameCache' => __DIR__ . '/../../..' . '/lib/private/User/DisplayNameCache.php', 'OC\\User\\LastInteractiveLogin' => __DIR__ . '/../../..' . '/lib/private/User/LastInteractiveLogin.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', 'OC\\User\\Listeners\\UserLoggedInWithCookieListener' => __DIR__ . '/../../..' . '/lib/private/User/Listeners/UserLoggedInWithCookieListener.php', 'OC\\User\\LoginException' => __DIR__ . '/../../..' . '/lib/private/User/LoginException.php', diff --git a/lib/private/Server.php b/lib/private/Server.php index 439974df99762..1e0109848b7de 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -154,7 +154,6 @@ use OC\Translation\TranslationManager; use OC\User\AvailabilityCoordinator; use OC\User\DisplayNameCache; -use OC\User\Listeners\BeforeUserDeletedListener; use OC\User\Listeners\UserChangedListener; use OC\User\Listeners\UserLoggedInWithCookieListener; use OC\User\Session; @@ -290,7 +289,6 @@ use OCP\Talk\IBroker; use OCP\Teams\ITeamManager; use OCP\Translation\ITranslationManager; -use OCP\User\Events\BeforeUserDeletedEvent; use OCP\User\Events\UserChangedEvent; use OCP\User\Events\UserDeletedEvent; use OCP\User\Events\UserLoggedInEvent; @@ -1128,7 +1126,6 @@ private function connectDispatcher(): void { $eventDispatcher->addServiceListener(UserLoggedInEvent::class, Store::class); $eventDispatcher->addServiceListener(UserLoggedInWithCookieEvent::class, Store::class); $eventDispatcher->addServiceListener(UserChangedEvent::class, UserChangedListener::class); - $eventDispatcher->addServiceListener(BeforeUserDeletedEvent::class, BeforeUserDeletedListener::class); $eventDispatcher->addServiceListener(UserDeletedEvent::class, SubAdmin::class); $eventDispatcher->addServiceListener(GroupDeletedEvent::class, SubAdmin::class); $eventDispatcher->addServiceListener(BeforeTemplateRenderedEvent::class, LoadViewerListener::class); diff --git a/lib/private/User/Listeners/BeforeUserDeletedListener.php b/lib/private/User/Listeners/BeforeUserDeletedListener.php deleted file mode 100644 index 163ec2bb2cdc6..0000000000000 --- a/lib/private/User/Listeners/BeforeUserDeletedListener.php +++ /dev/null @@ -1,54 +0,0 @@ - - */ -class BeforeUserDeletedListener implements IEventListener { - public function __construct( - private LoggerInterface $logger, - private IAvatarManager $avatarManager, - private ICredentialsManager $credentialsManager, - ) { - } - - #[\Override] - public function handle(Event $event): void { - if (!($event instanceof BeforeUserDeletedEvent)) { - return; - } - - $user = $event->getUser(); - - // Delete avatar on user deletion - try { - $avatar = $this->avatarManager->getAvatar($user->getUID()); - $avatar->remove(true); - } catch (NotFoundException $e) { - // no avatar to remove - } catch (\Exception $e) { - // Ignore exceptions - $this->logger->info('Could not cleanup avatar of ' . $user->getUID(), [ - 'exception' => $e, - ]); - } - // Delete storages credentials on user deletion - $this->credentialsManager->erase($user->getUID()); - } -} diff --git a/lib/private/User/User.php b/lib/private/User/User.php index c6e210779e3f4..e0afa46c30bd8 100644 --- a/lib/private/User/User.php +++ b/lib/private/User/User.php @@ -28,6 +28,7 @@ use OCP\IUser; use OCP\IUserBackend; use OCP\Notification\IManager as INotificationManager; +use OCP\Security\ICredentialsManager; use OCP\Server; use OCP\Support\Subscription\IAssertion; use OCP\User\Backend\IGetHomeBackend; @@ -292,6 +293,9 @@ public function delete(): bool { $avatarManager = Server::get(AvatarManager::class); $avatarManager->deleteUserAvatar($this->uid); + $credentialsManager = Server::get(ICredentialsManager::class); + $credentialsManager->erase($this->uid); + $notificationManager = Server::get(INotificationManager::class); $notification = $notificationManager->createNotification(); $notification->setUser($this->uid); diff --git a/tests/lib/User/UserTest.php b/tests/lib/User/UserTest.php index caaf3fd2311a4..fea25aa1ad34f 100644 --- a/tests/lib/User/UserTest.php +++ b/tests/lib/User/UserTest.php @@ -9,6 +9,7 @@ namespace Test\User; use OC\AllConfig; +use OC\Avatar\AvatarManager; use OC\Files\Mount\ObjectHomeMountProvider; use OC\User\Database; use OC\User\User; @@ -22,8 +23,10 @@ use OCP\IConfig; use OCP\IURLGenerator; use OCP\IUser; +use OCP\IUserManager; use OCP\Notification\IManager as INotificationManager; use OCP\Notification\INotification; +use OCP\Security\ICredentialsManager; use OCP\Server; use OCP\User\Events\BeforePasswordUpdatedEvent; use OCP\User\Events\BeforeUserDeletedEvent; @@ -34,6 +37,7 @@ use PHPUnit\Framework\Attributes\Group; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; +use Test\Util\User\Dummy; #[Group(name: 'DB')] class UserTest extends TestCase { @@ -454,6 +458,8 @@ function (Event $event) use (&$hooksCalled): void { $commentsManager = $this->createMock(ICommentsManager::class); $notificationManager = $this->createMock(INotificationManager::class); + $avatarManager = $this->createMock(AvatarManager::class); + $credentialsManager = $this->createMock(ICredentialsManager::class); if ($result) { $userConfig->expects($this->atLeastOnce()) @@ -478,6 +484,13 @@ function (Event $event) use (&$hooksCalled): void { $notificationManager->expects($this->once()) ->method('markProcessed') ->with($notification); + + $avatarManager->expects($this->once()) + ->method('deleteUserAvatar') + ->with('foo'); + $credentialsManager->expects($this->once()) + ->method('erase') + ->with('foo'); } else { $userConfig->expects($this->never()) ->method('deleteAllUserConfig'); @@ -491,9 +504,16 @@ function (Event $event) use (&$hooksCalled): void { ->method('createNotification'); $notificationManager->expects($this->never()) ->method('markProcessed'); + + $avatarManager->expects($this->never()) + ->method('deleteUserAvatar'); + $credentialsManager->expects($this->never()) + ->method('erase'); } $this->overwriteService(INotificationManager::class, $notificationManager); + $this->overwriteService(AvatarManager::class, $avatarManager); + $this->overwriteService(ICredentialsManager::class, $credentialsManager); $this->overwriteService(ICommentsManager::class, $commentsManager); $this->assertSame($result, $user->delete()); @@ -501,10 +521,44 @@ function (Event $event) use (&$hooksCalled): void { $this->restoreService(AllConfig::class); $this->restoreService(ICommentsManager::class); $this->restoreService(INotificationManager::class); + $this->restoreService(AvatarManager::class); + $this->restoreService(ICredentialsManager::class); $this->assertEquals($expectedHooks, $hooksCalled); } + public function testDeleteRefusedByBackendKeepsAvatarAndCredentials(): void { + $backend = new class extends Dummy { + #[\Override] + public function deleteUser($uid): bool { + return false; + } + }; + $uid = $this->getUniqueID('user'); + $backend->createUser($uid, 'password'); + + $userManager = Server::get(IUserManager::class); + $userManager->registerBackend($backend); + $avatarManager = Server::get(AvatarManager::class); + $credentialsManager = Server::get(ICredentialsManager::class); + + try { + $user = $userManager->get($uid); + $avatarManager->getAvatar($uid)->set(file_get_contents(\OC::$SERVERROOT . '/tests/data/testavatar.png')); + $credentialsManager->store($uid, 'test', ['password' => 'secret']); + + $this->assertFalse($user->delete()); + + $this->assertTrue($avatarManager->getAvatar($uid)->exists()); + $this->assertSame(['password' => 'secret'], $credentialsManager->retrieve($uid, 'test')); + } finally { + $avatarManager->deleteUserAvatar($uid); + $credentialsManager->erase($uid); + Server::get(IUserConfig::class)->deleteAllUserConfig($uid); + $userManager->removeBackend($backend); + } + } + public function testDeleteRecoverState() { $backend = $this->createMock(\Test\Util\User\Dummy::class); $backend->method('getBackendName')->willReturn('foo');