From 4f1e45c3b6d26e2c1c59e345735f8bb9130fc218 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 24 Mar 2026 12:54:25 +0100 Subject: [PATCH 01/15] Bump actions/checkout to v6 and improve Taskfile Update all GitHub workflow files to use actions/checkout@v6. Improve Taskfile with renamed PHP variable, new lint:composer task, better test:coverage with XDEBUG_MODE, and fixes for test:run and test:matrix:reset. Co-Authored-By: Claude Opus 4.6 (1M context) --- .github/workflows/changelog.yaml | 2 +- .github/workflows/composer.yaml | 6 ++--- .github/workflows/github_build_release.yml | 2 +- .github/workflows/markdown.yaml | 2 +- .github/workflows/php.yaml | 6 ++--- .github/workflows/yaml.yaml | 2 +- CHANGELOG.md | 9 ++++++++ Taskfile.yml | 27 ++++++++++++++-------- 8 files changed, 37 insertions(+), 19 deletions(-) diff --git a/.github/workflows/changelog.yaml b/.github/workflows/changelog.yaml index fead572..63638c2 100644 --- a/.github/workflows/changelog.yaml +++ b/.github/workflows/changelog.yaml @@ -18,7 +18,7 @@ jobs: fail-fast: false steps: - name: Checkout - uses: actions/checkout@v5 + uses: actions/checkout@v6 with: fetch-depth: 2 diff --git a/.github/workflows/composer.yaml b/.github/workflows/composer.yaml index eebafe7..965d42a 100644 --- a/.github/workflows/composer.yaml +++ b/.github/workflows/composer.yaml @@ -44,7 +44,7 @@ jobs: matrix: prefer: [prefer-lowest, prefer-stable] steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 - name: Create docker network run: | @@ -58,7 +58,7 @@ jobs: strategy: fail-fast: false steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 - name: Create docker network run: | @@ -73,7 +73,7 @@ jobs: strategy: fail-fast: false steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 - name: Create docker network run: | diff --git a/.github/workflows/github_build_release.yml b/.github/workflows/github_build_release.yml index 0751011..b2c083f 100644 --- a/.github/workflows/github_build_release.yml +++ b/.github/workflows/github_build_release.yml @@ -16,7 +16,7 @@ jobs: APP_ENV: prod steps: - name: Checkout - uses: actions/checkout@v5 + uses: actions/checkout@v6 - name: Create a release in GitHub run: gh release create ${{ github.ref_name }} --verify-tag --generate-notes diff --git a/.github/workflows/markdown.yaml b/.github/workflows/markdown.yaml index ae83163..8f0fc25 100644 --- a/.github/workflows/markdown.yaml +++ b/.github/workflows/markdown.yaml @@ -34,7 +34,7 @@ jobs: fail-fast: false steps: - name: Checkout - uses: actions/checkout@v5 + uses: actions/checkout@v6 - name: Create docker network run: | diff --git a/.github/workflows/php.yaml b/.github/workflows/php.yaml index 39a0643..ec59121 100644 --- a/.github/workflows/php.yaml +++ b/.github/workflows/php.yaml @@ -15,7 +15,7 @@ jobs: name: PHP - Check Coding Standards runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 - name: Create docker network run: | @@ -29,7 +29,7 @@ jobs: name: PHPStan runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 - name: Create docker network run: | @@ -65,7 +65,7 @@ jobs: php: "8.5" prefer: prefer-stable steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 - name: Create docker network run: | diff --git a/.github/workflows/yaml.yaml b/.github/workflows/yaml.yaml index 631e525..299d4e1 100644 --- a/.github/workflows/yaml.yaml +++ b/.github/workflows/yaml.yaml @@ -31,7 +31,7 @@ jobs: yaml-lint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 - name: Create docker network run: | diff --git a/CHANGELOG.md b/CHANGELOG.md index b43bf5d..29c48d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- 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 +- Improved test:coverage to enable XDEBUG_MODE and add text output +- Fixed test:matrix:reset to include CI profile when removing volumes +- Fixed test:run to remove stale composer.lock before updating dependencies + ## [4.1.0] - 2026-03-20 ### Added diff --git a/Taskfile.yml b/Taskfile.yml index ba09624..0eb711d 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -4,8 +4,8 @@ version: "3" vars: DOCKER_COMPOSE: "docker compose" - PHP_EXEC: "{{.DOCKER_COMPOSE}} exec phpfpm" - COMPOSER: "{{.PHP_EXEC}} composer" + PHP: "{{.DOCKER_COMPOSE}} exec phpfpm" + COMPOSER: "{{.PHP}} composer" tasks: default: @@ -74,7 +74,7 @@ tasks: composer:check: desc: Validate and audit composer cmds: - - "{{.PHP_EXEC}} composer validate --strict" + - "{{.PHP}} composer validate --strict" - "{{.COMPOSER}} normalize --dry-run" - "{{.COMPOSER}} audit" @@ -84,18 +84,26 @@ tasks: desc: Run all linters cmds: - task: lint:php + - task: lint:composer - task: lint:markdown - task: lint:yaml lint:php: desc: Check PHP coding standards cmds: - - "{{.PHP_EXEC}} vendor/bin/php-cs-fixer fix --dry-run --diff" + - "{{.PHP}} vendor/bin/php-cs-fixer fix --dry-run --diff" lint:php:fix: desc: Fix PHP coding standards cmds: - - "{{.PHP_EXEC}} vendor/bin/php-cs-fixer fix" + - "{{.PHP}} vendor/bin/php-cs-fixer fix" + + lint:composer: + desc: Validate and audit composer + cmds: + - "{{.PHP}} composer validate --strict" + - "{{.COMPOSER}} normalize --dry-run" + - "{{.COMPOSER}} audit" lint:markdown: desc: Lint markdown files @@ -122,19 +130,19 @@ tasks: analyze:php: desc: Run PHPStan static analysis cmds: - - "{{.PHP_EXEC}} vendor/bin/phpstan" + - "{{.PHP}} vendor/bin/phpstan" # Testing test: desc: Run tests cmds: - - "{{.PHP_EXEC}} vendor/bin/phpunit" + - "{{.PHP}} vendor/bin/phpunit" test:coverage: desc: Run tests with coverage cmds: - - "{{.PHP_EXEC}} vendor/bin/phpunit --coverage-clover=coverage/unit.xml" + - "{{.DOCKER_COMPOSE}} exec -e XDEBUG_MODE=coverage phpfpm vendor/bin/phpunit --coverage-text --coverage-clover=coverage/unit.xml" test:run: desc: "Run tests for a PHP version and dependency set (e.g. task test:run PHP=8.4 DEPS=lowest)" @@ -154,6 +162,7 @@ tasks: cp /app/vendor/.composer.lock /app/composer.lock composer install -q else + rm -f /app/composer.lock composer update -q --{{.PREFER}} cp /app/composer.lock /app/vendor/.composer.lock fi' @@ -162,7 +171,7 @@ tasks: test:matrix:reset: desc: Remove cached vendor volumes to force a fresh dependency resolve cmds: - - "{{.DOCKER_COMPOSE}} down --volumes" + - "{{.DOCKER_COMPOSE}} --profile ci down --volumes" test:matrix: desc: Run tests across all PHP versions (mirrors CI matrix) From bd43892857b1ba9e0e7f007f4c8f5edd35441449 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Thu, 7 May 2026 08:18:24 +0200 Subject: [PATCH 02/15] Expand README note on Symfony native OIDC support Document Symfony 7.3 OIDC additions (discovery, token introspection, JWE) and add a comparison table clarifying which features still require this bundle. Link upstream issue for authorization code flow. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 2 ++ README.md | 48 ++++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 29c48d5..bfeef1e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Expanded README note on Symfony native OIDC support (7.3 features, + comparison table, link to upstream authorization-code-flow issue) - 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/README.md b/README.md index d7e7dd6..a1c6f35 100644 --- a/README.md +++ b/README.md @@ -17,16 +17,48 @@ Symfony bundle for authorization via OpenID Connect. > Since this bundle was created Symfony has added [support for OpenID Connect](https://symfony.com/blog/new-in-symfony-6-3-openid-connect-token-handler) > as documented in ["Using OpenID Connect (OIDC)"](https://symfony.com/doc/current/security/access_token.html#using-openid-connect-oidc). > -> As of Symfony 7.4 (March 2026), Symfony's native OIDC support has matured: +> Symfony's native OIDC support has improved significantly in recent releases: > -> * [OIDC discovery](https://github.com/symfony/symfony/pull/54932) was added in Symfony 7.3, removing the need -> for manual keyset configuration. -> * Multiple providers are supported via multiple `base_uri` and `issuers` entries in the discovery config. +> * [OIDC discovery](https://github.com/symfony/symfony/pull/54932) was added in +> Symfony 7.3 (May 2025), removing the need for manual keyset configuration. +> Keys are fetched and cached automatically from the provider's +> `.well-known/openid-configuration` endpoint. +> * [OAuth2 Token Introspection](https://symfony.com/blog/new-in-symfony-7-3-security-improvements) +> (RFC 7662) support was added in Symfony 7.3, useful when access tokens are +> opaque (not JWTs). +> * [JWE (encrypted token) support](https://github.com/symfony/symfony/pull/57721) +> was added in Symfony 7.3 for OIDC token handlers. > -> However, Symfony's native OIDC support is designed for **bearer token validation** (API authentication) only. -> It does not implement the **authorization code flow** (browser-based login with redirect to the IdP and callback -> handling), which is the primary use case of this bundle. If your application needs browser-based OIDC login, -> this bundle is still required. +> However, Symfony's native OIDC support is designed for **stateless bearer +> token validation** (the `access_token` authenticator) only. It validates tokens +> that are already present on the request (e.g. in an `Authorization: Bearer` +> header). +> +> It does **not** implement the **authorization code flow** — the browser-based +> login where the application redirects to the IdP, handles the callback with an +> authorization code, exchanges it for tokens, and establishes a session. This +> is tracked upstream in [symfony/symfony#50896](https://github.com/symfony/symfony/issues/50896). +> +> This means the following features of this bundle have no native Symfony +> equivalent: +> +> | Feature | This bundle | Symfony native | +> |--------------------------------|:-----------:|:--------------:| +> | Authorization code flow | ✅ | ❌ | +> | Session-based browser login | ✅ | ❌ | +> | Multiple named OIDC providers | ✅ | ❌ ¹ | +> | CLI login tokens | ✅ | ❌ | +> | OIDC discovery | ✅ | ✅ | +> | Bearer token validation (API) | ❌ | ✅ | +> | OAuth2 token introspection | ❌ | ✅ | +> +> ¹ Symfony's `access_token` handler accepts multiple `issuers` for token +> validation, but this is not the same as this bundle's named provider model +> with distinct client credentials, redirect URIs, and selectable login routes +> per provider. +> +> If your application needs browser-based OIDC login, this bundle is still +> required. ## Installation From b5cd84dca5f9afa824399eba40e1de2a6be9785b Mon Sep 17 00:00:00 2001 From: turegjorup Date: Thu, 7 May 2026 09:32:16 +0200 Subject: [PATCH 03/15] Expose http_client_options for per-provider HTTP timeout/proxy/verify MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Forwards the league/oauth2-client-whitelisted Guzzle options (timeout, proxy, verify) through bundle configuration. Closes the long-standing inability to bound HTTP requests to the IdP. Additive only — existing config continues to work unchanged. README adds a short note explaining why the stack uses Guzzle rather than Symfony HttpClient (league/oauth2-client hard-types its httpClient as GuzzleHttp\ClientInterface). Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 6 ++ README.md | 30 ++++++++++ src/DependencyInjection/Configuration.php | 14 +++++ .../OpenIdConfigurationProviderManager.php | 9 +++ .../DependencyInjection/ConfigurationTest.php | 47 ++++++++++++++++ ...OpenIdConfigurationProviderManagerTest.php | 55 +++++++++++++++++++ 6 files changed, 161 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 29c48d5..bc2cc8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Per-provider `http_client_options` block (`timeout`, `proxy`, `verify`) + forwarded to the underlying Guzzle HTTP client used by league/oauth2-client. + Closes the long-standing inability to bound HTTP requests to the IdP. + ### Changed - Bumped actions/checkout from v5 to v6 in all GitHub workflows diff --git a/README.md b/README.md index d7e7dd6..8e5ada9 100644 --- a/README.md +++ b/README.md @@ -115,6 +115,36 @@ OIDC_CLI_LOGIN_ROUTE=OIDC_CLI_LOGIN_ROUTE Set the actual values your `env.local` file to ensure they are not committed to Git. +#### Configuring the HTTP client + +Each provider accepts an optional `http_client_options` block that is forwarded +to the underlying Guzzle HTTP client used by `league/oauth2-client`. This is +useful for setting a request timeout so a slow IdP cannot block worker +processes indefinitely. + +```yaml +itkdev_openid_connect: + openid_providers: + user: + options: + # ... existing keys ... + http_client_options: + timeout: 5 # seconds; default unset (no timeout) + proxy: "%env(string:HTTP_PROXY)%" + verify: true # only consulted by Guzzle when proxy is set +``` + +Only the keys whitelisted by `league/oauth2-client` are forwarded: `timeout`, +`proxy`, and `verify` (the last only when `proxy` is set). + +> **Why Guzzle and not Symfony HttpClient?** +> `league/oauth2-client`, which the underlying `itk-dev/openid-connect` +> library extends, hard-types its HTTP client as `GuzzleHttp\ClientInterface`. +> Symfony HttpClient implements PSR-18 / HTTPlug, not Guzzle's interface, and +> no maintained adapter goes Symfony → Guzzle. Configure Guzzle via the +> options above; full transport replacement is not currently possible without +> a custom adapter we are not yet shipping. + In `/config/routes/` you need a similar `itkdev_openid_connect.yaml` file for configuring the routing diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index a099b77..ac3f1fb 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -80,6 +80,20 @@ public function getConfigTreeBuilder(): TreeBuilder ->info('Whether to allow http or not (default: false)') ->defaultValue(false) ->end() + ->arrayNode('http_client_options') + ->info('Options forwarded to the underlying Guzzle HTTP client. league/oauth2-client only forwards: timeout, proxy, verify (verify is only consulted when proxy is set).') + ->children() + ->floatNode('timeout') + ->info('Total request timeout in seconds') + ->end() + ->scalarNode('proxy') + ->info('HTTP proxy URI') + ->end() + ->booleanNode('verify') + ->info('Verify TLS certificates (only consulted by Guzzle when proxy is set)') + ->end() + ->end() + ->end() ->end() ->validate() ->ifTrue(static fn (array $v) => isset($v['redirect_uri'], $v['redirect_route'])) diff --git a/src/Security/OpenIdConfigurationProviderManager.php b/src/Security/OpenIdConfigurationProviderManager.php index 589d950..942f4a6 100644 --- a/src/Security/OpenIdConfigurationProviderManager.php +++ b/src/Security/OpenIdConfigurationProviderManager.php @@ -25,6 +25,11 @@ class OpenIdConfigurationProviderManager * redirect_route_parameters?: array, * leeway?: int, * allow_http?: bool, + * http_client_options?: array{ + * timeout?: float, + * proxy?: string, + * verify?: bool, + * }, * }>, * } $config */ @@ -78,6 +83,10 @@ public function getProvider(string $key): OpenIdConfigurationProvider $providerOptions['allowHttp'] = $options['allow_http']; } + if (!empty($options['http_client_options'])) { + $providerOptions += $options['http_client_options']; + } + $this->providers[$key] = new OpenIdConfigurationProvider($providerOptions); } diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php index 385ee00..baa8d5a 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -109,6 +109,53 @@ public function testBothRedirectUriAndRouteThrows(): void ); } + public function testHttpClientOptionsAccepted(): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options']['http_client_options'] = [ + 'timeout' => 2.5, + 'proxy' => 'http://proxy:8080', + 'verify' => true, + ]; + + $config = $this->processor->processConfiguration( + $this->configuration, + [$input] + ); + + $httpClientOptions = $config['openid_providers']['provider1']['options']['http_client_options']; + $this->assertSame(2.5, $httpClientOptions['timeout']); + $this->assertSame('http://proxy:8080', $httpClientOptions['proxy']); + $this->assertTrue($httpClientOptions['verify']); + } + + public function testHttpClientOptionsAbsentByDefault(): void + { + $config = $this->processor->processConfiguration( + $this->configuration, + [$this->getMinimalConfig()] + ); + + $providerOptions = $config['openid_providers']['provider1']['options']; + // Block must either be absent or empty so nothing leaks into Guzzle. + $this->assertSame([], $providerOptions['http_client_options'] ?? []); + } + + public function testHttpClientOptionsRejectsUnknownKey(): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options']['http_client_options'] = [ + 'foo' => 1, + ]; + + $this->expectException(InvalidConfigurationException::class); + + $this->processor->processConfiguration( + $this->configuration, + [$input] + ); + } + public function testMultipleProviders(): void { $input = $this->getMinimalConfig(); diff --git a/tests/Security/OpenIdConfigurationProviderManagerTest.php b/tests/Security/OpenIdConfigurationProviderManagerTest.php index 94c3837..7d722bd 100644 --- a/tests/Security/OpenIdConfigurationProviderManagerTest.php +++ b/tests/Security/OpenIdConfigurationProviderManagerTest.php @@ -2,6 +2,7 @@ namespace ItkDev\OpenIdConnectBundle\Tests\Security; +use GuzzleHttp\Client as GuzzleClient; use ItkDev\OpenIdConnect\Security\OpenIdConfigurationProvider; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; @@ -119,6 +120,60 @@ public function testGetProviderWithAllowHttp(): void $this->assertInstanceOf(OpenIdConfigurationProvider::class, $provider); } + /** + * Read a Guzzle 7 client option. + * + * Guzzle's getConfig() carries a @deprecated tag for the planned v8 removal, + * but it remains the only public way to introspect a Client's effective + * config in v7 — which is what league/oauth2-client mandates. The tests + * below assert effective config, so we intentionally call the deprecated + * accessor and silence the single phpstan diagnostic it produces. + */ + private function getGuzzleConfig(GuzzleClient $client, string $option): mixed + { + // @phpstan-ignore method.deprecated (see docblock above) + return $client->getConfig($option); + } + + public function testGetProviderForwardsHttpClientOptions(): void + { + $manager = $this->createManager([ + 'test' => $this->getBaseProviderConfig() + [ + 'redirect_uri' => 'https://app.com/callback', + 'http_client_options' => [ + 'timeout' => 1.5, + 'proxy' => 'http://proxy:8080', + 'verify' => false, + ], + ], + ]); + + $provider = $manager->getProvider('test'); + $httpClient = $provider->getHttpClient(); + + $this->assertInstanceOf(GuzzleClient::class, $httpClient); + $this->assertSame(1.5, $this->getGuzzleConfig($httpClient, 'timeout')); + $this->assertSame('http://proxy:8080', $this->getGuzzleConfig($httpClient, 'proxy')); + // verify is only forwarded by league when proxy is set. + $this->assertFalse($this->getGuzzleConfig($httpClient, 'verify')); + } + + public function testGetProviderWithoutHttpClientOptionsLeavesGuzzleDefaults(): void + { + $manager = $this->createManager([ + 'test' => $this->getBaseProviderConfig() + [ + 'redirect_uri' => 'https://app.com/callback', + ], + ]); + + $provider = $manager->getProvider('test'); + $httpClient = $provider->getHttpClient(); + + $this->assertInstanceOf(GuzzleClient::class, $httpClient); + // No timeout configured ⇒ Guzzle's getConfig returns null. Asserts no leak from our pass-through. + $this->assertNull($this->getGuzzleConfig($httpClient, 'timeout')); + } + public function testGetProviderCachesInstance(): void { $manager = $this->createManager([ From f626dd47bbb93dc941a4634e37dfee7ba77366c0 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Thu, 7 May 2026 11:02:17 +0200 Subject: [PATCH 04/15] Fix broken CI badge in README The badge pointed to a `pr.yaml` workflow that no longer exists, and the link queried for a workflow named "Test & Code Style Review" that has been renamed/split. Repointed to `php.yaml` (the workflow running tests, phpstan, and php-cs-fixer) and pinned the badge to the `develop` branch. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index a1c6f35..3ade7dd 100644 --- a/README.md +++ b/README.md @@ -3,7 +3,7 @@ [![Github](https://img.shields.io/badge/source-itk--dev/openid--connect--bundle-blue?style=flat-square)](https://github.com/itk-dev/openid-connect-bundle) [![Release](https://img.shields.io/packagist/v/itk-dev/openid-connect-bundle.svg?style=flat-square&label=release)](https://packagist.org/packages/itk-dev/openid-connect-bundle) [![PHP Version](https://img.shields.io/packagist/php-v/itk-dev/openid-connect-bundle.svg?style=flat-square&colorB=%238892BF)](https://www.php.net/downloads) -[![Build Status](https://img.shields.io/github/actions/workflow/status/itk-dev/openid-connect-bundle/pr.yaml?label=CI&logo=github&style=flat-square)](https://github.com/itk-dev/openid-connect-bundle/actions?query=workflow%3A%22Test+%26+Code+Style+Review%22) +[![Build Status](https://img.shields.io/github/actions/workflow/status/itk-dev/openid-connect-bundle/php.yaml?branch=develop&label=CI&logo=github&style=flat-square)](https://github.com/itk-dev/openid-connect-bundle/actions/workflows/php.yaml?query=branch%3Adevelop) [![Codecov Code Coverage](https://img.shields.io/codecov/c/gh/itk-dev/openid-connect-bundle?label=codecov&logo=codecov&style=flat-square)](https://codecov.io/gh/itk-dev/openid-connect-bundle) [![Read License](https://img.shields.io/packagist/l/itk-dev/openid-connect-bundle.svg?style=flat-square&colorB=darkcyan)](https://github.com/itk-dev/openid-connect-bundle/blob/master/LICENSE.md) [![Package downloads on Packagist](https://img.shields.io/packagist/dt/itk-dev/openid-connect-bundle.svg?style=flat-square&colorB=darkmagenta)](https://packagist.org/packages/itk-dev/openid-connect-bundle/stats) From 900c872875300e25ab2a686138b862353b2aaefe Mon Sep 17 00:00:00 2001 From: turegjorup Date: Fri, 8 May 2026 09:25:31 +0200 Subject: [PATCH 05/15] Clarify docs and comments for http client options --- README.md | 8 ++++++-- src/DependencyInjection/Configuration.php | 4 ++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 8e5ada9..5ca84e6 100644 --- a/README.md +++ b/README.md @@ -128,10 +128,14 @@ itkdev_openid_connect: user: options: # ... existing keys ... + # @see https://docs.guzzlephp.org/en/stable/request-options.html http_client_options: - timeout: 5 # seconds; default unset (no timeout) + # Float describing the total timeout of the request in seconds. Use 0 to wait indefinitely (the default behavior). + timeout: 5.0 + # Pass a string to specify an HTTP proxy, or an array to specify different proxies for different protocols. (Default: none) proxy: "%env(string:HTTP_PROXY)%" - verify: true # only consulted by Guzzle when proxy is set + # Describes the SSL certificate verification behavior of a request. (Default: true) + verify: true ``` Only the keys whitelisted by `league/oauth2-client` are forwarded: `timeout`, diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index ac3f1fb..562fb2d 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -80,15 +80,19 @@ public function getConfigTreeBuilder(): TreeBuilder ->info('Whether to allow http or not (default: false)') ->defaultValue(false) ->end() + // Uses Guzzle under the hood through itk-dev/openid-connect -> league/oauth2-client -> guzzlehttp/guzzle ->arrayNode('http_client_options') ->info('Options forwarded to the underlying Guzzle HTTP client. league/oauth2-client only forwards: timeout, proxy, verify (verify is only consulted when proxy is set).') ->children() + // @see https://docs.guzzlephp.org/en/stable/request-options.html#timeout ->floatNode('timeout') ->info('Total request timeout in seconds') ->end() + // @see https://docs.guzzlephp.org/en/stable/request-options.html#proxy ->scalarNode('proxy') ->info('HTTP proxy URI') ->end() + // @see https://docs.guzzlephp.org/en/stable/request-options.html#verify ->booleanNode('verify') ->info('Verify TLS certificates (only consulted by Guzzle when proxy is set)') ->end() From 075e3dd90032d2744a2a9d8a45570a362cf8c72c Mon Sep 17 00:00:00 2001 From: turegjorup Date: Fri, 8 May 2026 10:24:32 +0200 Subject: [PATCH 06/15] Map LoginController failures to specific HTTP errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously any failure during the OIDC authorization redirect bubbled unhandled as a generic 500. Map to specific responses: - InvalidProviderException -> NotFoundHttpException (404) - HttpException / JsonException / CacheException (metadata fetch) -> ServiceUnavailableHttpException (503) Cause chained via `previous`. Other ItkOpenIdConnectException subtypes (config-time errors) still bubble as 500 — correctly indicates a server-side configuration bug. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 2 + src/Controller/LoginController.php | 34 ++++++--- tests/Controller/LoginControllerTest.php | 92 +++++++++++++++++++----- 3 files changed, 102 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bfeef1e..effdb50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Mapped LoginController failures to 404 (unknown provider) or 503 + (upstream/cache) instead of a generic 500; cause chained via `previous` - Expanded README note on Symfony native OIDC support (7.3 features, comparison table, link to upstream authorization-code-flow issue) - Bumped actions/checkout from v5 to v6 in all GitHub workflows diff --git a/src/Controller/LoginController.php b/src/Controller/LoginController.php index 576cb05..8e5d3a5 100644 --- a/src/Controller/LoginController.php +++ b/src/Controller/LoginController.php @@ -2,13 +2,17 @@ namespace ItkDev\OpenIdConnectBundle\Controller; -use ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException; +use ItkDev\OpenIdConnect\Exception\CacheException; +use ItkDev\OpenIdConnect\Exception\HttpException; +use ItkDev\OpenIdConnect\Exception\JsonException; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Session\SessionInterface; +use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; +use Symfony\Component\HttpKernel\Exception\ServiceUnavailableHttpException; /** * Login Controller class. @@ -23,11 +27,16 @@ public function __construct( /** * Login method redirecting to authorizer. * - * @throws ItkOpenIdConnectException|InvalidProviderException + * @throws NotFoundHttpException Provider key not configured (404) + * @throws ServiceUnavailableHttpException IdP unreachable, returned a non-200, served malformed JSON, or local cache failed (503) */ public function login(Request $request, SessionInterface $session, string $providerKey): RedirectResponse { - $provider = $this->providerManager->getProvider($providerKey); + try { + $provider = $this->providerManager->getProvider($providerKey); + } catch (InvalidProviderException $e) { + throw new NotFoundHttpException(sprintf('Unknown OIDC provider "%s"', $providerKey), $e); + } $nonce = $provider->generateNonce(); $state = $provider->generateState(); @@ -37,12 +46,19 @@ public function login(Request $request, SessionInterface $session, string $provi $session->set('oauth2state', $state); $session->set('oauth2nonce', $nonce); - $authUrl = $provider->getAuthorizationUrl([ - 'state' => $state, - 'nonce' => $nonce, - 'response_type' => 'code', - 'scope' => 'openid email profile', - ]); + try { + $authUrl = $provider->getAuthorizationUrl([ + 'state' => $state, + 'nonce' => $nonce, + 'response_type' => 'code', + 'scope' => 'openid email profile', + ]); + } catch (HttpException|JsonException|CacheException $e) { + // Building the authorization URL fetches the IdP's discovery + // document. Surface upstream/transport failures as 503 with the + // cause chained, rather than an unhandled 500. + throw new ServiceUnavailableHttpException(null, sprintf('Cannot reach OIDC provider "%s"', $providerKey), $e); + } return new RedirectResponse($authUrl); } diff --git a/tests/Controller/LoginControllerTest.php b/tests/Controller/LoginControllerTest.php index 7305e74..0179663 100644 --- a/tests/Controller/LoginControllerTest.php +++ b/tests/Controller/LoginControllerTest.php @@ -2,23 +2,26 @@ namespace ItkDev\OpenIdConnectBundle\Tests\Controller; +use ItkDev\OpenIdConnect\Exception\CacheException; +use ItkDev\OpenIdConnect\Exception\HttpException; +use ItkDev\OpenIdConnect\Exception\JsonException; use ItkDev\OpenIdConnect\Security\OpenIdConfigurationProvider; use ItkDev\OpenIdConnectBundle\Controller\LoginController; +use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use Symfony\Component\HttpFoundation\InputBag; use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Session\SessionInterface; +use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; +use Symfony\Component\HttpKernel\Exception\ServiceUnavailableHttpException; class LoginControllerTest extends TestCase { - private LoginController $loginController; - - public function setUp(): void + public function testLogin(): void { - parent::setUp(); - $mockProvider = $this->createMock(OpenIdConfigurationProvider::class); $mockProvider ->expects($this->exactly(1)) @@ -34,18 +37,8 @@ public function setUp(): void ->with(['state' => 'abcd', 'nonce' => '1234', 'response_type' => 'code', 'scope' => 'openid email profile']) ->willReturn('https://test.com'); - $mockProviderManager = $this->createMock(OpenIdConfigurationProviderManager::class); - $mockProviderManager - ->expects($this->once()) - ->method('getProvider') - ->with('test') - ->willReturn($mockProvider); - - $this->loginController = new LoginController($mockProviderManager); - } + $controller = $this->createController($mockProvider); - public function testLogin(): void - { $stubRequest = $this->createStub(Request::class); $stubRequest->query = new InputBag(['provider' => 'test']); $mockSession = $this->createMock(SessionInterface::class); @@ -67,8 +60,73 @@ public function testLogin(): void } }); - $response = $this->loginController->login($stubRequest, $mockSession, 'test'); + $response = $controller->login($stubRequest, $mockSession, 'test'); $this->assertInstanceOf(RedirectResponse::class, $response); $this->assertSame('https://test.com', $response->getTargetUrl()); } + + public function testUnknownProviderKeyMapsTo404(): void + { + $cause = new InvalidProviderException('Invalid provider: bogus'); + + $mockProviderManager = $this->createMock(OpenIdConfigurationProviderManager::class); + $mockProviderManager + ->expects($this->once()) + ->method('getProvider') + ->with('bogus') + ->willThrowException($cause); + + $controller = new LoginController($mockProviderManager); + + try { + $controller->login($this->createStub(Request::class), $this->createStub(SessionInterface::class), 'bogus'); + $this->fail('Expected NotFoundHttpException'); + } catch (NotFoundHttpException $thrown) { + $this->assertSame(404, $thrown->getStatusCode()); + $this->assertStringContainsString('bogus', $thrown->getMessage()); + $this->assertSame($cause, $thrown->getPrevious(), 'Original exception must be chained'); + } + } + + /** + * @return iterable + */ + public static function upstreamFailureProvider(): iterable + { + yield 'metadata HTTP timeout' => [new HttpException('Connection timed out')]; + yield 'metadata malformed JSON' => [new JsonException('Syntax error')]; + yield 'cache layer failure' => [new CacheException('Cache backend unreachable')]; + } + + #[DataProvider('upstreamFailureProvider')] + public function testUpstreamFailureMapsTo503(\Throwable $cause): void + { + $stubProvider = $this->createStub(OpenIdConfigurationProvider::class); + $stubProvider->method('generateNonce')->willReturn('1234'); + $stubProvider->method('generateState')->willReturn('abcd'); + $stubProvider->method('getAuthorizationUrl')->willThrowException($cause); + + $controller = $this->createController($stubProvider); + + try { + $controller->login($this->createStub(Request::class), $this->createStub(SessionInterface::class), 'test'); + $this->fail('Expected ServiceUnavailableHttpException'); + } catch (ServiceUnavailableHttpException $thrown) { + $this->assertSame(503, $thrown->getStatusCode()); + $this->assertStringContainsString('test', $thrown->getMessage()); + $this->assertSame($cause, $thrown->getPrevious(), 'Original exception must be chained'); + } + } + + private function createController(OpenIdConfigurationProvider $provider): LoginController + { + $mockProviderManager = $this->createMock(OpenIdConfigurationProviderManager::class); + $mockProviderManager + ->expects($this->once()) + ->method('getProvider') + ->with('test') + ->willReturn($provider); + + return new LoginController($mockProviderManager); + } } From ada8ba1d4da3903da2fac4dbb50d13a55c9d2b40 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Fri, 8 May 2026 11:16:23 +0200 Subject: [PATCH 07/15] Tighten http_client_options absent-by-default assertion Drop the `?? []` fallback that masked the difference between an absent key and an empty array. Per the schema (no defaultValue on the arrayNode), an omitted input must produce no key at all. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/DependencyInjection/ConfigurationTest.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php index baa8d5a..878d847 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -137,8 +137,10 @@ public function testHttpClientOptionsAbsentByDefault(): void ); $providerOptions = $config['openid_providers']['provider1']['options']; - // Block must either be absent or empty so nothing leaks into Guzzle. - $this->assertSame([], $providerOptions['http_client_options'] ?? []); + // The block has no default value, so an omitted input must produce no + // http_client_options key in the processed config — otherwise an empty + // array would still be merged into the provider options. + $this->assertArrayNotHasKey('http_client_options', $providerOptions); } public function testHttpClientOptionsRejectsUnknownKey(): void From f303f86b9bed5251eb3c4bbbd565d87a11457bea Mon Sep 17 00:00:00 2001 From: turegjorup Date: Fri, 8 May 2026 11:17:11 +0200 Subject: [PATCH 08/15] Clarify that bundle config rejects unknown http_client_options keys The previous wording read as if league silently strips non-whitelisted keys. The bundle config rejects them up front with InvalidConfigurationException at container compile time. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 5ca84e6..b6380ec 100644 --- a/README.md +++ b/README.md @@ -138,8 +138,10 @@ itkdev_openid_connect: verify: true ``` -Only the keys whitelisted by `league/oauth2-client` are forwarded: `timeout`, -`proxy`, and `verify` (the last only when `proxy` is set). +The bundle accepts only `timeout`, `proxy`, and `verify` under +`http_client_options` — these are the keys `league/oauth2-client` forwards to +Guzzle (`verify` is consulted only when `proxy` is set). Any other key causes +an `InvalidConfigurationException` at container compile time. > **Why Guzzle and not Symfony HttpClient?** > `league/oauth2-client`, which the underlying `itk-dev/openid-connect` From c08958d8ddbf220c9524fa5e343f3c2d77c03560 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Fri, 8 May 2026 11:47:00 +0200 Subject: [PATCH 09/15] Expose per-provider cache_duration option Forwarded to the underlying library's `cacheDuration` constructor option; controls the TTL of the cached OIDC discovery document and JWKS. Default remains 86400 seconds (24h), matching the library default. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 6 ++++++ README.md | 4 ++++ src/DependencyInjection/Configuration.php | 4 ++++ src/Security/OpenIdConfigurationProviderManager.php | 5 +++++ tests/DependencyInjection/ConfigurationTest.php | 3 +++ .../OpenIdConfigurationProviderManagerTest.php | 13 +++++++++++++ 6 files changed, 35 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bfeef1e..b296afe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Per-provider `cache_duration` option (seconds) forwarded to the + underlying library; lets consumers tighten or extend the 24h default + TTL for the cached OIDC discovery document and JWKS + ### Changed - Expanded README note on Symfony native OIDC support (7.3 features, diff --git a/README.md b/README.md index 3ade7dd..7e9f050 100644 --- a/README.md +++ b/README.md @@ -108,6 +108,9 @@ itkdev_openid_connect: # Optional: Specify leeway (seconds) to account for clock skew between provider and hosting # Defaults to 10 leeway: '%env(int:ADMIN_OIDC_LEEWAY)%' + # Optional: Cache duration (seconds) for the OIDC discovery document and JWKS + # Defaults to 86400 (24 hours) + cache_duration: '%env(int:ADMIN_OIDC_CACHE_DURATION)%' # Optional: Allow (non-secure) http requests (used for mocking a IdP). NOT RECOMMENDED FOR PRODUCTION. # Defaults to false allow_http: '%env(bool:ADMIN_OIDC_ALLOW_HTTP)%' @@ -133,6 +136,7 @@ ADMIN_OIDC_CLIENT_ID=ADMIN_APP_CLIENT_ID ADMIN_OIDC_CLIENT_SECRET=ADMIN_APP_CLIENT_SECRET ADMIN_OIDC_REDIRECT_URI=ADMIN_APP_REDIRECT_URI ADMIN_OIDC_LEEWAY=30 +ADMIN_OIDC_CACHE_DURATION=86400 ADMIN_OIDC_ALLOW_HTTP=false # "user" open id connect configuration variables diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index a099b77..90be238 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -65,6 +65,10 @@ public function getConfigTreeBuilder(): TreeBuilder ->info('Leeway in seconds to account for clock skew between server and provider') ->defaultValue(10) ->end() + ->integerNode('cache_duration') + ->info('Cache duration in seconds for the OIDC discovery document and JWKS (default: 86400 — 24 hours)') + ->defaultValue(86400) + ->end() ->scalarNode('redirect_uri') ->info('Redirect URI registered at identity provider') ->cannotBeEmpty() diff --git a/src/Security/OpenIdConfigurationProviderManager.php b/src/Security/OpenIdConfigurationProviderManager.php index 589d950..cb4249c 100644 --- a/src/Security/OpenIdConfigurationProviderManager.php +++ b/src/Security/OpenIdConfigurationProviderManager.php @@ -24,6 +24,7 @@ class OpenIdConfigurationProviderManager * redirect_route?: string, * redirect_route_parameters?: array, * leeway?: int, + * cache_duration?: int, * allow_http?: bool, * }>, * } $config @@ -74,6 +75,10 @@ public function getProvider(string $key): OpenIdConfigurationProvider $providerOptions['leeway'] = $options['leeway']; } + if (isset($options['cache_duration'])) { + $providerOptions['cacheDuration'] = $options['cache_duration']; + } + if (isset($options['allow_http'])) { $providerOptions['allowHttp'] = $options['allow_http']; } diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php index 385ee00..4abc0bf 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -56,6 +56,7 @@ public function testMinimalConfig(): void $this->assertSame('my_id', $provider['client_id']); $this->assertSame('my_secret', $provider['client_secret']); $this->assertSame(10, $provider['leeway']); + $this->assertSame(86400, $provider['cache_duration']); $this->assertFalse($provider['allow_http']); } @@ -64,6 +65,7 @@ public function testFullConfig(): void $input = $this->getMinimalConfig(); $input['user_provider'] = 'my_user_provider'; $input['openid_providers']['provider1']['options']['leeway'] = 30; + $input['openid_providers']['provider1']['options']['cache_duration'] = 3600; $input['openid_providers']['provider1']['options']['redirect_uri'] = 'https://app.com/callback'; $input['openid_providers']['provider1']['options']['allow_http'] = true; @@ -76,6 +78,7 @@ public function testFullConfig(): void $provider = $config['openid_providers']['provider1']['options']; $this->assertSame(30, $provider['leeway']); + $this->assertSame(3600, $provider['cache_duration']); $this->assertSame('https://app.com/callback', $provider['redirect_uri']); $this->assertTrue($provider['allow_http']); } diff --git a/tests/Security/OpenIdConfigurationProviderManagerTest.php b/tests/Security/OpenIdConfigurationProviderManagerTest.php index 94c3837..4957a7c 100644 --- a/tests/Security/OpenIdConfigurationProviderManagerTest.php +++ b/tests/Security/OpenIdConfigurationProviderManagerTest.php @@ -106,6 +106,19 @@ public function testGetProviderWithLeeway(): void $this->assertInstanceOf(OpenIdConfigurationProvider::class, $provider); } + public function testGetProviderWithCacheDuration(): void + { + $manager = $this->createManager([ + 'test' => $this->getBaseProviderConfig() + [ + 'redirect_uri' => 'https://app.com/callback', + 'cache_duration' => 3600, + ], + ]); + + $provider = $manager->getProvider('test'); + $this->assertInstanceOf(OpenIdConfigurationProvider::class, $provider); + } + public function testGetProviderWithAllowHttp(): void { $manager = $this->createManager([ From a341a30ac8c9e0466233dcd55d90e3f506e4029b Mon Sep 17 00:00:00 2001 From: turegjorup Date: Mon, 11 May 2026 11:10:52 +0200 Subject: [PATCH 10/15] Address review on LoginController HTTP error mapping - Widen getAuthorizationUrl() catch from HttpException|JsonException|CacheException to the documented base ItkOpenIdConnectException. The catch list was an incomplete enumeration of subtypes; widening to the public base is the contract the upstream library advertises. - Document in the @throws block that other ItkOpenIdConnectException subtypes raised during provider init (BadUrlException etc.) are config-time errors and intentionally bubble as 500 rather than be coerced to 503. - Move $this->fail() out of the try block in the two new test cases so a future refactor that drops the assertion line cannot silently weaken the tests. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Controller/LoginController.php | 14 ++++++++------ tests/Controller/LoginControllerTest.php | 8 ++++++-- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/Controller/LoginController.php b/src/Controller/LoginController.php index 8e5d3a5..b9cccc7 100644 --- a/src/Controller/LoginController.php +++ b/src/Controller/LoginController.php @@ -2,9 +2,7 @@ namespace ItkDev\OpenIdConnectBundle\Controller; -use ItkDev\OpenIdConnect\Exception\CacheException; -use ItkDev\OpenIdConnect\Exception\HttpException; -use ItkDev\OpenIdConnect\Exception\JsonException; +use ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; @@ -29,6 +27,10 @@ public function __construct( * * @throws NotFoundHttpException Provider key not configured (404) * @throws ServiceUnavailableHttpException IdP unreachable, returned a non-200, served malformed JSON, or local cache failed (503) + * + * Other ItkOpenIdConnectException subtypes raised during provider init + * (e.g. BadUrlException for a misconfigured metadata_url) are server-side + * configuration bugs and intentionally bubble as 500. */ public function login(Request $request, SessionInterface $session, string $providerKey): RedirectResponse { @@ -53,10 +55,10 @@ public function login(Request $request, SessionInterface $session, string $provi 'response_type' => 'code', 'scope' => 'openid email profile', ]); - } catch (HttpException|JsonException|CacheException $e) { + } catch (ItkOpenIdConnectException $e) { // Building the authorization URL fetches the IdP's discovery - // document. Surface upstream/transport failures as 503 with the - // cause chained, rather than an unhandled 500. + // document. Surface upstream/transport/cache failures as 503 with + // the cause chained, rather than an unhandled 500. throw new ServiceUnavailableHttpException(null, sprintf('Cannot reach OIDC provider "%s"', $providerKey), $e); } diff --git a/tests/Controller/LoginControllerTest.php b/tests/Controller/LoginControllerTest.php index 0179663..a873b9c 100644 --- a/tests/Controller/LoginControllerTest.php +++ b/tests/Controller/LoginControllerTest.php @@ -80,12 +80,14 @@ public function testUnknownProviderKeyMapsTo404(): void try { $controller->login($this->createStub(Request::class), $this->createStub(SessionInterface::class), 'bogus'); - $this->fail('Expected NotFoundHttpException'); } catch (NotFoundHttpException $thrown) { $this->assertSame(404, $thrown->getStatusCode()); $this->assertStringContainsString('bogus', $thrown->getMessage()); $this->assertSame($cause, $thrown->getPrevious(), 'Original exception must be chained'); + + return; } + $this->fail('Expected NotFoundHttpException'); } /** @@ -110,12 +112,14 @@ public function testUpstreamFailureMapsTo503(\Throwable $cause): void try { $controller->login($this->createStub(Request::class), $this->createStub(SessionInterface::class), 'test'); - $this->fail('Expected ServiceUnavailableHttpException'); } catch (ServiceUnavailableHttpException $thrown) { $this->assertSame(503, $thrown->getStatusCode()); $this->assertStringContainsString('test', $thrown->getMessage()); $this->assertSame($cause, $thrown->getPrevious(), 'Original exception must be chained'); + + return; } + $this->fail('Expected ServiceUnavailableHttpException'); } private function createController(OpenIdConfigurationProvider $provider): LoginController From c3385ca81e70ddf27d615a182c5e0f8cb21a3987 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Mon, 11 May 2026 11:23:57 +0200 Subject: [PATCH 11/15] Declare all login() throws including league boundary Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Controller/LoginController.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Controller/LoginController.php b/src/Controller/LoginController.php index b9cccc7..af536d8 100644 --- a/src/Controller/LoginController.php +++ b/src/Controller/LoginController.php @@ -27,10 +27,8 @@ public function __construct( * * @throws NotFoundHttpException Provider key not configured (404) * @throws ServiceUnavailableHttpException IdP unreachable, returned a non-200, served malformed JSON, or local cache failed (503) - * - * Other ItkOpenIdConnectException subtypes raised during provider init - * (e.g. BadUrlException for a misconfigured metadata_url) are server-side - * configuration bugs and intentionally bubble as 500. + * @throws ItkOpenIdConnectException Other provider-init failures (e.g. BadUrlException for a misconfigured metadata_url) — server-side configuration bugs that intentionally bubble as 500 + * @throws \InvalidArgumentException Declared by league\AbstractProvider::getAuthorizationUrl for missing scope/state. Unreachable in this flow (state always provided, getDefaultScopes() implemented in upstream OpenIdConfigurationProvider). Bubbles as 500 if it ever fires — programmer error. */ public function login(Request $request, SessionInterface $session, string $providerKey): RedirectResponse { From 3b6152bc827d15fd8ed8892b4db9a393835794fc Mon Sep 17 00:00:00 2001 From: turegjorup Date: Mon, 11 May 2026 12:32:30 +0200 Subject: [PATCH 12/15] 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'); } } From f2764ad4e6d553f3679ae0dc50ae638d7ca4a5c2 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Mon, 11 May 2026 12:46:03 +0200 Subject: [PATCH 13/15] =?UTF-8?q?docs:=20add=20ADR=20001=20=E2=80=94=20mar?= =?UTF-8?q?ker-interface=20exception=20hierarchy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Records the decision to migrate the bundle (and upstream library) to a marker-interface exception contract: cross-package marker interfaces, concretes extending SPL types, wrap-at-boundary discipline with `$previous` chained, and the controller HTTP-exception exit treated as a documented carve-out. Migration via coordinated 5.0 majors across `itk-dev/openid-connect` and this bundle, sequenced as four PRs. PR 0 (cause-chain bug fixes) already in flight as PR #35. The ADR is self-contained: context, drivers, three options considered, decision, consequences, and references (PSR-18, Symfony's component markers and Psr18Client, Bloch's wrap-at-boundary principle). Status: Draft — awaits team review. Co-Authored-By: Claude Opus 4.7 (1M context) --- ...01-marker-interface-exception-hierarchy.md | 361 ++++++++++++++++++ docs/adr/README.md | 14 + 2 files changed, 375 insertions(+) create mode 100644 docs/adr/001-marker-interface-exception-hierarchy.md create mode 100644 docs/adr/README.md diff --git a/docs/adr/001-marker-interface-exception-hierarchy.md b/docs/adr/001-marker-interface-exception-hierarchy.md new file mode 100644 index 0000000..c6579ca --- /dev/null +++ b/docs/adr/001-marker-interface-exception-hierarchy.md @@ -0,0 +1,361 @@ +# 001: Adopt marker-interface exception hierarchy across library and bundle + +- **Created By:** Ture Gjørup +- **Date:** 2026-05-11 +- **Decision Maker:** Ture Gjørup (draft — awaits team review) +- **Stakeholders:** Bundle consumers (downstream apps catching OIDC exceptions); + `itk-dev/openid-connect` library maintainers; + `itk-dev/openid-connect-bundle` maintainers +- **Status:** Draft + +## Context + +This bundle and its upstream `itk-dev/openid-connect` library expose +exception types that consuming applications catch in production code. The +shape of that exception hierarchy is part of the public API: a consumer +deciding whether to retry, re-authenticate, or fail hard has to be able to +distinguish failure modes by `catch` type. Today, the hierarchy makes +that decision harder than it needs to be. + +**Library — `itk-dev/openid-connect`.** +The base type is the abstract class +`\ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException`, which extends +`\Exception` directly. Every concrete exception (`CacheException`, +`HttpException`, `JsonException`, `ValidationException`, `ClaimsException`, +`CodeException`, `BadUrlException`, `IllegalSchemeException`, +`MissingParameterException`, `NegativeLeewayException`, +`NegativeCacheDurationException`, `DecodeException`, `KeyException`) +extends that abstract. + +**Bundle — `itk-dev/openid-connect-bundle`.** +The base type is a separate abstract class +`\ItkDev\OpenIdConnectBundle\Exception\ItkOpenIdConnectBundleException`, +also extending `\Exception` directly. Concrete bundle exceptions +(`CacheException`, `InvalidProviderException`, `TokenNotFoundException`, +`UsernameDoesNotExistException`, `UserDoesNotExistException`) extend that +abstract. **There is no inheritance link between the bundle's base and +the library's** — they are two parallel trees that happen to share a +domain. + +This produces three consumer-visible problems: + +1. **No cross-package catch.** A downstream application that needs to + handle "any OIDC failure" — for example, to render a generic + re-authentication page — cannot write a single `catch` block. + `catch (ItkOpenIdConnectBundleException $e)` misses everything the + library throws (`HttpException` from the IdP discovery fetch, + `ClaimsException` from token validation, …). `catch (\Exception $e)` + is too wide and matches unrelated failures. The only correct option is + a list of every concrete, which rots as the libraries evolve. + +2. **No SPL-typed semantics.** The concretes all extend the bundle/library + abstract, which extends `\Exception`. A generic retry decorator that + does `catch (\RuntimeException $e)` to retry transient failures, or a + strict input validator that catches `\InvalidArgumentException` — both + common idioms — does not match anything in the OIDC trees, because the + trees skip the SPL middle layer. + +3. **Lost cause chain.** Eight wrap sites across the bundle and library + catch a dependency exception, copy its message into a new bundle/library + exception, and discard `$previous`. Log traversal stops at the wrapper + — the originating Guzzle / PSR-cache / lcobucci-JWT exception is + invisible. Concretely: four sites in + `src/Util/CliLoginHelper.php` (PSR cache wraps), + `src/Security/OpenIdLoginAuthenticator.php` line 75 (library exception + wrap), and three sites in the upstream library's + `OpenIdConfigurationProvider` (JWKS fetch, JWT validation, JSON + resource fetch). + +There are two related defects that the same migration should fix: + +- `src/Security/OpenIdLoginAuthenticator::validateClaims` calls + `$this->providerManager->getProvider($providerKey)` (line 49) without a + surrounding `try`/`catch`. The upstream `@throws` declares + `ItkOpenIdConnectException`, which escapes uncaught from a `protected` + method on a class designed to be subclassed. +- Upstream `OpenIdConfigurationProvider::__construct` throws a raw + `\InvalidArgumentException` (not within the library's hierarchy); + `checkResponse` lets + `League\OAuth2\Client\Provider\Exception\IdentityProviderException` + escape unwrapped through `getIdToken`. Both are boundary leaks — a + dependency's type surfaces from a public method of ours. + +The gap surfaced concretely during review of +[PR #33](https://github.com/itk-dev/openid-connect-bundle/pull/33) +(LoginController HTTP error mapping): the reviewer flagged that the +controller's `catch` list was enumerating subtypes incompletely. +Widening to the documented base addressed the immediate question, but the +underlying contract problem — that the base isn't part of a usable +cross-package hierarchy — remained. + +### Drivers + +- **Functional:** + - A consumer must be able to catch "any OIDC failure" with a single + `catch` line. This is the conventional Symfony / PSR idiom: every + Symfony component ships an `ExceptionInterface` marker + (`Symfony\Component\Security\Core\Exception\ExceptionInterface`, + `Symfony\Component\Console\Exception\ExceptionInterface`, etc.); + PSR-18 ships + `Psr\Http\Client\ClientExceptionInterface`. + - The `$previous` chain is consumer-visible debugging information and + must be preserved. Symfony's `Psr18Client` is the canonical wrap-at- + boundary reference: it translates `TransportExceptionInterface` into + PSR-18 exceptions while passing the original as `$previous`. + - The HTTP controller already exits through Symfony's `HttpException` + subclasses (per PR #33). The contract needs to declare this as a + deliberate carve-out rather than treat it as a violation. + +- **Non-functional:** + - Maintainability: the current convention is informal — nothing + prevents a future throw site from regressing (a new exception that + doesn't fit the tree, a wrap site that drops `$previous`). Static + analysis can lock the contract on every CI run. + - Migration cost: any change to the public exception hierarchy is + consumer-visible. The migration's cost is dominated by downstream + apps mechanically rewriting `catch` blocks; the bundle's internal + refactor is small. + - Release coordination: the bundle depends on the library + (`"itk-dev/openid-connect": "^4.0"` in `composer.json`). A + contract-level change to the library forces a coordinated MAJOR cut + on both packages — composer cannot resolve the new bundle against + the old library. + - Exception granularity: PSR-18's meta document + ([psr-18 meta](https://www.php-fig.org/psr/psr-18/meta/)) explicitly + rejected adding subtypes like `TimeOutException` because consumers + would handle them identically. Target: three to five concrete + failure categories per package, not one per RFC section. + +### Options Considered + +1. **Marker interface + SPL re-parenting (proposed).** Introduce two + marker interfaces: + `\ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface` + (in the library) and + `\ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface` + (in the bundle, extending the library's). Re-parent every concrete to + its SPL base (`\RuntimeException` for transient/network/data, + `\LogicException` for programmer/config bugs, + `\InvalidArgumentException` for invalid input). Each concrete + implements the appropriate marker. Keep the existing abstract base + classes as `@deprecated` aliases that implement the marker, so + `catch (ItkOpenIdConnectBundleException $e)` keeps working through + the 5.x line. Coordinated 5.0 majors on both packages. Custom PHPStan + rules in the bundle lock the contract on every CI run. + - **Pros:** Single `catch` line covers both packages. SPL parents + give the rare consumer who catches on `\RuntimeException` correct + semantics. PHPStan-enforced so the contract cannot silently rot. + `$previous` chain becomes a tested invariant. The + marker-interface idiom matches Symfony and PSR conventions. + - **Cons:** Two coordinated MAJOR releases. Consumers must migrate + `catch` blocks at upgrade time (mechanical: catch on the marker + interface rather than the abstract; class names of concretes + unchanged). ~200 lines of in-tree PHPStan rule code maintained + alongside production code. + +2. **Marker interface only, no SPL re-parenting.** Add the marker + interface and have the existing abstract base classes implement it. + Concrete exceptions keep their current parent (the abstract). + Additive — no MAJOR bump needed. + - **Pros:** No BC break, no MAJOR cut, consumers can adopt at their + own pace. The cross-package catch problem is solved (the most + visible defect). + - **Cons:** Skips the SPL hierarchy entirely. The + catch-on-`\RuntimeException` idiom never becomes available. Future + maintainers see the abstract being both "deprecated, prefer the + marker" and "still the only path to concrete subtypes" — confusing. + The implementation is half a design. + +3. **Status quo + targeted bug fixes only.** Keep the current abstract- + class convention, but fix the discrete defects (`$previous` chains, + the missing `getProvider` catch in `validateClaims`, the library + boundary leaks). No public API change. + - **Pros:** Lowest-cost option. PATCH-level releases only. No + consumer migration. The most visible bugs go away. + - **Cons:** The cross-package catch problem remains unfixable + without a public API change. The catch-on-SPL idiom remains + unavailable. Future maintainers inherit the same gap and the + question recurs the next time someone writes a consumer that + needs to handle OIDC failures uniformly. + +## Decision + +**Adopt Option 1.** Migrate to the marker-interface contract via +coordinated 5.0 majors on the library and bundle, sequenced as four PRs: + +- **PR 0** — `$previous` chaining bug fixes (4.x patch). Bundle repo, + branch `fix/exception-cause-chain`, + [PR #35](https://github.com/itk-dev/openid-connect-bundle/pull/35). No + dependencies; ships on the 4.x line as a `### Fixed` bullet. +- **PR 1** — Library marker interface + SPL re-parenting + boundary + wraps. Library repo, branch `feature/exception-contract-5.0`, tag + `5.0.0`. No dependencies. +- **PR 2** — Bundle marker interface + re-parent + composer bump + a new + carve-out subsection that documents the controller HTTP-exception + exit. Bundle repo, branch `feature/exception-contract-5.0`, tag + `5.0.0`. Depends on PR 1 tagged so composer can resolve `^5.0`. +- **PR 3** — Custom PHPStan rules locking the contract. Bundled inside + PR 2 so a future revert of the migration also reverts the lock-in + (intentional friction; one cannot land without the other). + +PR 0 ships as part of the in-flight 4.2.0 release. PR 2's `[Unreleased]` +bullets are reserved for the section opened after 4.2 is cut. The +bundle's `composer.json` constraint moves from +`"itk-dev/openid-connect": "^4.0"` to `"^5.0"` in PR 2. + +### Concrete-class parent mapping + +Library concretes (PR 1): + +- `\LogicException` — `BadUrlException`, `IllegalSchemeException`, + `MissingParameterException`, and a new `ConfigurationException` that + replaces the raw `\InvalidArgumentException` currently thrown by + `OpenIdConfigurationProvider::__construct`. +- `\InvalidArgumentException` — `NegativeLeewayException`, + `NegativeCacheDurationException`. +- `\RuntimeException` — `CacheException`, `HttpException` (also keeps + `Psr\Http\Client\ClientExceptionInterface`, so PSR-18-savvy consumers + can still catch on the standard PSR interface), + `JsonException`, `DecodeException`, `KeyException`, `CodeException`, + `ValidationException`, `ClaimsException`. + +Bundle concretes (PR 2): + +- `\InvalidArgumentException` — `InvalidProviderException`, + `UsernameDoesNotExistException`. +- `\RuntimeException` — `CacheException`, `TokenNotFoundException`, + `UserDoesNotExistException`. + +### Boundary fixes (in scope for PR 1 and PR 2) + +- `OpenIdLoginAuthenticator::validateClaims` line 49 — wrap + `getProvider()` in `try`/`catch (OpenIdConnectExceptionInterface)` + and surface as a bundle-typed exception with `$previous` chained. +- `OpenIdConfigurationProvider::__construct` — wrap raw + `\InvalidArgumentException` into the new `ConfigurationException`. +- `OpenIdConfigurationProvider::checkResponse` — wrap + `League\OAuth2\Client\Provider\Exception\IdentityProviderException` + into `CodeException` with `$previous` at the public method that + triggers it (`getIdToken`). +- All eight `$previous`-discarding wrap sites named in Context — pass + the caught exception as `$previous`. PR 0 fixes the bundle's five + sites already. + +### Controller carve-out + +The `LoginController::login` method exits through Symfony's +`NotFoundHttpException` / `ServiceUnavailableHttpException` (added in +PR #33) rather than through the bundle marker. The Symfony HTTP kernel +reads the status code off these specific concretes to render the +response; wrapping them into bundle types would require an exception +listener to unwrap and re-throw, with no benefit to a catching consumer +(the controller's caller is `HttpKernel`, not user code). The OIDC +cause is preserved via `$previous` so logs still carry the underlying +failure. + +This carve-out is scoped to `src/Controller/` and encoded in the custom +PHPStan rule `ThrownExceptionImplementsBundleMarker` (see below). + +### Static-analysis lock-in (PR 3) + +Added to `require-dev`: + +- `phpstan/phpstan-strict-rules` — catches general exception-hygiene + issues (empty catches, throws from void methods). +- `phpstan/phpstan-deprecation-rules` — flags any in-tree code still + using the deprecated abstract bases, so the team is reminded if a + future throw site picks up the wrong parent. + +Custom rules registered via `phpstan/exception-contract.neon`: + +- **`ThrownExceptionImplementsBundleMarker`** — every `throw new X(...)` + node in `src/` (excluding `src/Controller/`) must throw a class that + implements either bundle marker or library marker. The Controller + exception accepts any class implementing + `\Symfony\Component\HttpKernel\Exception\HttpExceptionInterface`. +- **`WrappedExceptionChainsPrevious`** — every `throw new Y(...)` inside + a `catch (\Throwable $e)` block must pass `$e` as `$previous`. Escape + hatch via per-line `@phpstan-ignore` annotation with a + justification comment. + +The PHPStan `paths` setting is extended to include `tests/`, so +`@throws` drift in test stubs and test exception expectations gets +caught too. + +## Consequences + +### Positive + +- Consumers can write + `catch (OpenIdConnectBundleExceptionInterface $e)` (or the library + marker) to handle all OIDC failures with one block, instead of + enumerating concretes or catching `\Throwable`. +- Generic retry / validator code that catches on `\RuntimeException` or + `\InvalidArgumentException` now matches the appropriate OIDC failures. + Today such code is invisible to bundle/library exceptions. +- `$previous` chain becomes a tested invariant; log traversal across the + bundle → library → dependency boundary works without consumers having + to unwrap manually. +- The contract is mechanically enforced from CI day one. A future throw + site that picks up a non-marker exception, or a future wrap site that + drops `$previous`, fails CI before review can miss it. +- The controller HTTP-exception carve-out is explicit, documented, and + statically scoped — no longer an undeclared deviation. + +### Negative / Trade-offs + +- Two coordinated MAJOR releases. Consumers cannot install bundle 5.0 + against library ≤ 4.x — the `composer.json` constraint enforces the + new contract. +- Consumer catch-block migration: anyone catching + `ItkOpenIdConnectBundleException` or `ItkOpenIdConnectException` (the + abstract bases) must switch to the marker interface. Concrete class + names and namespaces stay stable so the migration is mechanical, not + semantic. Upgrade notes accompany the 5.0 release. +- A `CacheException` re-parented to `\RuntimeException` may now match a + downstream `catch (\RuntimeException $e)` that previously did not. + Reviewed during planning: this is the desired semantic outcome, and is + called out in the upgrade notes. +- ~200 lines of in-tree PHPStan rule code (`phpstan/Rule/`) plus rule + fixtures (`phpstan/tests/`) maintained alongside the production code. + Cost is one-time write + occasional adjustment when PHPStan's rule + API evolves. +- The deprecated abstract base classes stay in the codebase through + 5.x; removal deferred to 6.0. Cost is two empty class files surviving + one major. + +### Follow-up Actions + +- [x] PR 0 — `$previous`-chain bug fixes + ([PR #35](https://github.com/itk-dev/openid-connect-bundle/pull/35)) +- [ ] PR 1 — upstream library 5.0 + (`itk-dev/openid-connect`, separate repo) +- [ ] PR 2 — bundle 5.0 contract migration + carve-out subsection added + to the project's contributor docs (depends on PR 1 tagged) +- [ ] PR 3 — custom PHPStan rules (bundled inside PR 2) +- [ ] `UPGRADE-5.0.md` (or equivalent README section) listing the + consumer `catch`-migration steps +- [ ] Open follow-up to revisit removing the deprecated abstract base + classes in 6.0 + +## References + +- PSR-18 HTTP Client — `ClientExceptionInterface` / + `NetworkExceptionInterface` / `RequestExceptionInterface`: + +- PSR-18 meta document — design rationale for not subtyping by failure + mode: +- Symfony Security Core exception marker — + `Symfony\Component\Security\Core\Exception\ExceptionInterface`: + +- Symfony Console exception marker — + `Symfony\Component\Console\Exception\ExceptionInterface`: + +- Symfony `Psr18Client` — reference implementation of wrap-at-boundary + translating `TransportExceptionInterface` into PSR-18 exceptions + while preserving `$previous`: + +- Bloch, *Effective Java*, Item 73: "Throw exceptions appropriate to + the abstraction" — the principle behind wrap-at-boundary. +- Keep a Changelog: +- Semantic Versioning 2.0.0: diff --git a/docs/adr/README.md b/docs/adr/README.md new file mode 100644 index 0000000..d766d1a --- /dev/null +++ b/docs/adr/README.md @@ -0,0 +1,14 @@ +# Architecture Decision Records + +This directory contains Architecture Decision Records (ADRs) for +`itk-dev/openid-connect-bundle`. + +ADRs capture significant architectural or design decisions along with the +context that drove them and the consequences we accept. See +[adr.github.io](https://adr.github.io/) for background. + +## Index + +- **[001 — Adopt marker-interface exception hierarchy across library and + bundle](001-marker-interface-exception-hierarchy.md)** — Draft — + 2026-05-11 From 3daaf7842f3b84c867aac33d28d5b29e4b6b87ba Mon Sep 17 00:00:00 2001 From: turegjorup Date: Mon, 11 May 2026 13:35:27 +0200 Subject: [PATCH 14/15] docs: clarify ADR 001 is a course correction, not a redesign Add a short framing paragraph at the top of "Context" stating that the marker-interface principles the ADR proposes were the intended design all along; the implementation has drifted from intent on specific points and this migration closes those gaps. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/adr/001-marker-interface-exception-hierarchy.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/docs/adr/001-marker-interface-exception-hierarchy.md b/docs/adr/001-marker-interface-exception-hierarchy.md index c6579ca..6d0dbef 100644 --- a/docs/adr/001-marker-interface-exception-hierarchy.md +++ b/docs/adr/001-marker-interface-exception-hierarchy.md @@ -10,6 +10,15 @@ ## Context +The principles this ADR proposes — cross-package marker interfaces, +concrete exceptions extending SPL types, wrap-at-boundary discipline with +`$previous` chained — are the intended design of the bundle's and +library's exception layer. This is not a change in design direction. It +is a course correction: the implementation has drifted from the intent on +several specific points, and the migration below closes those gaps and +adds the static-analysis guardrails needed to keep the implementation +aligned going forward. + This bundle and its upstream `itk-dev/openid-connect` library expose exception types that consuming applications catch in production code. The shape of that exception hierarchy is part of the public API: a consumer From e2362749dad8653f3ca900e959759a13026039b2 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Mon, 11 May 2026 14:08:38 +0200 Subject: [PATCH 15/15] release: 4.2.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Roll up the [Unreleased] section into 4.2.0 dated 2026-05-11. MINOR per SemVer — two new optional configuration features (`cache_duration` per-provider, `http_client_options.{timeout, proxy, verify}` per-provider) extend the bundle's surface without breaking existing configs. Plus PATCH-level fixes: LoginController now maps unknown-provider → 404 and IdP-unreachable / cache-failure → 503 instead of bubbling those as 500; `CliLoginHelper` and `OpenIdLoginAuthenticator::validateClaims` exception wraps now preserve `$previous` so logs can traverse to the originating PSR/upstream cause. `composer.json` constraint on `itk-dev/openid-connect` stays at `^4.0` (the just-released 4.1.2 resolves under it). Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6eed748..2b6d219 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [4.2.0] - 2026-05-11 + ### Added - Per-provider `cache_duration` option (seconds) forwarded to the @@ -152,7 +154,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `itk-dev/openid-connect` 1.0.0 to 2.1.0 - OpenId Connect Bundle: Added CLI login feature. -[unreleased]: https://github.com/itk-dev/openid-connect-bundle/compare/4.1.0...HEAD +[unreleased]: https://github.com/itk-dev/openid-connect-bundle/compare/4.2.0...HEAD +[4.2.0]: https://github.com/itk-dev/openid-connect-bundle/compare/4.1.0...4.2.0 [4.1.0]: https://github.com/itk-dev/openid-connect-bundle/compare/4.0.1...4.1.0 [4.0.1]: https://github.com/itk-dev/openid-connect-bundle/compare/4.0.0...4.0.1 [4.0.0]: https://github.com/itk-dev/openid-connect-bundle/compare/3.1.0...4.0.0