From 103e8fe6058b52f0c0a9e04254d54bede5ad98b5 Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Wed, 19 Aug 2026 13:10:22 -0600 Subject: [PATCH 1/6] feat: built-in WebAuthn step-up for credential-altering operations Ships a default StepUpService so consumers get step-up out of the box instead of implementing the SPI themselves (#335). Off by default. Step-up is expressed as a freshness requirement on a Spring Security factor: WebAuthnAuthenticationProvider stamps FACTOR_WEBAUTHN with the moment of assertion, and re-running the ordinary passkey ceremony while already logged in merges a newly stamped factor onto the session. So the ceremony is the existing login flow, re-run. No new endpoints, no server-side challenge state, no step-up token, and no second client ceremony to write. - user.security.stepUp.{enabled,ttlSeconds,factors}, defaults false/120/ [WEBAUTHN]. Unknown or empty factor names fail startup. - DSFactorFreshnessStepUpService satisfies the SPI shipped in #334 unchanged, so consumer implementations keep working and still win via @ConditionalOnMissingBean. - Passkey delete/rename now go through the same gate. They previously required nothing at all on a passwordless account: the current-password check returns immediately when there is no password. Enforcement only applies when a StepUpService is configured, so existing deployments are unaffected. Rejections are 401 with error code step-up-required. - Enabling step-up now also enables factor merging, which was tied to user.mfa.enabled. Without it a re-assertion replaces the session's authorities instead of refreshing the factor on them. Also adds FactorAuthorityNameValidator, which rejects FACTOR_-prefixed role and privilege names. Such a name is indistinguishable from a real factor by name, and the authorities loaded from the database sort ahead of the stamped one: it satisfies MFA enforcement without the factor ever being completed (an authentication bypass for that deployment), and it shadows the genuine factor in a step-up freshness check. Startup fails when MFA or step-up is enabled, and logs an error otherwise. Binding model, per the decisions on #335: the proof is bound to user, session, and time, but is not single-use and not per-action. That closes the case the feature exists for, an attacker with a session cookie and no authenticator. It leaves an attacker sharing the session able to piggyback inside the window, hence the short default TTL. Docs: CONFIG.md and MIGRATION.md. ./gradlew check: 1204 tests, 0 failures. Refs #335 --- CONFIG.md | 32 +++- MIGRATION.md | 11 +- .../spring/user/api/UserAPI.java | 4 +- .../user/api/WebAuthnManagementAPI.java | 31 +++- .../user/api/WebAuthnManagementAPIAdvice.java | 10 ++ .../WebAuthnStepUpRequiredException.java | 28 ++++ .../DSFactorFreshnessStepUpService.java | 116 +++++++++++++ .../FactorAuthorityNameValidator.java | 85 ++++++++++ .../MfaFilterMergingConfiguration.java | 38 ++++- .../security/StepUpAutoConfiguration.java | 91 ++++++++++ .../user/security/StepUpConfigProperties.java | 69 ++++++++ ...ot.autoconfigure.AutoConfiguration.imports | 1 + .../config/dsspringuserconfig.properties | 12 ++ .../api/WebAuthnManagementAPIStepUpTest.java | 146 ++++++++++++++++ .../user/api/WebAuthnManagementAPITest.java | 40 +++-- .../DSFactorFreshnessStepUpServiceTest.java | 156 ++++++++++++++++++ .../FactorAuthorityNameValidatorTest.java | 91 ++++++++++ .../MfaFilterMergingConfigurationTest.java | 12 ++ .../security/StepUpAutoConfigurationTest.java | 97 +++++++++++ .../user/security/StepUpIntegrationTest.java | 69 ++++++++ 20 files changed, 1107 insertions(+), 32 deletions(-) create mode 100644 src/main/java/com/digitalsanctuary/spring/user/exceptions/WebAuthnStepUpRequiredException.java create mode 100644 src/main/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpService.java create mode 100644 src/main/java/com/digitalsanctuary/spring/user/security/FactorAuthorityNameValidator.java create mode 100644 src/main/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfiguration.java create mode 100644 src/main/java/com/digitalsanctuary/spring/user/security/StepUpConfigProperties.java create mode 100644 src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIStepUpTest.java create mode 100644 src/test/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpServiceTest.java create mode 100644 src/test/java/com/digitalsanctuary/spring/user/security/FactorAuthorityNameValidatorTest.java create mode 100644 src/test/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfigurationTest.java create mode 100644 src/test/java/com/digitalsanctuary/spring/user/security/StepUpIntegrationTest.java diff --git a/CONFIG.md b/CONFIG.md index 2fcb84df..63180bc8 100644 --- a/CONFIG.md +++ b/CONFIG.md @@ -125,12 +125,36 @@ 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. + +**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..e3c0daca 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -223,7 +223,16 @@ 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. + +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-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 configured while MFA or step-up is enabled, and logs an error otherwise. Rename them. +- **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..eeeadced 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java +++ b/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java @@ -367,7 +367,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 +376,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, 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..d35442c2 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,10 @@ 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.StepUpService; import com.digitalsanctuary.spring.user.service.LoginAttemptService; import com.digitalsanctuary.spring.user.service.UserService; import com.digitalsanctuary.spring.user.service.WebAuthnCredentialManagementService; @@ -78,6 +81,8 @@ 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; /** * Get user's registered passkeys. @@ -124,9 +129,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 +163,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 +183,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 +203,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 +252,24 @@ 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. + StepUpService stepUpService = stepUpServiceProvider.getIfAvailable(); + if (stepUpService != null && !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/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..f21e13fb --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpService.java @@ -0,0 +1,116 @@ +package com.digitalsanctuary.spring.user.security; + +import java.time.Duration; +import java.util.List; +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, who can never produce a recent factor. It leaves a narrower one open: an + * attacker sharing the session concurrently can piggyback inside the window. Keep the TTL 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 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 + */ + public DSFactorFreshnessStepUpService(StepUpConfigProperties config) { + 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()); + 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; + } +} 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..d28f3aff --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/security/FactorAuthorityNameValidator.java @@ -0,0 +1,85 @@ +package com.digitalsanctuary.spring.user.security; + +import java.util.List; +import java.util.Set; +import java.util.TreeSet; +import java.util.stream.Stream; +import com.digitalsanctuary.spring.user.roles.RolesAndPrivilegesConfig; +import jakarta.annotation.PostConstruct; +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 { + + /** 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; + + /** + * Runs the check at startup. + * + * @throws IllegalStateException if a configured role or privilege name starts with {@code FACTOR_} while MFA or + * step-up is enabled + */ + @PostConstruct + public void validateAuthorityNames() { + List offenders = findOffendingNames(); + if (offenders.isEmpty()) { + return; + } + + String message = "user.roles-and-privileges declares " + offenders + ", which 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 these roles/privileges."; + + 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(name -> name != null && name.toUpperCase().startsWith(FACTOR_PREFIX)).forEach(offenders::add)); + return List.copyOf(offenders); + } +} 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..e39c90aa --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfiguration.java @@ -0,0 +1,91 @@ +package com.digitalsanctuary.spring.user.security; + +import java.util.List; +import java.util.Set; +import java.util.TreeSet; +import java.util.stream.Collectors; +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.roles.RolesAndPrivilegesConfig; +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 + * @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) { + validateFactors(config.getFactors()); + log.info("Step-up enabled: a factor of {} issued within {}s authorizes credential-altering operations", + config.getFactors(), config.getTtlSeconds()); + return new DSFactorFreshnessStepUpService(config); + } + + /** + * 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 + * @return the validator + */ + @Bean + @ConditionalOnMissingBean(FactorAuthorityNameValidator.class) + public FactorAuthorityNameValidator factorAuthorityNameValidator(RolesAndPrivilegesConfig rolesAndPrivilegesConfig, + MfaConfigProperties mfaConfigProperties, StepUpConfigProperties stepUpConfigProperties) { + return new FactorAuthorityNameValidator(rolesAndPrivilegesConfig, mfaConfigProperties, stepUpConfigProperties); + } + + /** + * 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) { + 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())) + .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() + "."); + } + } + + 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..ae675df5 --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpConfigProperties.java @@ -0,0 +1,69 @@ +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.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 +@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; + + /** + * 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/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..7d2b8443 100644 --- a/src/main/resources/config/dsspringuserconfig.properties +++ b/src/main/resources/config/dsspringuserconfig.properties @@ -92,6 +92,18 @@ 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 +# 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/WebAuthnManagementAPIStepUpTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIStepUpTest.java new file mode 100644 index 00000000..a428ff5a --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIStepUpTest.java @@ -0,0 +1,146 @@ +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.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 User testUser; + + private final MockHttpServletRequest request = new MockHttpServletRequest(); + + @BeforeEach + void setUp() { + testUser = TestFixtures.Users.standardUser(); + api = new WebAuthnManagementAPI(credentialManagementService, userService, eventPublisher, loginAttemptService, + stepUpServiceProvider); + 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.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 reject passkey rename when step-up is not satisfied") + void shouldRejectRenameWithoutStepUp() { + 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.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.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); + } + } +} 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..95398dea 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java @@ -18,6 +18,7 @@ import org.junit.jupiter.api.Test; import org.mockito.InjectMocks; 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 +30,7 @@ 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.StepUpService; import com.digitalsanctuary.spring.user.service.LoginAttemptService; import com.digitalsanctuary.spring.user.service.UserService; import com.digitalsanctuary.spring.user.service.WebAuthnCredentialManagementService; @@ -57,6 +59,18 @@ 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; + @InjectMocks private WebAuthnManagementAPI api; @@ -175,7 +189,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 +208,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 +229,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 +244,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 +262,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 +280,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 +292,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 +310,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 +328,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 +343,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 +358,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 +373,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 +384,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/security/DSFactorFreshnessStepUpServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpServiceTest.java new file mode 100644 index 00000000..381d4866 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpServiceTest.java @@ -0,0 +1,156 @@ +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(); + } + } + + private static DSFactorFreshnessStepUpService service(int ttlSeconds, String... factors) { + StepUpConfigProperties config = new StepUpConfigProperties(); + config.setEnabled(true); + config.setTtlSeconds(ttlSeconds); + config.setFactors(List.of(factors)); + return new DSFactorFreshnessStepUpService(config); + } + + 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..64adb062 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/security/FactorAuthorityNameValidatorTest.java @@ -0,0 +1,91 @@ +package com.digitalsanctuary.spring.user.security; + +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.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +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) { + MfaConfigProperties mfa = new MfaConfigProperties(); + mfa.setEnabled(mfaEnabled); + StepUpConfigProperties stepUp = new StepUpConfigProperties(); + stepUp.setEnabled(stepUpEnabled); + return new FactorAuthorityNameValidator(config, mfa, stepUp); + } +} 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..5b946633 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfigurationTest.java @@ -0,0 +1,97 @@ +package com.digitalsanctuary.spring.user.security; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +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 com.digitalsanctuary.spring.user.roles.RolesAndPrivilegesConfig; + +/** + * 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); + + @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; + } +} From 7f844304f58eec08d250eb355bf88c80c42b413b Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Thu, 20 Aug 2026 12:46:19 -0600 Subject: [PATCH 2/6] fix: do not deny step-up to accounts that can never satisfy it Step-up denied any user who had not recently proven presence, on the assumption they could go and do so. That assumption fails for accounts holding no credential the configured factors accept. UserService.hasPassword() is a null check on the password column, and DSOAuth2UserService/DSOidcUserService create social users without ever setting one, so every OAuth2/OIDC account is "passwordless" here. With user.security.stepUp.enabled=true such an account could never produce a FACTOR_WEBAUTHN, and the else-if holding allowInitialPasswordSetWithoutStepUp was unreachable whenever a StepUpService bean existed, so POST /user/setPassword returned HTTP 401 permanently with no configuration that recovered it. "Sign in with Google, then add a password" was broken. Adds StepUpService.canSatisfyStepUp(User), a default method returning true so existing consumer implementations are unaffected in both source and behavior. The built-in service overrides it: WEBAUTHN needs a registered passkey, PASSWORD needs a password, and factors delivered out of band cannot be ruled out from here so they stay gated. UserAPI treats false as "step-up does not apply" and falls back to the configured default rather than denying. Placing the check on the SPI rather than at the call site keeps it correct for consumer implementations, whose mechanism may not involve passkeys at all and for which a call-site hasCredentials() test would silently skip a control that did apply. ./gradlew check: green on a forced full rerun. --- MIGRATION.md | 2 + .../spring/user/api/UserAPI.java | 5 +- .../DSFactorFreshnessStepUpService.java | 46 ++++++++++++++- .../security/StepUpAutoConfiguration.java | 14 ++++- .../spring/user/security/StepUpService.java | 24 ++++++++ .../spring/user/api/UserAPIUnitTest.java | 56 +++++++++++++++++++ .../DSFactorFreshnessStepUpServiceTest.java | 52 ++++++++++++++++- 7 files changed, 194 insertions(+), 5 deletions(-) diff --git a/MIGRATION.md b/MIGRATION.md index e3c0daca..ff906f58 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -225,6 +225,8 @@ Affected endpoints (all require `user.webauthn.enabled=true` except where noted) **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. +**`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`. + 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: 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 eeeadced..0a77de95 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java +++ b/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java @@ -570,7 +570,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)) { 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, diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpService.java b/src/main/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpService.java index f21e13fb..ea9aa826 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpService.java @@ -2,6 +2,8 @@ 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; @@ -55,6 +57,7 @@ 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(); @@ -62,8 +65,11 @@ public class DSFactorFreshnessStepUpService implements StepUpService { * 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)} to tell "has not asserted" apart from "has nothing to assert with" */ - public DSFactorFreshnessStepUpService(StepUpConfigProperties config) { + 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 @@ -113,4 +119,42 @@ public boolean isStepUpSatisfied(User user, String action, HttpServletRequest re 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 + * @return {@code true} if at least one configured factor could be produced by this user + */ + @Override + public boolean canSatisfyStepUp(User user) { + if (user == null) { + return false; + } + 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: the account holds no credential able to produce any of {}", factorNames); + return false; + } } diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfiguration.java b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfiguration.java index e39c90aa..186fdac5 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfiguration.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfiguration.java @@ -4,6 +4,7 @@ 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; @@ -11,6 +12,7 @@ import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.PropertySource; import com.digitalsanctuary.spring.user.roles.RolesAndPrivilegesConfig; +import com.digitalsanctuary.spring.user.service.WebAuthnCredentialManagementService; import lombok.extern.slf4j.Slf4j; /** @@ -38,17 +40,25 @@ public class StepUpAutoConfiguration { * Creates the built-in step-up service, validating the configured factor names first. * * @param config the step-up configuration + * @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) { + public StepUpService dsFactorFreshnessStepUpService(StepUpConfigProperties config, + ObjectProvider credentialServiceProvider) { validateFactors(config.getFactors()); log.info("Step-up enabled: a factor of {} issued within {}s authorizes credential-altering operations", config.getFactors(), config.getTtlSeconds()); - return new DSFactorFreshnessStepUpService(config); + // 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); + }); } /** 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..146656fe 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,28 @@ 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}) + * @return {@code true} if the user could satisfy step-up; {@code false} if no amount of user action would + */ + default boolean canSatisfyStepUp(User user) { + return true; + } } 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..aa05a16f 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java @@ -818,6 +818,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)).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 +845,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)).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 +862,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)).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)).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/security/DSFactorFreshnessStepUpServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpServiceTest.java index 381d4866..2ff3e981 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpServiceTest.java @@ -138,12 +138,62 @@ void shouldDenyWhenUserIsNull() { } } + @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)).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)).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)).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)).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)).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)).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); + return new DSFactorFreshnessStepUpService(config, u -> userHasPasskey); } private static void authenticate(String name, GrantedAuthority... authorities) { From 01a84358d86e288a35bc1cc8de026c925cd8a423 Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Thu, 20 Aug 2026 13:14:28 -0600 Subject: [PATCH 3/6] fix: apply step-up satisfiability to passkey endpoints and guard the config Follow-up to 7f84430, from review of that commit. requireCredentialProof did not consult canSatisfyStepUp. The reasoning was that rename and delete imply the user holds a passkey, which is true but answers the wrong question: what matters is whether they hold a credential the *configured* factors accept. validateFactors accepts factors=[PASSWORD] alone, and a passwordless passkey-holder then needs a FACTOR_PASSWORD it can never produce, so rename and non-last delete returned 401 permanently. That is the dead end 7f84430 removed from setPassword, recreated one API over. Also: - canSatisfyStepUp gains the action parameter, matching isStepUpSatisfied, so an implementation whose requirements differ per operation can say so. Free now; breaking for implementors after release. - Startup fails when WEBAUTHN is a step-up factor while WebAuthn is disabled, and PASSWORD logs a warning, mirroring MfaConfiguration. The rejected combination previously started clean and left step-up inert. - A null user keeps the gate rather than releasing it. The SPI says user is never null, and releasing a security gate is the wrong answer to a contract violation. - Locale.ROOT in buildManager and validateFactors. Under a Turkish default locale "authorization_code" uppercased to a dotted I and missed the factor map, failing startup on valid configuration. Tests: the auto-configuration lambda is now pinned (present, absent, and delegating to real credential state), the SPI default is pinned at true so the compatibility promise cannot be flipped silently, and the locale normalization is covered under @ResourceLock(LOCALE). ./gradlew check --rerun-tasks: green. ./gradlew javadoc: clean. --- MIGRATION.md | 6 +- .../spring/user/api/UserAPI.java | 2 +- .../user/api/WebAuthnManagementAPI.java | 6 +- .../DSFactorFreshnessStepUpService.java | 13 +-- .../security/StepUpAutoConfiguration.java | 20 ++++- .../spring/user/security/StepUpService.java | 4 +- .../spring/user/api/UserAPIUnitTest.java | 8 +- .../api/WebAuthnManagementAPIStepUpTest.java | 17 ++++ .../DSFactorFreshnessStepUpServiceTest.java | 20 +++-- .../security/StepUpAutoConfigurationTest.java | 84 ++++++++++++++++++- 10 files changed, 157 insertions(+), 23 deletions(-) diff --git a/MIGRATION.md b/MIGRATION.md index ff906f58..18157bd4 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -225,7 +225,11 @@ Affected endpoints (all require `user.webauthn.enabled=true` except where noted) **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. -**`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`. +**`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. 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 0a77de95..c819cdce 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java +++ b/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java @@ -573,7 +573,7 @@ public ResponseEntity setPassword(@AuthenticationPrincipal DSUserD // 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)) { + 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, 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 d35442c2..04d52cfc 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java +++ b/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java @@ -265,8 +265,12 @@ private void requireCredentialProof(User user, String action, String currentPass // 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). StepUpService stepUpService = stepUpServiceProvider.getIfAvailable(); - if (stepUpService != null && !stepUpService.isStepUpSatisfied(user, action, request)) { + 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."); } diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpService.java b/src/main/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpService.java index ea9aa826..c02e1bf2 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpService.java @@ -66,7 +66,7 @@ public class DSFactorFreshnessStepUpService implements StepUpService { * * @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)} to tell "has not asserted" apart from "has nothing to assert with" + * {@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; @@ -79,7 +79,7 @@ public DSFactorFreshnessStepUpService(StepUpConfigProperties config, Predicate buildManager(String factorName, Duration ttl) { - String authority = StepUpConfigProperties.FACTOR_AUTHORITIES.get(factorName.toUpperCase()); + String authority = StepUpConfigProperties.FACTOR_AUTHORITIES.get(factorName.toUpperCase(Locale.ROOT)); return AllRequiredFactorsAuthorizationManager.builder() .requireFactor(RequiredFactor.withAuthority(authority).validDuration(ttl).build()).build(); } @@ -130,12 +130,14 @@ public boolean isStepUpSatisfied(User user, String action, HttpServletRequest re *

* * @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) { + public boolean canSatisfyStepUp(User user, String action) { if (user == null) { - return false; + // 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)) { @@ -154,7 +156,8 @@ public boolean canSatisfyStepUp(User user) { } } } - log.debug("Step-up does not apply: the account holds no credential able to produce any of {}", factorNames); + 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/StepUpAutoConfiguration.java b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfiguration.java index 186fdac5..d8f258e9 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfiguration.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfiguration.java @@ -1,6 +1,7 @@ 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; @@ -40,6 +41,8 @@ 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} @@ -49,8 +52,9 @@ public class StepUpAutoConfiguration { @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()); + 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 @@ -81,18 +85,28 @@ public FactorAuthorityNameValidator factorAuthorityNameValidator(RolesAndPrivile * 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) { + 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())) + || !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() { 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 146656fe..9584f8f9 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/StepUpService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpService.java @@ -65,9 +65,11 @@ public interface StepUpService { *

* * @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) { + default boolean canSatisfyStepUp(User user, String action) { return true; } } 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 aa05a16f..9b4e4bf5 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java @@ -820,7 +820,7 @@ void setPassword_stepUpService_deniesReturns401() throws Exception { 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)).thenReturn(true); + 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))) @@ -847,7 +847,7 @@ void setPassword_stepUpService_grantsProceeds() throws Exception { 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)).thenReturn(true); + 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))) @@ -872,7 +872,7 @@ void setPassword_stepUpUnsatisfiable_fallsBackToDisabledDefault() throws Excepti 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)).thenReturn(false); + 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."); @@ -896,7 +896,7 @@ void setPassword_stepUpUnsatisfiable_allowedWhenFlagEnabled() throws Exception { when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser); when(userService.hasPassword(testUser)).thenReturn(false); StepUpService stepUp = mock(StepUpService.class); - when(stepUp.canSatisfyStepUp(testUser)).thenReturn(false); + 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))) diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIStepUpTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIStepUpTest.java index a428ff5a..f61e1356 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIStepUpTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIStepUpTest.java @@ -89,6 +89,7 @@ void provideStepUpService() { @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)) @@ -96,9 +97,23 @@ void shouldRejectDeleteWithoutStepUp() { 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", @@ -110,6 +125,7 @@ void shouldRejectRenameWithoutStepUp() { @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(); @@ -119,6 +135,7 @@ void shouldAllowDeleteWithStepUp() { @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); diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpServiceTest.java index 2ff3e981..5c3ac9d4 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpServiceTest.java @@ -145,13 +145,13 @@ 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)).isFalse(); + 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)).isTrue(); + assertThat(service(true, 120, "WEBAUTHN").canSatisfyStepUp(user, ACTION)).isTrue(); } @Test @@ -159,13 +159,13 @@ void shouldReportSatisfiableWhenUserHasPasskey() { void shouldReportSatisfiableWhenUserHasPasswordAndPasswordFactorConfigured() { user.setPassword("$2a$04$encoded"); - assertThat(service(false, 120, "PASSWORD").canSatisfyStepUp(user)).isTrue(); + 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)).isFalse(); + assertThat(service(false, 120, "PASSWORD").canSatisfyStepUp(user, ACTION)).isFalse(); } @Test @@ -173,14 +173,22 @@ void shouldReportUnsatisfiableWhenPasswordlessAndPasswordFactorConfigured() { void shouldReportSatisfiableWhenAnyConfiguredFactorIsAchievable() { user.setPassword("$2a$04$encoded"); - assertThat(service(false, 120, "WEBAUTHN", "PASSWORD").canSatisfyStepUp(user)).isTrue(); + 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)).isTrue(); + assertThat(service(false, 120, "OTT").canSatisfyStepUp(user, ACTION)).isTrue(); } } diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfigurationTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfigurationTest.java index 5b946633..a804a9b2 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfigurationTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfigurationTest.java @@ -2,13 +2,19 @@ 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}. @@ -23,7 +29,83 @@ class StepUpAutoConfigurationTest { private final ApplicationContextRunner runner = new ApplicationContextRunner() .withConfiguration(AutoConfigurations.of(StepUpAutoConfiguration.class)) .withBean(RolesAndPrivilegesConfig.class, RolesAndPrivilegesConfig::new) - .withBean(MfaConfigProperties.class, MfaConfigProperties::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 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") From e47fb7439f2e3a3120a50351530fabaa5b1e10d6 Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Thu, 20 Aug 2026 18:21:30 -0600 Subject: [PATCH 4/6] feat: stamp an authentication factor on login flows that leave none Groundwork for gating passkey enrollment on recent authentication. A freshness gate can only work if every interactive login leaves a factor to measure; verification found three paths that leave none. Verified in Spring Security 7.1.0 bytecode: the password provider stamps FACTOR_PASSWORD and OAuth2LoginAuthenticationProvider stamps FACTOR_AUTHORIZATION_CODE, but OidcAuthorizationCodeAuthenticationProvider stamps nothing. An OIDC login (Keycloak, or Google configured with the openid scope) therefore produces a session carrying no factor at all. UserService.authWithoutPassword has the same problem for a different reason: it builds its own Authentication rather than running a provider. It has three call sites, and only one is dev-only. The email-verification link and post-registration auto-login are production paths, so without this every newly registered user's first session would carry no factor and they could not enroll their first passkey until logging out and back in. - AuthWithoutPasswordFactor names what each path actually proves: EMAIL_VERIFICATION is a one-time token, REGISTRATION follows a password submitted in that same request, DEV_LOGIN proves nothing and stamps nothing. The single-argument authWithoutPassword delegates to DEV_LOGIN, so existing callers are unchanged. - LoginFactorStamper adds FACTOR_AUTHORIZATION_CODE only when no factor is present at all. The discriminator is deliberately not the token type: on both the OAuth2 and OIDC paths this framework's user services return a DSUserDetails carrying database roles, so the usual OidcUserAuthority versus OAuth2UserAuthority test does not apply here. Checking for the factor itself avoids duplicating the one the OAuth2 provider adds after the mapper runs. - LoginSuccessService applies it, rebuilding only OAuth2AuthenticationToken and logging anything else rather than guessing how to reconstruct it. The context is written back explicitly, since AbstractAuthenticationProcessingFilter saves it before the success handler runs, matching WebAuthnAuthenticationSuccessHandler. No gate yet, so nothing changes for users. ./gradlew check --rerun-tasks: green. ./gradlew javadoc: clean. --- .../spring/user/api/UserAPI.java | 4 +- .../user/controller/UserActionController.java | 4 +- .../spring/user/dev/DevLoginController.java | 4 +- .../service/AuthWithoutPasswordFactor.java | 65 +++++++++++++ .../user/service/LoginFactorStamper.java | 47 ++++++++++ .../user/service/LoginSuccessService.java | 60 ++++++++++++ .../spring/user/service/UserService.java | 19 +++- .../spring/user/api/UserAPIUnitTest.java | 3 +- .../controller/UserActionControllerTest.java | 5 +- .../user/dev/DevLoginControllerTest.java | 8 +- .../AuthWithoutPasswordFactorTest.java | 93 +++++++++++++++++++ .../LoginSuccessFactorStampingTest.java | 65 +++++++++++++ .../spring/user/service/UserServiceTest.java | 34 +++++++ 13 files changed, 402 insertions(+), 9 deletions(-) create mode 100644 src/main/java/com/digitalsanctuary/spring/user/service/AuthWithoutPasswordFactor.java create mode 100644 src/main/java/com/digitalsanctuary/spring/user/service/LoginFactorStamper.java create mode 100644 src/test/java/com/digitalsanctuary/spring/user/service/AuthWithoutPasswordFactorTest.java create mode 100644 src/test/java/com/digitalsanctuary/spring/user/service/LoginSuccessFactorStampingTest.java 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 c819cdce..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; @@ -642,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/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/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/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java index 9b4e4bf5..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 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/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() { From cd795596a71d19ec07b849dac1396554ff44c808 Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Thu, 20 Aug 2026 18:51:59 -0600 Subject: [PATCH 5/6] feat: gate passkey enrollment on recent authentication (closes E1) Enrolling a passkey was ungated, which meant step-up protected nothing. An attacker holding only a stolen session cookie on a passwordless account could register their own authenticator, assert with it to mint a genuinely fresh FACTOR_WEBAUTHN, and thereby satisfy every gate the feature adds. Worse, enabling step-up *opened* a path: for a social-login account, POST /user/setPassword is closed by allowInitialPasswordSetWithoutStepUp when step-up is off, but reachable via a self-enrolled passkey when it is on. POST /webauthn/register now requires an authentication factor issued within user.security.stepUp.enrollmentTtlSeconds (default 600), applied only while step-up is enabled. This follows GitHub's sudo mode, which asks for a password before adding a security key. Any factor counts, deliberately not the configured stepUp.factors list: with the default [WEBAUTHN] that would demand a passkey in order to register a first passkey, which nobody could satisfy. A plain authority merely named like a factor carries no issue time and can never be fresh, so it cannot forge recency. The window is separate from and longer than ttlSeconds because enrollment follows a login and a decision, whereas a step-up ceremony immediately precedes its operation. Wiring is an authorizeHttpRequests rule, which works because WebAuthnRegistrationFilter is registered after AuthorizationFilter. That ordering is the assumption the whole gate rests on, so WebAuthnEnrollmentGateIntegrationTest exercises the real filter chain rather than the manager alone; disabling the gate fails both of its rejection cases. Also corrects the threat-model claim this work started from. The JavaDoc, MIGRATION.md and the ticket all asserted that an attacker with a session cookie and no authenticator "can never produce a recent factor". That was false while enrollment was open, and the sentence had been copied into three places. It now states what is actually enforced, including the two residuals that remain: same-session piggybacking, and a session stolen within the enrollment window of a real login. ./gradlew check --rerun-tasks: green. ./gradlew javadoc: clean. --- CONFIG.md | 8 ++ MIGRATION.md | 6 +- .../user/api/WebAuthnManagementAPI.java | 2 +- .../DSFactorFreshnessStepUpService.java | 12 +- .../FreshFactorAuthorizationManager.java | 67 ++++++++++ .../user/security/StepUpConfigProperties.java | 16 +++ .../user/security/WebSecurityConfig.java | 17 +++ .../config/dsspringuserconfig.properties | 3 + .../FreshFactorAuthorizationManagerTest.java | 121 ++++++++++++++++++ ...WebAuthnEnrollmentGateIntegrationTest.java | 76 +++++++++++ 10 files changed, 323 insertions(+), 5 deletions(-) create mode 100644 src/main/java/com/digitalsanctuary/spring/user/security/FreshFactorAuthorizationManager.java create mode 100644 src/test/java/com/digitalsanctuary/spring/user/security/FreshFactorAuthorizationManagerTest.java create mode 100644 src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnEnrollmentGateIntegrationTest.java diff --git a/CONFIG.md b/CONFIG.md index 63180bc8..0abbb4fd 100644 --- a/CONFIG.md +++ b/CONFIG.md @@ -146,6 +146,14 @@ user: **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. diff --git a/MIGRATION.md b/MIGRATION.md index 18157bd4..ba67f805 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. @@ -225,6 +225,10 @@ Affected endpoints (all require `user.webauthn.enabled=true` except where noted) **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. 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 04d52cfc..b2d34b9c 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java +++ b/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java @@ -64,7 +64,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. *

diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpService.java b/src/main/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpService.java index c02e1bf2..058e6f7d 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/DSFactorFreshnessStepUpService.java @@ -39,9 +39,15 @@ * 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, who can never produce a recent factor. It leaves a narrower one open: an - * attacker sharing the session concurrently can piggyback inside the window. Keep the TTL short. + * 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. *

* *

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/StepUpConfigProperties.java b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpConfigProperties.java index ae675df5..b9947e20 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/StepUpConfigProperties.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpConfigProperties.java @@ -59,6 +59,22 @@ public class StepUpConfigProperties { @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 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/resources/config/dsspringuserconfig.properties b/src/main/resources/config/dsspringuserconfig.properties index 7d2b8443..edafb1f0 100644 --- a/src/main/resources/config/dsspringuserconfig.properties +++ b/src/main/resources/config/dsspringuserconfig.properties @@ -99,6 +99,9 @@ user.security.testHashTime=true 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. 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/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)); + } +} From b163e8afe83301579f28a503e1ed30c9e7ef7cdd Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Thu, 20 Aug 2026 19:45:29 -0600 Subject: [PATCH 6/6] fix: close the remaining step-up acceptance criteria (A3, B3, E3, F1) A3 - passkey delete/rename keyed off StepUpService bean presence, so an application that adopted the SPI in 5.3.1 for setPassword would have had two more operations gated on upgrade, with two action values its implementation never expected. The opt-in is now the property. B3 - @Min(1) on ttlSeconds was inert because StepUpConfigProperties was not @Validated, unlike its siblings. ttlSeconds=0 bound cleanly and made every gated operation deny forever with a debug log as the only signal. Removing @Validated again fails the two new tests, so the annotation is what enforces it. enrollmentTtlSeconds is covered too. E3 - FactorAuthorityNameValidator read configuration only, but AuthorityService grants from the role and privilege tables and RolePrivilegeSetupService never deletes. A FACTOR_-prefixed row created under an earlier configuration therefore survived its removal from YAML, kept being granted, and passed the check clean, leaving the MFA bypass the class exists to prevent. It now queries both tables as well, and the error message says to delete the rows rather than only rename the config. The check also moved from @PostConstruct to ContextRefreshedEvent: nothing injects this bean, so under spring.main.lazy-initialization=true it was never constructed and the check silently never ran. The event also orders it after RolePrivilegeSetupService. Matches the pattern that service and MfaConfiguration already use. F1 - the 401 and its step-up-required code had no test, though the distinct code is the entire reason the exception type exists. A second test pins that it does not fall through to the base WebAuthnException handler, which would return 400 with a null error field. Also corrects the property path in the validator's message and in MIGRATION.md: the real key is user.roles.roles-and-privileges, so an operator hit by the failure would have grepped for a name that does not exist. ./gradlew check --rerun-tasks: green. ./gradlew javadoc: clean. --- MIGRATION.md | 2 +- .../user/api/WebAuthnManagementAPI.java | 8 ++- .../FactorAuthorityNameValidator.java | 50 +++++++++++-- .../security/StepUpAutoConfiguration.java | 12 +++- .../user/security/StepUpConfigProperties.java | 2 + .../api/WebAuthnManagementAPIAdviceTest.java | 26 +++++++ .../api/WebAuthnManagementAPIStepUpTest.java | 29 +++++++- .../user/api/WebAuthnManagementAPITest.java | 7 ++ .../FactorAuthorityNameValidatorTest.java | 72 ++++++++++++++++++- .../security/StepUpAutoConfigurationTest.java | 16 +++++ 10 files changed, 212 insertions(+), 12 deletions(-) diff --git a/MIGRATION.md b/MIGRATION.md index ba67f805..af2ef219 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -239,7 +239,7 @@ Enabling it also gates passkey **delete and rename** on passwordless accounts, w Two things to check before enabling it: -- **Reserved authority names.** A role or privilege named `FACTOR_*` in `user.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 configured while MFA or step-up is enabled, and logs an error otherwise. Rename them. +- **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. 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 b2d34b9c..015f9a99 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java +++ b/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java @@ -23,6 +23,7 @@ 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; @@ -83,6 +84,8 @@ public class WebAuthnManagementAPI { 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. @@ -268,7 +271,10 @@ private void requireCredentialProof(User user, String action, String currentPass // 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). - StepUpService stepUpService = stepUpServiceProvider.getIfAvailable(); + // 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( diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/FactorAuthorityNameValidator.java b/src/main/java/com/digitalsanctuary/spring/user/security/FactorAuthorityNameValidator.java index d28f3aff..3a7a077f 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/FactorAuthorityNameValidator.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/FactorAuthorityNameValidator.java @@ -1,11 +1,18 @@ 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 jakarta.annotation.PostConstruct; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; @@ -36,7 +43,7 @@ */ @Slf4j @RequiredArgsConstructor -public class FactorAuthorityNameValidator { +public class FactorAuthorityNameValidator implements ApplicationListener { /** Prefix Spring Security reserves for authentication-factor authorities. */ static final String FACTOR_PREFIX = "FACTOR_"; @@ -44,24 +51,37 @@ public class FactorAuthorityNameValidator { 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 */ - @PostConstruct + @Override + public void onApplicationEvent(ContextRefreshedEvent event) { + validateAuthorityNames(); + } + public void validateAuthorityNames() { List offenders = findOffendingNames(); if (offenders.isEmpty()) { return; } - String message = "user.roles-and-privileges declares " + offenders + ", which collide with Spring Security's reserved " + 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 these roles/privileges."; + + "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); @@ -79,7 +99,25 @@ public List findOffendingNames() { Set offenders = new TreeSet<>(); rolesAndPrivilegesConfig.getRolesAndPrivileges().forEach((role, privileges) -> Stream .concat(Stream.of(role), privileges == null ? Stream.empty() : privileges.stream()) - .filter(name -> name != null && name.toUpperCase().startsWith(FACTOR_PREFIX)).forEach(offenders::add)); + .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/StepUpAutoConfiguration.java b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfiguration.java index d8f258e9..643d97cf 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfiguration.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfiguration.java @@ -12,6 +12,8 @@ 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; @@ -72,13 +74,19 @@ public StepUpService dsFactorFreshnessStepUpService(StepUpConfigProperties confi * @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) { - return new FactorAuthorityNameValidator(rolesAndPrivilegesConfig, mfaConfigProperties, stepUpConfigProperties); + MfaConfigProperties mfaConfigProperties, StepUpConfigProperties stepUpConfigProperties, + ObjectProvider roleRepositoryProvider, + ObjectProvider privilegeRepositoryProvider) { + return new FactorAuthorityNameValidator(rolesAndPrivilegesConfig, mfaConfigProperties, stepUpConfigProperties, + roleRepositoryProvider, privilegeRepositoryProvider); } /** diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/StepUpConfigProperties.java b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpConfigProperties.java index b9947e20..b9a4cfce 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/StepUpConfigProperties.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/StepUpConfigProperties.java @@ -4,6 +4,7 @@ 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; @@ -24,6 +25,7 @@ *

*/ @Data +@Validated @ConfigurationProperties(prefix = "user.security.step-up") public class StepUpConfigProperties { 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 index f61e1356..48c914c1 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIStepUpTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPIStepUpTest.java @@ -20,6 +20,7 @@ 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; @@ -62,6 +63,8 @@ class WebAuthnManagementAPIStepUpTest { private WebAuthnManagementAPI api; + private StepUpConfigProperties stepUpConfig; + private User testUser; private final MockHttpServletRequest request = new MockHttpServletRequest(); @@ -69,8 +72,11 @@ class WebAuthnManagementAPIStepUpTest { @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); + 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. @@ -160,4 +166,25 @@ void shouldAllowDeleteWhenNoStepUpServiceIsConfigured() { 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 95398dea..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,6 +17,7 @@ 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; @@ -30,6 +31,7 @@ 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; @@ -71,6 +73,11 @@ class WebAuthnManagementAPITest { @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; diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/FactorAuthorityNameValidatorTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/FactorAuthorityNameValidatorTest.java index 64adb062..2d8306b3 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/security/FactorAuthorityNameValidatorTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/security/FactorAuthorityNameValidatorTest.java @@ -1,12 +1,19 @@ 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; /** @@ -82,10 +89,73 @@ private static FactorAuthorityNameValidator validator(Map> } 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); + 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/StepUpAutoConfigurationTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfigurationTest.java index a804a9b2..70dac335 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfigurationTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/security/StepUpAutoConfigurationTest.java @@ -50,6 +50,22 @@ void shouldFailWhenWebAuthnFactorConfiguredButWebAuthnDisabled() { .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() {