Merge pull request #52224 from nextcloud/bugfix/noid/dont-break-when-checking-if-too-long-user-exists

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

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;

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'));
 		}

lib/public/IUser.php

@@ -15,6 +15,11 @@ use InvalidArgumentException;
  * @since 8.0.0
  */
 interface IUser {
+	/**
+	 * @since 32.0.0
+	 */
+	public const MAX_USERID_LENGTH = 64;
+
 	/**
 	 * get the user id
 	 *

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'],
 		];
 	}