allow storing multiple states, cleanup on failure is still missing

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>

Author: julien-nc

lib/Controller/LoginController.php

@@ -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,
@@ -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);
@@ -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
@@ -360,13 +375,22 @@ 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
+			$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),
 			]);
 
 			$message = $this->l10n->t('The received state does not match the expected value.');
@@ -376,15 +400,15 @@ public function code(string $state = '', string $code = '', string $scope = '',
 					'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());
@@ -409,7 +433,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 = [];
@@ -558,7 +583,7 @@ public function code(string $state = '', string $code = '', string $scope = '',
 			}
 		}
 
-		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');
 			$message = $this->l10n->t('The nonce does not match');
 			return $this->build403TemplateResponse($message, Http::STATUS_FORBIDDEN, ['reason' => 'invalid nonce']);
@@ -627,6 +652,7 @@ public function code(string $state = '', string $code = '', string $scope = '',
 		}
 
 		$this->session->set(self::ID_TOKEN, $idTokenRaw);
+		$this->session->set(self::PROVIDERID, $providerId);
 
 		$this->logger->debug('Logging user in');
 
@@ -692,7 +718,7 @@ 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);
 		if ($redirectUrl) {
 			return $this->getRedirectResponse($redirectUrl);
 		}