From 4f5f07217b701c2f02a0c1954aa94c84ba073d90 Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Tue, 18 Aug 2026 22:21:35 -0600 Subject: [PATCH 1/3] test: pin Spring Security factor-freshness behavior for WebAuthn step-up Adds WebAuthnStepUpFactorAssumptionsTest, which characterizes the four Spring Security 7.1 behaviors the planned built-in step-up primitive (#335) is built on, none of which are this framework's code: - RequiredFactor.validDuration denies a stale WEBAUTHN factor and grants a fresh one. - AbstractAuthenticationProcessingFilter with mfaEnabled=true merges a re-assertion into the existing session, refreshing the WEBAUTHN factor's issuedAt while retaining the user's other authorities, which flips a freshness gate from denied to granted. - With merging disabled the session's authorities are replaced instead, so merging has to be switched on deliberately. - The refreshed factor survives WebAuthnAuthenticationSuccessHandler's DSUserDetails principal conversion. These pin third-party behavior, so a Spring Security upgrade that changes it fails here rather than silently weakening step-up. Refs #335 --- .../WebAuthnStepUpFactorAssumptionsTest.java | 214 ++++++++++++++++++ 1 file changed, 214 insertions(+) create mode 100644 src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnStepUpFactorAssumptionsTest.java diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnStepUpFactorAssumptionsTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnStepUpFactorAssumptionsTest.java new file mode 100644 index 0000000..641089b --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnStepUpFactorAssumptionsTest.java @@ -0,0 +1,214 @@ +package com.digitalsanctuary.spring.user.security; + +import static org.assertj.core.api.Assertions.assertThat; +import java.time.Duration; +import java.time.Instant; +import java.util.List; +import java.util.Set; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.springframework.mock.web.MockFilterChain; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.security.authentication.TestingAuthenticationToken; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; +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.GrantedAuthority; +import org.springframework.security.core.authority.FactorGrantedAuthority; +import org.springframework.security.core.authority.SimpleGrantedAuthority; +import org.springframework.security.core.context.SecurityContext; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.security.core.userdetails.UserDetailsService; +import org.springframework.security.web.authentication.AbstractAuthenticationProcessingFilter; +import org.springframework.security.web.authentication.AuthenticationSuccessHandler; +import org.springframework.security.web.webauthn.api.Bytes; +import org.springframework.security.web.webauthn.api.ImmutablePublicKeyCredentialUserEntity; +import org.springframework.security.web.webauthn.api.PublicKeyCredentialUserEntity; +import org.springframework.security.web.webauthn.authentication.WebAuthnAuthentication; +import com.digitalsanctuary.spring.user.persistence.model.User; +import com.digitalsanctuary.spring.user.service.DSUserDetails; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; + +/** + * Characterization tests for the Spring Security behaviour the WebAuthn step-up design depends on (issue #335). + * + *

+ * The built-in step-up primitive is built on Spring Security's own factor machinery rather than on a bespoke + * challenge/verify flow: a sensitive operation requires a {@code FACTOR_WEBAUTHN} + * {@link FactorGrantedAuthority} issued within a short TTL, and the user refreshes it by re-running the ordinary + * passkey assertion at {@code /login/webauthn}. Three framework behaviours have to hold for that to work, and none of + * them are this framework's code, so they are pinned here and will fail loudly on a Spring Security upgrade that + * changes them: + *

+ *
    + *
  1. Freshness enforcement — {@code RequiredFactor.validDuration} denies a stale WEBAUTHN factor and + * grants a fresh one.
    2. + *
  2. Refresh — re-running the assertion while already authenticated produces a WEBAUTHN + * {@link FactorGrantedAuthority} with a new {@code issuedAt} while preserving the session's other authorities. This is + * the merging half of {@link AbstractAuthenticationProcessingFilter} that {@code setMfaEnabled(true)} activates; with + * it off, the second authentication REPLACES the first and step-up would drop the user's roles.
    3. + *
  3. Survival — the refreshed factor survives this framework's + * {@link WebAuthnAuthenticationSuccessHandler}, which rebuilds the authentication to swap in {@code DSUserDetails}.
    4. + *
+ * + *

+ * The merging logic lives in {@code AbstractAuthenticationProcessingFilter#doFilter}, shared by every authentication + * filter, and its only type-sensitive step is a reflective {@code declaresToBuilder(authenticationResult)} check. The + * filter here therefore returns a real {@link WebAuthnAuthentication} (which does declare {@code toBuilder()}) and + * stubs only the assertion verification, which needs an authenticator. The real + * {@code WebAuthnAuthenticationProvider} stamps {@code FactorGrantedAuthority.fromAuthority("FACTOR_WEBAUTHN")}, whose + * {@code issuedAt} defaults to now, so the freshness clock is driven by the genuine login path. + *

+ */ +@DisplayName("WebAuthn Step-Up Factor Assumptions Tests") +class WebAuthnStepUpFactorAssumptionsTest { + + private static final String EMAIL = "passkey-user@test.com"; + private static final Duration STEP_UP_TTL = Duration.ofMinutes(5); + + private final AuthorizationManager freshWebAuthnRequired = AllRequiredFactorsAuthorizationManager.builder() + .requireFactor(RequiredFactor.withAuthority(FactorGrantedAuthority.WEBAUTHN_AUTHORITY).validDuration(STEP_UP_TTL).build()) + .build(); + + @AfterEach + void clearContext() { + SecurityContextHolder.clearContext(); + } + + @Test + @DisplayName("should deny a stale WEBAUTHN factor and grant a fresh one when a validDuration is required") + void shouldEnforceFreshnessWhenValidDurationIsRequired() { + Authentication stale = authWith(webAuthnFactor(Instant.now().minus(Duration.ofMinutes(30)))); + Authentication fresh = authWith(webAuthnFactor(Instant.now())); + + assertThat(granted(freshWebAuthnRequired, stale)).as("30-minute-old WEBAUTHN factor against a 5-minute TTL").isFalse(); + assertThat(granted(freshWebAuthnRequired, fresh)).as("just-issued WEBAUTHN factor against a 5-minute TTL").isTrue(); + } + + @Test + @DisplayName("should refresh issuedAt and keep existing authorities when re-asserting while already authenticated") + void shouldRefreshWebAuthnFactorWhenReAssertingWhileAuthenticated() throws Exception { + Instant staleIssuedAt = Instant.now().minus(Duration.ofMinutes(30)); + Instant freshIssuedAt = Instant.now(); + + // The session as it stands before step-up: an old passkey login plus the user's role authorities. + SecurityContext context = SecurityContextHolder.createEmptyContext(); + context.setAuthentication(new UsernamePasswordAuthenticationToken(EMAIL, "n/a", + List.of(webAuthnFactor(staleIssuedAt), new SimpleGrantedAuthority("ROLE_USER")))); + SecurityContextHolder.setContext(context); + + Authentication merged = runAssertion(new WebAuthnAuthentication(userEntity(), Set.of(webAuthnFactor(freshIssuedAt))), true); + + assertThat(webAuthnFactorsOf(merged)).as("exactly one WEBAUTHN factor survives the merge (deduped by authority string)").hasSize(1); + assertThat(webAuthnFactorsOf(merged).get(0).getIssuedAt()).as("the surviving WEBAUTHN factor is the newly issued one") + .isEqualTo(freshIssuedAt); + assertThat(authorityStrings(merged)).as("non-factor authorities from the existing session are carried over") + .contains("ROLE_USER"); + + // The step-up assertion itself: the same gate that denied before the ceremony now grants. + assertThat(granted(freshWebAuthnRequired, context.getAuthentication())).as("gate before step-up").isFalse(); + assertThat(granted(freshWebAuthnRequired, merged)).as("gate after step-up").isTrue(); + } + + @Test + @DisplayName("should replace the session authorities when re-asserting with factor merging disabled") + void shouldReplaceAuthoritiesWhenMergingIsDisabled() throws Exception { + SecurityContext context = SecurityContextHolder.createEmptyContext(); + context.setAuthentication(new UsernamePasswordAuthenticationToken(EMAIL, "n/a", + List.of(webAuthnFactor(Instant.now().minus(Duration.ofMinutes(30))), new SimpleGrantedAuthority("ROLE_USER")))); + SecurityContextHolder.setContext(context); + + Authentication result = runAssertion(new WebAuthnAuthentication(userEntity(), Set.of(webAuthnFactor(Instant.now()))), false); + + assertThat(authorityStrings(result)).as("ROLE_USER is dropped when mfaEnabled=false, so merging must be switched on for step-up") + .doesNotContain("ROLE_USER"); + } + + @Test + @DisplayName("should preserve the refreshed factor when the success handler converts the principal to DSUserDetails") + void shouldPreserveRefreshedFactorWhenPrincipalIsConverted() throws Exception { + Instant freshIssuedAt = Instant.now(); + Set merged = Set.of(webAuthnFactor(freshIssuedAt), new SimpleGrantedAuthority("ROLE_USER")); + + User user = new User(); + user.setEmail(EMAIL); + user.setFirstName("Passkey"); + user.setLastName("User"); + UserDetailsService userDetailsService = username -> new DSUserDetails(user, merged); + + CapturingSuccessHandler captor = new CapturingSuccessHandler(); + WebAuthnAuthenticationSuccessHandler handler = new WebAuthnAuthenticationSuccessHandler(userDetailsService, captor, null); + + handler.onAuthenticationSuccess(new MockHttpServletRequest(), new MockHttpServletResponse(), + new WebAuthnAuthentication(userEntity(), merged)); + + assertThat(captor.captured.getPrincipal()).isInstanceOf(DSUserDetails.class); + assertThat(webAuthnFactorsOf(captor.captured)).hasSize(1); + assertThat(webAuthnFactorsOf(captor.captured).get(0).getIssuedAt()).as("issuedAt is preserved through the principal swap") + .isEqualTo(freshIssuedAt); + assertThat(granted(freshWebAuthnRequired, captor.captured)).as("gate still grants after conversion").isTrue(); + } + + /** + * Drives one authentication-filter pass with a stubbed assertion result, returning the authentication the filter + * hands to its success handler (i.e. after any factor merging). + */ + private Authentication runAssertion(WebAuthnAuthentication result, boolean mergingEnabled) throws Exception { + CapturingSuccessHandler captor = new CapturingSuccessHandler(); + AbstractAuthenticationProcessingFilter filter = new AbstractAuthenticationProcessingFilter(request -> true) { + @Override + public Authentication attemptAuthentication(HttpServletRequest request, HttpServletResponse response) { + return result; + } + }; + filter.setAuthenticationManager(authentication -> authentication); + filter.setAuthenticationSuccessHandler(captor); + filter.setMfaEnabled(mergingEnabled); + + filter.doFilter(new MockHttpServletRequest("POST", "/login/webauthn"), new MockHttpServletResponse(), new MockFilterChain()); + return captor.captured; + } + + private static boolean granted(AuthorizationManager manager, Authentication authentication) { + AuthorizationResult result = manager.authorize(() -> authentication, new Object()); + return result != null && result.isGranted(); + } + + private static Authentication authWith(GrantedAuthority... authorities) { + return new TestingAuthenticationToken(EMAIL, "n/a", List.of(authorities)); + } + + private static FactorGrantedAuthority webAuthnFactor(Instant issuedAt) { + return FactorGrantedAuthority.withAuthority(FactorGrantedAuthority.WEBAUTHN_AUTHORITY).issuedAt(issuedAt).build(); + } + + private static List authorityStrings(Authentication authentication) { + return authentication.getAuthorities().stream().map(GrantedAuthority::getAuthority).toList(); + } + + private static List webAuthnFactorsOf(Authentication authentication) { + return authentication.getAuthorities().stream().filter(FactorGrantedAuthority.class::isInstance) + .map(FactorGrantedAuthority.class::cast) + .filter(factor -> FactorGrantedAuthority.WEBAUTHN_AUTHORITY.equals(factor.getAuthority())).toList(); + } + + private static PublicKeyCredentialUserEntity userEntity() { + return ImmutablePublicKeyCredentialUserEntity.builder().name(EMAIL).id(new Bytes(new byte[] {1, 2, 3})).displayName("Passkey User") + .build(); + } + + private static final class CapturingSuccessHandler implements AuthenticationSuccessHandler { + private Authentication captured; + + @Override + public void onAuthenticationSuccess(HttpServletRequest request, HttpServletResponse response, Authentication authentication) { + this.captured = authentication; + } + } +} From 060e7c03f381f3aa0d1846156c324604294b4cc4 Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Wed, 19 Aug 2026 10:31:52 -0600 Subject: [PATCH 2/3] test: drive the real passkey path in step-up assumption tests Review of #364 found two tests that could not fail and a class JavaDoc that overstated what was exercised. Make the tests discriminating: - shouldPreserveRefreshedFactorWhenPrincipalIsConverted passed the same authority set to both the stubbed UserDetailsService and the incoming authentication, so it could not tell which one the handler reads. The stub now returns roles only, with the factor solely on the incoming authentication. Same flaw and same fix in the pre-existing WebAuthnAuthenticationSuccessHandlerTest.shouldPreserveAuthorities. Verified by mutation: pointing the handler at userDetails.getAuthorities() leaves both original tests green and fails both revised ones. - runAssertion now drives the real WebAuthnAuthenticationProvider through the filter's AuthenticationManager instead of returning a prebuilt WebAuthnAuthentication, so the FACTOR_WEBAUTHN stamping and its issuedAt default are genuinely exercised rather than assumed. Only WebAuthnRelyingPartyOperations.authenticate is stubbed; it needs an authenticator. - The pre-step-up session is now a WebAuthnAuthenticationToken over DSUserDetails, matching what a completed passkey login leaves in the context, so the name-equality gate in shouldPerformMfa is satisfied the way production satisfies it rather than by construction. - The merging-disabled control asserts the exact resulting authority set and gives the session a FACTOR_PASSWORD the UserDetailsService does not re-supply, so it shows a real loss rather than only an absence. Add two cases: the provider stamps a fresh factor onto the user's authorities, and a plain SimpleGrantedAuthority named FACTOR_WEBAUTHN does not satisfy a validDuration requirement. The latter matters because consumers can name any authority in user.roles-and-privileges. Correct the class JavaDoc: the step-up primitive is proposed in #335, not shipped (StepUpService is a consumer SPI and no production code sets validDuration); the merge is inherited only by AbstractAuthenticationProcessingFilter subclasses, not every authentication filter; all four shouldPerformMfa gates are named; and the success-handler test is a local regression guard, not a Spring Security upgrade tripwire. No production code changes. ./gradlew test: 275 suites, 1171 tests, 0 failures. --- ...AuthnAuthenticationSuccessHandlerTest.java | 5 +- .../WebAuthnStepUpFactorAssumptionsTest.java | 212 +++++++++++++----- 2 files changed, 157 insertions(+), 60 deletions(-) diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationSuccessHandlerTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationSuccessHandlerTest.java index eda22c8..c2f436b 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationSuccessHandlerTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnAuthenticationSuccessHandlerTest.java @@ -173,7 +173,10 @@ void shouldPreserveAuthorities() throws Exception { WebAuthnAuthentication webAuthnAuth = new WebAuthnAuthentication(userEntity, authorities); - DSUserDetails dsUserDetails = new DSUserDetails(testUser, authorities); + // The handler must read authorities off the incoming authentication, not off the UserDetails it loads. + // Giving the loaded principal a narrower set is what makes the assertion below discriminating: ROLE_ADMIN + // can only have come from the WebAuthnAuthentication. + DSUserDetails dsUserDetails = new DSUserDetails(testUser, Set.of(new SimpleGrantedAuthority("ROLE_USER"))); when(userDetailsService.loadUserByUsername(testUser.getEmail())).thenReturn(dsUserDetails); // When diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnStepUpFactorAssumptionsTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnStepUpFactorAssumptionsTest.java index 641089b..a62c393 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnStepUpFactorAssumptionsTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnStepUpFactorAssumptionsTest.java @@ -1,8 +1,12 @@ package com.digitalsanctuary.spring.user.security; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import java.time.Duration; import java.time.Instant; +import java.util.Collection; import java.util.List; import java.util.Set; import org.junit.jupiter.api.AfterEach; @@ -12,7 +16,6 @@ import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.security.authentication.TestingAuthenticationToken; -import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.authorization.AllRequiredFactorsAuthorizationManager; import org.springframework.security.authorization.AuthorizationManager; import org.springframework.security.authorization.AuthorizationResult; @@ -30,47 +33,74 @@ import org.springframework.security.web.webauthn.api.ImmutablePublicKeyCredentialUserEntity; import org.springframework.security.web.webauthn.api.PublicKeyCredentialUserEntity; import org.springframework.security.web.webauthn.authentication.WebAuthnAuthentication; +import org.springframework.security.web.webauthn.authentication.WebAuthnAuthenticationProvider; +import org.springframework.security.web.webauthn.authentication.WebAuthnAuthenticationRequestToken; +import org.springframework.security.web.webauthn.management.RelyingPartyAuthenticationRequest; +import org.springframework.security.web.webauthn.management.WebAuthnRelyingPartyOperations; import com.digitalsanctuary.spring.user.persistence.model.User; import com.digitalsanctuary.spring.user.service.DSUserDetails; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; /** - * Characterization tests for the Spring Security behaviour the WebAuthn step-up design depends on (issue #335). + * Characterization tests for the behaviour a built-in WebAuthn step-up primitive would depend on (issue #335). * *

- * The built-in step-up primitive is built on Spring Security's own factor machinery rather than on a bespoke - * challenge/verify flow: a sensitive operation requires a {@code FACTOR_WEBAUTHN} - * {@link FactorGrantedAuthority} issued within a short TTL, and the user refreshes it by re-running the ordinary - * passkey assertion at {@code /login/webauthn}. Three framework behaviours have to hold for that to work, and none of - * them are this framework's code, so they are pinned here and will fail loudly on a Spring Security upgrade that - * changes them: + * This framework ships no step-up primitive today: {@link StepUpService} is an SPI a consuming application implements, + * and nothing in production code configures {@code RequiredFactor.validDuration}. The design proposed in #335 would + * build step-up on Spring Security's own factor machinery rather than on a bespoke challenge/verify flow: a sensitive + * operation would require a {@code FACTOR_WEBAUTHN} {@link FactorGrantedAuthority} issued within a short TTL, and the + * user would refresh it by re-running the ordinary passkey assertion at {@code /login/webauthn}. These tests pin the + * behaviour that design rests on, ahead of building it. *

*
    - *
  1. Freshness enforcement — {@code RequiredFactor.validDuration} denies a stale WEBAUTHN factor and - * grants a fresh one.
    2. - *
  2. Refresh — re-running the assertion while already authenticated produces a WEBAUTHN - * {@link FactorGrantedAuthority} with a new {@code issuedAt} while preserving the session's other authorities. This is + *
  3. Freshness enforcement — {@code RequiredFactor.validDuration} denies a stale WEBAUTHN factor, grants + * a fresh one, and refuses a look-alike authority that is not a {@link FactorGrantedAuthority}.
    4. + *
  4. Stamping — {@code WebAuthnAuthenticationProvider} adds a {@code FACTOR_WEBAUTHN} authority whose + * {@code issuedAt} defaults to now, on top of whatever authorities the {@code UserDetailsService} supplies. That + * default is the freshness clock.
    5. + *
  5. Refresh — re-asserting while already authenticated merges the new factor into the existing session + * rather than replacing it, so the fresh {@code issuedAt} wins while the session's other authorities survive. This is * the merging half of {@link AbstractAuthenticationProcessingFilter} that {@code setMfaEnabled(true)} activates; with - * it off, the second authentication REPLACES the first and step-up would drop the user's roles.
    6. - *
  6. Survival — the refreshed factor survives this framework's - * {@link WebAuthnAuthenticationSuccessHandler}, which rebuilds the authentication to swap in {@code DSUserDetails}.
    7. + * it off, the second authentication replaces the first and any authority the {@code UserDetailsService} does not + * re-supply (the {@code FACTOR_PASSWORD} from the original login, say) is lost. *
* *

- * The merging logic lives in {@code AbstractAuthenticationProcessingFilter#doFilter}, shared by every authentication - * filter, and its only type-sensitive step is a reflective {@code declaresToBuilder(authenticationResult)} check. The - * filter here therefore returns a real {@link WebAuthnAuthentication} (which does declare {@code toBuilder()}) and - * stubs only the assertion verification, which needs an authenticator. The real - * {@code WebAuthnAuthenticationProvider} stamps {@code FactorGrantedAuthority.fromAuthority("FACTOR_WEBAUTHN")}, whose - * {@code issuedAt} defaults to now, so the freshness clock is driven by the genuine login path. + * Those three are Spring Security's behaviour, not this framework's, so they will fail loudly on an upgrade that + * changes them. The last test is different in kind: it pins this framework's own + * {@link WebAuthnAuthenticationSuccessHandler} against a regression that would silently drop the refreshed factor + * while swapping {@link DSUserDetails} in as the principal. + *

+ * + *

+ * The only behaviour stubbed is {@link WebAuthnRelyingPartyOperations#authenticate}, which needs a real + * authenticator (its request argument is a stand-in for the same reason, and is never read). + * Everything downstream is the genuine path: the real {@code WebAuthnAuthenticationProvider} assembles the + * authentication and stamps the factor, and the real {@code AbstractAuthenticationProcessingFilter#doFilter} performs + * the merge. Only the credential-JSON converter is skipped, by overriding {@code attemptAuthentication} to hand the + * authentication manager a request token directly. + *

+ * + *

+ * That merge lives in {@code AbstractAuthenticationProcessingFilter}, so it is inherited by the filters that extend it + * (form login, one-time token, WebAuthn) and not by the {@code OncePerRequestFilter}-based ones. It fires only when + * all four of {@code shouldPerformMfa}'s gates pass: {@code mfaEnabled} is set, an authenticated authentication is + * already in the context, the result's concrete class declares {@code toBuilder()} (the check reflects over + * {@code getDeclaredMethods()}, so an inherited one would not count), and {@code current.getName()} equals the new + * result's name. The last gate is why the pre-step-up session below is a {@link WebAuthnAuthenticationToken} over + * {@link DSUserDetails}, matching what a completed passkey login actually leaves in the context: its {@code getName()} + * resolves to the user's email, the same value {@code WebAuthnAuthentication} takes from its + * {@link PublicKeyCredentialUserEntity}. *

*/ @DisplayName("WebAuthn Step-Up Factor Assumptions Tests") class WebAuthnStepUpFactorAssumptionsTest { private static final String EMAIL = "passkey-user@test.com"; + private static final String ROLE_USER = "ROLE_USER"; private static final Duration STEP_UP_TTL = Duration.ofMinutes(5); + private static final Duration LONG_AGO = Duration.ofMinutes(30); private final AuthorizationManager freshWebAuthnRequired = AllRequiredFactorsAuthorizationManager.builder() .requireFactor(RequiredFactor.withAuthority(FactorGrantedAuthority.WEBAUTHN_AUTHORITY).validDuration(STEP_UP_TTL).build()) @@ -84,90 +114,115 @@ void clearContext() { @Test @DisplayName("should deny a stale WEBAUTHN factor and grant a fresh one when a validDuration is required") void shouldEnforceFreshnessWhenValidDurationIsRequired() { - Authentication stale = authWith(webAuthnFactor(Instant.now().minus(Duration.ofMinutes(30)))); + Authentication stale = authWith(webAuthnFactor(Instant.now().minus(LONG_AGO))); Authentication fresh = authWith(webAuthnFactor(Instant.now())); assertThat(granted(freshWebAuthnRequired, stale)).as("30-minute-old WEBAUTHN factor against a 5-minute TTL").isFalse(); assertThat(granted(freshWebAuthnRequired, fresh)).as("just-issued WEBAUTHN factor against a 5-minute TTL").isTrue(); } + @Test + @DisplayName("should deny a plain authority named FACTOR_WEBAUTHN when a validDuration is required") + void shouldDenyWhenWebAuthnAuthorityIsNotAFactorGrantedAuthority() { + // Consumers can name any authority they like in user.roles-and-privileges, so a granted string that happens to + // read FACTOR_WEBAUTHN must not pass a freshness check: it carries no issuedAt, and treating it as valid would + // mean a permanently satisfied step-up gate. AllRequiredFactorsAuthorizationManager type-checks for this. + Authentication lookAlike = authWith(new SimpleGrantedAuthority(FactorGrantedAuthority.WEBAUTHN_AUTHORITY)); + + assertThat(granted(freshWebAuthnRequired, lookAlike)).as("an authority with no issuedAt cannot satisfy a validDuration") + .isFalse(); + } + + @Test + @DisplayName("should stamp a fresh WEBAUTHN factor onto the user's authorities when the provider authenticates") + void shouldStampFreshWebAuthnFactorWhenProviderAuthenticates() { + Instant beforeAssertion = Instant.now(); + + Authentication result = provider(List.of(new SimpleGrantedAuthority(ROLE_USER))).authenticate(assertionRequestToken()); + + assertThat(webAuthnFactorsOf(result)).as("the provider stamps exactly one WEBAUTHN factor").hasSize(1); + assertThat(webAuthnFactorsOf(result).get(0).getIssuedAt()).as("issuedAt defaults to the moment of assertion, driving the clock") + .isBetween(beforeAssertion, Instant.now()); + assertThat(authorityStrings(result)).as("the UserDetailsService authorities are carried across").contains(ROLE_USER); + assertThat(granted(freshWebAuthnRequired, result)).as("a just-stamped factor satisfies the step-up gate").isTrue(); + } + @Test @DisplayName("should refresh issuedAt and keep existing authorities when re-asserting while already authenticated") void shouldRefreshWebAuthnFactorWhenReAssertingWhileAuthenticated() throws Exception { - Instant staleIssuedAt = Instant.now().minus(Duration.ofMinutes(30)); - Instant freshIssuedAt = Instant.now(); + Instant staleIssuedAt = Instant.now().minus(LONG_AGO); - // The session as it stands before step-up: an old passkey login plus the user's role authorities. - SecurityContext context = SecurityContextHolder.createEmptyContext(); - context.setAuthentication(new UsernamePasswordAuthenticationToken(EMAIL, "n/a", - List.of(webAuthnFactor(staleIssuedAt), new SimpleGrantedAuthority("ROLE_USER")))); - SecurityContextHolder.setContext(context); + // The session as it stands before step-up: a passkey login whose factor has gone stale, the FACTOR_PASSWORD + // from the original password login, and the user's role. + Authentication preStepUp = existingSession(webAuthnFactor(staleIssuedAt), passwordFactor(staleIssuedAt), + new SimpleGrantedAuthority(ROLE_USER)); + boolean grantedBeforeStepUp = granted(freshWebAuthnRequired, preStepUp); + setContext(preStepUp); - Authentication merged = runAssertion(new WebAuthnAuthentication(userEntity(), Set.of(webAuthnFactor(freshIssuedAt))), true); + Authentication merged = runAssertion(true, List.of(new SimpleGrantedAuthority(ROLE_USER))); assertThat(webAuthnFactorsOf(merged)).as("exactly one WEBAUTHN factor survives the merge (deduped by authority string)").hasSize(1); - assertThat(webAuthnFactorsOf(merged).get(0).getIssuedAt()).as("the surviving WEBAUTHN factor is the newly issued one") - .isEqualTo(freshIssuedAt); - assertThat(authorityStrings(merged)).as("non-factor authorities from the existing session are carried over") - .contains("ROLE_USER"); + assertThat(webAuthnFactorsOf(merged).get(0).getIssuedAt()).as("the surviving WEBAUTHN factor is the newly stamped one") + .isAfter(staleIssuedAt); + assertThat(authorityStrings(merged)).as("session authorities the UserDetailsService does not re-supply are carried over") + .contains(FactorGrantedAuthority.PASSWORD_AUTHORITY, ROLE_USER); - // The step-up assertion itself: the same gate that denied before the ceremony now grants. - assertThat(granted(freshWebAuthnRequired, context.getAuthentication())).as("gate before step-up").isFalse(); + // The payoff: the gate that denied the pre-step-up session grants the merged one. + assertThat(grantedBeforeStepUp).as("gate before step-up").isFalse(); assertThat(granted(freshWebAuthnRequired, merged)).as("gate after step-up").isTrue(); } @Test - @DisplayName("should replace the session authorities when re-asserting with factor merging disabled") + @DisplayName("should drop session-only authorities when re-asserting with factor merging disabled") void shouldReplaceAuthoritiesWhenMergingIsDisabled() throws Exception { - SecurityContext context = SecurityContextHolder.createEmptyContext(); - context.setAuthentication(new UsernamePasswordAuthenticationToken(EMAIL, "n/a", - List.of(webAuthnFactor(Instant.now().minus(Duration.ofMinutes(30))), new SimpleGrantedAuthority("ROLE_USER")))); - SecurityContextHolder.setContext(context); + Instant staleIssuedAt = Instant.now().minus(LONG_AGO); + setContext(existingSession(webAuthnFactor(staleIssuedAt), passwordFactor(staleIssuedAt), + new SimpleGrantedAuthority(ROLE_USER))); - Authentication result = runAssertion(new WebAuthnAuthentication(userEntity(), Set.of(webAuthnFactor(Instant.now()))), false); + Authentication result = runAssertion(false, List.of(new SimpleGrantedAuthority(ROLE_USER))); - assertThat(authorityStrings(result)).as("ROLE_USER is dropped when mfaEnabled=false, so merging must be switched on for step-up") - .doesNotContain("ROLE_USER"); + assertThat(authorityStrings(result)) + .as("with mfaEnabled=false the result is the new authentication alone, so the original login's FACTOR_PASSWORD is lost") + .containsExactlyInAnyOrder(FactorGrantedAuthority.WEBAUTHN_AUTHORITY, ROLE_USER); } @Test @DisplayName("should preserve the refreshed factor when the success handler converts the principal to DSUserDetails") void shouldPreserveRefreshedFactorWhenPrincipalIsConverted() throws Exception { Instant freshIssuedAt = Instant.now(); - Set merged = Set.of(webAuthnFactor(freshIssuedAt), new SimpleGrantedAuthority("ROLE_USER")); - User user = new User(); - user.setEmail(EMAIL); - user.setFirstName("Passkey"); - user.setLastName("User"); - UserDetailsService userDetailsService = username -> new DSUserDetails(user, merged); + // The handler must copy authorities off the incoming authentication, not off the UserDetails it loads: a + // DB-loaded DSUserDetails carries roles only, so reading authorities from it would drop the factor entirely. + // This stub therefore returns roles only, and the factor exists solely on the authentication passed in. + UserDetailsService rolesOnly = username -> new DSUserDetails(user(), List.of(new SimpleGrantedAuthority(ROLE_USER))); CapturingSuccessHandler captor = new CapturingSuccessHandler(); - WebAuthnAuthenticationSuccessHandler handler = new WebAuthnAuthenticationSuccessHandler(userDetailsService, captor, null); + WebAuthnAuthenticationSuccessHandler handler = new WebAuthnAuthenticationSuccessHandler(rolesOnly, captor, null); - handler.onAuthenticationSuccess(new MockHttpServletRequest(), new MockHttpServletResponse(), - new WebAuthnAuthentication(userEntity(), merged)); + handler.onAuthenticationSuccess(new MockHttpServletRequest(), new MockHttpServletResponse(), new WebAuthnAuthentication( + userEntity(), Set.of(webAuthnFactor(freshIssuedAt), new SimpleGrantedAuthority(ROLE_USER)))); assertThat(captor.captured.getPrincipal()).isInstanceOf(DSUserDetails.class); - assertThat(webAuthnFactorsOf(captor.captured)).hasSize(1); + assertThat(webAuthnFactorsOf(captor.captured)).as("the factor survives the principal swap").hasSize(1); assertThat(webAuthnFactorsOf(captor.captured).get(0).getIssuedAt()).as("issuedAt is preserved through the principal swap") .isEqualTo(freshIssuedAt); assertThat(granted(freshWebAuthnRequired, captor.captured)).as("gate still grants after conversion").isTrue(); } /** - * Drives one authentication-filter pass with a stubbed assertion result, returning the authentication the filter - * hands to its success handler (i.e. after any factor merging). + * Drives one authentication-filter pass through the real {@link WebAuthnAuthenticationProvider}, returning the + * authentication the filter hands to its success handler (i.e. after any factor merging). */ - private Authentication runAssertion(WebAuthnAuthentication result, boolean mergingEnabled) throws Exception { + private Authentication runAssertion(boolean mergingEnabled, Collection userAuthorities) throws Exception { CapturingSuccessHandler captor = new CapturingSuccessHandler(); AbstractAuthenticationProcessingFilter filter = new AbstractAuthenticationProcessingFilter(request -> true) { @Override public Authentication attemptAuthentication(HttpServletRequest request, HttpServletResponse response) { - return result; + // What WebAuthnAuthenticationFilter does once its converter has parsed the credential JSON. + return getAuthenticationManager().authenticate(assertionRequestToken()); } }; - filter.setAuthenticationManager(authentication -> authentication); + filter.setAuthenticationManager(provider(userAuthorities)::authenticate); filter.setAuthenticationSuccessHandler(captor); filter.setMfaEnabled(mergingEnabled); @@ -175,6 +230,33 @@ public Authentication attemptAuthentication(HttpServletRequest request, HttpServ return captor.captured; } + /** The real provider, with only the assertion verification stubbed out, since that needs an authenticator. */ + private static WebAuthnAuthenticationProvider provider(Collection userAuthorities) { + WebAuthnRelyingPartyOperations relyingParty = mock(WebAuthnRelyingPartyOperations.class); + when(relyingParty.authenticate(any())).thenReturn(userEntity()); + return new WebAuthnAuthenticationProvider(relyingParty, username -> new DSUserDetails(user(), userAuthorities)); + } + + /** + * The token the WebAuthn filter's converter would produce. Its payload is never read here: the provider passes + * it straight to the stubbed {@link WebAuthnRelyingPartyOperations#authenticate}, and the real request object + * can only be built from an authenticator's assertion. + */ + private static WebAuthnAuthenticationRequestToken assertionRequestToken() { + return new WebAuthnAuthenticationRequestToken(mock(RelyingPartyAuthenticationRequest.class)); + } + + /** The authentication a completed passkey login leaves in the context. */ + private static Authentication existingSession(GrantedAuthority... authorities) { + return new WebAuthnAuthenticationToken(new DSUserDetails(user(), List.of(authorities)), List.of(authorities)); + } + + private static void setContext(Authentication authentication) { + SecurityContext context = SecurityContextHolder.createEmptyContext(); + context.setAuthentication(authentication); + SecurityContextHolder.setContext(context); + } + private static boolean granted(AuthorizationManager manager, Authentication authentication) { AuthorizationResult result = manager.authorize(() -> authentication, new Object()); return result != null && result.isGranted(); @@ -188,6 +270,18 @@ private static FactorGrantedAuthority webAuthnFactor(Instant issuedAt) { return FactorGrantedAuthority.withAuthority(FactorGrantedAuthority.WEBAUTHN_AUTHORITY).issuedAt(issuedAt).build(); } + private static FactorGrantedAuthority passwordFactor(Instant issuedAt) { + return FactorGrantedAuthority.withAuthority(FactorGrantedAuthority.PASSWORD_AUTHORITY).issuedAt(issuedAt).build(); + } + + private static User user() { + User user = new User(); + user.setEmail(EMAIL); + user.setFirstName("Passkey"); + user.setLastName("User"); + return user; + } + private static List authorityStrings(Authentication authentication) { return authentication.getAuthorities().stream().map(GrantedAuthority::getAuthority).toList(); } From 34e051e7d9be6c6f343fba676225d3e2fdfd2871 Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Wed, 19 Aug 2026 10:48:32 -0600 Subject: [PATCH 3/3] test: pin look-alike factor authority shadowing a genuine one The existing look-alike case shows that a plain SimpleGrantedAuthority named FACTOR_WEBAUTHN cannot satisfy a validDuration requirement on its own. It also shadows a real one, which is the case that actually bites. AllRequiredFactorsAuthorizationManager resolves a RequiredFactor by taking the first authority whose string matches and type-checking only that one, and its getFactorGrantedAuthorities does not filter by type despite the name. WebAuthnAuthenticationProvider appends its stamp after the UserDetailsService's authorities, so a consumer privilege named FACTOR_WEBAUTHN always sorts first: the gate denies immediately after a genuine assertion and the fresh factor behind it is never considered. Fails closed, so not a hole, but step-up would be permanently unsatisfiable on such a deployment with an opaque symptom. Pinned here so the built-in primitive proposed in #335 rejects FACTOR_-prefixed names in user.roles-and-privileges at startup rather than discovering this in the field. No production code changes. ./gradlew test: 1172 tests, 0 failures. Refs #335 --- .../WebAuthnStepUpFactorAssumptionsTest.java | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnStepUpFactorAssumptionsTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnStepUpFactorAssumptionsTest.java index a62c393..69ab2c9 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnStepUpFactorAssumptionsTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnStepUpFactorAssumptionsTest.java @@ -55,7 +55,8 @@ *

*
    *
  1. Freshness enforcement — {@code RequiredFactor.validDuration} denies a stale WEBAUTHN factor, grants - * a fresh one, and refuses a look-alike authority that is not a {@link FactorGrantedAuthority}.
    2. + * a fresh one, and refuses a look-alike authority that is not a {@link FactorGrantedAuthority} — including when + * that look-alike sorts ahead of a genuine one and shadows it. *
  2. Stamping — {@code WebAuthnAuthenticationProvider} adds a {@code FACTOR_WEBAUTHN} authority whose * {@code issuedAt} defaults to now, on top of whatever authorities the {@code UserDetailsService} supplies. That * default is the freshness clock.
    3. @@ -133,6 +134,27 @@ void shouldDenyWhenWebAuthnAuthorityIsNotAFactorGrantedAuthority() { .isFalse(); } + @Test + @DisplayName("should deny a freshly stamped factor when a look-alike authority precedes it") + void shouldDenyWhenALookAlikeAuthorityPrecedesTheStampedFactor() { + // The look-alike does not merely fail on its own, it SHADOWS a genuine one. AllRequiredFactors resolves a + // RequiredFactor by taking the first authority whose string matches, then type-checks that one: a non-factor + // first match is reported expired and the real, fresh FactorGrantedAuthority behind it is never considered. + // The provider appends its stamp after the UserDetailsService's authorities, so a consumer privilege named + // FACTOR_WEBAUTHN always sorts first and step-up can never be satisfied on that deployment. It fails closed, + // but the symptom (a just-completed assertion that still does not satisfy the gate) is opaque, which is why a + // built-in step-up primitive should reject FACTOR_-prefixed names in user.roles-and-privileges at startup. + List withLookAlike = + List.of(new SimpleGrantedAuthority(FactorGrantedAuthority.WEBAUTHN_AUTHORITY), new SimpleGrantedAuthority(ROLE_USER)); + + Authentication result = provider(withLookAlike).authenticate(assertionRequestToken()); + + assertThat(authorityStrings(result)).as("the look-alike is carried through and sorts ahead of the stamped factor") + .startsWith(FactorGrantedAuthority.WEBAUTHN_AUTHORITY); + assertThat(webAuthnFactorsOf(result)).as("a genuine, freshly stamped factor is present all the same").hasSize(1); + assertThat(granted(freshWebAuthnRequired, result)).as("yet the gate denies, immediately after a real assertion").isFalse(); + } + @Test @DisplayName("should stamp a fresh WEBAUTHN factor onto the user's authorities when the provider authenticates") void shouldStampFreshWebAuthnFactorWhenProviderAuthenticates() {