From 844c79ae72f92bdcf8a72df756f017e1ee5b3858 Mon Sep 17 00:00:00 2001 From: milnerTill Date: Mon, 20 Apr 2026 13:38:17 -0300 Subject: [PATCH] improved error handling and tests added --- app/Access/Controllers/OidcController.php | 31 +++++++++-------- lang/en/errors.php | 1 - lang/es/errors.php | 1 - lang/es_AR/errors.php | 1 - package-lock.json | 2 +- tests/Helpers/OidcJwtHelper.php | 36 ++++++++++++++++++++ tests/Unit/OidcIdTokenTest.php | 41 +++++++++++++++++------ 7 files changed, 84 insertions(+), 29 deletions(-) diff --git a/app/Access/Controllers/OidcController.php b/app/Access/Controllers/OidcController.php index 203130602..b9cd57243 100644 --- a/app/Access/Controllers/OidcController.php +++ b/app/Access/Controllers/OidcController.php @@ -55,7 +55,7 @@ class OidcController extends Controller } try { - $this->_catchCustomErrorAndFail($request); + $this->throwIfAuthorizationError($request); $this->oidcService->processAuthorizeResponse($request->query('code')); } catch (OidcException $oidcException) { $this->showErrorNotification($oidcException->getMessage()); @@ -74,19 +74,22 @@ class OidcController extends Controller return redirect($this->oidcService->logout()); } - private function _catchCustomErrorAndFail(Request $request):void { - $_errorCode = $request->query('error'); - if ($_errorCode) { - if ($_errorCode === 'need_auth') { - if ( null === ($errorMsg = $request->query('error_description')) ){ - $customError = trans('errors.oidc_need_auth', ['system' => config('oidc.name')]); - }else { - $customError = $errorMsg; - } - } - throw new OidcException($customError); + /** + * + * @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')])); } - - } diff --git a/lang/en/errors.php b/lang/en/errors.php index 89225c9fe..20537d59f 100644 --- a/lang/en/errors.php +++ b/lang/en/errors.php @@ -26,7 +26,6 @@ return [ 'oidc_already_logged_in' => 'Already logged in', 'oidc_no_email_address' => 'Could not find an email address, for this user, in the data provided by the external authentication system', 'oidc_fail_authed' => 'Login using :system failed, system did not provide successful authorization', - 'oidc_need_auth' => 'You need to be authenticated to perform this action.', 'social_no_action_defined' => 'No action defined', 'social_login_bad_response' => "Error received during :socialAccount login: \n:error", 'social_account_in_use' => 'This :socialAccount account is already in use, Try logging in via the :socialAccount option.', diff --git a/lang/es/errors.php b/lang/es/errors.php index fd16c3163..25f82a5d8 100644 --- a/lang/es/errors.php +++ b/lang/es/errors.php @@ -26,7 +26,6 @@ return [ 'oidc_already_logged_in' => 'Ya tenías la sesión iniciada', 'oidc_no_email_address' => 'No se pudo encontrar una dirección de correo electrónico, para este usuario, en los datos proporcionados por el sistema de autenticación externo', 'oidc_fail_authed' => 'El inicio de sesión con :system falló, el sistema no proporcionó una autorización correcta', - 'oidc_need_auth' => 'Necesita estar autenticado para realizar esta acción.', 'social_no_action_defined' => 'Acción no definida', 'social_login_bad_response' => "Se ha recibido un error durante el acceso con :socialAccount error: \n:error", 'social_account_in_use' => 'la cuenta :socialAccount ya se encuentra en uso, intente acceder a través de la opción :socialAccount .', diff --git a/lang/es_AR/errors.php b/lang/es_AR/errors.php index c24fde88c..6f6b25bcf 100644 --- a/lang/es_AR/errors.php +++ b/lang/es_AR/errors.php @@ -26,7 +26,6 @@ return [ 'oidc_already_logged_in' => 'Ya está conectado', 'oidc_no_email_address' => 'No se pudo encontrar una dirección de correo electrónico para este usuario en los datos proporcionados por el sistema de autenticación externo', 'oidc_fail_authed' => 'El inicio de sesión con :system falló, el sistema no proporcionó una autorización correcta', - 'oidc_need_auth' => 'Necesita estar autenticado para realizar esta acción.', 'social_no_action_defined' => 'Acción no definida', 'social_login_bad_response' => "SE recibió un Error durante el acceso con :socialAccount : \n:error", 'social_account_in_use' => 'la cuenta :socialAccount ya se encuentra en uso, intente loguearse a través de la opcón :socialAccount .', diff --git a/package-lock.json b/package-lock.json index 97bb4b7f2..d239426ec 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,5 +1,5 @@ { - "name": "openId-HS256-support", + "name": "bookstack", "lockfileVersion": 3, "requires": true, "packages": { diff --git a/tests/Helpers/OidcJwtHelper.php b/tests/Helpers/OidcJwtHelper.php index 55a34d4dc..97841a688 100644 --- a/tests/Helpers/OidcJwtHelper.php +++ b/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 [ diff --git a/tests/Unit/OidcIdTokenTest.php b/tests/Unit/OidcIdTokenTest.php index 739323266..0eda9e937 100644 --- a/tests/Unit/OidcIdTokenTest.php +++ b/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); } }