test: pin Spring Security factor-freshness behavior for WebAuthn step-up

[devondragon]

Groundwork for #335 (built-in WebAuthn step-up primitive). No production code changes.

Why

A spike asked whether Spring Security 7.1 can serve as the step-up mechanism, instead of the custom challenge/verify endpoints and session marker the original design specified. It can, and the answer rests entirely on framework behavior this project does not own:

• FactorGrantedAuthority carries issuedAt, defaulting to now, and WebAuthnAuthenticationProvider stamps FACTOR_WEBAUTHN on every assertion.
• RequiredFactor.validDuration + AllRequiredFactorsAuthorizationManager deny an expired factor.
• AbstractAuthenticationProcessingFilter with mfaEnabled=true merges a re-assertion into the existing session, deduping by authority string in favor of the new authorities, so re-running /login/webauthn while logged in refreshes the factor clock and keeps the user's roles.

That makes step-up "re-run the ordinary passkey ceremony", with no new endpoints, no challenge store, and no change to the StepUpService SPI shipped in #334.

What this adds

WebAuthnStepUpFactorAssumptionsTest pins that behavior so a Spring Security upgrade that changes it fails here rather than quietly weakening step-up:

1. validDuration(5m) denies a 30-minute-old WEBAUTHN factor and grants a just-issued one.
2. A plain SimpleGrantedAuthority named FACTOR_WEBAUTHN does not satisfy a validDuration requirement: it has no issuedAt.
3. That look-alike also shadows a genuine factor. AllRequiredFactors takes the first authority matching by string and type-checks only that one, and the provider appends its stamp after the UserDetailsService's authorities, so a consumer privilege named FACTOR_WEBAUTHN sorts first and the gate denies right after a real assertion. Fails closed, but step-up would be permanently unsatisfiable on that deployment, so the primitive in Built-in WebAuthn step-up primitive (default StepUpService) for credential-altering operations #335 should reject FACTOR_-prefixed names in user.roles-and-privileges at startup.
4. WebAuthnAuthenticationProvider stamps exactly one FACTOR_WEBAUTHN whose issuedAt defaults to the moment of assertion, on top of the UserDetailsService's authorities.
5. Re-asserting while authenticated merges rather than replaces: the fresh issuedAt wins, session-only authorities such as the original login's FACTOR_PASSWORD survive, and the gate flips from denied to granted.
6. Control: with merging disabled, the result is the new authentication alone and FACTOR_PASSWORD is lost.
7. Local regression guard, different in kind from the rest: WebAuthnAuthenticationSuccessHandler must copy authorities off the incoming authentication rather than the UserDetails it loads, or the refreshed factor disappears during the DSUserDetails principal swap.

The only stub is WebAuthnRelyingPartyOperations.authenticate, which needs a real authenticator; the provider, the filter, and the merge are the genuine path. MfaLoginIntegrationTest already covers that the merging post-processor reaches the DSL-built filters.

WebAuthnAuthenticationSuccessHandlerTest.shouldPreserveAuthorities is tightened for the same reason as item 7: it previously handed the same authority set to both the stub and the incoming authentication, so it could not tell which one the handler read. Both revised tests were mutation-checked, and pointing the handler at userDetails.getAuthorities() fails exactly those two.

Verification

./gradlew test: 1172 tests, 0 failures.

[Copilot]

Pull request overview

Adds a new characterization/assumptions test that “pins” specific Spring Security 7.1 factor freshness + MFA authority-merging behaviors that the planned built-in WebAuthn step-up design (issue #335) intends to rely on, so upgrades fail loudly if these semantics change.

Changes:

• Introduces WebAuthnStepUpFactorAssumptionsTest to assert RequiredFactor.validDuration(...) freshness enforcement for FACTOR_WEBAUTHN.
• Verifies AbstractAuthenticationProcessingFilter#setMfaEnabled(true)-driven authority merging refreshes the WEBAUTHN factor while retaining non-factor authorities (and demonstrates the control case when disabled).
• Verifies the project’s WebAuthnAuthenticationSuccessHandler principal conversion preserves the refreshed factor’s issuedAt.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[devondragon]

Confirmed, and fixed in 060e7c0 — though by changing the test rather than the comment.

You were right that the test bypassed the provider entirely: attemptAuthentication was overridden to return a prebuilt WebAuthnAuthentication, with an identity AuthenticationManager that was never invoked. So the claim that the freshness clock is "driven by the genuine login path" was false of the test — every issuedAt was hand-supplied. A Spring Security release that stopped stamping FACTOR_WEBAUTHN, or stamped it with a non-now instant, would have left all four tests green while step-up silently stopped working.

Rather than weaken the comment, runAssertion now drives the real WebAuthnAuthenticationProvider through the filter's AuthenticationManager, so the stamping and its issuedAt default are genuinely exercised. The only stub left is WebAuthnRelyingPartyOperations.authenticate, which needs a real authenticator. That also retires the dead setAuthenticationManager line, since the manager is now on the path. A new test pins the stamping directly.

The surrounding JavaDoc was corrected on three further points while I was in there: step-up is proposed in #335 rather than shipped (StepUpService is a consumer SPI, and nothing in src/main sets validDuration); the merge is inherited only by AbstractAuthenticationProcessingFilter subclasses, not every authentication filter; and all four shouldPerformMfa gates are named, since the name-equality one was load-bearing and undocumented.

[devondragon]

Checked this against the Spring Security 7.1.0 source, and the assertion was safe as written — AbstractAuthenticationProcessingFilter.successfulAuthentication does:

SecurityContext context = this.securityContextHolderStrategy.createEmptyContext();
context.setAuthentication(authResult);
this.securityContextHolderStrategy.setContext(context);

It allocates a fresh SecurityContext and swaps it into the holder rather than mutating the one the test installed, and the MFA merge itself only reads getContext().getAuthentication(). So the local variable still held the pre-step-up token and the isFalse() was a real expired-factor denial.

Worth noting the failure mode would have been loud rather than silent: had the framework switched to mutating the existing context, the assertion would fail, which is the intended behavior for a characterization test.

That said, the readability point stands — "gate before step-up" reads as a precondition but executed after the ceremony. In 060e7c0 it is captured into a grantedBeforeStepUp local before the assertion runs, so execution order now matches how it reads.

[claude]

Review

This PR adds a single test-only file (WebAuthnStepUpFactorAssumptionsTest, 214 lines) with no production code changes — groundwork for #335, pinning Spring Security 7.1 framework behavior the future WebAuthn step-up feature will depend on. Overall this is solid, well-scoped work.

Strengths

• Clear intent, well-documented. The class-level Javadoc explicitly states these are characterization tests for framework behavior, not this library's code, and explains exactly which Spring Security internals (FactorGrantedAuthority, RequiredFactor.validDuration, AbstractAuthenticationProcessingFilter#setMfaEnabled) are being pinned and why. This is a good practice for a library depending on undocumented-but-load-bearing framework behavior — it converts a silent future breakage (Spring Security upgrade weakens step-up) into a loud, explainable test failure.
• Minimal, targeted stubbing. Only assertion verification is stubbed (attemptAuthentication override); the rest of the flow (AbstractAuthenticationProcessingFilter#doFilter, the real WebAuthnAuthentication/FactorGrantedAuthority types) runs unmodified, so the test exercises the real merge/dedup logic rather than a mock of it.
• Correct handling of SecurityContextHolder state. Given this repo runs JUnit 5 with parallel.mode.default=concurrent (junit-platform.properties), the @AfterEach clearContext() is the right guard against ThreadLocal leakage onto reused pool threads across tests.
• Test 4 (shouldPreserveRefreshedFactorWhenPrincipalIsConverted) is the one case that does exercise this library's own code (WebAuthnAuthenticationSuccessHandler), tying the framework assumptions back to how they're actually consumed — good coverage of the seam that matters most for Built-in WebAuthn step-up primitive (default StepUpService) for credential-altering operations #335.
• I traced through the local context variable vs. what AbstractAuthenticationProcessingFilter#successfulAuthentication does (it builds a new SecurityContext via createEmptyContext() rather than mutating the existing one) — the "gate before step-up" assertion checking context.getAuthentication() after runAssertion() has already run is correct, not a stale-read bug, since the filter doesn't mutate the original context object. Worth a comment near that assertion for the next reader who has the same "wait, wasn't this already mutated?" reaction I did.

Minor / non-blocking suggestions

• Since this test intentionally couples to Spring Security's internal declaresToBuilder/merge reflection behavior (per the Javadoc), it may be worth a one-line comment noting the Spring Security version this was verified against, so a future failure is easier to triage against release notes.
• Consider a short note (in the class Javadoc or a @Tag) flagging this as a "framework assumption / characterization" suite distinct from ordinary business-logic tests, so it's easy to identify and re-verify specifically when bumping the spring-security-webauthn / Spring Boot version in build.gradle.
• Formatting/imports/tab indentation are consistent with sibling files in this package (WebAuthnAuthenticationSuccessHandlerTest, MfaConfigurationTest), so no style concerns there despite CLAUDE.md nominally calling for 4-space indentation.

Bugs / security / performance

None found. No production code is touched, no new endpoints or data handling, and the test has no external dependencies (no DB, no network, no Spring context), so it's cheap to run and shouldn't affect suite performance meaningfully.

Nice groundwork — the four pinned assumptions map cleanly to the four points in the PR description, and this should make the actual #335 implementation much lower-risk.

[claude]

Review

This is a test-only PR (no production code changes), and it's unusually well self-audited already — the second commit explicitly fixes two non-discriminating assertions and a JavaDoc that overstated coverage, found in the author's own review pass. I read the new WebAuthnStepUpFactorAssumptionsTest and the tightened WebAuthnAuthenticationSuccessHandlerTest.shouldPreserveAuthorities in detail, plus the production WebAuthnAuthenticationSuccessHandler/WebAuthnAuthenticationToken classes they depend on.

Verified as correct:

• WebAuthnAuthenticationToken is this project's own class (in the same package, so no import needed) — not a Spring Security type, which I initially double-checked given the similar naming to WebAuthnAuthentication.
• The name-equality claim underpinning shouldPerformMfa's merge gate checks out: DSUserDetails.getUsername() returns user.getEmail(), and the test's userEntity() fixture sets the same email as its PublicKeyCredentialUserEntity name — so existingSession(...)'s WebAuthnAuthenticationToken and the freshly-produced WebAuthnAuthentication do resolve to the same getName(), which is exactly what the class JavaDoc claims is required for the merge to fire.
• WebAuthnAuthenticationSuccessHandler.onAuthenticationSuccess reads authorities off the incoming authentication, not off the UserDetails it loads (line 118-119) — confirming the fix to shouldPreserveAuthorities/shouldPreserveRefreshedFactorWhenPrincipalIsConverted (giving the stub UserDetailsService a narrower authority set than the incoming authentication) is what makes those tests actually discriminating, per the stated mutation-testing rationale.
• DSUserDetails constructor overloads used (User, Collection<? extends GrantedAuthority>) exist and match call sites.
• Imports in the modified WebAuthnAuthenticationSuccessHandlerTest already include Set and SimpleGrantedAuthority, so the diff there is a clean drop-in.

Minor observations, nothing blocking:

• The class JavaDoc is thorough (arguably more detailed than most test classes in this repo), including a good explanation of why only WebAuthnRelyingPartyOperations#authenticate is stubbed and everything else runs the real framework path — this raises confidence that a future Spring Security upgrade breaking these assumptions will fail loudly and legibly rather than needing someone to re-derive the reasoning.
• shouldDenyWhenWebAuthnAuthorityIsNotAFactorGrantedAuthority is a nice addition — pinning that a consumer-defined SimpleGrantedAuthority named FACTOR_WEBAUTHN (e.g. via user.roles-and-privileges) can't accidentally satisfy a freshness gate is a subtle and easy-to-miss edge case for the future step-up primitive (Built-in WebAuthn step-up primitive (default StepUpService) for credential-altering operations #335) to get right.
• Test naming (should[Behavior]When[Condition]) and AssertJ-style assertions with descriptive .as(...) messages match the conventions in CLAUDE.md and docs/TESTING.md.
• I wasn't able to execute ./gradlew test myself in this sandboxed review to independently re-confirm the "275 suites, 1171 tests, 0 failures" figure in the PR description, but the code reads as internally consistent and the imports/constructors all check out against current main.

No bugs, security concerns, or missing coverage found. This is solid groundwork for #335 — approving from a code-review standpoint.

[claude]

Review

This is a test-only PR (confirmed: no src/main changes), so the usual production-code concerns (transactions, security config, DI) don't apply. Reviewed the diff against WebAuthnAuthenticationSuccessHandler/WebAuthnAuthenticationToken in src/main to sanity-check the assumptions the new tests encode.

Strengths

• The shadowing scenario (shouldDenyWhenALookAlikeAuthorityPrecedesTheStampedFactor) is a genuinely useful finding. A consumer naming a privilege FACTOR_WEBAUTHN in user.roles-and-privileges would permanently and silently break step-up (fails closed, but with an opaque symptom) once Built-in WebAuthn step-up primitive (default StepUpService) for credential-altering operations #335 lands. Worth making sure that startup-time rejection of FACTOR_-prefixed names actually gets implemented in Built-in WebAuthn step-up primitive (default StepUpService) for credential-altering operations #335 -- this PR only documents the need for it.
• shouldPreserveAuthorities/shouldPreserveRefreshedFactorWhenPrincipalIsConverted are now actually discriminating. Giving the stubbed UserDetailsService a narrower authority set than the incoming authentication is the right fix -- the previous version of shouldPreserveAuthorities gave both the same set, so it would have passed even if the handler were changed to read userDetails.getAuthorities(). Confirmed in WebAuthnAuthenticationSuccessHandler.java lines 118-119 that production code already does the correct thing (reads off authentication, not userDetails), so these are pure regression guards, not currently-failing tests.
• Good use of a control test (shouldReplaceAuthoritiesWhenMergingIsDisabled) -- showing the negative case (authority loss with mfaEnabled=false) is what makes the merge-enabled test's claim credible rather than coincidental.
• Test naming follows the should[Behavior]When[Condition] convention from CLAUDE.md, and AssertJ usage with descriptive .as(...) messages is good practice for a suite whose entire point is 'when this fails, know why.'

Concerns worth discussing

1. Fragility / maintenance cost. This suite pins undocumented internals of Spring Security's WebAuthnAuthenticationProvider and AllRequiredFactorsAuthorizationManager -- authority ordering (provider appends its stamp after the UserDetailsService's authorities), dedup-by-string-in-favor-of-new merge semantics, and even a reflection detail (shouldPerformMfa's getDeclaredMethods() check, per the class Javadoc). None of that is public API contract. A future Spring Security patch release could reorder or refactor any of this without changing observable behavior in any way that matters to consumers, and this suite would go red. That's the explicit intent (fail loudly on an upgrade that changes it), but it does mean whoever triages a future failure here needs to read this class's Javadoc carefully to distinguish a real regression from a harmless internal reshuffle. Worth a one-line note in the PR/issue tracker (or CONTRIBUTING) that CI failures in this specific file should be triaged against Spring Security's release notes before assuming a step-up security hole.
2. Lifecycle of this file. Since this only characterizes assumptions for a not-yet-built feature (Built-in WebAuthn step-up primitive (default StepUpService) for credential-altering operations #335), consider linking a TODO or tracking item to revisit/retire this class once Built-in WebAuthn step-up primitive (default StepUpService) for credential-altering operations #335's real implementation and its own tests land, so the codebase doesn't end up carrying both the characterization suite and the eventual feature tests indefinitely.
3. Verification note only mentions ./gradlew test. Given the PR touches nothing in src/main, ./gradlew check is probably a formality, but worth confirming it was also run (e.g. any static analysis/license header checks) before merge.

Correctness spot-check

Traced through the authority ordering/dedup logic by hand for shouldRefreshWebAuthnFactorWhenReAssertingWhileAuthenticated and shouldDenyWhenALookAlikeAuthorityPrecedesTheStampedFactor -- both are internally consistent with the stated Spring Security behavior and with DSUserDetails's/WebAuthnAuthenticationToken's actual constructors in src/main. I wasn't able to execute ./gradlew test in this environment to independently confirm the suite is green, so I'd lean on the PR's stated '1172 tests, 0 failures' / mutation-testing verification rather than re-deriving it.

No security, performance, or test-coverage concerns beyond the maintenance-cost note above -- this is solid, well-documented groundwork for #335.