feat: Add MFA challenge UI pages and demo support#60
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds demo-app UI support for Spring Security MFA (password + WebAuthn), including a dedicated WebAuthn challenge page, profile-page MFA status badges, and Playwright coverage to validate the new UI/endpoint wiring.
Changes:
- Added a new
/user/mfa/webauthn-challenge.htmlThymeleaf page plus module JS to perform WebAuthn assertion and redirect. - Extended the profile “Authentication Methods” card to render MFA status badges sourced from
GET /user/mfa/status. - Added MFA configuration defaults and Playwright tests; bumped
ds-spring-user-frameworkto4.2.2-SNAPSHOT.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
build.gradle |
Bumps demo dependency to framework snapshot that includes MFA support. |
src/main/resources/application.yml |
Enables MFA and configures required factors + entry-point URIs. |
src/main/resources/application-playwright-test.yml |
Disables MFA for Playwright profile runs. |
src/main/java/com/digitalsanctuary/spring/demo/controller/PageController.java |
Adds route to render the MFA WebAuthn challenge page. |
src/main/resources/templates/user/mfa/webauthn-challenge.html |
New challenge page template shown when WebAuthn factor is missing. |
src/main/resources/static/js/user/mfa-webauthn-challenge.js |
New challenge page logic that calls authenticateWithPasskey() and redirects. |
src/main/resources/templates/user/update-user.html |
Adds an MFA status container section for JS-populated badges. |
src/main/resources/static/js/user/webauthn-manage.js |
Fetches MFA status and renders badges in the auth-methods card. |
playwright/tests/mfa/mfa-challenge.spec.ts |
New E2E checks for challenge page structure and MFA status endpoint behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+277
to
+280
| if (!response.ok) { | ||
| container.classList.add('d-none'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
updateMfaStatusUI() currently hides the MFA section on any non-OK response, which can mask real server-side errors (500s) and make debugging harder. Consider only hiding on 404 (MFA disabled) and treating other statuses as an error state (log + optional user-visible message).
Comment on lines
+60
to
+68
| // With MFA disabled in playwright-test profile, expect 404 | ||
| // With MFA enabled, expect 200 with proper response shape | ||
| expect([200, 404]).toContain(response.status()); | ||
|
|
||
| if (response.status() === 200) { | ||
| const body = await response.json(); | ||
| expect(typeof body.mfaEnabled).toBe('boolean'); | ||
| expect(typeof body.fullyAuthenticated).toBe('boolean'); | ||
| } |
There was a problem hiding this comment.
This test is non-deterministic for the playwright-test profile: application-playwright-test.yml sets user.mfa.enabled: false, so /user/mfa/status should consistently return 404. Allowing both 200 and 404 weakens the assertion and can hide regressions; consider asserting 404 here and adding a separate test/project/profile where MFA is enabled to assert the 200 + JSON shape case.
Suggested change
| // With MFA disabled in playwright-test profile, expect 404 | |
| // With MFA enabled, expect 200 with proper response shape | |
| expect([200, 404]).toContain(response.status()); | |
| if (response.status() === 200) { | |
| const body = await response.json(); | |
| expect(typeof body.mfaEnabled).toBe('boolean'); | |
| expect(typeof body.fullyAuthenticated).toBe('boolean'); | |
| } | |
| // With MFA disabled in playwright-test profile, this endpoint should return 404 | |
| expect(response.status()).toBe(404); | |
| // NOTE: When adding a profile/project with MFA enabled, create a separate test | |
| // that asserts a 200 response and validates the JSON response shape. | |
| // This keeps the playwright-test profile deterministic. |
Comment on lines
+263
to
+266
| /** | ||
| * Update the MFA Status section in the auth-methods card. | ||
| * Silently hides the container if the MFA status endpoint returns 404 (MFA disabled). | ||
| */ |
There was a problem hiding this comment.
The JSDoc says the MFA status container is hidden only when the endpoint returns 404, but the current implementation hides it for any non-2xx response. Either update the comment to match the behavior, or change the logic to check response.status === 404 specifically and handle other error statuses differently (e.g., log/show a message).
Owner
Author
Code reviewFound 1 issue:
SpringUserFrameworkDemoApp/build.gradle Lines 2 to 4 in 9175efc SpringUserFrameworkDemoApp/build.gradle Lines 100 to 102 in 9175efc 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Add WebAuthn challenge page for MFA second-factor verification, MFA status display on the user profile page, and Playwright tests. Companion to SpringUserFramework#268 / PR #272. - Add user.mfa config block to application.yml (PASSWORD + WEBAUTHN) - Create /user/mfa/webauthn-challenge.html template and JS module - Add controller route in PageController for the challenge page - Extend auth-methods card on profile page with MFA status badges - Add Playwright tests for challenge page structure and MFA status endpoint - Disable MFA in playwright-test profile to keep existing tests unaffected - Bump ds-spring-user-framework to 4.2.2-SNAPSHOT Closes #59
- Check for 404 specifically in updateMfaStatusUI() instead of hiding on any non-OK response; log a warning for other error statuses - Update JSDoc to document both 404 and non-OK handling behaviors - Make MFA status test assertion deterministic (expect 404 when MFA is disabled in playwright-test profile, not [200, 404])
- Simplify test assertion to expect 404 when MFA is disabled in playwright-test profile - Remove unnecessary CSRF header from GET request to /user/mfa/status - Add noscript fallback message on webauthn challenge page
devondragon
force-pushed
the
feature/mfa-challenge-ui
branch
from
9175efc to
6583b42
Compare
… testing - Add registration-guard profile to Configuration Profiles section - Update Testing Strategy to reflect Playwright as the primary E2E framework with Selenide as legacy
Resolves CLAUDE.md conflict (Selenide tests were migrated to Playwright on main) and picks up the TestDataController Date/Instant fix required to compile against framework 4.3.1.
The challenge page (webauthnEntryPointUri) was not in unprotectedURIs, so a partially-authenticated user redirected there was denied access and redirected back to the same page forever (ERR_TOO_MANY_REDIRECTS). The framework only auto-unprotects /user/mfa/status, not the entry point pages. MFA is now disabled by default and enabled via the new 'mfa' profile: with it forced on, users (including all new registrations) who have no passkey cannot satisfy the WEBAUTHN factor and are locked out of every protected page. The mfa profile also unprotects the passkey enrollment endpoints so partially- authenticated users can register their first passkey. The test profile now disables MFA explicitly; with it inherited as enabled, AdminRoleAccessControlTest failed 4 tests (302 instead of 200/403) because MockMvc test users carry no FactorGrantedAuthority. Adds MfaChallengeFlowIntegrationTest (redirect-loop regression guard, status envelope contract) and MfaConfigConsistencyTest (keeps entry point URIs and unprotectedURIs in sync).
The cancel/sign-out locator matched both the navbar logout and the page's cancel button (strict mode violation) — target the accessible name instead, and drop the discouraged networkidle wait. The unauthenticated status-endpoint test expected 404, but with MFA disabled the endpoint is simply protected, so Spring Security 302-redirects to the login page; assert that instead.
Without @EnableMultiFactorAuthentication, a successful WebAuthn assertion REPLACED the session authentication instead of adding to it: the user went from satisfiedFactors=[PASSWORD] to [WEBAUTHN], never both, bouncing between the two factor challenges forever. Spring Security 7 only merges authorities across logins when mfaEnabled is set on the authentication filters, which the annotation's BeanPostProcessor does. The framework's user.mfa support configures the authorization half but not this filter half, so the demo wires it explicitly (conditional on user.mfa.enabled).
/user/mfa/status returns {success, messages, data: {mfaEnabled, ...}} but the
badge renderer read the fields from the top level. Every field came back
undefined, so a fully-authenticated user was always shown the 'Additional
Factor Required' badge and never the MFA Active / Fully Authenticated ones.
Deleting a user with registered passkeys failed on the user_entities foreign key. Publish UserPreDeleteEvent so the framework's WebAuthnPreDeleteEventListener removes the credentials and user entity first, same as the normal deletion flow.
Chromium's CDP WebAuthn virtual authenticator drives the entire flow without hardware: enroll a passkey, password login, get redirected to the challenge page, verify with the passkey, confirm both factors are satisfied and the profile page renders the MFA badges. Runs in a new chromium-mfa Playwright project (tagged @mfa-enabled) against a server started with the mfa profile; the default projects exclude the tag since their specs assume MFA is off. The webServer profiles are now overridable via SPRING_PROFILES.
Until now CI only ran CodeQL and review jobs, so a branch could be green without compiling or passing a single test. Adds a unit/integration test job (H2) and a Playwright job with a MariaDB service container that runs the default chromium project (MFA off) and the chromium-mfa project (MFA on).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+14
to
+36
| name: Unit & Integration Tests | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - uses: actions/setup-java@v4 | ||
| with: | ||
| distribution: temurin | ||
| java-version: 21 | ||
|
|
||
| - uses: gradle/actions/setup-gradle@v4 | ||
|
|
||
| - name: Run tests | ||
| run: ./gradlew test | ||
|
|
||
| - name: Upload test report | ||
| if: failure() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: gradle-test-report | ||
| path: build/reports/tests/test/ | ||
|
|
||
| playwright-tests: |
Comment on lines
+37
to
+99
| name: Playwright E2E Tests | ||
| runs-on: ubuntu-latest | ||
| services: | ||
| mariadb: | ||
| image: mariadb:12.2 | ||
| env: | ||
| MARIADB_DATABASE: springuser | ||
| MARIADB_USER: springuser | ||
| MARIADB_PASSWORD: springuser | ||
| MARIADB_ROOT_PASSWORD: rootpassword | ||
| ports: | ||
| - 3306:3306 | ||
| options: >- | ||
| --health-cmd="healthcheck.sh --connect --innodb_initialized" | ||
| --health-interval=10s | ||
| --health-timeout=5s | ||
| --health-retries=5 | ||
| env: | ||
| # The MariaDB service container replaces Spring Boot's Docker Compose integration | ||
| SPRING_DOCKER_COMPOSE_ENABLED: "false" | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - uses: actions/setup-java@v4 | ||
| with: | ||
| distribution: temurin | ||
| java-version: 21 | ||
|
|
||
| - uses: gradle/actions/setup-gradle@v4 | ||
|
|
||
| - name: Build application | ||
| run: ./gradlew assemble | ||
|
|
||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 20 | ||
| cache: npm | ||
| cache-dependency-path: playwright/package-lock.json | ||
|
|
||
| - name: Install Playwright dependencies | ||
| working-directory: playwright | ||
| run: | | ||
| npm ci | ||
| npx playwright install --with-deps chromium | ||
|
|
||
| - name: Run E2E tests (MFA disabled) | ||
| working-directory: playwright | ||
| env: | ||
| SPRING_PROFILES: playwright-test | ||
| run: npx playwright test --project=chromium | ||
|
|
||
| - name: Run E2E tests (MFA enabled) | ||
| working-directory: playwright | ||
| env: | ||
| SPRING_PROFILES: playwright-test,mfa | ||
| run: npx playwright test --project=chromium-mfa | ||
|
|
||
| - name: Upload Playwright report | ||
| if: failure() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: playwright-report | ||
| path: playwright/reports/ |
Spring Boot's relaxed binding maps the SPRING_PROFILES environment variable to the invalid 'spring.profiles' property and refuses to start. APP_PROFILES is only read by the Playwright webServer command.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Companion to SpringUserFramework#268 / PR #272. Closes #59.
/user/mfa/webauthn-challenge.html) — shown when a partially-authenticated user needs to verify with their passkey after password loginGET /user/mfa/statusmfaprofile — MFA is opt-in via the newmfaprofile (e.g.--spring.profiles.active=local,mfa). It is off by default because a user without a registered passkey cannot satisfy theWEBAUTHNfactor and would be locked out of every protected page.MfaSecurityConfigapplies@EnableMultiFactorAuthenticationso a passkey verification adds theWEBAUTHNfactor to the session instead of replacing thePASSWORDfactor (without it the MFA flow can never complete)tests.ymlworkflow runs the gradle suite and both Playwright projects on PRsHow MFA works
PASSWORDfactor authorityWEBAUTHNfactor is missing → redirects to the challenge page (which must be inunprotectedURIs, or the redirect loops)@EnableMultiFactorAuthentication) → both factors satisfied → fully authenticatedBugs found and fixed during review
unprotectedURIs, so partially-authenticated users were redirected to a page they were not allowed to load, foreverPASSWORDfactor (satisfiedFactorswent[PASSWORD]→[WEBAUTHN]), so MFA could never completestatus.mfaEnabledetc. from the top level, but the endpoint wraps the payload inJSONResponse($.data.mfaEnabled) — fully-authenticated users always saw "Additional Factor Required"user.mfa.enabled: trueleaked into thetestprofile and broke 4AdminRoleAccessControlTesttests (302 instead of 200/403)user_entitiesFK; the Test API now publishesUserPreDeleteEventso the framework cleans up WebAuthn data firstTest plan
./gradlew test— 315 tests, 0 failures (includes newMfaChallengeFlowIntegrationTest+MfaConfigConsistencyTest)npx playwright test --project=chromium— 103 tests pass (MFA-off server)SPRING_PROFILES=local,playwright-test,mfa npx playwright test --project=chromium-mfa— full passkey MFA flow passes using the CDP virtual authenticator/user/mfa/statusreportssatisfiedFactors=[PASSWORD],missingFactors=[WEBAUTHN]Follow-up
The factor-merging gap (
@EnableMultiFactorAuthenticationfilter wiring) arguably belongs in the framework'sMfaConfigurationrather than in each consuming app — filed as SpringUserFramework#313.