From 222bf326f4c1158a3cd0a02f5b1f8f55d6515653 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Thu, 7 May 2026 09:53:30 +0200 Subject: [PATCH 1/5] Preserve specific exception types and causes through the authenticator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit validateClaims() no longer collapses library exceptions into a generic ValidationException. Specific subtypes (HttpException, CodeException, ClaimsException, etc.) now bubble up unchanged so callers can distinguish a timeout from a bad nonce from a malformed token. onAuthenticationFailure() now chains the original exception via `previous` and includes its message, so logs and error reporters retain the cause. Symfony's security still renders only the safe message key to the user. BC-safe: all library exceptions extend ItkOpenIdConnectException, which the method already declared in @throws — callers catching the parent keep working. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 12 +++++ src/Security/OpenIdLoginAuthenticator.php | 27 ++++++------ .../Security/OpenIdLoginAuthenticatorTest.php | 44 ++++++++++++++----- 3 files changed, 60 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bc2cc8b..e88aa6d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- `OpenIdLoginAuthenticator::validateClaims()` no longer collapses library + exceptions into a generic `ValidationException`. Specific subtypes + (`HttpException`, `CodeException`, `ClaimsException`, `JsonException`, + `CacheException`, etc.) bubble up unchanged so callers can react to the + actual failure mode (e.g., timeout vs. nonce mismatch). All subtypes + already extend `ItkOpenIdConnectException`, which the method's `@throws` + declaration already covered, so any caller catching the parent keeps + working. +- `OpenIdLoginAuthenticator::onAuthenticationFailure()` now chains the + original exception via `previous` and includes its message, so logs and + error reporters retain the cause. Symfony's security still renders only + the safe message key to the user. - Bumped actions/checkout from v5 to v6 in all GitHub workflows - Renamed PHP_EXEC variable to PHP in Taskfile - Added lint:composer task to Taskfile diff --git a/src/Security/OpenIdLoginAuthenticator.php b/src/Security/OpenIdLoginAuthenticator.php index 2379900..be11103 100644 --- a/src/Security/OpenIdLoginAuthenticator.php +++ b/src/Security/OpenIdLoginAuthenticator.php @@ -60,21 +60,19 @@ protected function validateClaims(Request $request): array throw new ValidationException('Nonce empty or not found'); } - try { - $code = $request->query->get('code'); + $code = $request->query->get('code'); - if (!is_string($code)) { - throw new ValidationException('Missing or invalid code'); - } - - $idToken = $provider->getIdToken($code); - $claims = $provider->validateIdToken($idToken, $oauth2nonce); - // Authentication successful - } catch (ItkOpenIdConnectException $exception) { - // Handle failed authentication - throw new ValidationException($exception->getMessage()); + if (!is_string($code)) { + throw new ValidationException('Missing or invalid code'); } + // Library exceptions (HttpException, CodeException, ClaimsException, + // ValidationException, JsonException, CacheException, KeyException, + // DecodeException) all extend ItkOpenIdConnectException and bubble up + // unchanged so callers can react to specific failure modes. + $idToken = $provider->getIdToken($code); + $claims = $provider->validateIdToken($idToken, $oauth2nonce); + /** @var array $claimsArray */ $claimsArray = (array) $claims; @@ -83,6 +81,9 @@ protected function validateClaims(Request $request): array public function onAuthenticationFailure(Request $request, AuthenticationException $exception): ?Response { - throw new AuthenticationException('Error occurred validating openid login'); + // Preserve the cause so logs and error reporters can see what actually + // failed (timeout, signature mismatch, wrong nonce, etc.). Symfony's + // security renders only the safe message key to the user. + throw new AuthenticationException(sprintf('Error occurred validating openid login: %s', $exception->getMessage()), $exception->getCode(), $exception); } } diff --git a/tests/Security/OpenIdLoginAuthenticatorTest.php b/tests/Security/OpenIdLoginAuthenticatorTest.php index 238b468..79b3409 100644 --- a/tests/Security/OpenIdLoginAuthenticatorTest.php +++ b/tests/Security/OpenIdLoginAuthenticatorTest.php @@ -3,6 +3,7 @@ namespace ItkDev\OpenIdConnectBundle\Tests\Security; use ItkDev\OpenIdConnect\Exception\ClaimsException; +use ItkDev\OpenIdConnect\Exception\HttpException; use ItkDev\OpenIdConnect\Exception\ValidationException; use ItkDev\OpenIdConnect\Security\OpenIdConfigurationProvider; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; @@ -39,14 +40,18 @@ public function testSupports(): void $this->assertTrue($this->authenticator->supports($request)); } - public function testOnAuthenticationFailure(): void + public function testOnAuthenticationFailurePreservesCause(): void { - $this->expectException(AuthenticationException::class); - $stubRequest = $this->createStub(Request::class); - $exception = new AuthenticationException(); - - $this->authenticator->onAuthenticationFailure($stubRequest, $exception); + $cause = new AuthenticationException('Original cause message'); + + try { + $this->authenticator->onAuthenticationFailure($stubRequest, $cause); + $this->fail('Expected AuthenticationException'); + } catch (AuthenticationException $thrown) { + $this->assertSame($cause, $thrown->getPrevious(), 'Original exception must be chained as previous'); + $this->assertStringContainsString('Original cause message', $thrown->getMessage(), 'Cause message must be preserved for logs'); + } } public function testValidateClaimsWrongState(): void @@ -85,10 +90,10 @@ public function testValidateClaimsMissingCode(): void $this->authenticator->authenticate($request); } - public function testValidateClaimsCodeDoesNotValidate(): void + public function testValidateClaimsBubblesClaimsExceptionUnchanged(): void { $stubProvider = $this->createStub(OpenIdConfigurationProvider::class); - $stubProvider->method('validateIdToken')->willThrowException(new ClaimsException('test message')); + $stubProvider->method('validateIdToken')->willThrowException(new ClaimsException('ID token has incorrect nonce')); $this->stubProviderManager->method('getProvider')->willReturn($stubProvider); $request = $this->createStub(Request::class); @@ -96,8 +101,27 @@ public function testValidateClaimsCodeDoesNotValidate(): void $this->setupStubSessionOnRequest($request); - $this->expectException(ValidationException::class); - $this->expectExceptionMessage('test message'); + // ClaimsException must reach the caller as itself, not collapsed into the + // base ItkOpenIdConnectException or ValidationException, so consumers can + // distinguish a bad nonce/issuer/audience from a network failure. + $this->expectException(ClaimsException::class); + $this->expectExceptionMessage('ID token has incorrect nonce'); + $this->authenticator->authenticate($request); + } + + public function testValidateClaimsBubblesHttpExceptionUnchanged(): void + { + $stubProvider = $this->createStub(OpenIdConfigurationProvider::class); + $stubProvider->method('getIdToken')->willThrowException(new HttpException('Connection timed out')); + $this->stubProviderManager->method('getProvider')->willReturn($stubProvider); + + $request = $this->createStub(Request::class); + $request->query = new InputBag(['state' => 'test_state', 'code' => 'test_code']); + + $this->setupStubSessionOnRequest($request); + + $this->expectException(HttpException::class); + $this->expectExceptionMessage('Connection timed out'); $this->authenticator->authenticate($request); } From 18b2c2ae2186d2cc7bfd64b58d056987d0e4a4c4 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Fri, 8 May 2026 11:33:34 +0200 Subject: [PATCH 2/5] Drop unreachable $this->fail() in onAuthenticationFailure test `onAuthenticationFailure` always throws (Assert::fail() also returns never), so the guard line was dead code per static analysis. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/Security/OpenIdLoginAuthenticatorTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/Security/OpenIdLoginAuthenticatorTest.php b/tests/Security/OpenIdLoginAuthenticatorTest.php index 79b3409..0251aee 100644 --- a/tests/Security/OpenIdLoginAuthenticatorTest.php +++ b/tests/Security/OpenIdLoginAuthenticatorTest.php @@ -47,7 +47,6 @@ public function testOnAuthenticationFailurePreservesCause(): void try { $this->authenticator->onAuthenticationFailure($stubRequest, $cause); - $this->fail('Expected AuthenticationException'); } catch (AuthenticationException $thrown) { $this->assertSame($cause, $thrown->getPrevious(), 'Original exception must be chained as previous'); $this->assertStringContainsString('Original cause message', $thrown->getMessage(), 'Cause message must be preserved for logs'); From 4faf34dd80b388f3bd6d3f28757c6dfe3b602c76 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Fri, 8 May 2026 11:34:48 +0200 Subject: [PATCH 3/5] Extract repeated cause message in onAuthenticationFailure test Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/Security/OpenIdLoginAuthenticatorTest.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/Security/OpenIdLoginAuthenticatorTest.php b/tests/Security/OpenIdLoginAuthenticatorTest.php index 0251aee..35ad86c 100644 --- a/tests/Security/OpenIdLoginAuthenticatorTest.php +++ b/tests/Security/OpenIdLoginAuthenticatorTest.php @@ -42,14 +42,15 @@ public function testSupports(): void public function testOnAuthenticationFailurePreservesCause(): void { + $causeMessage = 'Original cause message'; $stubRequest = $this->createStub(Request::class); - $cause = new AuthenticationException('Original cause message'); + $cause = new AuthenticationException($causeMessage); try { $this->authenticator->onAuthenticationFailure($stubRequest, $cause); } catch (AuthenticationException $thrown) { $this->assertSame($cause, $thrown->getPrevious(), 'Original exception must be chained as previous'); - $this->assertStringContainsString('Original cause message', $thrown->getMessage(), 'Cause message must be preserved for logs'); + $this->assertStringContainsString($causeMessage, $thrown->getMessage(), 'Cause message must be preserved for logs'); } } From 455fb6d231a988813e8d5bd1aa7c4dbd3914c46f Mon Sep 17 00:00:00 2001 From: turegjorup Date: Fri, 8 May 2026 11:35:33 +0200 Subject: [PATCH 4/5] Extract repeated claims message in ClaimsException test Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/Security/OpenIdLoginAuthenticatorTest.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/Security/OpenIdLoginAuthenticatorTest.php b/tests/Security/OpenIdLoginAuthenticatorTest.php index 35ad86c..c52c2b9 100644 --- a/tests/Security/OpenIdLoginAuthenticatorTest.php +++ b/tests/Security/OpenIdLoginAuthenticatorTest.php @@ -92,8 +92,9 @@ public function testValidateClaimsMissingCode(): void public function testValidateClaimsBubblesClaimsExceptionUnchanged(): void { + $claimsMessage = 'ID token has incorrect nonce'; $stubProvider = $this->createStub(OpenIdConfigurationProvider::class); - $stubProvider->method('validateIdToken')->willThrowException(new ClaimsException('ID token has incorrect nonce')); + $stubProvider->method('validateIdToken')->willThrowException(new ClaimsException($claimsMessage)); $this->stubProviderManager->method('getProvider')->willReturn($stubProvider); $request = $this->createStub(Request::class); @@ -105,7 +106,7 @@ public function testValidateClaimsBubblesClaimsExceptionUnchanged(): void // base ItkOpenIdConnectException or ValidationException, so consumers can // distinguish a bad nonce/issuer/audience from a network failure. $this->expectException(ClaimsException::class); - $this->expectExceptionMessage('ID token has incorrect nonce'); + $this->expectExceptionMessage($claimsMessage); $this->authenticator->authenticate($request); } From d60e0f9b3525566c6dbca4877f36b23d34fd5c6c Mon Sep 17 00:00:00 2001 From: turegjorup Date: Fri, 8 May 2026 11:35:59 +0200 Subject: [PATCH 5/5] Extract repeated http message in HttpException test Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/Security/OpenIdLoginAuthenticatorTest.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/Security/OpenIdLoginAuthenticatorTest.php b/tests/Security/OpenIdLoginAuthenticatorTest.php index c52c2b9..2bd6161 100644 --- a/tests/Security/OpenIdLoginAuthenticatorTest.php +++ b/tests/Security/OpenIdLoginAuthenticatorTest.php @@ -112,8 +112,9 @@ public function testValidateClaimsBubblesClaimsExceptionUnchanged(): void public function testValidateClaimsBubblesHttpExceptionUnchanged(): void { + $httpMessage = 'Connection timed out'; $stubProvider = $this->createStub(OpenIdConfigurationProvider::class); - $stubProvider->method('getIdToken')->willThrowException(new HttpException('Connection timed out')); + $stubProvider->method('getIdToken')->willThrowException(new HttpException($httpMessage)); $this->stubProviderManager->method('getProvider')->willReturn($stubProvider); $request = $this->createStub(Request::class); @@ -122,7 +123,7 @@ public function testValidateClaimsBubblesHttpExceptionUnchanged(): void $this->setupStubSessionOnRequest($request); $this->expectException(HttpException::class); - $this->expectExceptionMessage('Connection timed out'); + $this->expectExceptionMessage($httpMessage); $this->authenticator->authenticate($request); }