fix(users): Don't crash if disabled user is missing in the database

Signed-off-by: Louis Chemineau <louis@chmn.me>
nextcloud · Sep 23, 2024 · 1e870c0 · 1e870c0
1 parent 81216ed
commit 1e870c0
Show file tree

Hide file tree

Showing 8 changed files with 69 additions and 40 deletions.
diff --git a/apps/user_ldap/tests/LDAPProviderTest.php b/apps/user_ldap/tests/LDAPProviderTest.php
@@ -39,6 +39,7 @@
 use OCP\ICacheFactory;
 use OCP\IConfig;
 use OCP\IServerContainer;
+use Psr\Log\LoggerInterface;
 
 /**
  * Class LDAPProviderTest
@@ -77,6 +78,7 @@ private function getUserManagerMock(IUserLDAP $userBackend) {
 				$this->createMock(IConfig::class),
 				$this->createMock(ICacheFactory::class),
 				$this->createMock(IEventDispatcher::class),
+				$this->createMock(LoggerInterface::class),
 			])
 			->getMock();
 		$userManager->expects($this->any())

diff --git a/lib/private/User/LazyUser.php b/lib/private/User/LazyUser.php
@@ -51,9 +51,12 @@ private function getUser(): IUser {
 				$this->user = $this->userManager->get($this->uid);
 			}
 		}
-		/** @var IUser */
-		$user = $this->user;
-		return $user;
+
+		if ($this->user === null) {
+			throw new NoUserException('User not found in backend');
+		}
+
+		return $this->user;
 	}
 
 	public function getUID() {

diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php
@@ -99,7 +99,8 @@ class Manager extends PublicEmitter implements IUserManager {
 
 	public function __construct(IConfig $config,
 		ICacheFactory $cacheFactory,
-		IEventDispatcher $eventDispatcher) {
+		IEventDispatcher $eventDispatcher,
+		private LoggerInterface $logger) {
 		$this->config = $config;
 		$this->cache = new WithLocalCache($cacheFactory->createDistributed('user_backend_map'));
 		$cachedUsers = &$this->cachedUsers;
@@ -236,7 +237,7 @@ public function checkPassword($loginName, $password) {
 		$result = $this->checkPasswordNoLogging($loginName, $password);
 
 		if ($result === false) {
-			\OCP\Server::get(LoggerInterface::class)->warning('Login failed: \''. $loginName .'\' (Remote IP: \''. \OC::$server->getRequest()->getRemoteAddress(). '\')', ['app' => 'core']);
+			$this->logger->warning('Login failed: \''. $loginName .'\' (Remote IP: \''. \OC::$server->getRequest()->getRemoteAddress(). '\')', ['app' => 'core']);
 		}
 
 		return $result;
@@ -354,11 +355,16 @@ public function getDisabledUsers(?int $limit = null, int $offset = 0, string $se
 		if ($search !== '') {
 			$users = array_filter(
 				$users,
-				fn (IUser $user): bool =>
-					mb_stripos($user->getUID(), $search) !== false ||
-					mb_stripos($user->getDisplayName(), $search) !== false ||
-					mb_stripos($user->getEMailAddress() ?? '', $search) !== false,
-			);
+				function (IUser $user) use ($search): bool {
+					try {
+						return mb_stripos($user->getUID(), $search) !== false ||
+						mb_stripos($user->getDisplayName(), $search) !== false ||
+						mb_stripos($user->getEMailAddress() ?? '', $search) !== false;
+					} catch (NoUserException $ex) {
+						$this->logger->error('Error while filtering disabled users', ['exception' => $ex, 'userUID' => $user->getUID()]);
+						return false;
+					}
+				});
 		}
 
 		$tempLimit = ($limit === null ? null : $limit + $offset);

diff --git a/tests/lib/Files/Config/UserMountCacheTest.php b/tests/lib/Files/Config/UserMountCacheTest.php
@@ -61,7 +61,7 @@ protected function setUp(): void {
 			->expects($this->any())
 			->method('getAppValue')
 			->willReturnArgument(2);
-		$this->userManager = new Manager($config, $this->createMock(ICacheFactory::class), $this->createMock(IEventDispatcher::class));
+		$this->userManager = new Manager($config, $this->createMock(ICacheFactory::class), $this->createMock(IEventDispatcher::class), $this->createMock(LoggerInterface::class));
 		$userBackend = new Dummy();
 		$userBackend->createUser('u1', '');
 		$userBackend->createUser('u2', '');

diff --git a/tests/lib/Files/Storage/Wrapper/EncryptionTest.php b/tests/lib/Files/Storage/Wrapper/EncryptionTest.php
@@ -133,7 +133,8 @@ protected function setUp(): void {
 			->setConstructorArgs([new View(), new Manager(
 				$this->config,
 				$this->createMock(ICacheFactory::class),
-				$this->createMock(IEventDispatcher::class)
+				$this->createMock(IEventDispatcher::class),
+				$this->createMock(LoggerInterface::class),
 			), $this->groupManager, $this->config, $this->arrayCache])
 			->getMock();
 		$this->util->expects($this->any())
@@ -573,7 +574,8 @@ public function testGetHeader($path, $strippedPathExists, $strippedPath) {
 					new Manager(
 						$this->config,
 						$this->createMock(ICacheFactory::class),
-						$this->createMock(IEventDispatcher::class)
+						$this->createMock(IEventDispatcher::class),
+						$this->createMock(LoggerInterface::class),
 					),
 					$this->groupManager,
 					$this->config,
@@ -655,7 +657,8 @@ public function testGetHeaderAddLegacyModule($header, $isEncrypted, $exists, $ex
 			->setConstructorArgs([new View(), new Manager(
 				$this->config,
 				$this->createMock(ICacheFactory::class),
-				$this->createMock(IEventDispatcher::class)
+				$this->createMock(IEventDispatcher::class),
+				$this->createMock(LoggerInterface::class),
 			), $this->groupManager, $this->config, $this->arrayCache])
 			->getMock();
 

diff --git a/tests/lib/Files/Stream/EncryptionTest.php b/tests/lib/Files/Stream/EncryptionTest.php
@@ -9,6 +9,7 @@
 use OCP\Files\Cache\ICache;
 use OCP\ICacheFactory;
 use OCP\IConfig;
+use Psr\Log\LoggerInterface;
 
 class EncryptionTest extends \Test\TestCase {
 	public const DEFAULT_WRAPPER = '\OC\Files\Stream\Encryption';
@@ -53,7 +54,8 @@ protected function getStream($fileName, $mode, $unencryptedSize, $wrapper = self
 			->setConstructorArgs([new View(), new \OC\User\Manager(
 				$config,
 				$this->createMock(ICacheFactory::class),
-				$this->createMock(IEventDispatcher::class)
+				$this->createMock(IEventDispatcher::class),
+				$this->createMock(LoggerInterface::class),
 			), $groupManager, $config, $arrayCache])
 			->getMock();
 		$util->expects($this->any())

diff --git a/tests/lib/User/ManagerTest.php b/tests/lib/User/ManagerTest.php
@@ -17,6 +17,7 @@
 use OCP\ICacheFactory;
 use OCP\IConfig;
 use OCP\IUser;
+use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
 /**
@@ -35,6 +36,8 @@ class ManagerTest extends TestCase {
 	private $cacheFactory;
 	/** @var ICache */
 	private $cache;
+	/** @var LoggerInterface */
+	private $logger;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -43,14 +46,15 @@ protected function setUp(): void {
 		$this->eventDispatcher = $this->createMock(IEventDispatcher::class);
 		$this->cacheFactory = $this->createMock(ICacheFactory::class);
 		$this->cache = $this->createMock(ICache::class);
+		$this->logger = $this->createMock(LoggerInterface::class);
 
 		$this->cacheFactory->method('createDistributed')
 			->willReturn($this->cache);
 	}
 
 	public function testGetBackends() {
 		$userDummyBackend = $this->createMock(\Test\Util\User\Dummy::class);
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->registerBackend($userDummyBackend);
 		$this->assertEquals([$userDummyBackend], $manager->getBackends());
 		$dummyDatabaseBackend = $this->createMock(Database::class);
@@ -69,7 +73,7 @@ public function testUserExistsSingleBackendExists() {
 			->with($this->equalTo('foo'))
 			->willReturn(true);
 
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->registerBackend($backend);
 
 		$this->assertTrue($manager->userExists('foo'));
@@ -85,14 +89,14 @@ public function testUserExistsSingleBackendNotExists() {
 			->with($this->equalTo('foo'))
 			->willReturn(false);
 
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->registerBackend($backend);
 
 		$this->assertFalse($manager->userExists('foo'));
 	}
 
 	public function testUserExistsNoBackends() {
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 
 		$this->assertFalse($manager->userExists('foo'));
 	}
@@ -116,7 +120,7 @@ public function testUserExistsTwoBackendsSecondExists() {
 			->with($this->equalTo('foo'))
 			->willReturn(true);
 
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->registerBackend($backend1);
 		$manager->registerBackend($backend2);
 
@@ -140,7 +144,7 @@ public function testUserExistsTwoBackendsFirstExists() {
 		$backend2->expects($this->never())
 			->method('userExists');
 
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->registerBackend($backend1);
 		$manager->registerBackend($backend2);
 
@@ -167,7 +171,7 @@ public function testCheckPassword() {
 				}
 			});
 
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->registerBackend($backend);
 
 		$user = $manager->checkPassword('foo', 'bar');
@@ -186,7 +190,7 @@ public function testCheckPasswordNotSupported() {
 			->method('implementsActions')
 			->willReturn(false);
 
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->registerBackend($backend);
 
 		$this->assertFalse($manager->checkPassword('foo', 'bar'));
@@ -204,7 +208,7 @@ public function testGetOneBackendExists() {
 		$backend->expects($this->never())
 			->method('loginName2UserName');
 
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->registerBackend($backend);
 
 		$this->assertEquals('foo', $manager->get('foo')->getUID());
@@ -220,7 +224,7 @@ public function testGetOneBackendNotExists() {
 			->with($this->equalTo('foo'))
 			->willReturn(false);
 
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->registerBackend($backend);
 
 		$this->assertEquals(null, $manager->get('foo'));
@@ -238,7 +242,7 @@ public function testGetOneBackendDoNotTranslateLoginNames() {
 		$backend->expects($this->never())
 			->method('loginName2UserName');
 
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->registerBackend($backend);
 
 		$this->assertEquals('bLeNdEr', $manager->get('bLeNdEr')->getUID());
@@ -256,7 +260,7 @@ public function testSearchOneBackend() {
 		$backend->expects($this->never())
 			->method('loginName2UserName');
 
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->registerBackend($backend);
 
 		$result = $manager->search('fo');
@@ -290,7 +294,7 @@ public function testSearchTwoBackendLimitOffset() {
 		$backend2->expects($this->never())
 			->method('loginName2UserName');
 
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->registerBackend($backend1);
 		$manager->registerBackend($backend2);
 
@@ -344,7 +348,7 @@ public function testCreateUserInvalid($uid, $password, $exception) {
 			->willReturn(true);
 
 
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->registerBackend($backend);
 
 		$this->expectException(\InvalidArgumentException::class, $exception);
@@ -371,7 +375,7 @@ public function testCreateUserSingleBackendNotExists() {
 		$backend->expects($this->never())
 			->method('loginName2UserName');
 
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->registerBackend($backend);
 
 		$user = $manager->createUser('foo', 'bar');
@@ -398,7 +402,7 @@ public function testCreateUserSingleBackendExists() {
 			->with($this->equalTo('foo'))
 			->willReturn(true);
 
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->registerBackend($backend);
 
 		$manager->createUser('foo', 'bar');
@@ -419,14 +423,14 @@ public function testCreateUserSingleBackendNotSupported() {
 		$backend->expects($this->never())
 			->method('userExists');
 
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->registerBackend($backend);
 
 		$this->assertFalse($manager->createUser('foo', 'bar'));
 	}
 
 	public function testCreateUserNoBackends() {
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 
 		$this->assertFalse($manager->createUser('foo', 'bar'));
 	}
@@ -446,7 +450,7 @@ public function testCreateUserFromBackendWithBackendError() {
 			->with('MyUid', 'MyPassword')
 			->willReturn(false);
 
-		$manager = new Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->createUserFromBackend('MyUid', 'MyPassword', $backend);
 	}
 
@@ -486,15 +490,15 @@ public function testCreateUserTwoBackendExists() {
 			->with($this->equalTo('foo'))
 			->willReturn(true);
 
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->registerBackend($backend1);
 		$manager->registerBackend($backend2);
 
 		$manager->createUser('foo', 'bar');
 	}
 
 	public function testCountUsersNoBackend() {
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 
 		$result = $manager->countUsers();
 		$this->assertTrue(is_array($result));
@@ -519,7 +523,7 @@ public function testCountUsersOneBackend() {
 			->method('getBackendName')
 			->willReturn('Mock_Test_Util_User_Dummy');
 
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->registerBackend($backend);
 
 		$result = $manager->countUsers();
@@ -560,7 +564,7 @@ public function testCountUsersTwoBackends() {
 			->method('getBackendName')
 			->willReturn('Mock_Test_Util_User_Dummy');
 
-		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->registerBackend($backend1);
 		$manager->registerBackend($backend2);
 
@@ -673,7 +677,7 @@ public function testDeleteUser() {
 			->method('getAppValue')
 			->willReturnArgument(2);
 
-		$manager = new \OC\User\Manager($config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$backend = new \Test\Util\User\Dummy();
 
 		$manager->registerBackend($backend);
@@ -707,7 +711,7 @@ public function testGetByEmail() {
 				true
 			);
 
-		$manager = new \OC\User\Manager($config, $this->cacheFactory, $this->eventDispatcher);
+		$manager = new \OC\User\Manager($config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
 		$manager->registerBackend($backend);
 
 		$users = $manager->getByEmail('test@example.com');