test(LoginFlow): update tests for refactored service

Mostly just to accommodate internal exception string changes
One new test for the new defense-in-depth UID check

Signed-off-by: Josh <josh.t.richards@gmail.com>

core/Service/LoginFlowV2Service.php

@@ -25,66 +25,76 @@ use OCP\Security\ICrypto;
 use OCP\Security\ISecureRandom;
 use Psr\Log\LoggerInterface;
 
+/**
+ * Coordinates the Login Flow v2 token exchange.
+ *
+ * A flow stores temporary state for a login session, including a per-flow key pair.
+ * The app password is encrypted with the flow's public key and can later be recovered
+ * only by a client that holds the poll token needed to unlock the stored private key.
+ */
 class LoginFlowV2Service {
 	public function __construct(
-		private LoginFlowV2Mapper $mapper,
-		private ISecureRandom $random,
-		private ITimeFactory $time,
-		private IConfig $config,
-		private ICrypto $crypto,
-		private LoggerInterface $logger,
-		private IProvider $tokenProvider,
+		private readonly LoginFlowV2Mapper $mapper,
+		private readonly ISecureRandom $random,
+		private readonly ITimeFactory $time,
+		private readonly IConfig $config,
+		private readonly ICrypto $crypto,
+		private readonly LoggerInterface $logger,
+		private readonly IProvider $tokenProvider,
 	) {
 	}
 
 	/**
-	 * @param string $pollToken
-	 * @return LoginFlowV2Credentials
+	 * Returns the credentials for a completed login flow.
+	 *
+	 * The poll token is a one-time secret held by the client. It is used to look up
+	 * the flow and unlock the private key needed to decrypt the stored app password.
+	 * Once the credentials are available, the flow is consumed so the poll token
+	 * cannot be used again.
+	 *
 	 * @throws LoginFlowV2NotFoundException
 	 */
 	public function poll(string $pollToken): LoginFlowV2Credentials {
 		try {
-			$data = $this->mapper->getByPollToken($this->hashToken($pollToken));
+			$flow = $this->mapper->getByPollToken($this->hashToken($pollToken));
 		} catch (DoesNotExistException $e) {
 			throw new LoginFlowV2NotFoundException('Invalid token');
 		}
 
-		$loginName = $data->getLoginName();
-		$server = $data->getServer();
-		$appPassword = $data->getAppPassword();
+		$loginName = $flow->getLoginName();
+		$server = $flow->getServer();
+		$appPassword = $flow->getAppPassword();
 
 		if ($loginName === null || $server === null || $appPassword === null) {
 			throw new LoginFlowV2NotFoundException('Token not yet ready');
 		}
 
-		// Remove the data from the DB
-		$this->mapper->delete($data);
+		// Consume the flow so the poll token can only be used once.
+		$this->mapper->delete($flow);
 
 		try {
-			// Decrypt the apptoken
-			$privateKey = $this->crypto->decrypt($data->getPrivateKey(), $pollToken);
+			// Decrypt the stored private key using the poll token.
+			$unlockedPrivateKey = $this->crypto->decrypt($flow->getPrivateKey(), $pollToken);
 		} catch (\Exception) {
-			throw new LoginFlowV2NotFoundException('Apptoken could not be decrypted');
+			throw new LoginFlowV2NotFoundException('Private key could not be decrypted');
 		}
 
-		$appPassword = $this->decryptPassword($data->getAppPassword(), $privateKey);
-		if ($appPassword === null) {
-			throw new LoginFlowV2NotFoundException('Apptoken could not be decrypted');
+		// Decrypt the stored app password using the decrypted private key.
+		$decryptedAppPassword = $this->decryptPassword($flow->getAppPassword(), $unlockedPrivateKey);
+		if ($decryptedAppPassword === null) {
+			throw new LoginFlowV2NotFoundException('App password could not be decrypted');
 		}
 
-		return new LoginFlowV2Credentials($server, $loginName, $appPassword);
+		return new LoginFlowV2Credentials($server, $loginName, $decryptedAppPassword);
 	}
 
 	/**
-	 * @param string $loginToken
-	 * @return LoginFlowV2
+	 * Returns the flow for a login token, enforcing the allowed user-agent list.
+	 *
 	 * @throws LoginFlowV2NotFoundException
 	 * @throws LoginFlowV2ClientForbiddenException
 	 */
 	public function getByLoginToken(string $loginToken): LoginFlowV2 {
-		/** @var LoginFlowV2|null $flow */
-		$flow = null;
-
 		try {
 			$flow = $this->mapper->getByLoginToken($loginToken);
 		} catch (DoesNotExistException $e) {
@@ -109,32 +119,32 @@ class LoginFlowV2Service {
 	}
 
 	/**
-	 * @param string $loginToken
-	 * @return bool returns true if the start was successfull. False if not.
+	 * Marks the login flow as started.
+	 *
+	 * Returns false if the login token does not exist.
 	 */
 	public function startLoginFlow(string $loginToken): bool {
 		try {
-			$data = $this->mapper->getByLoginToken($loginToken);
+			$flow = $this->mapper->getByLoginToken($loginToken);
 		} catch (DoesNotExistException $e) {
 			return false;
 		}
 
-		$data->setStarted(1);
-		$this->mapper->update($data);
+		$flow->setStarted(1);
+		$this->mapper->update($flow);
 
 		return true;
 	}
 
 	/**
-	 * @param string $loginToken
-	 * @param string $sessionId
-	 * @param string $server
-	 * @param string $userId
-	 * @return bool true if the flow was successfully completed false otherwise
+	 * Completes the login flow by generating an app password for the authenticated session
+	 * and storing it encrypted with the flow's public key.
+	 *
+	 * Returns false if the login token or session token is invalid.
 	 */
 	public function flowDone(string $loginToken, string $sessionId, string $server, string $userId): bool {
 		try {
-			$data = $this->mapper->getByLoginToken($loginToken);
+			$flow = $this->mapper->getByLoginToken($loginToken);
 		} catch (DoesNotExistException $e) {
 			return false;
 		}
@@ -142,122 +152,163 @@ class LoginFlowV2Service {
 		try {
 			$sessionToken = $this->tokenProvider->getToken($sessionId);
 			$loginName = $sessionToken->getLoginName();
+
+			if ($sessionToken->getUID() !== $userId) {
+				return false;
+			}
+
 			try {
 				$password = $this->tokenProvider->getPassword($sessionToken, $sessionId);
 			} catch (PasswordlessTokenException $ex) {
+				// Some session tokens are passwordless, so no login password can be reused here.
 				$password = null;
 			}
 		} catch (InvalidTokenException $ex) {
 			return false;
 		}
 
-		$appPassword = $this->random->generate(72, ISecureRandom::CHAR_UPPER . ISecureRandom::CHAR_LOWER . ISecureRandom::CHAR_DIGITS);
+		$appPassword = $this->random->generate(72, ISecureRandom::CHAR_ALPHANUMERIC);
 		$this->tokenProvider->generateToken(
 			$appPassword,
 			$userId,
 			$loginName,
 			$password,
-			$data->getClientName(),
+			$flow->getClientName(),
 			IToken::PERMANENT_TOKEN,
 			IToken::DO_NOT_REMEMBER
 		);
 
-		$data->setLoginName($loginName);
-		$data->setServer($server);
+		$flow->setLoginName($loginName);
+		$flow->setServer($server);
 
-		// Properly encrypt
-		$data->setAppPassword($this->encryptPassword($appPassword, $data->getPublicKey()));
+		$encryptedAppPassword = $this->encryptPassword($appPassword, $flow->getPublicKey());
+		$flow->setAppPassword($encryptedAppPassword);
 
-		$this->mapper->update($data);
+		$this->mapper->update($flow);
 		return true;
 	}
 
+	/**
+	 * Completes the login flow with an app password that has already been created by the caller,
+	 * storing it encrypted with the flow's public key.
+	 *
+	 * Returns false if the login token does not exist.
+	 */
 	public function flowDoneWithAppPassword(string $loginToken, string $server, string $loginName, string $appPassword): bool {
 		try {
-			$data = $this->mapper->getByLoginToken($loginToken);
+			$flow = $this->mapper->getByLoginToken($loginToken);
 		} catch (DoesNotExistException $e) {
 			return false;
 		}
 
-		$data->setLoginName($loginName);
-		$data->setServer($server);
+		$flow->setLoginName($loginName);
+		$flow->setServer($server);
 
-		// Properly encrypt
-		$data->setAppPassword($this->encryptPassword($appPassword, $data->getPublicKey()));
+		$encryptedAppPassword = $this->encryptPassword($appPassword, $flow->getPublicKey());
+		$flow->setAppPassword($encryptedAppPassword);
 
-		$this->mapper->update($data);
+		$this->mapper->update($flow);
 		return true;
 	}
 
+	/**
+	 * Creates a new login flow with fresh poll and login tokens and a dedicated key pair.
+	 *
+	 * The poll token is returned only to the polling client. The generated private key is
+	 * encrypted with that poll token, and the corresponding public key is later used to
+	 * encrypt the app password before it is stored in the flow.
+	 */
 	public function createTokens(string $userAgent): LoginFlowV2Tokens {
 		$flow = new LoginFlowV2();
-		$pollToken = $this->random->generate(128, ISecureRandom::CHAR_DIGITS . ISecureRandom::CHAR_LOWER . ISecureRandom::CHAR_UPPER);
-		$loginToken = $this->random->generate(128, ISecureRandom::CHAR_DIGITS . ISecureRandom::CHAR_LOWER . ISecureRandom::CHAR_UPPER);
+		$pollToken = $this->random->generate(128, ISecureRandom::CHAR_ALPHANUMERIC);
+		$loginToken = $this->random->generate(128, ISecureRandom::CHAR_ALPHANUMERIC);
+
+		// Store the poll token only as a hash because it is later presented as a bearer secret.
+		// The login token must remain retrievable in plain form because it is looked up directly.
 		$flow->setPollToken($this->hashToken($pollToken));
 		$flow->setLoginToken($loginToken);
 		$flow->setStarted(0);
 		$flow->setTimestamp($this->time->getTime());
 		$flow->setClientName($userAgent);
 
-		[$publicKey, $privateKey] = $this->getKeyPair();
-		$privateKey = $this->crypto->encrypt($privateKey, $pollToken);
+		['publicKey' => $publicKey, 'privateKey' => $privateKey] = $this->getKeyPair();
+		$encryptedPrivateKey = $this->crypto->encrypt($privateKey, $pollToken);
 
 		$flow->setPublicKey($publicKey);
-		$flow->setPrivateKey($privateKey);
+		$flow->setPrivateKey($encryptedPrivateKey);
 
 		$this->mapper->insert($flow);
 
 		return new LoginFlowV2Tokens($loginToken, $pollToken);
 	}
 
+	/**
+	 * Hashes a poll token with the instance secret before persisting or looking it up.
+	 */
 	private function hashToken(string $token): string {
+		// Intentionally no default: the instance secret must be configured.
 		$secret = $this->config->getSystemValue('secret');
 		return hash('sha512', $token . $secret);
 	}
 
+	/**
+	 * Generates an RSA key pair for encrypting the app password during the flow.
+	 *
+	 * @return array{publicKey: string, privateKey: string}
+	 */
 	private function getKeyPair(): array {
 		$config = array_merge([
 			'digest_alg' => 'sha512',
 			'private_key_bits' => 2048,
 		], $this->config->getSystemValue('openssl', []));
 
-		// Generate new key
-		$res = openssl_pkey_new($config);
-		if ($res === false) {
+		// Generate a fresh RSA key pair for this flow.
+		$keyPair = openssl_pkey_new($config);
+		if ($keyPair === false) {
 			$this->logOpensslError();
 			throw new \RuntimeException('Could not initialize keys');
 		}
 
-		if (openssl_pkey_export($res, $privateKey, null, $config) === false) {
+		if (openssl_pkey_export($keyPair, $privateKey, null, $config) === false) {
 			$this->logOpensslError();
 			throw new \RuntimeException('OpenSSL reported a problem');
 		}
 
-		// Extract the public key from $res to $pubKey
-		$publicKey = openssl_pkey_get_details($res);
-		$publicKey = $publicKey['key'];
+		// Extract the PEM-encoded public key from the generated key pair.
+		$publicKeyDetails = openssl_pkey_get_details($keyPair);
+		$publicKey = $publicKeyDetails['key'];
 
-		return [$publicKey, $privateKey];
+		return [
+			'publicKey' => $publicKey,
+			'privateKey' => $privateKey,
+		];
 	}
 
 	private function logOpensslError(): void {
+		// Drain the OpenSSL error queue so the root cause is visible in server logs.
 		$errors = [];
 		while ($error = openssl_error_string()) {
 			$errors[] = $error;
 		}
-		$this->logger->critical('Something is wrong with your openssl setup: ' . implode(', ', $errors));
+		$this->logger->critical('OpenSSL error(s): ' . implode('; ', $errors));
 	}
 
+	/**
+	 * Encrypts a password with an RSA public key and returns it base64-encoded.
+	 */
 	private function encryptPassword(string $password, string $publicKey): string {
 		openssl_public_encrypt($password, $encryptedPassword, $publicKey, OPENSSL_PKCS1_OAEP_PADDING);
-		$encryptedPassword = base64_encode($encryptedPassword);
+		$encoded = base64_encode($encryptedPassword);
 
-		return $encryptedPassword;
+		return $encoded;
 	}
 
+	/**
+	 * Decrypts a base64-encoded RSA-encrypted password, returning null on failure.
+	 */
 	private function decryptPassword(string $encryptedPassword, string $privateKey): ?string {
-		$encryptedPassword = base64_decode($encryptedPassword);
-		$success = openssl_private_decrypt($encryptedPassword, $password, $privateKey, OPENSSL_PKCS1_OAEP_PADDING);
+		$decoded = base64_decode($encryptedPassword);
+		$success = openssl_private_decrypt($decoded, $password, $privateKey, OPENSSL_PKCS1_OAEP_PADDING);
 
 		return $success ? $password : null;
 	}

tests/Core/Service/LoginFlowV2ServiceUnitTest.php

@@ -104,7 +104,7 @@ class LoginFlowV2ServiceUnitTest extends TestCase {
 	 */
 	public function testPollPrivateKeyCouldNotBeDecrypted(): void {
 		$this->expectException(LoginFlowV2NotFoundException::class);
-		$this->expectExceptionMessage('Apptoken could not be decrypted');
+		$this->expectExceptionMessage('Private key could not be decrypted');
 
 		$this->crypto->expects($this->once())
 			->method('decrypt')
@@ -127,9 +127,9 @@ class LoginFlowV2ServiceUnitTest extends TestCase {
 		$this->subjectUnderTest->poll('');
 	}
 
-	public function testPollApptokenCouldNotBeDecrypted(): void {
+	public function testPollAppPasswordCouldNotBeDecrypted(): void {
 		$this->expectException(LoginFlowV2NotFoundException::class);
-		$this->expectExceptionMessage('Apptoken could not be decrypted');
+		$this->expectExceptionMessage('App password could not be decrypted');
 
 		/*
 		 * Cannot be mocked, because functions like getLoginName are magic functions.
@@ -352,7 +352,7 @@ class LoginFlowV2ServiceUnitTest extends TestCase {
 
 		$this->secureRandom->expects($this->once())
 			->method('generate')
-			->with(72, ISecureRandom::CHAR_UPPER . ISecureRandom::CHAR_LOWER . ISecureRandom::CHAR_DIGITS)
+			->with(72, ISecureRandom::CHAR_ALPHANUMERIC)
 			->willReturn('test_pass');
 
 		// session token
@@ -360,6 +360,10 @@ class LoginFlowV2ServiceUnitTest extends TestCase {
 		$sessionToken->expects($this->once())
 			->method('getLoginName')
 			->willReturn('login_name');
+		
+		$sessionToken->expects($this->once())
+			->method('getUID')
+			->willReturn('user_id');
 
 		$this->tokenProvider->expects($this->once())
 			->method('getPassword')
@@ -424,6 +428,36 @@ class LoginFlowV2ServiceUnitTest extends TestCase {
 		$this->assertFalse($result);
 	}
 
+	public function testFlowDoneReturnsFalseWhenSessionUserDoesNotMatch(): void {
+		$loginFlowV2 = new LoginFlowV2();
+		$loginFlowV2->setPublicKey('public');
+		$loginFlowV2->setClientName('client_name');
+
+		$this->mapper->expects($this->once())
+			->method('getByLoginToken')
+			->willReturn($loginFlowV2);
+
+		$sessionToken = $this->getMockBuilder(IToken::class)->disableOriginalConstructor()->getMock();
+		$sessionToken->expects($this->once())
+			->method('getLoginName')
+			->willReturn('login_name');
+		$sessionToken->expects($this->once())
+			->method('getUID')
+			->willReturn('different_user_id');
+
+		$this->tokenProvider->expects($this->once())
+			->method('getToken')
+			->willReturn($sessionToken);
+
+		$this->tokenProvider->expects($this->never())->method('getPassword');
+		$this->tokenProvider->expects($this->never())->method('generateToken');
+		$this->mapper->expects($this->never())->method('update');
+
+		$result = $this->subjectUnderTest->flowDone('login_token', 'session_id', 'server', 'user_id');
+
+		$this->assertFalse($result);
+	}
+
 	/*
 	 * Tests for createTokens
 	 */