diff --git a/CHANGELOG.md b/CHANGELOG.md index cb50b49..c2873c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,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..af536d8 100644 --- a/src/Controller/LoginController.php +++ b/src/Controller/LoginController.php @@ -9,6 +9,8 @@ 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 +25,18 @@ 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) + * @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 { - $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 (ItkOpenIdConnectException $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. + 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..a873b9c 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); + $controller = $this->createController($mockProvider); - $this->loginController = new LoginController($mockProviderManager); - } - - public function testLogin(): void - { $stubRequest = $this->createStub(Request::class); $stubRequest->query = new InputBag(['provider' => 'test']); $mockSession = $this->createMock(SessionInterface::class); @@ -67,8 +60,77 @@ 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'); + } 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'); + } + + /** + * @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'); + } 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 + { + $mockProviderManager = $this->createMock(OpenIdConfigurationProviderManager::class); + $mockProviderManager + ->expects($this->once()) + ->method('getProvider') + ->with('test') + ->willReturn($provider); + + return new LoginController($mockProviderManager); + } }