From d640411adb4d828cffefd1248407eb93db2eaee2 Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Tue, 16 Apr 2024 15:19:51 +0100
Subject: [PATCH] OIDC: Cleaned up provider settings, added extra validation

- Added endpoint validation to ensure HTTPS as per spec
- Added some missing types
- Removed redirectUri from OidcProviderSettings since it's not a
  provider-based setting, but a setting for the oauth client, so
  extracted that back to service.
---
 app/Access/Oidc/OidcProviderSettings.php | 19 +++++++++++++------
 app/Access/Oidc/OidcService.php          |  6 ++++--
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/app/Access/Oidc/OidcProviderSettings.php b/app/Access/Oidc/OidcProviderSettings.php
index 49ccab6f0..616bf1012 100644
--- a/app/Access/Oidc/OidcProviderSettings.php
+++ b/app/Access/Oidc/OidcProviderSettings.php
@@ -18,7 +18,6 @@ class OidcProviderSettings
     public string $issuer;
     public string $clientId;
     public string $clientSecret;
-    public ?string $redirectUri;
     public ?string $authorizationEndpoint;
     public ?string $tokenEndpoint;
     public ?string $endSessionEndpoint;
@@ -38,7 +37,7 @@ class OidcProviderSettings
     /**
      * Apply an array of settings to populate setting properties within this class.
      */
-    protected function applySettingsFromArray(array $settingsArray)
+    protected function applySettingsFromArray(array $settingsArray): void
     {
         foreach ($settingsArray as $key => $value) {
             if (property_exists($this, $key)) {
@@ -52,7 +51,7 @@ class OidcProviderSettings
      *
      * @throws InvalidArgumentException
      */
-    protected function validateInitial()
+    protected function validateInitial(): void
     {
         $required = ['clientId', 'clientSecret', 'redirectUri', 'issuer'];
         foreach ($required as $prop) {
@@ -74,12 +73,20 @@ class OidcProviderSettings
     public function validate(): void
     {
         $this->validateInitial();
+
         $required = ['keys', 'tokenEndpoint', 'authorizationEndpoint'];
         foreach ($required as $prop) {
             if (empty($this->$prop)) {
                 throw new InvalidArgumentException("Missing required configuration \"{$prop}\" value");
             }
         }
+
+        $endpointProperties = ['tokenEndpoint', 'authorizationEndpoint', 'userinfoEndpoint'];
+        foreach ($endpointProperties as $prop) {
+            if (is_string($this->$prop) && !str_starts_with($this->$prop, 'https://')) {
+                throw new InvalidArgumentException("Endpoint value for \"{$prop}\" must start with https://");
+            }
+        }
     }
 
     /**
@@ -87,7 +94,7 @@ class OidcProviderSettings
      *
      * @throws OidcIssuerDiscoveryException
      */
-    public function discoverFromIssuer(ClientInterface $httpClient, Repository $cache, int $cacheMinutes)
+    public function discoverFromIssuer(ClientInterface $httpClient, Repository $cache, int $cacheMinutes): void
     {
         try {
             $cacheKey = 'oidc-discovery::' . $this->issuer;
@@ -180,9 +187,9 @@ class OidcProviderSettings
     /**
      * Get the settings needed by an OAuth provider, as a key=>value array.
      */
-    public function arrayForProvider(): array
+    public function arrayForOAuthProvider(): array
     {
-        $settingKeys = ['clientId', 'clientSecret', 'redirectUri', 'authorizationEndpoint', 'tokenEndpoint', 'userinfoEndpoint'];
+        $settingKeys = ['clientId', 'clientSecret', 'authorizationEndpoint', 'tokenEndpoint', 'userinfoEndpoint'];
         $settings = [];
         foreach ($settingKeys as $setting) {
             $settings[$setting] = $this->$setting;
diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php
index 467e31417..00ac2b6dc 100644
--- a/app/Access/Oidc/OidcService.php
+++ b/app/Access/Oidc/OidcService.php
@@ -91,7 +91,6 @@ class OidcService
             'issuer'                => $config['issuer'],
             'clientId'              => $config['client_id'],
             'clientSecret'          => $config['client_secret'],
-            'redirectUri'           => url('/oidc/callback'),
             'authorizationEndpoint' => $config['authorization_endpoint'],
             'tokenEndpoint'         => $config['token_endpoint'],
             'endSessionEndpoint'    => is_string($config['end_session_endpoint']) ? $config['end_session_endpoint'] : null,
@@ -130,7 +129,10 @@ class OidcService
      */
     protected function getProvider(OidcProviderSettings $settings): OidcOAuthProvider
     {
-        $provider = new OidcOAuthProvider($settings->arrayForProvider(), [
+        $provider = new OidcOAuthProvider([
+            ...$settings->arrayForOAuthProvider(),
+            'redirectUri' => url('/oidc/callback'),
+        ], [
             'httpClient'     => $this->http->buildClient(5),
             'optionProvider' => new HttpBasicAuthOptionProvider(),
         ]);