From 26543ac9869b545648acec73218424d536075bef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 21 May 2026 15:30:31 +0200 Subject: [PATCH 1/2] fix: Use token expiration for ephemeral sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This simplifies the code a lot. Signed-off-by: Côme Chilliet --- .../ClientFlowLoginV2Controller.php | 2 - lib/composer/composer/autoload_classmap.php | 2 - lib/composer/composer/autoload_static.php | 2 - .../DependencyInjection/DIContainer.php | 3 - .../FlowV2EphemeralSessionsMiddleware.php | 74 ------------------- lib/private/Authentication/Login/Chain.php | 2 - .../Login/CreateSessionTokenCommand.php | 30 +++++--- .../Login/FlowV2EphemeralSessionsCommand.php | 32 -------- .../Authentication/Token/IProvider.php | 1 + lib/private/Authentication/Token/Manager.php | 2 + .../Token/PublicKeyTokenProvider.php | 12 ++- lib/private/User/Session.php | 23 +++--- 12 files changed, 44 insertions(+), 141 deletions(-) delete mode 100644 lib/private/AppFramework/Middleware/FlowV2EphemeralSessionsMiddleware.php delete mode 100644 lib/private/Authentication/Login/FlowV2EphemeralSessionsCommand.php diff --git a/core/Controller/ClientFlowLoginV2Controller.php b/core/Controller/ClientFlowLoginV2Controller.php index 8c0c1e8179d92..170743c4c4c13 100644 --- a/core/Controller/ClientFlowLoginV2Controller.php +++ b/core/Controller/ClientFlowLoginV2Controller.php @@ -44,8 +44,6 @@ class ClientFlowLoginV2Controller extends Controller { public const TOKEN_NAME = 'client.flow.v2.login.token'; public const STATE_NAME = 'client.flow.v2.state.token'; - // Denotes that the session was created for the login flow and should therefore be ephemeral. - public const EPHEMERAL_NAME = 'client.flow.v2.state.ephemeral'; public function __construct( string $appName, diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index c8bab9ba0da0a..aa4e54cf997ef 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1052,7 +1052,6 @@ 'OC\\AppFramework\\Http\\RequestId' => $baseDir . '/lib/private/AppFramework/Http/RequestId.php', 'OC\\AppFramework\\Middleware\\AdditionalScriptsMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/AdditionalScriptsMiddleware.php', 'OC\\AppFramework\\Middleware\\CompressionMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/CompressionMiddleware.php', - 'OC\\AppFramework\\Middleware\\FlowV2EphemeralSessionsMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/FlowV2EphemeralSessionsMiddleware.php', 'OC\\AppFramework\\Middleware\\MiddlewareDispatcher' => $baseDir . '/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php', 'OC\\AppFramework\\Middleware\\NotModifiedMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/NotModifiedMiddleware.php', 'OC\\AppFramework\\Middleware\\OCSMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/OCSMiddleware.php', @@ -1149,7 +1148,6 @@ 'OC\\Authentication\\Login\\CompleteLoginCommand' => $baseDir . '/lib/private/Authentication/Login/CompleteLoginCommand.php', 'OC\\Authentication\\Login\\CreateSessionTokenCommand' => $baseDir . '/lib/private/Authentication/Login/CreateSessionTokenCommand.php', 'OC\\Authentication\\Login\\FinishRememberedLoginCommand' => $baseDir . '/lib/private/Authentication/Login/FinishRememberedLoginCommand.php', - 'OC\\Authentication\\Login\\FlowV2EphemeralSessionsCommand' => $baseDir . '/lib/private/Authentication/Login/FlowV2EphemeralSessionsCommand.php', 'OC\\Authentication\\Login\\LoggedInCheckCommand' => $baseDir . '/lib/private/Authentication/Login/LoggedInCheckCommand.php', 'OC\\Authentication\\Login\\LoginData' => $baseDir . '/lib/private/Authentication/Login/LoginData.php', 'OC\\Authentication\\Login\\LoginResult' => $baseDir . '/lib/private/Authentication/Login/LoginResult.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 61769759115c4..0dea7fd24d5a0 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1093,7 +1093,6 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\AppFramework\\Http\\RequestId' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/RequestId.php', 'OC\\AppFramework\\Middleware\\AdditionalScriptsMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/AdditionalScriptsMiddleware.php', 'OC\\AppFramework\\Middleware\\CompressionMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/CompressionMiddleware.php', - 'OC\\AppFramework\\Middleware\\FlowV2EphemeralSessionsMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/FlowV2EphemeralSessionsMiddleware.php', 'OC\\AppFramework\\Middleware\\MiddlewareDispatcher' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php', 'OC\\AppFramework\\Middleware\\NotModifiedMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/NotModifiedMiddleware.php', 'OC\\AppFramework\\Middleware\\OCSMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/OCSMiddleware.php', @@ -1190,7 +1189,6 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Authentication\\Login\\CompleteLoginCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/CompleteLoginCommand.php', 'OC\\Authentication\\Login\\CreateSessionTokenCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/CreateSessionTokenCommand.php', 'OC\\Authentication\\Login\\FinishRememberedLoginCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/FinishRememberedLoginCommand.php', - 'OC\\Authentication\\Login\\FlowV2EphemeralSessionsCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/FlowV2EphemeralSessionsCommand.php', 'OC\\Authentication\\Login\\LoggedInCheckCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/LoggedInCheckCommand.php', 'OC\\Authentication\\Login\\LoginData' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/LoginData.php', 'OC\\Authentication\\Login\\LoginResult' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/LoginResult.php', diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 76261fe6b92c5..188234ae96880 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -15,7 +15,6 @@ use OC\AppFramework\Http\Output; use OC\AppFramework\Middleware\AdditionalScriptsMiddleware; use OC\AppFramework\Middleware\CompressionMiddleware; -use OC\AppFramework\Middleware\FlowV2EphemeralSessionsMiddleware; use OC\AppFramework\Middleware\MiddlewareDispatcher; use OC\AppFramework\Middleware\NotModifiedMiddleware; use OC\AppFramework\Middleware\OCSMiddleware; @@ -189,8 +188,6 @@ public function __construct(string $appName, array $urlParams = [], ?ServerConta $dispatcher->registerMiddleware($c->get(CORSMiddleware::class)); $dispatcher->registerMiddleware($c->get(OCSMiddleware::class)); - $dispatcher->registerMiddleware($c->get(FlowV2EphemeralSessionsMiddleware::class)); - $securityMiddleware = new SecurityMiddleware( $c->get(IRequest::class), $c->get(IControllerMethodReflector::class), diff --git a/lib/private/AppFramework/Middleware/FlowV2EphemeralSessionsMiddleware.php b/lib/private/AppFramework/Middleware/FlowV2EphemeralSessionsMiddleware.php deleted file mode 100644 index 658a120320e2f..0000000000000 --- a/lib/private/AppFramework/Middleware/FlowV2EphemeralSessionsMiddleware.php +++ /dev/null @@ -1,74 +0,0 @@ -session->get(ClientFlowLoginV2Controller::EPHEMERAL_NAME); - - // Not an ephemeral session. - if ($sessionCreationTime === null) { - return; - } - - // Lax enforcement until TTL is reached. - if ($this->timeFactory->getTime() < $sessionCreationTime + self::EPHEMERAL_SESSION_TTL) { - return; - } - - // Allow certain controllers/methods to proceed without logging out. - if ( - $controller instanceof ClientFlowLoginV2Controller - && ($methodName === 'grantPage' || $methodName === 'generateAppPassword') - ) { - return; - } - - if ($controller instanceof TwoFactorChallengeController - || $controller instanceof ALoginSetupController) { - return; - } - - if ($this->reflector->hasAnnotationOrAttribute('PublicPage', PublicPage::class)) { - return; - } - - $this->logger->info('Closing user and PHP session for ephemeral session', [ - 'controller' => $controller::class, - 'method' => $methodName, - ]); - $this->userSession->logout(); - $this->session->close(); - } -} diff --git a/lib/private/Authentication/Login/Chain.php b/lib/private/Authentication/Login/Chain.php index fc90d9225a7cd..baf59195b971f 100644 --- a/lib/private/Authentication/Login/Chain.php +++ b/lib/private/Authentication/Login/Chain.php @@ -21,7 +21,6 @@ public function __construct( private SetUserTimezoneCommand $setUserTimezoneCommand, private TwoFactorCommand $twoFactorCommand, private FinishRememberedLoginCommand $finishRememberedLoginCommand, - private FlowV2EphemeralSessionsCommand $flowV2EphemeralSessionsCommand, ) { } @@ -32,7 +31,6 @@ public function process(LoginData $loginData): LoginResult { ->setNext($this->uidLoginCommand) ->setNext($this->loggedInCheckCommand) ->setNext($this->completeLoginCommand) - ->setNext($this->flowV2EphemeralSessionsCommand) ->setNext($this->createSessionTokenCommand) ->setNext($this->clearLostPasswordTokensCommand) ->setNext($this->updateLastPasswordConfirmCommand) diff --git a/lib/private/Authentication/Login/CreateSessionTokenCommand.php b/lib/private/Authentication/Login/CreateSessionTokenCommand.php index 21f0433d948c4..db01084544a3c 100644 --- a/lib/private/Authentication/Login/CreateSessionTokenCommand.php +++ b/lib/private/Authentication/Login/CreateSessionTokenCommand.php @@ -10,19 +10,19 @@ use OC\Authentication\Token\IToken; use OC\User\Session; +use OCP\AppFramework\Utility\ITimeFactory; use OCP\IConfig; +use OCP\IURLGenerator; class CreateSessionTokenCommand extends ALoginCommand { - /** @var IConfig */ - private $config; + private const EPHEMERAL_SESSION_TTL = 5 * 60; // 5 minutes - /** @var Session */ - private $userSession; - - public function __construct(IConfig $config, - Session $userSession) { - $this->config = $config; - $this->userSession = $userSession; + public function __construct( + private IConfig $config, + private Session $userSession, + private IURLGenerator $urlGenerator, + private ITimeFactory $timeFactory, + ) { } public function process(LoginData $loginData): LoginResult { @@ -35,13 +35,20 @@ public function process(LoginData $loginData): LoginResult { $tokenType = IToken::DO_NOT_REMEMBER; } + $loginV2GrantRoute = $this->urlGenerator->linkToRoute('core.ClientFlowLoginV2.grantPage'); + $expires = null; + if (str_starts_with($loginData->getRedirectUrl() ?? '', $loginV2GrantRoute)) { + $expires = $this->timeFactory->getTime() + self::EPHEMERAL_SESSION_TTL; + } + if ($loginData->getPassword() === '') { $this->userSession->createSessionToken( $loginData->getRequest(), $loginData->getUser()->getUID(), $loginData->getUsername(), null, - $tokenType + $tokenType, + $expires, ); $this->userSession->updateTokens( $loginData->getUser()->getUID(), @@ -53,7 +60,8 @@ public function process(LoginData $loginData): LoginResult { $loginData->getUser()->getUID(), $loginData->getUsername(), $loginData->getPassword(), - $tokenType + $tokenType, + $expires, ); $this->userSession->updateTokens( $loginData->getUser()->getUID(), diff --git a/lib/private/Authentication/Login/FlowV2EphemeralSessionsCommand.php b/lib/private/Authentication/Login/FlowV2EphemeralSessionsCommand.php deleted file mode 100644 index 362aab7933e39..0000000000000 --- a/lib/private/Authentication/Login/FlowV2EphemeralSessionsCommand.php +++ /dev/null @@ -1,32 +0,0 @@ -urlGenerator->linkToRoute('core.ClientFlowLoginV2.grantPage'); - if (str_starts_with($loginData->getRedirectUrl() ?? '', $loginV2GrantRoute)) { - $this->session->set(ClientFlowLoginV2Controller::EPHEMERAL_NAME, $this->timeFactory->getTime()); - } - - return $this->processNextOrFinishSuccessfully($loginData); - } -} diff --git a/lib/private/Authentication/Token/IProvider.php b/lib/private/Authentication/Token/IProvider.php index d47427e79bf06..04a24bc559e9f 100644 --- a/lib/private/Authentication/Token/IProvider.php +++ b/lib/private/Authentication/Token/IProvider.php @@ -37,6 +37,7 @@ public function generateToken(string $token, int $type = OCPIToken::TEMPORARY_TOKEN, int $remember = OCPIToken::DO_NOT_REMEMBER, ?array $scope = null, + ?int $expires = null, ): OCPIToken; /** diff --git a/lib/private/Authentication/Token/Manager.php b/lib/private/Authentication/Token/Manager.php index b55970a497908..2fa0c5e6e1d49 100644 --- a/lib/private/Authentication/Token/Manager.php +++ b/lib/private/Authentication/Token/Manager.php @@ -44,6 +44,7 @@ public function generateToken(string $token, int $type = OCPIToken::TEMPORARY_TOKEN, int $remember = OCPIToken::DO_NOT_REMEMBER, ?array $scope = null, + ?int $expires = null, ): OCPIToken { if (mb_strlen($name) > 128) { $name = mb_substr($name, 0, 120) . '…'; @@ -59,6 +60,7 @@ public function generateToken(string $token, $type, $remember, $scope, + $expires, ); } catch (Exception $e) { if ($e->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { diff --git a/lib/private/Authentication/Token/PublicKeyTokenProvider.php b/lib/private/Authentication/Token/PublicKeyTokenProvider.php index 87063f5ccd1c5..74416af96891a 100644 --- a/lib/private/Authentication/Token/PublicKeyTokenProvider.php +++ b/lib/private/Authentication/Token/PublicKeyTokenProvider.php @@ -94,6 +94,7 @@ public function generateToken(string $token, int $type = OCPIToken::TEMPORARY_TOKEN, int $remember = OCPIToken::DO_NOT_REMEMBER, ?array $scope = null, + ?int $expires = null, ): OCPIToken { 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'); @@ -110,7 +111,7 @@ public function generateToken(string $token, $randomOldToken = $this->mapper->getFirstTokenForUser($uid); $oldTokenMatches = $randomOldToken && $randomOldToken->getPasswordHash() && $password !== null && $this->hasher->verify(sha1($password) . $password, $randomOldToken->getPasswordHash()); - $dbToken = $this->newToken($token, $uid, $loginName, $password, $name, $type, $remember); + $dbToken = $this->newToken($token, $uid, $loginName, $password, $name, $type, $remember, $expires); if ($oldTokenMatches) { $dbToken->setPasswordHash($randomOldToken->getPasswordHash()); @@ -258,6 +259,7 @@ public function renewSessionToken(string $oldSessionId, string $sessionId): OCPI OCPIToken::TEMPORARY_TOKEN, $token->getRemember(), $scope, + $token->getExpires(), ); $this->cacheToken($newToken); @@ -460,7 +462,9 @@ private function newToken(string $token, $password, string $name, int $type, - int $remember): PublicKeyToken { + int $remember, + ?int $expires, + ): PublicKeyToken { $dbToken = new PublicKeyToken(); $dbToken->setUid($uid); $dbToken->setLoginName($loginName); @@ -505,6 +509,10 @@ private function newToken(string $token, $dbToken->setLastCheck($this->time->getTime()); $dbToken->setVersion(PublicKeyToken::VERSION); + if ($expires !== null) { + $dbToken->setExpires($expires); + } + return $dbToken; } diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index 68ae35da01f4f..1e33fcb9ee9aa 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -36,6 +36,7 @@ use OCP\Lockdown\ILockdownManager; use OCP\Security\Bruteforce\IThrottler; use OCP\Security\ISecureRandom; +use OCP\Server; use OCP\Session\Exceptions\SessionNotAvailableException; use OCP\User\Events\PostLoginEvent; use OCP\User\Events\UserFirstTimeLoggedInEvent; @@ -653,15 +654,15 @@ private function loginWithToken($token) { /** * Create a new session token for the given user credentials - * - * @param IRequest $request - * @param string $uid user UID - * @param string $loginName login name - * @param string $password - * @param int $remember - * @return boolean */ - public function createSessionToken(IRequest $request, $uid, $loginName, $password = null, $remember = IToken::DO_NOT_REMEMBER) { + public function createSessionToken( + IRequest $request, + string $uid, + string $loginName, + ?string $password = null, + int $remember = IToken::DO_NOT_REMEMBER, + ?int $expires = null, + ): bool { if (is_null($this->manager->get($uid))) { // User does not exist return false; @@ -671,10 +672,10 @@ public function createSessionToken(IRequest $request, $uid, $loginName, $passwor $sessionId = $this->session->getId(); $pwd = $this->getPassword($password); // Make sure the current sessionId has no leftover tokens - $this->atomic(function () use ($sessionId, $uid, $loginName, $pwd, $name, $remember) { + $this->atomic(function () use ($sessionId, $uid, $loginName, $pwd, $name, $remember, $expires): void { $this->tokenProvider->invalidateToken($sessionId); - $this->tokenProvider->generateToken($sessionId, $uid, $loginName, $pwd, $name, IToken::TEMPORARY_TOKEN, $remember); - }, \OCP\Server::get(IDBConnection::class)); + $this->tokenProvider->generateToken($sessionId, $uid, $loginName, $pwd, $name, IToken::TEMPORARY_TOKEN, $remember, expires:$expires); + }, Server::get(IDBConnection::class)); return true; } catch (SessionNotAvailableException $ex) { // This can happen with OCC, where a memory session is used From ef05f24eb13f280e6d66241b0f1c82a78b2562cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 21 May 2026 16:24:48 +0200 Subject: [PATCH 2/2] chore: Fix CreateSessionTokenCommandTest and add test for ephemeral session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../Login/CreateSessionTokenCommandTest.php | 77 ++++++++++++++++--- 1 file changed, 67 insertions(+), 10 deletions(-) diff --git a/tests/lib/Authentication/Login/CreateSessionTokenCommandTest.php b/tests/lib/Authentication/Login/CreateSessionTokenCommandTest.php index 668c0a6d6eaa6..dd44882bb3d50 100644 --- a/tests/lib/Authentication/Login/CreateSessionTokenCommandTest.php +++ b/tests/lib/Authentication/Login/CreateSessionTokenCommandTest.php @@ -1,40 +1,49 @@ config = $this->createMock(IConfig::class); $this->userSession = $this->createMock(Session::class); + $this->urlGenerator = $this->createMock(IURLGenerator::class); + $this->timeFactory = $this->createMock(ITimeFactory::class); $this->cmd = new CreateSessionTokenCommand( $this->config, - $this->userSession + $this->userSession, + $this->urlGenerator, + $this->timeFactory, ); } public function testProcess(): void { + // Just return the route name as path to not return an empty string + $this->urlGenerator->expects(self::once()) + ->method('linkToRoute') + ->willReturnArgument(0); $data = $this->getLoggedInLoginData(); $this->config->expects($this->once()) ->method('getSystemValueInt') @@ -53,7 +62,8 @@ public function testProcess(): void { $this->username, $this->username, $this->password, - IToken::REMEMBER + IToken::REMEMBER, + null ); $this->userSession->expects($this->once()) ->method('updateTokens') @@ -68,6 +78,10 @@ public function testProcess(): void { } public function testProcessDoNotRemember(): void { + // Just return the route name as path to not return an empty string + $this->urlGenerator->expects(self::once()) + ->method('linkToRoute') + ->willReturnArgument(0); $data = $this->getLoggedInLoginData(); $this->config->expects($this->once()) ->method('getSystemValueInt') @@ -86,7 +100,8 @@ public function testProcessDoNotRemember(): void { $this->username, $this->username, $this->password, - IToken::DO_NOT_REMEMBER + IToken::DO_NOT_REMEMBER, + null ); $this->userSession->expects($this->once()) ->method('updateTokens') @@ -100,4 +115,46 @@ public function testProcessDoNotRemember(): void { $this->assertTrue($result->isSuccess()); $this->assertFalse($data->isRememberLogin()); } + + public function testLoginFlowEphemeral(): void { + $this->redirectUrl = 'EPHEMERAL_ROUTE'; + $this->urlGenerator->expects(self::once()) + ->method('linkToRoute') + ->willReturn($this->redirectUrl); + $this->timeFactory->expects(self::once()) + ->method('getTime') + ->willReturn(1000); + + $data = $this->getLoggedInLoginDataWithRedirectUrl(); + $this->config->expects($this->once()) + ->method('getSystemValueInt') + ->with( + 'remember_login_cookie_lifetime', + 60 * 60 * 24 * 15 + ) + ->willReturn(100); + $this->user->expects($this->any()) + ->method('getUID') + ->willReturn($this->username); + $this->userSession->expects($this->once()) + ->method('createSessionToken') + ->with( + $this->request, + $this->username, + $this->username, + $this->password, + IToken::REMEMBER, + 1000 + 5 * 60 + ); + $this->userSession->expects($this->once()) + ->method('updateTokens') + ->with( + $this->username, + $this->password + ); + + $result = $this->cmd->process($data); + + $this->assertTrue($result->isSuccess()); + } }