Preserve specific exception types and causes through the authenticator#32
Closed
turegjorup wants to merge 5 commits into
Closed
Preserve specific exception types and causes through the authenticator#32turegjorup wants to merge 5 commits into
turegjorup wants to merge 5 commits into
Conversation
validateClaims() no longer collapses library exceptions into a generic ValidationException. Specific subtypes (HttpException, CodeException, ClaimsException, etc.) now bubble up unchanged so callers can distinguish a timeout from a bad nonce from a malformed token. onAuthenticationFailure() now chains the original exception via `previous` and includes its message, so logs and error reporters retain the cause. Symfony's security still renders only the safe message key to the user. BC-safe: all library exceptions extend ItkOpenIdConnectException, which the method already declared in @throws — callers catching the parent keep working. 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 @@
## feature/http-client-options #32 +/- ##
===============================================================
Coverage 100.00% 100.00%
+ Complexity 57 56 -1
===============================================================
Files 9 9
Lines 264 266 +2
===============================================================
+ Hits 264 266 +2
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:
|
jekuaitk
requested changes
May 7, 2026
`onAuthenticationFailure` always throws (Assert::fail() also returns never), so the guard line was dead code per static analysis. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jekuaitk
approved these changes
May 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on top of #31 — please merge that one first; this PR's base will then auto-retarget to develop.
Summary
validateClaims()no longer collapses library exceptions into a genericValidationException. Specific subtypes (HttpException,CodeException,ClaimsException,JsonException,CacheException, etc.) bubble up unchanged so callers can distinguish a timeout from a bad nonce from a malformed token.onAuthenticationFailure()chains the original exception viapreviousand includes its message in the rethrownAuthenticationException. Logs and error reporters retain the cause; Symfony's security still renders only the safe message key to the user.ClaimsExceptionandHttpExceptionreach the caller unchanged, and thatonAuthenticationFailurepreserves the cause viagetPrevious().Why
A consumer asked: "do we see meaningful exceptions on timeout / IdP errors?" Audit found two leaks:
validateClaims()caughtItkOpenIdConnectExceptionand rethrew as the subtypeValidationExceptionwith only the message copied — type info andpreviouschain both lost.onAuthenticationFailure()threw a freshAuthenticationException('Error occurred validating openid login')that completely discarded the input exception.After this PR, a
http_client_options.timeoutfiring insidegetIdToken()reaches the application asHttpException(or transitively asCodeExceptionwrapping the Guzzle exception, withgetPrevious()populated), not as a genericValidationExceptionwith no context.Backward compatibility
Additive only. All library exceptions extend
ItkOpenIdConnectException, which the method's@throwsalready declared — anything catching the parent keeps working. Callers that catch only the narrowedValidationExceptionwill now miss subtypes, but that's the desired outcome (and the new behavior is the documented one).Test plan
task test— 52 tests pass (one renamed, two new)task analyze:php— phpstan max level, no errorstask lint— cleantask test:matrix— all 6 PHP × deps combinations greenCompanion PR
Companion library PR: itk-dev/openid-connect#37 — chains
previousconsistently in the library's catch blocks (4 of 5 were dropping the cause) and tightens@throwsphpdoc on public methods.🤖 Generated with Claude Code