diff --git a/CHANGELOG.md b/CHANGELOG.md index 848ac9a1..c98e21a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,26 +4,30 @@ All notable changes to this project are documented here. This project follows [S ## [Unreleased] -Security hardening from the SUF review series (SUF-01, SUF-03, SUF-04) plus a documentation correction (SUF-06). All changes are backward compatible; the one new property is opt-in and defaults to the pre-existing behavior. +Security hardening from the SUF review series (SUF-01 through SUF-06). Most changes are backward compatible. The one notable behavior change: `POST /user/setPassword` is now **disabled by default** (SUF-02) — see **Behavior changes** below. ### Security - **SUF-01 — Host-header link poisoning (CWE-640): the ordinary request host is now allow-listed, not just `X-Forwarded-Host`.** When `user.security.trustedHosts` is configured, the finally-chosen host for a password-reset / verification link — including the ordinary request server name, which on common servlet containers is derived from the attacker-influenceable `Host` header — must be in the allow-list. A non-allow-listed host now falls back to the first configured trusted host (a known-good canonical authority) instead of being emitted into the link. Matching on this ordinary-host path is case-insensitive (RFC 4343; previously only the forwarded-host path was), and blank/whitespace `trustedHosts` entries are ignored. - New opt-in property **`user.security.requireCanonicalAppUrl`** (default `false`): when `true`, application startup fails unless `user.security.appUrl` or a non-empty `user.security.trustedHosts` is configured — a hard guarantee that email links can never derive their authority from a spoofable `Host` header. The default stays `false` in this release (a startup warning is logged instead), and is **planned to become the default in a future major version**. Setting `user.security.appUrl` (recommended) or `trustedHosts` in production is advised regardless of this flag. +- **SUF-02 — Step-up required to set an initial password on a passwordless account.** `POST /user/setPassword` adds a durable password to a passwordless (passkey-only) account, but previously required only an authenticated session. A new SPI, **`StepUpService`**, lets a consuming application require a fresh proof of presence/possession (WebAuthn assertion, TOTP, recent-auth) before the operation. If a `StepUpService` bean is present it is required (failure → `HTTP 401`); if none is present the endpoint is **disabled by default** (`HTTP 403`). Set **`user.security.allowInitialPasswordSetWithoutStepUp=true`** to keep the previous session-only behavior. (The full WebAuthn step-up primitive is tracked separately; this ships the interim guard.) - **SUF-03 — Revoke every user session on account delete/disable, after the change commits.** Deleting or disabling an account now revokes *all* of that user's active sessions (not just the caller's current request; `DSUserDetails` caches the enabled flag at login and is not re-checked per request). Revocation is deferred until *after* the delete/disable transaction commits, closing a race where a login landing between the session scan and the commit could register a new, surviving session. If the transaction rolls back, sessions are no longer revoked. - **SUF-04 — Authenticated password change participates in brute-force lockout.** `POST /user/updatePassword` now rejects a locked account up front with `HTTP 423 Locked`, reports a wrong current password to `LoginAttemptService` (counting toward lockout, matching the login path), and resets the counter on success. **Passwordless (passkey-only / OAuth-only) accounts** — which have no current password to verify — are rejected up front with `HTTP 400` (the message directs the user to `POST /user/setPassword`) and **never feed the lockout counter**, preventing a session-holding caller from locking such an account out of every authentication method. +- **SUF-05 — Password-reset token no longer leaks via Referer or caches (CWE-598).** The reset flow carries the token in the page URL; the library now sets `Referrer-Policy: no-referrer` and `Cache-Control: no-store` on the reset pages (via a scoped interceptor over the configured reset URIs) so the token is not sent in the `Referer` header or written to shared/browser caches. The token-in-body `POST /user/savePassword` contract is unchanged (non-breaking); residual address-bar/history exposure remains and is documented. ### Fixes -- **SUF-06** — Corrected the documented audit-log rotation default in `CONFIG.md`. +- **SUF-06 (docs)** — Corrected the documented audit-log rotation default in `CONFIG.md` (rotation stays opt-in / `0` by design). +- **SUF-06 (code hardening)** — `showChangePasswordPage` no longer mints an `HttpSession` for the anonymous token-validation request (session amplification), and audits an invalid token as `Failure` rather than `Success`. Audit-log rotation intentionally remains disabled by default (rotated archives are excluded from GDPR/audit queries); rate-limiting anonymous endpoints remains delegated to Bucket4J. ### Behavior changes (client impact) +- **`POST /user/setPassword` is disabled by default (SUF-02).** It returns `HTTP 403` unless a `StepUpService` bean is provided (then step-up is required; failures return `HTTP 401`), or `user.security.allowInitialPasswordSetWithoutStepUp=true` is set to restore the previous session-only behavior. Consumers whose passwordless users set an initial password must choose one of those options. - `POST /user/updatePassword` can now return `HTTP 423 Locked` (account locked) in addition to its existing `200`/`400`. Clients that treat any non-`200` as a generic failure need no change; clients that branch on status can surface the locked state. A passwordless account calling this endpoint receives `400` with a "no password set" message and is directed to `POST /user/setPassword`. ### Documentation -- `CONFIG.md`: documented `user.security.requireCanonicalAppUrl` and the ordinary-host allow-list behavior; corrected the account-lockout property prefixes to `user.security.*`; noted that `/user/updatePassword` participates in lockout. -- `MIGRATION.md`: the credential-changes section no longer states `/user/updatePassword` is "unchanged" (it now enforces lockout and rejects passwordless accounts); documented the `requireCanonicalAppUrl` opt-in and corrected the trusted-host fallback description. +- `CONFIG.md`: documented `user.security.requireCanonicalAppUrl`, the ordinary-host allow-list behavior, and `user.security.allowInitialPasswordSetWithoutStepUp` / the `StepUpService` SPI; corrected the account-lockout property prefixes to `user.security.*`; noted that `/user/updatePassword` participates in lockout. +- `MIGRATION.md`: the credential-changes section no longer states `/user/updatePassword` is "unchanged"; documented the `setPassword` step-up requirement / default-disabled behavior and the opt-in flag; documented the `requireCanonicalAppUrl` opt-in and corrected the trusted-host fallback description. ### Testing -- Added unit and full-context integration coverage: passwordless `updatePassword` rejection without touching the lockout counter; end-to-end lockout through a real Spring context; session revocation deferred to after commit (delete and disable paths); `AppUrlResolver` ordinary-host allow-list (blank-entry filtering, case-insensitive matching, IPv6 literal); and `user.security.requireCanonicalAppUrl` through real `@Value` binding (`CoreBeanOverrideTest`) plus strict-mode tests that assert the resolver actually uses the configured values. +- Added unit and full-context coverage: passwordless `updatePassword` rejection without touching the lockout counter; end-to-end lockout through a real Spring context; session revocation deferred to after commit (delete and disable paths); `AppUrlResolver` ordinary-host allow-list (blank-entry filtering, case-insensitive matching, IPv6 literal); `user.security.requireCanonicalAppUrl` through real `@Value` binding plus strict-mode assertions; `setPassword` step-up guard (disabled-by-default, opt-in flag, `StepUpService` grant/deny); reset-page security headers; and the anonymous-session / audit-status hygiene on `showChangePasswordPage`. ## [5.0.1] - 2026-06-15 ### Features diff --git a/CONFIG.md b/CONFIG.md index 5e32cbfa..c149e23c 100644 --- a/CONFIG.md +++ b/CONFIG.md @@ -92,6 +92,13 @@ Password-reset and verification emails contain a link back to your application. When neither `appUrl` nor `trustedHosts` is set, links are built from the request host (backward-compatible behavior) and a startup warning is logged. +### Passwordless Initial Password Step-Up (SUF-02) + +`POST /user/setPassword` adds an *initial* password to a passwordless (passkey-only) account. Because there is no current credential to verify, the endpoint is gated: + +- **Step-Up Service (`com.digitalsanctuary.spring.user.security.StepUpService` SPI)**: If your application provides a `StepUpService` bean, it is **required** — `setPassword` proceeds only when `isStepUpSatisfied(user, "set-password", request)` returns `true`; otherwise it returns `HTTP 401`. Implement it to require a fresh WebAuthn assertion, TOTP, or recent-auth proof. +- **Allow Without Step-Up (`user.security.allowInitialPasswordSetWithoutStepUp`)**: When no `StepUpService` bean is present, `setPassword` is **disabled** (`HTTP 403`) unless this is `true`, which restores the previous session-only behavior. Default: `false`. + ### Token Security Verification and password-reset tokens are **hashed at rest**. The raw token is only ever sent to the user in the emailed link; the database stores its hash. Lookups hash the incoming token and match by hash, with a transparent fallback to plaintext lookup so that any links issued before upgrading keep working until they expire. This requires no schema migration and no action from consuming applications. diff --git a/MIGRATION.md b/MIGRATION.md index 330a51d6..c0949768 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -150,9 +150,16 @@ Affected endpoints (all require `user.webauthn.enabled=true` except where noted) **Passwordless (passkey-only) accounts — residual risk:** For accounts with no password set, there is no current credential to verify, and this library does not yet implement a WebAuthn step-up assertion (a feasible recent-authentication signal does not currently exist in the framework). As a result: - Deleting or renaming a passkey on a passwordless account remains a session-only operation (last-credential lockout protection and ownership checks still apply). -- Setting an *initial* password via `POST /user/setPassword` on a passwordless account also cannot require a current password (there is none); this endpoint still rejects accounts that already have a password. +- Setting an *initial* password via `POST /user/setPassword` cannot require a current password (there is none). **As of the SUF-02 hardening this endpoint is disabled by default** — it returns `HTTP 403` unless you either provide a step-up service (below) or explicitly opt into the previous behavior. It still rejects accounts that already have a password. -This is a deliberate, documented limitation rather than a half-measure: implementing a true WebAuthn step-up assertion would require significant new challenge/response infrastructure. Consuming applications that need stronger guarantees for passwordless accounts can front these endpoints with their own step-up (e.g. require a fresh passkey assertion) before allowing the call. This will be revisited if/when a step-up mechanism is added to the framework. +**Step-up SPI and the `setPassword` default (SUF-02).** A new SPI, `com.digitalsanctuary.spring.user.security.StepUpService`, lets you require a fresh proof of presence/possession (WebAuthn assertion, TOTP, recent-auth) before `setPassword` proceeds: + +- If you register a `StepUpService` bean, it is **required**: `setPassword` proceeds only when `isStepUpSatisfied(...)` returns `true`, otherwise it returns `HTTP 401`. +- If no `StepUpService` bean is present, `setPassword` is **disabled** (`HTTP 403`) unless you set `user.security.allowInitialPasswordSetWithoutStepUp=true`, which restores the prior session-only behavior. + +**Action required if your application lets passwordless users set an initial password:** provide a `StepUpService` bean (recommended), or set `user.security.allowInitialPasswordSetWithoutStepUp=true`. Otherwise `POST /user/setPassword` returns `403`. + +Implementing a *full* WebAuthn step-up assertion (challenge/response bound to user/session/action/RP ID/origin/expiry) is tracked as separate feature work; the `StepUpService` SPI is the interim mechanism so applications can enforce their own step-up now. ### Database schema: unique role/privilege names diff --git a/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java b/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java index 893c7908..f722be07 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java +++ b/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java @@ -33,6 +33,7 @@ import com.digitalsanctuary.spring.user.persistence.model.User; import com.digitalsanctuary.spring.user.registration.RegistrationDeniedException; import com.digitalsanctuary.spring.user.registration.RegistrationGuard; +import com.digitalsanctuary.spring.user.security.StepUpService; import com.digitalsanctuary.spring.user.service.DSUserDetails; import com.digitalsanctuary.spring.user.service.LoginAttemptService; import com.digitalsanctuary.spring.user.service.PasswordPolicyService; @@ -92,6 +93,8 @@ public class UserAPI { private final ObjectProvider webAuthnCredentialManagementServiceProvider; private final AppUrlResolver appUrlResolver; private final LoginAttemptService loginAttemptService; + /** Optional consumer-provided step-up (re-)authentication service; see {@link StepUpService} (SUF-02). */ + private final ObjectProvider stepUpServiceProvider; @Value("${user.security.registrationPendingURI}") private String registrationPendingURI; @@ -102,6 +105,14 @@ public class UserAPI { @Value("${user.security.forgotPasswordPendingURI}") private String forgotPasswordPendingURI; + /** + * SUF-02: controls the fallback behavior of {@code /user/setPassword} when no {@link StepUpService} bean is present. + * When {@code false} (the default), setting an initial password on a passwordless account is disabled unless a + * {@link StepUpService} is provided; set to {@code true} to explicitly allow the session-only behavior. + */ + @Value("${user.security.allowInitialPasswordSetWithoutStepUp:false}") + private boolean allowInitialPasswordSetWithoutStepUp; + /** * Registers a new user account. * @@ -519,14 +530,14 @@ public ResponseEntity registerPasswordlessAccount(@Valid @RequestB * This endpoint only applies to passwordless (passkey-only) accounts and rejects the request if a password is already * set (use {@code /user/updatePassword} to change an existing password, which requires the current password). Because * the account has no current password to verify, this credential-altering operation cannot require re-authentication - * via a current password, and this library does not yet implement a WebAuthn step-up assertion. + * via a current password. *

* *

- * Residual risk: a session-only actor on a passwordless account could set an initial password. This is - * a known, documented limitation (see MIGRATION.md "Re-authentication required for credential changes"). It is not a - * regression and is bounded: the new password does not displace any existing credential, and consuming applications can - * front this endpoint with their own step-up if required. + * Step-up (SUF-02): to address the residual risk that a session-only actor could add a durable + * password, this endpoint requires step-up when a {@link StepUpService} bean is provided, and is otherwise + * disabled by default. Set {@code user.security.allowInitialPasswordSetWithoutStepUp=true} to keep + * the previous session-only behavior when no step-up service is available. See MIGRATION.md. *

* * @param userDetails the authenticated user details @@ -549,6 +560,23 @@ public ResponseEntity setPassword(@AuthenticationPrincipal DSUserD return buildErrorResponse("User already has a password. Use the change password feature instead.", 1, HttpStatus.BAD_REQUEST); } + // SUF-02: setting an initial password on a passwordless (passkey-only) account is a credential change with no + // current credential to verify, so a session-only actor could otherwise add a durable password. If a consumer + // supplies a StepUpService, require it to pass; otherwise the endpoint is disabled by default. Set + // user.security.allowInitialPasswordSetWithoutStepUp=true to explicitly keep the session-only behavior. + final StepUpService stepUpService = stepUpServiceProvider.getIfAvailable(); + if (stepUpService != null) { + if (!stepUpService.isStepUpSatisfied(user, "set-password", request)) { + logAuditEvent("SetPassword", "Failure", "Step-up verification failed", user, request); + return buildErrorResponse(messages.getMessage("message.set-password.step-up-required", null, + "Additional verification is required to set a password.", locale), 6, HttpStatus.UNAUTHORIZED); + } + } else if (!allowInitialPasswordSetWithoutStepUp) { + logAuditEvent("SetPassword", "Failure", "Initial password set disabled (no step-up configured)", user, request); + return buildErrorResponse(messages.getMessage("message.set-password.disabled", null, + "Setting an initial password is not enabled on this server.", locale), 6, HttpStatus.FORBIDDEN); + } + if (!setPasswordDto.getNewPassword().equals(setPasswordDto.getConfirmPassword())) { return buildErrorResponse(messages.getMessage("message.password.mismatch", null, "Passwords do not match", locale), 2, HttpStatus.BAD_REQUEST); diff --git a/src/main/java/com/digitalsanctuary/spring/user/controller/UserActionController.java b/src/main/java/com/digitalsanctuary/spring/user/controller/UserActionController.java index f595277f..371bcfda 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/controller/UserActionController.java +++ b/src/main/java/com/digitalsanctuary/spring/user/controller/UserActionController.java @@ -19,6 +19,7 @@ import com.digitalsanctuary.spring.user.util.UserUtils; import com.digitalsanctuary.spring.user.web.IncludeUserInModel; import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpSession; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; @@ -82,13 +83,20 @@ public ModelAndView showChangePasswordPage(final HttpServletRequest request, fin log.debug("UserAPI.showChangePasswordPage: called with token: {}", TokenHasher.fingerprint(token)); final TokenValidationResult result = userService.validatePasswordResetToken(token); log.debug("UserAPI.showChangePasswordPage: result: {}", result); - AuditEvent changePasswordAuditEvent = AuditEvent.builder().source(this).sessionId(request.getSession().getId()) + final boolean valid = TokenValidationResult.VALID.equals(result); + // SUF-06: this is an anonymous, token-validating GET. Do NOT mint an HttpSession just to audit it (a stream of + // anonymous invalid-token probes would otherwise accumulate server-side session state); reuse an existing + // session id only if the caller already has one. + final HttpSession existingSession = request.getSession(false); + final String sessionId = existingSession != null ? existingSession.getId() : null; + AuditEvent changePasswordAuditEvent = AuditEvent.builder().source(this).sessionId(sessionId) .ipAddress(UserUtils.getClientIP(request)).userAgent(request.getHeader("User-Agent")) .action("showChangePasswordPage") - .actionStatus("Success").message("Requested. Result:" + result).build(); + // SUF-06: label by validity so anonymous invalid-token attempts are not recorded as "Success". + .actionStatus(valid ? "Success" : "Failure").message("Requested. Result:" + result).build(); eventPublisher.publishEvent(changePasswordAuditEvent); - if (TokenValidationResult.VALID.equals(result)) { + if (valid) { model.addAttribute("token", token); String redirectString = "redirect:" + forgotPasswordChangeURI; return new ModelAndView(redirectString, model); diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/StepUpService.java b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpService.java new file mode 100644 index 00000000..d69f6858 --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpService.java @@ -0,0 +1,44 @@ +package com.digitalsanctuary.spring.user.security; + +import com.digitalsanctuary.spring.user.persistence.model.User; +import jakarta.servlet.http.HttpServletRequest; + +/** + * SPI for step-up (re-)authentication before a sensitive, credential-altering operation. + * + *

+ * The framework does not ship a step-up primitive of its own. A consuming application may provide a Spring bean + * implementing this interface to require a fresh proof of presence/possession — e.g. a WebAuthn assertion, a TOTP + * code, or a recent-authentication signal — before an operation that cannot verify a current credential. + * The motivating case is setting an initial password on a passwordless (passkey-only) account via + * {@code POST /user/setPassword} (SUF-02): there is no existing password to check, so without step-up a caller who + * merely holds an authenticated session could add a durable password credential. + *

+ * + *

+ * Enforcement in {@code UserAPI.setPassword}: + *

+ *
    + *
  • If a {@code StepUpService} bean is present, it is invoked and must return {@code true} for the request to proceed.
    • + *
  • If no bean is present, the endpoint is disabled by default; set + * {@code user.security.allowInitialPasswordSetWithoutStepUp=true} to explicitly allow the session-only behavior.
    • + *
+ * + *

+ * Implementations should be side-effect free with respect to the operation itself; they only decide whether the caller + * has satisfied step-up. Return {@code false} (rather than throwing) to reject; the caller maps that to an HTTP 401. + *

+ */ +public interface StepUpService { + + /** + * Decides whether the caller has satisfied step-up authentication for the given action. + * + * @param user the authenticated user the operation targets (never {@code null}) + * @param action a short, stable action identifier (e.g. {@code "set-password"}) + * @param request the current HTTP request, so the implementation can read a step-up assertion/token supplied by the + * client (headers, body, or a prior challenge stored in the session) + * @return {@code true} if step-up is satisfied and the operation may proceed; {@code false} to reject it + */ + boolean isStepUpSatisfied(User user, String action, HttpServletRequest request); +} diff --git a/src/main/java/com/digitalsanctuary/spring/user/web/PasswordResetSecurityHeadersInterceptor.java b/src/main/java/com/digitalsanctuary/spring/user/web/PasswordResetSecurityHeadersInterceptor.java new file mode 100644 index 00000000..cf8475b8 --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/web/PasswordResetSecurityHeadersInterceptor.java @@ -0,0 +1,27 @@ +package com.digitalsanctuary.spring.user.web; + +import org.springframework.web.servlet.HandlerInterceptor; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; + +/** + * Adds privacy and no-cache response headers to the password-reset pages. + * + *

+ * The password-reset flow carries the reset token in the page URL (SUF-05 / CWE-598). This interceptor sets + * {@code Referrer-Policy: no-referrer} so the token is not leaked in the {@code Referer} header when the page loads + * sub-resources or navigates away, and {@code Cache-Control: no-store} so the token-bearing URL is not written to + * shared or browser caches. It is a non-breaking mitigation: it does not change the token contract, so the reset flow + * continues to work unchanged for existing consumers. Registered against the configured reset URIs by + * {@link WebInterceptorConfig}. + *

+ */ +public class PasswordResetSecurityHeadersInterceptor implements HandlerInterceptor { + + @Override + public boolean preHandle(final HttpServletRequest request, final HttpServletResponse response, final Object handler) { + response.setHeader("Referrer-Policy", "no-referrer"); + response.setHeader("Cache-Control", "no-store"); + return true; + } +} diff --git a/src/main/java/com/digitalsanctuary/spring/user/web/WebInterceptorConfig.java b/src/main/java/com/digitalsanctuary/spring/user/web/WebInterceptorConfig.java index b3e5c294..b774b9fe 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/web/WebInterceptorConfig.java +++ b/src/main/java/com/digitalsanctuary/spring/user/web/WebInterceptorConfig.java @@ -1,5 +1,6 @@ package com.digitalsanctuary.spring.user.web; +import org.springframework.beans.factory.annotation.Value; import org.springframework.context.annotation.Configuration; import org.springframework.web.servlet.config.annotation.InterceptorRegistry; import org.springframework.web.servlet.config.annotation.WebMvcConfigurer; @@ -22,12 +23,25 @@ public class WebInterceptorConfig implements WebMvcConfigurer { private final GlobalUserModelInterceptor globalUserModelInterceptor; + /** The password-reset token-validation endpoint; the reset token appears in its redirect URL. */ + @Value("${user.security.changePasswordURI:/user/changePassword}") + private String changePasswordURI; + + /** The change-password page the reset flow redirects to; the reset token appears in its URL. */ + @Value("${user.security.forgotPasswordChangeURI:/user/forgot-password-change.html}") + private String forgotPasswordChangeURI; + /** - * Add the global user model interceptor to the registry + * Add the global user model interceptor to the registry, plus the SUF-05 reset-page security-headers interceptor. */ @Override public void addInterceptors(InterceptorRegistry registry) { registry.addInterceptor(globalUserModelInterceptor).addPathPatterns("/", "/**") // Apply to all paths .excludePathPatterns("/static/**", "/css/**", "/js/**", "/images/**", "/favicon.ico"); // Exclude static assets + + // SUF-05 (CWE-598): the reset flow carries the reset token in the page URL. Add Referrer-Policy: no-referrer and + // Cache-Control: no-store to those pages so the token is not leaked via the Referer header or written to caches. + registry.addInterceptor(new PasswordResetSecurityHeadersInterceptor()) + .addPathPatterns(changePasswordURI, forgotPasswordChangeURI); } } diff --git a/src/main/resources/messages/dsspringusermessages.properties b/src/main/resources/messages/dsspringusermessages.properties index 54aa408f..74289cc2 100644 --- a/src/main/resources/messages/dsspringusermessages.properties +++ b/src/main/resources/messages/dsspringusermessages.properties @@ -16,6 +16,8 @@ message.update-password.success=Your password has been successfully updated. message.update-password.invalid-old=The old password is incorrect. message.update-password.account-locked=Your account is locked due to too many failed attempts. Please try again later. message.update-password.no-password=No password is set on this account. Please use the set password feature instead. +message.set-password.step-up-required=Additional verification is required to set a password. +message.set-password.disabled=Setting an initial password is not enabled on this server. message.password.mismatch=Passwords do not match. message.reset-password.success=Your password has been successfully reset. You can now log in with your new password. diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java index 4552e6c0..dc2fb3aa 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java @@ -5,6 +5,7 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -20,8 +21,12 @@ import com.digitalsanctuary.spring.user.audit.AuditEvent; import com.digitalsanctuary.spring.user.dto.PasswordDto; +import com.digitalsanctuary.spring.user.dto.SetPasswordDto; import com.digitalsanctuary.spring.user.dto.UserDto; import com.digitalsanctuary.spring.user.dto.UserProfileUpdateDto; +import com.digitalsanctuary.spring.user.security.StepUpService; +import java.util.List; +import org.springframework.beans.factory.ObjectProvider; import com.digitalsanctuary.spring.user.event.OnRegistrationCompleteEvent; import com.digitalsanctuary.spring.user.exceptions.InvalidOldPasswordException; import com.digitalsanctuary.spring.user.exceptions.UserAlreadyExistException; @@ -35,6 +40,7 @@ import com.digitalsanctuary.spring.user.util.AppUrlResolver; import com.digitalsanctuary.spring.user.util.JSONResponse; import com.fasterxml.jackson.databind.ObjectMapper; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; @@ -48,6 +54,7 @@ import org.springframework.context.MessageSource; import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; +import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.web.method.annotation.AuthenticationPrincipalArgumentResolver; import org.springframework.test.util.ReflectionTestUtils; import org.springframework.test.web.servlet.MockMvc; @@ -110,6 +117,12 @@ public JSONResponse handleSecurityException(SecurityException e) { @BeforeEach void setUp() { + // Tests run in parallel (junit-platform.properties: concurrent methods + classes) with thread reuse, and the + // `_notAuthenticated` tests resolve @AuthenticationPrincipal from the thread-local SecurityContext (expecting it + // empty). Clear any context another test left on this thread so those tests are deterministic — matching the + // convention in UserServiceTest / WebAuthnAuthenticationSuccessHandlerTest / UserEmailServiceTest. + SecurityContextHolder.clearContext(); + testUser = UserTestDataBuilder.aUser() .withId(1L) .withEmail("test@example.com") @@ -141,6 +154,12 @@ void setUp() { .build(); } + @AfterEach + void tearDown() { + // Do not leak a SecurityContext onto this (pooled, reused) thread for a subsequently scheduled parallel test. + SecurityContextHolder.clearContext(); + } + @Nested @DisplayName("User Registration Tests") class UserRegistrationTests { @@ -639,6 +658,108 @@ void updatePassword_notAuthenticated() throws Exception { .andExpect(jsonPath("$.code").value(401)) .andExpect(jsonPath("$.messages[0]").value("User not logged in.")); } + + // ---- SUF-02: /user/setPassword step-up guard ---- + + @SuppressWarnings("unchecked") + private ObjectProvider stepUpProvider(StepUpService service) { + ObjectProvider provider = mock(ObjectProvider.class); + when(provider.getIfAvailable()).thenReturn(service); + return provider; + } + + private SetPasswordDto newSetPasswordDto() { + SetPasswordDto dto = new SetPasswordDto(); + dto.setNewPassword("NewValidPass1!"); + dto.setConfirmPassword("NewValidPass1!"); + return dto; + } + + @Test + @DisplayName("POST /user/setPassword - disabled by default when no StepUpService is configured") + void setPassword_noStepUpService_disabledByDefault() throws Exception { + mockMvc = updatePasswordMockMvc(); + when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser); + when(userService.hasPassword(testUser)).thenReturn(false); + ReflectionTestUtils.setField(userAPI, "stepUpServiceProvider", stepUpProvider(null)); + when(messageSource.getMessage(eq("message.set-password.disabled"), any(), any(), any(Locale.class))) + .thenReturn("Setting an initial password is not enabled on this server."); + + mockMvc.perform(post("/user/setPassword") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(newSetPasswordDto())) + .with(csrf())) + .andExpect(status().isForbidden()) + .andExpect(jsonPath("$.success").value(false)); + + // Endpoint disabled by default: no step-up service, opt-in flag off -> the credential is never set. + verify(userService, never()).setInitialPassword(any(), any()); + } + + @Test + @DisplayName("POST /user/setPassword - allowed session-only when the opt-in flag is enabled") + void setPassword_noStepUpService_allowedWhenFlagEnabled() throws Exception { + mockMvc = updatePasswordMockMvc(); + when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser); + when(userService.hasPassword(testUser)).thenReturn(false); + ReflectionTestUtils.setField(userAPI, "stepUpServiceProvider", stepUpProvider(null)); + ReflectionTestUtils.setField(userAPI, "allowInitialPasswordSetWithoutStepUp", true); + when(passwordPolicyService.validate(eq(testUser), eq("NewValidPass1!"), eq(testUser.getEmail()), any(Locale.class))) + .thenReturn(List.of()); + + mockMvc.perform(post("/user/setPassword") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(newSetPasswordDto())) + .with(csrf())) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.success").value(true)); + + verify(userService).setInitialPassword(testUser, "NewValidPass1!"); + } + + @Test + @DisplayName("POST /user/setPassword - rejected 401 when the StepUpService denies step-up") + void setPassword_stepUpService_deniesReturns401() throws Exception { + mockMvc = updatePasswordMockMvc(); + when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser); + when(userService.hasPassword(testUser)).thenReturn(false); + StepUpService stepUp = mock(StepUpService.class); + when(stepUp.isStepUpSatisfied(eq(testUser), eq("set-password"), any())).thenReturn(false); + ReflectionTestUtils.setField(userAPI, "stepUpServiceProvider", stepUpProvider(stepUp)); + when(messageSource.getMessage(eq("message.set-password.step-up-required"), any(), any(), any(Locale.class))) + .thenReturn("Additional verification is required to set a password."); + + mockMvc.perform(post("/user/setPassword") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(newSetPasswordDto())) + .with(csrf())) + .andExpect(status().isUnauthorized()) + .andExpect(jsonPath("$.success").value(false)); + + verify(userService, never()).setInitialPassword(any(), any()); + } + + @Test + @DisplayName("POST /user/setPassword - proceeds when the StepUpService grants step-up") + void setPassword_stepUpService_grantsProceeds() throws Exception { + mockMvc = updatePasswordMockMvc(); + when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser); + when(userService.hasPassword(testUser)).thenReturn(false); + StepUpService stepUp = mock(StepUpService.class); + when(stepUp.isStepUpSatisfied(eq(testUser), eq("set-password"), any())).thenReturn(true); + ReflectionTestUtils.setField(userAPI, "stepUpServiceProvider", stepUpProvider(stepUp)); + when(passwordPolicyService.validate(eq(testUser), eq("NewValidPass1!"), eq(testUser.getEmail()), any(Locale.class))) + .thenReturn(List.of()); + + mockMvc.perform(post("/user/setPassword") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(newSetPasswordDto())) + .with(csrf())) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.success").value(true)); + + verify(userService).setInitialPassword(testUser, "NewValidPass1!"); + } } @Nested diff --git a/src/test/java/com/digitalsanctuary/spring/user/controller/UserActionControllerTest.java b/src/test/java/com/digitalsanctuary/spring/user/controller/UserActionControllerTest.java index 091a6fc8..955b8891 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/controller/UserActionControllerTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/controller/UserActionControllerTest.java @@ -140,6 +140,59 @@ void showChangePasswordPage_missingToken_returns400() throws Exception { mockMvc.perform(get("/user/changePassword")) .andExpect(status().isBadRequest()); } + + @Test + @DisplayName("SUF-06: invalid token audits as Failure and does not mint an HttpSession") + void showChangePasswordPage_invalidToken_auditsFailureAndCreatesNoSession() throws Exception { + // Given + String token = "invalid-token-123"; + when(userService.validatePasswordResetToken(token)).thenReturn(TokenValidationResult.INVALID_TOKEN); + + // When + var result = mockMvc.perform(get("/user/changePassword").param("token", token)) + .andExpect(status().is3xxRedirection()) + .andReturn(); + + // Then: an anonymous invalid-token probe must NOT be recorded as "Success"... + ArgumentCaptor auditCaptor = ArgumentCaptor.forClass(AuditEvent.class); + verify(eventPublisher).publishEvent(auditCaptor.capture()); + assert auditCaptor.getValue().getActionStatus().equals("Failure"); + // ...and must NOT mint a server-side HttpSession for the anonymous request (session amplification). + assert result.getRequest().getSession(false) == null; + } + + @Test + @DisplayName("SUF-06: valid token does not mint an HttpSession either") + void showChangePasswordPage_validToken_createsNoSession() throws Exception { + // Given + String token = "valid-token-123"; + when(userService.validatePasswordResetToken(token)).thenReturn(TokenValidationResult.VALID); + + // When + var result = mockMvc.perform(get("/user/changePassword").param("token", token)) + .andExpect(status().is3xxRedirection()) + .andReturn(); + + // Then: token validation is anonymous; no session should be created. + assert result.getRequest().getSession(false) == null; + } + + @Test + @DisplayName("SUF-05: reset page carries Referrer-Policy: no-referrer and Cache-Control: no-store") + void resetPage_carriesPrivacyHeaders() throws Exception { + // Given: the SUF-05 interceptor mapped to the reset URI (as WebInterceptorConfig registers it). + String token = "valid-token-123"; + when(userService.validatePasswordResetToken(token)).thenReturn(TokenValidationResult.VALID); + MockMvc mvc = MockMvcBuilders.standaloneSetup(userActionController) + .addMappedInterceptors(new String[] {"/user/changePassword"}, + new com.digitalsanctuary.spring.user.web.PasswordResetSecurityHeadersInterceptor()) + .build(); + + // When & Then: the token-bearing reset response must suppress Referer leakage and caching. + mvc.perform(get("/user/changePassword").param("token", token)) + .andExpect(header().string("Referrer-Policy", "no-referrer")) + .andExpect(header().string("Cache-Control", "no-store")); + } } @Nested