diff --git a/CHANGELOG.md b/CHANGELOG.md index 790328eb..013ef85c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ All notable changes to this project are documented here. This project follows [S ## [5.0.1] - Unreleased ### Changed - Reverted `Role.privileges` to `FetchType.EAGER` (it was changed to `LAZY` in 5.0.0). The 5.0.0 change made `role.getPrivileges()` throw `LazyInitializationException` when accessed outside an open transaction/session — a surprising footgun for consumers — in exchange for a negligible gain, since privileges are small, static reference data with no bulk-load path. `User.roles` remains `LAZY` (that is where the real N+1 win is), and the authentication path still loads the full `User → roles → privileges` graph in a single round trip via `UserRepository.findWithRolesByEmail`. This is a **non-breaking relaxation**: code written against 5.0.0 continues to work unchanged. See `MIGRATION.md` ("Lazy fetching of roles and privileges"). +- WebAuthn credential-management re-authentication failures now use distinct HTTP statuses so clients can tell them apart: an incorrect current password returns **401 Unauthorized** and a locked account returns **423 Locked** (both previously **400**). A missing/blank `currentPassword` field remains **400 Bad Request**. Affects `DELETE /user/webauthn/password`, `DELETE /user/webauthn/credentials/{id}`, and `PUT /user/webauthn/credentials/{id}/label`. These endpoints are new in 5.0.0; brute-force lockout behaviour is unchanged. ### Fixed - `user.security.trustedHosts` matching is now case-insensitive (RFC 4343). A mixed-case configured entry or forwarded host (e.g. `App.Example.Com`) now matches `app.example.com`; previously a case mismatch caused the resolver to ignore a legitimately trusted `X-Forwarded-Host` and fall back to the container's server name on the CWE-640 path. diff --git a/MIGRATION.md b/MIGRATION.md index 9dc46c89..06f2890e 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -128,11 +128,14 @@ Affected endpoints (all require `user.webauthn.enabled=true` except where noted) | `/user/webauthn/credentials/{id}` | `DELETE` | Deleting a passkey requires the current password **when the account has a password**. | JSON body `{"currentPassword": "..."}` | | `/user/webauthn/credentials/{id}/label` | `PUT` | Renaming a passkey requires the current password **when the account has a password**. The existing body gains a `currentPassword` field. | JSON body `{"label": "...", "currentPassword": "..."}` | -**Behavior when the account has a password:** -- Missing `currentPassword` → `HTTP 400` with message *"Current password is required to change authentication methods."* — nothing is mutated. -- Incorrect `currentPassword` → `HTTP 400` with message *"Current password is incorrect."* — nothing is mutated. +**Behavior when the account has a password** (status codes refined in **5.0.1** — see note below): +- Missing/blank `currentPassword` → `HTTP 400 Bad Request`, message *"Current password is required to change authentication methods."* — nothing is mutated. +- Incorrect `currentPassword` → `HTTP 401 Unauthorized`, message *"Current password is incorrect."* — nothing is mutated. +- Account locked (too many failed attempts) → `HTTP 423 Locked`, message *"Account is locked due to too many failed attempts…"* — nothing is mutated. - Correct `currentPassword` → the operation proceeds as before. +> **5.0.0 → 5.0.1 note:** In 5.0.0 all three failure cases returned `HTTP 400`. As of 5.0.1 they return distinct statuses (400 missing / 401 incorrect / 423 locked) so clients can tell them apart. If you wrote a client against 5.0.0 that treats any `4xx` as "re-auth failed", no change is needed; only update it if you branched specifically on `400`. + `/user/updatePassword` is unchanged: it already required and verified `oldPassword`. **Action required:** Update any client that calls the three endpoints above so that it collects the user's current password and sends it in the request body. `DELETE /user/webauthn/credentials/{id}` and `DELETE /user/webauthn/password`, which previously had no request body, now accept (and for password-holding accounts require) a JSON body carrying `currentPassword`. Existing IDOR/ownership checks and last-credential lockout protection are unchanged. diff --git a/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java b/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java index 99b4e03a..1e3565e7 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java +++ b/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java @@ -16,7 +16,9 @@ import org.springframework.web.bind.annotation.RestController; import com.digitalsanctuary.spring.user.audit.AuditEvent; import com.digitalsanctuary.spring.user.dto.WebAuthnCredentialInfo; +import com.digitalsanctuary.spring.user.exceptions.WebAuthnAccountLockedException; import com.digitalsanctuary.spring.user.exceptions.WebAuthnException; +import com.digitalsanctuary.spring.user.exceptions.WebAuthnReauthenticationException; import com.digitalsanctuary.spring.user.exceptions.WebAuthnUserNotFoundException; import com.digitalsanctuary.spring.user.persistence.model.User; import com.digitalsanctuary.spring.user.service.LoginAttemptService; @@ -228,10 +230,12 @@ private User findAuthenticatedUser(UserDetails userDetails) throws WebAuthnUserN * *

* If the account has a password, the supplied {@code currentPassword} must be present and valid (verified via - * {@link UserService#checkIfValidOldPassword(User, String)}); otherwise a {@link WebAuthnException} (HTTP 400) is - * thrown before any mutation, preventing a session-only actor from changing the account's authentication - * methods. If the account is passwordless (passkey-only) there is no current credential to verify, so this check is - * a no-op — see the residual-risk note in MIGRATION.md. + * {@link UserService#checkIfValidOldPassword(User, String)}) before any mutation, preventing a session-only + * actor from changing the account's authentication methods. The failure modes map to distinct HTTP statuses so + * clients can tell them apart: a missing/blank field → {@link WebAuthnException} (HTTP 400), an incorrect + * password → {@link WebAuthnReauthenticationException} (HTTP 401), and a locked account → + * {@link WebAuthnAccountLockedException} (HTTP 423). If the account is passwordless (passkey-only) there is no current + * credential to verify, so this check is a no-op — see the residual-risk note in MIGRATION.md. *

* *

@@ -244,7 +248,9 @@ private User findAuthenticatedUser(UserDetails userDetails) throws WebAuthnUserN * * @param user the authenticated user * @param currentPassword the current password supplied by the client (may be {@code null}) - * @throws WebAuthnException if the account is locked, or has a password and the supplied current password is missing or incorrect + * @throws WebAuthnAccountLockedException if the account is locked (HTTP 423) + * @throws WebAuthnReauthenticationException if the account has a password and the supplied current password is incorrect (HTTP 401) + * @throws WebAuthnException if the account has a password and the current password is missing/blank (HTTP 400) */ private void requireCurrentPasswordIfSet(User user, String currentPassword) { if (!userService.hasPassword(user)) { @@ -252,15 +258,17 @@ private void requireCurrentPasswordIfSet(User user, String currentPassword) { return; } if (loginAttemptService.isLocked(user.getEmail())) { - throw new WebAuthnException("Account is locked due to too many failed attempts. Please try again later."); + // Locked account -> HTTP 423, distinct from a wrong password (401) or a missing field (400). + throw new WebAuthnAccountLockedException("Account is locked due to too many failed attempts. Please try again later."); } if (currentPassword == null || currentPassword.isBlank()) { - // A missing field is a client error, not a password guess, so it does not count toward lockout. + // A missing field is a client error (HTTP 400), not a password guess, so it does not count toward lockout. throw new WebAuthnException("Current password is required to change authentication methods."); } if (!userService.checkIfValidOldPassword(user, currentPassword)) { loginAttemptService.loginFailed(user.getEmail()); - throw new WebAuthnException("Current password is incorrect."); + // A wrong password is an authentication failure -> HTTP 401. + throw new WebAuthnReauthenticationException("Current password is incorrect."); } // Successful re-authentication clears the failed-attempt counter, matching login semantics. loginAttemptService.loginSucceeded(user.getEmail()); diff --git a/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIAdvice.java b/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIAdvice.java index 5c16304d..6c538b87 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIAdvice.java +++ b/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIAdvice.java @@ -6,7 +6,9 @@ import org.springframework.web.bind.MethodArgumentNotValidException; import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.RestControllerAdvice; +import com.digitalsanctuary.spring.user.exceptions.WebAuthnAccountLockedException; import com.digitalsanctuary.spring.user.exceptions.WebAuthnException; +import com.digitalsanctuary.spring.user.exceptions.WebAuthnReauthenticationException; import com.digitalsanctuary.spring.user.exceptions.WebAuthnUserNotFoundException; import com.digitalsanctuary.spring.user.util.GenericResponse; import jakarta.validation.ConstraintViolationException; @@ -26,6 +28,18 @@ public ResponseEntity handleUserNotFound(WebAuthnUserNotFoundEx return ResponseEntity.status(HttpStatus.NOT_FOUND).body(new GenericResponse(ex.getMessage())); } + @ExceptionHandler(WebAuthnAccountLockedException.class) + public ResponseEntity handleAccountLocked(WebAuthnAccountLockedException ex) { + log.warn("WebAuthn account locked: {}", ex.getMessage()); + return ResponseEntity.status(HttpStatus.LOCKED).body(new GenericResponse(ex.getMessage())); + } + + @ExceptionHandler(WebAuthnReauthenticationException.class) + public ResponseEntity handleReauthenticationFailure(WebAuthnReauthenticationException ex) { + log.warn("WebAuthn re-authentication failure: {}", ex.getMessage()); + return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body(new GenericResponse(ex.getMessage())); + } + @ExceptionHandler(WebAuthnException.class) public ResponseEntity handleWebAuthnError(WebAuthnException ex) { log.warn("WebAuthn error: {}", ex.getMessage()); diff --git a/src/main/java/com/digitalsanctuary/spring/user/exceptions/WebAuthnAccountLockedException.java b/src/main/java/com/digitalsanctuary/spring/user/exceptions/WebAuthnAccountLockedException.java new file mode 100644 index 00000000..22502d2c --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/exceptions/WebAuthnAccountLockedException.java @@ -0,0 +1,21 @@ +package com.digitalsanctuary.spring.user.exceptions; + +/** + * Thrown when a credential-altering WebAuthn operation is rejected because the account is currently locked due to too + * many failed authentication attempts. Maps to HTTP 423 (Locked) so clients can distinguish a temporary lockout from a + * wrong password (→ 401) or a malformed request (→ 400). + */ +public class WebAuthnAccountLockedException extends WebAuthnException { + + /** Serial Version UID. */ + private static final long serialVersionUID = 1L; + + /** + * Instantiates a new WebAuthn account-locked exception. + * + * @param message the message + */ + public WebAuthnAccountLockedException(final String message) { + super(message); + } +} diff --git a/src/main/java/com/digitalsanctuary/spring/user/exceptions/WebAuthnReauthenticationException.java b/src/main/java/com/digitalsanctuary/spring/user/exceptions/WebAuthnReauthenticationException.java new file mode 100644 index 00000000..9774c30c --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/exceptions/WebAuthnReauthenticationException.java @@ -0,0 +1,22 @@ +package com.digitalsanctuary.spring.user.exceptions; + +/** + * Thrown when re-authentication for a credential-altering WebAuthn operation fails because the supplied current password + * is incorrect. Maps to HTTP 401 (Unauthorized) so clients can distinguish a wrong password from a malformed request + * (missing field → 400) or a locked account (→ 423). A missing/blank current-password field is a client error + * and remains a plain {@link WebAuthnException} (400). + */ +public class WebAuthnReauthenticationException extends WebAuthnException { + + /** Serial Version UID. */ + private static final long serialVersionUID = 1L; + + /** + * Instantiates a new WebAuthn re-authentication exception. + * + * @param message the message + */ + public WebAuthnReauthenticationException(final String message) { + super(message); + } +} diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/PasswordResetToken.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/PasswordResetToken.java index 4060ce63..578ae74e 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/PasswordResetToken.java +++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/PasswordResetToken.java @@ -42,6 +42,7 @@ public class PasswordResetToken { private String token; /** The user. */ + // EAGER so getUser() works on a detached token; note the loaded User's roles are still LAZY — use UserRepository.findWithRolesByEmail if you need authorities. @ToString.Exclude @OneToOne(targetEntity = User.class, fetch = FetchType.EAGER) @JoinColumn(nullable = false, name = "user_id") diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/VerificationToken.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/VerificationToken.java index 134298c4..42a8d4a3 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/VerificationToken.java +++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/VerificationToken.java @@ -44,6 +44,7 @@ public class VerificationToken { private String token; /** The user. */ + // EAGER so getUser() works on a detached token; note the loaded User's roles are still LAZY — use UserRepository.findWithRolesByEmail if you need authorities. @ToString.Exclude @OneToOne(targetEntity = User.class, fetch = FetchType.EAGER) @JoinColumn(nullable = false, name = "user_id", foreignKey = @ForeignKey(name = "FK_VERIFY_USER")) diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepository.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepository.java index 51bc07c9..450656ae 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepository.java +++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepository.java @@ -34,6 +34,10 @@ public interface UserRepository extends JpaRepository { * {@link #findByEmail(String)} remains for callers (token lookups, existence checks, lockout counters) that do not * need the authority graph.

* + *

Implementation note: the two-level graph is a single JOIN FETCH, so the result set is a Cartesian product of + * roles × privileges (Hibernate de-duplicates via the {@code Set} mappings). This is fine for the small, + * bounded role/privilege graphs of a single user; it is not intended for bulk loading many users at once.

+ * * @param email the email * @return the user with roles and privileges initialized, or {@code null} if none found */ diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnFeatureEnabledIntegrationTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnFeatureEnabledIntegrationTest.java index bfe33ec1..99ea55ca 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnFeatureEnabledIntegrationTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnFeatureEnabledIntegrationTest.java @@ -150,7 +150,7 @@ void shouldRejectDeleteWhenCurrentPasswordMissingForPasswordAccount() throws Exc void shouldRejectDeleteWhenCurrentPasswordIncorrectForPasswordAccount() throws Exception { String payload = "{\"currentPassword\":\"wrongPassword!\"}"; mockMvc.perform(delete("/user/webauthn/credentials/cred-1").with(user(TEST_EMAIL).roles("USER")).with(csrf()) - .contentType(MediaType.APPLICATION_JSON).content(payload)).andExpect(status().isBadRequest()) + .contentType(MediaType.APPLICATION_JSON).content(payload)).andExpect(status().isUnauthorized()) .andExpect(jsonPath("$.message", containsString("Current password is incorrect"))); assertThat(webAuthnCredentialRepository.findByIdWithUser("cred-1")).isPresent(); @@ -174,7 +174,7 @@ void shouldRejectRenameWhenCurrentPasswordMissingForPasswordAccount() throws Exc void shouldRejectRenameWhenCurrentPasswordIncorrectForPasswordAccount() throws Exception { String payload = "{\"label\":\"New Label\",\"currentPassword\":\"wrongPassword!\"}"; mockMvc.perform(put("/user/webauthn/credentials/cred-1/label").with(user(TEST_EMAIL).roles("USER")).with(csrf()) - .contentType(MediaType.APPLICATION_JSON).content(payload)).andExpect(status().isBadRequest()) + .contentType(MediaType.APPLICATION_JSON).content(payload)).andExpect(status().isUnauthorized()) .andExpect(jsonPath("$.message", containsString("Current password is incorrect"))); assertThat(webAuthnCredentialRepository.findByIdWithUser("cred-1")) diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIAdviceTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIAdviceTest.java new file mode 100644 index 00000000..e87b5856 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIAdviceTest.java @@ -0,0 +1,52 @@ +package com.digitalsanctuary.spring.user.api; + +import static org.assertj.core.api.Assertions.assertThat; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import com.digitalsanctuary.spring.user.exceptions.WebAuthnAccountLockedException; +import com.digitalsanctuary.spring.user.exceptions.WebAuthnException; +import com.digitalsanctuary.spring.user.exceptions.WebAuthnReauthenticationException; +import com.digitalsanctuary.spring.user.exceptions.WebAuthnUserNotFoundException; +import com.digitalsanctuary.spring.user.util.GenericResponse; + +/** + * Unit tests for {@link WebAuthnManagementAPIAdvice}, verifying that each WebAuthn exception type maps to the intended + * HTTP status so credential-management clients can distinguish the failure modes (missing field, wrong password, locked + * account, unknown user). + */ +@DisplayName("WebAuthnManagementAPIAdvice status mapping") +class WebAuthnManagementAPIAdviceTest { + + private final WebAuthnManagementAPIAdvice advice = new WebAuthnManagementAPIAdvice(); + + @Test + @DisplayName("base WebAuthnException (e.g. missing current password) -> 400 Bad Request") + void baseExceptionMapsToBadRequest() { + ResponseEntity response = advice.handleWebAuthnError(new WebAuthnException("Current password is required")); + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.BAD_REQUEST); + } + + @Test + @DisplayName("incorrect current password -> 401 Unauthorized") + void reauthenticationFailureMapsToUnauthorized() { + ResponseEntity response = + advice.handleReauthenticationFailure(new WebAuthnReauthenticationException("Current password is incorrect.")); + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.UNAUTHORIZED); + } + + @Test + @DisplayName("locked account -> 423 Locked") + void accountLockedMapsToLocked() { + ResponseEntity response = advice.handleAccountLocked(new WebAuthnAccountLockedException("Account is locked")); + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.LOCKED); + } + + @Test + @DisplayName("unknown user -> 404 Not Found") + void userNotFoundMapsToNotFound() { + ResponseEntity response = advice.handleUserNotFound(new WebAuthnUserNotFoundException("User not found")); + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.NOT_FOUND); + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java index f669cff7..dc91f61d 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java @@ -24,7 +24,9 @@ import org.springframework.security.core.userdetails.UserDetails; import com.digitalsanctuary.spring.user.audit.AuditEvent; import com.digitalsanctuary.spring.user.dto.WebAuthnCredentialInfo; +import com.digitalsanctuary.spring.user.exceptions.WebAuthnAccountLockedException; import com.digitalsanctuary.spring.user.exceptions.WebAuthnException; +import com.digitalsanctuary.spring.user.exceptions.WebAuthnReauthenticationException; import com.digitalsanctuary.spring.user.exceptions.WebAuthnUserNotFoundException; import com.digitalsanctuary.spring.user.persistence.model.User; import com.digitalsanctuary.spring.user.service.LoginAttemptService; @@ -213,7 +215,7 @@ void shouldRejectRenameWhenAccountLocked() { new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop", "currentPass"); // When & Then - locked accounts are rejected before the password is even checked. - assertThatThrownBy(() -> api.renameCredential("cred-1", request, userDetails)).isInstanceOf(WebAuthnException.class) + assertThatThrownBy(() -> api.renameCredential("cred-1", request, userDetails)).isInstanceOf(WebAuthnAccountLockedException.class) .hasMessageContaining("locked"); verify(credentialManagementService, never()).renameCredential(any(), any(), any()); verify(userService, never()).checkIfValidOldPassword(any(), any()); @@ -246,7 +248,7 @@ void shouldRejectRenameWhenCurrentPasswordIncorrect() { new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop", "wrongPass"); // When & Then - assertThatThrownBy(() -> api.renameCredential("cred-1", request, userDetails)).isInstanceOf(WebAuthnException.class) + assertThatThrownBy(() -> api.renameCredential("cred-1", request, userDetails)).isInstanceOf(WebAuthnReauthenticationException.class) .hasMessageContaining("Current password is incorrect"); verify(credentialManagementService, never()).renameCredential(any(), any(), any()); // A wrong password is reported to the lockout service so repeated guesses eventually lock the account. @@ -342,7 +344,7 @@ void shouldRejectDeleteWhenCurrentPasswordIncorrect() { WebAuthnManagementAPI.CurrentPasswordRequest request = new WebAuthnManagementAPI.CurrentPasswordRequest("wrongPass"); // When & Then - assertThatThrownBy(() -> api.deleteCredential("cred-1", request, userDetails)).isInstanceOf(WebAuthnException.class) + assertThatThrownBy(() -> api.deleteCredential("cred-1", request, userDetails)).isInstanceOf(WebAuthnReauthenticationException.class) .hasMessageContaining("Current password is incorrect"); verify(credentialManagementService, never()).deleteCredential(any(), any()); } @@ -434,7 +436,7 @@ void shouldRejectRemovalWhenCurrentPasswordIncorrect() { // When & Then assertThatThrownBy(() -> api.removePassword(request, userDetails, mockRequest)) - .isInstanceOf(WebAuthnException.class) + .isInstanceOf(WebAuthnReauthenticationException.class) .hasMessageContaining("Current password is incorrect"); verify(userService, never()).removeUserPassword(any()); }