Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[stable30] fix: Don't crash if disabled user is missing in the database #48285

Merged
merged 1 commit into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions apps/user_ldap/tests/LDAPProviderTest.php
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
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
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
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
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
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
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('test@example.com');
Expand Down
Loading
Loading