Skip to content

Commit a07d47a

Browse files
artongeartonge
committed
fix(users): Don't crash if disabled user is missing in the database
Signed-off-by: Louis Chemineau <[email protected]>
1 parent c15e241 commit a07d47a

File tree

8 files changed

+65
-36
lines changed

8 files changed

+65
-36
lines changed

‎apps/user_ldap/tests/LDAPProviderTest.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use OCP\ICacheFactory;
1717
use OCP\IConfig;
1818
use OCP\IServerContainer;
19+
use Psr\Log\LoggerInterface;
1920

2021
/**
2122
* Class LDAPProviderTest
@@ -54,6 +55,7 @@ private function getUserManagerMock(IUserLDAP $userBackend) {
5455
$this->createMock(IConfig::class),
5556
$this->createMock(ICacheFactory::class),
5657
$this->createMock(IEventDispatcher::class),
58+
$this->createMock(LoggerInterface::class),
5759
])
5860
->getMock();
5961
$userManager->expects($this->any())

‎lib/private/User/LazyUser.php‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,12 @@ private function getUser(): IUser {
3636
$this->user = $this->userManager->get($this->uid);
3737
}
3838
}
39-
/** @var IUser */
40-
$user = $this->user;
41-
return $user;
39+
40+
if ($this->user === null) {
41+
throw new NoUserException('User not found in backend');
42+
}
43+
44+
return $this->user;
4245
}
4346

4447
public function getUID() {

‎lib/private/User/Manager.php‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ public function __construct(
6868
private IConfig $config,
6969
ICacheFactory $cacheFactory,
7070
private IEventDispatcher $eventDispatcher,
71+
private LoggerInterface $logger,
7172
) {
7273
$this->cache = new WithLocalCache($cacheFactory->createDistributed('user_backend_map'));
7374
$this->listen('\OC\User', 'postDelete', function (IUser $user): void {
@@ -201,7 +202,7 @@ public function checkPassword($loginName, $password) {
201202
$result = $this->checkPasswordNoLogging($loginName, $password);
202203

203204
if ($result === false) {
204-
\OCP\Server::get(LoggerInterface::class)->warning('Login failed: \''. $loginName .'\' (Remote IP: \''. \OC::$server->getRequest()->getRemoteAddress(). '\')', ['app' => 'core']);
205+
$this->logger->warning('Login failed: \''. $loginName .'\' (Remote IP: \''. \OC::$server->getRequest()->getRemoteAddress(). '\')', ['app' => 'core']);
205206
}
206207

207208
return $result;
@@ -319,11 +320,16 @@ public function getDisabledUsers(?int $limit = null, int $offset = 0, string $se
319320
if ($search !== '') {
320321
$users = array_filter(
321322
$users,
322-
fn (IUser $user): bool =>
323-
mb_stripos($user->getUID(), $search) !== false ||
324-
mb_stripos($user->getDisplayName(), $search) !== false ||
325-
mb_stripos($user->getEMailAddress() ?? '', $search) !== false,
326-
);
323+
function (IUser $user) use ($search): bool {
324+
try {
325+
return mb_stripos($user->getUID(), $search) !== false ||
326+
mb_stripos($user->getDisplayName(), $search) !== false ||
327+
mb_stripos($user->getEMailAddress() ?? '', $search) !== false;
328+
} catch (NoUserException $ex) {
329+
$this->logger->error('Error while filtering disabled users', ['exception' => $ex, 'userUID' => $user->getUID()]);
330+
return false;
331+
}
332+
});
327333
}
328334

329335
$tempLimit = ($limit === null ? null : $limit + $offset);

‎tests/lib/Files/Config/UserMountCacheTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ protected function setUp(): void {
6262
->expects($this->any())
6363
->method('getAppValue')
6464
->willReturnArgument(2);
65-
$this->userManager = new Manager($config, $this->createMock(ICacheFactory::class), $this->createMock(IEventDispatcher::class));
65+
$this->userManager = new Manager($config, $this->createMock(ICacheFactory::class), $this->createMock(IEventDispatcher::class), $this->createMock(LoggerInterface::class));
6666
$userBackend = new Dummy();
6767
$userBackend->createUser('u1', '');
6868
$userBackend->createUser('u2', '');

‎tests/lib/Files/Storage/Wrapper/EncryptionTest.php‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,8 @@ protected function setUp(): void {
137137
->setConstructorArgs([new View(), new Manager(
138138
$this->config,
139139
$this->createMock(ICacheFactory::class),
140-
$this->createMock(IEventDispatcher::class)
140+
$this->createMock(IEventDispatcher::class),
141+
$this->createMock(LoggerInterface::class),
141142
), $this->groupManager, $this->config, $this->arrayCache])
142143
->getMock();
143144
$this->util->expects($this->any())
@@ -577,7 +578,8 @@ public function testGetHeader($path, $strippedPathExists, $strippedPath) {
577578
new Manager(
578579
$this->config,
579580
$this->createMock(ICacheFactory::class),
580-
$this->createMock(IEventDispatcher::class)
581+
$this->createMock(IEventDispatcher::class),
582+
$this->createMock(LoggerInterface::class),
581583
),
582584
$this->groupManager,
583585
$this->config,
@@ -659,7 +661,8 @@ public function testGetHeaderAddLegacyModule($header, $isEncrypted, $exists, $ex
659661
->setConstructorArgs([new View(), new Manager(
660662
$this->config,
661663
$this->createMock(ICacheFactory::class),
662-
$this->createMock(IEventDispatcher::class)
664+
$this->createMock(IEventDispatcher::class),
665+
$this->createMock(LoggerInterface::class),
663666
), $this->groupManager, $this->config, $this->arrayCache])
664667
->getMock();
665668

‎tests/lib/Files/Stream/EncryptionTest.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
use OCP\Files\Cache\ICache;
1414
use OCP\ICacheFactory;
1515
use OCP\IConfig;
16+
use Psr\Log\LoggerInterface;
1617

1718
class EncryptionTest extends \Test\TestCase {
1819
public const DEFAULT_WRAPPER = '\OC\Files\Stream\Encryption';
@@ -57,7 +58,8 @@ protected function getStream($fileName, $mode, $unencryptedSize, $wrapper = self
5758
->setConstructorArgs([new View(), new \OC\User\Manager(
5859
$config,
5960
$this->createMock(ICacheFactory::class),
60-
$this->createMock(IEventDispatcher::class)
61+
$this->createMock(IEventDispatcher::class),
62+
$this->createMock(LoggerInterface::class),
6163
), $groupManager, $config, $arrayCache])
6264
->getMock();
6365
$util->expects($this->any())

‎tests/lib/User/ManagerTest.php‎

Lines changed: 26 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use OCP\ICacheFactory;
1717
use OCP\IConfig;
1818
use OCP\IUser;
19+
use Psr\Log\LoggerInterface;
1920
use Test\TestCase;
2021

2122
/**
@@ -34,6 +35,8 @@ class ManagerTest extends TestCase {
3435
private $cacheFactory;
3536
/** @var ICache */
3637
private $cache;
38+
/** @var LoggerInterface */
39+
private $logger;
3740

3841
protected function setUp(): void {
3942
parent::setUp();
@@ -42,14 +45,15 @@ protected function setUp(): void {
4245
$this->eventDispatcher = $this->createMock(IEventDispatcher::class);
4346
$this->cacheFactory = $this->createMock(ICacheFactory::class);
4447
$this->cache = $this->createMock(ICache::class);
48+
$this->logger = $this->createMock(LoggerInterface::class);
4549

4650
$this->cacheFactory->method('createDistributed')
4751
->willReturn($this->cache);
4852
}
4953

5054
public function testGetBackends() {
5155
$userDummyBackend = $this->createMock(\Test\Util\User\Dummy::class);
52-
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
56+
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
5357
$manager->registerBackend($userDummyBackend);
5458
$this->assertEquals([$userDummyBackend], $manager->getBackends());
5559
$dummyDatabaseBackend = $this->createMock(Database::class);
@@ -68,7 +72,7 @@ public function testUserExistsSingleBackendExists() {
6872
->with($this->equalTo('foo'))
6973
->willReturn(true);
7074

71-
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
75+
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
7276
$manager->registerBackend($backend);
7377

7478
$this->assertTrue($manager->userExists('foo'));
@@ -84,7 +88,7 @@ public function testUserExistsSingleBackendNotExists() {
8488
->with($this->equalTo('foo'))
8589
->willReturn(false);
8690

87-
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
91+
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
8892
$manager->registerBackend($backend);
8993

9094
$this->assertFalse($manager->userExists('foo'));
@@ -115,7 +119,7 @@ public function testUserExistsTwoBackendsSecondExists() {
115119
->with($this->equalTo('foo'))
116120
->willReturn(true);
117121

118-
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
122+
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
119123
$manager->registerBackend($backend1);
120124
$manager->registerBackend($backend2);
121125

@@ -139,7 +143,7 @@ public function testUserExistsTwoBackendsFirstExists() {
139143
$backend2->expects($this->never())
140144
->method('userExists');
141145

142-
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
146+
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
143147
$manager->registerBackend($backend1);
144148
$manager->registerBackend($backend2);
145149

@@ -166,7 +170,7 @@ public function testCheckPassword() {
166170
}
167171
});
168172

169-
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
173+
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
170174
$manager->registerBackend($backend);
171175

172176
$user = $manager->checkPassword('foo', 'bar');
@@ -185,7 +189,7 @@ public function testCheckPasswordNotSupported() {
185189
->method('implementsActions')
186190
->willReturn(false);
187191

188-
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
192+
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
189193
$manager->registerBackend($backend);
190194

191195
$this->assertFalse($manager->checkPassword('foo', 'bar'));
@@ -203,7 +207,7 @@ public function testGetOneBackendExists() {
203207
$backend->expects($this->never())
204208
->method('loginName2UserName');
205209

206-
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
210+
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
207211
$manager->registerBackend($backend);
208212

209213
$this->assertEquals('foo', $manager->get('foo')->getUID());
@@ -219,7 +223,7 @@ public function testGetOneBackendNotExists() {
219223
->with($this->equalTo('foo'))
220224
->willReturn(false);
221225

222-
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
226+
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
223227
$manager->registerBackend($backend);
224228

225229
$this->assertEquals(null, $manager->get('foo'));
@@ -237,7 +241,7 @@ public function testGetOneBackendDoNotTranslateLoginNames() {
237241
$backend->expects($this->never())
238242
->method('loginName2UserName');
239243

240-
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
244+
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
241245
$manager->registerBackend($backend);
242246

243247
$this->assertEquals('bLeNdEr', $manager->get('bLeNdEr')->getUID());
@@ -255,7 +259,7 @@ public function testSearchOneBackend() {
255259
$backend->expects($this->never())
256260
->method('loginName2UserName');
257261

258-
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
262+
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
259263
$manager->registerBackend($backend);
260264

261265
$result = $manager->search('fo');
@@ -289,7 +293,7 @@ public function testSearchTwoBackendLimitOffset() {
289293
$backend2->expects($this->never())
290294
->method('loginName2UserName');
291295

292-
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
296+
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
293297
$manager->registerBackend($backend1);
294298
$manager->registerBackend($backend2);
295299

@@ -343,7 +347,7 @@ public function testCreateUserInvalid($uid, $password, $exception) {
343347
->willReturn(true);
344348

345349

346-
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
350+
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
347351
$manager->registerBackend($backend);
348352

349353
$this->expectException(\InvalidArgumentException::class, $exception);
@@ -370,7 +374,7 @@ public function testCreateUserSingleBackendNotExists() {
370374
$backend->expects($this->never())
371375
->method('loginName2UserName');
372376

373-
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
377+
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
374378
$manager->registerBackend($backend);
375379

376380
$user = $manager->createUser('foo', 'bar');
@@ -397,7 +401,7 @@ public function testCreateUserSingleBackendExists() {
397401
->with($this->equalTo('foo'))
398402
->willReturn(true);
399403

400-
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
404+
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
401405
$manager->registerBackend($backend);
402406

403407
$manager->createUser('foo', 'bar');
@@ -418,7 +422,7 @@ public function testCreateUserSingleBackendNotSupported() {
418422
$backend->expects($this->never())
419423
->method('userExists');
420424

421-
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
425+
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
422426
$manager->registerBackend($backend);
423427

424428
$this->assertFalse($manager->createUser('foo', 'bar'));
@@ -445,7 +449,7 @@ public function testCreateUserFromBackendWithBackendError() {
445449
->with('MyUid', 'MyPassword')
446450
->willReturn(false);
447451

448-
$manager = new Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
452+
$manager = new Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
449453
$manager->createUserFromBackend('MyUid', 'MyPassword', $backend);
450454
}
451455

@@ -485,7 +489,7 @@ public function testCreateUserTwoBackendExists() {
485489
->with($this->equalTo('foo'))
486490
->willReturn(true);
487491

488-
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
492+
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
489493
$manager->registerBackend($backend1);
490494
$manager->registerBackend($backend2);
491495

@@ -518,7 +522,7 @@ public function testCountUsersOneBackend() {
518522
->method('getBackendName')
519523
->willReturn('Mock_Test_Util_User_Dummy');
520524

521-
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
525+
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
522526
$manager->registerBackend($backend);
523527

524528
$result = $manager->countUsers();
@@ -559,7 +563,7 @@ public function testCountUsersTwoBackends() {
559563
->method('getBackendName')
560564
->willReturn('Mock_Test_Util_User_Dummy');
561565

562-
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher);
566+
$manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
563567
$manager->registerBackend($backend1);
564568
$manager->registerBackend($backend2);
565569

@@ -672,7 +676,7 @@ public function testDeleteUser() {
672676
->method('getAppValue')
673677
->willReturnArgument(2);
674678

675-
$manager = new \OC\User\Manager($config, $this->cacheFactory, $this->eventDispatcher);
679+
$manager = new \OC\User\Manager($config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
676680
$backend = new \Test\Util\User\Dummy();
677681

678682
$manager->registerBackend($backend);
@@ -706,7 +710,7 @@ public function testGetByEmail() {
706710
true
707711
);
708712

709-
$manager = new \OC\User\Manager($config, $this->cacheFactory, $this->eventDispatcher);
713+
$manager = new \OC\User\Manager($config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
710714
$manager->registerBackend($backend);
711715

712716
$users = $manager->getByEmail('[email protected]');

0 commit comments

Comments
 (0)