Merge pull request #39 from itk-dev/feature/exception-contract-migration

turegjorup · web-flow · commit 05b5375d9c8c · 2026-06-02T12:08:44.000+02:00
Rework exception hierarchy onto marker interfaces (5.0, BREAKING)
diff --git a/.markdownlintignore b/.markdownlintignore
@@ -10,3 +10,5 @@ web/*.md
 web/core/
 web/libraries/
 web/*/contrib/
+# Claude Code guidance — verbose narrative prose, intentionally long lines.
+CLAUDE.md
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,13 +7,46 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed (BREAKING)
+
+- **Exception hierarchy reworked.** Every exception thrown from a public
+  method now implements `OpenIdConnectBundleExceptionInterface` (which
+  extends `\ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface`
+  from the upstream library). Concrete exceptions now extend the SPL type
+  that best describes the failure category (`\RuntimeException`,
+  `\InvalidArgumentException`); they no longer extend
+  `ItkOpenIdConnectBundleException`. Consumers catching the abstract base
+  must migrate to `OpenIdConnectBundleExceptionInterface` — the abstract
+  class is kept for this release as a documented alias and is
+  `@deprecated`, but `catch (ItkOpenIdConnectBundleException $e)` blocks
+  will no longer match any concrete thrown by the bundle.
+- Bumped `itk-dev/openid-connect` requirement to `^5.0` for the matching
+  upstream contract.
+- `OpenIdLoginAuthenticator::validateClaims` now catches on the marker
+  interface (`OpenIdConnectExceptionInterface`) instead of the deprecated
+  upstream abstract. The `$previous`-chain behaviour is preserved.
+- `LoginController::login` catches on the marker interface before mapping
+  to `ServiceUnavailableHttpException`. No consumer-visible behaviour
+  change.
+
+### Added
+
+- `ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface`
+  marker for catching all bundle-thrown OIDC failures.
+
 ### Changed
 
 - Hardened static analysis. PHPStan now analyses `tests/` in addition to
   `src/`, runs the strict, deprecation, PHPUnit and Symfony rule packs, and
   requires a comment on every ignore (`reportIgnoresWithoutComments`). Pinned
   `phpstan/phpstan` to `^2.1.41`. No public-API or behavioural change.
 
+### Deprecated
+
+- `ItkDev\OpenIdConnectBundle\Exception\ItkOpenIdConnectBundleException`
+  abstract class (catch `OpenIdConnectBundleExceptionInterface` instead).
+  Will be removed in 6.0.
+
 ### Fixed
 
 - Tests build real `Request` instances instead of stubbing `Request`, which
diff --git a/composer.json b/composer.json
@@ -18,7 +18,7 @@
         "ext-json": "*",
         "ext-openssl": "*",
         "doctrine/orm": "^2.8 || ^3.0",
-        "itk-dev/openid-connect": "^4.0",
+        "itk-dev/openid-connect": "^5.0",
         "symfony/cache": "^6.4 || ^7.0 || ^8.0",
         "symfony/framework-bundle": "^6.4.13 || ^7.0 || ^8.0",
         "symfony/security-bundle": "^6.4.13 || ^7.0 || ^8.0",
diff --git a/src/Controller/LoginController.php b/src/Controller/LoginController.php
@@ -2,7 +2,7 @@
 
 namespace ItkDev\OpenIdConnectBundle\Controller;
 
-use ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException;
+use ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface;
 use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException;
 use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager;
 use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
@@ -27,7 +27,7 @@ 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)
-     * @throws ItkOpenIdConnectException       Other provider-init failures (e.g. BadUrlException for a misconfigured metadata_url) — server-side configuration bugs that intentionally bubble as 500
+     * @throws OpenIdConnectExceptionInterface 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
@@ -53,7 +53,7 @@ public function login(Request $request, SessionInterface $session, string $provi
                 'response_type' => 'code',
                 'scope' => 'openid email profile',
             ]);
-        } catch (ItkOpenIdConnectException $e) {
+        } catch (OpenIdConnectExceptionInterface $e) {
             // Building the authorization URL fetches the IdP's discovery
             // document. Surface upstream/transport/cache failures as 503 with
             // the cause chained, rather than an unhandled 500.
diff --git a/src/Exception/CacheException.php b/src/Exception/CacheException.php
@@ -2,6 +2,6 @@
 
 namespace ItkDev\OpenIdConnectBundle\Exception;
 
-class CacheException extends ItkOpenIdConnectBundleException
+class CacheException extends \RuntimeException implements OpenIdConnectBundleExceptionInterface
 {
 }
diff --git a/src/Exception/InvalidProviderException.php b/src/Exception/InvalidProviderException.php
@@ -2,6 +2,6 @@
 
 namespace ItkDev\OpenIdConnectBundle\Exception;
 
-class InvalidProviderException extends ItkOpenIdConnectBundleException
+class InvalidProviderException extends \InvalidArgumentException implements OpenIdConnectBundleExceptionInterface
 {
 }
diff --git a/src/Exception/ItkOpenIdConnectBundleException.php b/src/Exception/ItkOpenIdConnectBundleException.php
@@ -2,6 +2,12 @@
 
 namespace ItkDev\OpenIdConnectBundle\Exception;
 
-abstract class ItkOpenIdConnectBundleException extends \Exception
+/**
+ * @deprecated since 5.0, will be removed in 6.0.
+ *             Catch {@see OpenIdConnectBundleExceptionInterface} instead. Concrete bundle
+ *             exceptions no longer extend this class; they extend the SPL exception that best
+ *             describes the failure category and implement the marker interface.
+ */
+abstract class ItkOpenIdConnectBundleException extends \Exception implements OpenIdConnectBundleExceptionInterface
 {
 }
diff --git a/src/Exception/OpenIdConnectBundleExceptionInterface.php b/src/Exception/OpenIdConnectBundleExceptionInterface.php
@@ -0,0 +1,16 @@
+<?php
+
+namespace ItkDev\OpenIdConnectBundle\Exception;
+
+use ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface;
+
+/**
+ * Marker interface for every exception thrown from a public method of this bundle.
+ *
+ * Extends the upstream library marker so a consumer can catch every OIDC failure
+ * from both packages with a single `catch (OpenIdConnectExceptionInterface $e)`,
+ * or scope to bundle-only failures with `catch (OpenIdConnectBundleExceptionInterface $e)`.
+ */
+interface OpenIdConnectBundleExceptionInterface extends OpenIdConnectExceptionInterface
+{
+}
diff --git a/src/Exception/TokenNotFoundException.php b/src/Exception/TokenNotFoundException.php
@@ -2,6 +2,6 @@
 
 namespace ItkDev\OpenIdConnectBundle\Exception;
 
-class TokenNotFoundException extends ItkOpenIdConnectBundleException
+class TokenNotFoundException extends \RuntimeException implements OpenIdConnectBundleExceptionInterface
 {
 }
diff --git a/src/Exception/UserDoesNotExistException.php b/src/Exception/UserDoesNotExistException.php
@@ -2,6 +2,6 @@
 
 namespace ItkDev\OpenIdConnectBundle\Exception;
 
-class UserDoesNotExistException extends ItkOpenIdConnectBundleException
+class UserDoesNotExistException extends \RuntimeException implements OpenIdConnectBundleExceptionInterface
 {
 }
diff --git a/src/Exception/UsernameDoesNotExistException.php b/src/Exception/UsernameDoesNotExistException.php
@@ -2,6 +2,6 @@
 
 namespace ItkDev\OpenIdConnectBundle\Exception;
 
-class UsernameDoesNotExistException extends ItkOpenIdConnectBundleException
+class UsernameDoesNotExistException extends \InvalidArgumentException implements OpenIdConnectBundleExceptionInterface
 {
 }
diff --git a/src/Security/OpenIdConfigurationProviderManager.php b/src/Security/OpenIdConfigurationProviderManager.php
@@ -2,7 +2,7 @@
 
 namespace ItkDev\OpenIdConnectBundle\Security;
 
-use ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException;
+use ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface;
 use ItkDev\OpenIdConnect\Security\OpenIdConfigurationProvider;
 use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException;
 use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
@@ -53,8 +53,7 @@ public function getProviderKeys(): array
     /**
      * Get a provider by key.
      *
-     * @throws InvalidProviderException
-     * @throws ItkOpenIdConnectException
+     * @throws OpenIdConnectExceptionInterface
      */
     public function getProvider(string $key): OpenIdConfigurationProvider
     {
@@ -92,6 +91,7 @@ public function getProvider(string $key): OpenIdConfigurationProvider
                 $providerOptions += $options['http_client_options'];
             }
 
+            // @phpstan-ignore argument.type (library 5.0 narrowed $options to a strict array shape; the incremental build above is verified by the manager's tests but PHPStan can't track its evolution to the final shape)
             $this->providers[$key] = new OpenIdConfigurationProvider($providerOptions);
         }
 
diff --git a/src/Security/OpenIdLoginAuthenticator.php b/src/Security/OpenIdLoginAuthenticator.php
@@ -2,10 +2,8 @@
 
 namespace ItkDev\OpenIdConnectBundle\Security;
 
-use ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException;
+use ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface;
 use ItkDev\OpenIdConnect\Exception\ValidationException;
-use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException;
-use Psr\Http\Client\ClientExceptionInterface;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\Security\Core\Exception\AuthenticationException;
@@ -36,10 +34,7 @@ public function supports(Request $request): ?bool
      *
      * @return array<string, string> Array of claims
      *
-     * @throws InvalidProviderException
-     * @throws ItkOpenIdConnectException
-     * @throws ValidationException
-     * @throws ClientExceptionInterface
+     * @throws OpenIdConnectExceptionInterface
      */
     protected function validateClaims(Request $request): array
     {
@@ -70,7 +65,7 @@ protected function validateClaims(Request $request): array
             $idToken = $provider->getIdToken($code);
             $claims = $provider->validateIdToken($idToken, $oauth2nonce);
             // Authentication successful
-        } catch (ItkOpenIdConnectException $exception) {
+        } catch (OpenIdConnectExceptionInterface $exception) {
             // Handle failed authentication
             throw new ValidationException($exception->getMessage(), previous: $exception);
         }
diff --git a/src/Util/CliLoginHelper.php b/src/Util/CliLoginHelper.php
@@ -3,6 +3,7 @@
 namespace ItkDev\OpenIdConnectBundle\Util;
 
 use ItkDev\OpenIdConnectBundle\Exception\CacheException;
+use ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface;
 use ItkDev\OpenIdConnectBundle\Exception\TokenNotFoundException;
 use Psr\Cache\CacheItemPoolInterface;
 use Psr\Cache\InvalidArgumentException;
@@ -23,7 +24,7 @@ public function __construct(
     /**
      * Creates login token for CLI login.
      *
-     * @throws CacheException
+     * @throws OpenIdConnectBundleExceptionInterface
      */
     public function createToken(string $username): string
     {
@@ -65,8 +66,11 @@ public function createToken(string $username): string
     /**
      * Gets username from login token.
      *
+     * `TokenNotFoundException` is part of the public contract — {@see CliLoginTokenAuthenticator}
+     * catches it specifically to distinguish "no such token" from other cache failures.
+     *
      * @throws TokenNotFoundException
-     * @throws CacheException
+     * @throws OpenIdConnectBundleExceptionInterface
      */
     public function getUsername(string $token): ?string
     {
diff --git a/tests/Exception/ExceptionHierarchyTest.php b/tests/Exception/ExceptionHierarchyTest.php
@@ -0,0 +1,133 @@
+<?php
+
+namespace ItkDev\OpenIdConnectBundle\Tests\Exception;
+
+use ItkDev\OpenIdConnect\Exception\HttpException as LibraryHttpException;
+use ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface;
+use ItkDev\OpenIdConnectBundle\Exception\CacheException;
+use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException;
+use ItkDev\OpenIdConnectBundle\Exception\ItkOpenIdConnectBundleException;
+use ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface;
+use ItkDev\OpenIdConnectBundle\Exception\TokenNotFoundException;
+use ItkDev\OpenIdConnectBundle\Exception\UserDoesNotExistException;
+use ItkDev\OpenIdConnectBundle\Exception\UsernameDoesNotExistException;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+
+/**
+ * Locks the bundle's exception contract (see
+ * docs/adr/001-marker-interface-exception-hierarchy.md):
+ *
+ * - Every concrete bundle exception implements {@see OpenIdConnectBundleExceptionInterface}
+ *   (and therefore {@see OpenIdConnectExceptionInterface} from the upstream library).
+ * - Each concrete extends the SPL type that best describes its failure category.
+ * - A single `catch (OpenIdConnectExceptionInterface $e)` matches every concrete
+ *   from both the bundle and the upstream library.
+ *
+ * A change that breaks any of these properties is a MAJOR version bump per
+ * SemVer commitments — failing this test class is the early warning.
+ */
+class ExceptionHierarchyTest extends TestCase
+{
+    /**
+     * @return iterable<string, array{class-string<\Throwable>, class-string<\Throwable>}>
+     */
+    public static function concreteProvider(): iterable
+    {
+        // Invalid input to a public method → \InvalidArgumentException
+        yield 'InvalidProviderException' => [InvalidProviderException::class, \InvalidArgumentException::class];
+        yield 'UsernameDoesNotExistException' => [UsernameDoesNotExistException::class, \InvalidArgumentException::class];
+
+        // Runtime conditions → \RuntimeException
+        yield 'CacheException' => [CacheException::class, \RuntimeException::class];
+        yield 'TokenNotFoundException' => [TokenNotFoundException::class, \RuntimeException::class];
+        yield 'UserDoesNotExistException' => [UserDoesNotExistException::class, \RuntimeException::class];
+    }
+
+    /**
+     * @param class-string<\Throwable> $concrete
+     * @param class-string<\Throwable> $expectedSplParent
+     */
+    #[DataProvider('concreteProvider')]
+    public function testConcreteImplementsBundleMarker(string $concrete, string $expectedSplParent): void
+    {
+        $instance = new $concrete('test');
+        $this->assertInstanceOf(OpenIdConnectBundleExceptionInterface::class, $instance);
+    }
+
+    /**
+     * @param class-string<\Throwable> $concrete
+     * @param class-string<\Throwable> $expectedSplParent
+     */
+    #[DataProvider('concreteProvider')]
+    public function testConcreteImplementsLibraryMarker(string $concrete, string $expectedSplParent): void
+    {
+        $instance = new $concrete('test');
+        $this->assertInstanceOf(OpenIdConnectExceptionInterface::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 testCatchByBundleMarkerMatchesEveryConcrete(string $concrete, string $expectedSplParent): void
+    {
+        try {
+            throw new $concrete('test');
+        } catch (OpenIdConnectBundleExceptionInterface $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 OpenIdConnectBundleExceptionInterface should have matched '.$concrete);
+    }
+
+    /**
+     * Cross-package contract: a single `catch (OpenIdConnectExceptionInterface $e)`
+     * must catch failures from both the library and the bundle. This is the whole
+     * point of the bundle marker extending the library marker.
+     */
+    public function testLibraryMarkerCatchesBothPackages(): void
+    {
+        $caught = [];
+
+        foreach ([new LibraryHttpException('lib'), new CacheException('bundle')] as $exception) {
+            try {
+                throw $exception;
+            } catch (OpenIdConnectExceptionInterface $e) {
+                $caught[] = $e::class;
+            }
+        }
+
+        $this->assertSame([LibraryHttpException::class, CacheException::class], $caught);
+    }
+
+    public function testDeprecatedAbstractBaseImplementsBundleMarker(): void
+    {
+        // `ItkOpenIdConnectBundleException` is kept as a deprecated alias through 5.x.
+        // Concrete bundle exceptions no longer extend it, but it still implements the
+        // marker so any consumer-defined subclass remains catchable via the marker.
+        // PHPStan can statically prove the assertion holds today; the test exists so
+        // the day a refactor removes the implements, the failure is loud.
+        $deprecated = ItkOpenIdConnectBundleException::class; // @phpstan-ignore classConstant.deprecatedClass (the test asserts a property of this deprecated class on purpose)
+        // @phpstan-ignore method.alreadyNarrowedType (the assertion is the guard — PHPStan proves it today; the test fails the day the guard stops holding)
+        $this->assertTrue(
+            // @phpstan-ignore function.alreadyNarrowedType (same as above — the static proof IS the contract being asserted)
+            is_subclass_of($deprecated, OpenIdConnectBundleExceptionInterface::class),
+            'Deprecated abstract base must continue to implement the bundle marker through 5.x.',
+        );
+    }
+}
diff --git a/tests/Security/OpenIdLoginAuthenticatorTest.php b/tests/Security/OpenIdLoginAuthenticatorTest.php
diff --git a/tests/Util/CliLoginHelperTest.php b/tests/Util/CliLoginHelperTest.php

Original file line number	Diff line number	Diff line change
`@@ -2,6 +2,6 @@`
`2`	`2`
`3`	`3`	`namespace ItkDev\OpenIdConnectBundle\Exception;`
`4`	`4`
`5`		`-class CacheException extends ItkOpenIdConnectBundleException`
	`5`	`+class CacheException extends \RuntimeException implements OpenIdConnectBundleExceptionInterface`
`6`	`6`	`{`
`7`	`7`	`}`
Original file line number	Diff line number	Diff line change
`@@ -2,6 +2,6 @@`
`2`	`2`
`3`	`3`	`namespace ItkDev\OpenIdConnectBundle\Exception;`
`4`	`4`
`5`		`-class InvalidProviderException extends ItkOpenIdConnectBundleException`
	`5`	`+class InvalidProviderException extends \InvalidArgumentException implements OpenIdConnectBundleExceptionInterface`
`6`	`6`	`{`
`7`	`7`	`}`
Original file line number	Diff line number	Diff line change
`@@ -2,6 +2,12 @@`
`2`	`2`
`3`	`3`	`namespace ItkDev\OpenIdConnectBundle\Exception;`
`4`	`4`
`5`		`-abstract class ItkOpenIdConnectBundleException extends \Exception`
	`5`	`+/**`
	`6`	`+ * @deprecated since 5.0, will be removed in 6.0.`
	`7`	`+ * Catch {@see OpenIdConnectBundleExceptionInterface} instead. Concrete bundle`
	`8`	`+ * exceptions no longer extend this class; they extend the SPL exception that best`
	`9`	`+ * describes the failure category and implement the marker interface.`
	`10`	`+ */`
	`11`	`+abstract class ItkOpenIdConnectBundleException extends \Exception implements OpenIdConnectBundleExceptionInterface`
`6`	`12`	`{`
`7`	`13`	`}`
Original file line number	Diff line number	Diff line change
`@@ -2,6 +2,6 @@`
`2`	`2`
`3`	`3`	`namespace ItkDev\OpenIdConnectBundle\Exception;`
`4`	`4`
`5`		`-class TokenNotFoundException extends ItkOpenIdConnectBundleException`
	`5`	`+class TokenNotFoundException extends \RuntimeException implements OpenIdConnectBundleExceptionInterface`
`6`	`6`	`{`
`7`	`7`	`}`
Original file line number	Diff line number	Diff line change
`@@ -2,6 +2,6 @@`
`2`	`2`
`3`	`3`	`namespace ItkDev\OpenIdConnectBundle\Exception;`
`4`	`4`
`5`		`-class UserDoesNotExistException extends ItkOpenIdConnectBundleException`
	`5`	`+class UserDoesNotExistException extends \RuntimeException implements OpenIdConnectBundleExceptionInterface`
`6`	`6`	`{`
`7`	`7`	`}`