From 775ca882f31df3663a8876952363070199c2816b Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 16 Apr 2025 17:28:10 +0200 Subject: [PATCH 1/2] fix(usermanager): Don't throw when checking if a too long user id is an existing user Signed-off-by: Joas Schilling --- lib/private/User/DisplayNameCache.php | 7 +++++++ lib/private/User/Manager.php | 22 +++++++++++++++++--- tests/lib/User/ManagerTest.php | 29 +++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 3 deletions(-) diff --git a/lib/private/User/DisplayNameCache.php b/lib/private/User/DisplayNameCache.php index 34db783de6833..114ec3627dd75 100644 --- a/lib/private/User/DisplayNameCache.php +++ b/lib/private/User/DisplayNameCache.php @@ -24,6 +24,8 @@ * @template-implements IEventListener */ class DisplayNameCache implements IEventListener { + /** @see \OC\Config\UserConfig::USER_MAX_LENGTH */ + public const MAX_USERID_LENGTH = 64; private array $cache = []; private ICache $memCache; private IUserManager $userManager; @@ -37,6 +39,11 @@ public function getDisplayName(string $userId): ?string { if (isset($this->cache[$userId])) { return $this->cache[$userId]; } + + if (strlen($userId) > self::MAX_USERID_LENGTH) { + return null; + } + $displayName = $this->memCache->get($userId); if ($displayName) { $this->cache[$userId] = $displayName; diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index 1f23a9ae9fd43..b26e6b3973a37 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -53,6 +53,9 @@ * @package OC\User */ class Manager extends PublicEmitter implements IUserManager { + /** @see \OC\Config\UserConfig::USER_MAX_LENGTH */ + public const MAX_USERID_LENGTH = 64; + /** * @var UserInterface[] $backends */ @@ -118,6 +121,10 @@ public function get($uid) { return $this->cachedUsers[$uid]; } + if (strlen($uid) > self::MAX_USERID_LENGTH) { + return null; + } + $cachedBackend = $this->cache->get(sha1($uid)); if ($cachedBackend !== null && isset($this->backends[$cachedBackend])) { // Cache has the info of the user backend already, so ask that one directly @@ -177,6 +184,10 @@ public function getUserObject($uid, $backend, $cacheUser = true) { * @return bool */ public function userExists($uid) { + if (strlen($uid) > self::MAX_USERID_LENGTH) { + return false; + } + $user = $this->get($uid); return ($user !== null); } @@ -692,14 +703,14 @@ public function getByEmail($email) { public function validateUserId(string $uid, bool $checkDataDirectory = false): void { $l = Server::get(IFactory::class)->get('lib'); - // Check the name for bad characters + // Check the ID for bad characters // Allowed are: "a-z", "A-Z", "0-9", spaces and "_.@-'" if (preg_match('/[^a-zA-Z0-9 _.@\-\']/', $uid)) { throw new \InvalidArgumentException($l->t('Only the following characters are allowed in an Login:' . ' "a-z", "A-Z", "0-9", spaces and "_.@-\'"')); } - // No empty username + // No empty user ID if (trim($uid) === '') { throw new \InvalidArgumentException($l->t('A valid Login must be provided')); } @@ -709,11 +720,16 @@ public function validateUserId(string $uid, bool $checkDataDirectory = false): v throw new \InvalidArgumentException($l->t('Login contains whitespace at the beginning or at the end')); } - // Username only consists of 1 or 2 dots (directory traversal) + // User ID only consists of 1 or 2 dots (directory traversal) if ($uid === '.' || $uid === '..') { throw new \InvalidArgumentException($l->t('Login must not consist of dots only')); } + // User ID is too long + if (strlen($uid) > self::MAX_USERID_LENGTH) { + throw new \InvalidArgumentException($l->t('Login is too long')); + } + if (!$this->verifyUid($uid, $checkDataDirectory)) { throw new \InvalidArgumentException($l->t('Login is invalid because files already exist for this user')); } diff --git a/tests/lib/User/ManagerTest.php b/tests/lib/User/ManagerTest.php index 53d63ea675875..5d3966cf4d89a 100644 --- a/tests/lib/User/ManagerTest.php +++ b/tests/lib/User/ManagerTest.php @@ -79,6 +79,20 @@ public function testUserExistsSingleBackendExists(): void { $this->assertTrue($manager->userExists('foo')); } + public function testUserExistsTooLong(): void { + /** @var \Test\Util\User\Dummy|MockObject $backend */ + $backend = $this->createMock(\Test\Util\User\Dummy::class); + $backend->expects($this->never()) + ->method('userExists') + ->with($this->equalTo('foo')) + ->willReturn(true); + + $manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger); + $manager->registerBackend($backend); + + $this->assertFalse($manager->userExists('foo' . str_repeat('a', 62))); + } + public function testUserExistsSingleBackendNotExists(): void { /** * @var \Test\Util\User\Dummy | \PHPUnit\Framework\MockObject\MockObject $backend @@ -230,6 +244,20 @@ public function testGetOneBackendNotExists(): void { $this->assertEquals(null, $manager->get('foo')); } + public function testGetTooLong(): void { + /** @var \Test\Util\User\Dummy|MockObject $backend */ + $backend = $this->createMock(\Test\Util\User\Dummy::class); + $backend->expects($this->never()) + ->method('userExists') + ->with($this->equalTo('foo')) + ->willReturn(false); + + $manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger); + $manager->registerBackend($backend); + + $this->assertEquals(null, $manager->get('foo' . str_repeat('a', 62))); + } + public function testGetOneBackendDoNotTranslateLoginNames(): void { /** * @var \Test\Util\User\Dummy | \PHPUnit\Framework\MockObject\MockObject $backend @@ -333,6 +361,7 @@ public static function dataCreateUserInvalid(): array { ['..', 'foo', 'Username must not consist of dots only'], ['.test', '', 'A valid password must be provided'], ['test', '', 'A valid password must be provided'], + ['test' . str_repeat('a', 61), '', 'Login is too long'], ]; } From 1ada9910b10052e91809b111687e2713488fa6a6 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 17 Apr 2025 09:38:49 +0200 Subject: [PATCH 2/2] fix(user): Introduce a public constant for max length of user id Signed-off-by: Joas Schilling --- lib/private/User/DisplayNameCache.php | 5 ++--- lib/private/User/Manager.php | 9 +++------ lib/public/IUser.php | 5 +++++ 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/lib/private/User/DisplayNameCache.php b/lib/private/User/DisplayNameCache.php index 114ec3627dd75..40e1c75258916 100644 --- a/lib/private/User/DisplayNameCache.php +++ b/lib/private/User/DisplayNameCache.php @@ -11,6 +11,7 @@ use OCP\EventDispatcher\IEventListener; use OCP\ICache; use OCP\ICacheFactory; +use OCP\IUser; use OCP\IUserManager; use OCP\User\Events\UserChangedEvent; use OCP\User\Events\UserDeletedEvent; @@ -24,8 +25,6 @@ * @template-implements IEventListener */ class DisplayNameCache implements IEventListener { - /** @see \OC\Config\UserConfig::USER_MAX_LENGTH */ - public const MAX_USERID_LENGTH = 64; private array $cache = []; private ICache $memCache; private IUserManager $userManager; @@ -40,7 +39,7 @@ public function getDisplayName(string $userId): ?string { return $this->cache[$userId]; } - if (strlen($userId) > self::MAX_USERID_LENGTH) { + if (strlen($userId) > IUser::MAX_USERID_LENGTH) { return null; } diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index b26e6b3973a37..ca5d90f8c00a0 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -53,9 +53,6 @@ * @package OC\User */ class Manager extends PublicEmitter implements IUserManager { - /** @see \OC\Config\UserConfig::USER_MAX_LENGTH */ - public const MAX_USERID_LENGTH = 64; - /** * @var UserInterface[] $backends */ @@ -121,7 +118,7 @@ public function get($uid) { return $this->cachedUsers[$uid]; } - if (strlen($uid) > self::MAX_USERID_LENGTH) { + if (strlen($uid) > IUser::MAX_USERID_LENGTH) { return null; } @@ -184,7 +181,7 @@ public function getUserObject($uid, $backend, $cacheUser = true) { * @return bool */ public function userExists($uid) { - if (strlen($uid) > self::MAX_USERID_LENGTH) { + if (strlen($uid) > IUser::MAX_USERID_LENGTH) { return false; } @@ -726,7 +723,7 @@ public function validateUserId(string $uid, bool $checkDataDirectory = false): v } // User ID is too long - if (strlen($uid) > self::MAX_USERID_LENGTH) { + if (strlen($uid) > IUser::MAX_USERID_LENGTH) { throw new \InvalidArgumentException($l->t('Login is too long')); } diff --git a/lib/public/IUser.php b/lib/public/IUser.php index 227e4f902c7e5..52f79083dc178 100644 --- a/lib/public/IUser.php +++ b/lib/public/IUser.php @@ -15,6 +15,11 @@ * @since 8.0.0 */ interface IUser { + /** + * @since 32.0.0 + */ + public const MAX_USERID_LENGTH = 64; + /** * get the user id *