improved error handling and tests added

app/Access/Controllers/OidcController.php

@@ -55,6 +55,7 @@ class OidcController extends Controller
         }
 
         try {
+            $this->throwIfAuthorizationError($request);
             $this->oidcService->processAuthorizeResponse($request->query('code'));
         } catch (OidcException $oidcException) {
             $this->showErrorNotification($oidcException->getMessage());
@@ -72,4 +73,23 @@ class OidcController extends Controller
     {
         return redirect($this->oidcService->logout());
     }
+
+    /**
+     *
+     * @throws OidcException
+     */
+    private function throwIfAuthorizationError(Request $request): void
+    {
+        $errorCode = $request->query('error');
+        if (!$errorCode) {
+            return;
+        }
+
+        $errorDescription = $request->query('error_description');
+        if ($errorDescription) {
+            throw new OidcException($errorDescription);
+        }
+
+        throw new OidcException(trans('errors.oidc_fail_authed', ['system' => config('oidc.name')]));
+    }
 }

app/Access/Oidc/OidcHttpBasicWithClientIdOptionProvider.php

@@ -0,0 +1,27 @@
+<?php
+
+namespace BookStack\Access\Oidc;
+
+use League\OAuth2\Client\OptionProvider\HttpBasicAuthOptionProvider;
+
+/**
+ * Option provider that sends credentials via HTTP Basic Auth header
+ * and also includes client_id in the request body.
+ */
+class OidcHttpBasicWithClientIdOptionProvider extends HttpBasicAuthOptionProvider
+{
+    public function getAccessTokenOptions($method, array $params)
+    {
+        $clientId = $params['client_id'] ?? null;
+
+        $options = parent::getAccessTokenOptions($method, $params);
+
+        if ($clientId) {
+            parse_str($options['body'] ?? '', $body);
+            $body['client_id'] = $clientId;
+            $options['body'] = http_build_query($body);
+        }
+
+        return $options;
+    }
+}

app/Access/Oidc/OidcIdToken.php

@@ -9,10 +9,10 @@ class OidcIdToken extends OidcJwtWithClaims implements ProvidesClaims
      *
      * @throws OidcInvalidTokenException
      */
-    public function validate(string $clientId): bool
+    public function validate(OidcProviderSettings $settings): bool
     {
-        parent::validateCommonTokenDetails($clientId);
-        $this->validateTokenClaims($clientId);
+        parent::validateCommonTokenDetails($settings);
+        $this->validateTokenClaims($settings->clientId);
 
         return true;
     }

app/Access/Oidc/OidcJwtWithClaims.php

@@ -9,7 +9,9 @@ class OidcJwtWithClaims implements ProvidesClaims
     protected string $signature;
     protected string $issuer;
     protected array $tokenParts = [];
-
+    protected array $acceptedSignatures = [self::hs256Signature, self::rs256Signature];
+    private const hs256Signature = 'HS256'
+        , rs256Signature = 'RS256';
     /**
      * @var array[]|string[]
      */
@@ -59,11 +61,11 @@ class OidcJwtWithClaims implements ProvidesClaims
      *
      * @throws OidcInvalidTokenException
      */
-    public function validateCommonTokenDetails(string $clientId): bool
+    public function validateCommonTokenDetails(OidcProviderSettings $settings): bool
     {
         $this->validateTokenStructure();
-        $this->validateTokenSignature();
-        $this->validateCommonClaims($clientId);
+        $this->validateTokenSignature($settings);
+        $this->validateCommonClaims($settings->clientId);
 
         return true;
     }
@@ -102,12 +104,12 @@ class OidcJwtWithClaims implements ProvidesClaims
     protected function validateTokenStructure(): void
     {
         foreach (['header', 'payload'] as $prop) {
-            if (empty($this->$prop)) {
+            if (empty($this->$prop) || !is_array($this->$prop)) {
                 throw new OidcInvalidTokenException("Could not parse out a valid {$prop} within the provided token");
             }
         }
 
-        if (empty($this->signature)) {
+        if (empty($this->signature) || !is_string($this->signature)) {
             throw new OidcInvalidTokenException('Could not parse out a valid signature within the provided token');
         }
     }
@@ -117,31 +119,42 @@ class OidcJwtWithClaims implements ProvidesClaims
      *
      * @throws OidcInvalidTokenException
      */
-    protected function validateTokenSignature(): void
-    {
-        if ($this->header['alg'] !== 'RS256') {
-            throw new OidcInvalidTokenException("Only RS256 signature validation is supported. Token reports using {$this->header['alg']}");
+    protected function validateTokenSignature(OidcProviderSettings $settings): void {  
+        $validSignatures = implode(', ',$this->acceptedSignatures);
+        switch ($this->header['alg']) {
+            case self::rs256Signature:
+                $parsedKeys = array_map(function ($key) {
+                    try {
+                        return new OidcJwtSigningKey($key);
+                    } catch (OidcInvalidKeyException $e) {
+                        throw new OidcInvalidTokenException('Failed to read signing key with error: ' . $e->getMessage());
+                    }
+                }, $this->keys);
+
+                $parsedKeys = array_filter($parsedKeys);
+
+                $contentToSign = $this->tokenParts[0] . '.' . $this->tokenParts[1];
+                /** @var OidcJwtSigningKey $parsedKey */
+                foreach ($parsedKeys as $parsedKey) {
+                    if ($parsedKey->verify($contentToSign, $this->signature)) {
+                        return;
+                    }
+                }
+            
+                throw new OidcInvalidTokenException('Token signature could not be validated using the provided keys');                
+            case self::hs256Signature:
+                $secret = $settings->clientSecret;
+                $contentToSign = $this->tokenParts[0] . '.' . $this->tokenParts[1];
+                $expectedSignature = hash_hmac('sha256', $contentToSign, $secret, true);
+
+                if (hash_equals($expectedSignature, $this->signature)) {
+                    return;
+                }
+            
+                throw new OidcInvalidTokenException('Token signature could not be validated using the provided secret');                
+            default:
+                throw new OidcInvalidTokenException("Only $validSignatures signatures validation are supported. Token reports using {$this->header['alg']}");
         }
-
-        $parsedKeys = array_map(function ($key) {
-            try {
-                return new OidcJwtSigningKey($key);
-            } catch (OidcInvalidKeyException $e) {
-                throw new OidcInvalidTokenException('Failed to read signing key with error: ' . $e->getMessage());
-            }
-        }, $this->keys);
-
-        $parsedKeys = array_filter($parsedKeys);
-
-        $contentToSign = $this->tokenParts[0] . '.' . $this->tokenParts[1];
-        /** @var OidcJwtSigningKey $parsedKey */
-        foreach ($parsedKeys as $parsedKey) {
-            if ($parsedKey->verify($contentToSign, $this->signature)) {
-                return;
-            }
-        }
-
-        throw new OidcInvalidTokenException('Token signature could not be validated using the provided keys');
     }
 
     /**

app/Access/Oidc/OidcProviderSettings.php

@@ -74,7 +74,7 @@ class OidcProviderSettings
     {
         $this->validateInitial();
 
-        $required = ['keys', 'tokenEndpoint', 'authorizationEndpoint'];
+        $required = ['tokenEndpoint', 'authorizationEndpoint'];
         foreach ($required as $prop) {
             if (empty($this->$prop)) {
                 throw new InvalidArgumentException("Missing required configuration \"{$prop}\" value");

app/Access/Oidc/OidcService.php

@@ -14,7 +14,6 @@ use BookStack\Theming\ThemeEvents;
 use BookStack\Uploads\UserAvatars;
 use BookStack\Users\Models\User;
 use Illuminate\Support\Facades\Cache;
-use League\OAuth2\Client\OptionProvider\HttpBasicAuthOptionProvider;
 use League\OAuth2\Client\Provider\Exception\IdentityProviderException;
 
 /**
@@ -140,7 +139,7 @@ class OidcService
             'redirectUri' => url('/oidc/callback'),
         ], [
             'httpClient'     => $this->http->buildClient(5),
-            'optionProvider' => new HttpBasicAuthOptionProvider(),
+            'optionProvider' => new OidcHttpBasicWithClientIdOptionProvider(),
         ]);
 
         foreach ($this->getAdditionalScopes() as $scope) {
@@ -199,7 +198,7 @@ class OidcService
         }
 
         try {
-            $idToken->validate($settings->clientId);
+            $idToken->validate($settings);
         } catch (OidcInvalidTokenException $exception) {
             throw new OidcException("ID token validation failed with error: {$exception->getMessage()}");
         }

tests/Helpers/OidcJwtHelper.php

@@ -2,6 +2,7 @@
 
 namespace Tests\Helpers;
 
+use BookStack\Access\Oidc\OidcProviderSettings;
 use phpseclib3\Crypt\RSA;
 
 /**
@@ -119,6 +120,41 @@ q/1PY4iJviGKddtmfClH3v4=
 -----END PRIVATE KEY-----';
     }
 
+    public static function defaultSecret(): string
+    {
+        return 'test-client-secret-for-hs256';
+    }
+
+    /**
+     * Build a minimal OidcProviderSettings for use in token validation tests.
+     */
+    public static function defaultProviderSettings(array $overrides = []): OidcProviderSettings
+    {
+        return new OidcProviderSettings(array_merge([
+            'issuer'       => static::defaultIssuer(),
+            'clientId'     => static::defaultClientId(),
+            'clientSecret' => static::defaultSecret(),
+        ], $overrides));
+    }
+
+    /**
+     * Build an HS256-signed ID token using the given secret.
+     */
+    public static function hs256IdToken(string $secret, array $payloadOverrides = []): string
+    {
+        $payload = array_merge(static::defaultPayload(), $payloadOverrides);
+        $header = ['alg' => 'HS256', 'typ' => 'JWT'];
+
+        $top = implode('.', [
+            static::base64UrlEncode(json_encode($header)),
+            static::base64UrlEncode(json_encode($payload)),
+        ]);
+
+        $signature = hash_hmac('sha256', $top, $secret, true);
+
+        return $top . '.' . static::base64UrlEncode($signature);
+    }
+
     public static function publicJwkKeyArray(): array
     {
         return [

tests/Unit/OidcIdTokenTest.php

@@ -15,7 +15,7 @@ class OidcIdTokenTest extends TestCase
             OidcJwtHelper::publicJwkKeyArray(),
         ]);
 
-        $this->assertTrue($token->validate('xxyyzz.aaa.bbccdd.123'));
+        $this->assertTrue($token->validate(OidcJwtHelper::defaultProviderSettings()));
     }
 
     public function test_get_claim_returns_value_if_existing()
@@ -56,7 +56,7 @@ class OidcIdTokenTest extends TestCase
             $err = null;
 
             try {
-                $token->validate('abc');
+                $token->validate(OidcJwtHelper::defaultProviderSettings());
             } catch (\Exception $exception) {
                 $err = $exception;
             }
@@ -71,7 +71,7 @@ class OidcIdTokenTest extends TestCase
         $token = new OidcIdToken(OidcJwtHelper::idToken(), OidcJwtHelper::defaultIssuer(), []);
         $this->expectException(OidcInvalidTokenException::class);
         $this->expectExceptionMessage('Token signature could not be validated using the provided keys');
-        $token->validate('abc');
+        $token->validate(OidcJwtHelper::defaultProviderSettings());
     }
 
     public function test_error_thrown_if_token_signature_not_validated_from_non_matching_key()
@@ -83,7 +83,7 @@ class OidcIdTokenTest extends TestCase
         ]);
         $this->expectException(OidcInvalidTokenException::class);
         $this->expectExceptionMessage('Token signature could not be validated using the provided keys');
-        $token->validate('abc');
+        $token->validate(OidcJwtHelper::defaultProviderSettings());
     }
 
     public function test_error_thrown_if_invalid_key_provided()
@@ -91,15 +91,34 @@ class OidcIdTokenTest extends TestCase
         $token = new OidcIdToken(OidcJwtHelper::idToken(), OidcJwtHelper::defaultIssuer(), ['url://example.com']);
         $this->expectException(OidcInvalidTokenException::class);
         $this->expectExceptionMessage('Unexpected type of key value provided');
-        $token->validate('abc');
+        $token->validate(OidcJwtHelper::defaultProviderSettings());
     }
 
-    public function test_error_thrown_if_token_algorithm_is_not_rs256()
+    public function test_error_thrown_if_token_algorithm_is_not_supported()
     {
-        $token = new OidcIdToken(OidcJwtHelper::idToken([], ['alg' => 'HS256']), OidcJwtHelper::defaultIssuer(), []);
+        $token = new OidcIdToken(OidcJwtHelper::idToken([], ['alg' => 'ES256']), OidcJwtHelper::defaultIssuer(), []);
         $this->expectException(OidcInvalidTokenException::class);
-        $this->expectExceptionMessage('Only RS256 signature validation is supported. Token reports using HS256');
-        $token->validate('abc');
+        $this->expectExceptionMessage('Only HS256, RS256 signatures validation are supported. Token reports using ES256');
+        $token->validate(OidcJwtHelper::defaultProviderSettings());
+    }
+
+    public function test_hs256_token_passes_validation_with_correct_secret()
+    {
+        $secret = OidcJwtHelper::defaultSecret();
+        $token = new OidcIdToken(OidcJwtHelper::hs256IdToken($secret), OidcJwtHelper::defaultIssuer(), []);
+        $settings = OidcJwtHelper::defaultProviderSettings(['clientSecret' => $secret]);
+
+        $this->assertTrue($token->validate($settings));
+    }
+
+    public function test_hs256_token_fails_validation_with_wrong_secret()
+    {
+        $token = new OidcIdToken(OidcJwtHelper::hs256IdToken('correct-secret'), OidcJwtHelper::defaultIssuer(), []);
+        $settings = OidcJwtHelper::defaultProviderSettings(['clientSecret' => 'wrong-secret']);
+
+        $this->expectException(OidcInvalidTokenException::class);
+        $this->expectExceptionMessage('Token signature could not be validated using the provided secret');
+        $token->validate($settings);
     }
 
     public function test_token_claim_error_cases()
@@ -141,7 +160,7 @@ class OidcIdTokenTest extends TestCase
             $err = null;
 
             try {
-                $token->validate('xxyyzz.aaa.bbccdd.123');
+                $token->validate(OidcJwtHelper::defaultProviderSettings());
             } catch (\Exception $exception) {
                 $err = $exception;
             }
@@ -160,7 +179,7 @@ class OidcIdTokenTest extends TestCase
             $testFilePath,
         ]);
 
-        $this->assertTrue($token->validate('xxyyzz.aaa.bbccdd.123'));
+        $this->assertTrue($token->validate(OidcJwtHelper::defaultProviderSettings()));
         unlink($testFilePath);
     }
 }