diff --git a/.gitignore b/.gitignore index ae0dcca..15e3791 100644 --- a/.gitignore +++ b/.gitignore @@ -19,3 +19,6 @@ phpcs-report.xml .php-cs-fixer.cache coverage/ yarn.lock + +# Per-developer Claude Code context — local tooling, not part of the public source. +/CLAUDE.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 557a083..470efa0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,66 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed (BREAKING) + +- **Exception hierarchy reworked.** Every exception thrown from a public + method now implements + `\ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface` (new + marker interface, extends `\Throwable`). Concrete exception classes now + extend the SPL type that best describes the failure category: + `\RuntimeException` (network/cache/data — `CacheException`, + `HttpException`, `JsonException`, `DecodeException`, `KeyException`, + `CodeException`, `ValidationException`, `ClaimsException`), + `\LogicException` (programmer/config bug — `BadUrlException`, + `IllegalSchemeException`, `MissingParameterException`), + `\InvalidArgumentException` (invalid input — `ConfigurationException`, + `NegativeCacheDurationException`, `NegativeLeewayException`). + Consumers catching `ItkOpenIdConnectException` should migrate to + `OpenIdConnectExceptionInterface`; the abstract class is kept as a + `@deprecated` alias and still implements the marker, but **concrete + exceptions no longer extend it**, so existing `catch + (ItkOpenIdConnectException $e)` blocks will not match anything thrown + by 5.0+ code. +- `OpenIdConfigurationProvider::__construct` now throws + `ConfigurationException` (new, `\InvalidArgumentException`-typed) + instead of a raw `\InvalidArgumentException` when a required option + is missing. The new type implements the marker; existing + `catch (\InvalidArgumentException $e)` blocks continue to match. +- `OpenIdConfigurationProvider::getIdToken` narrowed its boundary + `catch` from `\Exception` to + `IdentityProviderException|ClientExceptionInterface|\JsonException`. + Cache failures during `getConfiguration` (called for the token + endpoint lookup) now propagate as `CacheException` rather than being + re-wrapped as `CodeException`. Both implement the marker, so a + consumer catching that is unaffected; a consumer catching only + `CodeException` will need to widen to the marker for this code path. + +### Added + +- `ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface` + marker for catching every OIDC failure from this library. +- `ItkDev\OpenIdConnect\Exception\ConfigurationException` for missing + or invalid constructor options. +- `tests/Exception/ExceptionHierarchyTest.php` locks the contract: + every concrete implements the marker, extends the correct SPL parent, + and is caught by a `catch (OpenIdConnectExceptionInterface $e)` + block. Failing this test class is the early warning that the public + contract has drifted. + +### Deprecated + +- `ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException` abstract + class (catch `OpenIdConnectExceptionInterface` instead). Kept through + 5.x; removal scheduled for 6.0. + +### Documentation + +- Added a new "Exception handling" section to `README.md` describing the + marker interface, the SPL parents of each concrete, the PSR-18 + co-implementation on `HttpException`, and the 4.x → 5.0 catch-block + migration. Also fixed the `validateIdToken` example to catch the + marker interface instead of the now-deprecated abstract. + ## [4.1.2] - 2026-05-11 - Chained `previous` consistently in `OpenIdConfigurationProvider` catch diff --git a/README.md b/README.md index 852c83d..bc9eb0f 100644 --- a/README.md +++ b/README.md @@ -185,17 +185,58 @@ if (!$sessionState || $request->query->get('state') !== $sessionState) { // Validate the id token. This will validate the token against the keys published by the // provider (Azure AD B2C). If the token is invalid or the nonce doesn't match an -// exception will thrown. +// exception will be thrown. try { $claims = $provider->validateIdToken($request->query->get('id_token'), $session->get('oauth2nonce')); // Authentication successful -} catch (ItkOpenIdConnectException $exception) { +} catch (OpenIdConnectExceptionInterface $exception) { // Handle failed authentication } finally { $this->session->remove('oauth2nonce'); } ``` +### Exception handling + +Every exception thrown from a public method of this library implements +`\ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface`. Catch the +marker to handle any OIDC failure with a single block, or scope to a more +specific type when you need to discriminate: + +```php +use ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface; + +try { + $claims = $provider->validateIdToken($idToken, $nonce); +} catch (OpenIdConnectExceptionInterface $e) { + // Cause is preserved via $e->getPrevious() +} +``` + +Concrete exception classes extend the SPL type that describes the failure +category, so a `catch` block scoped to that SPL type will also match: + +| SPL parent | Concrete types | Category | +| ---------- | -------------- | -------- | +| `\RuntimeException` | `CacheException`, `HttpException`, `JsonException`, `DecodeException`, `KeyException`, `CodeException`, `ValidationException`, `ClaimsException` | Network, cache, token validation, claims mismatch — transient or data-shape failures | +| `\LogicException` | `BadUrlException`, `IllegalSchemeException`, `MissingParameterException` | Programmer/config bugs — should be fixed in code | +| `\InvalidArgumentException` | `ConfigurationException`, `NegativeCacheDurationException`, `NegativeLeewayException` | Invalid input to the constructor / setters | + +`HttpException` additionally implements PSR-18's +`Psr\Http\Client\ClientExceptionInterface`, so existing PSR-18-aware +consumers can keep catching on the standard PSR marker. + +Every wrap site preserves the underlying cause via `$previous`, so +`$e->getPrevious()` walks back to the originating Guzzle, firebase/php-jwt +or PSR-6 cache exception. + +> **Upgrading from 4.x:** the concrete exceptions no longer extend the +> abstract `ItkOpenIdConnectException`. Catches written as +> `catch (ItkOpenIdConnectException $e)` will not match anything thrown +> by 5.0+ code — migrate to `catch (OpenIdConnectExceptionInterface $e)`. +> The abstract class itself is kept through 5.x as a documented alias +> (`@deprecated`); removal is scheduled for 6.0. + ## Development Setup A `docker-compose.yml` file with a PHP 8.3+ image is included in this project. diff --git a/composer.json b/composer.json index 196637c..1bf1ecb 100644 --- a/composer.json +++ b/composer.json @@ -31,7 +31,7 @@ "ergebnis/composer-normalize": "^2.50", "friendsofphp/php-cs-fixer": "^3.75", "mockery/mockery": "^1.6.12", - "phpstan/phpstan": "^2.1.40", + "phpstan/phpstan": "^2.1.41", "phpunit/php-code-coverage": "^12", "phpunit/phpunit": "^12" }, diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon new file mode 100644 index 0000000..9d14590 --- /dev/null +++ b/phpstan-baseline.neon @@ -0,0 +1,139 @@ +parameters: + ignoreErrors: + - + message: '#^Call to function is_subclass_of\(\) with ''ItkDev\\\\OpenIdConnect\\\\Exception\\\\ItkOpenIdConnectException'' and ''ItkDev\\\\OpenIdConnect\\\\Exception\\\\OpenIdConnectExceptionInterface'' will always evaluate to true\.$#' + identifier: function.alreadyNarrowedType + count: 1 + path: tests/Exception/ExceptionHierarchyTest.php + + - + message: '#^Call to method PHPUnit\\Framework\\Assert\:\:assertTrue\(\) with true will always evaluate to true\.$#' + identifier: method.alreadyNarrowedType + count: 1 + path: tests/Exception/ExceptionHierarchyTest.php + + - + message: '#^Property Tests\\Security\\MockJWT\:\:\$leeway has no type specified\.$#' + identifier: missingType.property + count: 1 + path: tests/Security/MockJWT.php + + - + message: '#^Access to an undefined property object\:\:\$aud\.$#' + identifier: property.notFound + count: 2 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Access to an undefined property object\:\:\$nonce\.$#' + identifier: property.notFound + count: 3 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Call to an undefined method Mockery\\ExpectationInterface\|Mockery\\HigherOrderMessage\:\:andReturn\(\)\.$#' + identifier: method.notFound + count: 6 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Call to an undefined method Mockery\\ExpectationInterface\|Mockery\\HigherOrderMessage\:\:andThrow\(\)\.$#' + identifier: method.notFound + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Call to an undefined method Mockery\\ExpectationInterface\|Mockery\\HigherOrderMessage\:\:with\(\)\.$#' + identifier: method.notFound + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Call to method PHPUnit\\Framework\\Assert\:\:assertTrue\(\) with true will always evaluate to true\.$#' + identifier: method.alreadyNarrowedType + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method generateNonce\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method generateState\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method getAuthorizationUrl\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 3 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method getBaseAccessTokenUrl\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method getBaseAuthorizationUrl\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method getDefaultScopes\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method getEndSessionUrl\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 6 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method getGuarded\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method getResourceOwnerDetailsUrl\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method getState\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method validateIdToken\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 7 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Parameter \#1 \$json of function json_decode expects string, string\|false given\.$#' + identifier: argument.type + count: 3 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Parameter \#1 \$string of function parse_str expects string, string\|false\|null given\.$#' + identifier: argument.type + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Property Tests\\Security\\OpenIdConfigurationProviderTest\:\:\$provider \(ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\) is never assigned null so it can be removed from the property type\.$#' + identifier: property.unusedType + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php diff --git a/phpstan.neon b/phpstan.neon index 500b620..b0a1a59 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -1,7 +1,12 @@ +includes: + - phpstan-baseline.neon + parameters: level: 8 paths: - src + - tests + reportIgnoresWithoutComments: true ignoreErrors: - identifier: missingType.iterableValue diff --git a/src/Exception/BadUrlException.php b/src/Exception/BadUrlException.php index 909ab38..ee00e77 100644 --- a/src/Exception/BadUrlException.php +++ b/src/Exception/BadUrlException.php @@ -2,6 +2,6 @@ namespace ItkDev\OpenIdConnect\Exception; -class BadUrlException extends ItkOpenIdConnectException +class BadUrlException extends \LogicException implements OpenIdConnectExceptionInterface { } diff --git a/src/Exception/CacheException.php b/src/Exception/CacheException.php index 72a6bff..89e6b45 100644 --- a/src/Exception/CacheException.php +++ b/src/Exception/CacheException.php @@ -2,6 +2,6 @@ namespace ItkDev\OpenIdConnect\Exception; -class CacheException extends ItkOpenIdConnectException +class CacheException extends \RuntimeException implements OpenIdConnectExceptionInterface { } diff --git a/src/Exception/ClaimsException.php b/src/Exception/ClaimsException.php index ce5fadd..86ec72f 100644 --- a/src/Exception/ClaimsException.php +++ b/src/Exception/ClaimsException.php @@ -2,6 +2,6 @@ namespace ItkDev\OpenIdConnect\Exception; -class ClaimsException extends ItkOpenIdConnectException +class ClaimsException extends \RuntimeException implements OpenIdConnectExceptionInterface { } diff --git a/src/Exception/CodeException.php b/src/Exception/CodeException.php index 424c7cf..82d7186 100644 --- a/src/Exception/CodeException.php +++ b/src/Exception/CodeException.php @@ -2,6 +2,6 @@ namespace ItkDev\OpenIdConnect\Exception; -class CodeException extends ItkOpenIdConnectException +class CodeException extends \RuntimeException implements OpenIdConnectExceptionInterface { } diff --git a/src/Exception/ConfigurationException.php b/src/Exception/ConfigurationException.php new file mode 100644 index 0000000..45279cd --- /dev/null +++ b/src/Exception/ConfigurationException.php @@ -0,0 +1,13 @@ +setCacheItemPool($options['cacheItemPool']); @@ -84,7 +85,7 @@ public function __construct(array $options = [], array $collaborators = []) } if (!array_key_exists('openIDConnectMetadataUrl', $options)) { - throw new \InvalidArgumentException('Required options not defined: openIDConnectMetadataUrl'); + throw new ConfigurationException('Required options not defined: openIDConnectMetadataUrl'); } if (empty($collaborators['jwt'])) { @@ -114,7 +115,7 @@ public function getBaseAuthorizationUrl(): string } /** - * @throws ItkOpenIdConnectException + * @throws OpenIdConnectExceptionInterface */ public function getAuthorizationUrl(array $options = []): string { @@ -246,7 +247,7 @@ public function validateIdToken(string $idToken, string $nonce): object * @return string * The ID token * - * @throws CodeException Wraps any \Exception thrown by token-endpoint HTTP, JSON parsing, or `getConfiguration()` (with `previous` chained) + * @throws OpenIdConnectExceptionInterface */ public function getIdToken(string $code): string { @@ -265,7 +266,11 @@ public function getIdToken(string $code): string $payload = json_decode((string) $response->getBody(), true, 512, JSON_THROW_ON_ERROR); return $payload['id_token']; - } catch (\Exception $e) { + } catch (IdentityProviderException|ClientExceptionInterface|\JsonException $e) { + // Narrow boundary: IdentityProviderException from league's checkResponse, + // ClientExceptionInterface from Guzzle, \JsonException from json_decode. + // Other failures (e.g. CacheException from getConfiguration) propagate + // as their own concrete OpenIdConnectExceptionInterface subtypes. throw new CodeException('Get ID token failed: '.$e->getMessage(), 0, $e); } } @@ -346,7 +351,7 @@ protected function createResourceOwner(array $response, AccessToken $token): Res * @return array * Array of keys * - * @throws ItkOpenIdConnectException + * @throws OpenIdConnectExceptionInterface */ private function getJwtVerificationKeys(): array { @@ -538,7 +543,7 @@ private function setAllowHttp(bool $allowHttp): void /** * Set the OpenID Connect Metadata Url. * - * @throws ItkOpenIdConnectException + * @throws OpenIdConnectExceptionInterface */ private function setOpenIDConnectMetadataUrl(string $url): void { diff --git a/tests/Exception/ExceptionHierarchyTest.php b/tests/Exception/ExceptionHierarchyTest.php new file mode 100644 index 0000000..06fdd92 --- /dev/null +++ b/tests/Exception/ExceptionHierarchyTest.php @@ -0,0 +1,131 @@ +, class-string<\Throwable>}> + */ + public static function concreteProvider(): iterable + { + // Programmer / config errors → \LogicException + yield 'BadUrlException' => [BadUrlException::class, \LogicException::class]; + yield 'IllegalSchemeException' => [IllegalSchemeException::class, \LogicException::class]; + yield 'MissingParameterException' => [MissingParameterException::class, \LogicException::class]; + + // Invalid input to a public method / constructor → \InvalidArgumentException + yield 'ConfigurationException' => [ConfigurationException::class, \InvalidArgumentException::class]; + yield 'NegativeCacheDurationException' => [NegativeCacheDurationException::class, \InvalidArgumentException::class]; + yield 'NegativeLeewayException' => [NegativeLeewayException::class, \InvalidArgumentException::class]; + + // Runtime conditions → \RuntimeException + yield 'CacheException' => [CacheException::class, \RuntimeException::class]; + yield 'HttpException' => [HttpException::class, \RuntimeException::class]; + yield 'JsonException' => [JsonException::class, \RuntimeException::class]; + yield 'DecodeException' => [DecodeException::class, \RuntimeException::class]; + yield 'KeyException' => [KeyException::class, \RuntimeException::class]; + yield 'CodeException' => [CodeException::class, \RuntimeException::class]; + yield 'ValidationException' => [ValidationException::class, \RuntimeException::class]; + yield 'ClaimsException' => [ClaimsException::class, \RuntimeException::class]; + } + + /** + * @param class-string<\Throwable> $concrete + * @param class-string<\Throwable> $expectedSplParent + */ + #[DataProvider('concreteProvider')] + public function testConcreteImplementsMarker(string $concrete, string $expectedSplParent): void + { + $instance = new $concrete('test'); + $this->assertInstanceOf(OpenIdConnectExceptionInterface::class, $instance); + $this->assertInstanceOf(\Throwable::class, $instance); + } + + /** + * @param class-string<\Throwable> $concrete + * @param class-string<\Throwable> $expectedSplParent + */ + #[DataProvider('concreteProvider')] + public function testConcreteExtendsExpectedSplParent(string $concrete, string $expectedSplParent): void + { + $instance = new $concrete('test'); + $this->assertInstanceOf($expectedSplParent, $instance); + } + + /** + * @param class-string<\Throwable> $concrete + * @param class-string<\Throwable> $expectedSplParent + */ + #[DataProvider('concreteProvider')] + public function testCatchByMarkerMatchesEveryConcrete(string $concrete, string $expectedSplParent): void + { + try { + throw new $concrete('test'); + } catch (OpenIdConnectExceptionInterface $caught) { + $this->assertInstanceOf($concrete, $caught); + + return; + } + // @phpstan-ignore deadCode.unreachable (safety net if a future regression breaks the catch-by-marker contract) + $this->fail('Catch on OpenIdConnectExceptionInterface should have matched '.$concrete); + } + + public function testHttpExceptionAlsoImplementsPsr18ClientInterface(): void + { + // HttpException is part of the public contract for two markers — the + // OIDC marker and PSR-18's ClientExceptionInterface — so a PSR-18-savvy + // consumer can catch HTTP failures via the standard PSR interface. + $instance = new HttpException('test'); + $this->assertInstanceOf(ClientExceptionInterface::class, $instance); + } + + public function testAbstractBaseImplementsMarker(): void + { + // The deprecated abstract `ItkOpenIdConnectException` is kept for + // backward compatibility through 5.x and still implements the marker. + // Catch sites that wrote `catch (ItkOpenIdConnectException $e)` should + // migrate to the marker interface; this assertion guards the marker + // implementation while the deprecation window is open. + $this->assertTrue( + is_subclass_of( + \ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException::class, + OpenIdConnectExceptionInterface::class, + ), + ); + } +} diff --git a/tests/Security/OpenIdConfigurationProviderTest.php b/tests/Security/OpenIdConfigurationProviderTest.php index 2aaa02d..c3ef46b 100644 --- a/tests/Security/OpenIdConfigurationProviderTest.php +++ b/tests/Security/OpenIdConfigurationProviderTest.php @@ -12,8 +12,8 @@ use ItkDev\OpenIdConnect\Exception\CodeException; use ItkDev\OpenIdConnect\Exception\HttpException; use ItkDev\OpenIdConnect\Exception\IllegalSchemeException; -use ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException; use ItkDev\OpenIdConnect\Exception\KeyException; +use ItkDev\OpenIdConnect\Exception\MissingParameterException; use ItkDev\OpenIdConnect\Exception\NegativeCacheDurationException; use ItkDev\OpenIdConnect\Exception\NegativeLeewayException; use ItkDev\OpenIdConnect\Exception\ValidationException; @@ -179,7 +179,7 @@ public function testGetAuthorizationUrl(): void public function testGetAuthorizationUrlStateException(): void { - $this->expectException(ItkOpenIdConnectException::class); + $this->expectException(MissingParameterException::class); $this->expectExceptionMessage('Required parameter "state" missing'); $authUrl = $this->provider->getAuthorizationUrl(['nonce' => 'abcd']); @@ -187,7 +187,7 @@ public function testGetAuthorizationUrlStateException(): void public function testGetAuthorizationUrlNonceException(): void { - $this->expectException(ItkOpenIdConnectException::class); + $this->expectException(MissingParameterException::class); $this->expectExceptionMessage('Required parameter "nonce" missing'); $authUrl = $this->provider->getAuthorizationUrl(['state' => 'abcd']); @@ -512,7 +512,11 @@ public function testGetIdTokenFailure(): void $openIDConnectMetadataUrl = 'https://some.url/openid-configuration'; $mockHttpClient = $this->createStub(ClientInterface::class); - $mockHttpClient->method('request')->willThrowException(new \RuntimeException('Connection failed')); + // PSR-18 transport stub — Guzzle's real exceptions need a RequestInterface + // we don't have here, and any ClientExceptionInterface satisfies getIdToken's catch. + $mockHttpClient->method('request')->willThrowException( + new class('Connection failed') extends \RuntimeException implements ClientExceptionInterface {} + ); $mockCacheItem = $this->createStub(CacheItemInterface::class); $mockCacheItem->method('isHit')->willReturn(false);