Map LoginController failures to specific HTTP errors#33
Conversation
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) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #33 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 58 60 +2
===========================================
Files 9 9
Lines 274 278 +4
===========================================
+ Hits 274 278 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| 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); | ||
| } |
There was a problem hiding this comment.
According to https://github.com/itk-dev/openid-connect/blob/develop/src/Security/OpenIdConfigurationProvider.php#L119 and https://github.com/thephpleague/oauth2-client/blob/master/src/Provider/AbstractProvider.php#L483 this throws either a ItkOpenIdConnectException or InvalidArgumentException. Am i missing something?
There was a problem hiding this comment.
InvalidArgumentException maps to ServiceUnavailableHttpException. The rest are allowed to bubble up.
|
|
||
| try { | ||
| $controller->login($this->createStub(Request::class), $this->createStub(SessionInterface::class), 'bogus'); | ||
| $this->fail('Expected NotFoundHttpException'); |
There was a problem hiding this comment.
| $this->fail('Expected NotFoundHttpException'); |
|
|
||
| try { | ||
| $controller->login($this->createStub(Request::class), $this->createStub(SessionInterface::class), 'test'); | ||
| $this->fail('Expected ServiceUnavailableHttpException'); |
There was a problem hiding this comment.
| $this->fail('Expected ServiceUnavailableHttpException'); |
| try { | ||
| $provider = $this->providerManager->getProvider($providerKey); | ||
| } catch (InvalidProviderException $e) { | ||
| throw new NotFoundHttpException(sprintf('Unknown OIDC provider "%s"', $providerKey), $e); | ||
| } |
There was a problem hiding this comment.
https://github.com/itk-dev/openid-connect-bundle/blob/feature/login-controller-http-errors/src/Security/OpenIdConfigurationProviderManager.php#L52-L58 seems to indicate that this throws more than just InvalidProviderException.
There was a problem hiding this comment.
@throws updated.
- 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) <noreply@anthropic.com>
…roller-http-errors
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
I think this has surfaced a need for refactoring the Exception design/structure, but out of scope for this PR. |
3 participants
Summary
Previously, any failure during the OIDC authorization redirect (
LoginController::login()) bubbled unhandled as a generic HTTP 500. Map to specific responses:InvalidProviderException(unknown provider key)NotFoundHttpException(404)HttpException/JsonException/CacheException(metadata fetch / cache failure)ServiceUnavailableHttpException(503)ItkOpenIdConnectExceptionsubtypes (config-time errors likeBadUrlException)Original exception chained via
previousso logs and error reporters retain the cause.Backward compatibility
Additive only. The 500-bubbling behavior was never an explicit contract; consumers catching
\Throwablekeep working. Anything specifically catchingItkOpenIdConnectExceptionfrom this controller would have been broken anyway (Symfony was rendering 500 before, not handing the exception back to caller code).Test plan
task test— 50 tests pass (3 new: 1 happy path retained, 1 unknown-provider-404 case, 1 data-provided 503 case across HttpException/JsonException/CacheException)task analyze:php— phpstan max level, no errorstask lint— clean (php-cs-fixer normalized one docblock)task test:matrix— all 6 PHP × deps combinations green🤖 Generated with Claude Code