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

diff --git a/app/Auth/Access/Oidc/OidcJwtSigningKey.php b/app/Auth/Access/Oidc/OidcJwtSigningKey.php
index 012a6cbf9c10c72a7a27a26883f8a23173e58f72..f003ec93ca1c1df3f2c80ef9a3a940e2d90697fe 100644 (file)
--- 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}");
         }
 
             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']}");
         }
 
             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 d15705782721f0963cfc670060a7d7476b904fe0..30ec8cac8b8e867438a0630b9097f2ab6eeaf53e 100644 (file)
--- 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
 {
  */
 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[]
      */
 
     /**
      * @var string[]|array[]
      */

-    public $keys = [];
+    public ?array $keys = [];

 
     public function __construct(array $settings)
     {
 
     public function __construct(array $settings)
     {


@@ -164,9 +141,10 @@ class OidcProviderSettings
     protected function filterKeys(array $keys): array
     {
         return array_filter($keys, function (array $key) {
     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 c9c3cc51180277c754625eb0617b9663e23176d2..a9323d4233109e544bd94ec044fa4e57530a2cd9 100644 (file)
--- 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);
     {
         $settings = $this->getProviderSettings();
         $provider = $this->getProvider($settings);

-

         return [
             'url'   => $provider->getAuthorizationUrl(),
             'state' => $provider->getState(),
         return [
             'url'   => $provider->getAuthorizationUrl(),
             'state' => $provider->getState(),

diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php
index c015adb8619e410bb0adfc97d05e735dfcb278a4..db1f87bd5676a585ddb9867a7636cbc841211657 100644 (file)
--- a/tests/Auth/OidcTest.php
+++ b/tests/Auth/OidcTest.php
@@ -360,6 +360,37 @@ class OidcTest extends TestCase
         $this->assertTrue(auth()->check());
     }
 
         $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([
     public function test_login_group_sync()
     {
         config()->set([