Skip to content

Commit 8194adf

Browse files
julien-ncjulien-nc
authored
Merge pull request #1410 from nextcloud/fix/noid/only-redirect-navigation-requests
Prevent parallel login flows after logout
2 parents 59d114e + ca67713 commit 8194adf

5 files changed

Lines changed: 176 additions & 15 deletions

File tree

‎lib/AppInfo/Application.php‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
use OCA\UserOIDC\Listener\TimezoneHandlingListener;
2323
use OCA\UserOIDC\Listener\TokenInvalidatedListener;
2424
use OCA\UserOIDC\Service\ID4MeService;
25+
use OCA\UserOIDC\Service\RequestClassificationService;
2526
use OCA\UserOIDC\Service\SettingsService;
2627
use OCA\UserOIDC\Service\TokenService;
2728
use OCA\UserOIDC\User\Backend;
@@ -114,7 +115,10 @@ private function registerRedirect(IRequest $request, IURLGenerator $urlGenerator
114115
} catch (Exception $e) {
115116
// in case any errors happen when checking for the path do not apply redirect logic as it is only needed for the login
116117
}
117-
if ($isDefaultLogin && !$settings->getAllowMultipleUserBackEnds()) {
118+
if ($isDefaultLogin
119+
&& RequestClassificationService::isTopLevelHtmlNavigation($request)
120+
&& !$settings->getAllowMultipleUserBackEnds()
121+
) {
118122
$providers = $this->getCachedProviders($providerMapper);
119123
if (count($providers) === 1) {
120124
$targetUrl = $urlGenerator->linkToRoute(self::APP_ID . '.login.login', [

‎lib/Controller/LoginController.php‎

Lines changed: 75 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -67,12 +67,24 @@
6767

6868
#[OpenAPI(scope: OpenAPI::SCOPE_IGNORE)]
6969
class LoginController extends BaseOidcController {
70+
// these keys (state, nonce, login_providerid, redirect, code_verifier, timestamp)
71+
// are suffixed with the state value so they can be stored once per login flow
7072
private const STATE = 'oidc.state';
7173
private const NONCE = 'oidc.nonce';
72-
public const PROVIDERID = 'oidc.providerid';
74+
// this is the provider ID we store during the login flow (set by login, get by code)
75+
public const LOGIN_PROVIDERID = 'oidc.login.providerid';
7376
public const REDIRECT_AFTER_LOGIN = 'oidc.redirect';
74-
private const ID_TOKEN = 'oidc.id_token';
7577
private const CODE_VERIFIER = 'oidc.code_verifier';
78+
private const TIMESTAMP = 'oidc.timestamp';
79+
80+
// this is the provider ID we store once the authentication was successful
81+
// it is used by the singleLogout endpoint and the user backend
82+
public const PROVIDERID = 'oidc.providerid';
83+
// this id token is used to send id_token_hint to the IdP logout endpoint
84+
private const ID_TOKEN = 'oidc.id_token';
85+
86+
// we consider that a login flow should complete within 5 minutes
87+
private const LOGIN_FLOW_TIMEOUT = 300;
7688

7789
public function __construct(
7890
IRequest $request,
@@ -189,12 +201,15 @@ public function login(int $providerId, ?string $redirectUrl = null) {
189201
}
190202

191203
$state = $this->random->generate(32, ISecureRandom::CHAR_DIGITS . ISecureRandom::CHAR_UPPER);
192-
$this->session->set(self::STATE, $state);
204+
$sessionKeySuffix = '-' . $state;
205+
$this->session->set(self::STATE . $sessionKeySuffix, $state);
193206
$this->logger->debug('Storing OIDC state', ['state' => $state]);
194-
$this->session->set(self::REDIRECT_AFTER_LOGIN, $redirectUrl);
207+
$timestamp = $this->timeFactory->getTime();
208+
$this->session->set(self::TIMESTAMP . $sessionKeySuffix, $timestamp);
209+
$this->session->set(self::REDIRECT_AFTER_LOGIN . $sessionKeySuffix, $redirectUrl);
195210

196211
$nonce = $this->random->generate(32, ISecureRandom::CHAR_DIGITS . ISecureRandom::CHAR_UPPER);
197-
$this->session->set(self::NONCE, $nonce);
212+
$this->session->set(self::NONCE . $sessionKeySuffix, $nonce);
198213

199214
$oidcSystemConfig = $this->config->getSystemValue('user_oidc', []);
200215
$isPkceSupported = in_array('S256', $discovery['code_challenge_methods_supported'] ?? [], true);
@@ -203,10 +218,10 @@ public function login(int $providerId, ?string $redirectUrl = null) {
203218
if ($isPkceEnabled) {
204219
// PKCE code_challenge see https://datatracker.ietf.org/doc/html/rfc7636
205220
$code_verifier = $this->random->generate(128, ISecureRandom::CHAR_DIGITS . ISecureRandom::CHAR_UPPER . ISecureRandom::CHAR_LOWER);
206-
$this->session->set(self::CODE_VERIFIER, $code_verifier);
221+
$this->session->set(self::CODE_VERIFIER . $sessionKeySuffix, $code_verifier);
207222
}
208223

209-
$this->session->set(self::PROVIDERID, $providerId);
224+
$this->session->set(self::LOGIN_PROVIDERID . $sessionKeySuffix, $providerId);
210225
$this->session->close();
211226

212227
// get attribute mapping settings
@@ -343,6 +358,12 @@ public function login(int $providerId, ?string $redirectUrl = null) {
343358
#[UseSession]
344359
#[BruteForceProtection(action: 'userOidcCode')]
345360
public function code(string $state = '', string $code = '', string $scope = '', string $error = '', string $error_description = '') {
361+
if ($this->userSession->isLoggedIn()) {
362+
$sessionKeySuffix = '-' . $state;
363+
$redirectUrl = $this->session->get(self::REDIRECT_AFTER_LOGIN . $sessionKeySuffix);
364+
$this->cleanupSessionState($sessionKeySuffix);
365+
return $this->getRedirectResponse(!empty($redirectUrl) ? $redirectUrl : null);
366+
}
346367
if (!$this->isSecure()) {
347368
return $this->buildProtocolErrorResponse();
348369
}
@@ -360,36 +381,48 @@ public function code(string $state = '', string $code = '', string $scope = '',
360381
return $this->build403TemplateResponse($message, Http::STATUS_BAD_REQUEST, [], false);
361382
}
362383

363-
$storedState = $this->session->get(self::STATE);
384+
$sessionKeySuffix = '-' . $state;
385+
$storedState = $this->session->get(self::STATE . $sessionKeySuffix);
386+
387+
$currentTimestamp = $this->timeFactory->getTime();
388+
$sessionTimestamp = $this->session->get(self::TIMESTAMP . $sessionKeySuffix);
389+
if ($currentTimestamp - $sessionTimestamp > self::LOGIN_FLOW_TIMEOUT) {
390+
// the state, nonce etc... were stored too long ago, the login flow has expired
391+
$this->cleanupSessionState($sessionKeySuffix);
392+
$message = $this->l10n->t('The received state has expired.');
393+
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, [], false);
394+
}
364395

365396
if ($storedState !== $state) {
366397
$this->logger->warning('state does not match', [
367398
'got' => $state,
368399
'expected' => $storedState,
369-
'state_exists_in_session' => $this->session->exists(self::STATE),
400+
'state_exists_in_session' => $this->session->exists(self::STATE . $sessionKeySuffix),
370401
]);
371402

403+
$this->cleanupSessionState($sessionKeySuffix);
372404
$message = $this->l10n->t('The received state does not match the expected value.');
373405
if ($this->isDebugModeEnabled()) {
374406
$responseData = [
375407
'error' => 'invalid_state',
376408
'error_description' => $message,
377409
'got' => $state,
378410
'expected' => $storedState,
379-
'state_exists_in_session' => $this->session->exists(self::STATE),
411+
'state_exists_in_session' => $this->session->exists(self::STATE . $sessionKeySuffix),
380412
];
381413
return new JSONResponse($responseData, Http::STATUS_FORBIDDEN);
382414
}
383415
// we know debug mode is off, always throttle
384416
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, ['reason' => 'state does not match'], true);
385417
}
386418

387-
$providerId = (int)$this->session->get(self::PROVIDERID);
419+
$providerId = (int)$this->session->get(self::LOGIN_PROVIDERID . $sessionKeySuffix);
388420
$provider = $this->providerMapper->getProvider($providerId);
389421
try {
390422
$providerClientSecret = $this->crypto->decrypt($provider->getClientSecret());
391423
} catch (\Exception $e) {
392424
$this->logger->error('Failed to decrypt the client secret', ['exception' => $e]);
425+
$this->cleanupSessionState($sessionKeySuffix);
393426
$message = $this->l10n->t('Failed to decrypt the OIDC provider client secret');
394427
return $this->buildErrorTemplateResponse($message, Http::STATUS_BAD_REQUEST, [], false);
395428
}
@@ -409,7 +442,8 @@ public function code(string $state = '', string $code = '', string $scope = '',
409442
'grant_type' => 'authorization_code',
410443
];
411444
if ($isPkceEnabled) {
412-
$requestBody['code_verifier'] = $this->session->get(self::CODE_VERIFIER); // Set for the PKCE flow
445+
// Set for the PKCE flow
446+
$requestBody['code_verifier'] = $this->session->get(self::CODE_VERIFIER . $sessionKeySuffix);
413447
}
414448

415449
$headers = [];
@@ -461,10 +495,12 @@ public function code(string $state = '', string $code = '', string $scope = '',
461495
$this->logger->debug('Failed to contact the OIDC provider token endpoint', ['exception' => $e]);
462496
$message = $this->l10n->t('Failed to contact the OIDC provider token endpoint');
463497
}
498+
$this->cleanupSessionState($sessionKeySuffix);
464499
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, [], false);
465500
} catch (\Exception $e) {
466501
$this->logger->debug('Failed to contact the OIDC provider token endpoint', ['exception' => $e]);
467502
$message = $this->l10n->t('Failed to contact the OIDC provider token endpoint');
503+
$this->cleanupSessionState($sessionKeySuffix);
468504
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, [], false);
469505
}
470506

@@ -476,12 +512,14 @@ public function code(string $state = '', string $code = '', string $scope = '',
476512
'body' => $body,
477513
]);
478514
$message = $this->l10n->t('Failed to contact the OIDC provider token endpoint');
515+
$this->cleanupSessionState($sessionKeySuffix);
479516
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, [], false);
480517
}
481518

482519
if (!isset($data['id_token'])) {
483520
$this->logger->error('Missing id_token in IdP token response', ['keys' => array_keys($data)]);
484521
$message = $this->l10n->t('Failed to contact the OIDC provider token endpoint');
522+
$this->cleanupSessionState($sessionKeySuffix);
485523
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, [], false);
486524
}
487525

@@ -519,13 +557,15 @@ public function code(string $state = '', string $code = '', string $scope = '',
519557

520558
if (!isset($idTokenPayload->exp) || $idTokenPayload->exp < $this->timeFactory->getTime()) {
521559
$this->logger->debug('Token expired');
560+
$this->cleanupSessionState($sessionKeySuffix);
522561
$message = $this->l10n->t('The received token is expired.');
523562
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, ['reason' => 'token expired']);
524563
}
525564

526565
// Verify issuer
527566
if (!isset($idTokenPayload->iss) || $idTokenPayload->iss !== $discovery['issuer']) {
528567
$this->logger->debug('This token is issued by the wrong issuer');
568+
$this->cleanupSessionState($sessionKeySuffix);
529569
$message = $this->l10n->t('The issuer does not match the one from the discovery endpoint');
530570
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, ['invalid_issuer' => $idTokenPayload->iss]);
531571
}
@@ -541,6 +581,7 @@ public function code(string $state = '', string $code = '', string $scope = '',
541581
|| (is_array($tokenAudience) && !in_array($providerClientId, $tokenAudience, true))
542582
) {
543583
$this->logger->debug('This token is not for us');
584+
$this->cleanupSessionState($sessionKeySuffix);
544585
$message = $this->l10n->t('The audience does not match ours');
545586
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, ['invalid_audience' => $idTokenPayload->aud]);
546587
}
@@ -553,13 +594,15 @@ public function code(string $state = '', string $code = '', string $scope = '',
553594
// If the azp claim is present, it should be the client ID
554595
if (isset($idTokenPayload->azp) && $idTokenPayload->azp !== $provider->getClientId()) {
555596
$this->logger->debug('This token is not for us, authorized party (azp) is different than the client ID');
597+
$this->cleanupSessionState($sessionKeySuffix);
556598
$message = $this->l10n->t('The authorized party does not match ours');
557599
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, ['invalid_azp' => $idTokenPayload->azp]);
558600
}
559601
}
560602

561-
if (isset($idTokenPayload->nonce) && $idTokenPayload->nonce !== $this->session->get(self::NONCE)) {
603+
if (isset($idTokenPayload->nonce) && $idTokenPayload->nonce !== $this->session->get(self::NONCE . $sessionKeySuffix)) {
562604
$this->logger->debug('Nonce does not match');
605+
$this->cleanupSessionState($sessionKeySuffix);
563606
$message = $this->l10n->t('The nonce does not match');
564607
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, ['reason' => 'invalid nonce']);
565608
}
@@ -569,6 +612,7 @@ public function code(string $state = '', string $code = '', string $scope = '',
569612
$userId = $this->provisioningService->getClaimValue($idTokenPayload, $uidAttribute, $providerId);
570613

571614
if ($userId === null) {
615+
$this->cleanupSessionState($sessionKeySuffix);
572616
$message = $this->l10n->t('Failed to provision the user');
573617
return $this->build403TemplateResponse($message, Http::STATUS_BAD_REQUEST, ['reason' => 'failed to provision user']);
574618
}
@@ -580,6 +624,7 @@ public function code(string $state = '', string $code = '', string $scope = '',
580624

581625
if ($syncGroups === null || count($syncGroups) === 0) {
582626
$this->logger->debug('Prevented user from login as user is not part of a whitelisted group');
627+
$this->cleanupSessionState($sessionKeySuffix);
583628
$message = $this->l10n->t('You do not have permission to log in to this instance. If you think this is an error, please contact an administrator.');
584629
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, ['reason' => 'user not in any whitelisted group']);
585630
}
@@ -605,6 +650,7 @@ public function code(string $state = '', string $code = '', string $scope = '',
605650
if (!$softAutoProvisionAllowed && $existingUser !== null && $existingUser->getBackendClassName() !== Application::APP_ID) {
606651
// if soft auto-provisioning is disabled,
607652
// we refuse login for a user that already exists in another backend
653+
$this->cleanupSessionState($sessionKeySuffix);
608654
$message = $this->l10n->t('User conflict');
609655
return $this->build403TemplateResponse($message, Http::STATUS_BAD_REQUEST, ['reason' => 'non-soft auto provision, user conflict'], false);
610656
}
@@ -622,11 +668,13 @@ public function code(string $state = '', string $code = '', string $scope = '',
622668
}
623669

624670
if ($user === null) {
671+
$this->cleanupSessionState($sessionKeySuffix);
625672
$message = $this->l10n->t('Failed to provision the user');
626673
return $this->build403TemplateResponse($message, Http::STATUS_BAD_REQUEST, ['reason' => 'failed to provision user']);
627674
}
628675

629676
$this->session->set(self::ID_TOKEN, $idTokenRaw);
677+
$this->session->set(self::PROVIDERID, $providerId);
630678

631679
$this->logger->debug('Logging user in');
632680

@@ -692,7 +740,8 @@ public function code(string $state = '', string $code = '', string $scope = '',
692740

693741
$this->logger->debug('Redirecting user');
694742

695-
$redirectUrl = $this->session->get(self::REDIRECT_AFTER_LOGIN);
743+
$redirectUrl = $this->session->get(self::REDIRECT_AFTER_LOGIN . $sessionKeySuffix);
744+
$this->cleanupSessionState($sessionKeySuffix);
696745
if ($redirectUrl) {
697746
return $this->getRedirectResponse($redirectUrl);
698747
}
@@ -972,4 +1021,16 @@ private function toCodeChallenge(string $data): string {
9721021
$s = str_replace('/', '_', $s); // 63rd char of encoding
9731022
return $s;
9741023
}
1024+
1025+
/**
1026+
* Clean up session values for a given state suffix
1027+
*/
1028+
private function cleanupSessionState(string $sessionKeySuffix): void {
1029+
$this->session->remove(self::STATE . $sessionKeySuffix);
1030+
$this->session->remove(self::NONCE . $sessionKeySuffix);
1031+
$this->session->remove(self::LOGIN_PROVIDERID . $sessionKeySuffix);
1032+
$this->session->remove(self::REDIRECT_AFTER_LOGIN . $sessionKeySuffix);
1033+
$this->session->remove(self::CODE_VERIFIER . $sessionKeySuffix);
1034+
$this->session->remove(self::TIMESTAMP . $sessionKeySuffix);
1035+
}
9751036
}

‎lib/Service/RequestClassificationService.php‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\UserOIDC\Service;
11+
12+
use OCP\IRequest;
13+
14+
class RequestClassificationService {
15+
public static function isTopLevelHtmlNavigation(IRequest $request): bool {
16+
if (strtoupper($request->getMethod()) !== 'GET') {
17+
return false;
18+
}
19+
20+
if ($request->getHeader('OCS-apirequest') !== '') {
21+
return false;
22+
}
23+
24+
if ($request->getHeader('X-Requested-With') === 'XMLHttpRequest') {
25+
return false;
26+
}
27+
28+
return true;
29+
}
30+
}

‎lib/Service/TokenService.php‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,15 @@ public function checkLoginToken(): void {
185185
}
186186

187187
public function reauthenticate(int $providerId) {
188+
if (!RequestClassificationService::isTopLevelHtmlNavigation($this->request)) {
189+
$this->userSession->logout();
190+
$this->logger->debug('[TokenService] reauthenticate skipped: request is not a top-level HTML navigation', [
191+
'provider_id' => $providerId,
192+
'request_uri' => $this->request->getRequestUri(),
193+
]);
194+
return;
195+
}
196+
188197
// Logout the user and redirect to the oidc login flow to gather a fresh token
189198
$this->userSession->logout();
190199
$redirectUrl = $this->urlGenerator->linkToRouteAbsolute(Application::APP_ID . '.login.login', [

‎tests/unit/Service/RequestClassificationServiceTest.php‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
<?php
2+
3+
/**
4+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
5+
* SPDX-License-Identifier: AGPL-3.0-or-later
6+
*/
7+
8+
declare(strict_types=1);
9+
10+
use OCA\UserOIDC\Service\RequestClassificationService;
11+
use OCP\IRequest;
12+
use PHPUnit\Framework\Attributes\DataProvider;
13+
use PHPUnit\Framework\TestCase;
14+
15+
class RequestClassificationServiceTest extends TestCase {
16+
#[DataProvider('topLevelHtmlNavigationProvider')]
17+
public function testIsTopLevelHtmlNavigation(string $method, array $headers, bool $expected): void {
18+
$request = $this->createMock(IRequest::class);
19+
$request->method('getMethod')
20+
->willReturn($method);
21+
$request->method('getHeader')
22+
->willReturnCallback(static function (string $name) use ($headers): string {
23+
return $headers[$name] ?? '';
24+
});
25+
26+
self::assertSame($expected, RequestClassificationService::isTopLevelHtmlNavigation($request));
27+
}
28+
29+
public static function topLevelHtmlNavigationProvider(): array {
30+
return [
31+
'top level navigation' => [
32+
'GET',
33+
[],
34+
true,
35+
],
36+
'xhr request' => [
37+
'GET',
38+
[
39+
'X-Requested-With' => 'XMLHttpRequest',
40+
],
41+
false,
42+
],
43+
'ocs api request' => [
44+
'GET',
45+
[
46+
'OCS-apirequest' => 'true',
47+
],
48+
false,
49+
],
50+
'non get request' => [
51+
'POST',
52+
[],
53+
false,
54+
],
55+
];
56+
}
57+
}

0 commit comments

Comments
 (0)