Skip to content

Preserve specific exception types and causes through the authenticator#32

Closed
turegjorup wants to merge 5 commits into
feature/http-client-optionsfrom
feature/exception-flow
Closed

Preserve specific exception types and causes through the authenticator#32
turegjorup wants to merge 5 commits into
feature/http-client-optionsfrom
feature/exception-flow

Conversation

@turegjorup
Copy link
Copy Markdown
Contributor

@turegjorup turegjorup commented May 7, 2026

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 generic ValidationException. 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 via previous and includes its message in the rethrown AuthenticationException. Logs and error reporters retain the cause; Symfony's security still renders only the safe message key to the user.
  • New tests assert ClaimsException and HttpException reach the caller unchanged, and that onAuthenticationFailure preserves the cause via getPrevious().

Why

A consumer asked: "do we see meaningful exceptions on timeout / IdP errors?" Audit found two leaks:

  1. validateClaims() caught ItkOpenIdConnectException and rethrew as the subtype ValidationException with only the message copied — type info and previous chain both lost.
  2. onAuthenticationFailure() threw a fresh AuthenticationException('Error occurred validating openid login') that completely discarded the input exception.

After this PR, a http_client_options.timeout firing inside getIdToken() reaches the application as HttpException (or transitively as CodeException wrapping the Guzzle exception, with getPrevious() populated), not as a generic ValidationException with no context.

Backward compatibility

Additive only. All library exceptions extend ItkOpenIdConnectException, which the method's @throws already declared — anything catching the parent keeps working. Callers that catch only the narrowed ValidationException will 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 errors
  • task lint — clean
  • task test:matrix — all 6 PHP × deps combinations green

Companion PR

Companion library PR: itk-dev/openid-connect#37 — chains previous consistently in the library's catch blocks (4 of 5 were dropping the cause) and tightens @throws phpdoc on public methods.

🤖 Generated with Claude Code

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-commenter
Copy link
Copy Markdown

codecov-commenter commented May 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (b5cd84d) to head (d60e0f9).
⚠️ Report is 3 commits behind head on feature/http-client-options.

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     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@turegjorup turegjorup self-assigned this May 7, 2026
@turegjorup turegjorup requested a review from jekuaitk May 7, 2026 09:03
Comment thread tests/Security/OpenIdLoginAuthenticatorTest.php Outdated
Comment thread tests/Security/OpenIdLoginAuthenticatorTest.php Outdated
Comment thread tests/Security/OpenIdLoginAuthenticatorTest.php Outdated
Comment thread tests/Security/OpenIdLoginAuthenticatorTest.php Outdated
turegjorup and others added 4 commits May 8, 2026 11:33
`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>
@turegjorup turegjorup requested a review from jekuaitk May 8, 2026 09:37
@turegjorup turegjorup deleted the branch feature/http-client-options May 8, 2026 13:16
@turegjorup turegjorup closed this May 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants