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..2bd6161 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); - + $causeMessage = 'Original cause message'; $stubRequest = $this->createStub(Request::class); - $exception = new AuthenticationException(); - - $this->authenticator->onAuthenticationFailure($stubRequest, $exception); + $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($causeMessage, $thrown->getMessage(), 'Cause message must be preserved for logs'); + } } public function testValidateClaimsWrongState(): void @@ -85,10 +90,11 @@ public function testValidateClaimsMissingCode(): void $this->authenticator->authenticate($request); } - public function testValidateClaimsCodeDoesNotValidate(): void + public function testValidateClaimsBubblesClaimsExceptionUnchanged(): void { + $claimsMessage = 'ID token has incorrect nonce'; $stubProvider = $this->createStub(OpenIdConfigurationProvider::class); - $stubProvider->method('validateIdToken')->willThrowException(new ClaimsException('test message')); + $stubProvider->method('validateIdToken')->willThrowException(new ClaimsException($claimsMessage)); $this->stubProviderManager->method('getProvider')->willReturn($stubProvider); $request = $this->createStub(Request::class); @@ -96,8 +102,28 @@ 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($claimsMessage); + $this->authenticator->authenticate($request); + } + + public function testValidateClaimsBubblesHttpExceptionUnchanged(): void + { + $httpMessage = 'Connection timed out'; + $stubProvider = $this->createStub(OpenIdConfigurationProvider::class); + $stubProvider->method('getIdToken')->willThrowException(new HttpException($httpMessage)); + $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($httpMessage); $this->authenticator->authenticate($request); }