diff --git a/CONFIG.md b/CONFIG.md index 2fcb84df..0abbb4fd 100644 --- a/CONFIG.md +++ b/CONFIG.md @@ -125,12 +125,44 @@ user: **Client contract**: these endpoints consume JSON bodies, so the CAPTCHA token must be sent in the `X-Captcha-Token` request header (preferred) or the `cf-turnstile-response` query parameter — it cannot be added as a form field. Rejections return `HTTP 403` with a `JSONResponse` body (`code: 8`), customizable via the `message.captcha.validation-failed` message key. The site key is exposed to MVC pages as the `captchaSiteKey` model attribute. See the README's [CAPTCHA Protection](README.md#captcha-protection-cloudflare-turnstile) section for the full client-side contract, fail-closed semantics, and scope notes (login is not covered). -### Passwordless Initial Password Step-Up (SUF-02) +### Step-Up Re-Authentication (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: +Credential-altering operations on a passwordless (passkey-only) account have no current credential to verify: `POST /user/setPassword` adds an *initial* password, and passkey delete/rename change how the account authenticates. Step-up requires a recent proof of presence before those proceed. -- **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`. +**How it works.** Spring Security records how a user authenticated as a factor authority carrying an issue time. Step-up requires one of the configured factors to have been issued within a short window. The user refreshes it by re-running that login ceremony while already logged in — for `WEBAUTHN`, the ordinary passkey assertion at `/login/webauthn`. There is no separate step-up endpoint, challenge store, or token, and the client reuses its existing login ceremony. + +```yaml +user: + security: + stepUp: + enabled: false # default + ttlSeconds: 120 # how recently the factor must have been issued + factors: [WEBAUTHN] # any one is sufficient +``` + +- **Enabled (`user.security.stepUp.enabled`)**: Registers the framework's built-in `StepUpService`. Default: `false`, in which case nothing below applies and behavior is unchanged. +- **TTL (`user.security.stepUp.ttlSeconds`)**: How recently the factor must have been issued. Default: `120`. Keep it short: within the window, one ceremony authorizes any credential-altering operation on that session. +- **Factors (`user.security.stepUp.factors`)**: Any one satisfies step-up. Valid values: `WEBAUTHN`, `PASSWORD`, `OTT`, `AUTHORIZATION_CODE`, `SAML_RESPONSE`, `CAS`, `X509`, `BEARER`. Default: `WEBAUTHN`, the only factor whose refresh reliably proves presence — re-running an OAuth2 login typically completes with no user interaction while the identity-provider session is alive. Naming a factor your deployment never issues makes the gated operations permanently unavailable. Startup fails on an unknown name. + +**Client contract.** A gated operation with no sufficiently recent factor returns `HTTP 401`: `setPassword` with `JSONResponse` code `6`, passkey delete/rename with error code `step-up-required`. The client re-runs its passkey login ceremony and retries the original call. + +- **Enrollment window (`user.security.stepUp.enrollmentTtlSeconds`)**: How recently the user must have authenticated, by *any* means, to register a new passkey at `POST /webauthn/register`. Default: `600`. Applies only while step-up is enabled. + + Enrolling a passkey is what turns a stolen session into durable access: the credential outlives a password change, since session invalidation ends sessions rather than credentials, and asserting with it refreshes `FACTOR_WEBAUTHN`. Without this gate an attacker holding only a session cookie could enroll their own authenticator and use it to satisfy step-up, so the rest of the feature would protect nothing. This mirrors GitHub's sudo mode, which asks for a password before adding a security key. + + Any authentication factor counts, deliberately not the `factors` list above: with the default `[WEBAUTHN]`, requiring a configured factor would demand a passkey in order to register a first passkey. The window is longer than `ttlSeconds` because enrollment normally follows a login, a look around the settings page, and a decision, whereas a step-up ceremony immediately precedes its operation. + + **Residual risk:** within this window of a genuine login, an attacker sharing the session can still enroll. The window bounds the exposure rather than eliminating it, which is the same trade-off sudo mode makes. A `PasskeyRegistration` audit event is recorded for every enrollment. + +**Enabling step-up also enables factor merging** (`setMfaEnabled(true)` on authentication processing filters), without which re-authenticating replaces the session's authorities instead of merging them. If your application registers its own `AbstractAuthenticationProcessingFilter`, see the warning in `MfaFilterMergingConfiguration`. + +**Accounts with no passkey** (OAuth-only, for example) cannot satisfy `WEBAUTHN` step-up. For them `setPassword` remains governed by `allowInitialPasswordSetWithoutStepUp` below, exactly as before. + +**Reserved authority names.** Do not name a role or privilege `FACTOR_*` in `user.roles-and-privileges`. Spring Security uses that prefix for factor authorities, and a plain authority with such a name is indistinguishable from a real factor by name: it satisfies MFA enforcement without the factor ever being completed, and it shadows the genuine factor in a step-up freshness check. Startup fails when such a name is configured while MFA or step-up is enabled, and logs an error otherwise. + +**Custom implementations.** A `StepUpService` bean supplied by your application takes precedence over the built-in one, whatever `enabled` is set to. Implement the SPI to require TOTP, a hardware token, or any other proof. + +- **Allow Without Step-Up (`user.security.allowInitialPasswordSetWithoutStepUp`)**: When no `StepUpService` bean is present at all, `setPassword` is **disabled** (`HTTP 403`) unless this is `true`, which restores the previous session-only behavior. Default: `false`. ### Token Security diff --git a/MIGRATION.md b/MIGRATION.md index 1ba3bda9..af2ef219 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -212,7 +212,7 @@ Affected endpoints (all require `user.webauthn.enabled=true` except where noted) **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. -**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: +**Passwordless (passkey-only) accounts — residual risk:** For accounts with no password set, there is no current credential to verify. The framework now ships a step-up mechanism (see *Built-in step-up* below), but it is off by default, so unless you enable `user.security.stepUp.enabled` the following still applies: - 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` 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. @@ -223,7 +223,26 @@ Affected endpoints (all require `user.webauthn.enabled=true` except where noted) **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. +**Built-in step-up (this release).** The framework now ships a `StepUpService` of its own, off by default. Set `user.security.stepUp.enabled=true` and it requires one of `user.security.stepUp.factors` to have been issued within `user.security.stepUp.ttlSeconds` (default `120`). The user refreshes a factor by re-running that login ceremony while already logged in — for `WEBAUTHN`, the ordinary passkey assertion — so there is no new endpoint and the client reuses its existing ceremony. A `StepUpService` bean you supply still takes precedence. + +**Passkey enrollment is gated when step-up is enabled.** `POST /webauthn/register` now requires an authentication factor issued within `user.security.stepUp.enrollmentTtlSeconds` (default `600`). Without it the feature protected nothing: an attacker holding a session cookie could enroll their own passkey and assert with it to satisfy every other gate. Any factor counts, not the configured `factors` list, so a first passkey can still be registered after an ordinary password or social login. + +To make that possible, login flows that previously left no factor now stamp one: OIDC logins (`OidcAuthorizationCodeAuthenticationProvider` stamps none, unlike the password and plain-OAuth2 providers), the email-verification link (`FACTOR_OTT`), and post-registration auto-login (`FACTOR_PASSWORD`). Dev login still stamps nothing, so passkey enrollment is unavailable under `user.dev.auto-login-enabled` while step-up is on. `UserService.authWithoutPassword(User)` is unchanged for existing callers; a new overload takes the factor. + +**`StepUpService` gained a default method.** `canSatisfyStepUp(User)` reports whether a user could satisfy step-up at all, as opposed to whether they have. It defaults to `true`, so existing implementations compile and behave exactly as before. Override it when your mechanism depends on a credential some accounts lack: an OAuth2/OIDC account with no passkey can never produce a WebAuthn factor, and callers treat `false` as "step-up does not apply" and fall back to their configured default rather than rejecting an operation the user could never unlock. The built-in service overrides it, so with `user.security.stepUp.enabled=true` a social-login account with no passkey keeps the `allowInitialPasswordSetWithoutStepUp` behavior instead of receiving a permanent `HTTP 401` on `POST /user/setPassword`. The same applies to passkey delete and rename, which fall back to their pre-feature behavior for accounts that cannot satisfy the configured factors. + +If you unit-test against a mocked `StepUpService`, note that Mockito returns `false` for the unstubbed default, which makes callers skip step-up. Stub `canSatisfyStepUp(...)` to `true` in tests that are about whether step-up was *satisfied*. Real implementations inherit `true` and are unaffected. + +Startup now fails when `user.security.stepUp.factors` includes `WEBAUTHN` while `user.webauthn.enabled=false`, matching the existing check on `user.mfa.factors`: no account could produce that factor, so step-up would silently never apply. A `PASSWORD` factor logs a warning for the same reason, since passwordless and social-login accounts cannot satisfy it. + +Enabling it also gates passkey **delete and rename** on passwordless accounts, which previously required nothing beyond a session. Accounts with a password keep the current-password path unchanged. Rejections return `HTTP 401`, with error code `step-up-required` on the passkey endpoints. + +Two things to check before enabling it: + +- **Reserved authority names.** A role or privilege named `FACTOR_*` in `user.roles.roles-and-privileges` collides with Spring Security's factor authorities. Such a name satisfies MFA enforcement without the factor ever being completed, and shadows the genuine factor in a step-up freshness check. Startup now **fails** when one is present while MFA or step-up is enabled, and logs an error otherwise. The check covers both the configured names and the `role`/`privilege` tables, because `RolePrivilegeSetupService` never deletes, so a name removed from configuration survives as a row and is still granted. Rename the configured entries *and* delete the persisted rows. +- **Factor merging.** Enabling step-up turns on `setMfaEnabled(true)` for authentication processing filters, as `user.mfa.enabled=true` already did. If your application registers its own `AbstractAuthenticationProcessingFilter`, see the warning on `MfaFilterMergingConfiguration`. + +See CONFIG.md for the full configuration. ### 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 5360e1eb..71d217a2 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java +++ b/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java @@ -34,6 +34,7 @@ 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.AuthWithoutPasswordFactor; import com.digitalsanctuary.spring.user.security.UserSecurityConfigProperties; import com.digitalsanctuary.spring.user.service.DSUserDetails; import com.digitalsanctuary.spring.user.service.LoginAttemptService; @@ -367,7 +368,7 @@ public ResponseEntity updatePassword(@AuthenticationPrincipal DSUs // report a "failed attempt" to the lockout counter below — letting any authenticated (or session-hijacking) // caller lock the account out of EVERY authentication method by hitting this endpoint repeatedly. Reject up // front, before the lockout logic and without touching the counter, and point the user at the set-password - // flow. Mirrors WebAuthnManagementAPI.requireCurrentPasswordIfSet and the symmetric guard in setPassword(). + // flow. Mirrors WebAuthnManagementAPI.requireCredentialProof and the symmetric guard in setPassword(). if (!userService.hasPassword(user)) { logAuditEvent("PasswordUpdate", "Failure", "No password set", user, request); return buildErrorResponse(messages.getMessage("message.update-password.no-password", null, @@ -376,7 +377,7 @@ public ResponseEntity updatePassword(@AuthenticationPrincipal DSUs // Verifying the current password is an authentication surface, so it participates in the same brute-force // lockout as login: reject a locked account up front (HTTP 423) so a session-holding actor cannot make - // unlimited old-password guesses here. Mirrors WebAuthnManagementAPI.requireCurrentPasswordIfSet. + // unlimited old-password guesses here. Mirrors WebAuthnManagementAPI.requireCredentialProof. if (loginAttemptService.isLocked(user.getEmail())) { logAuditEvent("PasswordUpdate", "Failure", "Account locked", user, request); return buildErrorResponse(messages.getMessage("message.update-password.account-locked", null, @@ -570,7 +571,10 @@ public ResponseEntity setPassword(@AuthenticationPrincipal DSUserD // 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) { + // canSatisfyStepUp() separates "has not proven presence" from "could never prove it". A social-login + // account with no passkey falls in the second group, where denying is a dead end rather than a prompt, + // so step-up does not apply and allowInitialPasswordSetWithoutStepUp governs as it did before. + if (stepUpService != null && stepUpService.canSatisfyStepUp(user, "set-password")) { 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, @@ -639,7 +643,8 @@ private void validateAuthenticatedUser(DSUserDetails userDetails) { * @return the URI to redirect to after registration */ private String handleAutoLogin(User user) { - userService.authWithoutPassword(user); + // The password was submitted in the very request that produced this registration. + userService.authWithoutPassword(user, AuthWithoutPasswordFactor.REGISTRATION); return userSecurityConfig.getRegistrationSuccessUri(); } 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 1e3565e7..015f9a99 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java +++ b/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java @@ -1,6 +1,7 @@ package com.digitalsanctuary.spring.user.api; import java.util.List; +import org.springframework.beans.factory.ObjectProvider; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.context.ApplicationEventPublisher; import org.springframework.http.ResponseEntity; @@ -19,8 +20,11 @@ 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.WebAuthnStepUpRequiredException; import com.digitalsanctuary.spring.user.exceptions.WebAuthnUserNotFoundException; import com.digitalsanctuary.spring.user.persistence.model.User; +import com.digitalsanctuary.spring.user.security.StepUpConfigProperties; +import com.digitalsanctuary.spring.user.security.StepUpService; import com.digitalsanctuary.spring.user.service.LoginAttemptService; import com.digitalsanctuary.spring.user.service.UserService; import com.digitalsanctuary.spring.user.service.WebAuthnCredentialManagementService; @@ -61,7 +65,7 @@ * *

* Residual risk (passwordless accounts): For passwordless (passkey-only) accounts there is no current - * password to verify, and this library does not yet implement a WebAuthn step-up assertion. Passkey delete/rename on such + * password to verify, and step-up is available but off by default (see {@code user.security.stepUp.enabled}). Passkey delete/rename on such * accounts therefore remain session-only operations. Last-credential protection still prevents lockout, and ownership * (IDOR) checks remain enforced in the service layer. See MIGRATION.md for details and guidance. *

@@ -78,6 +82,10 @@ public class WebAuthnManagementAPI { private final UserService userService; private final ApplicationEventPublisher eventPublisher; private final LoginAttemptService loginAttemptService; + /** Step-up service, present when the framework's built-in one is enabled or a consumer supplies one (SUF-02). */ + private final ObjectProvider stepUpServiceProvider; + /** Step-up configuration; the enabled flag, not bean presence, is what opts these endpoints in. */ + private final StepUpConfigProperties stepUpConfigProperties; /** * Get user's registered passkeys. @@ -124,9 +132,9 @@ public ResponseEntity hasCredentials(@AuthenticationPrincipal UserDetai @PutMapping("/credentials/{id}/label") public ResponseEntity renameCredential(@PathVariable @NotBlank @Size(max = 512) String id, @RequestBody @Valid RenameCredentialRequest request, - @AuthenticationPrincipal UserDetails userDetails) { + @AuthenticationPrincipal UserDetails userDetails, HttpServletRequest httpRequest) { User user = findAuthenticatedUser(userDetails); - requireCurrentPasswordIfSet(user, request.currentPassword()); + requireCredentialProof(user, "rename-passkey", request.currentPassword(), httpRequest); credentialManagementService.renameCredential(id, request.label(), user); return ResponseEntity.ok(new GenericResponse("Passkey renamed successfully")); } @@ -158,9 +166,9 @@ public ResponseEntity renameCredential(@PathVariable @NotBlank @DeleteMapping("/credentials/{id}") public ResponseEntity deleteCredential(@PathVariable @NotBlank @Size(max = 512) String id, @RequestBody(required = false) CurrentPasswordRequest request, - @AuthenticationPrincipal UserDetails userDetails) { + @AuthenticationPrincipal UserDetails userDetails, HttpServletRequest httpRequest) { User user = findAuthenticatedUser(userDetails); - requireCurrentPasswordIfSet(user, request != null ? request.currentPassword() : null); + requireCredentialProof(user, "delete-passkey", request != null ? request.currentPassword() : null, httpRequest); credentialManagementService.deleteCredential(id, user); return ResponseEntity.ok(new GenericResponse("Passkey deleted successfully")); } @@ -178,7 +186,7 @@ public ResponseEntity deleteCredential(@PathVariable @NotBlank * caller must supply {@code currentPassword} in the request body. The body is declared {@code required = false} so * that a missing body does not produce a generic 415/400 from the message converter; instead, a missing or empty body * is treated identically to a missing {@code currentPassword} field — both are routed through - * {@link #requireCurrentPasswordIfSet} and result in HTTP 400 with the message + * {@link #requireCredentialProof} and result in HTTP 400 with the message * {@code "Current password is required to change authentication methods."}. A blank or incorrect * {@code currentPassword} is likewise rejected with a 400 before any mutation occurs. *

@@ -198,7 +206,7 @@ public ResponseEntity removePassword(@RequestBody(required = fa throw new WebAuthnException("User does not have a password to remove"); } - requireCurrentPasswordIfSet(user, body != null ? body.currentPassword() : null); + requireCredentialProof(user, "remove-password", body != null ? body.currentPassword() : null, request); if (!credentialManagementService.hasCredentials(user)) { throw new WebAuthnException("Cannot remove password. Please register a passkey first."); @@ -247,14 +255,31 @@ private User findAuthenticatedUser(UserDetails userDetails) throws WebAuthnUserN *

* * @param user the authenticated user + * @param action the operation being gated, passed to the step-up service for logging (e.g. {@code delete-passkey}) * @param currentPassword the current password supplied by the client (may be {@code null}) + * @param request the current HTTP request, so a step-up implementation can read proof supplied by the client + * @throws WebAuthnStepUpRequiredException if the account is passwordless and step-up is configured but unsatisfied (HTTP 401) * @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) { + private void requireCredentialProof(User user, String action, String currentPassword, HttpServletRequest request) { if (!userService.hasPassword(user)) { - // Passwordless (passkey-only) account: no current credential exists to verify. See MIGRATION.md residual-risk note. + // Passwordless (passkey-only) account: there is no current password to verify, so the only proof available + // is step-up. When no StepUpService is configured the operation proceeds as it always has; see MIGRATION.md + // for the residual risk that leaves. + // canSatisfyStepUp() first: an account holding no credential the configured factors accept could never + // pass the gate, so enforcing it would make the operation permanently impossible rather than prompting + // for a ceremony. Fall back to the pre-feature session-only behavior there (see MIGRATION.md). + // Keyed on the property rather than bean presence: applications that adopted the StepUpService SPI in + // 5.3.1 wired it for setPassword alone, and gating these endpoints off bean presence would newly enforce + // it for them on upgrade, with two action values their implementation never expected. + StepUpService stepUpService = stepUpConfigProperties.isEnabled() ? stepUpServiceProvider.getIfAvailable() : null; + if (stepUpService != null && stepUpService.canSatisfyStepUp(user, action) + && !stepUpService.isStepUpSatisfied(user, action, request)) { + throw new WebAuthnStepUpRequiredException( + "Recent authentication is required to change authentication methods. Please verify with your passkey and retry."); + } return; } if (loginAttemptService.isLocked(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 6c538b87..5adfc43c 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIAdvice.java +++ b/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIAdvice.java @@ -9,6 +9,7 @@ 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.WebAuthnStepUpRequiredException; import com.digitalsanctuary.spring.user.exceptions.WebAuthnUserNotFoundException; import com.digitalsanctuary.spring.user.util.GenericResponse; import jakarta.validation.ConstraintViolationException; @@ -40,6 +41,15 @@ public ResponseEntity handleReauthenticationFailure(WebAuthnRea return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body(new GenericResponse(ex.getMessage())); } + @ExceptionHandler(WebAuthnStepUpRequiredException.class) + public ResponseEntity handleStepUpRequired(WebAuthnStepUpRequiredException ex) { + log.warn("WebAuthn step-up required: {}", ex.getMessage()); + // A distinct error code, so a client can launch its login ceremony and retry rather than prompting for a + // password it may not have. + return ResponseEntity.status(HttpStatus.UNAUTHORIZED) + .body(new GenericResponse(ex.getMessage(), WebAuthnStepUpRequiredException.ERROR_CODE)); + } + @ExceptionHandler(WebAuthnException.class) public ResponseEntity handleWebAuthnError(WebAuthnException ex) { log.warn("WebAuthn error: {}", ex.getMessage()); 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 1cdb9322..5e641d4f 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/controller/UserActionController.java +++ b/src/main/java/com/digitalsanctuary/spring/user/controller/UserActionController.java @@ -11,6 +11,7 @@ import org.springframework.web.servlet.ModelAndView; import com.digitalsanctuary.spring.user.audit.AuditEvent; import com.digitalsanctuary.spring.user.persistence.model.User; +import com.digitalsanctuary.spring.user.service.AuthWithoutPasswordFactor; import com.digitalsanctuary.spring.user.security.UserSecurityConfigProperties; import com.digitalsanctuary.spring.user.service.TokenHasher; import com.digitalsanctuary.spring.user.service.UserService; @@ -111,7 +112,8 @@ public ModelAndView confirmRegistration(final HttpServletRequest request, final if (result == TokenValidationResult.VALID) { if (user != null) { // The token was already consumed (deleted) atomically inside validateVerificationToken. - userService.authWithoutPassword(user); + // The user proved possession of an emailed one-time token to reach this point. + userService.authWithoutPassword(user, AuthWithoutPasswordFactor.EMAIL_VERIFICATION); AuditEvent registrationAuditEvent = AuditEvent.builder().source(this).user(user) .sessionId(request.getSession().getId()) diff --git a/src/main/java/com/digitalsanctuary/spring/user/dev/DevLoginController.java b/src/main/java/com/digitalsanctuary/spring/user/dev/DevLoginController.java index e7565265..16253335 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/dev/DevLoginController.java +++ b/src/main/java/com/digitalsanctuary/spring/user/dev/DevLoginController.java @@ -12,6 +12,7 @@ import com.digitalsanctuary.spring.user.persistence.model.User; import com.digitalsanctuary.spring.user.persistence.repository.UserRepository; import com.digitalsanctuary.spring.user.service.UserService; +import com.digitalsanctuary.spring.user.service.AuthWithoutPasswordFactor; import com.digitalsanctuary.spring.user.util.JSONResponse; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; @@ -65,7 +66,8 @@ public ResponseEntity loginAs(@PathVariable String email) { .build()); } - userService.authWithoutPassword(user); + // Impersonation proves nothing about presence, so it stamps no factor. + userService.authWithoutPassword(user, AuthWithoutPasswordFactor.DEV_LOGIN); log.warn("Dev login successful for user: {}", email); return ResponseEntity.status(HttpStatus.FOUND) diff --git a/src/main/java/com/digitalsanctuary/spring/user/exceptions/WebAuthnStepUpRequiredException.java b/src/main/java/com/digitalsanctuary/spring/user/exceptions/WebAuthnStepUpRequiredException.java new file mode 100644 index 00000000..492bf5cd --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/exceptions/WebAuthnStepUpRequiredException.java @@ -0,0 +1,28 @@ +package com.digitalsanctuary.spring.user.exceptions; + +/** + * Thrown when a credential-altering passkey operation needs step-up (re-)authentication that the caller has not + * satisfied. + * + *

+ * Distinct from {@link WebAuthnReauthenticationException}, which reports a supplied credential being wrong. This one + * reports that no recent proof of presence exists, so the client should re-run its login ceremony and retry. It maps to + * HTTP 401 with the error code {@code step-up-required}. + *

+ */ +public class WebAuthnStepUpRequiredException extends WebAuthnException { + + private static final long serialVersionUID = 1L; + + /** Error code returned to clients, so a step-up prompt can be told apart from a wrong-credential failure. */ + public static final String ERROR_CODE = "step-up-required"; + + /** + * Creates a new exception with the given message. + * + * @param message the detail message + */ + public WebAuthnStepUpRequiredException(final String message) { + super(message); + } +} diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpService.java b/src/main/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpService.java new file mode 100644 index 00000000..058e6f7d --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpService.java @@ -0,0 +1,169 @@ +package com.digitalsanctuary.spring.user.security; + +import java.time.Duration; +import java.util.List; +import java.util.Locale; +import java.util.function.Predicate; +import org.springframework.security.authorization.AllRequiredFactorsAuthorizationManager; +import org.springframework.security.authorization.AuthorizationManager; +import org.springframework.security.authorization.AuthorizationResult; +import org.springframework.security.authorization.RequiredFactor; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.security.core.context.SecurityContextHolderStrategy; +import com.digitalsanctuary.spring.user.persistence.model.User; +import jakarta.servlet.http.HttpServletRequest; +import lombok.extern.slf4j.Slf4j; + +/** + * The framework's built-in {@link StepUpService}: step-up is satisfied when the current authentication carries a + * configured factor that was issued within {@code user.security.stepUp.ttlSeconds}. + * + *

+ * Spring Security stamps a {@code FactorGrantedAuthority} on every successful login and records when it was issued + * (for passkeys, {@code WebAuthnAuthenticationProvider} adds {@code FACTOR_WEBAUTHN} with the moment of assertion). + * Re-running that login while already authenticated merges the new factor into the existing session, so its issue time + * moves forward and the user's other authorities survive. Step-up is therefore just "assert again", using the login + * ceremony the application already has: no challenge endpoint, no server-side challenge state, no step-up token. + *

+ * + *

+ * That merge is not on by default. It requires {@code mfaEnabled} on the authentication filters, which + * {@link MfaFilterMergingConfiguration} sets whenever MFA or step-up is enabled. Without it a second login + * replaces the first, and the user would lose every authority the {@code UserDetailsService} does not re-supply. + *

+ * + *

What this does and does not bind

+ * + *

+ * The proof is bound to the user (the factor lives on their authentication), to the session (authentication is + * session-scoped), and to time (the TTL). It is not single-use and not bound to a + * particular operation: within the window, one ceremony authorizes any credential-altering operation on that session, + * and {@code action} is used only for logging. + * + *

+ * This closes the case the feature exists for, an attacker holding a session cookie but no authenticator. That + * depends on passkey enrollment being gated too, which {@code user.security.stepUp.enrollmentTtlSeconds} + * does: otherwise such an attacker would simply register their own authenticator and assert with it, producing a + * genuinely fresh factor. Two narrower cases stay open: an attacker sharing the session concurrently can piggyback + * inside either window, and a session stolen within {@code enrollmentTtlSeconds} of a real login can still enroll. + * Keep both TTLs short. + *

+ * + *

+ * Note for anyone tempted to make the proof single-use: do not consume it by removing the factor authority. When + * {@code user.mfa.factors} includes the same factor, removing it revokes access to every authenticated endpoint, + * because MFA enforcement reads that authority too. Re-stamp it with a backdated issue time instead, since MFA checks + * only presence while step-up checks age. + *

+ */ +@Slf4j +public class DSFactorFreshnessStepUpService implements StepUpService { + + private final List> factorManagers; + private final List factorNames; + private final Duration ttl; + private final Predicate hasPasskey; + + private SecurityContextHolderStrategy securityContextHolderStrategy = SecurityContextHolder.getContextHolderStrategy(); + + /** + * Creates a service enforcing the configured factors and TTL. + * + * @param config the step-up configuration; its factor names must already have been validated + * @param hasPasskey answers whether a user holds at least one registered WebAuthn credential, used by + * {@link #canSatisfyStepUp(User, String)} to tell "has not asserted" apart from "has nothing to assert with" + */ + public DSFactorFreshnessStepUpService(StepUpConfigProperties config, Predicate hasPasskey) { + this.hasPasskey = hasPasskey; + this.ttl = Duration.ofSeconds(config.getTtlSeconds()); + this.factorNames = List.copyOf(config.getFactors()); + // One manager per factor, evaluated as "any of". Spring Security 7.1 offers + // AllRequiredFactorsAuthorizationManager.anyOf for this, but it is deliberately not used: the framework also + // supports Spring Security 7.0 consumers, where that method does not exist. + this.factorManagers = this.factorNames.stream().map(name -> buildManager(name, this.ttl)).toList(); + } + + private static AuthorizationManager buildManager(String factorName, Duration ttl) { + String authority = StepUpConfigProperties.FACTOR_AUTHORITIES.get(factorName.toUpperCase(Locale.ROOT)); + return AllRequiredFactorsAuthorizationManager.builder() + .requireFactor(RequiredFactor.withAuthority(authority).validDuration(ttl).build()).build(); + } + + /** + * Sets the {@link SecurityContextHolderStrategy} used to read the current authentication. + * + * @param securityContextHolderStrategy the strategy to use; must not be null + */ + public void setSecurityContextHolderStrategy(SecurityContextHolderStrategy securityContextHolderStrategy) { + this.securityContextHolderStrategy = securityContextHolderStrategy; + } + + @Override + public boolean isStepUpSatisfied(User user, String action, HttpServletRequest request) { + Authentication authentication = securityContextHolderStrategy.getContext().getAuthentication(); + if (authentication == null || !authentication.isAuthenticated()) { + log.debug("Step-up denied for action {}: no authenticated request context", action); + return false; + } + if (user == null || user.getEmail() == null || !user.getEmail().equalsIgnoreCase(authentication.getName())) { + // The operation targets someone other than the authenticated caller, so the caller's factor says nothing + // about it. Callers in this framework always pass the authenticated user, so this indicates a bug or a + // consumer calling the SPI directly. + log.warn("Step-up denied for action {}: target user does not match the authenticated principal", action); + return false; + } + + for (AuthorizationManager manager : factorManagers) { + AuthorizationResult result = manager.authorize(() -> authentication, action); + if (result != null && result.isGranted()) { + log.debug("Step-up satisfied for action {} by a factor issued within {}", action, ttl); + return true; + } + } + + log.debug("Step-up denied for action {}: no factor of {} issued within {}", action, factorNames, ttl); + return false; + } + + /** + * Reports whether any configured factor is achievable for this user at all. + * + *

+ * {@code WEBAUTHN} needs a registered passkey and {@code PASSWORD} needs a password, so an account holding + * neither cannot produce either no matter what the user does. Every other factor is delivered out of band and + * cannot be ruled out from here, so it is assumed achievable and the operation stays gated. + *

+ * + * @param user the authenticated user the operation targets + * @param action the operation being gated, used only for logging + * @return {@code true} if at least one configured factor could be produced by this user + */ + @Override + public boolean canSatisfyStepUp(User user, String action) { + if (user == null) { + // Contract violation rather than a real account. Keep the gate; isStepUpSatisfied does the denying. + return true; + } + for (String factorName : factorNames) { + switch (factorName.toUpperCase(Locale.ROOT)) { + case "WEBAUTHN" -> { + if (hasPasskey.test(user)) { + return true; + } + } + case "PASSWORD" -> { + if (user.getPassword() != null && !user.getPassword().isEmpty()) { + return true; + } + } + default -> { + return true; + } + } + } + log.debug("Step-up does not apply to action {}: the account holds no credential able to produce any of {}", + action, factorNames); + return false; + } +} diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/FactorAuthorityNameValidator.java b/src/main/java/com/digitalsanctuary/spring/user/security/FactorAuthorityNameValidator.java new file mode 100644 index 00000000..3a7a077f --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/security/FactorAuthorityNameValidator.java @@ -0,0 +1,123 @@ +package com.digitalsanctuary.spring.user.security; + +import java.util.List; +import java.util.Locale; +import java.util.Set; +import java.util.TreeSet; +import java.util.stream.Stream; +import org.springframework.beans.factory.ObjectProvider; +import org.springframework.context.ApplicationListener; +import org.springframework.context.event.ContextRefreshedEvent; +import com.digitalsanctuary.spring.user.persistence.model.Privilege; +import com.digitalsanctuary.spring.user.persistence.model.Role; +import com.digitalsanctuary.spring.user.persistence.repository.PrivilegeRepository; +import com.digitalsanctuary.spring.user.persistence.repository.RoleRepository; +import com.digitalsanctuary.spring.user.roles.RolesAndPrivilegesConfig; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; + +/** + * Rejects role and privilege names that collide with Spring Security's factor authorities ({@code FACTOR_*}). + * + *

+ * Spring Security records how a user authenticated as a {@code FactorGrantedAuthority} carrying an issue time, and + * resolves a required factor by taking the first authority whose string matches, then inspecting that one. A + * plain granted authority that happens to be named {@code FACTOR_WEBAUTHN} therefore behaves as a counterfeit factor, + * and because {@code user.roles-and-privileges} authorities are loaded from the database ahead of the stamped one, the + * counterfeit always wins: + *

+ *
    + *
  • MFA enforcement ({@code user.mfa.enabled=true}) checks only that the authority is present, so the + * counterfeit satisfies the requirement outright and the user reaches protected endpoints without ever completing that + * factor. This is an authentication bypass for the affected deployment.
    • + *
  • Step-up additionally checks the issue time. The counterfeit has none, so it is treated as expired and + * shadows the genuine factor behind it: the gate denies immediately after a real ceremony, and the operation can never + * be performed. This fails closed, but the symptom is opaque.
    • + *
+ * + *

+ * The check therefore runs whether or not either feature is switched on. It fails startup when MFA or step-up is + * enabled, since the consequence is a security hole or an unusable feature, and logs an error otherwise, since the + * names are inert today but will break either feature the moment it is turned on. + *

+ */ +@Slf4j +@RequiredArgsConstructor +public class FactorAuthorityNameValidator implements ApplicationListener { + + /** Prefix Spring Security reserves for authentication-factor authorities. */ + static final String FACTOR_PREFIX = "FACTOR_"; + + private final RolesAndPrivilegesConfig rolesAndPrivilegesConfig; + private final MfaConfigProperties mfaConfigProperties; + private final StepUpConfigProperties stepUpConfigProperties; + private final ObjectProvider roleRepositoryProvider; + private final ObjectProvider privilegeRepositoryProvider; + + /** + * Runs the check at startup. + * + *

+ * Driven by {@code ContextRefreshedEvent} rather than {@code @PostConstruct}: nothing injects this bean, so under + * {@code spring.main.lazy-initialization=true} a construction-time callback would never fire and the check would + * silently not run. It also puts the check after {@code RolePrivilegeSetupService}, which creates the rows. + *

+ * + * @throws IllegalStateException if a configured role or privilege name starts with {@code FACTOR_} while MFA or + * step-up is enabled + */ + @Override + public void onApplicationEvent(ContextRefreshedEvent event) { + validateAuthorityNames(); + } + + public void validateAuthorityNames() { + List offenders = findOffendingNames(); + if (offenders.isEmpty()) { + return; + } + + String message = "Reserved authority names in use: " + offenders + ". These collide with Spring Security's reserved " + + FACTOR_PREFIX + "* authentication-factor authorities. A granted authority with such a name is indistinguishable " + + "from a real factor by name: it satisfies MFA enforcement without the factor ever being completed, and it shadows " + + "the genuine factor in a step-up freshness check. Rename them in user.roles.roles-and-privileges, and delete " + + "any matching row already persisted in the role or privilege table."; + + if (mfaConfigProperties.isEnabled() || stepUpConfigProperties.isEnabled()) { + throw new IllegalStateException(message); + } + log.error("{} They are inert while both user.mfa.enabled and user.security.stepUp.enabled are false, and startup will fail " + + "once either is turned on.", message); + } + + /** + * Exposes the offending names for tests and diagnostics. + * + * @return the configured role and privilege names that collide with the reserved prefix, in sorted order + */ + public List findOffendingNames() { + Set offenders = new TreeSet<>(); + rolesAndPrivilegesConfig.getRolesAndPrivileges().forEach((role, privileges) -> Stream + .concat(Stream.of(role), privileges == null ? Stream.empty() : privileges.stream()) + .filter(FactorAuthorityNameValidator::isReserved).forEach(offenders::add)); + + // Configuration is not the whole authority space. AuthorityService grants from the role and privilege + // tables, and RolePrivilegeSetupService never deletes, so a FACTOR_-prefixed row created under an earlier + // configuration survives its removal from YAML and is still granted while the config check reports clean. + RoleRepository roleRepository = roleRepositoryProvider.getIfAvailable(); + if (roleRepository != null) { + roleRepository.findAll().stream().map(Role::getName).filter(FactorAuthorityNameValidator::isReserved) + .forEach(offenders::add); + } + PrivilegeRepository privilegeRepository = privilegeRepositoryProvider.getIfAvailable(); + if (privilegeRepository != null) { + privilegeRepository.findAll().stream().map(Privilege::getName) + .filter(FactorAuthorityNameValidator::isReserved).forEach(offenders::add); + } + return List.copyOf(offenders); + } + + private static boolean isReserved(String name) { + return name != null && name.toUpperCase(Locale.ROOT).startsWith(FACTOR_PREFIX); + } +} diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/FreshFactorAuthorizationManager.java b/src/main/java/com/digitalsanctuary/spring/user/security/FreshFactorAuthorizationManager.java new file mode 100644 index 00000000..a3436216 --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/security/FreshFactorAuthorizationManager.java @@ -0,0 +1,67 @@ +package com.digitalsanctuary.spring.user.security; + +import java.time.Clock; +import java.time.Duration; +import java.time.Instant; +import java.util.function.Supplier; +import org.springframework.security.authorization.AuthorizationDecision; +import org.springframework.security.authorization.AuthorizationManager; +import org.springframework.security.authorization.AuthorizationResult; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.authority.FactorGrantedAuthority; +import lombok.extern.slf4j.Slf4j; + +/** + * Grants access only when the caller authenticated recently, by any means. + * + *

+ * Used to gate passkey enrollment, the way GitHub's sudo mode asks for a password before adding a security key. + * Enrolling a credential is what makes a stolen session durable: the new passkey outlives a password change, and it + * refreshes {@code FACTOR_WEBAUTHN}, which would otherwise let an attacker satisfy step-up with an authenticator + * they enrolled moments earlier. + *

+ * + *

+ * Any {@link FactorGrantedAuthority} counts, deliberately not the {@code user.security.stepUp.factors} list. With the + * default {@code [WEBAUTHN]}, requiring a configured factor would demand a passkey in order to register a first + * passkey, which no one could satisfy. A plain authority merely named like a factor carries no issue time and can + * never be fresh, so it cannot be used to forge recency. + *

+ * + * @param the authorization context type, unused: the decision depends only on the authentication + */ +@Slf4j +public class FreshFactorAuthorizationManager implements AuthorizationManager { + + private final Duration ttl; + private final Clock clock; + + /** + * Creates a manager granting access when a factor was issued within the given window. + * + * @param ttl how recently the caller must have authenticated + * @param clock the time source, injectable so freshness boundaries can be tested deterministically + */ + public FreshFactorAuthorizationManager(Duration ttl, Clock clock) { + this.ttl = ttl; + this.clock = clock; + } + + @Override + public AuthorizationResult authorize(Supplier authentication, T context) { + Authentication auth = authentication != null ? authentication.get() : null; + if (auth == null || !auth.isAuthenticated()) { + return new AuthorizationDecision(false); + } + + Instant cutoff = clock.instant().minus(ttl); + boolean fresh = auth.getAuthorities().stream().filter(FactorGrantedAuthority.class::isInstance) + .map(FactorGrantedAuthority.class::cast).map(FactorGrantedAuthority::getIssuedAt) + .anyMatch(issuedAt -> issuedAt != null && issuedAt.isAfter(cutoff)); + + if (!fresh) { + log.debug("Passkey enrollment denied for {}: no authentication factor issued within {}", auth.getName(), ttl); + } + return new AuthorizationDecision(fresh); + } +} diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/MfaFilterMergingConfiguration.java b/src/main/java/com/digitalsanctuary/spring/user/security/MfaFilterMergingConfiguration.java index 1bdb8ed3..5d4f51b0 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/MfaFilterMergingConfiguration.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/MfaFilterMergingConfiguration.java @@ -1,9 +1,12 @@ package com.digitalsanctuary.spring.user.security; import org.springframework.beans.factory.config.BeanPostProcessor; +import org.springframework.boot.autoconfigure.condition.AnyNestedCondition; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Conditional; import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.ConfigurationCondition; import org.springframework.security.web.authentication.AbstractAuthenticationProcessingFilter; import lombok.extern.slf4j.Slf4j; @@ -21,14 +24,21 @@ *

* *

- * WARNING — scope of {@code setMfaEnabled(true)}. When {@code user.mfa.enabled=true}, the post-processor flips + * Also required by step-up. The framework's built-in {@link StepUpService} refreshes a factor by having the user + * re-run an ordinary login ceremony while already authenticated, which only preserves the session's existing authorities + * when this merging is active. The configuration is therefore enabled by {@code user.mfa.enabled=true} or + * {@code user.security.stepUp.enabled=true}, and the warning below applies equally to either. + *

+ * + *

+ * WARNING — scope of {@code setMfaEnabled(true)}. When this configuration is active, the post-processor flips * MFA mode on every {@link AbstractAuthenticationProcessingFilter} bean in the application context. That includes * any filter a consuming application defines that extends this base class (e.g. a custom JWT or API-key * authentication filter). Such a filter will then also perform SS7 factor merging: on a subsequent authentication for an * already-authenticated principal it rebuilds the result via {@code authenticationResult.toBuilder()...}. If that filter's * {@link org.springframework.security.core.Authentication} implementation does not support {@code toBuilder()}, the merge can * throw at runtime. Consumers enabling MFA who also register custom processing filters should be aware their filters are - * affected. (This mirrors the framework default: it is only active when MFA is explicitly enabled.) + * affected. (This mirrors the framework default: it is only active when MFA or step-up is explicitly enabled.) *

* * @see MfaConfiguration @@ -36,13 +46,33 @@ */ @Slf4j @Configuration -@ConditionalOnProperty(name = "user.mfa.enabled", havingValue = "true", matchIfMissing = false) +@Conditional(MfaFilterMergingConfiguration.FactorMergingEnabled.class) public class MfaFilterMergingConfiguration { + /** + * Active when either feature that depends on factor merging is switched on. Registered as a + * {@link ConfigurationCondition} evaluating in the {@code REGISTER_BEAN} phase, matching the phase + * {@code @ConditionalOnProperty} uses on a {@code @Configuration} class. + */ + static final class FactorMergingEnabled extends AnyNestedCondition { + + FactorMergingEnabled() { + super(ConfigurationPhase.REGISTER_BEAN); + } + + @ConditionalOnProperty(name = "user.mfa.enabled", havingValue = "true", matchIfMissing = false) + static final class MfaEnabled { + } + + @ConditionalOnProperty(prefix = "user.security.step-up", name = "enabled", havingValue = "true", matchIfMissing = false) + static final class StepUpEnabled { + } + } + /** * Replicates the behaviour of {@code @EnableMultiFactorAuthentication}'s internal {@code EnableMfaFiltersPostProcessor} * using only public API, by invoking the public {@link AbstractAuthenticationProcessingFilter#setMfaEnabled(boolean)} on - * every authentication processing filter. Without this, completing a second factor would REPLACE the first factor's + * every authentication processing filter. Without this, completing a second factor (or re-asserting for step-up) would REPLACE the first factor's * authentication (dropping its authority) and the user could never satisfy all required factors (the H4 lockout). * *

diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfiguration.java b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfiguration.java new file mode 100644 index 00000000..643d97cf --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfiguration.java @@ -0,0 +1,123 @@ +package com.digitalsanctuary.spring.user.security; + +import java.util.List; +import java.util.Locale; +import java.util.Set; +import java.util.TreeSet; +import java.util.stream.Collectors; +import org.springframework.beans.factory.ObjectProvider; +import org.springframework.boot.autoconfigure.AutoConfiguration; +import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.boot.context.properties.EnableConfigurationProperties; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.PropertySource; +import com.digitalsanctuary.spring.user.persistence.repository.PrivilegeRepository; +import com.digitalsanctuary.spring.user.persistence.repository.RoleRepository; +import com.digitalsanctuary.spring.user.roles.RolesAndPrivilegesConfig; +import com.digitalsanctuary.spring.user.service.WebAuthnCredentialManagementService; +import lombok.extern.slf4j.Slf4j; + +/** + * Registers the framework's built-in {@link StepUpService} when {@code user.security.stepUp.enabled=true}. + * + *

+ * The bean backs off entirely when a consuming application supplies its own {@code StepUpService}, so an application + * with a bespoke step-up mechanism keeps it. When step-up is disabled no bean is registered at all, which leaves + * {@code POST /user/setPassword} governed by {@code user.security.allowInitialPasswordSetWithoutStepUp} and passkey + * delete/rename unchanged. + *

+ * + *

+ * The factor-authority name check registered here runs regardless of whether step-up is enabled, because a colliding + * authority name breaks MFA as well. See {@link FactorAuthorityNameValidator}. + *

+ */ +@Slf4j +@AutoConfiguration +@PropertySource("classpath:config/dsspringuserconfig.properties") +@EnableConfigurationProperties(StepUpConfigProperties.class) +public class StepUpAutoConfiguration { + + /** + * Creates the built-in step-up service, validating the configured factor names first. + * + * @param config the step-up configuration + * @param webAuthnConfigProperties the WebAuthn configuration, so a WEBAUTHN factor that no login flow could + * ever issue is rejected at startup rather than silently making step-up inert + * @param credentialServiceProvider provides the WebAuthn credential service when WebAuthn is enabled, so the + * service can tell an account with no passkey apart from one that simply has not asserted + * @return the built-in {@link StepUpService} + * @throws IllegalStateException if {@code user.security.stepUp.factors} is empty or names an unknown factor + */ + @Bean + @ConditionalOnMissingBean(StepUpService.class) + @ConditionalOnProperty(prefix = "user.security.step-up", name = "enabled", havingValue = "true") + public StepUpService dsFactorFreshnessStepUpService(StepUpConfigProperties config, + WebAuthnConfigProperties webAuthnConfigProperties, + ObjectProvider credentialServiceProvider) { + validateFactors(config.getFactors(), webAuthnConfigProperties); + log.info("Step-up enabled: a factor of {} issued within {}s authorizes credential-altering operations", + config.getFactors(), config.getTtlSeconds()); + // Resolved per call rather than captured: WebAuthnCredentialManagementService is conditional on + // user.webauthn.enabled, and when it is absent no account can hold a passkey. + return new DSFactorFreshnessStepUpService(config, user -> { + WebAuthnCredentialManagementService credentialService = credentialServiceProvider.getIfAvailable(); + return credentialService != null && credentialService.hasCredentials(user); + }); + } + + /** + * Creates the startup check that rejects role and privilege names colliding with Spring Security factor + * authorities. + * + * @param rolesAndPrivilegesConfig the configured roles and privileges + * @param mfaConfigProperties the MFA configuration + * @param stepUpConfigProperties the step-up configuration + * @param roleRepositoryProvider provides the role table, so names persisted under an earlier configuration are + * caught as well as those currently declared + * @param privilegeRepositoryProvider provides the privilege table, for the same reason + * @return the validator + */ + @Bean + @ConditionalOnMissingBean(FactorAuthorityNameValidator.class) + public FactorAuthorityNameValidator factorAuthorityNameValidator(RolesAndPrivilegesConfig rolesAndPrivilegesConfig, + MfaConfigProperties mfaConfigProperties, StepUpConfigProperties stepUpConfigProperties, + ObjectProvider roleRepositoryProvider, + ObjectProvider privilegeRepositoryProvider) { + return new FactorAuthorityNameValidator(rolesAndPrivilegesConfig, mfaConfigProperties, stepUpConfigProperties, + roleRepositoryProvider, privilegeRepositoryProvider); + } + + /** + * Fails startup rather than at the first gated request: a typo here would otherwise surface as an operation nobody + * can ever perform, since the required factor is never issued. + */ + private static void validateFactors(List factors, WebAuthnConfigProperties webAuthnConfigProperties) { + if (factors == null || factors.isEmpty()) { + throw new IllegalStateException("Step-up is enabled (user.security.stepUp.enabled=true) but no factors are configured. " + + "Set user.security.stepUp.factors to one or more of " + knownFactors() + "."); + } + Set unknown = factors.stream().filter(factor -> factor == null || factor.isBlank() + || !StepUpConfigProperties.FACTOR_AUTHORITIES.containsKey(factor.toUpperCase(Locale.ROOT))) + .map(factor -> factor == null ? "null" : factor).collect(Collectors.toCollection(TreeSet::new)); + if (!unknown.isEmpty()) { + throw new IllegalStateException("Unknown step-up factor(s) " + unknown + " in user.security.stepUp.factors. " + + "Valid values are " + knownFactors() + "."); + } + if (factors.stream().anyMatch(factor -> "WEBAUTHN".equalsIgnoreCase(factor)) && !webAuthnConfigProperties.isEnabled()) { + throw new IllegalStateException( + "Step-up factor WEBAUTHN is configured but WebAuthn is disabled (user.webauthn.enabled=false). " + + "No account could ever produce that factor, so step-up would never apply. " + + "Enable WebAuthn or remove WEBAUTHN from user.security.stepUp.factors."); + } + if (factors.stream().anyMatch(factor -> "PASSWORD".equalsIgnoreCase(factor))) { + log.warn("Step-up factor PASSWORD is configured. Passwordless (passkey-only and social-login) accounts " + + "cannot satisfy it, so step-up will not apply to them. Consider your account types carefully."); + } + } + + private static String knownFactors() { + return new TreeSet<>(StepUpConfigProperties.FACTOR_AUTHORITIES.keySet()).toString(); + } +} diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/StepUpConfigProperties.java b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpConfigProperties.java new file mode 100644 index 00000000..b9a4cfce --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpConfigProperties.java @@ -0,0 +1,87 @@ +package com.digitalsanctuary.spring.user.security; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import org.springframework.boot.context.properties.ConfigurationProperties; +import org.springframework.validation.annotation.Validated; +import org.springframework.security.core.authority.FactorGrantedAuthority; +import jakarta.validation.constraints.Min; +import lombok.Data; + +/** + * Configuration properties for step-up (re-)authentication before credential-altering operations. + * + *

+ * Step-up is satisfied by an authentication factor that was issued recently: the user proves presence by re-running an + * ordinary login ceremony (for {@code WEBAUTHN}, the passkey assertion at {@code /login/webauthn}) while already + * logged in, which refreshes that factor's issue time. There is no separate step-up ceremony, endpoint, or token. + *

+ * + *

+ * The window is bound to the session and to time, not to a single operation: within {@code ttlSeconds} of a + * ceremony, any credential-altering operation on that session is authorized. That is a deliberate trade-off; see + * {@link DSFactorFreshnessStepUpService}. + *

+ */ +@Data +@Validated +@ConfigurationProperties(prefix = "user.security.step-up") +public class StepUpConfigProperties { + + /** + * Factor names accepted in {@link #factors}, mapped to the Spring Security authority they require. Every + * {@link FactorGrantedAuthority} constant is offered, but only factors the deployment actually issues can ever be + * refreshed: requiring a factor no login flow produces makes the gated operations permanently unavailable. + */ + static final Map FACTOR_AUTHORITIES = Map.of( + "WEBAUTHN", FactorGrantedAuthority.WEBAUTHN_AUTHORITY, + "PASSWORD", FactorGrantedAuthority.PASSWORD_AUTHORITY, + "OTT", FactorGrantedAuthority.OTT_AUTHORITY, + "AUTHORIZATION_CODE", FactorGrantedAuthority.AUTHORIZATION_CODE_AUTHORITY, + "SAML_RESPONSE", FactorGrantedAuthority.SAML_RESPONSE_AUTHORITY, + "CAS", FactorGrantedAuthority.CAS_AUTHORITY, + "X509", FactorGrantedAuthority.X509_AUTHORITY, + "BEARER", FactorGrantedAuthority.BEARER_AUTHORITY); + + /** + * Whether the framework registers its built-in step-up service. When false (default), no {@code StepUpService} bean + * is created and behavior is unchanged: {@code POST /user/setPassword} stays governed by + * {@code user.security.allowInitialPasswordSetWithoutStepUp}, and passkey delete/rename keep their current-password + * check with no additional requirement for passwordless accounts. A consumer-supplied {@code StepUpService} bean + * still takes precedence when this is true. + */ + private boolean enabled = false; + + /** + * How recently the factor must have been issued, in seconds. The ceremony immediately precedes the operation, so + * the default is deliberately short: a longer window widens the period in which an attacker sharing the session can + * piggyback on the legitimate user's ceremony. + */ + @Min(1) + private int ttlSeconds = 120; + + /** + * How recently the user must have authenticated, by any means, to register a new passkey. Separate from + * {@link #ttlSeconds} and deliberately longer: the step-up ceremony immediately precedes the operation, whereas + * enrollment usually follows a login, a look around the settings page, and a decision. + * + *

+ * The gate applies only while step-up is {@link #enabled}. It exists because enrolling a passkey is what turns a + * stolen session into durable access: the credential outlives a password change, and asserting with it refreshes + * {@code FACTOR_WEBAUTHN}, which would otherwise let an attacker satisfy step-up with an authenticator they + * enrolled seconds earlier. Note the residual: within this window of a genuine login, a concurrent attacker on + * the same session can still enroll. + *

+ */ + @Min(1) + private int enrollmentTtlSeconds = 600; + + /** + * Factors that satisfy step-up, any one of which is sufficient. Values are the keys of + * {@link #FACTOR_AUTHORITIES}; unknown values fail startup. Defaults to {@code WEBAUTHN} alone, which is the only + * factor whose refresh reliably proves user presence: re-running an OAuth2 login, for instance, typically completes + * with no user interaction when the identity provider session is still alive. + */ + private List factors = new ArrayList<>(List.of("WEBAUTHN")); +} diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/StepUpService.java b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpService.java index f327eaaa..9584f8f9 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/StepUpService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpService.java @@ -46,4 +46,30 @@ public interface StepUpService { * @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); + + /** + * Reports whether this user could satisfy step-up at all, regardless of whether they have done so. + * + *

+ * Step-up denies a user who has not recently proven presence, on the assumption that they can go and do so. That + * assumption fails for a user who holds no credential this implementation accepts: a social-login (OAuth2/OIDC) + * account with no passkey can never produce a WebAuthn factor, so denying it is a dead end rather than a prompt. + * Callers treat {@code false} as "step-up does not apply here" and fall back to their configured default, instead + * of rejecting an operation the user could never unlock. + *

+ * + *

+ * The default returns {@code true}, preserving the behavior of implementations written before this method existed: + * every user is expected to be able to satisfy step-up. Override it when your mechanism depends on a credential + * some accounts may not have. + *

+ * + * @param user the authenticated user the operation targets (never {@code null}) + * @param action the same action identifier passed to {@link #isStepUpSatisfied}, so an implementation whose + * requirements differ per operation can answer per operation + * @return {@code true} if the user could satisfy step-up; {@code false} if no amount of user action would + */ + default boolean canSatisfyStepUp(User user, String action) { + return true; + } } diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java b/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java index 15e7b655..aa71d663 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java @@ -1,5 +1,7 @@ package com.digitalsanctuary.spring.user.security; +import java.time.Clock; +import java.time.Duration; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -12,6 +14,7 @@ import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.core.env.Environment; +import org.springframework.security.web.access.intercept.RequestAuthorizationContext; import org.springframework.http.HttpMethod; import org.springframework.security.config.ObjectPostProcessor; import org.springframework.security.config.annotation.web.builders.HttpSecurity; @@ -65,6 +68,7 @@ public class WebSecurityConfig { private final DSOAuth2UserService dsOAuth2UserService; private final DSOidcUserService dsOidcUserService; private final WebAuthnConfigProperties webAuthnConfigProperties; + private final StepUpConfigProperties stepUpConfigProperties; private final MfaConfigProperties mfaConfigProperties; private final Environment environment; private final ApplicationEventPublisher applicationEventPublisher; @@ -179,6 +183,19 @@ public SecurityFilterChain buildSecurityFilterChain(HttpSecurity http, SessionRe if (webAuthnConfigProperties.isEnabled()) { http.authorizeHttpRequests( (authorize) -> authorize.requestMatchers(HttpMethod.DELETE, "/webauthn/register/**").denyAll()); + + // Gate passkey enrollment on a recent authentication of any kind, the way GitHub asks for a password + // before adding a security key. Enrolling is what turns a stolen session into durable access: the new + // credential outlives a password change, and asserting with it refreshes FACTOR_WEBAUTHN, which would + // otherwise let an attacker satisfy step-up with an authenticator they enrolled seconds earlier. Only + // active with step-up, so the opt-in contract holds. Registered before anyRequest() below. + if (stepUpConfigProperties.isEnabled()) { + FreshFactorAuthorizationManager enrollmentGate = + new FreshFactorAuthorizationManager<>( + Duration.ofSeconds(stepUpConfigProperties.getEnrollmentTtlSeconds()), Clock.systemUTC()); + http.authorizeHttpRequests((authorize) -> authorize + .requestMatchers(HttpMethod.POST, "/webauthn/register").access(enrollmentGate)); + } } // Configure authorization rules based on the default action diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/AuthWithoutPasswordFactor.java b/src/main/java/com/digitalsanctuary/spring/user/service/AuthWithoutPasswordFactor.java new file mode 100644 index 00000000..e6fc69a1 --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/service/AuthWithoutPasswordFactor.java @@ -0,0 +1,65 @@ +package com.digitalsanctuary.spring.user.service; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.authority.FactorGrantedAuthority; + +/** + * Names the authentication factor each password-less login path actually represents. + * + *

+ * {@link UserService#authWithoutPassword} builds its own {@code Authentication} rather than running a Spring + * Security {@code AuthenticationProvider}, so nothing stamps a {@link FactorGrantedAuthority} on the session. A + * session with no factor cannot satisfy any freshness check, which would leave a just-registered user unable to + * enroll their first passkey until they logged out and back in. + *

+ * + *

+ * The factor is chosen per call site rather than defaulted, because the paths prove different things: clicking an + * emailed verification link is a one-time token, auto-login straight after registration follows a password the user + * submitted in that same request, and dev login proves nothing at all. + *

+ */ +public enum AuthWithoutPasswordFactor { + + /** Clicking the emailed verification link. Possession of a one-time token. */ + EMAIL_VERIFICATION(FactorGrantedAuthority.OTT_AUTHORITY), + + /** Auto-login immediately after registration, when email verification is disabled. */ + REGISTRATION(FactorGrantedAuthority.PASSWORD_AUTHORITY), + + /** Dev-only impersonation ({@code local} profile). Proves nothing about presence, so stamps nothing. */ + DEV_LOGIN(null); + + private final String authority; + + AuthWithoutPasswordFactor(String authority) { + this.authority = authority; + } + + /** + * Returns the factor authority this path stamps. + * + * @return the authority string, or {@code null} when the path stamps no factor + */ + public String getAuthority() { + return authority; + } + + /** + * Returns the given authorities plus a freshly issued factor for this path. + * + * @param authorities the authorities resolved for the user + * @return a new list with the factor appended, or the authorities unchanged when this path stamps none + */ + public List withFactor(Collection authorities) { + if (authority == null) { + return List.copyOf(authorities); + } + List result = new ArrayList<>(authorities); + result.add(FactorGrantedAuthority.fromAuthority(authority)); + return List.copyOf(result); + } +} diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/LoginFactorStamper.java b/src/main/java/com/digitalsanctuary/spring/user/service/LoginFactorStamper.java new file mode 100644 index 00000000..0a1206cf --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/service/LoginFactorStamper.java @@ -0,0 +1,47 @@ +package com.digitalsanctuary.spring.user.service; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.authority.FactorGrantedAuthority; + +/** + * Ensures a successful login leaves a freshness signal on the session. + * + *

+ * Spring Security stamps a {@link FactorGrantedAuthority} on some login flows but not all. Verified against 7.1.0: + * the password provider stamps {@code FACTOR_PASSWORD} and {@code OAuth2LoginAuthenticationProvider} stamps + * {@code FACTOR_AUTHORIZATION_CODE}, but {@code OidcAuthorizationCodeAuthenticationProvider} stamps nothing. An OIDC + * login therefore produces a session with no factor at all, which can never satisfy a freshness requirement. + *

+ * + *

+ * The test is whether a factor is already present, not what kind of authentication this is. On both the OAuth2 and + * the OIDC path this framework's user services return a {@code DSUserDetails} carrying database-derived authorities, + * so the usual {@code OidcUserAuthority} versus {@code OAuth2UserAuthority} discriminator does not survive here. + * Checking for the factor itself is both simpler and correct for any future flow that leaves one out. + *

+ */ +public final class LoginFactorStamper { + + private LoginFactorStamper() { + } + + /** + * Returns the given authorities, adding a freshly issued {@code FACTOR_AUTHORIZATION_CODE} only when no factor + * is present at all. + * + * @param authorities the authorities the login produced + * @return the authorities, with a factor guaranteed present + */ + public static List ensureFactor(Collection authorities) { + boolean alreadyStamped = authorities.stream().anyMatch(FactorGrantedAuthority.class::isInstance); + if (alreadyStamped) { + return List.copyOf(authorities); + } + List result = new ArrayList<>(authorities); + result.add(FactorGrantedAuthority.fromAuthority(FactorGrantedAuthority.AUTHORIZATION_CODE_AUTHORITY)); + return List.copyOf(result); + } +} diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/LoginSuccessService.java b/src/main/java/com/digitalsanctuary/spring/user/service/LoginSuccessService.java index 52d2b232..c6c52d04 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/LoginSuccessService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/LoginSuccessService.java @@ -1,8 +1,17 @@ package com.digitalsanctuary.spring.user.service; import java.io.IOException; +import java.util.List; import org.springframework.context.ApplicationEventPublisher; import org.springframework.security.core.Authentication; +import org.springframework.security.web.context.SecurityContextRepository; +import org.springframework.security.web.context.HttpSessionSecurityContextRepository; +import org.springframework.security.oauth2.core.user.OAuth2User; +import org.springframework.security.oauth2.client.authentication.OAuth2AuthenticationToken; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.security.core.context.SecurityContext; +import org.springframework.security.core.authority.FactorGrantedAuthority; +import org.springframework.security.core.GrantedAuthority; import org.springframework.security.web.authentication.SavedRequestAwareAuthenticationSuccessHandler; import org.springframework.security.web.savedrequest.RequestCache; import org.springframework.stereotype.Service; @@ -34,6 +43,9 @@ public class LoginSuccessService extends SavedRequestAwareAuthenticationSuccessH /** The event publisher. */ private final ApplicationEventPublisher eventPublisher; + /** Writes the replaced context back; the filter already saved the original before this handler runs. */ + private final SecurityContextRepository securityContextRepository = new HttpSessionSecurityContextRepository(); + /** The user security configuration properties. */ private final UserSecurityConfigProperties userSecurityConfig; @@ -88,6 +100,8 @@ public void onAuthenticationSuccess(HttpServletRequest request, HttpServletRespo log.debug("LoginSuccessService.onAuthenticationSuccess: targetUrl: {}", super.determineTargetUrl(request, response)); + stampFactorIfMissing(request, response, authentication); + User user = null; if (authentication != null && authentication.getPrincipal() != null) { log.debug("LoginSuccessService.onAuthenticationSuccess() user: {}", authentication.getName()); @@ -149,4 +163,50 @@ public void onAuthenticationSuccess(HttpServletRequest request, HttpServletRespo log.debug("After super.onAuthenticationSuccess - if you see this, no redirect happened"); } + + /** + * Guarantees the session carries an authentication factor, so a freshness check has something to read. + * + *

+ * {@code OidcAuthorizationCodeAuthenticationProvider} stamps no {@link FactorGrantedAuthority}, unlike the + * password and plain-OAuth2 providers, so an OIDC login would otherwise leave a session that can never satisfy + * step-up or the passkey-enrollment gate. Only an entirely unstamped authentication is touched, so the flows that + * already stamp are left exactly as they were. + *

+ * + *

+ * {@code AbstractAuthenticationProcessingFilter} saves the context before invoking this handler, so a replacement + * has to be written back to the repository explicitly, the same way {@code WebAuthnAuthenticationSuccessHandler} + * does after its principal swap. + *

+ */ + private void stampFactorIfMissing(HttpServletRequest request, HttpServletResponse response, Authentication authentication) { + if (authentication == null || !authentication.isAuthenticated()) { + return; + } + List withFactor = LoginFactorStamper.ensureFactor(authentication.getAuthorities()); + if (withFactor.size() == authentication.getAuthorities().size()) { + return; + } + + // Only OAuth2AuthenticationToken is rebuilt: OIDC login is the unstamped flow this exists for, and it + // produces that type. Guessing how to reconstruct an unknown token type risks losing state that matters more + // than the factor, so anything else is left alone and logged. + if (!(authentication instanceof OAuth2AuthenticationToken oauthToken) + || !(authentication.getPrincipal() instanceof OAuth2User oauth2User)) { + log.warn("LoginSuccessService: {} carries no authentication factor and cannot be re-stamped; " + + "step-up and passkey enrollment will not be available to this session", + authentication.getClass().getSimpleName()); + return; + } + Authentication stamped = new OAuth2AuthenticationToken(oauth2User, withFactor, + oauthToken.getAuthorizedClientRegistrationId()); + SecurityContext context = SecurityContextHolder.getContext(); + context.setAuthentication(stamped); + SecurityContextHolder.setContext(context); + securityContextRepository.saveContext(context, request, response); + log.debug("LoginSuccessService: stamped an authorization-code factor on an otherwise unstamped login for {}", + authentication.getName()); + } + } diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java b/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java index e7d14919..d42406d0 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java @@ -1127,6 +1127,23 @@ public List getUsersFromSessionRegistry() { * @param user The user to authenticate without password verification */ public void authWithoutPassword(User user) { + // Stamps nothing, preserving the behavior of callers written before the factor overload existed. + authWithoutPassword(user, AuthWithoutPasswordFactor.DEV_LOGIN); + } + + /** + * Authenticates a user without a password, stamping the authentication factor the calling path represents. + * + *

+ * This path builds its own {@code Authentication} rather than running an {@code AuthenticationProvider}, so + * nothing stamps a {@code FactorGrantedAuthority} unless it is done here. A session with no factor cannot satisfy + * any freshness check, which would leave a just-registered user unable to enroll their first passkey. + *

+ * + * @param user the user to authenticate + * @param factor the factor this login path genuinely represents; see {@link AuthWithoutPasswordFactor} + */ + public void authWithoutPassword(User user, AuthWithoutPasswordFactor factor) { log.debug("UserService.authWithoutPassword: authenticating user: {}", user != null ? user.getEmail() : null); if (user == null || user.getEmail() == null) { log.error("Invalid user or user email"); @@ -1144,7 +1161,7 @@ public void authWithoutPassword(User user) { // Reuse the authorities already resolved by loadUserByUsername (which loads roles and privileges via the // entity-graph finder). The incoming `user` may be detached, so deriving authorities from it directly could // trigger a LazyInitializationException now that roles/privileges are lazily fetched. - Collection authorities = userDetails.getAuthorities(); + Collection authorities = factor.withFactor(userDetails.getAuthorities()); // Authenticate user authenticateUser(userDetails, authorities); diff --git a/src/main/resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports b/src/main/resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports index 4ff463fe..69d42400 100644 --- a/src/main/resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports +++ b/src/main/resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports @@ -1,5 +1,6 @@ com.digitalsanctuary.spring.user.UserConfiguration com.digitalsanctuary.spring.user.audit.AuditMailAutoConfiguration com.digitalsanctuary.spring.user.captcha.CaptchaAutoConfiguration +com.digitalsanctuary.spring.user.security.StepUpAutoConfiguration com.digitalsanctuary.spring.user.security.UserSecurityBeansAutoConfiguration com.digitalsanctuary.spring.user.security.WebSecurityFilterChainAutoConfiguration diff --git a/src/main/resources/config/dsspringuserconfig.properties b/src/main/resources/config/dsspringuserconfig.properties index 563f4092..edafb1f0 100644 --- a/src/main/resources/config/dsspringuserconfig.properties +++ b/src/main/resources/config/dsspringuserconfig.properties @@ -92,6 +92,21 @@ user.security.requireCanonicalAppUrl=false # If true, the test hash time will be logged to the console on startup. This is useful for determining the optimal bcryptStrength value. user.security.testHashTime=true +# Step-up (re-)authentication before credential-altering operations (SUF-02). When enabled, the framework registers a +# StepUpService that requires one of the configured factors to have been issued within stepUp.ttlSeconds. The user +# refreshes a factor by re-running that login ceremony (for WEBAUTHN, the passkey assertion) while already logged in; +# there is no separate step-up endpoint. A consumer-supplied StepUpService bean takes precedence over the built-in one. +user.security.stepUp.enabled=false +# How recently the factor must have been issued, in seconds. +user.security.stepUp.ttlSeconds=120 +# How recently the user must have authenticated, by any means, to register a new passkey. Applies only +# while step-up is enabled. Longer than ttlSeconds because enrollment follows a login and a decision. +user.security.stepUp.enrollmentTtlSeconds=600 +# Factors that satisfy step-up, any one of which is sufficient. Valid values: WEBAUTHN, PASSWORD, OTT, +# AUTHORIZATION_CODE, SAML_RESPONSE, CAS, X509, BEARER. Requiring a factor the deployment never issues makes the gated +# operations permanently unavailable. +user.security.stepUp.factors=WEBAUTHN + # Remember-me ("stay signed in") support. Disabled by default. To enable it you must set BOTH enabled=true and a # secret key, AND your login form must post the remember-me request parameter (a checkbox named "remember-me" by # default) - without the parameter no cookie is ever issued. 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 702b7b5d..2311cc50 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java @@ -31,6 +31,7 @@ import com.digitalsanctuary.spring.user.dto.UserDto; import com.digitalsanctuary.spring.user.dto.UserProfileUpdateDto; import com.digitalsanctuary.spring.user.security.StepUpService; +import com.digitalsanctuary.spring.user.service.AuthWithoutPasswordFactor; import com.digitalsanctuary.spring.user.security.UserSecurityConfigProperties; import java.util.List; import org.springframework.beans.factory.ObjectProvider; @@ -249,7 +250,7 @@ void registerUserAccount_success_withAutoLogin() throws Exception { .andExpect(jsonPath("$.redirectUrl").value("/user/registration-complete.html")); // Verify auto-login was called - verify(userService).authWithoutPassword(newUser); + verify(userService).authWithoutPassword(newUser, AuthWithoutPasswordFactor.REGISTRATION); } @Test @@ -818,6 +819,9 @@ void setPassword_stepUpService_deniesReturns401() throws Exception { when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser); when(userService.hasPassword(testUser)).thenReturn(false); StepUpService stepUp = mock(StepUpService.class); + // A mocked interface returns false for the unstubbed canSatisfyStepUp() default; a real implementation + // inherits true. Stub it so these cases stay about whether step-up was *satisfied*, not whether it applies. + when(stepUp.canSatisfyStepUp(testUser, "set-password")).thenReturn(true); 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))) @@ -842,6 +846,9 @@ void setPassword_stepUpService_grantsProceeds() throws Exception { when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser); when(userService.hasPassword(testUser)).thenReturn(false); StepUpService stepUp = mock(StepUpService.class); + // A mocked interface returns false for the unstubbed canSatisfyStepUp() default; a real implementation + // inherits true. Stub it so these cases stay about whether step-up was *satisfied*, not whether it applies. + when(stepUp.canSatisfyStepUp(testUser, "set-password")).thenReturn(true); 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))) @@ -856,6 +863,56 @@ void setPassword_stepUpService_grantsProceeds() throws Exception { verify(userService).setInitialPassword(testUser, "NewValidPass1!"); } + + @Test + @DisplayName("POST /user/setPassword - falls back to the opt-in flag when the user cannot satisfy step-up") + void setPassword_stepUpUnsatisfiable_fallsBackToDisabledDefault() throws Exception { + mockMvc = updatePasswordMockMvc(); + when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser); + when(userService.hasPassword(testUser)).thenReturn(false); + StepUpService stepUp = mock(StepUpService.class); + // A social-login account has no passkey, so it can never produce the configured factor. Step-up does + // not apply, and the endpoint falls back to allowInitialPasswordSetWithoutStepUp (false by default). + when(stepUp.canSatisfyStepUp(testUser, "set-password")).thenReturn(false); + ReflectionTestUtils.setField(userAPI, "stepUpServiceProvider", stepUpProvider(stepUp)); + 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)) + .andExpect(jsonPath("$.code").value(7)); + + verify(stepUp, never()).isStepUpSatisfied(any(), any(), any()); + verify(userService, never()).setInitialPassword(any(), any()); + } + + @Test + @DisplayName("POST /user/setPassword - proceeds when the user cannot satisfy step-up and the opt-in flag is enabled") + void setPassword_stepUpUnsatisfiable_allowedWhenFlagEnabled() throws Exception { + mockMvc = updatePasswordMockMvc(); + when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser); + when(userService.hasPassword(testUser)).thenReturn(false); + StepUpService stepUp = mock(StepUpService.class); + when(stepUp.canSatisfyStepUp(testUser, "set-password")).thenReturn(false); + ReflectionTestUtils.setField(userAPI, "stepUpServiceProvider", stepUpProvider(stepUp)); + userSecurityConfig.setAllowInitialPasswordSetWithoutStepUp(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(stepUp, never()).isStepUpSatisfied(any(), any(), any()); + verify(userService).setInitialPassword(testUser, "NewValidPass1!"); + } } @Nested diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIAdviceTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIAdviceTest.java index e87b5856..b9369eb4 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIAdviceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIAdviceTest.java @@ -8,6 +8,7 @@ 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.WebAuthnStepUpRequiredException; import com.digitalsanctuary.spring.user.exceptions.WebAuthnUserNotFoundException; import com.digitalsanctuary.spring.user.util.GenericResponse; @@ -36,6 +37,31 @@ void reauthenticationFailureMapsToUnauthorized() { assertThat(response.getStatusCode()).isEqualTo(HttpStatus.UNAUTHORIZED); } + @Test + @DisplayName("step-up required -> 401 Unauthorized with the step-up-required error code") + void stepUpRequiredMapsToUnauthorizedWithErrorCode() { + ResponseEntity response = advice + .handleStepUpRequired(new WebAuthnStepUpRequiredException("Recent authentication is required.")); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.UNAUTHORIZED); + // The distinct code is the whole reason this exception type exists: it tells a client to re-run its login + // ceremony rather than prompt for a password the account may not have. + assertThat(response.getBody().getError()).isEqualTo(WebAuthnStepUpRequiredException.ERROR_CODE); + } + + @Test + @DisplayName("step-up required is handled by its own mapping, not the base WebAuthnException one") + void stepUpRequiredDoesNotFallThroughToBaseHandler() { + // WebAuthnStepUpRequiredException extends WebAuthnException. If the specific @ExceptionHandler were removed, + // Spring would route it to handleWebAuthnError and the client would see 400 with a null error code. + ResponseEntity viaBase = + advice.handleWebAuthnError(new WebAuthnStepUpRequiredException("Recent authentication is required.")); + + assertThat(viaBase.getStatusCode()).isEqualTo(HttpStatus.BAD_REQUEST); + assertThat(advice.handleStepUpRequired(new WebAuthnStepUpRequiredException("x")).getStatusCode()) + .isEqualTo(HttpStatus.UNAUTHORIZED); + } + @Test @DisplayName("locked account -> 423 Locked") void accountLockedMapsToLocked() { diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIStepUpTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIStepUpTest.java new file mode 100644 index 00000000..48c914c1 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIStepUpTest.java @@ -0,0 +1,190 @@ +package com.digitalsanctuary.spring.user.api; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.springframework.beans.factory.ObjectProvider; +import org.springframework.context.ApplicationEventPublisher; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.security.core.userdetails.UserDetails; +import com.digitalsanctuary.spring.user.exceptions.WebAuthnStepUpRequiredException; +import com.digitalsanctuary.spring.user.persistence.model.User; +import com.digitalsanctuary.spring.user.security.StepUpConfigProperties; +import com.digitalsanctuary.spring.user.security.StepUpService; +import com.digitalsanctuary.spring.user.service.LoginAttemptService; +import com.digitalsanctuary.spring.user.service.UserService; +import com.digitalsanctuary.spring.user.service.WebAuthnCredentialManagementService; +import com.digitalsanctuary.spring.user.test.annotations.ServiceTest; +import com.digitalsanctuary.spring.user.test.fixtures.TestFixtures; + +/** + * Step-up enforcement on the credential-altering passkey endpoints. + *

+ * These endpoints previously had no gate at all for passwordless accounts: the password check returns + * immediately when there is no password, so a session alone authorized deleting or renaming a passkey. Step-up closes + * that, and only when a {@code StepUpService} is configured, so deployments that have not enabled it are unaffected. + *

+ */ +@ServiceTest +@DisplayName("WebAuthnManagementAPI Step-Up Tests") +class WebAuthnManagementAPIStepUpTest { + + @Mock + private WebAuthnCredentialManagementService credentialManagementService; + + @Mock + private UserService userService; + + @Mock + private ApplicationEventPublisher eventPublisher; + + @Mock + private LoginAttemptService loginAttemptService; + + @Mock + private UserDetails userDetails; + + @Mock + private ObjectProvider stepUpServiceProvider; + + @Mock + private StepUpService stepUpService; + + private WebAuthnManagementAPI api; + + private StepUpConfigProperties stepUpConfig; + + private User testUser; + + private final MockHttpServletRequest request = new MockHttpServletRequest(); + + @BeforeEach + void setUp() { + testUser = TestFixtures.Users.standardUser(); + stepUpConfig = new StepUpConfigProperties(); + // These tests are about a deployment that has switched step-up on; the disabled case is covered explicitly. + stepUpConfig.setEnabled(true); + api = new WebAuthnManagementAPI(credentialManagementService, userService, eventPublisher, loginAttemptService, + stepUpServiceProvider, stepUpConfig); + when(userDetails.getUsername()).thenReturn(testUser.getEmail()); + when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser); + // Passwordless (passkey-only) account: the case step-up exists for. + when(userService.hasPassword(testUser)).thenReturn(false); + } + + @Nested + @DisplayName("Step-up configured") + class StepUpConfiguredTests { + + @BeforeEach + void provideStepUpService() { + when(stepUpServiceProvider.getIfAvailable()).thenReturn(stepUpService); + } + + @Test + @DisplayName("should reject passkey deletion when step-up is not satisfied") + void shouldRejectDeleteWithoutStepUp() { + when(stepUpService.canSatisfyStepUp(eq(testUser), any())).thenReturn(true); + when(stepUpService.isStepUpSatisfied(eq(testUser), any(), any())).thenReturn(false); + + assertThatThrownBy(() -> api.deleteCredential("cred-1", null, userDetails, request)) + .isInstanceOf(WebAuthnStepUpRequiredException.class); + verify(credentialManagementService, never()).deleteCredential(any(), any()); + } + + @Test + @DisplayName("should proceed with passkey deletion when the account cannot satisfy step-up at all") + void shouldProceedWhenStepUpIsUnsatisfiable() { + // factors=[PASSWORD] against a passwordless account: no user action could ever produce the factor, so + // gating would be a permanent dead end rather than a prompt. Fall back to the pre-feature behavior. + when(stepUpService.canSatisfyStepUp(eq(testUser), any())).thenReturn(false); + + api.deleteCredential("cred-1", null, userDetails, request); + + verify(stepUpService, never()).isStepUpSatisfied(any(), any(), any()); + verify(credentialManagementService).deleteCredential("cred-1", testUser); + } + + @Test + @DisplayName("should reject passkey rename when step-up is not satisfied") + void shouldRejectRenameWithoutStepUp() { + when(stepUpService.canSatisfyStepUp(eq(testUser), any())).thenReturn(true); + when(stepUpService.isStepUpSatisfied(eq(testUser), any(), any())).thenReturn(false); + + assertThatThrownBy(() -> api.renameCredential("cred-1", + new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop", null), userDetails, request)) + .isInstanceOf(WebAuthnStepUpRequiredException.class); + verify(credentialManagementService, never()).renameCredential(any(), any(), any()); + } + + @Test + @DisplayName("should allow passkey deletion when step-up is satisfied") + void shouldAllowDeleteWithStepUp() { + when(stepUpService.canSatisfyStepUp(eq(testUser), any())).thenReturn(true); + when(stepUpService.isStepUpSatisfied(eq(testUser), any(), any())).thenReturn(true); + + assertThat(api.deleteCredential("cred-1", null, userDetails, request).getStatusCode().is2xxSuccessful()).isTrue(); + verify(credentialManagementService).deleteCredential("cred-1", testUser); + } + + @Test + @DisplayName("should pass a distinct action per operation so implementations can distinguish them") + void shouldPassPerOperationAction() { + when(stepUpService.canSatisfyStepUp(eq(testUser), any())).thenReturn(true); + when(stepUpService.isStepUpSatisfied(eq(testUser), any(), any())).thenReturn(true); + + api.deleteCredential("cred-1", null, userDetails, request); + api.renameCredential("cred-1", new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop", null), userDetails, request); + + ArgumentCaptor actions = ArgumentCaptor.forClass(String.class); + verify(stepUpService, times(2)).isStepUpSatisfied(eq(testUser), actions.capture(), any()); + assertThat(actions.getAllValues()).containsExactly("delete-passkey", "rename-passkey"); + } + } + + @Nested + @DisplayName("Step-up not configured") + class StepUpAbsentTests { + + @Test + @DisplayName("should allow passkey deletion on a passwordless account, unchanged from before step-up existed") + void shouldAllowDeleteWhenNoStepUpServiceIsConfigured() { + when(stepUpServiceProvider.getIfAvailable()).thenReturn(null); + + assertThat(api.deleteCredential("cred-1", null, userDetails, request).getStatusCode().is2xxSuccessful()).isTrue(); + verify(credentialManagementService).deleteCredential("cred-1", testUser); + } + } + + @Nested + @DisplayName("Step-up bean present but not enabled") + class StepUpNotEnabledTests { + + @Test + @DisplayName("should not gate passkey deletion for a consumer SPI bean when step-up is not enabled") + void shouldNotGateWhenStepUpDisabled() { + // Applications that adopted the StepUpService SPI in 5.3.1 wired it for setPassword, the only thing it + // gated then. Keying these endpoints off bean presence would newly enforce it for them on upgrade, + // against an implementation never written for these two action values. The opt-in is the property. + stepUpConfig.setEnabled(false); + + api.deleteCredential("cred-1", null, userDetails, request); + + verify(stepUpService, never()).isStepUpSatisfied(any(), any(), any()); + verify(stepUpService, never()).canSatisfyStepUp(any(), any()); + verify(credentialManagementService).deleteCredential("cred-1", testUser); + } + } + +} 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 dc91f61d..bfb5e85f 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java @@ -17,7 +17,9 @@ import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.mockito.InjectMocks; +import org.mockito.Spy; import org.mockito.Mock; +import org.springframework.beans.factory.ObjectProvider; import org.springframework.context.ApplicationEventPublisher; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; @@ -29,6 +31,8 @@ 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.security.StepUpConfigProperties; +import com.digitalsanctuary.spring.user.security.StepUpService; import com.digitalsanctuary.spring.user.service.LoginAttemptService; import com.digitalsanctuary.spring.user.service.UserService; import com.digitalsanctuary.spring.user.service.WebAuthnCredentialManagementService; @@ -57,6 +61,23 @@ class WebAuthnManagementAPITest { @Mock private UserDetails userDetails; + /** + * Left unstubbed on purpose: {@code getIfAvailable()} returns null, which is the shape of a deployment with step-up + * disabled and no consumer-supplied service. Step-up enforcement itself is covered in + * {@code WebAuthnManagementAPIStepUpTest}. + */ + @Mock + private ObjectProvider stepUpServiceProvider; + + /** Passed to the endpoints that now accept the current request; its content is never read on these paths. */ + @Mock + private HttpServletRequest httpRequest; + + // A real instance rather than a mock: defaults to enabled=false, which is exactly the shape this class tests, + // a deployment that has not switched step-up on. @InjectMocks fills @Spy fields as well as @Mock ones. + @Spy + private StepUpConfigProperties stepUpConfigProperties = new StepUpConfigProperties(); + @InjectMocks private WebAuthnManagementAPI api; @@ -175,7 +196,7 @@ void shouldRenameSuccessfullyWhenAccountPasswordless() { WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop", null); // When - ResponseEntity response = api.renameCredential("cred-1", request, userDetails); + ResponseEntity response = api.renameCredential("cred-1", request, userDetails, httpRequest); // Then assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK); @@ -194,7 +215,7 @@ void shouldRenameSuccessfullyWhenCorrectCurrentPassword() { new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop", "currentPass"); // When - ResponseEntity response = api.renameCredential("cred-1", request, userDetails); + ResponseEntity response = api.renameCredential("cred-1", request, userDetails, httpRequest); // Then assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK); @@ -215,7 +236,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(WebAuthnAccountLockedException.class) + assertThatThrownBy(() -> api.renameCredential("cred-1", request, userDetails, httpRequest)).isInstanceOf(WebAuthnAccountLockedException.class) .hasMessageContaining("locked"); verify(credentialManagementService, never()).renameCredential(any(), any(), any()); verify(userService, never()).checkIfValidOldPassword(any(), any()); @@ -230,7 +251,7 @@ void shouldRejectRenameWhenCurrentPasswordMissing() { WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop", null); // When & Then - assertThatThrownBy(() -> api.renameCredential("cred-1", request, userDetails)).isInstanceOf(WebAuthnException.class) + assertThatThrownBy(() -> api.renameCredential("cred-1", request, userDetails, httpRequest)).isInstanceOf(WebAuthnException.class) .hasMessageContaining("Current password is required"); verify(credentialManagementService, never()).renameCredential(any(), any(), any()); // A missing field is a client error, not a password guess, so it must not count toward lockout. @@ -248,7 +269,7 @@ void shouldRejectRenameWhenCurrentPasswordIncorrect() { new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop", "wrongPass"); // When & Then - assertThatThrownBy(() -> api.renameCredential("cred-1", request, userDetails)).isInstanceOf(WebAuthnReauthenticationException.class) + assertThatThrownBy(() -> api.renameCredential("cred-1", request, userDetails, httpRequest)).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. @@ -266,7 +287,7 @@ void shouldThrowOnFailure() { .renameCredential(eq("cred-999"), eq("New Name"), any(User.class)); // When - assertThatThrownBy(() -> api.renameCredential("cred-999", request, userDetails)).isInstanceOf(WebAuthnException.class) + assertThatThrownBy(() -> api.renameCredential("cred-999", request, userDetails, httpRequest)).isInstanceOf(WebAuthnException.class) .hasMessageContaining("not found"); } @@ -278,7 +299,7 @@ void shouldThrowNotFoundWhenUserNotFound() { when(userService.findUserByEmail(testUser.getEmail())).thenReturn(null); // When - assertThatThrownBy(() -> api.renameCredential("cred-1", request, userDetails)) + assertThatThrownBy(() -> api.renameCredential("cred-1", request, userDetails, httpRequest)) .isInstanceOf(WebAuthnUserNotFoundException.class).hasMessageContaining("User not found"); verify(credentialManagementService, never()).renameCredential(any(), any(), any()); } @@ -296,7 +317,7 @@ void shouldDeleteSuccessfullyWhenAccountPasswordless() { when(userService.hasPassword(testUser)).thenReturn(false); // When - ResponseEntity response = api.deleteCredential("cred-1", null, userDetails); + ResponseEntity response = api.deleteCredential("cred-1", null, userDetails, httpRequest); // Then assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK); @@ -314,7 +335,7 @@ void shouldDeleteSuccessfullyWhenCorrectCurrentPassword() { WebAuthnManagementAPI.CurrentPasswordRequest request = new WebAuthnManagementAPI.CurrentPasswordRequest("currentPass"); // When - ResponseEntity response = api.deleteCredential("cred-1", request, userDetails); + ResponseEntity response = api.deleteCredential("cred-1", request, userDetails, httpRequest); // Then assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK); @@ -329,7 +350,7 @@ void shouldRejectDeleteWhenCurrentPasswordMissing() { when(userService.hasPassword(testUser)).thenReturn(true); // When & Then - assertThatThrownBy(() -> api.deleteCredential("cred-1", null, userDetails)).isInstanceOf(WebAuthnException.class) + assertThatThrownBy(() -> api.deleteCredential("cred-1", null, userDetails, httpRequest)).isInstanceOf(WebAuthnException.class) .hasMessageContaining("Current password is required"); verify(credentialManagementService, never()).deleteCredential(any(), any()); } @@ -344,7 +365,7 @@ void shouldRejectDeleteWhenCurrentPasswordIncorrect() { WebAuthnManagementAPI.CurrentPasswordRequest request = new WebAuthnManagementAPI.CurrentPasswordRequest("wrongPass"); // When & Then - assertThatThrownBy(() -> api.deleteCredential("cred-1", request, userDetails)).isInstanceOf(WebAuthnReauthenticationException.class) + assertThatThrownBy(() -> api.deleteCredential("cred-1", request, userDetails, httpRequest)).isInstanceOf(WebAuthnReauthenticationException.class) .hasMessageContaining("Current password is incorrect"); verify(credentialManagementService, never()).deleteCredential(any(), any()); } @@ -359,7 +380,7 @@ void shouldThrowOnFailure() { any(User.class)); // When - assertThatThrownBy(() -> api.deleteCredential("cred-1", null, userDetails)).isInstanceOf(WebAuthnException.class) + assertThatThrownBy(() -> api.deleteCredential("cred-1", null, userDetails, httpRequest)).isInstanceOf(WebAuthnException.class) .hasMessageContaining("Cannot delete last passkey"); } @@ -370,7 +391,7 @@ void shouldThrowNotFoundWhenUserNotFound() { when(userService.findUserByEmail(testUser.getEmail())).thenReturn(null); // When - assertThatThrownBy(() -> api.deleteCredential("cred-1", null, userDetails)) + assertThatThrownBy(() -> api.deleteCredential("cred-1", null, userDetails, httpRequest)) .isInstanceOf(WebAuthnUserNotFoundException.class).hasMessageContaining("User not found"); verify(credentialManagementService, never()).deleteCredential(any(), any()); } 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 7299b257..29b9a740 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/controller/UserActionControllerTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/controller/UserActionControllerTest.java @@ -10,6 +10,7 @@ import com.digitalsanctuary.spring.user.audit.AuditEvent; import com.digitalsanctuary.spring.user.persistence.model.User; +import com.digitalsanctuary.spring.user.service.AuthWithoutPasswordFactor; import com.digitalsanctuary.spring.user.security.UserSecurityConfigProperties; import com.digitalsanctuary.spring.user.service.UserService; import com.digitalsanctuary.spring.user.service.UserService.TokenValidationResult; @@ -221,7 +222,9 @@ void confirmRegistration_validToken_confirmsAndAuthenticatesUser() throws Except // Verify interactions. The token is consumed atomically inside validateVerificationToken, so the // controller no longer issues a separate deleteVerificationToken call. - verify(userService).authWithoutPassword(testUser); + // Pinned per path: the verification link is possession of a one-time token, and the stamped + // factor is what lets the just-verified user enroll a passkey without logging out first. + verify(userService).authWithoutPassword(testUser, AuthWithoutPasswordFactor.EMAIL_VERIFICATION); verify(userVerificationService, never()).deleteVerificationToken(anyString()); // Verify audit event diff --git a/src/test/java/com/digitalsanctuary/spring/user/dev/DevLoginControllerTest.java b/src/test/java/com/digitalsanctuary/spring/user/dev/DevLoginControllerTest.java index 508bb85d..d21ab6be 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/dev/DevLoginControllerTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/dev/DevLoginControllerTest.java @@ -16,6 +16,7 @@ import org.springframework.http.ResponseEntity; import com.digitalsanctuary.spring.user.persistence.model.User; import com.digitalsanctuary.spring.user.persistence.repository.UserRepository; +import com.digitalsanctuary.spring.user.service.AuthWithoutPasswordFactor; import com.digitalsanctuary.spring.user.service.UserService; import com.digitalsanctuary.spring.user.test.annotations.ServiceTest; import com.digitalsanctuary.spring.user.test.builders.UserTestDataBuilder; @@ -56,7 +57,8 @@ void shouldAuthenticateAndRedirectWhenValidUser() { ResponseEntity result = devLoginController.loginAs("dev@test.com"); // Then - verify(userService).authWithoutPassword(enabledUser); + // Dev login stamps no factor: impersonation proves nothing about presence. + verify(userService).authWithoutPassword(enabledUser, AuthWithoutPasswordFactor.DEV_LOGIN); assertThat(result.getStatusCode()).isEqualTo(HttpStatus.FOUND); assertThat(result.getHeaders().getFirst("Location")).isEqualTo("/dashboard"); assertThat(result.getBody()).isNull(); @@ -75,7 +77,7 @@ void shouldReturn404WhenUserNotFound() { assertThat(result.getStatusCode()).isEqualTo(HttpStatus.NOT_FOUND); assertThat(result.getBody()).isNotNull(); assertThat(result.getBody().isSuccess()).isFalse(); - verify(userService, never()).authWithoutPassword(any()); + verify(userService, never()).authWithoutPassword(any(), any()); } @Test @@ -91,7 +93,7 @@ void shouldReturn403WhenUserDisabled() { assertThat(result.getStatusCode()).isEqualTo(HttpStatus.FORBIDDEN); assertThat(result.getBody()).isNotNull(); assertThat(result.getBody().isSuccess()).isFalse(); - verify(userService, never()).authWithoutPassword(any()); + verify(userService, never()).authWithoutPassword(any(), any()); } @Test diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpServiceTest.java new file mode 100644 index 00000000..5c3ac9d4 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpServiceTest.java @@ -0,0 +1,214 @@ +package com.digitalsanctuary.spring.user.security; + +import static org.assertj.core.api.Assertions.assertThat; +import java.time.Instant; +import java.util.List; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.security.authentication.TestingAuthenticationToken; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.authority.FactorGrantedAuthority; +import org.springframework.security.core.authority.SimpleGrantedAuthority; +import org.springframework.security.core.context.SecurityContextHolder; +import com.digitalsanctuary.spring.user.persistence.model.User; +import jakarta.servlet.http.HttpServletRequest; + +/** + * Tests for {@link DSFactorFreshnessStepUpService}. + */ +@DisplayName("DSFactorFreshnessStepUpService Tests") +class DSFactorFreshnessStepUpServiceTest { + + private static final String EMAIL = "passkey-user@test.com"; + private static final String ACTION = "set-password"; + + private final HttpServletRequest request = new MockHttpServletRequest(); + + private User user; + + @BeforeEach + void setUp() { + user = new User(); + user.setEmail(EMAIL); + SecurityContextHolder.clearContext(); + } + + @Nested + @DisplayName("Freshness") + class FreshnessTests { + + @Test + @DisplayName("should satisfy step-up when the required factor was issued inside the window") + void shouldSatisfyWhenFactorIsFresh() { + authenticate(EMAIL, webAuthnFactor(Instant.now())); + + assertThat(service(120, "WEBAUTHN").isStepUpSatisfied(user, ACTION, request)).isTrue(); + } + + @Test + @DisplayName("should deny step-up when the required factor was issued before the window") + void shouldDenyWhenFactorIsStale() { + authenticate(EMAIL, webAuthnFactor(Instant.now().minusSeconds(121))); + + assertThat(service(120, "WEBAUTHN").isStepUpSatisfied(user, ACTION, request)).isFalse(); + } + + @Test + @DisplayName("should deny step-up when the session carries no factor of the required kind") + void shouldDenyWhenFactorIsAbsent() { + authenticate(EMAIL, new SimpleGrantedAuthority("ROLE_USER"), + FactorGrantedAuthority.withAuthority(FactorGrantedAuthority.PASSWORD_AUTHORITY).issuedAt(Instant.now()).build()); + + assertThat(service(120, "WEBAUTHN").isStepUpSatisfied(user, ACTION, request)).isFalse(); + } + + @Test + @DisplayName("should honor the configured TTL rather than a fixed one") + void shouldHonorConfiguredTtl() { + authenticate(EMAIL, webAuthnFactor(Instant.now().minusSeconds(200))); + + assertThat(service(120, "WEBAUTHN").isStepUpSatisfied(user, ACTION, request)).as("200s old against a 120s TTL").isFalse(); + assertThat(service(600, "WEBAUTHN").isStepUpSatisfied(user, ACTION, request)).as("200s old against a 600s TTL").isTrue(); + } + } + + @Nested + @DisplayName("Configured factors") + class FactorSelectionTests { + + @Test + @DisplayName("should accept any one of the configured factors") + void shouldAcceptAnyConfiguredFactor() { + authenticate(EMAIL, + FactorGrantedAuthority.withAuthority(FactorGrantedAuthority.PASSWORD_AUTHORITY).issuedAt(Instant.now()).build()); + + assertThat(service(120, "WEBAUTHN", "PASSWORD").isStepUpSatisfied(user, ACTION, request)) + .as("a fresh PASSWORD factor satisfies a WEBAUTHN-or-PASSWORD configuration").isTrue(); + assertThat(service(120, "WEBAUTHN").isStepUpSatisfied(user, ACTION, request)) + .as("but not a WEBAUTHN-only configuration").isFalse(); + } + } + + @Nested + @DisplayName("Caller identity") + class CallerIdentityTests { + + @Test + @DisplayName("should deny step-up when there is no authentication in the context") + void shouldDenyWhenUnauthenticated() { + assertThat(service(120, "WEBAUTHN").isStepUpSatisfied(user, ACTION, request)).isFalse(); + } + + @Test + @DisplayName("should deny step-up when the authentication is not authenticated") + void shouldDenyWhenAuthenticationIsNotAuthenticated() { + TestingAuthenticationToken token = new TestingAuthenticationToken(EMAIL, "n/a", List.of(webAuthnFactor(Instant.now()))); + token.setAuthenticated(false); + SecurityContextHolder.getContext().setAuthentication(token); + + assertThat(service(120, "WEBAUTHN").isStepUpSatisfied(user, ACTION, request)).isFalse(); + } + + @Test + @DisplayName("should deny step-up when the target user is not the authenticated principal") + void shouldDenyWhenTargetUserDiffers() { + // Someone else's fresh assertion must not authorize an operation on this user's credentials. + authenticate("someone-else@test.com", webAuthnFactor(Instant.now())); + + assertThat(service(120, "WEBAUTHN").isStepUpSatisfied(user, ACTION, request)).isFalse(); + } + + @Test + @DisplayName("should match the authenticated principal case-insensitively") + void shouldMatchPrincipalCaseInsensitively() { + authenticate(EMAIL.toUpperCase(), webAuthnFactor(Instant.now())); + + assertThat(service(120, "WEBAUTHN").isStepUpSatisfied(user, ACTION, request)).isTrue(); + } + + @Test + @DisplayName("should deny step-up when no user is supplied") + void shouldDenyWhenUserIsNull() { + authenticate(EMAIL, webAuthnFactor(Instant.now())); + + assertThat(service(120, "WEBAUTHN").isStepUpSatisfied(null, ACTION, request)).isFalse(); + } + } + + @Nested + @DisplayName("Satisfiability") + class SatisfiabilityTests { + + @Test + @DisplayName("should report step-up unsatisfiable when WEBAUTHN is required and the user has no passkey") + void shouldReportUnsatisfiableWhenUserHasNoPasskey() { + assertThat(service(false, 120, "WEBAUTHN").canSatisfyStepUp(user, ACTION)).isFalse(); + } + + @Test + @DisplayName("should report step-up satisfiable when WEBAUTHN is required and the user has a passkey") + void shouldReportSatisfiableWhenUserHasPasskey() { + assertThat(service(true, 120, "WEBAUTHN").canSatisfyStepUp(user, ACTION)).isTrue(); + } + + @Test + @DisplayName("should report step-up satisfiable when the user has a password and PASSWORD is configured") + void shouldReportSatisfiableWhenUserHasPasswordAndPasswordFactorConfigured() { + user.setPassword("$2a$04$encoded"); + + assertThat(service(false, 120, "PASSWORD").canSatisfyStepUp(user, ACTION)).isTrue(); + } + + @Test + @DisplayName("should report step-up unsatisfiable when PASSWORD is configured and the account is passwordless") + void shouldReportUnsatisfiableWhenPasswordlessAndPasswordFactorConfigured() { + assertThat(service(false, 120, "PASSWORD").canSatisfyStepUp(user, ACTION)).isFalse(); + } + + @Test + @DisplayName("should report step-up satisfiable when any one configured factor is achievable") + void shouldReportSatisfiableWhenAnyConfiguredFactorIsAchievable() { + user.setPassword("$2a$04$encoded"); + + assertThat(service(false, 120, "WEBAUTHN", "PASSWORD").canSatisfyStepUp(user, ACTION)).isTrue(); + } + + @Test + @DisplayName("should not treat a null user as unsatisfiable") + void shouldNotTreatNullUserAsUnsatisfiable() { + // The SPI says user is never null. Answering "unsatisfiable" to a contract violation would release the + // gate; keep it instead, and let isStepUpSatisfied do the denying. + assertThat(service(false, 120, "WEBAUTHN").canSatisfyStepUp(null, ACTION)).isTrue(); + } + + @Test + @DisplayName("should report step-up satisfiable for factors whose achievability cannot be determined") + void shouldReportSatisfiableForUndeterminableFactors() { + // OTT is delivered out of band, so the framework cannot rule it out. Assume achievable and keep gating. + assertThat(service(false, 120, "OTT").canSatisfyStepUp(user, ACTION)).isTrue(); + } + } + + private static DSFactorFreshnessStepUpService service(int ttlSeconds, String... factors) { + return service(true, ttlSeconds, factors); + } + + private static DSFactorFreshnessStepUpService service(boolean userHasPasskey, int ttlSeconds, String... factors) { + StepUpConfigProperties config = new StepUpConfigProperties(); + config.setEnabled(true); + config.setTtlSeconds(ttlSeconds); + config.setFactors(List.of(factors)); + return new DSFactorFreshnessStepUpService(config, u -> userHasPasskey); + } + + private static void authenticate(String name, GrantedAuthority... authorities) { + SecurityContextHolder.getContext().setAuthentication(new TestingAuthenticationToken(name, "n/a", List.of(authorities))); + } + + private static FactorGrantedAuthority webAuthnFactor(Instant issuedAt) { + return FactorGrantedAuthority.withAuthority(FactorGrantedAuthority.WEBAUTHN_AUTHORITY).issuedAt(issuedAt).build(); + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/FactorAuthorityNameValidatorTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/FactorAuthorityNameValidatorTest.java new file mode 100644 index 00000000..2d8306b3 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/security/FactorAuthorityNameValidatorTest.java @@ -0,0 +1,161 @@ +package com.digitalsanctuary.spring.user.security; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import java.util.List; +import java.util.Map; +import org.springframework.beans.factory.ObjectProvider; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import com.digitalsanctuary.spring.user.persistence.model.Privilege; +import com.digitalsanctuary.spring.user.persistence.model.Role; +import com.digitalsanctuary.spring.user.persistence.repository.PrivilegeRepository; +import com.digitalsanctuary.spring.user.persistence.repository.RoleRepository; +import com.digitalsanctuary.spring.user.roles.RolesAndPrivilegesConfig; + +/** + * Tests for {@link FactorAuthorityNameValidator}. + *

+ * A role or privilege named {@code FACTOR_*} is not a naming nit. MFA enforcement checks only that the authority is + * present, so such a name satisfies a required factor the user never completed, and step-up's freshness check finds the + * counterfeit first and denies even a genuine, just-completed ceremony. + *

+ */ +@DisplayName("FactorAuthorityNameValidator Tests") +class FactorAuthorityNameValidatorTest { + + @Test + @DisplayName("should pass when no role or privilege uses the reserved prefix") + void shouldPassForOrdinaryNames() { + FactorAuthorityNameValidator validator = validator(Map.of("ROLE_USER", List.of("READ_PRIVILEGE")), false, false); + + assertThatCode(validator::validateAuthorityNames).doesNotThrowAnyException(); + assertThat(validator.findOffendingNames()).isEmpty(); + } + + @Test + @DisplayName("should fail startup when a privilege uses the reserved prefix and MFA is enabled") + void shouldFailWhenPrivilegeCollidesAndMfaEnabled() { + FactorAuthorityNameValidator validator = + validator(Map.of("ROLE_USER", List.of("READ_PRIVILEGE", "FACTOR_WEBAUTHN")), true, false); + + assertThatThrownBy(validator::validateAuthorityNames).isInstanceOf(IllegalStateException.class) + .hasMessageContaining("FACTOR_WEBAUTHN").hasMessageContaining("Rename"); + } + + @Test + @DisplayName("should fail startup when a role uses the reserved prefix and step-up is enabled") + void shouldFailWhenRoleCollidesAndStepUpEnabled() { + FactorAuthorityNameValidator validator = validator(Map.of("FACTOR_PASSWORD", List.of("READ_PRIVILEGE")), false, true); + + assertThatThrownBy(validator::validateAuthorityNames).isInstanceOf(IllegalStateException.class) + .hasMessageContaining("FACTOR_PASSWORD"); + } + + @Test + @DisplayName("should log rather than fail when neither feature is enabled") + void shouldNotFailWhenBothFeaturesDisabled() { + FactorAuthorityNameValidator validator = validator(Map.of("ROLE_USER", List.of("FACTOR_WEBAUTHN")), false, false); + + assertThatCode(validator::validateAuthorityNames).doesNotThrowAnyException(); + assertThat(validator.findOffendingNames()).containsExactly("FACTOR_WEBAUTHN"); + } + + @Test + @DisplayName("should detect the reserved prefix regardless of case") + void shouldDetectPrefixCaseInsensitively() { + FactorAuthorityNameValidator validator = validator(Map.of("ROLE_USER", List.of("factor_webauthn")), true, false); + + assertThatThrownBy(validator::validateAuthorityNames).isInstanceOf(IllegalStateException.class); + } + + @Test + @DisplayName("should tolerate a role with no privileges") + void shouldTolerateNullPrivileges() { + RolesAndPrivilegesConfig config = new RolesAndPrivilegesConfig(); + config.getRolesAndPrivileges().put("ROLE_USER", null); + + assertThatCode(validator(config, false, false)::validateAuthorityNames).doesNotThrowAnyException(); + } + + private static FactorAuthorityNameValidator validator(Map> rolesAndPrivileges, boolean mfaEnabled, + boolean stepUpEnabled) { + RolesAndPrivilegesConfig config = new RolesAndPrivilegesConfig(); + config.getRolesAndPrivileges().putAll(rolesAndPrivileges); + return validator(config, mfaEnabled, stepUpEnabled); + } + + private static FactorAuthorityNameValidator validator(RolesAndPrivilegesConfig config, boolean mfaEnabled, boolean stepUpEnabled) { + return validator(config, mfaEnabled, stepUpEnabled, List.of(), List.of()); + } + + private static FactorAuthorityNameValidator validator(RolesAndPrivilegesConfig config, boolean mfaEnabled, + boolean stepUpEnabled, List persistedRoles, List persistedPrivileges) { + MfaConfigProperties mfa = new MfaConfigProperties(); + mfa.setEnabled(mfaEnabled); + StepUpConfigProperties stepUp = new StepUpConfigProperties(); + stepUp.setEnabled(stepUpEnabled); + return new FactorAuthorityNameValidator(config, mfa, stepUp, provider(roleRows(persistedRoles)), + provider(privilegeRows(persistedPrivileges))); + } + + private static List roleRows(List names) { + return names.stream().map(name -> { + Role role = new Role(); + role.setName(name); + return role; + }).toList(); + } + + private static List privilegeRows(List names) { + return names.stream().map(name -> { + Privilege privilege = new Privilege(); + privilege.setName(name); + return privilege; + }).toList(); + } + + @SuppressWarnings("unchecked") + private static ObjectProvider provider(List rows) { + ObjectProvider provider = mock(ObjectProvider.class); + Object repository = rows.isEmpty() ? null : repositoryReturning(rows); + when(provider.getIfAvailable()).thenReturn((T) repository); + return provider; + } + + private static Object repositoryReturning(List rows) { + if (rows.get(0) instanceof Role) { + RoleRepository repository = mock(RoleRepository.class); + when(repository.findAll()).thenReturn((List) rows); + return repository; + } + PrivilegeRepository repository = mock(PrivilegeRepository.class); + when(repository.findAll()).thenReturn((List) rows); + return repository; + } + + @Test + @DisplayName("should reject a reserved name persisted in the database but absent from configuration") + void shouldRejectReservedNamePersistedButNotConfigured() { + // RolePrivilegeSetupService never deletes, so a FACTOR_-prefixed row created under an earlier configuration + // survives its removal from YAML and is still granted by AuthorityService. Checking configuration alone + // reports clean while the counterfeit authority is live. + FactorAuthorityNameValidator validator = + validator(new RolesAndPrivilegesConfig(), true, false, List.of("FACTOR_WEBAUTHN"), List.of()); + + assertThat(validator.findOffendingNames()).containsExactly("FACTOR_WEBAUTHN"); + assertThatThrownBy(validator::validateAuthorityNames).isInstanceOf(IllegalStateException.class); + } + + @Test + @DisplayName("should reject a reserved privilege name persisted in the database") + void shouldRejectReservedPrivilegePersisted() { + FactorAuthorityNameValidator validator = + validator(new RolesAndPrivilegesConfig(), false, true, List.of(), List.of("FACTOR_OTT")); + + assertThat(validator.findOffendingNames()).containsExactly("FACTOR_OTT"); + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/FreshFactorAuthorizationManagerTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/FreshFactorAuthorizationManagerTest.java new file mode 100644 index 00000000..7e21f8fb --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/security/FreshFactorAuthorizationManagerTest.java @@ -0,0 +1,121 @@ +package com.digitalsanctuary.spring.user.security; + +import static org.assertj.core.api.Assertions.assertThat; +import java.time.Clock; +import java.time.Duration; +import java.time.Instant; +import java.time.ZoneOffset; +import java.util.List; +import java.util.function.Supplier; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.springframework.security.authentication.AnonymousAuthenticationToken; +import org.springframework.security.authentication.TestingAuthenticationToken; +import org.springframework.security.authorization.AuthorizationResult; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.authority.FactorGrantedAuthority; +import org.springframework.security.core.authority.SimpleGrantedAuthority; + +/** + * Guards passkey enrollment behind a recent authentication of any kind, the way GitHub's sudo mode guards adding a + * security key. + *

+ * Any factor counts, not the {@code user.security.stepUp.factors} list. With the default {@code [WEBAUTHN]} a + * user-configured list would demand a passkey to register a first passkey, which nobody could ever satisfy. + *

+ */ +@DisplayName("Fresh Factor Authorization Manager Tests") +class FreshFactorAuthorizationManagerTest { + + private static final Instant NOW = Instant.parse("2026-08-20T12:00:00Z"); + private static final Clock CLOCK = Clock.fixed(NOW, ZoneOffset.UTC); + + private static FreshFactorAuthorizationManager manager() { + return new FreshFactorAuthorizationManager<>(Duration.ofSeconds(600), CLOCK); + } + + private static Supplier auth(GrantedAuthority... authorities) { + return () -> new TestingAuthenticationToken("user@test.com", "n/a", List.of(authorities)); + } + + private static FactorGrantedAuthority factor(String authority, Instant issuedAt) { + return FactorGrantedAuthority.withAuthority(authority).issuedAt(issuedAt).build(); + } + + private static boolean granted(AuthorizationResult result) { + return result != null && result.isGranted(); + } + + @Nested + @DisplayName("Freshness") + class FreshnessTests { + + @Test + @DisplayName("should grant when a factor was issued inside the window") + void shouldGrantWhenFactorIsFresh() { + assertThat(granted(manager().authorize( + auth(factor(FactorGrantedAuthority.PASSWORD_AUTHORITY, NOW.minusSeconds(599))), null))).isTrue(); + } + + @Test + @DisplayName("should deny when the only factor was issued outside the window") + void shouldDenyWhenFactorIsStale() { + assertThat(granted(manager().authorize( + auth(factor(FactorGrantedAuthority.PASSWORD_AUTHORITY, NOW.minusSeconds(601))), null))).isFalse(); + } + + @Test + @DisplayName("should grant when any one of several factors is fresh") + void shouldGrantWhenAnyFactorIsFresh() { + // A stale password login plus a recent passkey assertion: the recent one is what matters. + assertThat(granted(manager().authorize( + auth(factor(FactorGrantedAuthority.PASSWORD_AUTHORITY, NOW.minusSeconds(5000)), + factor(FactorGrantedAuthority.WEBAUTHN_AUTHORITY, NOW.minusSeconds(10))), + null))).isTrue(); + } + + @Test + @DisplayName("should accept any factor kind, not only the configured step-up factors") + void shouldAcceptAnyFactorKind() { + // Requiring WEBAUTHN here would demand a passkey in order to register a first passkey. + assertThat(granted(manager().authorize( + auth(factor(FactorGrantedAuthority.OTT_AUTHORITY, NOW.minusSeconds(10))), null))).isTrue(); + } + } + + @Nested + @DisplayName("Denial") + class DenialTests { + + @Test + @DisplayName("should deny when the session carries no factor at all") + void shouldDenyWhenNoFactorPresent() { + assertThat(granted(manager().authorize(auth(new SimpleGrantedAuthority("ROLE_USER")), null))).isFalse(); + } + + @Test + @DisplayName("should deny a plain authority named like a factor") + void shouldDenyLookAlikeAuthority() { + // No issuedAt to read, so it cannot be fresh. FactorAuthorityNameValidator rejects such names at + // startup; this is the runtime half of that defense. + assertThat(granted(manager().authorize( + auth(new SimpleGrantedAuthority(FactorGrantedAuthority.PASSWORD_AUTHORITY)), null))).isFalse(); + } + + @Test + @DisplayName("should deny an unauthenticated request") + void shouldDenyUnauthenticated() { + assertThat(granted(manager().authorize(() -> null, null))).isFalse(); + } + + @Test + @DisplayName("should deny an anonymous request") + void shouldDenyAnonymous() { + Supplier anonymous = () -> new AnonymousAuthenticationToken("key", "anonymousUser", + List.of(new SimpleGrantedAuthority("ROLE_ANONYMOUS"))); + assertThat(granted(manager().authorize(anonymous, null))).isFalse(); + } + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/MfaFilterMergingConfigurationTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/MfaFilterMergingConfigurationTest.java index 09935752..bb91deab 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/security/MfaFilterMergingConfigurationTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/security/MfaFilterMergingConfigurationTest.java @@ -54,4 +54,16 @@ void shouldGatePostProcessorOnProperty() { .run(context -> assertThat(context).hasNotFailed().doesNotHaveBean("mfaFilterMergingPostProcessor")); runner.run(context -> assertThat(context).hasNotFailed().doesNotHaveBean("mfaFilterMergingPostProcessor")); } + + @Test + @DisplayName("Post-processor bean is also registered when only step-up is enabled") + void shouldRegisterPostProcessorForStepUpAlone() { + // Step-up refreshes a factor by re-running a login ceremony on an authenticated session. Without merging, that + // second authentication replaces the first and the user loses every authority the UserDetailsService does not + // re-supply, so enabling step-up must enable merging even with MFA off. + runner.withPropertyValues("user.security.step-up.enabled=true", "user.mfa.enabled=false") + .run(context -> assertThat(context).hasNotFailed().hasBean("mfaFilterMergingPostProcessor")); + runner.withPropertyValues("user.security.step-up.enabled=false", "user.mfa.enabled=false") + .run(context -> assertThat(context).hasNotFailed().doesNotHaveBean("mfaFilterMergingPostProcessor")); + } } diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfigurationTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfigurationTest.java new file mode 100644 index 00000000..70dac335 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfigurationTest.java @@ -0,0 +1,195 @@ +package com.digitalsanctuary.spring.user.security; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.springframework.boot.autoconfigure.AutoConfigurations; +import org.springframework.boot.test.context.runner.ApplicationContextRunner; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import java.util.Locale; +import org.junit.jupiter.api.parallel.ResourceLock; +import org.junit.jupiter.api.parallel.Resources; +import com.digitalsanctuary.spring.user.persistence.model.User; +import com.digitalsanctuary.spring.user.roles.RolesAndPrivilegesConfig; +import com.digitalsanctuary.spring.user.service.WebAuthnCredentialManagementService; + +/** + * Tests for {@link StepUpAutoConfiguration}. + *

+ * The default-off contract matters as much as the feature itself: an existing deployment that upgrades must see no + * {@code StepUpService} bean, since one appearing would start gating {@code setPassword} and passkey delete/rename. + *

+ */ +@DisplayName("StepUpAutoConfiguration Tests") +class StepUpAutoConfigurationTest { + + private final ApplicationContextRunner runner = new ApplicationContextRunner() + .withConfiguration(AutoConfigurations.of(StepUpAutoConfiguration.class)) + .withBean(RolesAndPrivilegesConfig.class, RolesAndPrivilegesConfig::new) + .withBean(MfaConfigProperties.class, MfaConfigProperties::new) + .withBean(WebAuthnConfigProperties.class, WebAuthnConfigProperties::new) + // Set as a property, not on the instance: @ConfigurationProperties beans are re-bound after + // construction, so a value set in the supplier is overwritten by dsspringuserconfig.properties. + .withPropertyValues("user.webauthn.enabled=true"); + + @Test + @DisplayName("should fail startup when WEBAUTHN step-up is configured but WebAuthn is disabled") + void shouldFailWhenWebAuthnFactorConfiguredButWebAuthnDisabled() { + // Mirrors MfaConfiguration: requiring a factor the deployment cannot issue makes the gate unsatisfiable, + // which would silently render step-up inert rather than enforcing it. + new ApplicationContextRunner() + .withConfiguration(AutoConfigurations.of(StepUpAutoConfiguration.class)) + .withBean(RolesAndPrivilegesConfig.class, RolesAndPrivilegesConfig::new) + .withBean(MfaConfigProperties.class, MfaConfigProperties::new) + .withBean(WebAuthnConfigProperties.class, WebAuthnConfigProperties::new) + .withPropertyValues("user.security.step-up.enabled=true") + .run(context -> assertThat(context).hasFailed().getFailure() + .hasMessageContaining("user.webauthn.enabled=false")); + } + + @Test + @DisplayName("should fail startup when the step-up TTL is not positive") + void shouldFailStartupForNonPositiveTtl() { + // A zero or negative TTL makes every factor instantly stale, so every gated operation denies forever with a + // debug log as the only signal. @Min(1) only runs because the class is @Validated. + runner.withPropertyValues("user.security.step-up.enabled=true", "user.security.step-up.ttlSeconds=0") + .run(context -> assertThat(context).hasFailed()); + } + + @Test + @DisplayName("should fail startup when the enrollment TTL is not positive") + void shouldFailStartupForNonPositiveEnrollmentTtl() { + runner.withPropertyValues("user.security.step-up.enabled=true", "user.security.step-up.enrollmentTtlSeconds=-1") + .run(context -> assertThat(context).hasFailed()); + } + + @Test + @DisplayName("should answer satisfiability from the WebAuthn credential service when one is present") + void shouldDelegateSatisfiabilityToTheCredentialService() { + // Pins the auto-configuration's lambda: the built-in service must read real credential state, not a constant. + WebAuthnCredentialManagementService credentialService = mock(WebAuthnCredentialManagementService.class); + // Distinct ids: User equality keys on id alone, so two unsaved instances compare equal and Mockito could + // not tell the stubs apart. See the note on User.id and EntityEqualityTest. + User withPasskey = new User(); + withPasskey.setId(1L); + User withoutPasskey = new User(); + withoutPasskey.setId(2L); + when(credentialService.hasCredentials(withPasskey)).thenReturn(true); + when(credentialService.hasCredentials(withoutPasskey)).thenReturn(false); + + runner.withBean(WebAuthnCredentialManagementService.class, () -> credentialService) + .withPropertyValues("user.security.step-up.enabled=true").run(context -> { + StepUpService service = context.getBean(StepUpService.class); + assertThat(service.canSatisfyStepUp(withPasskey, "set-password")).isTrue(); + assertThat(service.canSatisfyStepUp(withoutPasskey, "set-password")).isFalse(); + }); + } + + @Test + @DisplayName("should report step-up unsatisfiable when no WebAuthn credential service is available") + void shouldReportUnsatisfiableWhenCredentialServiceAbsent() { + // Without the service no account can hold a passkey, so WEBAUTHN is unachievable and step-up does not + // apply. Claiming otherwise would gate an operation nobody could ever unlock. + runner.withPropertyValues("user.security.step-up.enabled=true").run(context -> assertThat( + context.getBean(StepUpService.class).canSatisfyStepUp(new User(), "set-password")).isFalse()); + } + + @Test + @DisplayName("should default canSatisfyStepUp to true for implementations that do not override it") + void shouldDefaultSatisfiabilityToTrue() { + // The compatibility promise in MIGRATION.md: a consumer SPI implementation written before this method + // existed keeps being enforced, rather than silently opting out of step-up. + StepUpService legacyImplementation = (user, action, request) -> false; + + assertThat(legacyImplementation.canSatisfyStepUp(new User(), "set-password")).isTrue(); + } + + @Test + @ResourceLock(Resources.LOCALE) + @DisplayName("should normalize factor names independently of the default locale") + void shouldNormalizeFactorNamesIndependentlyOfDefaultLocale() { + Locale original = Locale.getDefault(); + try { + // In Turkish, "authorization_code".toUpperCase() dots the I and no longer matches the factor map. + Locale.setDefault(Locale.forLanguageTag("tr")); + runner.withPropertyValues("user.security.step-up.enabled=true", + "user.security.step-up.factors=authorization_code") + .run(context -> assertThat(context).hasNotFailed().hasSingleBean(StepUpService.class)); + } finally { + Locale.setDefault(original); + } + } + + @Test + @DisplayName("should register no step-up service by default") + void shouldRegisterNoServiceByDefault() { + runner.run(context -> assertThat(context).hasNotFailed().doesNotHaveBean(StepUpService.class)); + } + + @Test + @DisplayName("should register no step-up service when explicitly disabled") + void shouldRegisterNoServiceWhenDisabled() { + runner.withPropertyValues("user.security.step-up.enabled=false") + .run(context -> assertThat(context).hasNotFailed().doesNotHaveBean(StepUpService.class)); + } + + @Test + @DisplayName("should register the built-in service when enabled") + void shouldRegisterBuiltInServiceWhenEnabled() { + runner.withPropertyValues("user.security.step-up.enabled=true").run(context -> assertThat(context).hasNotFailed() + .getBean(StepUpService.class).isInstanceOf(DSFactorFreshnessStepUpService.class)); + } + + @Test + @DisplayName("should bind the camelCase property spelling used in the documentation") + void shouldBindCamelCasePropertySpelling() { + runner.withPropertyValues("user.security.stepUp.enabled=true", "user.security.stepUp.ttlSeconds=300") + .run(context -> { + assertThat(context).hasNotFailed().hasSingleBean(StepUpService.class); + assertThat(context.getBean(StepUpConfigProperties.class).getTtlSeconds()).isEqualTo(300); + }); + } + + @Test + @DisplayName("should back off when the application supplies its own step-up service") + void shouldBackOffForConsumerSuppliedService() { + runner.withPropertyValues("user.security.step-up.enabled=true").withUserConfiguration(ConsumerStepUpConfiguration.class) + .run(context -> assertThat(context).hasNotFailed().getBean(StepUpService.class) + .isNotInstanceOf(DSFactorFreshnessStepUpService.class)); + } + + @Test + @DisplayName("should fail startup when a configured factor name is unknown") + void shouldFailStartupForUnknownFactor() { + runner.withPropertyValues("user.security.step-up.enabled=true", "user.security.step-up.factors=WEBAUTHN,TELEPATHY") + .run(context -> assertThat(context).hasFailed().getFailure() + .hasMessageContaining("TELEPATHY").hasMessageContaining("Valid values")); + } + + @Test + @DisplayName("should fail startup when no factors are configured") + void shouldFailStartupForEmptyFactors() { + runner.withPropertyValues("user.security.step-up.enabled=true", "user.security.step-up.factors=") + .run(context -> assertThat(context).hasFailed().getFailure().hasMessageContaining("no factors are configured")); + } + + @Test + @DisplayName("should register the factor-authority name validator regardless of whether step-up is enabled") + void shouldAlwaysRegisterAuthorityNameValidator() { + runner.run(context -> assertThat(context).hasNotFailed().hasSingleBean(FactorAuthorityNameValidator.class)); + runner.withPropertyValues("user.security.step-up.enabled=true") + .run(context -> assertThat(context).hasNotFailed().hasSingleBean(FactorAuthorityNameValidator.class)); + } + + @Configuration(proxyBeanMethods = false) + static class ConsumerStepUpConfiguration { + + @Bean + StepUpService consumerStepUpService() { + return mock(StepUpService.class); + } + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/StepUpIntegrationTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/StepUpIntegrationTest.java new file mode 100644 index 00000000..1784a32a --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/security/StepUpIntegrationTest.java @@ -0,0 +1,69 @@ +package com.digitalsanctuary.spring.user.security; + +import static org.assertj.core.api.Assertions.assertThat; +import java.util.ArrayList; +import java.util.List; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.security.web.FilterChainProxy; +import org.springframework.security.web.SecurityFilterChain; +import org.springframework.security.web.authentication.AbstractAuthenticationProcessingFilter; +import org.springframework.test.context.TestPropertySource; +import org.springframework.test.util.ReflectionTestUtils; +import com.digitalsanctuary.spring.user.test.annotations.SecurityTest; +import jakarta.servlet.Filter; + +/** + * Verifies that enabling step-up in a full application context produces a working configuration. + * + *

+ * The two halves have to arrive together. The {@link StepUpService} bean enforces freshness, and factor merging on the + * authentication processing filters is what lets a user refresh a factor at all: without it, re-asserting while logged + * in replaces the authentication and drops every authority the {@code UserDetailsService} does not re-supply. Merging + * was previously tied to {@code user.mfa.enabled}, so this checks that step-up alone switches it on, with MFA off. + *

+ */ +@SecurityTest +@TestPropertySource(properties = {"user.security.stepUp.enabled=true", "user.mfa.enabled=false", "user.webauthn.enabled=true"}) +@DisplayName("Step-Up Integration Tests") +class StepUpIntegrationTest { + + @Autowired + private FilterChainProxy filterChainProxy; + + @Autowired(required = false) + private StepUpService stepUpService; + + @Test + @DisplayName("should register the built-in step-up service when step-up is enabled") + void shouldRegisterBuiltInStepUpService() { + assertThat(stepUpService).as("user.security.stepUp.enabled=true must register a StepUpService") + .isInstanceOf(DSFactorFreshnessStepUpService.class); + } + + @Test + @DisplayName("should enable factor merging on authentication filters when step-up is enabled and MFA is not") + void shouldEnableFactorMergingForStepUpAlone() { + List processingFilters = findAuthenticationProcessingFilters(); + + assertThat(processingFilters).as("the security filter chain must contain authentication processing filters").isNotEmpty(); + assertThat(processingFilters) + .as("every authentication processing filter must have mfaEnabled=true, or re-asserting for step-up would " + + "replace the session's authorities instead of refreshing the factor on them") + .allSatisfy(filter -> assertThat((Boolean) ReflectionTestUtils.getField(filter, "mfaEnabled")) + .as("mfaEnabled on %s", filter.getClass().getSimpleName()).isTrue()); + } + + private List findAuthenticationProcessingFilters() { + List result = new ArrayList<>(); + for (SecurityFilterChain chain : filterChainProxy.getFilterChains()) { + for (Filter filter : chain.getFilters()) { + if (filter instanceof AbstractAuthenticationProcessingFilter processingFilter) { + result.add(processingFilter); + } + } + } + return result; + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnEnrollmentGateIntegrationTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnEnrollmentGateIntegrationTest.java new file mode 100644 index 00000000..83899590 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnEnrollmentGateIntegrationTest.java @@ -0,0 +1,76 @@ +package com.digitalsanctuary.spring.user.security; + +import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.authentication; +import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; +import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.user; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; +import java.time.Instant; +import java.util.List; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.http.MediaType; +import org.springframework.security.authentication.TestingAuthenticationToken; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.authority.FactorGrantedAuthority; +import org.springframework.security.core.authority.SimpleGrantedAuthority; +import org.springframework.test.context.TestPropertySource; +import org.springframework.test.web.servlet.MockMvc; +import com.digitalsanctuary.spring.user.test.annotations.SecurityTest; + +/** + * Proves the enrollment gate is actually wired into the filter chain, not merely that + * {@link FreshFactorAuthorizationManager} works in isolation. + *

+ * This is the assumption the whole feature rests on: {@code WebAuthnRegistrationFilter} is registered after + * {@code AuthorizationFilter}, so an {@code authorizeHttpRequests} rule is evaluated before the endpoint runs. If + * that ordering were wrong the gate would silently do nothing while every unit test still passed. + *

+ */ +@SecurityTest +@TestPropertySource(properties = {"user.webauthn.enabled=true", "user.security.stepUp.enabled=true", + "user.security.stepUp.enrollmentTtlSeconds=600"}) +@DisplayName("WebAuthn Enrollment Gate Integration Tests") +class WebAuthnEnrollmentGateIntegrationTest { + + @Autowired + private MockMvc mockMvc; + + private static Authentication withFactor(Instant issuedAt) { + List authorities = List.of(new SimpleGrantedAuthority("ROLE_USER"), + FactorGrantedAuthority.withAuthority(FactorGrantedAuthority.PASSWORD_AUTHORITY).issuedAt(issuedAt).build()); + return new TestingAuthenticationToken("user@test.com", "n/a", authorities); + } + + @Test + @DisplayName("should reject passkey enrollment when the session carries no authentication factor") + void shouldRejectEnrollmentWithoutFactor() throws Exception { + // The attack this closes: a stolen session cookie enrolls an attacker-controlled passkey, asserts with it to + // mint a fresh FACTOR_WEBAUTHN, and thereby satisfies every step-up gate. + mockMvc.perform(post("/webauthn/register").with(user("user@test.com").roles("USER")).with(csrf()) + .contentType(MediaType.APPLICATION_JSON).content("{}")).andExpect(status().isForbidden()); + } + + @Test + @DisplayName("should reject passkey enrollment when the only factor is older than the window") + void shouldRejectEnrollmentWithStaleFactor() throws Exception { + mockMvc.perform(post("/webauthn/register").with(authentication(withFactor(Instant.now().minusSeconds(601)))) + .with(csrf()).contentType(MediaType.APPLICATION_JSON).content("{}")).andExpect(status().isForbidden()); + } + + @Test + @DisplayName("should not reject passkey enrollment when a factor was issued inside the window") + void shouldAllowEnrollmentWithFreshFactor() throws Exception { + // The request body is not a real attestation, so the endpoint itself fails it. What matters here is that the + // gate let it through: anything other than 403 proves authorization passed and the filter ran. + mockMvc.perform(post("/webauthn/register").with(authentication(withFactor(Instant.now().minusSeconds(5)))) + .with(csrf()).contentType(MediaType.APPLICATION_JSON).content("{}")) + .andExpect(status().is(not(403))); + } + + private static org.hamcrest.Matcher not(int status) { + return org.hamcrest.Matchers.not(org.hamcrest.Matchers.is(status)); + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/AuthWithoutPasswordFactorTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/AuthWithoutPasswordFactorTest.java new file mode 100644 index 00000000..21170dc0 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/service/AuthWithoutPasswordFactorTest.java @@ -0,0 +1,93 @@ +package com.digitalsanctuary.spring.user.service; + +import static org.assertj.core.api.Assertions.assertThat; +import java.time.Instant; +import java.util.List; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.authority.FactorGrantedAuthority; +import org.springframework.security.core.context.SecurityContextHolder; + +/** + * The framework logs users in without a password on three paths: the email-verification link, auto-login straight + * after registration, and dev login. None of them runs a Spring Security {@code AuthenticationProvider}, so none of + * them stamps a {@link FactorGrantedAuthority}, and a session with no factor cannot satisfy any freshness check. + *

+ * That matters for the passkey-enrollment gate: without a stamp, a user who has just registered and verified their + * email could never add their first passkey until they logged out and back in. Each path therefore names the factor + * it genuinely represents. + *

+ */ +@DisplayName("Auth Without Password Factor Tests") +class AuthWithoutPasswordFactorTest { + + @AfterEach + void tearDown() { + SecurityContextHolder.clearContext(); + } + + @Nested + @DisplayName("Factor selection") + class FactorSelectionTests { + + @Test + @DisplayName("should describe the email-verification link as a one-time token") + void shouldDescribeVerificationLinkAsOneTimeToken() { + assertThat(AuthWithoutPasswordFactor.EMAIL_VERIFICATION.getAuthority()) + .isEqualTo(FactorGrantedAuthority.OTT_AUTHORITY); + } + + @Test + @DisplayName("should describe post-registration auto-login as a password login") + void shouldDescribePostRegistrationAsPasswordLogin() { + // The user submitted their password in the very request that triggers this login. + assertThat(AuthWithoutPasswordFactor.REGISTRATION.getAuthority()) + .isEqualTo(FactorGrantedAuthority.PASSWORD_AUTHORITY); + } + + @Test + @DisplayName("should stamp no factor for dev login") + void shouldStampNoFactorForDevLogin() { + // Dev login proves nothing about presence; it is a local-profile impersonation tool. + assertThat(AuthWithoutPasswordFactor.DEV_LOGIN.getAuthority()).isNull(); + } + } + + @Nested + @DisplayName("Stamping") + class StampingTests { + + @Test + @DisplayName("should add a freshly issued factor to the authenticated authorities") + void shouldAddFreshlyIssuedFactor() { + Instant before = Instant.now().minusSeconds(1); + + List result = AuthWithoutPasswordFactor.EMAIL_VERIFICATION + .withFactor(List.of(new org.springframework.security.core.authority.SimpleGrantedAuthority("ROLE_USER"))); + + assertThat(result).extracting(GrantedAuthority::getAuthority) + .contains("ROLE_USER", FactorGrantedAuthority.OTT_AUTHORITY); + FactorGrantedAuthority factor = result.stream().filter(FactorGrantedAuthority.class::isInstance) + .map(FactorGrantedAuthority.class::cast).findFirst().orElseThrow(); + assertThat(factor.getIssuedAt()).isAfter(before); + } + + @Test + @DisplayName("should leave the authorities untouched for dev login") + void shouldLeaveAuthoritiesUntouchedForDevLogin() { + List original = + List.of(new org.springframework.security.core.authority.SimpleGrantedAuthority("ROLE_USER")); + + assertThat(AuthWithoutPasswordFactor.DEV_LOGIN.withFactor(original)) + .containsExactlyElementsOf(original); + } + } + + private static Authentication currentAuthentication() { + return SecurityContextHolder.getContext().getAuthentication(); + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/LoginSuccessFactorStampingTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/LoginSuccessFactorStampingTest.java new file mode 100644 index 00000000..710563d5 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/service/LoginSuccessFactorStampingTest.java @@ -0,0 +1,65 @@ +package com.digitalsanctuary.spring.user.service; + +import static org.assertj.core.api.Assertions.assertThat; +import java.time.Instant; +import java.util.List; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.springframework.security.authentication.TestingAuthenticationToken; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.authority.FactorGrantedAuthority; +import org.springframework.security.core.authority.SimpleGrantedAuthority; +import org.springframework.security.core.context.SecurityContextHolder; + +/** + * Not every login flow stamps a {@link FactorGrantedAuthority}. Verified in Spring Security 7.1.0: the password and + * plain-OAuth2 providers do, but {@code OidcAuthorizationCodeAuthenticationProvider} does not, so an OIDC login + * (Keycloak, or Google configured with the {@code openid} scope) leaves a session carrying no factor at all. Such a + * session can never satisfy a freshness check, so those users could never enroll a passkey once the gate is on. + *

+ * The discriminator is deliberately "is a factor already present" rather than the authentication's type: on both the + * OAuth2 and OIDC paths this framework's user services return a {@code DSUserDetails} whose authorities are database + * roles, so the usual {@code OidcUserAuthority} versus {@code OAuth2UserAuthority} test does not apply here. + *

+ */ +@DisplayName("Login Success Factor Stamping Tests") +class LoginSuccessFactorStampingTest { + + @AfterEach + void tearDown() { + SecurityContextHolder.clearContext(); + } + + @Test + @DisplayName("should stamp an authorization-code factor when the login flow stamped none") + void shouldStampWhenNoFactorPresent() { + Instant before = Instant.now().minusSeconds(1); + Authentication oidcLogin = new TestingAuthenticationToken("user@test.com", "n/a", + List.of(new SimpleGrantedAuthority("ROLE_USER"))); + + List result = LoginFactorStamper.ensureFactor(oidcLogin.getAuthorities()); + + assertThat(result).extracting(GrantedAuthority::getAuthority) + .contains("ROLE_USER", FactorGrantedAuthority.AUTHORIZATION_CODE_AUTHORITY); + FactorGrantedAuthority stamped = result.stream().filter(FactorGrantedAuthority.class::isInstance) + .map(FactorGrantedAuthority.class::cast).findFirst().orElseThrow(); + assertThat(stamped.getIssuedAt()).isAfter(before); + } + + @Test + @DisplayName("should not stamp a second factor when the login flow already stamped one") + void shouldNotStampWhenFactorAlreadyPresent() { + // Form login and plain OAuth2 both stamp before the success handler runs. Adding again would duplicate, + // and a stale duplicate could shadow the genuine one in a freshness check. + Authentication passwordLogin = new TestingAuthenticationToken("user@test.com", "n/a", + List.of(new SimpleGrantedAuthority("ROLE_USER"), + FactorGrantedAuthority.fromAuthority(FactorGrantedAuthority.PASSWORD_AUTHORITY))); + + List result = LoginFactorStamper.ensureFactor(passwordLogin.getAuthorities()); + + assertThat(result).containsExactlyElementsOf(passwordLogin.getAuthorities()); + assertThat(result).filteredOn(FactorGrantedAuthority.class::isInstance).hasSize(1); + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java index 89bb62c5..ec54c9df 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java @@ -770,6 +770,40 @@ void authWithoutPassword_authenticatesValidUser() { } } + @Test + @DisplayName("authWithoutPassword - stamps the factor the calling path represents") + void authWithoutPassword_stampsFactorForPath() { + // Without a stamp the session carries no factor at all, so it can never satisfy a freshness check and a + // just-verified user could not enroll their first passkey until they logged out and back in. + Collection authorities = Arrays.asList(new SimpleGrantedAuthority("ROLE_USER")); + DSUserDetails userDetails = new DSUserDetails(testUser, authorities); + when(dsUserDetailsService.loadUserByUsername(testUser.getEmail())).thenReturn(userDetails); + + HttpServletRequest mockRequest = mock(HttpServletRequest.class); + HttpSession mockSession = mock(HttpSession.class); + ServletRequestAttributes attrs = mock(ServletRequestAttributes.class); + when(attrs.getRequest()).thenReturn(mockRequest); + when(mockRequest.getSession(true)).thenReturn(mockSession); + + try (MockedStatic mockedHolder = mockStatic(RequestContextHolder.class); + MockedStatic mockedSecurityHolder = mockStatic(SecurityContextHolder.class)) { + mockedHolder.when(RequestContextHolder::getRequestAttributes).thenReturn(attrs); + SecurityContext securityContext = mock(SecurityContext.class); + mockedSecurityHolder.when(SecurityContextHolder::getContext).thenReturn(securityContext); + final Authentication[] storedAuth = new Authentication[1]; + org.mockito.Mockito.doAnswer(invocation -> { + storedAuth[0] = invocation.getArgument(0); + return null; + }).when(securityContext).setAuthentication(any()); + when(securityContext.getAuthentication()).thenAnswer(invocation -> storedAuth[0]); + + userService.authWithoutPassword(testUser, AuthWithoutPasswordFactor.EMAIL_VERIFICATION); + + assertThat(storedAuth[0].getAuthorities()).extracting(GrantedAuthority::getAuthority) + .contains("ROLE_USER", org.springframework.security.core.authority.FactorGrantedAuthority.OTT_AUTHORITY); + } + } + @Test @DisplayName("authWithoutPassword - publishes InteractiveAuthenticationSuccessEvent") void shouldPublishInteractiveAuthenticationSuccessEventWhenAuthSucceeds() {