Merge pull request #956 from nextcloud/fix/pkce-default

fix: re-enable PKCE by default
nextcloud · Oct 9, 2024 · 94a818b · 94a818b
2 parents 8e0ac5d + f06bfc6
commit 94a818b
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 24 deletions.
diff --git a/README.md b/README.md
@@ -88,10 +88,12 @@ sudo -u www-data php var/www/nextcloud/occ config:app:set --value=0 user_oidc al
 
 This app supports PKCE (Proof Key for Code Exchange).
 https://datatracker.ietf.org/doc/html/rfc7636
-It is disabled by default and can be enabled in `config.php`:
+Unless PKCE is not supported by the configured OpenID Connect provider,
+it is enabled by default.
+You can also manually disable it in `config.php`:
 ``` php
 'user_oidc' => [
-    'use_pkce' => true,
+    'use_pkce' => false,
 ],
 ```
 

diff --git a/lib/Controller/LoginController.php b/lib/Controller/LoginController.php
@@ -224,6 +224,24 @@ public function login(int $providerId, ?string $redirectUrl = null) {
 			return $this->buildErrorTemplateResponse($message, Http::STATUS_NOT_FOUND, ['provider_not_found' => $providerId]);
 		}
 
+		// pass discovery query parameters also on to the authentication
+		$data = [];
+		$discoveryUrl = parse_url($provider->getDiscoveryEndpoint());
+		if (isset($discoveryUrl['query'])) {
+			$this->logger->debug('Add custom discovery query: ' . $discoveryUrl['query']);
+			$discoveryQuery = [];
+			parse_str($discoveryUrl['query'], $discoveryQuery);
+			$data += $discoveryQuery;
+		}
+
+		try {
+			$discovery = $this->discoveryService->obtainDiscovery($provider);
+		} catch (\Exception $e) {
+			$this->logger->error('Could not reach the provider at URL ' . $provider->getDiscoveryEndpoint(), ['exception' => $e]);
+			$message = $this->l10n->t('Could not reach the OpenID Connect provider.');
+			return $this->buildErrorTemplateResponse($message, Http::STATUS_NOT_FOUND, ['reason' => 'provider unreachable']);
+		}
+
 		$state = $this->random->generate(32, ISecureRandom::CHAR_DIGITS . ISecureRandom::CHAR_UPPER);
 		$this->session->set(self::STATE, $state);
 		$this->session->set(self::REDIRECT_AFTER_LOGIN, $redirectUrl);
@@ -232,7 +250,9 @@ public function login(int $providerId, ?string $redirectUrl = null) {
 		$this->session->set(self::NONCE, $nonce);
 
 		$oidcSystemConfig = $this->config->getSystemValue('user_oidc', []);
-		$isPkceEnabled = isset($oidcSystemConfig['use_pkce']) && $oidcSystemConfig['use_pkce'];
+		$isPkceSupported = in_array('S256', $discovery['code_challenge_methods_supported'] ?? [], true);
+		$isPkceEnabled = $isPkceSupported && ($oidcSystemConfig['use_pkce'] ?? true);
+
 		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);
@@ -295,7 +315,7 @@ public function login(int $providerId, ?string $redirectUrl = null) {
 			}
 		}
 
-		$data = [
+		$data += [
 			'client_id' => $provider->getClientId(),
 			'response_type' => 'code',
 			'scope' => trim($provider->getScope()),
@@ -308,23 +328,6 @@ public function login(int $providerId, ?string $redirectUrl = null) {
 			$data['code_challenge'] = $this->toCodeChallenge($code_verifier);
 			$data['code_challenge_method'] = 'S256';
 		}
-		// pass discovery query parameters also on to the authentication
-		$discoveryUrl = parse_url($provider->getDiscoveryEndpoint());
-		if (isset($discoveryUrl['query'])) {
-			$this->logger->debug('Add custom discovery query: ' . $discoveryUrl['query']);
-			$discoveryQuery = [];
-			parse_str($discoveryUrl['query'], $discoveryQuery);
-			$data += $discoveryQuery;
-		}
-
-		try {
-			$discovery = $this->discoveryService->obtainDiscovery($provider);
-		} catch (\Exception $e) {
-			$this->logger->error('Could not reach the provider at URL ' . $provider->getDiscoveryEndpoint(), ['exception' => $e]);
-			$message = $this->l10n->t('Could not reach the OpenID Connect provider.');
-			return $this->buildErrorTemplateResponse($message, Http::STATUS_NOT_FOUND, ['reason' => 'provider unreachable']);
-		}
-
 		$authorizationUrl = $this->discoveryService->buildAuthorizationUrl($discovery['authorization_endpoint'], $data);
 
 		$this->logger->debug('Redirecting user to: ' . $authorizationUrl);
@@ -395,13 +398,14 @@ public function code(string $state = '', string $code = '', string $scope = '',
 			return $this->buildErrorTemplateResponse($message, Http::STATUS_BAD_REQUEST, [], false);
 		}
 
-		$oidcSystemConfig = $this->config->getSystemValue('user_oidc', []);
-		$isPkceEnabled = isset($oidcSystemConfig['use_pkce']) && $oidcSystemConfig['use_pkce'];
-
 		$discovery = $this->discoveryService->obtainDiscovery($provider);
 
 		$this->logger->debug('Obtainting data from: ' . $discovery['token_endpoint']);
 
+		$oidcSystemConfig = $this->config->getSystemValue('user_oidc', []);
+		$isPkceSupported = in_array('S256', $discovery['code_challenge_methods_supported'] ?? [], true);
+		$isPkceEnabled = $isPkceSupported && ($oidcSystemConfig['use_pkce'] ?? true);
+
 		$client = $this->clientService->newClient();
 		try {
 			$requestBody = [