From 8d197cc3f6c7fceadc8bad06a310003b61a29ed7 Mon Sep 17 00:00:00 2001 From: Florian Klinger Date: Thu, 4 Jan 2024 11:36:39 +0100 Subject: [PATCH 1/4] Ensure the discovery endpoint is validated and all required fields are verified Signed-off-by: Florian Klinger --- lib/Controller/SettingsController.php | 56 +++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php index 3ec2b6b6..2d870551 100644 --- a/lib/Controller/SettingsController.php +++ b/lib/Controller/SettingsController.php @@ -63,12 +63,59 @@ public function __construct( $this->crypto = $crypto; } + public function isDiscoveryEndpointValid($url) { + // Initialize cURL session + $ch = curl_init($url); + curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); + curl_setopt($ch, CURLOPT_HEADER, false); + $response = curl_exec($ch); + $httpCode = curl_getinfo($ch, CURLINFO_HTTP_CODE); + curl_close($ch); + + $result = [ + 'isReachable' => false, + 'missingFields' => [], + ]; + + // Check if the request was successful + if ($httpCode == 200 && !empty($response)) { + $result['isReachable'] = true; + $data = json_decode($response, true); + + // Check for necessary fields in the response + $requiredFields = [ + 'issuer', 'authorization_endpoint', 'token_endpoint', 'jwks_uri', + 'response_types_supported', 'subject_types_supported', 'id_token_signing_alg_values_supported', + ]; + + foreach ($requiredFields as $field) { + if (!isset($data[$field])) { + $result['missingFields'][] = $field; + } + } + } else { + // Set isReachable to false if http code wasnt 200 + $result['isReachable'] = false; + } + + return $result; + } + public function createProvider(string $identifier, string $clientId, string $clientSecret, string $discoveryEndpoint, array $settings = [], string $scope = 'openid email profile', ?string $endSessionEndpoint = null): JSONResponse { if ($this->providerService->getProviderByIdentifier($identifier) !== null) { return new JSONResponse(['message' => 'Provider with the given identifier already exists'], Http::STATUS_CONFLICT); } + $result = $this->isDiscoveryEndpointValid($discoveryEndpoint); + if (!$result['isReachable']) { + $message = 'The discovery endpoint is not reachable.'; + return new JSONResponse(['message' => $message], Http::STATUS_BAD_REQUEST); + } elseif (!empty($result['missingFields'])) { + $message = 'Invalid discovery endpoint. Missing fields: ' . implode(', ', $result['missingFields']); + return new JSONResponse(['message' => $message], Http::STATUS_BAD_REQUEST); + } + $provider = new Provider(); $provider->setIdentifier($identifier); $provider->setClientId($clientId); @@ -92,6 +139,15 @@ public function updateProvider(int $providerId, string $identifier, string $clie return new JSONResponse(['message' => 'Provider with the given identifier does not exist'], Http::STATUS_NOT_FOUND); } + $result = $this->isDiscoveryEndpointValid($discoveryEndpoint); + if (!$result['isReachable']) { + $message = 'The discovery endpoint is not reachable.'; + return new JSONResponse(['message' => $message], Http::STATUS_BAD_REQUEST); + } elseif (!empty($result['missingFields'])) { + $message = 'Invalid discovery endpoint. Missing fields: ' . implode(', ', $result['missingFields']); + return new JSONResponse(['message' => $message], Http::STATUS_BAD_REQUEST); + } + $provider->setIdentifier($identifier); $provider->setClientId($clientId); if ($clientSecret) { From 62d17c26f5bbf4d49d0ff0271f16bd2df15d6dbb Mon Sep 17 00:00:00 2001 From: Florian Klinger Date: Fri, 5 Jan 2024 10:21:28 +0100 Subject: [PATCH 2/4] Refactored from cURL to IClientService Signed-off-by: Florian Klinger --- lib/Controller/SettingsController.php | 61 +++++++++++++++------------ 1 file changed, 34 insertions(+), 27 deletions(-) diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php index 2d870551..da4ff0d9 100644 --- a/lib/Controller/SettingsController.php +++ b/lib/Controller/SettingsController.php @@ -25,6 +25,7 @@ namespace OCA\UserOIDC\Controller; +use Exception; use OCA\UserOIDC\AppInfo\Application; use OCA\UserOIDC\Db\Provider; use OCA\UserOIDC\Db\ProviderMapper; @@ -34,6 +35,7 @@ use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Http; use OCP\AppFramework\Http\JSONResponse; +use OCP\Http\Client\IClientService; use OCP\IRequest; use OCP\Security\ICrypto; @@ -47,13 +49,16 @@ class SettingsController extends Controller { private $providerService; /** @var ICrypto */ private $crypto; + /** @var IClientService */ + private $clientService; public function __construct( IRequest $request, ProviderMapper $providerMapper, ID4MeService $id4meService, ProviderService $providerService, - ICrypto $crypto + ICrypto $crypto, + IClientService $clientService ) { parent::__construct(Application::APP_ID, $request); @@ -61,43 +66,45 @@ public function __construct( $this->id4meService = $id4meService; $this->providerService = $providerService; $this->crypto = $crypto; + $this->clientService = $clientService; } public function isDiscoveryEndpointValid($url) { - // Initialize cURL session - $ch = curl_init($url); - curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); - curl_setopt($ch, CURLOPT_HEADER, false); - $response = curl_exec($ch); - $httpCode = curl_getinfo($ch, CURLINFO_HTTP_CODE); - curl_close($ch); - $result = [ 'isReachable' => false, 'missingFields' => [], ]; - - // Check if the request was successful - if ($httpCode == 200 && !empty($response)) { - $result['isReachable'] = true; - $data = json_decode($response, true); - - // Check for necessary fields in the response - $requiredFields = [ - 'issuer', 'authorization_endpoint', 'token_endpoint', 'jwks_uri', - 'response_types_supported', 'subject_types_supported', 'id_token_signing_alg_values_supported', - ]; - - foreach ($requiredFields as $field) { - if (!isset($data[$field])) { - $result['missingFields'][] = $field; + + try { + $client = $this->clientService->newClient(); + $response = $client->get($url); + $httpCode = $response->getStatusCode(); + $body = $response->getBody(); + + // Check if the request was successful + if ($httpCode == 200 && !empty($body)) { + $result['isReachable'] = true; + $data = json_decode($body, true); + + // Check for required fields as defined in: https://openid.net/specs/openid-connect-discovery-1_0.html + $requiredFields = [ + 'issuer', 'authorization_endpoint', 'token_endpoint', 'jwks_uri', + 'response_types_supported', 'subject_types_supported', 'id_token_signing_alg_values_supported', + ]; + + foreach ($requiredFields as $field) { + if (!isset($data[$field])) { + $result['missingFields'][] = $field; + } } + } else { + // Set isReachable to false if http code wasn't 200 + $result['isReachable'] = false; } - } else { - // Set isReachable to false if http code wasnt 200 + } catch (Exception $e) { $result['isReachable'] = false; } - + return $result; } From ee13ccd1781789b96e72ae263c1f66ab55a26df5 Mon Sep 17 00:00:00 2001 From: Florian Klinger Date: Fri, 5 Jan 2024 14:57:01 +0100 Subject: [PATCH 3/4] Removed unnecessary lines and added error reporting Signed-off-by: Florian Klinger --- lib/Controller/SettingsController.php | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php index da4ff0d9..589e2193 100644 --- a/lib/Controller/SettingsController.php +++ b/lib/Controller/SettingsController.php @@ -38,6 +38,7 @@ use OCP\Http\Client\IClientService; use OCP\IRequest; use OCP\Security\ICrypto; +use Psr\Log\LoggerInterface; class SettingsController extends Controller { @@ -51,6 +52,8 @@ class SettingsController extends Controller { private $crypto; /** @var IClientService */ private $clientService; + /** @var LoggerInterface */ + private $logger; public function __construct( IRequest $request, @@ -58,7 +61,8 @@ public function __construct( ID4MeService $id4meService, ProviderService $providerService, ICrypto $crypto, - IClientService $clientService + IClientService $clientService, + LoggerInterface $logger ) { parent::__construct(Application::APP_ID, $request); @@ -67,6 +71,7 @@ public function __construct( $this->providerService = $providerService; $this->crypto = $crypto; $this->clientService = $clientService; + $this->logger = $logger; } public function isDiscoveryEndpointValid($url) { @@ -97,12 +102,9 @@ public function isDiscoveryEndpointValid($url) { $result['missingFields'][] = $field; } } - } else { - // Set isReachable to false if http code wasn't 200 - $result['isReachable'] = false; } } catch (Exception $e) { - $result['isReachable'] = false; + $this->logger->error('Error - discovery endpoint validation: ' . $e); } return $result; From 10c0ea208cbe2a913e4656cf6f8d338f650a1361 Mon Sep 17 00:00:00 2001 From: Julien Veyssier Date: Tue, 23 Jan 2024 16:42:25 +0100 Subject: [PATCH 4/4] polish #750 Signed-off-by: Julien Veyssier --- lib/Controller/SettingsController.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php index 589e2193..89fdfefc 100644 --- a/lib/Controller/SettingsController.php +++ b/lib/Controller/SettingsController.php @@ -87,7 +87,7 @@ public function isDiscoveryEndpointValid($url) { $body = $response->getBody(); // Check if the request was successful - if ($httpCode == 200 && !empty($body)) { + if ($httpCode === Http::STATUS_OK && !empty($body)) { $result['isReachable'] = true; $data = json_decode($body, true); @@ -104,7 +104,7 @@ public function isDiscoveryEndpointValid($url) { } } } catch (Exception $e) { - $this->logger->error('Error - discovery endpoint validation: ' . $e); + $this->logger->error('Discovery endpoint validation error', ['exception' => $e]); } return $result;