diff --git a/.github/workflows/composer.yaml b/.github/workflows/composer.yaml index f807f83..e2a1411 100644 --- a/.github/workflows/composer.yaml +++ b/.github/workflows/composer.yaml @@ -77,4 +77,5 @@ jobs: docker network create frontend - run: | + docker compose run --rm phpfpm composer install docker compose run --rm phpfpm composer audit 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..6b50b6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,70 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [5.0.0] - 2026-06-02 + +Reworked exception hierarchy and tightened IdP-payload validations. The runtime +behaviour is unchanged for spec-compliant IdPs — see [UPGRADE-5.0.md](UPGRADE-5.0.md) +for the consumer migration guide. + +### Changed (BREAKING) + +- Reworked exception hierarchy around the new + `OpenIdConnectExceptionInterface` marker. Concrete exception classes now extend the + SPL type that best describes the failure category (`\RuntimeException`, + `\LogicException`, `\InvalidArgumentException`) instead of the abstract + `ItkOpenIdConnectException`. Existing `catch (ItkOpenIdConnectException $e)` blocks + will not match anything thrown by 5.0+ code — catch the marker, or scope to a more + specific concrete / SPL parent +- Renamed `KeyException` → `JwksException` for symmetry with the new + `MetadataException` and to better describe its scope (the type fires for both + JWKS-document-level and JWK-entry-level errors) +- `OpenIdConfigurationProvider::__construct` now throws the typed + `ConfigurationException` (still extending `\InvalidArgumentException`) instead of + a raw `\InvalidArgumentException` for missing required options +- New typed throws replace 4.x silent coercions: malformed JWKS payload + (missing `keys` array, non-object JWK entry, missing/non-string `kid` / + `kty` / RSA `e` / `n`, unsupported `kty`) → `JwksException`; malformed + OIDC discovery document → `MetadataException`; token endpoint response + missing string `id_token` → `CodeException` +- `OpenIdConfigurationProvider::getIdToken` narrowed its boundary `catch` from + `\Exception` to the three actually-thrown families + (`IdentityProviderException|ClientExceptionInterface|\JsonException`). + Exceptions from the upstream `getConfiguration('token_endpoint')` call + (`CacheException`, `HttpException`, `MetadataException`, library + `JsonException`) now propagate as themselves rather than being re-wrapped + as `CodeException` + +### Added + +- Marker interface `OpenIdConnectExceptionInterface` (extends `\Throwable`) +- Concrete exceptions `ConfigurationException` and `MetadataException` +- `tests/Exception/ExceptionHierarchyTest.php` locks the contract: every concrete + implements the marker, extends the correct SPL parent, and is caught by a single + `catch (OpenIdConnectExceptionInterface $e)` block + +### Deprecated + +- Abstract `ItkOpenIdConnectException` — catch `OpenIdConnectExceptionInterface` + instead. Kept through 5.x as a documented alias that still implements the marker; + removal scheduled for 6.0 + +### Documentation + +- Added an "Exception handling" section to `README.md` covering the marker + interface, SPL parents, PSR-18 co-implementation on `HttpException`, and the + 4.x → 5.0 catch-block migration +- Class-level PHPDoc on every concrete exception describing its trigger sites and + the boundary against related types + +### Tooling + +- PHPStan bumped to `level: max` (was 8). Scans `src/` + `tests/` +- `reportIgnoresWithoutComments: true` so unexplained `@phpstan-ignore` directives + fail CI +- Added `phpstan/phpstan-mockery` to `require-dev` for stubs covering Mockery's + fluent `shouldReceive(...)->andReturn(...)` API + ## [4.1.2] - 2026-05-11 - Chained `previous` consistently in `OpenIdConfigurationProvider` catch @@ -163,7 +227,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - This CHANGELOG file to hopefully serve as an evolving example of a standardized open source project CHANGELOG. -[Unreleased]: https://github.com/itk-dev/openid-connect/compare/4.1.2...HEAD +[Unreleased]: https://github.com/itk-dev/openid-connect/compare/5.0.0...HEAD +[5.0.0]: https://github.com/itk-dev/openid-connect/compare/4.1.2...5.0.0 [4.1.2]: https://github.com/itk-dev/openid-connect/compare/4.1.1...4.1.2 [4.1.1]: https://github.com/itk-dev/openid-connect/compare/4.1.0...4.1.1 [4.1.0]: https://github.com/itk-dev/openid-connect/compare/4.0.3...4.1.0 diff --git a/README.md b/README.md index 852c83d..e7f5f06 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`, `JwksException`, `CodeException`, `ValidationException`, `ClaimsException`, `MetadataException` | 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/UPGRADE-5.0.md b/UPGRADE-5.0.md new file mode 100644 index 0000000..2ced64e --- /dev/null +++ b/UPGRADE-5.0.md @@ -0,0 +1,149 @@ +# Upgrading from 4.x to 5.0 + +5.0 reworks the exception hierarchy and tightens several IdP-payload +validations. The runtime behaviour is unchanged for spec-compliant IdPs +— this document covers the consumer-visible API changes you'll need to +adjust catch blocks for. + +## Catch the marker interface, not the abstract + +Concrete exception classes no longer extend +`\ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException`. Existing +catches against the abstract will not match anything thrown by 5.0+ +code: + +```diff +- } catch (\ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException $e) { ++ } catch (\ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface $e) { +``` + +The abstract class is kept through the 5.x line as a `@deprecated` +alias that still implements the marker; removal is scheduled for 6.0. + +A consumer that needs to discriminate by failure category can scope to +the concrete's SPL parent instead — catching `\RuntimeException` +matches every transient/data failure (network, cache, token +validation, claims mismatch); catching `\LogicException` matches the +programmer-error variants (`BadUrlException`, +`IllegalSchemeException`, `MissingParameterException`); catching +`\InvalidArgumentException` matches the invalid-constructor-input +variants (`ConfigurationException`, `NegativeCacheDurationException`, +`NegativeLeewayException`). + +## Catch on `\InvalidArgumentException` still works for constructor errors + +`OpenIdConfigurationProvider::__construct` previously threw raw +`\InvalidArgumentException` for missing required options. It now +throws the typed `ConfigurationException`, which still **extends** +`\InvalidArgumentException`. Existing catches at the SPL level +continue to match without any change: + +```php +try { + new OpenIdConfigurationProvider([]); +} catch (\InvalidArgumentException $e) { // still catches in 5.0 + // ... +} +``` + +## New typed throws where 4.x silently coerced + +Three sites that previously cast malformed IdP-returned values to +strings now throw a typed exception: + +- **Malformed JWKS payload (`keys` array missing, JWK entry without + string `kid` / `kty` / `e` / `n`)** → `JwksException`. 4.x silently + built a degraded `Key` from whatever the (string) cast produced, or + tripped a downstream type error in `XMLSecurityKey::convertRSA`. +- **OIDC discovery doc with missing / non-string required key** → + `MetadataException` (was bubbled as `CacheException`, semantically + misleading — the failure is the IdP-returned payload not + conforming to the OIDC Discovery spec, not the cache layer + misbehaving). +- **Token endpoint response missing string `id_token`** → + `CodeException` (was a return of `mixed` that produced confusing + errors at the call site). + +If you've been catching `CacheException` specifically for the +missing-config-key case, you'll need to widen to the marker interface +or to `MetadataException`: + +```diff + try { + $provider->getBaseAuthorizationUrl(); +- } catch (\ItkDev\OpenIdConnect\Exception\CacheException $e) { ++ } catch (\ItkDev\OpenIdConnect\Exception\MetadataException $e) { + // Discovery document is malformed + } +``` + +`CacheException` still fires for PSR-6 cache-layer failures (its +strictly-cache-only meaning in 5.0). + +## `getIdToken`'s `@throws` no longer advertises `ClientExceptionInterface` + +The previous `@throws ClientExceptionInterface` declaration on +`getIdToken` documented a dead-code path — the body's catch-all +wrapped the transport exception into `CodeException` before it could +surface. The declaration was removed in 4.1.2 already; the 5.0 boundary +catch is now narrowed to +`IdentityProviderException|ClientExceptionInterface|\JsonException` +explicitly, but the public type returned to the caller remains +`CodeException` (with the cause chained via `$previous`). + +If you were catching `ClientExceptionInterface` after a `getIdToken` +call to handle transport failures specifically, switch to catching +`CodeException` and inspect `$e->getPrevious()` for the original +PSR-18 exception: + +```diff + try { + $idToken = $provider->getIdToken($code); +- } catch (\Psr\Http\Client\ClientExceptionInterface $e) { +- // transport failure ++ } catch (\ItkDev\OpenIdConnect\Exception\CodeException $e) { ++ $transport = $e->getPrevious(); ++ if ($transport instanceof \Psr\Http\Client\ClientExceptionInterface) { ++ // transport failure ++ } + } +``` + +(`HttpException` — fired by the *discovery* / JWKS fetches, not the +token-exchange POST — still implements `ClientExceptionInterface` as +part of its public contract.) + +## Catch-on-SPL semantic change + +The re-parenting onto SPL types means catches on `\RuntimeException`, +`\LogicException`, or `\InvalidArgumentException` now match OIDC +failures where they previously did not. Concretely: a generic retry +decorator wrapping `validateIdToken` with `catch (\RuntimeException +$e)` will now retry on `CacheException`, `HttpException`, +`ValidationException`, `ClaimsException`, etc. + +This is the intended semantic — `\RuntimeException` represents +"transient or data-shape failure," which is exactly what those +concrete types now signal. If your wrapping logic relies on the old +behaviour, narrow the catch to a more specific bundle / library +exception. + +## Per-class summary + +| Concrete | Parent | When it fires | +| --- | --- | --- | +| `BadUrlException` | `\LogicException` | `openIDConnectMetadataUrl` syntax invalid | +| `IllegalSchemeException` | `\LogicException` | http scheme without `allowHttp: true` | +| `MissingParameterException` | `\LogicException` | `getAuthorizationUrl()` missing required `state` / `nonce` | +| `ConfigurationException` | `\InvalidArgumentException` | Constructor missing `cacheItemPool` / `openIDConnectMetadataUrl` | +| `NegativeCacheDurationException` | `\InvalidArgumentException` | `cacheDuration` < 0 | +| `NegativeLeewayException` | `\InvalidArgumentException` | `leeway` < 0 | +| `CacheException` | `\RuntimeException` | PSR-6 cache layer failed | +| `HttpException` (+ `ClientExceptionInterface`) | `\RuntimeException` | HTTP transport failure for discovery / JWKS | +| `JsonException` | `\RuntimeException` | `json_decode` failed on an IdP response body | +| `DecodeException` | `\RuntimeException` | JWK base64 decode failed | +| `JwksException` | `\RuntimeException` | JWKS payload doesn't conform to RFC 7517 | +| `CodeException` | `\RuntimeException` | Token endpoint exchange failed | +| `ValidationException` | `\RuntimeException` | JWT signature / decode failed | +| `ClaimsException` | `\RuntimeException` | JWT claim values wrong (aud / iss / nonce) | +| `MetadataException` | `\RuntimeException` | OIDC discovery document doesn't conform to the spec | diff --git a/composer.json b/composer.json index 196637c..bb3321b 100644 --- a/composer.json +++ b/composer.json @@ -31,7 +31,8 @@ "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", + "phpstan/phpstan-mockery": "^2.0", "phpunit/php-code-coverage": "^12", "phpunit/phpunit": "^12" }, diff --git a/phpstan.neon b/phpstan.neon index 500b620..c39bc0d 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -1,7 +1,12 @@ +includes: + - vendor/phpstan/phpstan-mockery/extension.neon + parameters: - level: 8 + level: max paths: - src + - tests + reportIgnoresWithoutComments: true ignoreErrors: - identifier: missingType.iterableValue diff --git a/src/Exception/BadUrlException.php b/src/Exception/BadUrlException.php index 909ab38..2ec409b 100644 --- a/src/Exception/BadUrlException.php +++ b/src/Exception/BadUrlException.php @@ -2,6 +2,16 @@ namespace ItkDev\OpenIdConnect\Exception; -class BadUrlException extends ItkOpenIdConnectException +/** + * Thrown when `openIDConnectMetadataUrl` fails URL syntax validation + * (`parse_url` rejects it because no scheme can be parsed). A programmer + * error — the value is hard-coded or comes from misread configuration, so + * fixing it requires editing code or env config, not retrying at runtime. + * Hence `\LogicException`. + * + * Distinct from {@see IllegalSchemeException} (URL parses successfully but + * uses an `http://` scheme without `allowHttp: true`). + */ +class BadUrlException extends \LogicException implements OpenIdConnectExceptionInterface { } diff --git a/src/Exception/CacheException.php b/src/Exception/CacheException.php index 72a6bff..f4d2cbd 100644 --- a/src/Exception/CacheException.php +++ b/src/Exception/CacheException.php @@ -2,6 +2,19 @@ namespace ItkDev\OpenIdConnect\Exception; -class CacheException extends ItkOpenIdConnectException +/** + * Wraps PSR-6 cache layer failures. Specifically thrown when the injected + * `Psr\Cache\CacheItemPoolInterface` raises `Psr\Cache\InvalidArgumentException` + * from `getItem` / `save` / `deleteItem` — typically because the cache key + * contains a character the backend rejects, or the backend itself is + * unhealthy. The original exception is chained via `$previous`. Hence + * `\RuntimeException` (transient — a different cache backend or a sanitized + * key may resolve it). + * + * Strictly cache-layer failures only. Discovery-document validation problems + * are {@see MetadataException}; JWKS validation problems are + * {@see JwksException}; JSON parse failures are {@see JsonException}. + */ +class CacheException extends \RuntimeException implements OpenIdConnectExceptionInterface { } diff --git a/src/Exception/ClaimsException.php b/src/Exception/ClaimsException.php index ce5fadd..612259d 100644 --- a/src/Exception/ClaimsException.php +++ b/src/Exception/ClaimsException.php @@ -2,6 +2,19 @@ namespace ItkDev\OpenIdConnect\Exception; -class ClaimsException extends ItkOpenIdConnectException +/** + * Thrown from `validateIdToken()` when the decoded ID token's claims + * don't match expectations — wrong `aud` (audience does not contain + * our client id), wrong `iss` (issuer doesn't match the discovery + * document), or wrong `nonce` (didn't match the value we sent on the + * authorization request). Hence `\RuntimeException` (typically requires + * either re-authenticating, or auditing why the IdP issued a token + * meant for someone else — security-relevant if persistent). + * + * Distinct from {@see ValidationException} (token cryptographically + * invalid — bad signature, expired) and from {@see CodeException} + * (failure obtaining the token in the first place, before decoding). + */ +class ClaimsException extends \RuntimeException implements OpenIdConnectExceptionInterface { } diff --git a/src/Exception/CodeException.php b/src/Exception/CodeException.php index 424c7cf..d95693c 100644 --- a/src/Exception/CodeException.php +++ b/src/Exception/CodeException.php @@ -2,6 +2,22 @@ namespace ItkDev\OpenIdConnect\Exception; -class CodeException extends ItkOpenIdConnectException +/** + * Thrown by `getIdToken()` when the OAuth authorization-code exchange + * fails: the token endpoint returned a transport error + * (`Psr\Http\Client\ClientExceptionInterface`), an OAuth error response + * (`League\OAuth2\Client\Provider\Exception\IdentityProviderException`), + * non-JSON body (`\JsonException`), or a JSON body missing a string + * `id_token`. The originating exception is chained via `$previous`. + * Hence `\RuntimeException` (often transient — typical causes are an + * expired or already-used authorization code, or a brief IdP outage). + * + * Distinct from {@see HttpException} (general HTTP transport failures + * for the discovery / JWKS endpoints, not the token-exchange POST). And + * distinct from {@see ValidationException} / {@see ClaimsException}, + * which fire later in the flow on a successfully-received but invalid + * token. + */ +class CodeException extends \RuntimeException implements OpenIdConnectExceptionInterface { } diff --git a/src/Exception/ConfigurationException.php b/src/Exception/ConfigurationException.php new file mode 100644 index 0000000..50009ab --- /dev/null +++ b/src/Exception/ConfigurationException.php @@ -0,0 +1,17 @@ +setCacheItemPool($options['cacheItemPool']); @@ -79,12 +103,12 @@ public function __construct(array $options = [], array $collaborators = []) $this->setCacheDuration($options['cacheDuration']); } - if (array_key_exists('leeway', $options) && is_int($options['leeway'])) { + if (array_key_exists('leeway', $options)) { $this->setLeeway($options['leeway']); } 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'])) { @@ -107,6 +131,7 @@ public function getGuarded(): array * @throws CacheException * @throws HttpException * @throws JsonException + * @throws MetadataException */ public function getBaseAuthorizationUrl(): string { @@ -114,7 +139,7 @@ public function getBaseAuthorizationUrl(): string } /** - * @throws ItkOpenIdConnectException + * @throws OpenIdConnectExceptionInterface */ public function getAuthorizationUrl(array $options = []): string { @@ -144,19 +169,16 @@ public function getAuthorizationUrl(array $options = []): string * @see https://docs.microsoft.com/en-us/azure/active-directory-b2c/openid-connect#send-a-sign-out-request * @see https://openid.net/specs/openid-connect-rpinitiated-1_0.html#RPLogout * - * @param string|null $postLogoutRedirectUri - * The URL that the user should be redirected to after successful sign out - * @param string|null $state - * If a state parameter is included in the request, the same value should appear in the response. The application should verify that the state values in the request and response are identical. - * @param string|null $idToken - * The id token + * @param string|null $postLogoutRedirectUri The URL that the user should be redirected to after successful sign out + * @param string|null $state If a state parameter is included in the request, the same value should appear in the response. The application should verify that the state values in the request and response are identical. + * @param string|null $idToken The id token * - * @return string - * The Url to redirect the client to for session logout + * @return string The Url to redirect the client to for session logout * * @throws CacheException * @throws HttpException * @throws JsonException + * @throws MetadataException */ public function getEndSessionUrl(?string $postLogoutRedirectUri = null, ?string $state = null, ?string $idToken = null): string { @@ -194,20 +216,18 @@ public function getEndSessionUrl(?string $postLogoutRedirectUri = null, ?string * processes), the leeway value set by the last provider to call validateIdToken() * will apply globally until overwritten. * - * @param string $idToken - * Raw id token - * @param string $nonce - * Nonce + * @param string $idToken Raw id token + * @param string $nonce Nonce * - * @return object - * The JWT's payload as a PHP object + * @return object The JWT's payload as a PHP object * * @throws CacheException * @throws ClaimsException * @throws DecodeException * @throws HttpException * @throws JsonException - * @throws KeyException + * @throws JwksException + * @throws MetadataException * @throws ValidationException */ public function validateIdToken(string $idToken, string $nonce): object @@ -217,6 +237,7 @@ public function validateIdToken(string $idToken, string $nonce): object // NB: JWT::$leeway is a static property shared across all instances. // Always set it immediately before decode to ensure the correct value. JWT::$leeway = $this->leeway; + /** @var \stdClass&object{aud: string|array, iss: string, nonce: string} $claims */ $claims = JWT::decode($idToken, $keys); // "aud" may be an array of strings or a single string // (cf. https://openid.net/specs/openid-connect-core-1_0.html#IDToken). @@ -240,13 +261,11 @@ public function validateIdToken(string $idToken, string $nonce): object /** * Get id token from code. * - * @param string $code - * The code + * @param string $code The code * - * @return string - * The ID token + * @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 { @@ -264,8 +283,16 @@ public function getIdToken(string $code): string $payload = json_decode((string) $response->getBody(), true, 512, JSON_THROW_ON_ERROR); + if (!is_array($payload) || !is_string($payload['id_token'] ?? null)) { + throw new CodeException('Token endpoint response missing string "id_token"'); + } + 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); } } @@ -274,11 +301,9 @@ public function getIdToken(string $code): string * Generates a new random string to use as the state parameter in an * authorization flow. * - * @param int $length - * Length of the random string to be generated + * @param int $length Length of the random string to be generated * - * @return string - * The generated state + * @return string The generated state */ public function generateState(int $length = 32): string { @@ -291,11 +316,9 @@ public function generateState(int $length = 32): string * Generates a new random string to use as the nonce parameter in an * authorization flow. * - * @param int $length - * Length of the random string to be generated + * @param int $length Length of the random string to be generated * - * @return string - * The generated nonce + * @return string The generated nonce */ public function generateNonce(int $length = 32): string { @@ -343,10 +366,9 @@ protected function createResourceOwner(array $response, AccessToken $token): Res /** * Get JWT verification keys from Azure Active Directory. * - * @return array - * Array of keys + * @return array Array of keys indexed by JWK `kid` * - * @throws ItkOpenIdConnectException + * @throws OpenIdConnectExceptionInterface */ private function getJwtVerificationKeys(): array { @@ -359,20 +381,37 @@ private function getJwtVerificationKeys(): array $item = $this->cacheItemPool->getItem($cacheKey); if ($item->isHit()) { + /** @var array $keys (we only ever store this shape) */ $keys = (array) $item->get(); } else { $keysUri = $this->getConfiguration('jwks_uri'); $jwks = $this->fetchJsonResource($keysUri); + if (!isset($jwks['keys']) || !is_array($jwks['keys'])) { + throw new JwksException('JWKS payload missing array "keys" property (RFC 7517 §5)'); + } + foreach ($jwks['keys'] as $key) { - $kid = (string) $key['kid']; + if (!is_array($key)) { + throw new JwksException('JWK entry is not a JSON object'); + } + if (!is_string($key['kid'] ?? null)) { + throw new JwksException('JWK entry missing string "kid" (RFC 7517 §4.5)'); + } + $kid = $key['kid']; + if (!is_string($key['kty'] ?? null)) { + throw new JwksException('JWK entry missing string "kty" for key id: '.$kid); + } if ('RSA' === $key['kty']) { + if (!is_string($key['e'] ?? null) || !is_string($key['n'] ?? null)) { + throw new JwksException('JWK RSA entry missing string "e"/"n" for key id: '.$kid); + } $e = self::base64urlDecode($key['e']); $n = self::base64urlDecode($key['n']); $publicKey = XMLSecurityKey::convertRSA($n, $e); $keys[$kid] = new Key($publicKey, 'RS256'); } else { - throw new KeyException('Unsupported key data for key id: '.$kid); + throw new JwksException('Unsupported key data for key id: '.$kid); } } @@ -406,8 +445,7 @@ private static function base64urlDecode(string $input): string /** * Fetch remote json resource. * - * @return array - * Json decoded to array + * @return array Json decoded to array * * @throws HttpException * @throws JsonException @@ -437,15 +475,14 @@ private function fetchJsonResource(string $resourceUrl): array /** * Get Configuration option for key. * - * @param string $key - * The configuration key + * @param string $key The configuration key * - * @return string - * The configuration value for the given key + * @return string The configuration value for the given key * * @throws CacheException * @throws HttpException * @throws JsonException + * @throws MetadataException */ private function getConfiguration(string $key): string { @@ -463,13 +500,14 @@ private function getConfiguration(string $key): string $this->cacheItemPool->save($item); } - if (isset($configuration[$key])) { - $value = (string) $configuration[$key]; - } else { - throw new CacheException('Required config key not defined: '.$key); + if (!isset($configuration[$key])) { + throw new MetadataException('OIDC discovery document missing required key: '.$key); + } + if (!is_string($configuration[$key])) { + throw new MetadataException(sprintf('OIDC discovery document value for "%s" is not a string (got %s)', $key, get_debug_type($configuration[$key]))); } - return $value; + return $configuration[$key]; } catch (InvalidArgumentException $e) { throw new CacheException($e->getMessage(), 0, $e); } @@ -495,8 +533,7 @@ private function setCacheItemPool(CacheItemPoolInterface $cacheItemPool): void /** * Set the provider cache duration. * - * @param int $cacheDuration - * The cache duration in seconds + * @param int $cacheDuration The cache duration in seconds * * @throws NegativeCacheDurationException */ @@ -511,8 +548,7 @@ private function setCacheDuration(int $cacheDuration): void /** * Set the leeway to allow for clock skew between hosting server and provider. * - * @param int $leeway - * The leeway in seconds. Must be positive + * @param int $leeway The leeway in seconds. Must be positive * * @throws NegativeLeewayException */ @@ -527,8 +563,7 @@ private function setLeeway(int $leeway): void /** * Set allow HTTP. * - * @param bool $allowHttp - * Whether to allow HTTP + * @param bool $allowHttp Whether to allow HTTP */ private function setAllowHttp(bool $allowHttp): void { @@ -538,7 +573,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..501858b --- /dev/null +++ b/tests/Exception/ExceptionHierarchyTest.php @@ -0,0 +1,137 @@ +, 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 'JwksException' => [JwksException::class, \RuntimeException::class]; + yield 'CodeException' => [CodeException::class, \RuntimeException::class]; + yield 'ValidationException' => [ValidationException::class, \RuntimeException::class]; + yield 'ClaimsException' => [ClaimsException::class, \RuntimeException::class]; + yield 'MetadataException' => [MetadataException::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. + // + // ReflectionClass keeps the check at runtime so PHPStan can't fold it + // into a constant tautology — the value of the test is catching a + // *future* regression that removes the marker from the abstract. + $reflection = new \ReflectionClass(\ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException::class); + $this->assertContains( + OpenIdConnectExceptionInterface::class, + $reflection->getInterfaceNames(), + 'Deprecated abstract must still implement the marker for 5.x BC.', + ); + } +} diff --git a/tests/Security/MockJWT.php b/tests/Security/MockJWT.php index 95a8cde..cdd40e7 100644 --- a/tests/Security/MockJWT.php +++ b/tests/Security/MockJWT.php @@ -4,5 +4,5 @@ class MockJWT { - public static $leeway; + public static ?int $leeway = null; } diff --git a/tests/Security/OpenIdConfigurationProviderTest.php b/tests/Security/OpenIdConfigurationProviderTest.php index 2aaa02d..9bffc5d 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\JwksException; +use ItkDev\OpenIdConnect\Exception\MissingParameterException; use ItkDev\OpenIdConnect\Exception\NegativeCacheDurationException; use ItkDev\OpenIdConnect\Exception\NegativeLeewayException; use ItkDev\OpenIdConnect\Exception\ValidationException; @@ -37,7 +37,7 @@ class OpenIdConfigurationProviderTest extends TestCase private const REDIRECT_URI = 'https://redirect.url'; private const NONCE = '12345678'; - private ?OpenIdConfigurationProvider $provider; + private OpenIdConfigurationProvider $provider; public function setUp(): void { @@ -168,7 +168,9 @@ public function testGetAuthorizationUrl(): void $authUrl = $this->provider->getAuthorizationUrl(['state' => $state, 'nonce' => $nonce]); $query = []; - parse_str(parse_url($authUrl, PHP_URL_QUERY), $query); + $queryString = parse_url($authUrl, PHP_URL_QUERY); + $this->assertIsString($queryString, 'Generated authorization URL must have a query string'); + parse_str($queryString, $query); $this->assertSame('openid', $query['scope']); $this->assertSame('id_token', $query['response_type']); @@ -179,7 +181,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 +189,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']); @@ -224,6 +226,7 @@ public function testGetBaseAccessTokenUrl(): void public function testValidateIdTokenSuccess(): void { + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); $mockClaims = $this->getMockClaims(); @@ -238,6 +241,7 @@ public function testValidateIdTokenSuccess(): void ) )->andReturn($mockClaims); + /** @var object{nonce: string, aud: string|list} $claims */ $claims = $this->provider->validateIdToken('token', self::NONCE); $this->assertEquals(self::NONCE, $claims->nonce); @@ -246,6 +250,7 @@ public function testValidateIdTokenSuccess(): void public function testValidateIdTokenFailure(): void { + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); $mockJWT->shouldReceive('decode')->andThrow(SignatureInvalidException::class, 'Signature verification failed'); @@ -257,6 +262,7 @@ public function testValidateIdTokenFailure(): void public function testValidateIdTokenAudience(): void { + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); $mockClaims = $this->getMockClaims(); $mockClaims->aud = 'incorrect aud'; @@ -271,6 +277,7 @@ public function testValidateIdTokenAudience(): void public function testValidateIdTokenIssuer(): void { + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); $mockClaims = $this->getMockClaims(); $mockClaims->iss = 'incorrect iss'; @@ -285,6 +292,7 @@ public function testValidateIdTokenIssuer(): void public function testValidateIdTokenNonce(): void { + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); $mockClaims = $this->getMockClaims(); $mockClaims->nonce = 'incorrect nonce'; @@ -370,6 +378,8 @@ public function testGetResourceOwnerDetailsUrl(): void public function testCheckResponseSuccess(): void { + $this->expectNotToPerformAssertions(); + $response = $this->createStub(ResponseInterface::class); $response->method('getStatusCode')->willReturn(200); @@ -377,7 +387,6 @@ public function testCheckResponseSuccess(): void // Should not throw $method->invoke($this->provider, $response, ['data' => 'value']); - $this->assertTrue(true); } public function testCheckResponseWithErrorString(): void @@ -425,17 +434,20 @@ public function testCreateResourceOwner(): void $method = new \ReflectionMethod(OpenIdConfigurationProvider::class, 'createResourceOwner'); $owner = $method->invoke($this->provider, ['id' => '123', 'name' => 'Test'], $token); + $this->assertInstanceOf(\League\OAuth2\Client\Provider\ResourceOwnerInterface::class, $owner); $this->assertSame('123', $owner->getId()); } public function testValidateIdTokenArrayAudience(): void { + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); $mockClaims = $this->getMockClaims(); $mockClaims->aud = [self::CLIENT_ID, 'other_client']; $mockJWT->shouldReceive('decode')->andReturn($mockClaims); + /** @var object{nonce: string, aud: string|list} $claims */ $claims = $this->provider->validateIdToken('token', self::NONCE); $this->assertEquals(self::NONCE, $claims->nonce); @@ -444,6 +456,7 @@ public function testValidateIdTokenArrayAudience(): void public function testValidateIdTokenArrayAudienceInvalid(): void { + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); $mockClaims = $this->getMockClaims(); $mockClaims->aud = ['wrong_client_1', 'wrong_client_2']; @@ -512,7 +525,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); @@ -536,12 +553,61 @@ public function testGetIdTokenFailure(): void $provider->getIdToken('test-code'); } + public function testGetIdTokenRejectsResponseWithoutStringIdToken(): void + { + $tokenEndpoint = 'https://azure_b2c_test.b2clogin.com/azure_b2c_test.onmicrosoft.com/oauth2/v2.0/token?p=test-policy'; + $openIDConnectMetadataUrl = 'https://some.url/openid-configuration'; + + $mockConfigResponse = $this->getMockHttpSuccessResponse('/../MockData/mockOpenIDConfiguration.json'); + + // Spec-compliant token endpoint returns JSON with `id_token`. + // Here the IdP returns a JSON object that's missing it entirely. + $malformedTokenResponseBody = (string) json_encode(['access_token' => 'not-an-id-token']); + $mockTokenStream = $this->createStub(StreamInterface::class); + $mockTokenStream->method('getContents')->willReturn($malformedTokenResponseBody); + $mockTokenStream->method('__toString')->willReturn($malformedTokenResponseBody); + + $mockTokenResponse = $this->createStub(ResponseInterface::class); + $mockTokenResponse->method('getStatusCode')->willReturn(200); + $mockTokenResponse->method('getBody')->willReturn($mockTokenStream); + + $mockHttpClient = $this->createStub(ClientInterface::class); + $mockHttpClient->method('request')->willReturnMap([ + ['GET', $openIDConnectMetadataUrl, [], $mockConfigResponse], + ['POST', $tokenEndpoint, ['form_params' => [ + 'client_id' => self::CLIENT_ID, + 'client_secret' => self::CLIENT_SECRET, + 'redirect_uri' => self::REDIRECT_URI, + 'grant_type' => 'authorization_code', + 'code' => 'test-code', + ]], $mockTokenResponse], + ]); + + $mockCacheItem = $this->createStub(CacheItemInterface::class); + $mockCacheItem->method('isHit')->willReturn(false); + + $mockCacheItemPool = $this->createStub(CacheItemPoolInterface::class); + $mockCacheItemPool->method('getItem')->willReturn($mockCacheItem); + + $provider = new OpenIdConfigurationProvider([ + 'openIDConnectMetadataUrl' => $openIDConnectMetadataUrl, + 'cacheItemPool' => $mockCacheItemPool, + 'clientId' => self::CLIENT_ID, + 'clientSecret' => self::CLIENT_SECRET, + 'redirectUri' => self::REDIRECT_URI, + ], [ + 'httpClient' => $mockHttpClient, + ]); + + $this->expectException(CodeException::class); + $this->expectExceptionMessage('Token endpoint response missing string "id_token"'); + + $provider->getIdToken('test-code'); + } + public function testGetConfigurationCacheHit(): void { - $configuration = json_decode( - file_get_contents(__DIR__.'/../MockData/mockOpenIDConfiguration.json'), - true - ); + $configuration = $this->loadMockFixture('mockOpenIDConfiguration.json'); $mockCacheItem = $this->createStub(CacheItemInterface::class); $mockCacheItem->method('isHit')->willReturn(true); @@ -568,13 +634,39 @@ public function testGetConfigurationCacheHit(): void public function testGetConfigurationMissingKey(): void { - $this->expectException(CacheException::class); - $this->expectExceptionMessage('Required config key not defined: nonexistent_key'); + $this->expectException(\ItkDev\OpenIdConnect\Exception\MetadataException::class); + $this->expectExceptionMessage('OIDC discovery document missing required key: nonexistent_key'); $method = new \ReflectionMethod(OpenIdConfigurationProvider::class, 'getConfiguration'); $method->invoke($this->provider, 'nonexistent_key'); } + public function testGetConfigurationNonStringValue(): void + { + $mockCacheItem = $this->createStub(CacheItemInterface::class); + $mockCacheItem->method('isHit')->willReturn(true); + $mockCacheItem->method('get')->willReturn(['authorization_endpoint' => 42]); + + $mockCacheItemPool = $this->createStub(CacheItemPoolInterface::class); + $mockCacheItemPool->method('getItem')->willReturn($mockCacheItem); + + $provider = new OpenIdConfigurationProvider([ + 'openIDConnectMetadataUrl' => 'https://some.url/openid-configuration', + 'cacheItemPool' => $mockCacheItemPool, + 'clientId' => self::CLIENT_ID, + 'clientSecret' => self::CLIENT_SECRET, + 'redirectUri' => self::REDIRECT_URI, + ], [ + 'httpClient' => $this->createStub(ClientInterface::class), + ]); + + $this->expectException(\ItkDev\OpenIdConnect\Exception\MetadataException::class); + $this->expectExceptionMessage('OIDC discovery document value for "authorization_endpoint" is not a string (got int)'); + + $method = new \ReflectionMethod(OpenIdConfigurationProvider::class, 'getConfiguration'); + $method->invoke($provider, 'authorization_endpoint'); + } + public function testFetchJsonResourceNon200(): void { $openIDConnectMetadataUrl = 'https://some.url/openid-configuration'; @@ -677,6 +769,101 @@ public function testFetchJsonResourceInvalidJson(): void $provider->getBaseAuthorizationUrl(); } + public function testGetJwtVerificationKeysRejectsJwksMissingKeysArray(): void + { + $provider = $this->createProviderWithCustomJwks((string) json_encode(['something_else' => 1])); + \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); + + $this->expectException(JwksException::class); + $this->expectExceptionMessage('JWKS payload missing array "keys" property (RFC 7517 §5)'); + + $provider->validateIdToken('token', self::NONCE); + } + + public function testGetJwtVerificationKeysRejectsNonObjectJwkEntry(): void + { + $provider = $this->createProviderWithCustomJwks((string) json_encode(['keys' => [42]])); + \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); + + $this->expectException(JwksException::class); + $this->expectExceptionMessage('JWK entry is not a JSON object'); + + $provider->validateIdToken('token', self::NONCE); + } + + public function testGetJwtVerificationKeysRejectsNonStringKty(): void + { + $provider = $this->createProviderWithCustomJwks( + (string) json_encode(['keys' => [['kid' => 'key-1', 'kty' => 42]]]), + ); + \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); + + $this->expectException(JwksException::class); + $this->expectExceptionMessage('JWK entry missing string "kty" for key id: key-1'); + + $provider->validateIdToken('token', self::NONCE); + } + + public function testGetJwtVerificationKeysRejectsRsaWithoutStringExpOrModulus(): void + { + $provider = $this->createProviderWithCustomJwks( + (string) json_encode(['keys' => [['kid' => 'key-1', 'kty' => 'RSA', 'e' => 42, 'n' => 'abc']]]), + ); + \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); + + $this->expectException(JwksException::class); + $this->expectExceptionMessage('JWK RSA entry missing string "e"/"n" for key id: key-1'); + + $provider->validateIdToken('token', self::NONCE); + } + + public function testGetJwtVerificationKeysRejectsNonStringKid(): void + { + $openIDConnectMetadataUrl = 'https://some.url/openid-configuration'; + $jwks_uri = 'https://azure_b2c_test.b2clogin.com/azure_b2c_test.onmicrosoft.com/discovery/v2.0/keys?p=test-policy'; + + $mockConfigResponse = $this->getMockHttpSuccessResponse('/../MockData/mockOpenIDConfiguration.json'); + + // JWK with an int `kid` — violates RFC 7517 §4.5 (kid must be a string). + $badJwks = json_encode(['keys' => [['kid' => 42, 'kty' => 'RSA', 'e' => 'AQAB', 'n' => 'abc']]]); + $mockKeysStream = $this->createStub(StreamInterface::class); + $mockKeysStream->method('getContents')->willReturn($badJwks); + + $mockKeysResponse = $this->createStub(ResponseInterface::class); + $mockKeysResponse->method('getStatusCode')->willReturn(200); + $mockKeysResponse->method('getBody')->willReturn($mockKeysStream); + + $mockHttpClient = $this->createStub(ClientInterface::class); + $mockHttpClient->method('request')->willReturnMap([ + ['GET', $openIDConnectMetadataUrl, [], $mockConfigResponse], + ['GET', $jwks_uri, [], $mockKeysResponse], + ]); + + $mockCacheItem = $this->createStub(CacheItemInterface::class); + $mockCacheItem->method('isHit')->willReturn(false); + + $mockCacheItemPool = $this->createStub(CacheItemPoolInterface::class); + $mockCacheItemPool->method('getItem')->willReturn($mockCacheItem); + + $provider = new OpenIdConfigurationProvider([ + 'openIDConnectMetadataUrl' => $openIDConnectMetadataUrl, + 'cacheItemPool' => $mockCacheItemPool, + 'clientId' => self::CLIENT_ID, + 'clientSecret' => self::CLIENT_SECRET, + 'redirectUri' => self::REDIRECT_URI, + ], [ + 'httpClient' => $mockHttpClient, + ]); + + /** @var \Mockery\MockInterface $mockJWT */ + $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); + + $this->expectException(JwksException::class); + $this->expectExceptionMessage('JWK entry missing string "kid" (RFC 7517 §4.5)'); + + $provider->validateIdToken('token', self::NONCE); + } + public function testGetJwtVerificationKeysUnsupportedKeyType(): void { $openIDConnectMetadataUrl = 'https://some.url/openid-configuration'; @@ -714,9 +901,10 @@ public function testGetJwtVerificationKeysUnsupportedKeyType(): void 'httpClient' => $mockHttpClient, ]); + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); - $this->expectException(KeyException::class); + $this->expectException(JwksException::class); $this->expectExceptionMessage('Unsupported key data for key id: ec-key-1'); $provider->validateIdToken('token', self::NONCE); @@ -726,10 +914,7 @@ public function testGetJwtVerificationKeysCacheHit(): void { $openIDConnectMetadataUrl = 'https://some.url/openid-configuration'; - $configuration = json_decode( - file_get_contents(__DIR__.'/../MockData/mockOpenIDConfiguration.json'), - true - ); + $configuration = $this->loadMockFixture('mockOpenIDConfiguration.json'); $cachedKeys = ['key1' => new Key('public-key-data', 'RS256')]; @@ -762,10 +947,12 @@ public function testGetJwtVerificationKeysCacheHit(): void 'httpClient' => $mockHttpClient, ]); + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); $mockClaims = $this->getMockClaims(); $mockJWT->shouldReceive('decode')->andReturn($mockClaims); + /** @var object{nonce: string} $claims */ $claims = $provider->validateIdToken('token', self::NONCE); $this->assertEquals(self::NONCE, $claims->nonce); } @@ -800,10 +987,7 @@ public function testGetConfigurationCacheInvalidArgument(): void public function testGetJwtVerificationKeysCacheInvalidArgument(): void { $openIDConnectMetadataUrl = 'https://some.url/openid-configuration'; - $configuration = json_decode( - file_get_contents(__DIR__.'/../MockData/mockOpenIDConfiguration.json'), - true - ); + $configuration = $this->loadMockFixture('mockOpenIDConfiguration.json'); $configCacheItem = $this->createStub(CacheItemInterface::class); $configCacheItem->method('isHit')->willReturn(true); @@ -877,6 +1061,7 @@ public function testBase64urlDecodeFailure(): void 'httpClient' => $mockHttpClient, ]); + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); $this->expectException(\ItkDev\OpenIdConnect\Exception\DecodeException::class); @@ -888,12 +1073,68 @@ public function testBase64urlDecodeFailure(): void /** * Get a mock success response with mock date. * - * @param string $mockResponseDataPath - * Path to the file containing the mock response data + * @return ResponseInterface A success ("200") response with mock body data + */ + /** + * Load a JSON fixture from tests/MockData and decode it as an associative + * array. Fails the test with an explicit message if the file is missing / + * unreadable / not valid JSON, rather than letting `false` or `null` flow + * silently into the assertion under test. * - * @return ResponseInterface - * A success ("200") response with mock body data + * @return array top-level decoded JSON; callers cast / narrow as needed + */ + private function loadMockFixture(string $filename): array + { + $path = __DIR__.'/../MockData/'.$filename; + $contents = file_get_contents($path); + $this->assertNotFalse($contents, sprintf('Mock fixture not readable: %s', $path)); + $decoded = json_decode($contents, true); + $this->assertIsArray($decoded, sprintf('Mock fixture is not valid JSON: %s', $path)); + + return $decoded; + } + + /** + * Build a provider whose JWKS endpoint returns the given raw JSON body. + * Used by the JWKS validation tests to feed deliberately-malformed + * payloads through `getJwtVerificationKeys`. */ + private function createProviderWithCustomJwks(string $jwksJson): OpenIdConfigurationProvider + { + $openIDConnectMetadataUrl = 'https://some.url/openid-configuration'; + $jwks_uri = 'https://azure_b2c_test.b2clogin.com/azure_b2c_test.onmicrosoft.com/discovery/v2.0/keys?p=test-policy'; + + $mockConfigResponse = $this->getMockHttpSuccessResponse('/../MockData/mockOpenIDConfiguration.json'); + + $mockKeysStream = $this->createStub(StreamInterface::class); + $mockKeysStream->method('getContents')->willReturn($jwksJson); + $mockKeysResponse = $this->createStub(ResponseInterface::class); + $mockKeysResponse->method('getStatusCode')->willReturn(200); + $mockKeysResponse->method('getBody')->willReturn($mockKeysStream); + + $mockHttpClient = $this->createStub(ClientInterface::class); + $mockHttpClient->method('request')->willReturnMap([ + ['GET', $openIDConnectMetadataUrl, [], $mockConfigResponse], + ['GET', $jwks_uri, [], $mockKeysResponse], + ]); + + $mockCacheItem = $this->createStub(CacheItemInterface::class); + $mockCacheItem->method('isHit')->willReturn(false); + + $mockCacheItemPool = $this->createStub(CacheItemPoolInterface::class); + $mockCacheItemPool->method('getItem')->willReturn($mockCacheItem); + + return new OpenIdConfigurationProvider([ + 'openIDConnectMetadataUrl' => $openIDConnectMetadataUrl, + 'cacheItemPool' => $mockCacheItemPool, + 'clientId' => self::CLIENT_ID, + 'clientSecret' => self::CLIENT_SECRET, + 'redirectUri' => self::REDIRECT_URI, + ], [ + 'httpClient' => $mockHttpClient, + ]); + } + private function getMockHttpSuccessResponse(string $mockResponseDataPath): ResponseInterface { $mockResponseData = file_get_contents(__DIR__.$mockResponseDataPath);