From 1e870c02f5450c9103b1a4e13ea4e76fa3e74868 Mon Sep 17 00:00:00 2001 From: Louis Chemineau Date: Fri, 20 Sep 2024 11:26:22 +0200 Subject: [PATCH] fix(users): Don't crash if disabled user is missing in the database Signed-off-by: Louis Chemineau --- apps/user_ldap/tests/LDAPProviderTest.php | 2 + lib/private/User/LazyUser.php | 9 ++-- lib/private/User/Manager.php | 20 ++++--- tests/lib/Files/Config/UserMountCacheTest.php | 2 +- .../Files/Storage/Wrapper/EncryptionTest.php | 9 ++-- tests/lib/Files/Stream/EncryptionTest.php | 4 +- tests/lib/User/ManagerTest.php | 54 ++++++++++--------- tests/lib/User/SessionTest.php | 9 ++++ 8 files changed, 69 insertions(+), 40 deletions(-) diff --git a/apps/user_ldap/tests/LDAPProviderTest.php b/apps/user_ldap/tests/LDAPProviderTest.php index 3e34ce7bdbb14..3c80eb2f41f54 100644 --- 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 index 396d3c252f11d..704075c1b9287 100644 --- 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 index f0d5c7c1e4a96..7067afc273b90 100644 --- 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 index 9e910f4f47f5b..9aebe26178fac 100644 --- 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 index be5f191c6eb14..9888d837f171d 100644 --- 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 index 3d45ff2fc3ced..7a2e1534c9ba5 100644 --- 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 index 5e8c2ed713176..ce408894ec50b 100644 --- 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,6 +46,7 @@ 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); @@ -50,7 +54,7 @@ protected function setUp(): void { 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,7 +490,7 @@ 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); @@ -494,7 +498,7 @@ public function testCreateUserTwoBackendExists() { } 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'); diff --git a/tests/lib/User/SessionTest.php b/tests/lib/User/SessionTest.php index ad4eaccd9de5b..5b5afbe618a09 100644 --- a/tests/lib/User/SessionTest.php +++ b/tests/lib/User/SessionTest.php @@ -182,6 +182,7 @@ public function testLoginValidPasswordEnabled() { $this->config, $this->createMock(ICacheFactory::class), $this->createMock(IEventDispatcher::class), + $this->createMock(LoggerInterface::class), ]) ->getMock(); @@ -248,6 +249,7 @@ public function testLoginValidPasswordDisabled() { $this->config, $this->createMock(ICacheFactory::class), $this->createMock(IEventDispatcher::class), + $this->createMock(LoggerInterface::class), ]) ->getMock(); @@ -281,6 +283,7 @@ public function testLoginInvalidPassword() { $this->config, $this->createMock(ICacheFactory::class), $this->createMock(IEventDispatcher::class), + $this->createMock(LoggerInterface::class), ]) ->getMock(); $backend = $this->createMock(\Test\Util\User\Dummy::class); @@ -324,6 +327,7 @@ public function testPasswordlessLoginNoLastCheckUpdate(): void { $this->config, $this->createMock(ICacheFactory::class), $this->createMock(IEventDispatcher::class), + $this->createMock(LoggerInterface::class), ]) ->getMock(); $userSession = new Session($manager, $session, $this->timeFactory, $this->tokenProvider, $this->config, $this->random, $this->lockdownManager, $this->logger, $this->dispatcher); @@ -361,6 +365,7 @@ public function testLoginLastCheckUpdate(): void { $this->config, $this->createMock(ICacheFactory::class), $this->createMock(IEventDispatcher::class), + $this->createMock(LoggerInterface::class), ]) ->getMock(); $userSession = new Session($manager, $session, $this->timeFactory, $this->tokenProvider, $this->config, $this->random, $this->lockdownManager, $this->logger, $this->dispatcher); @@ -618,6 +623,7 @@ public function testRememberLoginValidToken() { $this->config, $this->createMock(ICacheFactory::class), $this->createMock(IEventDispatcher::class), + $this->createMock(LoggerInterface::class), ]) ->getMock(); $userSession = $this->getMockBuilder(Session::class) @@ -707,6 +713,7 @@ public function testRememberLoginInvalidSessionToken() { $this->config, $this->createMock(ICacheFactory::class), $this->createMock(IEventDispatcher::class), + $this->createMock(LoggerInterface::class), ]) ->getMock(); $userSession = $this->getMockBuilder(Session::class) @@ -771,6 +778,7 @@ public function testRememberLoginInvalidToken() { $this->config, $this->createMock(ICacheFactory::class), $this->createMock(IEventDispatcher::class), + $this->createMock(LoggerInterface::class), ]) ->getMock(); $userSession = $this->getMockBuilder(Session::class) @@ -823,6 +831,7 @@ public function testRememberLoginInvalidUser() { $this->config, $this->createMock(ICacheFactory::class), $this->createMock(IEventDispatcher::class), + $this->createMock(LoggerInterface::class), ]) ->getMock(); $userSession = $this->getMockBuilder(Session::class)