Skip to content

Commit

Permalink
fix(users): Don't crash if disabled user is missing in the database
Browse files Browse the repository at this point in the history 
Signed-off-by: Louis Chemineau <[email protected]>
  • Loading branch information
@artonge
artonge committed Sep 23, 2024
1 parent c15e241 commit 9a34a6c
Show file tree
Hide file tree
Showing 8 changed files with 68 additions and 39 deletions.
2 changes: 2 additions & 0 deletions apps/user_ldap/tests/LDAPProviderTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use OCP\ICacheFactory;
use OCP\IConfig;
use OCP\IServerContainer;
use Psr\Log\LoggerInterface;

/**
* Class LDAPProviderTest
Expand Down Expand Up @@ -54,6 +55,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())
Expand Down
9 changes: 6 additions & 3 deletions lib/private/User/LazyUser.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,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() {
Expand Down
18 changes: 12 additions & 6 deletions lib/private/User/Manager.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ public function __construct(
private IConfig $config,
ICacheFactory $cacheFactory,
private IEventDispatcher $eventDispatcher,
private LoggerInterface $logger,
) {
$this->cache = new WithLocalCache($cacheFactory->createDistributed('user_backend_map'));
$this->listen('\OC\User', 'postDelete', function (IUser $user): void {
Expand Down Expand Up @@ -201,7 +202,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;
Expand Down Expand Up @@ -319,11 +320,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);
Expand Down
2 changes: 1 addition & 1 deletion tests/lib/Files/Config/UserMountCacheTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,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', '');
Expand Down
9 changes: 6 additions & 3 deletions tests/lib/Files/Storage/Wrapper/EncryptionTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,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())
Expand Down Expand Up @@ -577,7 +578,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,
Expand Down Expand Up @@ -659,7 +661,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();

Expand Down
4 changes: 3 additions & 1 deletion tests/lib/Files/Stream/EncryptionTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,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';
Expand Down Expand Up @@ -57,7 +58,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())
Expand Down
54 changes: 29 additions & 25 deletions tests/lib/User/ManagerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use OCP\ICacheFactory;
use OCP\IConfig;
use OCP\IUser;
use Psr\Log\LoggerInterface;
use Test\TestCase;

/**
Expand All @@ -34,6 +35,8 @@ class ManagerTest extends TestCase {
private $cacheFactory;
/** @var ICache */
private $cache;
/** @var LoggerInterface */
private $logger;

protected function setUp(): void {
parent::setUp();
Expand All @@ -42,14 +45,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);
Expand All @@ -68,7 +72,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'));
Expand All @@ -84,14 +88,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'));
}
Expand All @@ -115,7 +119,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);

Expand All @@ -139,7 +143,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);

Expand All @@ -166,7 +170,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');
Expand All @@ -185,7 +189,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'));
Expand All @@ -203,7 +207,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());
Expand All @@ -219,7 +223,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'));
Expand All @@ -237,7 +241,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());
Expand All @@ -255,7 +259,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');
Expand Down Expand Up @@ -289,7 +293,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);

Expand Down Expand Up @@ -343,7 +347,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);
Expand All @@ -370,7 +374,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');
Expand All @@ -397,7 +401,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');
Expand All @@ -418,14 +422,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'));
}
Expand All @@ -445,7 +449,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);
}

Expand Down Expand Up @@ -485,15 +489,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));
Expand All @@ -518,7 +522,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();
Expand Down Expand Up @@ -559,7 +563,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);

Expand Down Expand Up @@ -672,7 +676,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);
Expand Down Expand Up @@ -706,7 +710,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('[email protected]');
Expand Down
Loading

0 comments on commit 9a34a6c

Please sign in to comment.