diff options
| -rw-r--r-- | lib/private/User/DisplayNameCache.php | 6 | ||||
| -rw-r--r-- | lib/private/User/Manager.php | 19 | ||||
| -rw-r--r-- | lib/public/IUser.php | 5 | ||||
| -rw-r--r-- | tests/lib/User/ManagerTest.php | 29 |
4 files changed, 56 insertions, 3 deletions
diff --git a/lib/private/User/DisplayNameCache.php b/lib/private/User/DisplayNameCache.php index 34db783de68..40e1c752589 100644 --- a/lib/private/User/DisplayNameCache.php +++ b/lib/private/User/DisplayNameCache.php @@ -11,6 +11,7 @@ use OCP\EventDispatcher\Event; 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; @@ -37,6 +38,11 @@ class DisplayNameCache implements IEventListener { if (isset($this->cache[$userId])) { return $this->cache[$userId]; } + + if (strlen($userId) > IUser::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 1f23a9ae9fd..ca5d90f8c00 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -118,6 +118,10 @@ class Manager extends PublicEmitter implements IUserManager { return $this->cachedUsers[$uid]; } + if (strlen($uid) > IUser::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 +181,10 @@ class Manager extends PublicEmitter implements IUserManager { * @return bool */ public function userExists($uid) { + if (strlen($uid) > IUser::MAX_USERID_LENGTH) { + return false; + } + $user = $this->get($uid); return ($user !== null); } @@ -692,14 +700,14 @@ class Manager extends PublicEmitter implements IUserManager { 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 +717,16 @@ class Manager extends PublicEmitter implements IUserManager { 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) > IUser::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/lib/public/IUser.php b/lib/public/IUser.php index 227e4f902c7..52f79083dc1 100644 --- a/lib/public/IUser.php +++ b/lib/public/IUser.php @@ -16,6 +16,11 @@ use InvalidArgumentException; */ interface IUser { /** + * @since 32.0.0 + */ + public const MAX_USERID_LENGTH = 64; + + /** * get the user id * * @return string diff --git a/tests/lib/User/ManagerTest.php b/tests/lib/User/ManagerTest.php index 53d63ea6758..5d3966cf4d8 100644 --- a/tests/lib/User/ManagerTest.php +++ b/tests/lib/User/ManagerTest.php @@ -79,6 +79,20 @@ class ManagerTest extends TestCase { $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 @@ class ManagerTest extends TestCase { $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 @@ class ManagerTest extends TestCase { ['..', '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'], ]; } |