Merge pull request #740 from nextcloud/enh/make-pkce-optional

Make pkce optional
nextcloud · Dec 21, 2023 · 486b4ec · 486b4ec
2 parents 9a5e996 + fe0de68
commit 486b4ec
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 15 deletions.
diff --git a/README.md b/README.md
@@ -64,6 +64,17 @@ parameter to the login URL.
 sudo -u www-data php var/www/nextcloud/occ config:app:set --value=0 user_oidc allow_multiple_user_backends
 ```
 
+### PKCE
+
+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`:
+``` php
+'user_oidc' => [
+    'use_pkce' => true,
+],
+```
+
 ### Single logout
 
 Single logout is enabled by default. When logging out of Nextcloud,

diff --git a/lib/Controller/LoginController.php b/lib/Controller/LoginController.php
@@ -230,9 +230,13 @@ public function login(int $providerId, string $redirectUrl = null) {
 		$nonce = $this->random->generate(32, ISecureRandom::CHAR_DIGITS . ISecureRandom::CHAR_UPPER);
 		$this->session->set(self::NONCE, $nonce);
 
-		// 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);
+		$oidcSystemConfig = $this->config->getSystemValue('user_oidc', []);
+		$isPkceEnabled = isset($oidcSystemConfig['use_pkce']) && $oidcSystemConfig['use_pkce'];
+		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::PROVIDERID, $providerId);
 		$this->session->close();
@@ -285,9 +289,11 @@ public function login(int $providerId, string $redirectUrl = null) {
 			'claims' => json_encode($claims),
 			'state' => $state,
 			'nonce' => $nonce,
-			'code_challenge' => $this->toCodeChallenge($code_verifier),
-			'code_challenge_method' => 'S256',
 		];
+		if ($isPkceEnabled) {
+			$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'])) {
@@ -375,25 +381,29 @@ public function code(string $state = '', string $code = '', string $scope = '',
 			return $this->buildErrorTemplateResponse($message, Http::STATUS_BAD_REQUEST, [], false);
 		}
 
-		$code_verifier = $this->session->get(self::CODE_VERIFIER);
+		$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']);
 
 		$client = $this->clientService->newClient();
 		try {
+			$requestBody = [
+				'code' => $code,
+				'client_id' => $provider->getClientId(),
+				'client_secret' => $providerClientSecret,
+				'redirect_uri' => $this->urlGenerator->linkToRouteAbsolute(Application::APP_ID . '.login.code'),
+				'grant_type' => 'authorization_code',
+			];
+			if ($isPkceEnabled) {
+				$requestBody['code_verifier'] = $this->session->get(self::CODE_VERIFIER); // Set for the PKCE flow
+			}
 			$result = $client->post(
 				$discovery['token_endpoint'],
 				[
-					'body' => [
-						'code' => $code,
-						'client_id' => $provider->getClientId(),
-						'client_secret' => $providerClientSecret,
-						'redirect_uri' => $this->urlGenerator->linkToRouteAbsolute(Application::APP_ID . '.login.code'),
-						'grant_type' => 'authorization_code',
-						'code_verifier' => $code_verifier, // Set for the PKCE flow
-					],
+					'body' => $requestBody,
 				]
 			);
 		} catch (ClientException | ServerException $e) {
@@ -480,7 +490,6 @@ public function code(string $state = '', string $code = '', string $scope = '',
 			return $this->build403TemplateResponse($message, Http::STATUS_BAD_REQUEST, ['reason' => 'failed to provision user']);
 		}
 
-		$oidcSystemConfig = $this->config->getSystemValue('user_oidc', []);
 		$autoProvisionAllowed = (!isset($oidcSystemConfig['auto_provision']) || $oidcSystemConfig['auto_provision']);
 
 		// Provisioning