From 1da54ae7d737b36b4a1ba2871cdb5e830e52fec9 Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 3 Apr 2026 09:49:25 -0400 Subject: [PATCH 1/8] refactor(LoginFlow): add comments and rename variables for clarity in service Signed-off-by: Josh --- core/Service/LoginFlowV2Service.php | 134 ++++++++++++++++++---------- 1 file changed, 87 insertions(+), 47 deletions(-) diff --git a/core/Service/LoginFlowV2Service.php b/core/Service/LoginFlowV2Service.php index 13bd18e0ffa0d..b2552d64c0cd8 100644 --- a/core/Service/LoginFlowV2Service.php +++ b/core/Service/LoginFlowV2Service.php @@ -25,54 +25,67 @@ 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); } /** @@ -109,32 +122,32 @@ public function getByLoginToken(string $loginToken): LoginFlowV2 { } /** - * @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; } @@ -145,6 +158,7 @@ public function flowDone(string $loginToken, string $sessionId, string $server, 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) { @@ -157,42 +171,58 @@ public function flowDone(string $loginToken, string $sessionId, string $server, $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 + * and 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); + + // 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); @@ -210,18 +240,27 @@ public function createTokens(string $userAgent): LoginFlowV2Tokens { 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{0: string, 1: string} [publicKey, privateKey] + */ private function getKeyPair(): array { $config = array_merge([ 'digest_alg' => 'sha512', 'private_key_bits' => 2048, ], $this->config->getSystemValue('openssl', [])); - // Generate new key + // Generate a fresh RSA key pair for this flow. $res = openssl_pkey_new($config); if ($res === false) { $this->logOpensslError(); @@ -233,7 +272,7 @@ private function getKeyPair(): array { throw new \RuntimeException('OpenSSL reported a problem'); } - // Extract the public key from $res to $pubKey + // Extract the PEM-encoded public key from the generated key pair. $publicKey = openssl_pkey_get_details($res); $publicKey = $publicKey['key']; @@ -241,6 +280,7 @@ private function getKeyPair(): array { } 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; From a3275026fb7d82b070ce9bc06c96da6a32546710 Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 3 Apr 2026 09:58:48 -0400 Subject: [PATCH 2/8] refactor(LoginFlow(: slightly streamline getByLoginToken Signed-off-by: Josh --- core/Service/LoginFlowV2Service.php | 3 --- 1 file changed, 3 deletions(-) diff --git a/core/Service/LoginFlowV2Service.php b/core/Service/LoginFlowV2Service.php index b2552d64c0cd8..c5677eca6214a 100644 --- a/core/Service/LoginFlowV2Service.php +++ b/core/Service/LoginFlowV2Service.php @@ -95,9 +95,6 @@ public function poll(string $pollToken): LoginFlowV2Credentials { * @throws LoginFlowV2ClientForbiddenException */ public function getByLoginToken(string $loginToken): LoginFlowV2 { - /** @var LoginFlowV2|null $flow */ - $flow = null; - try { $flow = $this->mapper->getByLoginToken($loginToken); } catch (DoesNotExistException $e) { From 68a240126fcd70fd2ba8bcd53aed04b6b4194197 Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 3 Apr 2026 11:19:10 -0400 Subject: [PATCH 3/8] refactor(LoginFlow): improve readability of crypto functions in service Signed-off-by: Josh --- core/Service/LoginFlowV2Service.php | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/core/Service/LoginFlowV2Service.php b/core/Service/LoginFlowV2Service.php index c5677eca6214a..3fee603626106 100644 --- a/core/Service/LoginFlowV2Service.php +++ b/core/Service/LoginFlowV2Service.php @@ -258,20 +258,20 @@ private function getKeyPair(): array { ], $this->config->getSystemValue('openssl', [])); // Generate a fresh RSA key pair for this flow. - $res = openssl_pkey_new($config); - if ($res === false) { + $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 PEM-encoded public key from the generated key pair. - $publicKey = openssl_pkey_get_details($res); - $publicKey = $publicKey['key']; + $publicKeyDetails = openssl_pkey_get_details($keyPair); + $publicKey = $publicKeyDetails['key']; return [$publicKey, $privateKey]; } @@ -287,14 +287,14 @@ private function logOpensslError(): void { 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; } 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; } From aa4b02b07493bdeb8f19f9bc7de2c7c32ba82ba5 Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 3 Apr 2026 11:42:48 -0400 Subject: [PATCH 4/8] refactor(LoginFlow): give getKeyPair() a named shape return value for clarity Signed-off-by: Josh --- core/Service/LoginFlowV2Service.php | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/core/Service/LoginFlowV2Service.php b/core/Service/LoginFlowV2Service.php index 3fee603626106..f44add9ab91a1 100644 --- a/core/Service/LoginFlowV2Service.php +++ b/core/Service/LoginFlowV2Service.php @@ -226,11 +226,11 @@ public function createTokens(string $userAgent): LoginFlowV2Tokens { $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); @@ -249,7 +249,7 @@ private function hashToken(string $token): string { /** * Generates an RSA key pair for encrypting the app password during the flow. * - * @return array{0: string, 1: string} [publicKey, privateKey] + * @return array{publicKey: string, privateKey: string} */ private function getKeyPair(): array { $config = array_merge([ @@ -273,7 +273,10 @@ private function getKeyPair(): array { $publicKeyDetails = openssl_pkey_get_details($keyPair); $publicKey = $publicKeyDetails['key']; - return [$publicKey, $privateKey]; + return [ + 'publicKey' => $publicKey, + 'privateKey' => $privateKey, + ]; } private function logOpensslError(): void { From 58a33b4d54918c043151abf6110cf37ed7cbbedd Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 3 Apr 2026 11:54:43 -0400 Subject: [PATCH 5/8] chore(LoginFlow): improve grammar Signed-off-by: Josh --- core/Service/LoginFlowV2Service.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/Service/LoginFlowV2Service.php b/core/Service/LoginFlowV2Service.php index f44add9ab91a1..9b045f9cdc0d1 100644 --- a/core/Service/LoginFlowV2Service.php +++ b/core/Service/LoginFlowV2Service.php @@ -184,8 +184,8 @@ public function flowDone(string $loginToken, string $sessionId, string $server, } /** - * Completes the login flow with an app password that has already been created by the caller - * and storing it encrypted with the flow's public key. + * 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. */ From 29ca192f9011392f8acd65872992f30286236617 Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 3 Apr 2026 12:14:00 -0400 Subject: [PATCH 6/8] refactor(LoginFlow): use existing ISecureRandom::CHAR_ALPHANUMERIC constant 100% equivalent Signed-off-by: Josh --- core/Service/LoginFlowV2Service.php | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/core/Service/LoginFlowV2Service.php b/core/Service/LoginFlowV2Service.php index 9b045f9cdc0d1..485957e4a8864 100644 --- a/core/Service/LoginFlowV2Service.php +++ b/core/Service/LoginFlowV2Service.php @@ -89,8 +89,8 @@ public function poll(string $pollToken): LoginFlowV2Credentials { } /** - * @param string $loginToken - * @return LoginFlowV2 + * Returns the flow for a login token, enforcing the allowed user-agent list. + * * @throws LoginFlowV2NotFoundException * @throws LoginFlowV2ClientForbiddenException */ @@ -162,7 +162,7 @@ public function flowDone(string $loginToken, string $sessionId, string $server, 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, @@ -215,8 +215,8 @@ public function flowDoneWithAppPassword(string $loginToken, string $server, stri */ 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. @@ -285,9 +285,12 @@ private function logOpensslError(): void { 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); $encoded = base64_encode($encryptedPassword); @@ -295,6 +298,9 @@ private function encryptPassword(string $password, string $publicKey): string { return $encoded; } + /** + * Decrypts a base64-encoded RSA-encrypted password, returning null on failure. + */ private function decryptPassword(string $encryptedPassword, string $privateKey): ?string { $decoded = base64_decode($encryptedPassword); $success = openssl_private_decrypt($decoded, $password, $privateKey, OPENSSL_PKCS1_OAEP_PADDING); From d33a0078d40ef220ba5cdad9b80ae96ef2a7dc2c Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 3 Apr 2026 12:43:12 -0400 Subject: [PATCH 7/8] fix(LoginFlow): defense-in-depth for UID in flowDone() Signed-off-by: Josh --- core/Service/LoginFlowV2Service.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/core/Service/LoginFlowV2Service.php b/core/Service/LoginFlowV2Service.php index 485957e4a8864..bcf9c2ab55210 100644 --- a/core/Service/LoginFlowV2Service.php +++ b/core/Service/LoginFlowV2Service.php @@ -152,6 +152,11 @@ public function flowDone(string $loginToken, string $sessionId, string $server, 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) { From 34707875d08fb7635d7426aad91d20682585c9e1 Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 3 Apr 2026 13:12:06 -0400 Subject: [PATCH 8/8] 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 --- .../Service/LoginFlowV2ServiceUnitTest.php | 42 +++++++++++++++++-- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/tests/Core/Service/LoginFlowV2ServiceUnitTest.php b/tests/Core/Service/LoginFlowV2ServiceUnitTest.php index 625b5f3cd1175..b88d75ad176a7 100644 --- a/tests/Core/Service/LoginFlowV2ServiceUnitTest.php +++ b/tests/Core/Service/LoginFlowV2ServiceUnitTest.php @@ -104,7 +104,7 @@ private function getOpenSSLEncryptedPublicAndPrivateKey(string $appPassword): ar */ 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 @@ public function testPollPrivateKeyCouldNotBeDecrypted(): void { $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 @@ public function testFlowDone(): void { $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 @@ public function testFlowDone(): void { $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 @@ public function testFlowDonePasswordlessTokenException(): void { $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 */