Merge pull request #52256 from nextcloud/backport/52224/stable31

[stable31] fix(usermanager): Don't throw when checking if a too long user id is an existing user

Author: nickvergessen

lib/private/User/DisplayNameCache.php

@@ -24,6 +24,8 @@
  * @template-implements IEventListener<UserChangedEvent|UserDeletedEvent>
  */
 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;

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 \OCP\UserInterface[] $backends
 	 */
@@ -131,6 +134,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
@@ -190,6 +197,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);
 	}
@@ -705,14 +716,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'));
 		}
@@ -722,11 +733,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'));
 		}

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 function dataCreateUserInvalid() {
 			['..', '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'],
 		];
 	}