5.0.1: distinct WebAuthn re-auth HTTP statuses (#8) + review doc notes (#5, #6)

[devondragon]

Closes out the deferred review notes so 5.0.1 ships clean.

#8 — WebAuthn credential-change re-auth: finer HTTP statuses

requireCurrentPasswordIfSet previously returned 400 for missing field, wrong password, and locked account alike — clients couldn't distinguish them. Now:

Case	Before	After
Missing/blank currentPassword	400	400 (unchanged — client error)
Incorrect currentPassword	400	401 Unauthorized
Account locked	400	423 Locked

• New WebAuthnReauthenticationException (401) + WebAuthnAccountLockedException (423), both extending WebAuthnException (base stays 400).
• Brute-force lockout wiring unchanged. Endpoints are new in 5.0.0, so blast radius is minimal; documented in CHANGELOG/MIGRATION.

Doc notes

• Redundant configmethod in WebSecurityConfig #5: document the @EntityGraph two-level Cartesian-join trade-off on findWithRolesByEmail.
• Issue 5 redundant configmethod in web security config #6: note that an eagerly-loaded token.getUser() still has LAZY roles (use findWithRolesByEmail for authorities). token.user stays EAGER by design (kept consistent with the Role.privileges decision).

Not changed (verified already-fine)

RolePrivilegeSetupService warn log (already mentions multi-node startup), globalErrorKey null path (already null-safe), OnRegistrationCompleteEvent super() (correct defensive code).

Test Plan

• ./gradlew clean test green.
• New WebAuthnManagementAPIAdviceTest asserts 400/401/423/404 mapping; API unit tests tightened to the new subtypes; integration "incorrect" assertions now expect 401.

[claude]

Code Review — PR #321: 5.0.1 WebAuthn HTTP status refinement

Note: This PR is already merged; this review is retrospective and intended as notes for future reference.

---

Overview

This patch does two things cleanly:

1. Splits the single WebAuthnException (400) thrown by requireCurrentPasswordIfSet into three semantically distinct exceptions (400 / 401 / 423).
2. Adds inline documentation on two important design trade-offs: the @EntityGraph Cartesian-join behavior and the eager-user / lazy-roles relationship on token entities.

Both are focused, low-blast-radius changes. The core auth logic is untouched.

---

Code Quality and Style ✅

• Exception hierarchy is correct. Both WebAuthnAccountLockedException and WebAuthnReauthenticationException extend WebAuthnException, so any catch-all catch (WebAuthnException e) in downstream code still works as expected — a good backwards-compatible widening.
• @ExceptionHandler dispatch is correct. Spring MVC resolves @ExceptionHandler methods by class hierarchy (most-specific wins), not by declaration order, so the placement of the new handlers above the base handleWebAuthnError is fine. This isn't obvious from reading the file, so a brief comment there could help future contributors avoid accidentally moving handlers around.
• requireCurrentPasswordIfSet logic order is sound: locked-check → blank-check → wrong-password-check. Locking is checked before the password is even evaluated, which prevents an attacker from learning the lock state via a different response code on a wrong-password guess when already locked.
• Comments in requireCurrentPasswordIfSet are the right kind — they explain the semantic intent (why this is HTTP 400 vs. 401 vs. 423) which is not derivable from the code alone.
• Inline comments on token entities (PasswordResetToken, VerificationToken) are appropriately brief and flag the exact footgun: EAGER user / LAZY roles on the loaded User.

---

Potential Issue: Integration test gap for HTTP 423 ⚠️

WebAuthnManagementAPIAdviceTest (new, pure unit test) and WebAuthnManagementAPITest (unit, mocked) both cover the 423 path. However, WebAuthnFeatureEnabledIntegrationTest was updated only for the 401 case — there is no integration-level test that drives LoginAttemptService to a locked state and then calls one of the three endpoints to confirm a 423 comes back end-to-end. The unit coverage is reasonable, but an integration path through the full Spring filter chain would give higher confidence that the handler wiring holds under load (e.g., if a future security filter intercepts 401 before the advice does).

Suggestion (non-blocking): Add an integration test that pre-locks an account via repeated failed calls and then asserts 423 on the credential-management endpoint.

---

Security Considerations ✅

• Information disclosure: Returning 423 (locked) vs. 401 (wrong password) does confirm to an attacker that the account exists and is locked. This is pre-existing by design — the lockout mechanism already has to expose this to the legitimate user so they know to wait. No regression here.
• 401 in a session context: The 401 response does not trigger a Spring Security re-auth redirect (there's no WWW-Authenticate header), which is correct — this is a JSON API endpoint signalling semantic failure, not a filter-level challenge. No issue.
• Lockout counter correctness is preserved: loginAttemptService.loginFailed() is still called on wrong password, and loginSucceeded() still clears it on correct password. The new exception types don't affect this accounting.

---

Documentation ✅

• CHANGELOG entry is accurate and includes the full set of affected endpoints.
• MIGRATION.md covers the 5.0.0 → 5.0.1 behavioural delta clearly, with the important callout that clients treating any 4xx as "failure" need no changes.
• UserRepository JavaDoc addition is the right place for the Cartesian-product warning — it's on the method that users will call and where the trade-off bites.

---

Minor Observations (non-blocking)

• The serialVersionUID = 1L on both new exception classes is conventional and fine.
• WebAuthnManagementAPIAdviceTest is a plain instantiation test (no Spring context), which is the right call — it tests the status mapping in isolation and stays fast.
• The test naming follows the project's should[ExpectedBehavior]When[Condition] convention in the integration/unit tests but uses @DisplayName strings in the advice unit test. Either style is fine; worth picking one convention for new test classes.

---

Summary

Clean patch. The exception hierarchy, handler dispatch, logic ordering, and documentation are all correct. The only gap worth tracking is integration-level coverage of the 423 (account locked) path through the full Spring context. Everything else is in good shape.

[Copilot]

Pull request overview

This PR refines WebAuthn credential-management “re-authentication required” failure handling so clients can distinguish malformed requests vs authentication failures vs lockout, and it closes deferred documentation notes for 5.0.1.

Changes:

• Introduces WebAuthnReauthenticationException (401) and WebAuthnAccountLockedException (423) and maps them via WebAuthnManagementAPIAdvice.
• Updates WebAuthn credential-management unit/integration tests to expect 401 for incorrect currentPassword and adds advice mapping unit tests.
• Documents @EntityGraph join trade-offs and clarifies token getUser() eager-load vs lazy roles; updates MIGRATION/CHANGELOG for the new HTTP status behavior.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java	Throws distinct exception subtypes for locked vs wrong-password vs missing-field in requireCurrentPasswordIfSet.
src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIAdvice.java	Adds exception handlers mapping new subtypes to 423/401 while keeping base WebAuthnException at 400.
src/main/java/com/digitalsanctuary/spring/user/exceptions/WebAuthnAccountLockedException.java	New exception subtype representing lockout (intended 423 mapping).
src/main/java/com/digitalsanctuary/spring/user/exceptions/WebAuthnReauthenticationException.java	New exception subtype representing incorrect current password (intended 401 mapping).
src/main/java/com/digitalsanctuary/spring/user/persistence/model/PasswordResetToken.java	Adds note clarifying eager user loading vs lazy roles for authorities.
src/main/java/com/digitalsanctuary/spring/user/persistence/model/VerificationToken.java	Adds note clarifying eager user loading vs lazy roles for authorities.
src/main/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepository.java	Documents entity-graph join/cross-product implications for findWithRolesByEmail.
src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnFeatureEnabledIntegrationTest.java	Updates integration assertions for incorrect current password to expect 401.
src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIAdviceTest.java	Adds unit tests for the intended status mapping per exception subtype.
src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java	Updates unit tests to assert the new exception subtypes are thrown for locked/wrong-password cases.
MIGRATION.md	Updates re-auth section to describe refined 400/401/423 behavior.
CHANGELOG.md	Adds 5.0.1 entry describing distinct HTTP statuses for WebAuthn re-auth failures.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.