Merge pull request #36621 from nextcloud/perf/noid/only-check-for-token-when-it-can-actually-be

fix(performance): Only search for auth tokens when the provided login…

2 files changed, 72 insertions, 48 deletions

diff --git a/lib/private/Authentication/Token/PublicKeyTokenProvider.php b/lib/private/Authentication/Token/PublicKeyTokenProvider.php
index e64abcd231f..84708065070 100644
--- a/lib/private/Authentication/Token/PublicKeyTokenProvider.php
+++ b/lib/private/Authentication/Token/PublicKeyTokenProvider.php
@@ -46,6 +46,8 @@ use OCP\Security\IHasher;
 use Psr\Log\LoggerInterface;
 
 class PublicKeyTokenProvider implements IProvider {
+	public const TOKEN_MIN_LENGTH = 22;
+
 	use TTransactional;
 
 	/** @var PublicKeyTokenMapper */
@@ -98,6 +100,12 @@ class PublicKeyTokenProvider implements IProvider {
 								  string $name,
 								  int $type = IToken::TEMPORARY_TOKEN,
 								  int $remember = IToken::DO_NOT_REMEMBER): IToken {
+		if (strlen($token) < self::TOKEN_MIN_LENGTH) {
+			$exception = new InvalidTokenException('Token is too short, minimum of ' . self::TOKEN_MIN_LENGTH . ' characters is required, ' . strlen($token) . ' characters given');
+			$this->logger->error('Invalid token provided when generating new token', ['exception' => $exception]);
+			throw $exception;
+		}
+
 		if (mb_strlen($name) > 128) {
 			$name = mb_substr($name, 0, 120) . '…';
 		}
@@ -126,6 +134,27 @@ class PublicKeyTokenProvider implements IProvider {
 	}
 
 	public function getToken(string $tokenId): IToken {
+		/**
+		 * Token length: 72
+		 * @see \OC\Core\Controller\ClientFlowLoginController::generateAppPassword
+		 * @see \OC\Core\Controller\AppPasswordController::getAppPassword
+		 * @see \OC\Core\Command\User\AddAppPassword::execute
+		 * @see \OC\Core\Service\LoginFlowV2Service::flowDone
+		 * @see \OCA\Talk\MatterbridgeManager::generatePassword
+		 * @see \OCA\Preferred_Providers\Controller\PasswordController::generateAppPassword
+		 * @see \OCA\GlobalSiteSelector\TokenHandler::generateAppPassword
+		 *
+		 * Token length: 22-256 - https://www.php.net/manual/en/session.configuration.php#ini.session.sid-length
+		 * @see \OC\User\Session::createSessionToken
+		 *
+		 * Token length: 29
+		 * @see \OCA\Settings\Controller\AuthSettingsController::generateRandomDeviceToken
+		 * @see \OCA\Registration\Service\RegistrationService::generateAppPassword
+		 */
+		if (strlen($tokenId) < self::TOKEN_MIN_LENGTH) {
+			throw new InvalidTokenException('Token is too short for a generated token, should be the password during basic auth');
+		}
+
 		$tokenHash = $this->hashToken($tokenId);
 
 		if (isset($this->cache[$tokenHash])) {
@@ -136,7 +165,7 @@ class PublicKeyTokenProvider implements IProvider {
 			$token = $this->cache[$tokenHash];
 		} else {
 			try {
-				$token = $this->mapper->getToken($this->hashToken($tokenId));
+				$token = $this->mapper->getToken($tokenHash);
 				$this->cache[$token->getToken()] = $token;
 			} catch (DoesNotExistException $ex) {
 				try {
diff --git a/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php b/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php
index ca7618dfd6d..d19f98eeda9 100644
--- a/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php
+++ b/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php
@@ -93,7 +93,7 @@ class PublicKeyTokenProviderTest extends TestCase {
 	}
 
 	public function testGenerateToken() {
-		$token = 'token';
+		$token = 'tokentokentokentokentoken';
 		$uid = 'user';
 		$user = 'User';
 		$password = 'passme';
@@ -115,7 +115,7 @@ class PublicKeyTokenProviderTest extends TestCase {
 	}
 
 	public function testGenerateTokenNoPassword(): void {
-		$token = 'token';
+		$token = 'tokentokentokentokentoken';
 		$uid = 'user';
 		$user = 'User';
 		$password = 'passme';
@@ -138,7 +138,7 @@ class PublicKeyTokenProviderTest extends TestCase {
 	}
 
 	public function testGenerateTokenLongPassword() {
-		$token = 'token';
+		$token = 'tokentokentokentokentoken';
 		$uid = 'user';
 		$user = 'User';
 		$password = '';
@@ -157,7 +157,7 @@ class PublicKeyTokenProviderTest extends TestCase {
 	}
 
 	public function testGenerateTokenInvalidName() {
-		$token = 'token';
+		$token = 'tokentokentokentokentoken';
 		$uid = 'user';
 		$user = 'User';
 		$password = 'passme';
@@ -222,7 +222,7 @@ class PublicKeyTokenProviderTest extends TestCase {
 	}
 
 	public function testGetPassword() {
-		$token = 'token';
+		$token = 'tokentokentokentokentoken';
 		$uid = 'user';
 		$user = 'User';
 		$password = 'passme';
@@ -253,7 +253,7 @@ class PublicKeyTokenProviderTest extends TestCase {
 	public function testGetPasswordInvalidToken() {
 		$this->expectException(\OC\Authentication\Exceptions\InvalidTokenException::class);
 
-		$token = 'token';
+		$token = 'tokentokentokentokentoken';
 		$uid = 'user';
 		$user = 'User';
 		$password = 'passme';
@@ -270,7 +270,7 @@ class PublicKeyTokenProviderTest extends TestCase {
 	}
 
 	public function testSetPassword() {
-		$token = 'token';
+		$token = 'tokentokentokentokentoken';
 		$uid = 'user';
 		$user = 'User';
 		$password = 'passme';
@@ -291,13 +291,13 @@ class PublicKeyTokenProviderTest extends TestCase {
 		$this->mapper->expects($this->once())
 			->method('update')
 			->with($this->callback(function ($token) use ($newpass) {
-				return $newpass === $this->tokenProvider->getPassword($token, 'token');
+				return $newpass === $this->tokenProvider->getPassword($token, 'tokentokentokentokentoken');
 			}));
 
 
 		$this->tokenProvider->setPassword($actual, $token, $newpass);
 
-		$this->assertSame($newpass, $this->tokenProvider->getPassword($actual, 'token'));
+		$this->assertSame($newpass, $this->tokenProvider->getPassword($actual, 'tokentokentokentokentoken'));
 	}
 
 
@@ -312,12 +312,12 @@ class PublicKeyTokenProviderTest extends TestCase {
 	}
 
 	public function testInvalidateToken() {
-		$this->mapper->expects($this->at(0))
-			->method('invalidate')
-			->with(hash('sha512', 'token7'.'1f4h9s'));
-		$this->mapper->expects($this->at(1))
+		$this->mapper->expects($this->exactly(2))
 			->method('invalidate')
-			->with(hash('sha512', 'token7'));
+			->withConsecutive(
+				[hash('sha512', 'token7'.'1f4h9s')],
+				[hash('sha512', 'token7')]
+			);
 
 		$this->tokenProvider->invalidateToken('token7');
 	}
@@ -352,7 +352,7 @@ class PublicKeyTokenProviderTest extends TestCase {
 	}
 
 	public function testRenewSessionTokenWithoutPassword() {
-		$token = 'oldId';
+		$token = 'oldIdtokentokentokentoken';
 		$uid = 'user';
 		$user = 'User';
 		$password = null;
@@ -364,7 +364,7 @@ class PublicKeyTokenProviderTest extends TestCase {
 		$this->mapper
 			->expects($this->once())
 			->method('getToken')
-			->with(hash('sha512', 'oldId' . '1f4h9s'))
+			->with(hash('sha512', 'oldIdtokentokentokentoken' . '1f4h9s'))
 			->willReturn($oldToken);
 		$this->mapper
 			->expects($this->once())
@@ -384,11 +384,11 @@ class PublicKeyTokenProviderTest extends TestCase {
 				return $token === $oldToken;
 			}));
 
-		$this->tokenProvider->renewSessionToken('oldId', 'newId');
+		$this->tokenProvider->renewSessionToken('oldIdtokentokentokentoken', 'newIdtokentokentokentoken');
 	}
 
 	public function testRenewSessionTokenWithPassword(): void {
-		$token = 'oldId';
+		$token = 'oldIdtokentokentokentoken';
 		$uid = 'user';
 		$user = 'User';
 		$password = 'password';
@@ -404,7 +404,7 @@ class PublicKeyTokenProviderTest extends TestCase {
 		$this->mapper
 			->expects($this->once())
 			->method('getToken')
-			->with(hash('sha512', 'oldId' . '1f4h9s'))
+			->with(hash('sha512', 'oldIdtokentokentokentoken' . '1f4h9s'))
 			->willReturn($oldToken);
 		$this->mapper
 			->expects($this->once())
@@ -416,7 +416,7 @@ class PublicKeyTokenProviderTest extends TestCase {
 					$token->getType() === IToken::DO_NOT_REMEMBER &&
 					$token->getLastActivity() === $this->time &&
 					$token->getPassword() !== null &&
-					$this->tokenProvider->getPassword($token, 'newId') === 'password';
+					$this->tokenProvider->getPassword($token, 'newIdtokentokentokentoken') === 'password';
 			}));
 		$this->mapper
 			->expects($this->once())
@@ -425,7 +425,7 @@ class PublicKeyTokenProviderTest extends TestCase {
 				return $token === $oldToken;
 			}));
 
-		$this->tokenProvider->renewSessionToken('oldId', 'newId');
+		$this->tokenProvider->renewSessionToken('oldIdtokentokentokentoken', 'newIdtokentokentokentoken');
 	}
 
 	public function testGetToken(): void {
@@ -438,37 +438,32 @@ class PublicKeyTokenProviderTest extends TestCase {
 		$this->mapper->method('getToken')
 			->with(
 				$this->callback(function (string $token) {
-					return hash('sha512', 'unhashedToken'.'1f4h9s') === $token;
+					return hash('sha512', 'unhashedTokentokentokentokentoken'.'1f4h9s') === $token;
 				})
 			)->willReturn($token);
 
-		$this->assertSame($token, $this->tokenProvider->getToken('unhashedToken'));
+		$this->assertSame($token, $this->tokenProvider->getToken('unhashedTokentokentokentokentoken'));
 	}
 
 	public function testGetInvalidToken() {
 		$this->expectException(InvalidTokenException::class);
 
-		$this->mapper->expects($this->at(0))
-			->method('getToken')
-			->with(
-				$this->callback(function (string $token): bool {
-					return hash('sha512', 'unhashedToken'.'1f4h9s') === $token;
-				})
-			)->willThrowException(new DoesNotExistException('nope'));
-
-		$this->mapper->expects($this->at(1))
+		$this->mapper->expects($this->exactly(2))
 			->method('getToken')
-			->with(
-				$this->callback(function (string $token): bool {
-					return hash('sha512', 'unhashedToken') === $token;
-				})
+			->withConsecutive(
+				[$this->callback(function (string $token): bool {
+					return hash('sha512', 'unhashedTokentokentokentokentoken'.'1f4h9s') === $token;
+				})],
+				[$this->callback(function (string $token): bool {
+					return hash('sha512', 'unhashedTokentokentokentokentoken') === $token;
+				})]
 			)->willThrowException(new DoesNotExistException('nope'));
 
-		$this->tokenProvider->getToken('unhashedToken');
+		$this->tokenProvider->getToken('unhashedTokentokentokentokentoken');
 	}
 
 	public function testGetExpiredToken() {
-		$token = 'token';
+		$token = 'tokentokentokentokentoken';
 		$uid = 'user';
 		$user = 'User';
 		$password = 'passme';
@@ -481,12 +476,12 @@ class PublicKeyTokenProviderTest extends TestCase {
 		$this->mapper->method('getToken')
 			->with(
 				$this->callback(function (string $token) {
-					return hash('sha512', 'token'.'1f4h9s') === $token;
+					return hash('sha512', 'tokentokentokentokentoken'.'1f4h9s') === $token;
 				})
 			)->willReturn($actual);
 
 		try {
-			$this->tokenProvider->getToken('token');
+			$this->tokenProvider->getToken('tokentokentokentokentoken');
 			$this->fail();
 		} catch (ExpiredTokenException $e) {
 			$this->assertSame($actual, $e->getToken());
@@ -533,7 +528,7 @@ class PublicKeyTokenProviderTest extends TestCase {
 	}
 
 	public function testRotate() {
-		$token = 'oldtoken';
+		$token = 'oldtokentokentokentokentoken';
 		$uid = 'user';
 		$user = 'User';
 		$password = 'password';
@@ -546,13 +541,13 @@ class PublicKeyTokenProviderTest extends TestCase {
 			]);
 		$actual = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER);
 
-		$new = $this->tokenProvider->rotate($actual, 'oldtoken', 'newtoken');
+		$new = $this->tokenProvider->rotate($actual, 'oldtokentokentokentokentoken', 'newtokentokentokentokentoken');
 
-		$this->assertSame('password', $this->tokenProvider->getPassword($new, 'newtoken'));
+		$this->assertSame('password', $this->tokenProvider->getPassword($new, 'newtokentokentokentokentoken'));
 	}
 
 	public function testRotateNoPassword() {
-		$token = 'oldtoken';
+		$token = 'oldtokentokentokentokentoken';
 		$uid = 'user';
 		$user = 'User';
 		$password = null;
@@ -563,7 +558,7 @@ class PublicKeyTokenProviderTest extends TestCase {
 
 		$oldPrivate = $actual->getPrivateKey();
 
-		$new = $this->tokenProvider->rotate($actual, 'oldtoken', 'newtoken');
+		$new = $this->tokenProvider->rotate($actual, 'oldtokentokentokentokentoken', 'newtokentokentokentokentoken');
 
 		$newPrivate = $new->getPrivateKey();
 
@@ -595,7 +590,7 @@ class PublicKeyTokenProviderTest extends TestCase {
 	public function testUpdatePasswords() {
 		$uid = 'myUID';
 		$token1 = $this->tokenProvider->generateToken(
-			'foo',
+			'foobetokentokentokentoken',
 			$uid,
 			$uid,
 			'bar',
@@ -603,7 +598,7 @@ class PublicKeyTokenProviderTest extends TestCase {
 			IToken::PERMANENT_TOKEN,
 			IToken::REMEMBER);
 		$token2 = $this->tokenProvider->generateToken(
-			'foobar',
+			'foobartokentokentokentoken',
 			$uid,
 			$uid,
 			'bar',