Upgrade to SpringUserFramework 4.4.0#72
Merged
Merged
Conversation
Point the demo at the released 4.4.0 security batch on Maven Central (was 4.3.1) and adapt the application code/config to its API and behavior changes. Dependency: - ds-spring-user-framework 4.3.1 -> 4.4.0. - Drop the direct org.passay:passay pin. passay is supplied transitively by the framework at the version it requires; pinning it here forced a conflicting downgrade. Application adaptations to 4.4.0: - CustomUserEmailService: UserEmailService's constructor now takes a TokenHasher (token-hashing security fix); pass it through. - Remove MfaSecurityConfig: 4.4.0 installs @EnableMultiFactorAuthentication / the MFA filter-merging post-processor itself, so the demo no longer needs its own copy. - application.yml / application-mfa.yml: the framework now auto-unprotects the configured MFA factor entry-point URIs (#313), so the WebAuthn challenge page no longer has to be listed in unprotectedURIs by hand. - MfaConfigConsistencyTest: assert only what is still the demo's own responsibility (MFA opt-in, passkey enrollment endpoints reachable). - EmailVerificationEdgeCaseSimpleTest: verification tokens are now atomically consumed on the valid path (single-use), so assert the token is gone and the user enabled after validation. Drop framework-internal unit tests that duplicated the library's own suite and no longer match its internals: - UserServiceTest, UserVerificationServiceTest, LoginAttemptServiceTest.
The integration tests under com.digitalsanctuary.spring.user.* were moved
from the library into this demo. 4.4.0's security hardening changed four
behaviors they assert against; this updates them to the new, correct
behavior (full suite green again: 301 tests, 0 failures).
Test isolation (application-test.properties):
- Switch the H2 URL to jdbc:h2:mem:testdb-${random.uuid} and ddl-auto to
create-drop so each Spring context gets its own database. 4.4.0 made
registerNewUserAccount run with Propagation.NOT_SUPPORTED and the
LoginAttemptService mutations @transactional, so they COMMIT in their own
transactions and survive a test's @transactional rollback. With the old
shared db name those committed rows leaked across classes and collided
with other classes' deleteAll()/registration (UserAlreadyExistException,
FK_VERIFY_USER violations). Mirrors the library's own test-isolation fix.
Behavior changes adapted:
- Account status enforced on load (LoginHelperService.assertAccountUsable):
DSUserDetailsService now throws LockedException/DisabledException for
locked/disabled accounts. DSUserDetailsServiceIntegrationTest now asserts
those exceptions, and pins accountLockoutDuration so the auto-unlock case
is deterministic.
- registerNewUserAccount commits outside the test transaction:
AccountLockoutIntegrationTest seeds its user via the repository (in-tx,
rolls back) instead of the registration service; UserApiIntegrationTestFixed
cleans up the committed user in a REQUIRES_NEW transaction before/after
each test so registrations don't collide.
- Role names are now granted as authorities (for hasRole() checks):
AuthorityServiceIntegrationTest expects the ROLE_* names alongside the
privileges.
- RegistrationListener skips the verification email for already-enabled
users: EventSystemIntegrationTest builds its user as unverified so the
registration-email events still fire.
Contributor
There was a problem hiding this comment.
Pull request overview
Upgrades the demo application to ds-spring-user-framework 4.4.0 and updates the demo’s application wiring, security configuration, and integration tests to match the framework’s changed security behavior (MFA entry-point handling, account usability enforcement, and transactional semantics).
Changes:
- Bump framework dependency to 4.4.0 and remove the direct
passaypin. - Adapt application configuration for MFA entry-point auto-unprotect behavior and update
CustomUserEmailServiceto pass the newTokenHasherdependency. - Adjust/relocate integration tests to assert the updated account status, authority, token-consumption, and transaction/cleanup behavior; remove no-longer-relevant framework-internal unit tests.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/resources/application-test.properties | Update unprotected URIs and introduce per-context H2 DB isolation + create-drop schema for test isolation. |
| build.gradle | Upgrade ds-spring-user-framework to 4.4.0; remove direct passay dependency pin. |
| src/main/java/com/digitalsanctuary/spring/demo/service/CustomUserEmailService.java | Pass TokenHasher into the framework UserEmailService superclass constructor. |
| src/main/java/com/digitalsanctuary/spring/demo/config/MfaSecurityConfig.java | Remove redundant MFA filter-merging configuration now provided by the framework. |
| src/main/resources/application.yml | Remove WebAuthn challenge page from unprotectedURIs per framework auto-unprotect behavior. |
| src/main/resources/application-mfa.yml | Update MFA profile notes and unprotectedURIs to rely on framework auto-unprotect for entry points while keeping enrollment endpoints open. |
| src/test/java/com/digitalsanctuary/spring/demo/mfa/MfaConfigConsistencyTest.java | Update assertions to reflect new responsibility boundaries (entry-point auto-unprotect vs enrollment endpoint accessibility). |
| src/test/java/com/digitalsanctuary/spring/user/security/EmailVerificationEdgeCaseSimpleTest.java | Update assertions for single-use verification token consumption and correct-user enablement. |
| src/test/java/com/digitalsanctuary/spring/user/security/AccountLockoutIntegrationTest.java | Change user seeding to avoid framework NOT_SUPPORTED registration commits leaking across tests. |
| src/test/java/com/digitalsanctuary/spring/user/integration/EventSystemIntegrationTest.java | Ensure test user is unverified so registration-email events fire under 4.4.0 behavior. |
| src/test/java/com/digitalsanctuary/spring/user/integration/DSUserDetailsServiceIntegrationTest.java | Assert new locked/disabled enforcement behavior (exceptions) and deterministic auto-unlock via accountLockoutDuration. |
| src/test/java/com/digitalsanctuary/spring/user/integration/AuthorityServiceIntegrationTest.java | Update expectations to include ROLE_* authorities alongside privileges. |
| src/test/java/com/digitalsanctuary/spring/user/api/UserApiIntegrationTestFixed.java | Add committed cleanup to prevent user/token leakage across test methods due to framework transactional changes. |
| src/test/java/com/digitalsanctuary/spring/user/service/UserVerificationServiceTest.java | Remove framework-internal unit test no longer appropriate for the demo repo. |
| src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java | Remove framework-internal unit test no longer appropriate for the demo repo. |
| src/test/java/com/digitalsanctuary/spring/user/service/LoginAttemptServiceTest.java | Remove framework-internal unit test no longer appropriate for the demo repo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
6
to
7
| # The MFA challenge page must stay unprotected: MFA tests redirect partially-authenticated users to | ||
| # it, and a protected challenge page would redirect back to itself forever. |
Comment on lines
+83
to
+87
| // Create the test user directly via the repository so it lives inside the test's transaction and | ||
| // rolls back cleanly. As of 4.4.0, UserService.registerNewUserAccount runs with | ||
| // Propagation.NOT_SUPPORTED and commits the new user in its own transaction, which would survive | ||
| // the rollback and leak the same email into the next test method (UserAlreadyExistException). The | ||
| // lockout tests only need a persisted, enabled user row; they exercise LoginAttemptService directly. |
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
Upgrades the demo to the released ds-spring-user-framework 4.4.0 (the 4.4.0 security batch on Maven Central, up from 4.3.1) and adapts the demo's application code, configuration, and tests to the framework's API and behavior changes.
Full Java test suite is green against 4.4.0: 301 tests, 0 failures, 0 errors (verified locally with
./gradlew test). For reference, cleanmainon 4.3.1 was 315 tests/0 failures; the difference is the three framework-internal unit tests removed here.Dependency
ds-spring-user-framework4.3.1 → 4.4.0.org.passay:passaypin — it's provided transitively by the framework at the version it requires, and pinning it here forced a conflicting downgrade.Application adaptations
CustomUserEmailService— the framework'sUserEmailServiceconstructor now takes aTokenHasher(token-hashing security fix); passed through.MfaSecurityConfig— 4.4.0 installs@EnableMultiFactorAuthentication/ the MFA filter-merging post-processor itself, so the demo's copy is redundant.application.yml/application-mfa.yml— the framework now auto-unprotects the configured MFA factor entry-point URIs (#313), so the WebAuthn challenge page no longer needs to be listed inunprotectedURIsby hand.Test adaptations
The integration tests under
com.digitalsanctuary.spring.user.*were relocated from the library into this demo. 4.4.0's security hardening changed four behaviors they assert against:assertAccountUsableenforces account status on loadDSUserDetailsServicethrowsLockedException/DisabledExceptionfor locked/disabled accountsDSUserDetailsServiceIntegrationTestasserts the exceptions; pinsaccountLockoutDurationfor a deterministic auto-unlock caseregisterNewUserAccountrunsNOT_SUPPORTEDand commits outside the caller's tx@Transactionalrollback and leak across tests (UserAlreadyExistException,FK_VERIFY_USER)AccountLockoutIntegrationTestseeds via the repository (in-tx);UserApiIntegrationTestFixedcleans up in aREQUIRES_NEWtx; per-context H2 DB isolation inapplication-test.propertieshasRole())ROLE_*AuthorityServiceIntegrationTestexpects the role names alongside privilegesRegistrationListenerskips the verification email for already-enabled usersEventSystemIntegrationTestbuilds its user as unverifiedThe H2 isolation change (
jdbc:h2:mem:testdb-${random.uuid}+ddl-auto=create-drop) mirrors the library's own test-isolation fix and stops committed rows from one context polluting another.Also removes three framework-internal unit tests that duplicated the library's own suite and no longer matched its internals:
UserServiceTest,UserVerificationServiceTest,LoginAttemptServiceTest.Test plan
./gradlew test— 301 passed, 0 failedworkers=1as in CI):--project=chromium): 103 passed, 0 failed--project=chromium-mfa,playwright-test,mfa): 1 passed, 0 failedshould reset password with valid tokenspec flakes under local parallel execution but passes reliably serially (5/5); CI runsworkers=1+retries=2.