From 075e3dd90032d2744a2a9d8a45570a362cf8c72c Mon Sep 17 00:00:00 2001 From: turegjorup Date: Fri, 8 May 2026 10:24:32 +0200 Subject: [PATCH 1/3] 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 a341a30ac8c9e0466233dcd55d90e3f506e4029b Mon Sep 17 00:00:00 2001 From: turegjorup Date: Mon, 11 May 2026 11:10:52 +0200 Subject: [PATCH 2/3] 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 3/3] 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 {