From 3b6152bc827d15fd8ed8892b4db9a393835794fc Mon Sep 17 00:00:00 2001 From: turegjorup Date: Mon, 11 May 2026 12:32:30 +0200 Subject: [PATCH] Preserve $previous in CliLoginHelper and validateClaims wraps Five wrap sites (4 in CliLoginHelper, 1 in OpenIdLoginAuthenticator::validateClaims) copied the message from the caught exception but discarded the chain. Logs lost the originating PSR cache / upstream OIDC failure, making debug traversal impossible. Add `previous: $e` to each `new CacheException(...)` / `new ValidationException(...)`. Test changes: convert the existing message-only assertions to a try/catch + early-return pattern so each test also asserts `getPrevious()` returns the original cause object. Five existing tests touched; test count and coverage unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 7 +++ src/Security/OpenIdLoginAuthenticator.php | 2 +- src/Util/CliLoginHelper.php | 8 +-- .../Security/OpenIdLoginAuthenticatorTest.php | 15 +++-- tests/Util/CliLoginHelperTest.php | 61 +++++++++++++------ 5 files changed, 64 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2873c7..6eed748 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 forwarded to the underlying Guzzle HTTP client used by league/oauth2-client. Closes the long-standing inability to bound HTTP requests to the IdP. +### Fixed + +- Preserve original cause via `$previous` in `CliLoginHelper` and + `OpenIdLoginAuthenticator::validateClaims` exception wraps. Previously + the message was copied but the chain to the originating PSR cache or + upstream OIDC failure was lost, making logs harder to debug. + ### Changed - Mapped LoginController failures to 404 (unknown provider) or 503 diff --git a/src/Security/OpenIdLoginAuthenticator.php b/src/Security/OpenIdLoginAuthenticator.php index 2379900..5df3488 100644 --- a/src/Security/OpenIdLoginAuthenticator.php +++ b/src/Security/OpenIdLoginAuthenticator.php @@ -72,7 +72,7 @@ protected function validateClaims(Request $request): array // Authentication successful } catch (ItkOpenIdConnectException $exception) { // Handle failed authentication - throw new ValidationException($exception->getMessage()); + throw new ValidationException($exception->getMessage(), previous: $exception); } /** @var array $claimsArray */ diff --git a/src/Util/CliLoginHelper.php b/src/Util/CliLoginHelper.php index 333dbfd..2c469c8 100644 --- a/src/Util/CliLoginHelper.php +++ b/src/Util/CliLoginHelper.php @@ -34,7 +34,7 @@ public function createToken(string $username): string try { $revCacheItem = $this->cache->getItem($encodedUsername); } catch (InvalidArgumentException $e) { - throw new CacheException($e->getMessage()); + throw new CacheException($e->getMessage(), previous: $e); } if ($revCacheItem->isHit()) { @@ -53,7 +53,7 @@ public function createToken(string $username): string try { $cacheItem = $this->cache->getItem($token); } catch (InvalidArgumentException $e) { - throw new CacheException($e->getMessage()); + throw new CacheException($e->getMessage(), previous: $e); } $cacheItem->set($encodedUsername); @@ -73,7 +73,7 @@ public function getUsername(string $token): ?string try { $usernameItem = $this->cache->getItem($token); } catch (InvalidArgumentException $e) { - throw new CacheException($e->getMessage()); + throw new CacheException($e->getMessage(), previous: $e); } if (!$usernameItem->isHit()) { @@ -91,7 +91,7 @@ public function getUsername(string $token): ?string $this->cache->deleteItem($token); $this->cache->deleteItem($username); } catch (InvalidArgumentException $e) { - throw new CacheException($e->getMessage()); + throw new CacheException($e->getMessage(), previous: $e); } return $this->decodeKey($username); diff --git a/tests/Security/OpenIdLoginAuthenticatorTest.php b/tests/Security/OpenIdLoginAuthenticatorTest.php index 238b468..29f0a02 100644 --- a/tests/Security/OpenIdLoginAuthenticatorTest.php +++ b/tests/Security/OpenIdLoginAuthenticatorTest.php @@ -87,8 +87,9 @@ public function testValidateClaimsMissingCode(): void public function testValidateClaimsCodeDoesNotValidate(): void { + $cause = new ClaimsException('test message'); $stubProvider = $this->createStub(OpenIdConfigurationProvider::class); - $stubProvider->method('validateIdToken')->willThrowException(new ClaimsException('test message')); + $stubProvider->method('validateIdToken')->willThrowException($cause); $this->stubProviderManager->method('getProvider')->willReturn($stubProvider); $request = $this->createStub(Request::class); @@ -96,9 +97,15 @@ public function testValidateClaimsCodeDoesNotValidate(): void $this->setupStubSessionOnRequest($request); - $this->expectException(ValidationException::class); - $this->expectExceptionMessage('test message'); - $this->authenticator->authenticate($request); + try { + $this->authenticator->authenticate($request); + } catch (ValidationException $thrown) { + $this->assertSame('test message', $thrown->getMessage()); + $this->assertSame($cause, $thrown->getPrevious(), 'Original cause must be chained'); + + return; + } + $this->fail('Expected ValidationException'); } public function testValidateClaimsSuccess(): void diff --git a/tests/Util/CliLoginHelperTest.php b/tests/Util/CliLoginHelperTest.php index 8c05943..919b50c 100644 --- a/tests/Util/CliLoginHelperTest.php +++ b/tests/Util/CliLoginHelperTest.php @@ -83,16 +83,21 @@ public function testCreateTokenAndGetUsername(): void public function testCreateTokenThrowsCacheExceptionOnGetItem(): void { + $cause = new TestInvalidArgumentException('Cache error'); $stubCache = $this->createStub(CacheItemPoolInterface::class); - $stubCache->method('getItem') - ->willThrowException(new TestInvalidArgumentException('Cache error')); + $stubCache->method('getItem')->willThrowException($cause); $cliHelper = new CliLoginHelper($stubCache); - $this->expectException(CacheException::class); - $this->expectExceptionMessage('Cache error'); + try { + $cliHelper->createToken('test_user'); + } catch (CacheException $thrown) { + $this->assertSame('Cache error', $thrown->getMessage()); + $this->assertSame($cause, $thrown->getPrevious(), 'Original cause must be chained'); - $cliHelper->createToken('test_user'); + return; + } + $this->fail('Expected CacheException'); } public function testCreateTokenThrowsCacheExceptionOnSecondGetItem(): void @@ -101,38 +106,49 @@ public function testCreateTokenThrowsCacheExceptionOnSecondGetItem(): void $stubCacheItem->method('isHit')->willReturn(false); $stubCacheItem->method('get')->willReturn(null); + $cause = new TestInvalidArgumentException('Second cache error'); $stubCache = $this->createStub(CacheItemPoolInterface::class); $callCount = 0; $stubCache->method('getItem') - ->willReturnCallback(function () use ($stubCacheItem, &$callCount) { + ->willReturnCallback(function () use ($stubCacheItem, $cause, &$callCount) { ++$callCount; if (1 === $callCount) { return $stubCacheItem; } - throw new TestInvalidArgumentException('Second cache error'); + throw $cause; }); $stubCache->method('save')->willReturn(true); $cliHelper = new CliLoginHelper($stubCache); - $this->expectException(CacheException::class); - $this->expectExceptionMessage('Second cache error'); + try { + $cliHelper->createToken('another_user'); + } catch (CacheException $thrown) { + $this->assertSame('Second cache error', $thrown->getMessage()); + $this->assertSame($cause, $thrown->getPrevious(), 'Original cause must be chained'); - $cliHelper->createToken('another_user'); + return; + } + $this->fail('Expected CacheException'); } public function testGetUsernameThrowsCacheExceptionOnGetItem(): void { + $cause = new TestInvalidArgumentException('Cache error'); $stubCache = $this->createStub(CacheItemPoolInterface::class); - $stubCache->method('getItem') - ->willThrowException(new TestInvalidArgumentException('Cache error')); + $stubCache->method('getItem')->willThrowException($cause); $cliHelper = new CliLoginHelper($stubCache); - $this->expectException(CacheException::class); - $this->expectExceptionMessage('Cache error'); + try { + $cliHelper->getUsername('some-token'); + } catch (CacheException $thrown) { + $this->assertSame('Cache error', $thrown->getMessage()); + $this->assertSame($cause, $thrown->getPrevious(), 'Original cause must be chained'); - $cliHelper->getUsername('some-token'); + return; + } + $this->fail('Expected CacheException'); } public function testCreateTokenThrowsCacheExceptionOnNonStringCachedToken(): void @@ -175,16 +191,21 @@ public function testGetUsernameThrowsCacheExceptionOnDeleteItem(): void $stubCacheItem->method('isHit')->willReturn(true); $stubCacheItem->method('get')->willReturn('encoded_username'); + $cause = new TestInvalidArgumentException('Delete error'); $stubCache = $this->createStub(CacheItemPoolInterface::class); $stubCache->method('getItem')->willReturn($stubCacheItem); - $stubCache->method('deleteItem') - ->willThrowException(new TestInvalidArgumentException('Delete error')); + $stubCache->method('deleteItem')->willThrowException($cause); $cliHelper = new CliLoginHelper($stubCache); - $this->expectException(CacheException::class); - $this->expectExceptionMessage('Delete error'); + try { + $cliHelper->getUsername('some-token'); + } catch (CacheException $thrown) { + $this->assertSame('Delete error', $thrown->getMessage()); + $this->assertSame($cause, $thrown->getPrevious(), 'Original cause must be chained'); - $cliHelper->getUsername('some-token'); + return; + } + $this->fail('Expected CacheException'); } }