From e20c944350c78f33c1ed0c34db8fde6a8a639b3d Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Wed, 23 Nov 2022 11:50:59 +0000
Subject: [PATCH] Fixed OIDC handling when no JWKS 'use' prop exists

Now assume, based on OIDC discovery spec, that keys without 'use' are
'sig' keys. Should not affect existing use-cases since existance of such
keys would have throw exceptions in prev. versions of bookstack.

For #3869
---
 app/Auth/Access/Oidc/OidcJwtSigningKey.php    |  9 ++--
 app/Auth/Access/Oidc/OidcProviderSettings.php | 42 +++++--------------
 app/Auth/Access/Oidc/OidcService.php          |  1 -
 tests/Auth/OidcTest.php                       | 31 ++++++++++++++
 4 files changed, 45 insertions(+), 38 deletions(-)

diff --git a/app/Auth/Access/Oidc/OidcJwtSigningKey.php b/app/Auth/Access/Oidc/OidcJwtSigningKey.php
index 012a6cbf9..f003ec93c 100644
--- a/app/Auth/Access/Oidc/OidcJwtSigningKey.php
+++ b/app/Auth/Access/Oidc/OidcJwtSigningKey.php
@@ -67,11 +67,10 @@ class OidcJwtSigningKey
             throw new OidcInvalidKeyException("Only RS256 keys are currently supported. Found key using {$alg}");
         }
 
-        if (empty($jwk['use'])) {
-            throw new OidcInvalidKeyException('A "use" parameter on the provided key is expected');
-        }
-
-        if ($jwk['use'] !== 'sig') {
+        // 'use' is optional for a JWK but we assume 'sig' where no value exists since that's what
+        // the OIDC discovery spec infers since 'sig' MUST be set if encryption keys come into play.
+        $use = $jwk['use'] ?? 'sig';
+        if ($use !== 'sig') {
             throw new OidcInvalidKeyException("Only signature keys are currently supported. Found key for use {$jwk['use']}");
         }
 
diff --git a/app/Auth/Access/Oidc/OidcProviderSettings.php b/app/Auth/Access/Oidc/OidcProviderSettings.php
index d15705782..30ec8cac8 100644
--- a/app/Auth/Access/Oidc/OidcProviderSettings.php
+++ b/app/Auth/Access/Oidc/OidcProviderSettings.php
@@ -15,40 +15,17 @@ use Psr\Http\Client\ClientInterface;
  */
 class OidcProviderSettings
 {
-    /**
-     * @var string
-     */
-    public $issuer;
-
-    /**
-     * @var string
-     */
-    public $clientId;
-
-    /**
-     * @var string
-     */
-    public $clientSecret;
-
-    /**
-     * @var string
-     */
-    public $redirectUri;
-
-    /**
-     * @var string
-     */
-    public $authorizationEndpoint;
-
-    /**
-     * @var string
-     */
-    public $tokenEndpoint;
+    public string $issuer;
+    public string $clientId;
+    public string $clientSecret;
+    public ?string $redirectUri;
+    public ?string $authorizationEndpoint;
+    public ?string $tokenEndpoint;
 
     /**
      * @var string[]|array[]
      */
-    public $keys = [];
+    public ?array $keys = [];
 
     public function __construct(array $settings)
     {
@@ -164,9 +141,10 @@ class OidcProviderSettings
     protected function filterKeys(array $keys): array
     {
         return array_filter($keys, function (array $key) {
-            $alg = $key['alg'] ?? null;
+            $alg = $key['alg'] ?? 'RS256';
+            $use = $key['use'] ?? 'sig';
 
-            return $key['kty'] === 'RSA' && $key['use'] === 'sig' && (is_null($alg) || $alg === 'RS256');
+            return $key['kty'] === 'RSA' && $use === 'sig' && $alg === 'RS256';
         });
     }
 
diff --git a/app/Auth/Access/Oidc/OidcService.php b/app/Auth/Access/Oidc/OidcService.php
index c9c3cc511..a9323d423 100644
--- a/app/Auth/Access/Oidc/OidcService.php
+++ b/app/Auth/Access/Oidc/OidcService.php
@@ -52,7 +52,6 @@ class OidcService
     {
         $settings = $this->getProviderSettings();
         $provider = $this->getProvider($settings);
-
         return [
             'url'   => $provider->getAuthorizationUrl(),
             'state' => $provider->getState(),
diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php
index c015adb86..db1f87bd5 100644
--- a/tests/Auth/OidcTest.php
+++ b/tests/Auth/OidcTest.php
@@ -360,6 +360,37 @@ class OidcTest extends TestCase
         $this->assertTrue(auth()->check());
     }
 
+    public function test_auth_login_with_autodiscovery_with_keys_that_do_not_have_use_property()
+    {
+        // Based on reading the OIDC discovery spec:
+        // > This contains the signing key(s) the RP uses to validate signatures from the OP. The JWK Set MAY also
+        // > contain the Server's encryption key(s), which are used by RPs to encrypt requests to the Server. When
+        // > both signing and encryption keys are made available, a use (Key Use) parameter value is REQUIRED for all
+        // > keys in the referenced JWK Set to indicate each key's intended usage.
+        // We can assume that keys without use are intended for signing.
+        $this->withAutodiscovery();
+
+        $keyArray = OidcJwtHelper::publicJwkKeyArray();
+        unset($keyArray['use']);
+
+        $this->mockHttpClient([
+            $this->getAutoDiscoveryResponse(),
+            new Response(200, [
+                'Content-Type'  => 'application/json',
+                'Cache-Control' => 'no-cache, no-store',
+                'Pragma'        => 'no-cache',
+            ], json_encode([
+                'keys' => [
+                    $keyArray,
+                ],
+            ])),
+        ]);
+
+        $this->assertFalse(auth()->check());
+        $this->runLogin();
+        $this->assertTrue(auth()->check());
+    }
+
     public function test_login_group_sync()
     {
         config()->set([