Merge pull request #65952 from nextcloud/backport/65948/stable34

[stable34] fix: Remove avatar and credentials only after user removal

Author: AndyScherzinger

lib/composer/composer/autoload_classmap.php

@@ -2328,7 +2328,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',

lib/composer/composer/autoload_static.php

@@ -2369,7 +2369,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',

lib/private/Server.php

@@ -155,7 +155,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\BeforeUserLoggedInEvent;
 use OCP\User\Events\BeforeUserLoggedInWithCookieEvent;
 use OCP\User\Events\BeforeUserLoggedOutEvent;
@@ -1358,7 +1356,6 @@ private function connectDispatcher(): void {
 		$eventDispatcher->addServiceListener(LoginFailed::class, LoginFailedListener::class);
 		$eventDispatcher->addServiceListener(PostLoginEvent::class, UserLoggedInListener::class);
 		$eventDispatcher->addServiceListener(UserChangedEvent::class, UserChangedListener::class);
-		$eventDispatcher->addServiceListener(BeforeUserDeletedEvent::class, BeforeUserDeletedListener::class);
 		$eventDispatcher->addServiceListener(UserLoggedInWithCookieEvent::class, UserLoggedInWithCookieListener::class);
 
 		FilesMetadataManager::loadListeners($eventDispatcher);

lib/private/User/Listeners/BeforeUserDeletedListener.php

@@ -26,6 +26,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;
@@ -293,6 +294,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);

lib/private/User/User.php

@@ -9,24 +9,29 @@
 namespace Test\User;
 
 use OC\AllConfig;
+use OC\Avatar\AvatarManager;
 use OC\Files\Mount\ObjectHomeMountProvider;
 use OC\Hooks\PublicEmitter;
 use OC\User\Database;
 use OC\User\User;
 use OCP\Comments\ICommentsManager;
+use OCP\Config\IUserConfig;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Files\FileInfo;
 use OCP\Files\Storage\IStorageFactory;
 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 PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\Attributes\Group;
 use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;
+use Test\Util\User\Dummy;
 
 #[Group('DB')]
 class UserTest extends TestCase {
@@ -429,6 +434,8 @@ public function testDeleteHooks(bool $result, int $expectedHooks): void {
 
 		$commentsManager = $this->createMock(ICommentsManager::class);
 		$notificationManager = $this->createMock(INotificationManager::class);
+		$avatarManager = $this->createMock(AvatarManager::class);
+		$credentialsManager = $this->createMock(ICredentialsManager::class);
 
 		if ($result) {
 			$config->expects($this->atLeastOnce())
@@ -453,6 +460,13 @@ public function testDeleteHooks(bool $result, int $expectedHooks): 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 {
 			$config->expects($this->never())
 				->method('deleteAllUserValues');
@@ -466,20 +480,61 @@ public function testDeleteHooks(bool $result, int $expectedHooks): 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());
 
 		$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');