Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/AppInfo/Application.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use OCA\UserOIDC\Listener\TimezoneHandlingListener;
use OCA\UserOIDC\Listener\TokenInvalidatedListener;
use OCA\UserOIDC\Service\ID4MeService;
use OCA\UserOIDC\Service\RequestClassificationService;
use OCA\UserOIDC\Service\SettingsService;
use OCA\UserOIDC\Service\TokenService;
use OCA\UserOIDC\User\Backend;
Expand Down Expand Up @@ -114,7 +115,10 @@ private function registerRedirect(IRequest $request, IURLGenerator $urlGenerator
} catch (Exception $e) {
// in case any errors happen when checking for the path do not apply redirect logic as it is only needed for the login
}
if ($isDefaultLogin && !$settings->getAllowMultipleUserBackEnds()) {
if ($isDefaultLogin
&& RequestClassificationService::isTopLevelHtmlNavigation($request)
&& !$settings->getAllowMultipleUserBackEnds()
) {
$providers = $this->getCachedProviders($providerMapper);
if (count($providers) === 1) {
$targetUrl = $urlGenerator->linkToRoute(self::APP_ID . '.login.login', [
Expand Down
89 changes: 75 additions & 14 deletions lib/Controller/LoginController.php
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,24 @@

#[OpenAPI(scope: OpenAPI::SCOPE_IGNORE)]
class LoginController extends BaseOidcController {
// these keys (state, nonce, login_providerid, redirect, code_verifier, timestamp)
// are suffixed with the state value so they can be stored once per login flow
private const STATE = 'oidc.state';
private const NONCE = 'oidc.nonce';
public const PROVIDERID = 'oidc.providerid';
// this is the provider ID we store during the login flow (set by login, get by code)
public const LOGIN_PROVIDERID = 'oidc.login.providerid';
public const REDIRECT_AFTER_LOGIN = 'oidc.redirect';
private const ID_TOKEN = 'oidc.id_token';
private const CODE_VERIFIER = 'oidc.code_verifier';
private const TIMESTAMP = 'oidc.timestamp';

// this is the provider ID we store once the authentication was successful
// it is used by the singleLogout endpoint and the user backend
public const PROVIDERID = 'oidc.providerid';
// this id token is used to send id_token_hint to the IdP logout endpoint
private const ID_TOKEN = 'oidc.id_token';

// we consider that a login flow should complete within 5 minutes
private const LOGIN_FLOW_TIMEOUT = 300;

public function __construct(
IRequest $request,
Expand Down Expand Up @@ -189,12 +201,15 @@ public function login(int $providerId, ?string $redirectUrl = null) {
}

$state = $this->random->generate(32, ISecureRandom::CHAR_DIGITS . ISecureRandom::CHAR_UPPER);
$this->session->set(self::STATE, $state);
$sessionKeySuffix = '-' . $state;
$this->session->set(self::STATE . $sessionKeySuffix, $state);
$this->logger->debug('Storing OIDC state', ['state' => $state]);
$this->session->set(self::REDIRECT_AFTER_LOGIN, $redirectUrl);
$timestamp = $this->timeFactory->getTime();
$this->session->set(self::TIMESTAMP . $sessionKeySuffix, $timestamp);
$this->session->set(self::REDIRECT_AFTER_LOGIN . $sessionKeySuffix, $redirectUrl);

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

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

$this->session->set(self::PROVIDERID, $providerId);
$this->session->set(self::LOGIN_PROVIDERID . $sessionKeySuffix, $providerId);
$this->session->close();

// get attribute mapping settings
Expand Down Expand Up @@ -343,6 +358,12 @@ public function login(int $providerId, ?string $redirectUrl = null) {
#[UseSession]
#[BruteForceProtection(action: 'userOidcCode')]
public function code(string $state = '', string $code = '', string $scope = '', string $error = '', string $error_description = '') {
if ($this->userSession->isLoggedIn()) {
$sessionKeySuffix = '-' . $state;
$redirectUrl = $this->session->get(self::REDIRECT_AFTER_LOGIN . $sessionKeySuffix);
$this->cleanupSessionState($sessionKeySuffix);
return $this->getRedirectResponse(!empty($redirectUrl) ? $redirectUrl : null);
}
if (!$this->isSecure()) {
return $this->buildProtocolErrorResponse();
}
Expand All @@ -360,36 +381,48 @@ public function code(string $state = '', string $code = '', string $scope = '',
return $this->build403TemplateResponse($message, Http::STATUS_BAD_REQUEST, [], false);
}

$storedState = $this->session->get(self::STATE);
$sessionKeySuffix = '-' . $state;
$storedState = $this->session->get(self::STATE . $sessionKeySuffix);

$currentTimestamp = $this->timeFactory->getTime();
$sessionTimestamp = $this->session->get(self::TIMESTAMP . $sessionKeySuffix);
if ($currentTimestamp - $sessionTimestamp > self::LOGIN_FLOW_TIMEOUT) {
// the state, nonce etc... were stored too long ago, the login flow has expired
$this->cleanupSessionState($sessionKeySuffix);
$message = $this->l10n->t('The received state has expired.');
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, [], false);
}

if ($storedState !== $state) {
$this->logger->warning('state does not match', [
'got' => $state,
'expected' => $storedState,
'state_exists_in_session' => $this->session->exists(self::STATE),
'state_exists_in_session' => $this->session->exists(self::STATE . $sessionKeySuffix),
]);

$this->cleanupSessionState($sessionKeySuffix);
$message = $this->l10n->t('The received state does not match the expected value.');
if ($this->isDebugModeEnabled()) {
$responseData = [
'error' => 'invalid_state',
'error_description' => $message,
'got' => $state,
'expected' => $storedState,
'state_exists_in_session' => $this->session->exists(self::STATE),
'state_exists_in_session' => $this->session->exists(self::STATE . $sessionKeySuffix),
];
return new JSONResponse($responseData, Http::STATUS_FORBIDDEN);
}
// we know debug mode is off, always throttle
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, ['reason' => 'state does not match'], true);
}

$providerId = (int)$this->session->get(self::PROVIDERID);
$providerId = (int)$this->session->get(self::LOGIN_PROVIDERID . $sessionKeySuffix);
$provider = $this->providerMapper->getProvider($providerId);
try {
$providerClientSecret = $this->crypto->decrypt($provider->getClientSecret());
} catch (\Exception $e) {
$this->logger->error('Failed to decrypt the client secret', ['exception' => $e]);
$this->cleanupSessionState($sessionKeySuffix);
$message = $this->l10n->t('Failed to decrypt the OIDC provider client secret');
return $this->buildErrorTemplateResponse($message, Http::STATUS_BAD_REQUEST, [], false);
}
Expand All @@ -409,7 +442,8 @@ public function code(string $state = '', string $code = '', string $scope = '',
'grant_type' => 'authorization_code',
];
if ($isPkceEnabled) {
$requestBody['code_verifier'] = $this->session->get(self::CODE_VERIFIER); // Set for the PKCE flow
// Set for the PKCE flow
$requestBody['code_verifier'] = $this->session->get(self::CODE_VERIFIER . $sessionKeySuffix);
}

$headers = [];
Expand Down Expand Up @@ -461,10 +495,12 @@ public function code(string $state = '', string $code = '', string $scope = '',
$this->logger->debug('Failed to contact the OIDC provider token endpoint', ['exception' => $e]);
$message = $this->l10n->t('Failed to contact the OIDC provider token endpoint');
}
$this->cleanupSessionState($sessionKeySuffix);
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, [], false);
} catch (\Exception $e) {
$this->logger->debug('Failed to contact the OIDC provider token endpoint', ['exception' => $e]);
$message = $this->l10n->t('Failed to contact the OIDC provider token endpoint');
$this->cleanupSessionState($sessionKeySuffix);
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, [], false);
}

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

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

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

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

// Verify issuer
if (!isset($idTokenPayload->iss) || $idTokenPayload->iss !== $discovery['issuer']) {
$this->logger->debug('This token is issued by the wrong issuer');
$this->cleanupSessionState($sessionKeySuffix);
$message = $this->l10n->t('The issuer does not match the one from the discovery endpoint');
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, ['invalid_issuer' => $idTokenPayload->iss]);
}
Expand All @@ -541,6 +581,7 @@ public function code(string $state = '', string $code = '', string $scope = '',
|| (is_array($tokenAudience) && !in_array($providerClientId, $tokenAudience, true))
) {
$this->logger->debug('This token is not for us');
$this->cleanupSessionState($sessionKeySuffix);
$message = $this->l10n->t('The audience does not match ours');
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, ['invalid_audience' => $idTokenPayload->aud]);
}
Expand All @@ -553,13 +594,15 @@ public function code(string $state = '', string $code = '', string $scope = '',
// If the azp claim is present, it should be the client ID
if (isset($idTokenPayload->azp) && $idTokenPayload->azp !== $provider->getClientId()) {
$this->logger->debug('This token is not for us, authorized party (azp) is different than the client ID');
$this->cleanupSessionState($sessionKeySuffix);
$message = $this->l10n->t('The authorized party does not match ours');
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, ['invalid_azp' => $idTokenPayload->azp]);
}
}

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

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

if ($syncGroups === null || count($syncGroups) === 0) {
$this->logger->debug('Prevented user from login as user is not part of a whitelisted group');
$this->cleanupSessionState($sessionKeySuffix);
$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.');
return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, ['reason' => 'user not in any whitelisted group']);
}
Expand All @@ -605,6 +650,7 @@ public function code(string $state = '', string $code = '', string $scope = '',
if (!$softAutoProvisionAllowed && $existingUser !== null && $existingUser->getBackendClassName() !== Application::APP_ID) {
// if soft auto-provisioning is disabled,
// we refuse login for a user that already exists in another backend
$this->cleanupSessionState($sessionKeySuffix);
$message = $this->l10n->t('User conflict');
return $this->build403TemplateResponse($message, Http::STATUS_BAD_REQUEST, ['reason' => 'non-soft auto provision, user conflict'], false);
}
Expand All @@ -622,11 +668,13 @@ public function code(string $state = '', string $code = '', string $scope = '',
}

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

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

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

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

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

$redirectUrl = $this->session->get(self::REDIRECT_AFTER_LOGIN);
$redirectUrl = $this->session->get(self::REDIRECT_AFTER_LOGIN . $sessionKeySuffix);
$this->cleanupSessionState($sessionKeySuffix);
if ($redirectUrl) {
return $this->getRedirectResponse($redirectUrl);
}
Expand Down Expand Up @@ -972,4 +1021,16 @@ private function toCodeChallenge(string $data): string {
$s = str_replace('/', '_', $s); // 63rd char of encoding
return $s;
}

/**
* Clean up session values for a given state suffix
*/
private function cleanupSessionState(string $sessionKeySuffix): void {
$this->session->remove(self::STATE . $sessionKeySuffix);
$this->session->remove(self::NONCE . $sessionKeySuffix);
$this->session->remove(self::LOGIN_PROVIDERID . $sessionKeySuffix);
$this->session->remove(self::REDIRECT_AFTER_LOGIN . $sessionKeySuffix);
$this->session->remove(self::CODE_VERIFIER . $sessionKeySuffix);
$this->session->remove(self::TIMESTAMP . $sessionKeySuffix);
}
}
30 changes: 30 additions & 0 deletions lib/Service/RequestClassificationService.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace OCA\UserOIDC\Service;

use OCP\IRequest;

class RequestClassificationService {
public static function isTopLevelHtmlNavigation(IRequest $request): bool {
if (strtoupper($request->getMethod()) !== 'GET') {
return false;
}

if ($request->getHeader('OCS-apirequest') !== '') {
return false;
}

if ($request->getHeader('X-Requested-With') === 'XMLHttpRequest') {
return false;
}

return true;
Comment on lines +24 to +28
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ($request->getHeader('X-Requested-With') === 'XMLHttpRequest') {
return false;
}
return true;
return $request->getHeader('X-Requested-With') !== 'XMLHttpRequest');

}
}
9 changes: 9 additions & 0 deletions lib/Service/TokenService.php
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,15 @@ public function checkLoginToken(): void {
}

public function reauthenticate(int $providerId) {
if (!RequestClassificationService::isTopLevelHtmlNavigation($this->request)) {
$this->userSession->logout();
$this->logger->debug('[TokenService] reauthenticate skipped: request is not a top-level HTML navigation', [
'provider_id' => $providerId,
'request_uri' => $this->request->getRequestUri(),
]);
return;
}

// Logout the user and redirect to the oidc login flow to gather a fresh token
$this->userSession->logout();
$redirectUrl = $this->urlGenerator->linkToRouteAbsolute(Application::APP_ID . '.login.login', [
Expand Down
57 changes: 57 additions & 0 deletions tests/unit/Service/RequestClassificationServiceTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
<?php

/**
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

declare(strict_types=1);

use OCA\UserOIDC\Service\RequestClassificationService;
use OCP\IRequest;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\TestCase;

class RequestClassificationServiceTest extends TestCase {
#[DataProvider('topLevelHtmlNavigationProvider')]
public function testIsTopLevelHtmlNavigation(string $method, array $headers, bool $expected): void {
$request = $this->createMock(IRequest::class);
$request->method('getMethod')
->willReturn($method);
$request->method('getHeader')
->willReturnCallback(static function (string $name) use ($headers): string {
return $headers[$name] ?? '';
});

self::assertSame($expected, RequestClassificationService::isTopLevelHtmlNavigation($request));
}

public static function topLevelHtmlNavigationProvider(): array {
return [
'top level navigation' => [
'GET',
[],
true,
],
'xhr request' => [
'GET',
[
'X-Requested-With' => 'XMLHttpRequest',
],
false,
],
'ocs api request' => [
'GET',
[
'OCS-apirequest' => 'true',
],
false,
],
'non get request' => [
'POST',
[],
false,
],
];
}
}
Loading