From 5ad55b38235ca5d520da420fef5d4cb0fdf3fae7 Mon Sep 17 00:00:00 2001
From: Devon Hillard
Date: Mon, 15 Jun 2026 09:22:15 -0600
Subject: [PATCH 01/23] =?UTF-8?q?chore(v5):=20start=205.0.0=20line=20?=
=?UTF-8?q?=E2=80=94=20set=205.0.0-SNAPSHOT,=20seed=20CHANGELOG/MIGRATION?=
=?UTF-8?q?=20sections?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Begin the Tier 2 / 5.0.0 breaking-change work off the 4.4.0 tag.
Seeds the [5.0.0] CHANGELOG section and a 'Migrating to 5.0.x' guide
with the prominent reverse-proxy ACTION REQUIRED notice (Task 1.3).
Notes that audit-log JSON-per-line (Task 4.3) is intentionally dropped
(injection already fixed via sanitization in 4.4.0).
---
CHANGELOG.md | 15 +++++++++++++++
MIGRATION.md | 27 +++++++++++++++++++++++++++
gradle.properties | 2 +-
3 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 287019a4..9049be1b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,3 +1,18 @@
+## [5.0.0] - Unreleased
+
+> **Major release.** Contains breaking changes (Java API, HTTP/response contracts, database schema, required configuration, and bean/auto-configuration structure). Read `MIGRATION.md` ("Migrating to 5.0.x") before upgrading.
+>
+> **⚠️ ACTION REQUIRED if you run behind a reverse proxy:** password-reset and email-verification links are now built from a configured canonical URL and ignore `X-Forwarded-Host` unless it is explicitly allow-listed. Set `user.security.appUrl` (recommended) or `user.security.trustedHosts`, or your reset/verification links will point at the wrong host. See `MIGRATION.md`.
+
+### Features
+
+### Fixes
+
+### Breaking Changes
+
+### Notes
+- Audit-log injection (originally slated here as a JSON-per-line format change) was already resolved in 4.4.0 via field sanitization (CR/LF and `|` stripped). The breaking JSON-per-line conversion was intentionally **not** carried into 5.0.0, as it offered no additional security benefit.
+
## [4.4.0] - 2026-06-15
### Features
- Token security hardened: verification and password-reset tokens are now hashed at rest (HMAC-SHA-256 when user.security.tokenHashSecret is set; plain SHA-256 otherwise). Dual-read ensures pre-upgrade plaintext tokens still work until expiry. Only one active token per user is kept; generators delete any previous token before issuing a new one. New properties:
diff --git a/MIGRATION.md b/MIGRATION.md
index 2d39dec0..8a8afccb 100644
--- a/MIGRATION.md
+++ b/MIGRATION.md
@@ -29,6 +29,33 @@ This guide covers migrating applications using the Spring User Framework between
- [Common Issues](#common-issues)
- [Version Compatibility Matrix](#version-compatibility-matrix)
+## Migrating to 5.0.x
+
+This section covers migrating from Spring User Framework 4.4.x to 5.0.x. Version 5.0.0 is a **major release** containing breaking changes. Spring Boot compatibility is unchanged from 4.4.x (Spring Boot 4.0 on Java 21+, and Spring Boot 3.5 on Java 17+); the major-version bump reflects this library's own API/contract changes, not a Spring Boot major change.
+
+> **Note on versioning:** This library follows Semantic Versioning for its own API. Its major version is deliberately **not** tied to Spring Boot's major version — a single release line supports more than one Spring Boot major. See the Version Compatibility Matrix for supported Spring Boot versions.
+
+### ⚠️ ACTION REQUIRED: Reverse-proxy deployments must configure a canonical app URL
+
+Password-reset and email-verification links are now built from a configured canonical base URL instead of the inbound request's `Host` / `X-Forwarded-Host` header (CWE-640, host-header / reset poisoning). By default `X-Forwarded-Host` is **ignored** unless the host is explicitly allow-listed.
+
+**If your application runs behind a reverse proxy or load balancer, you must take action or your reset/verification links will break:**
+
+- **Recommended:** set the canonical base URL:
+ ```properties
+ user.security.appUrl=https://app.example.com
+ ```
+ When set, `X-Forwarded-Host` is ignored entirely and all email links use this URL.
+- **Alternative:** allow-list the trusted forwarded host(s):
+ ```properties
+ user.security.trustedHosts=app.example.com,www.example.com
+ ```
+ `X-Forwarded-Host` is then honored only for hosts in this list; all others fall back to the container's own server name.
+
+Local development with no proxy needs no change. `UserUtils.getAppUrl(HttpServletRequest)` is deprecated in favor of `AppUrlResolver`.
+
+
+
## Migrating to 4.0.x (Spring Boot 4.0)
This section covers migrating from Spring User Framework 3.x (Spring Boot 3.x) to 4.x (Spring Boot 4.0).
diff --git a/gradle.properties b/gradle.properties
index 44573430..651d7d70 100644
--- a/gradle.properties
+++ b/gradle.properties
@@ -1,3 +1,3 @@
-version=4.4.0
+version=5.0.0-SNAPSHOT
mavenCentralPublishing=true
mavenCentralAutomaticPublishing=true
From a89e63e13397dd35e4e69a9e8e1bceefcca1d48a Mon Sep 17 00:00:00 2001
From: Devon Hillard
Date: Mon, 15 Jun 2026 09:29:48 -0600
Subject: [PATCH 02/23] fix(security): resolve email link base URL from
configured appUrl/allowlist to prevent reset poisoning (H2, CWE-640)
---
CHANGELOG.md | 2 +
.../spring/user/api/UserAPI.java | 8 +-
.../UserSecurityBeansAutoConfiguration.java | 19 +++
.../spring/user/util/AppUrlResolver.java | 110 +++++++++++++++++
.../spring/user/util/UserUtils.java | 6 +
...itional-spring-configuration-metadata.json | 10 ++
.../config/dsspringuserconfig.properties | 5 +
.../api/UserAPIRegistrationGuardTest.java | 4 +
.../spring/user/api/UserAPIUnitTest.java | 6 +
.../spring/user/util/AppUrlResolverTest.java | 115 ++++++++++++++++++
10 files changed, 282 insertions(+), 3 deletions(-)
create mode 100644 src/main/java/com/digitalsanctuary/spring/user/util/AppUrlResolver.java
create mode 100644 src/test/java/com/digitalsanctuary/spring/user/util/AppUrlResolverTest.java
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9049be1b..b0a9612d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,10 +5,12 @@
> **⚠️ ACTION REQUIRED if you run behind a reverse proxy:** password-reset and email-verification links are now built from a configured canonical URL and ignore `X-Forwarded-Host` unless it is explicitly allow-listed. Set `user.security.appUrl` (recommended) or `user.security.trustedHosts`, or your reset/verification links will point at the wrong host. See `MIGRATION.md`.
### Features
+- Added `AppUrlResolver` and `user.security.appUrl` / `user.security.trustedHosts` properties: password-reset and email-verification links are now built from a configured canonical URL, ignoring `X-Forwarded-Host` unless allow-listed (CWE-640).
### Fixes
### Breaking Changes
+- Reset/verification email links no longer trust `X-Forwarded-Host` by default. Deployments behind a reverse proxy must set `user.security.appUrl` or `user.security.trustedHosts` (see MIGRATION.md). `UserUtils.getAppUrl(HttpServletRequest)` is deprecated for removal.
### Notes
- Audit-log injection (originally slated here as a JSON-per-line format change) was already resolved in 4.4.0 via field sanitization (CR/LF and `|` stripped). The breaking JSON-per-line conversion was intentionally **not** carried into 5.0.0, as it offered no additional security benefit.
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 1dd2dfb4..ff58e8e1 100644
--- a/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java
+++ b/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java
@@ -38,6 +38,7 @@
import com.digitalsanctuary.spring.user.service.UserEmailService;
import com.digitalsanctuary.spring.user.service.UserService;
import com.digitalsanctuary.spring.user.service.WebAuthnCredentialManagementService;
+import com.digitalsanctuary.spring.user.util.AppUrlResolver;
import com.digitalsanctuary.spring.user.util.JSONResponse;
import com.digitalsanctuary.spring.user.util.UserUtils;
import jakarta.servlet.ServletException;
@@ -72,6 +73,7 @@ public class UserAPI {
private final ApplicationEventPublisher eventPublisher;
private final PasswordPolicyService passwordPolicyService;
private final ObjectProvider webAuthnCredentialManagementServiceProvider;
+ private final AppUrlResolver appUrlResolver;
@Value("${user.security.registrationPendingURI}")
private String registrationPendingURI;
@@ -151,7 +153,7 @@ public ResponseEntity resendRegistrationToken(@Valid @RequestBody
if (user.isEnabled()) {
return buildErrorResponse("Account is already verified.", 1, HttpStatus.CONFLICT);
}
- userEmailService.sendRegistrationVerificationEmail(user, UserUtils.getAppUrl(request));
+ userEmailService.sendRegistrationVerificationEmail(user, appUrlResolver.resolveAppUrl(request));
logAuditEvent("Resend Reg Token", "Success", "Verification Email Resent", user, request);
return buildSuccessResponse("Verification Email Resent Successfully!", registrationPendingURI);
}
@@ -203,7 +205,7 @@ public ResponseEntity updateUserAccount(@AuthenticationPrincipal D
public ResponseEntity resetPassword(@Valid @RequestBody PasswordResetRequestDto passwordResetRequest, HttpServletRequest request) {
User user = userService.findUserByEmail(passwordResetRequest.getEmail());
if (user != null) {
- userEmailService.sendForgotPasswordVerificationEmail(user, UserUtils.getAppUrl(request));
+ userEmailService.sendForgotPasswordVerificationEmail(user, appUrlResolver.resolveAppUrl(request));
logAuditEvent("Reset Password", "Success", "Password reset email sent", user, request);
}
return buildSuccessResponse("If account exists, password reset email has been sent!", forgotPasswordPendingURI);
@@ -542,7 +544,7 @@ private void logoutUser(HttpServletRequest request) {
* @param request the HTTP servlet request
*/
private void publishRegistrationEvent(User user, HttpServletRequest request) {
- String appUrl = UserUtils.getAppUrl(request);
+ String appUrl = appUrlResolver.resolveAppUrl(request);
eventPublisher.publishEvent(new OnRegistrationCompleteEvent(user, request.getLocale(), appUrl));
}
diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java b/src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java
index 34c506a3..e703a689 100644
--- a/src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java
+++ b/src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java
@@ -1,5 +1,6 @@
package com.digitalsanctuary.spring.user.security;
+import java.util.List;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.boot.autoconfigure.AutoConfiguration;
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
@@ -20,6 +21,7 @@
import org.springframework.security.web.session.HttpSessionEventPublisher;
import com.digitalsanctuary.spring.user.UserConfiguration;
import com.digitalsanctuary.spring.user.roles.RolesAndPrivilegesConfig;
+import com.digitalsanctuary.spring.user.util.AppUrlResolver;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
@@ -166,4 +168,21 @@ public HttpSessionEventPublisher httpSessionEventPublisher() {
public AuthenticationEventPublisher authenticationEventPublisher(ApplicationEventPublisher applicationEventPublisher) {
return new DefaultAuthenticationEventPublisher(applicationEventPublisher);
}
+
+ /**
+ * Creates the library's default {@link AppUrlResolver}, which builds the base URL for security-sensitive email links (password reset, email
+ * verification) from the configured canonical URL ({@code user.security.appUrl}) and/or the trusted-host allow-list
+ * ({@code user.security.trustedHosts}), defending against Host-header / X-Forwarded-Host poisoning (CWE-640). Backs off entirely if the consuming
+ * application defines its own {@link AppUrlResolver}.
+ *
+ * @param appUrl the configured canonical base URL, or {@code null} when unset
+ * @param trustedHosts the allow-listed forwarded hosts (empty when unset)
+ * @return the default {@link AppUrlResolver}
+ */
+ @Bean
+ @ConditionalOnMissingBean(AppUrlResolver.class)
+ public AppUrlResolver appUrlResolver(@Value("${user.security.appUrl:#{null}}") String appUrl,
+ @Value("${user.security.trustedHosts:}") List trustedHosts) {
+ return new AppUrlResolver(appUrl, trustedHosts);
+ }
}
diff --git a/src/main/java/com/digitalsanctuary/spring/user/util/AppUrlResolver.java b/src/main/java/com/digitalsanctuary/spring/user/util/AppUrlResolver.java
new file mode 100644
index 00000000..6c7c2a92
--- /dev/null
+++ b/src/main/java/com/digitalsanctuary/spring/user/util/AppUrlResolver.java
@@ -0,0 +1,110 @@
+package com.digitalsanctuary.spring.user.util;
+
+import java.util.List;
+import jakarta.servlet.http.HttpServletRequest;
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * Resolves the application base URL used to build security-sensitive email links (password reset, email verification), defending against Host-header /
+ * X-Forwarded-Host poisoning (CWE-640).
+ *
+ *
+ * Resolution order:
+ *
+ * - If {@code user.security.appUrl} is configured, always use it (forwarded headers ignored).
+ * - Otherwise build from the request, honoring {@code X-Forwarded-*} only when the resolved host is in {@code user.security.trustedHosts};
+ * otherwise use the container's own server name (the untrusted forwarded host is ignored and a warning is logged).
+ *
+ *
+ *
+ * The request-derived URL preserves the historical {@code UserUtils.getAppUrl} format with one refinement: the {@code :port} suffix is omitted when the
+ * port is the default for the scheme (80 for {@code http}, 443 for {@code https}), and the context path is appended only when non-empty. This keeps
+ * link formatting stable for existing deployments while producing clean canonical URLs.
+ */
+@Slf4j
+public class AppUrlResolver {
+
+ private static final int DEFAULT_HTTP_PORT = 80;
+ private static final int DEFAULT_HTTPS_PORT = 443;
+
+ private final String configuredAppUrl;
+ private final List trustedHosts;
+
+ /**
+ * Creates a resolver.
+ *
+ * @param configuredAppUrl the canonical base URL ({@code user.security.appUrl}); blank/null means "not configured"
+ * @param trustedHosts the allow-listed forwarded hosts ({@code user.security.trustedHosts}); null is treated as empty
+ */
+ public AppUrlResolver(String configuredAppUrl, List trustedHosts) {
+ this.configuredAppUrl = (configuredAppUrl == null || configuredAppUrl.isBlank()) ? null : configuredAppUrl.trim();
+ this.trustedHosts = trustedHosts == null ? List.of() : trustedHosts.stream().map(String::trim).toList();
+ }
+
+ /**
+ * Resolves the application base URL for the given request.
+ *
+ * @param request the current HTTP request
+ * @return the resolved base URL (no trailing slash)
+ */
+ public String resolveAppUrl(HttpServletRequest request) {
+ if (configuredAppUrl != null) {
+ return configuredAppUrl;
+ }
+
+ String fwdHost = request.getHeader("X-Forwarded-Host");
+ boolean useForwarded = fwdHost != null && !fwdHost.isEmpty() && trustedHosts.contains(stripPort(fwdHost));
+ if (fwdHost != null && !fwdHost.isEmpty() && !useForwarded) {
+ log.warn("AppUrlResolver: ignoring untrusted X-Forwarded-Host '{}' (not in user.security.trustedHosts)", fwdHost);
+ }
+
+ String scheme = useForwarded ? headerOr(request, "X-Forwarded-Proto", request.getScheme()) : request.getScheme();
+ String host = useForwarded ? stripPort(fwdHost) : request.getServerName();
+ int port = useForwarded ? forwardedPort(request, scheme) : request.getServerPort();
+
+ StringBuilder url = new StringBuilder();
+ url.append(scheme).append("://").append(host);
+ if (!isDefaultPort(scheme, port)) {
+ url.append(':').append(port);
+ }
+ String contextPath = request.getContextPath();
+ if (contextPath != null && !contextPath.isEmpty()) {
+ url.append(contextPath);
+ }
+ return url.toString();
+ }
+
+ private static int forwardedPort(HttpServletRequest request, String forwardedScheme) {
+ String portHeader = request.getHeader("X-Forwarded-Port");
+ if (portHeader != null && !portHeader.isBlank()) {
+ try {
+ return Integer.parseInt(portHeader.trim());
+ } catch (NumberFormatException e) {
+ log.warn("AppUrlResolver: ignoring non-numeric X-Forwarded-Port '{}'", portHeader);
+ }
+ }
+ // No usable X-Forwarded-Port: derive the port from the forwarded scheme so we don't leak the
+ // container's internal port (e.g. 8080) into the email link. Default ports are omitted later.
+ return "https".equalsIgnoreCase(forwardedScheme) ? DEFAULT_HTTPS_PORT : DEFAULT_HTTP_PORT;
+ }
+
+ private static boolean isDefaultPort(String scheme, int port) {
+ return ("http".equalsIgnoreCase(scheme) && port == DEFAULT_HTTP_PORT)
+ || ("https".equalsIgnoreCase(scheme) && port == DEFAULT_HTTPS_PORT);
+ }
+
+ private static String headerOr(HttpServletRequest req, String header, String fallback) {
+ String v = req.getHeader(header);
+ return (v == null || v.isEmpty()) ? fallback : v;
+ }
+
+ private static String stripPort(String host) {
+ if (host.startsWith("[")) {
+ // IPv6 literal: [::1] or [::1]:8443
+ int bracket = host.indexOf(']');
+ return bracket > 0 ? host.substring(0, bracket + 1) : host;
+ }
+ int colon = host.lastIndexOf(':');
+ return colon > 0 ? host.substring(0, colon) : host;
+ }
+}
diff --git a/src/main/java/com/digitalsanctuary/spring/user/util/UserUtils.java b/src/main/java/com/digitalsanctuary/spring/user/util/UserUtils.java
index b659b5a9..9c526a9a 100644
--- a/src/main/java/com/digitalsanctuary/spring/user/util/UserUtils.java
+++ b/src/main/java/com/digitalsanctuary/spring/user/util/UserUtils.java
@@ -50,7 +50,13 @@ public static String getClientIP(HttpServletRequest request) {
*
* @param request The HttpServletRequest object.
* @return The application URL as a String.
+ * @deprecated This method trusts {@code X-Forwarded-Host} unconditionally, which makes security-sensitive email links (password reset, email
+ * verification) vulnerable to Host-header / X-Forwarded-Host poisoning (CWE-640). Use
+ * {@link com.digitalsanctuary.spring.user.util.AppUrlResolver#resolveAppUrl(HttpServletRequest)} instead, which builds the base URL from
+ * a configured canonical URL ({@code user.security.appUrl}) and/or a trusted-host allow-list ({@code user.security.trustedHosts}).
+ * Scheduled for removal in a future release.
*/
+ @Deprecated(forRemoval = true)
public static String getAppUrl(HttpServletRequest request) {
// Check for forwarded protocol
String scheme = request.getHeader("X-Forwarded-Proto");
diff --git a/src/main/resources/META-INF/additional-spring-configuration-metadata.json b/src/main/resources/META-INF/additional-spring-configuration-metadata.json
index 09914cfa..035a2a97 100644
--- a/src/main/resources/META-INF/additional-spring-configuration-metadata.json
+++ b/src/main/resources/META-INF/additional-spring-configuration-metadata.json
@@ -36,6 +36,16 @@
"type": "java.lang.String",
"description": "Cron expression for token purge schedule"
},
+ {
+ "name": "user.security.appUrl",
+ "type": "java.lang.String",
+ "description": "Canonical base URL for security email links (password reset, verification). STRONGLY recommended in production to prevent Host-header poisoning (CWE-640). When set, X-Forwarded-Host is ignored."
+ },
+ {
+ "name": "user.security.trustedHosts",
+ "type": "java.util.List",
+ "description": "When user.security.appUrl is not set, X-Forwarded-Host is honored only for hosts in this comma-separated allowlist; otherwise the container's own server name is used."
+ },
{
"name": "user.security.loginActionURI",
"type": "java.lang.String",
diff --git a/src/main/resources/config/dsspringuserconfig.properties b/src/main/resources/config/dsspringuserconfig.properties
index 7582f512..4c8ffc00 100644
--- a/src/main/resources/config/dsspringuserconfig.properties
+++ b/src/main/resources/config/dsspringuserconfig.properties
@@ -81,6 +81,11 @@ user.security.bcryptStrength=12
# user.security.tokenHashSecret=
# The lifetime, in minutes, of a password reset token before it expires. Default is 1440 (24 hours).
user.security.passwordResetTokenValidityMinutes=1440
+# Canonical base URL for security email links (password reset, verification). STRONGLY recommended in production
+# to prevent Host-header poisoning (CWE-640). When set, X-Forwarded-Host is ignored.
+user.security.appUrl=
+# When appUrl is not set, X-Forwarded-Host is honored only for hosts in this comma-separated allowlist.
+user.security.trustedHosts=
# 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
# The default action for all requests. This can be either deny or allow.
diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIRegistrationGuardTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIRegistrationGuardTest.java
index e26c8938..30ac4732 100644
--- a/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIRegistrationGuardTest.java
+++ b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIRegistrationGuardTest.java
@@ -33,6 +33,7 @@
import com.digitalsanctuary.spring.user.service.PasswordPolicyService;
import com.digitalsanctuary.spring.user.service.UserEmailService;
import com.digitalsanctuary.spring.user.service.UserService;
+import com.digitalsanctuary.spring.user.util.AppUrlResolver;
import com.digitalsanctuary.spring.user.service.WebAuthnCredentialManagementService;
import com.digitalsanctuary.spring.user.test.builders.UserTestDataBuilder;
import com.fasterxml.jackson.databind.ObjectMapper;
@@ -67,6 +68,9 @@ class UserAPIRegistrationGuardTest {
@Mock
private WebAuthnCredentialManagementService webAuthnService;
+ @Mock
+ private AppUrlResolver appUrlResolver;
+
@InjectMocks
private UserAPI userAPI;
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 62be1f0d..79ced2c5 100644
--- a/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java
+++ b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java
@@ -22,6 +22,7 @@
import com.digitalsanctuary.spring.user.service.UserEmailService;
import com.digitalsanctuary.spring.user.service.UserService;
import com.digitalsanctuary.spring.user.test.builders.UserTestDataBuilder;
+import com.digitalsanctuary.spring.user.util.AppUrlResolver;
import com.digitalsanctuary.spring.user.util.JSONResponse;
import com.fasterxml.jackson.databind.ObjectMapper;
@@ -91,6 +92,9 @@ public JSONResponse handleSecurityException(SecurityException e) {
@Mock
private PasswordPolicyService passwordPolicyService;
+ @Mock
+ private AppUrlResolver appUrlResolver;
+
@InjectMocks
private UserAPI userAPI;
@@ -286,6 +290,7 @@ void resendRegistrationToken_success() throws Exception {
.disabled()
.build();
when(userService.findUserByEmail(testUserDto.getEmail())).thenReturn(unverifiedUser);
+ when(appUrlResolver.resolveAppUrl(any())).thenReturn("http://localhost:8080");
// When & Then
mockMvc.perform(post("/user/resendRegistrationToken")
@@ -342,6 +347,7 @@ class PasswordManagementTests {
void resetPassword_success() throws Exception {
// Given
when(userService.findUserByEmail(testUserDto.getEmail())).thenReturn(testUser);
+ when(appUrlResolver.resolveAppUrl(any())).thenReturn("http://localhost:8080");
// When & Then
mockMvc.perform(post("/user/resetPassword")
diff --git a/src/test/java/com/digitalsanctuary/spring/user/util/AppUrlResolverTest.java b/src/test/java/com/digitalsanctuary/spring/user/util/AppUrlResolverTest.java
new file mode 100644
index 00000000..1aa1d4dd
--- /dev/null
+++ b/src/test/java/com/digitalsanctuary/spring/user/util/AppUrlResolverTest.java
@@ -0,0 +1,115 @@
+package com.digitalsanctuary.spring.user.util;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import java.util.List;
+import org.junit.jupiter.api.Test;
+import org.springframework.mock.web.MockHttpServletRequest;
+
+/**
+ * Unit tests for {@link AppUrlResolver}, verifying defense against Host-header / X-Forwarded-Host
+ * poisoning (CWE-640).
+ */
+class AppUrlResolverTest {
+
+ @Test
+ void usesConfiguredAppUrlAndIgnoresForwardedHostWhenConfigured() {
+ AppUrlResolver resolver = new AppUrlResolver("https://app.example.com", List.of());
+ MockHttpServletRequest req = new MockHttpServletRequest();
+ req.addHeader("X-Forwarded-Host", "evil.com");
+ assertThat(resolver.resolveAppUrl(req)).isEqualTo("https://app.example.com");
+ }
+
+ @Test
+ void rejectsForwardedHostNotInAllowlistWhenNoConfiguredUrl() {
+ AppUrlResolver resolver = new AppUrlResolver(null, List.of("trusted.example.com"));
+ MockHttpServletRequest req = new MockHttpServletRequest();
+ req.setScheme("https");
+ req.setServerName("trusted.example.com");
+ req.setServerPort(443);
+ req.addHeader("X-Forwarded-Host", "evil.com");
+ assertThat(resolver.resolveAppUrl(req)).contains("trusted.example.com").doesNotContain("evil.com");
+ }
+
+ @Test
+ void honorsForwardedHostOnlyWhenAllowListed() {
+ AppUrlResolver resolver = new AppUrlResolver(null, List.of("trusted.example.com"));
+ MockHttpServletRequest req = new MockHttpServletRequest();
+ req.setScheme("http");
+ req.setServerName("internal");
+ req.setServerPort(8080);
+ req.addHeader("X-Forwarded-Proto", "https");
+ req.addHeader("X-Forwarded-Host", "trusted.example.com");
+ // A correctly-behaving HTTPS reverse proxy also forwards the public port; with the default
+ // HTTPS port (443) the resolver omits the redundant ":port" suffix, yielding a clean URL.
+ req.addHeader("X-Forwarded-Port", "443");
+ assertThat(resolver.resolveAppUrl(req)).isEqualTo("https://trusted.example.com");
+ }
+
+ @Test
+ void honorsAllowListedIpv6ForwardedHostWhenForwarded() {
+ AppUrlResolver resolver = new AppUrlResolver(null, List.of("[::1]"));
+ MockHttpServletRequest req = new MockHttpServletRequest();
+ req.setScheme("http");
+ req.setServerName("internal");
+ req.setServerPort(8080);
+ req.addHeader("X-Forwarded-Proto", "https");
+ req.addHeader("X-Forwarded-Host", "[::1]:8443");
+ // The IPv6 literal (with embedded colons) must be matched against the allow-list correctly,
+ // so the trusted forwarded host is honored rather than falling back to the container host.
+ String resolved = resolver.resolveAppUrl(req);
+ assertThat(resolved).contains("[::1]").doesNotContain("internal");
+ }
+
+ @Test
+ void derivesPortFromForwardedSchemeWhenForwardedPortAbsent() {
+ AppUrlResolver resolver = new AppUrlResolver(null, List.of("trusted.example.com"));
+ MockHttpServletRequest req = new MockHttpServletRequest();
+ req.setScheme("http");
+ req.setServerName("internal");
+ req.setServerPort(8080);
+ req.addHeader("X-Forwarded-Proto", "https");
+ req.addHeader("X-Forwarded-Host", "trusted.example.com");
+ // No X-Forwarded-Port: the port must be derived from the forwarded https scheme (443) and then
+ // omitted as the default, NOT taken from the container's internal port (8080).
+ assertThat(resolver.resolveAppUrl(req)).isEqualTo("https://trusted.example.com");
+ }
+
+ @Test
+ void omitsDefaultPortAndContextPathForNonForwardedRequest() {
+ AppUrlResolver resolver = new AppUrlResolver(null, List.of());
+ MockHttpServletRequest req = new MockHttpServletRequest();
+ req.setScheme("https");
+ req.setServerName("api.example.com");
+ req.setServerPort(443);
+ req.setContextPath("");
+ assertThat(resolver.resolveAppUrl(req)).isEqualTo("https://api.example.com");
+ }
+
+ @Test
+ void includesNonDefaultPortAndContextPathForNonForwardedRequest() {
+ AppUrlResolver resolver = new AppUrlResolver(null, List.of());
+ MockHttpServletRequest req = new MockHttpServletRequest();
+ req.setScheme("http");
+ req.setServerName("localhost");
+ req.setServerPort(8080);
+ req.setContextPath("/app");
+ assertThat(resolver.resolveAppUrl(req)).isEqualTo("http://localhost:8080/app");
+ }
+
+ @Test
+ void ignoresUntrustedForwardedHostAndUsesContainerServerName() {
+ AppUrlResolver resolver = new AppUrlResolver(null, List.of("trusted.example.com"));
+ MockHttpServletRequest req = new MockHttpServletRequest();
+ req.setScheme("http");
+ req.setServerName("internal");
+ req.setServerPort(8080);
+ req.addHeader("X-Forwarded-Proto", "https");
+ req.addHeader("X-Forwarded-Host", "evil.com");
+ req.addHeader("X-Forwarded-Port", "443");
+ // Untrusted forwarded host -> forwarded headers are NOT honored; the container's own
+ // scheme/host/port are used instead.
+ String resolved = resolver.resolveAppUrl(req);
+ assertThat(resolved).isEqualTo("http://internal:8080");
+ assertThat(resolved).doesNotContain("evil.com");
+ }
+}
From 6e1fb79f9f51ff6e1b1ea6e2516cdcee6796c98e Mon Sep 17 00:00:00 2001
From: Devon Hillard
Date: Mon, 15 Jun 2026 09:39:54 -0600
Subject: [PATCH 03/23] fix(security): add unique constraint on token column
(PasswordResetToken, VerificationToken)
Adds @Column(nullable=false, unique=true) to the token field of both
PasswordResetToken and VerificationToken. Token values have been hashed
at rest since 4.4.0, so the fixed-length values are safe to index.
Includes a @DatabaseTest (H2 slice) that verifies the constraint is
enforced on flush for both tables. Updates CHANGELOG.md (Breaking
Changes) and MIGRATION.md (DDL migration guidance for Flyway/Liquibase
users).
---
CHANGELOG.md | 1 +
MIGRATION.md | 21 ++++++
.../persistence/model/PasswordResetToken.java | 2 +
.../persistence/model/VerificationToken.java | 2 +
.../model/TokenUniquenessTest.java | 64 +++++++++++++++++++
5 files changed, 90 insertions(+)
create mode 100644 src/test/java/com/digitalsanctuary/spring/user/persistence/model/TokenUniquenessTest.java
diff --git a/CHANGELOG.md b/CHANGELOG.md
index b0a9612d..d313f9f7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,6 +11,7 @@
### Breaking Changes
- Reset/verification email links no longer trust `X-Forwarded-Host` by default. Deployments behind a reverse proxy must set `user.security.appUrl` or `user.security.trustedHosts` (see MIGRATION.md). `UserUtils.getAppUrl(HttpServletRequest)` is deprecated for removal.
+- Added a UNIQUE, NOT NULL constraint on the `token` column of `password_reset_token` and `verification_token`. This is a schema/DDL change — see MIGRATION.md.
### Notes
- Audit-log injection (originally slated here as a JSON-per-line format change) was already resolved in 4.4.0 via field sanitization (CR/LF and `|` stripped). The breaking JSON-per-line conversion was intentionally **not** carried into 5.0.0, as it offered no additional security benefit.
diff --git a/MIGRATION.md b/MIGRATION.md
index 8a8afccb..3b9c3bcc 100644
--- a/MIGRATION.md
+++ b/MIGRATION.md
@@ -54,6 +54,27 @@ Password-reset and email-verification links are now built from a configured cano
Local development with no proxy needs no change. `UserUtils.getAppUrl(HttpServletRequest)` is deprecated in favor of `AppUrlResolver`.
+### Database schema: unique token constraint
+
+The `token` column on both `password_reset_token` and `verification_token` now carries a **UNIQUE + NOT NULL** constraint. The stored value is always a fixed-length hash (introduced in 4.4.0), so the column length is predictable and the index is safe.
+
+**Applications using `spring.jpa.hibernate.ddl-auto=update` (or `create`/`create-drop`):** Hibernate will add the unique index automatically on startup — no manual action required.
+
+**Applications managing schema manually (Flyway, Liquibase, or `ddl-auto=validate`/`none`):** apply the following DDL before upgrading and before starting the application:
+
+```sql
+-- Ensure no existing null or duplicate token values exist first.
+-- (4.4.0 already enforces one active token per user, so duplicates are unlikely.)
+ALTER TABLE password_reset_token ALTER COLUMN token SET NOT NULL;
+ALTER TABLE verification_token ALTER COLUMN token SET NOT NULL;
+CREATE UNIQUE INDEX ux_password_reset_token_token ON password_reset_token (token);
+CREATE UNIQUE INDEX ux_verification_token_token ON verification_token (token);
+```
+
+> **Note:** The DDL syntax above is standard SQL (compatible with PostgreSQL and MariaDB/MySQL). For MySQL/MariaDB, `ALTER COLUMN token SET NOT NULL` may need to include the full column definition, e.g. `MODIFY COLUMN token VARCHAR(255) NOT NULL`.
+
+If your database contains rows with a `null` token value (possible only if tokens were created before 4.4.0 without the hash path), delete or back-fill those rows before applying the NOT NULL constraint.
+
## Migrating to 4.0.x (Spring Boot 4.0)
diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/PasswordResetToken.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/PasswordResetToken.java
index e74b158a..97325469 100644
--- a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/PasswordResetToken.java
+++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/PasswordResetToken.java
@@ -2,6 +2,7 @@
import java.util.Calendar;
import java.util.Date;
+import jakarta.persistence.Column;
import jakarta.persistence.Entity;
import jakarta.persistence.FetchType;
import jakarta.persistence.GeneratedValue;
@@ -27,6 +28,7 @@ public class PasswordResetToken {
private Long id;
/** The token. */
+ @Column(name = "token", nullable = false, unique = true)
private String token;
/** The user. */
diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/VerificationToken.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/VerificationToken.java
index 0aa5f589..2bdfa07b 100644
--- a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/VerificationToken.java
+++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/VerificationToken.java
@@ -2,6 +2,7 @@
import java.util.Calendar;
import java.util.Date;
+import jakarta.persistence.Column;
import jakarta.persistence.Entity;
import jakarta.persistence.FetchType;
import jakarta.persistence.ForeignKey;
@@ -29,6 +30,7 @@ public class VerificationToken {
private Long id;
/** The token. */
+ @Column(name = "token", nullable = false, unique = true)
private String token;
/** The user. */
diff --git a/src/test/java/com/digitalsanctuary/spring/user/persistence/model/TokenUniquenessTest.java b/src/test/java/com/digitalsanctuary/spring/user/persistence/model/TokenUniquenessTest.java
new file mode 100644
index 00000000..d338b387
--- /dev/null
+++ b/src/test/java/com/digitalsanctuary/spring/user/persistence/model/TokenUniquenessTest.java
@@ -0,0 +1,64 @@
+package com.digitalsanctuary.spring.user.persistence.model;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.jpa.test.autoconfigure.TestEntityManager;
+import org.springframework.dao.DataIntegrityViolationException;
+import com.digitalsanctuary.spring.user.persistence.repository.PasswordResetTokenRepository;
+import com.digitalsanctuary.spring.user.persistence.repository.VerificationTokenRepository;
+import com.digitalsanctuary.spring.user.test.annotations.DatabaseTest;
+import com.digitalsanctuary.spring.user.test.builders.UserTestDataBuilder;
+
+/**
+ * Database-slice tests verifying that the UNIQUE + NOT NULL constraint on the {@code token} column
+ * of {@link PasswordResetToken} and {@link VerificationToken} is enforced by the schema. Two tokens
+ * with the same value must not coexist in either table.
+ */
+@DatabaseTest
+class TokenUniquenessTest {
+
+ @Autowired
+ private PasswordResetTokenRepository passwordResetTokenRepository;
+
+ @Autowired
+ private VerificationTokenRepository verificationTokenRepository;
+
+ @Autowired
+ private TestEntityManager entityManager;
+
+ private User persistUser(String email) {
+ User user = UserTestDataBuilder.aUser().withId(null).withEmail(email).build();
+ return entityManager.persistAndFlush(user);
+ }
+
+ @Test
+ void shouldRejectDuplicateTokenValueWhenPasswordResetTokenIsFlushed() {
+ User user1 = persistUser("prt-unique1@test.com");
+ User user2 = persistUser("prt-unique2@test.com");
+ String duplicateToken = "duplicate-hash-value-prt";
+
+ PasswordResetToken first = new PasswordResetToken(duplicateToken, user1);
+ passwordResetTokenRepository.saveAndFlush(first);
+
+ PasswordResetToken second = new PasswordResetToken(duplicateToken, user2);
+
+ assertThatThrownBy(() -> passwordResetTokenRepository.saveAndFlush(second))
+ .isInstanceOf(DataIntegrityViolationException.class);
+ }
+
+ @Test
+ void shouldRejectDuplicateTokenValueWhenVerificationTokenIsFlushed() {
+ User user1 = persistUser("vt-unique1@test.com");
+ User user2 = persistUser("vt-unique2@test.com");
+ String duplicateToken = "duplicate-hash-value-vt";
+
+ VerificationToken first = new VerificationToken(duplicateToken, user1);
+ verificationTokenRepository.saveAndFlush(first);
+
+ VerificationToken second = new VerificationToken(duplicateToken, user2);
+
+ assertThatThrownBy(() -> verificationTokenRepository.saveAndFlush(second))
+ .isInstanceOf(DataIntegrityViolationException.class);
+ }
+}
From 28e15fb17c592d3effa4de1d3ba0baf2aef668be Mon Sep 17 00:00:00 2001
From: Devon Hillard
Date: Mon, 15 Jun 2026 09:45:10 -0600
Subject: [PATCH 04/23] fix(security): return uniform responses on
registration/resend to prevent account enumeration
---
CHANGELOG.md | 2 +
MIGRATION.md | 25 +++++
.../spring/user/api/UserAPI.java | 47 ++++++--
.../spring/user/api/UserAPIUnitTest.java | 105 ++++++++++--------
.../spring/user/api/UserApiTest.java | 22 ++--
5 files changed, 141 insertions(+), 60 deletions(-)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index d313f9f7..a05cc5d4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,10 +8,12 @@
- Added `AppUrlResolver` and `user.security.appUrl` / `user.security.trustedHosts` properties: password-reset and email-verification links are now built from a configured canonical URL, ignoring `X-Forwarded-Host` unless allow-listed (CWE-640).
### Fixes
+- Registration and resend-verification endpoints no longer reveal whether an email is registered or already verified (account-enumeration hardening).
### Breaking Changes
- Reset/verification email links no longer trust `X-Forwarded-Host` by default. Deployments behind a reverse proxy must set `user.security.appUrl` or `user.security.trustedHosts` (see MIGRATION.md). `UserUtils.getAppUrl(HttpServletRequest)` is deprecated for removal.
- Added a UNIQUE, NOT NULL constraint on the `token` column of `password_reset_token` and `verification_token`. This is a schema/DDL change — see MIGRATION.md.
+- The registration and resend-verification endpoints now always return HTTP 200 with a uniform generic body. Previously an existing email returned 409 on registration, and resend returned 409 (already verified) or 500 (unknown email). Clients that branched on those status codes or messages must update — the response is now intentionally uniform.
### Notes
- Audit-log injection (originally slated here as a JSON-per-line format change) was already resolved in 4.4.0 via field sanitization (CR/LF and `|` stripped). The breaking JSON-per-line conversion was intentionally **not** carried into 5.0.0, as it offered no additional security benefit.
diff --git a/MIGRATION.md b/MIGRATION.md
index 3b9c3bcc..f1b6cc0e 100644
--- a/MIGRATION.md
+++ b/MIGRATION.md
@@ -75,6 +75,31 @@ CREATE UNIQUE INDEX ux_verification_token_token ON verification_token (token);
If your database contains rows with a `null` token value (possible only if tokens were created before 4.4.0 without the hash path), delete or back-fill those rows before applying the NOT NULL constraint.
+### Registration & resend responses are now uniform (anti-enumeration)
+
+The `/user/registration` and `/user/resendRegistrationToken` endpoints now return the **same generic, success-shaped HTTP 200 response** regardless of whether the email is already registered or already verified. This prevents attackers from using these endpoints to enumerate which email addresses have accounts and which are verified (CWE-204).
+
+**Old behavior:**
+
+| Endpoint | Case | Old status | Old body message |
+|---|---|---|---|
+| `/user/registration` | New email | 200 | `Registration Successful!` |
+| `/user/registration` | Email already exists | 409 Conflict | `An account already exists for the email address` (code 2) |
+| `/user/resendRegistrationToken` | Unverified account | 200 | `Verification Email Resent Successfully!` |
+| `/user/resendRegistrationToken` | Already-verified account | 409 Conflict | `Account is already verified.` (code 1) |
+| `/user/resendRegistrationToken` | Unknown email | 500 Internal Server Error | `System Error!` (code 2) |
+
+**New behavior:**
+
+| Endpoint | All cases | New status | New body message |
+|---|---|---|---|
+| `/user/registration` | New email **or** already exists | 200 | `If your email address is eligible, you will receive a verification email shortly.` (success, code 0) |
+| `/user/resendRegistrationToken` | Unverified, already-verified, **or** unknown email | 200 | `If your account requires verification, a new verification email has been sent.` (success, code 0) |
+
+Internally the framework still does the correct thing — a brand-new registration creates the account and sends verification, an existing email creates nothing, and resend sends an email only when the account exists and is unverified — and the true outcome is still recorded in the audit log. Only the externally observable response is now uniform.
+
+**Action required:** Clients (web UIs, mobile apps, integrations) must no longer rely on the `409` status (existing/verified account) or the `500` status (unknown email on resend) to detect account existence or verification state. Branch only on the `success` flag for these two endpoints, and present the generic message to end users.
+
## Migrating to 4.0.x (Spring Boot 4.0)
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 ff58e8e1..df38ff5d 100644
--- a/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java
+++ b/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java
@@ -67,6 +67,22 @@ public class UserAPI {
/** Error code returned when the {@link RegistrationGuard} denies a registration attempt. */
private static final int ERROR_CODE_REGISTRATION_DENIED = 6;
+ /**
+ * Generic, success-shaped message returned by {@code /registration} for every outcome (new account
+ * created, or email already registered). Keeping the body identical across cases prevents an attacker
+ * from using the registration endpoint to enumerate which email addresses are already registered.
+ */
+ private static final String REGISTRATION_GENERIC_MESSAGE =
+ "If your email address is eligible, you will receive a verification email shortly.";
+
+ /**
+ * Generic, success-shaped message returned by {@code /resendRegistrationToken} for every outcome
+ * (email unknown, account already verified, or verification email actually resent). Keeping the body
+ * identical across cases prevents enumeration of which emails exist and which are already verified.
+ */
+ private static final String RESEND_GENERIC_MESSAGE =
+ "If your account requires verification, a new verification email has been sent.";
+
private final UserService userService;
private final UserEmailService userEmailService;
private final MessageSource messages;
@@ -121,14 +137,23 @@ public ResponseEntity registerUserAccount(@Valid @RequestBody User
String nextURL = registeredUser.isEnabled() ? handleAutoLogin(registeredUser) : registrationPendingURI;
- return buildSuccessResponse("Registration Successful!", nextURL);
+ return buildSuccessResponse(REGISTRATION_GENERIC_MESSAGE, nextURL);
} catch (RegistrationDeniedException ex) {
log.info("Registration denied for email: {} source: FORM reason: {}", userDto.getEmail(), ex.getReason());
return buildErrorResponse(ex.getReason(), ERROR_CODE_REGISTRATION_DENIED, HttpStatus.FORBIDDEN);
} catch (UserAlreadyExistException ex) {
+ // Anti-enumeration: the email is already registered, so we create NOTHING and publish no
+ // registration event, but we return exactly the same generic 200 response a brand-new
+ // registration would produce. The true reason is recorded server-side via the audit event.
+ //
+ // Returning registrationPendingURI here mirrors what a genuine new (unverified) registration
+ // returns in the default verification-enabled config, making both cases indistinguishable to
+ // the caller. In verification-disabled / auto-login mode a real new registration additionally
+ // establishes a session — that is an inherent, accepted difference that cannot be avoided
+ // without skipping auto-login for legitimate new users.
log.warn("User already exists with email: {}", userDto.getEmail());
logAuditEvent("Registration", "Failure", "User Already Exists", null, request);
- return buildErrorResponse("An account already exists for the email address", 2, HttpStatus.CONFLICT);
+ return buildSuccessResponse(REGISTRATION_GENERIC_MESSAGE, registrationPendingURI);
} catch (Exception ex) {
log.error("Unexpected error during registration.", ex);
logAuditEvent("Registration", "Failure", ex.getMessage(), null, request);
@@ -148,16 +173,22 @@ public ResponseEntity registerUserAccount(@Valid @RequestBody User
@PostMapping("/resendRegistrationToken")
public ResponseEntity resendRegistrationToken(@Valid @RequestBody UserDto userDto,
HttpServletRequest request) {
+ // Anti-enumeration: this endpoint ALWAYS returns the same generic 200 response, regardless of
+ // whether the email is unknown, already verified, or genuinely awaiting verification. Internally
+ // we only send the verification email when the account exists AND is still unverified. The true
+ // outcome is recorded server-side via audit/log events so operators retain visibility.
User user = userService.findUserByEmail(userDto.getEmail());
- if (user != null) {
- if (user.isEnabled()) {
- return buildErrorResponse("Account is already verified.", 1, HttpStatus.CONFLICT);
- }
+ if (user == null) {
+ log.info("Resend verification requested for unknown email; returning generic response.");
+ logAuditEvent("Resend Reg Token", "Failure", "Unknown Email", null, request);
+ } else if (user.isEnabled()) {
+ log.info("Resend verification requested for already-verified account; returning generic response.");
+ logAuditEvent("Resend Reg Token", "Failure", "Account Already Verified", user, request);
+ } else {
userEmailService.sendRegistrationVerificationEmail(user, appUrlResolver.resolveAppUrl(request));
logAuditEvent("Resend Reg Token", "Success", "Verification Email Resent", user, request);
- return buildSuccessResponse("Verification Email Resent Successfully!", registrationPendingURI);
}
- return buildErrorResponse("System Error!", 2, HttpStatus.INTERNAL_SERVER_ERROR);
+ return buildSuccessResponse(RESEND_GENERIC_MESSAGE, registrationPendingURI);
}
/**
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 79ced2c5..9c9b84bf 100644
--- a/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java
+++ b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java
@@ -1,13 +1,22 @@
package com.digitalsanctuary.spring.user.api;
+import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.argThat;
import static org.mockito.ArgumentMatchers.eq;
-import static org.mockito.Mockito.*;
-import static org.assertj.core.api.Assertions.assertThat;
-import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*;
-import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*;
-import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print;
+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 static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf;
+import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete;
+import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
+
+import java.util.Collections;
+import java.util.Locale;
import com.digitalsanctuary.spring.user.audit.AuditEvent;
import com.digitalsanctuary.spring.user.dto.PasswordDto;
@@ -25,7 +34,6 @@
import com.digitalsanctuary.spring.user.util.AppUrlResolver;
import com.digitalsanctuary.spring.user.util.JSONResponse;
import com.fasterxml.jackson.databind.ObjectMapper;
-
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Nested;
@@ -37,22 +45,16 @@
import org.mockito.junit.jupiter.MockitoExtension;
import org.springframework.context.ApplicationEventPublisher;
import org.springframework.context.MessageSource;
+import org.springframework.http.HttpStatus;
import org.springframework.http.MediaType;
+import org.springframework.security.web.method.annotation.AuthenticationPrincipalArgumentResolver;
import org.springframework.test.util.ReflectionTestUtils;
import org.springframework.test.web.servlet.MockMvc;
-import org.springframework.test.web.servlet.MvcResult;
import org.springframework.test.web.servlet.setup.MockMvcBuilders;
-import org.springframework.web.method.support.HandlerMethodArgumentResolver;
-import org.springframework.security.web.method.annotation.AuthenticationPrincipalArgumentResolver;
+import org.springframework.validation.beanvalidation.LocalValidatorFactoryBean;
import org.springframework.web.bind.annotation.ExceptionHandler;
import org.springframework.web.bind.annotation.RestControllerAdvice;
-import org.springframework.http.HttpStatus;
-import org.springframework.validation.beanvalidation.LocalValidatorFactoryBean;
-
-import java.util.Collections;
-import java.util.Locale;
-
-import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf;
+import org.springframework.web.method.support.HandlerMethodArgumentResolver;
@ExtendWith(MockitoExtension.class)
@DisplayName("UserAPI Unit Tests")
@@ -155,16 +157,16 @@ void registerUserAccount_success_withVerificationEmail() throws Exception {
.thenReturn(Collections.emptyList());
// When & Then
- MvcResult result = mockMvc.perform(post("/user/registration")
+ mockMvc.perform(post("/user/registration")
.contentType(MediaType.APPLICATION_JSON)
.content(objectMapper.writeValueAsString(testUserDto))
.with(csrf()))
.andExpect(status().isOk())
.andExpect(jsonPath("$.success").value(true))
.andExpect(jsonPath("$.code").value(0))
- .andExpect(jsonPath("$.messages[0]").value("Registration Successful!"))
- .andExpect(jsonPath("$.redirectUrl").value("/user/registration-pending.html"))
- .andReturn();
+ .andExpect(jsonPath("$.messages[0]")
+ .value("If your email address is eligible, you will receive a verification email shortly."))
+ .andExpect(jsonPath("$.redirectUrl").value("/user/registration-pending.html"));
// Verify event publishing
verify(eventPublisher, times(2)).publishEvent(any());
@@ -209,23 +211,29 @@ void registerUserAccount_success_withAutoLogin() throws Exception {
}
@Test
- @DisplayName("POST /user/registration - user already exists")
- void registerUserAccount_userAlreadyExists() throws Exception {
- // Given
+ @DisplayName("POST /user/registration - existing email returns the same uniform 200 body as a new registration")
+ void registerUserAccount_existingEmail_returnsUniformResponse() throws Exception {
+ // Given - the service signals the email is already registered
when(userService.registerNewUserAccount(any(UserDto.class)))
.thenThrow(new UserAlreadyExistException("User already exists"));
when(passwordPolicyService.validate(any(), anyString(), anyString(), any(Locale.class)))
.thenReturn(Collections.emptyList());
- // When & Then
+ // When & Then - response is indistinguishable from a brand-new registration
mockMvc.perform(post("/user/registration")
.contentType(MediaType.APPLICATION_JSON)
.content(objectMapper.writeValueAsString(testUserDto))
.with(csrf()))
- .andExpect(status().isConflict())
- .andExpect(jsonPath("$.success").value(false))
- .andExpect(jsonPath("$.code").value(2))
- .andExpect(jsonPath("$.messages[0]").value("An account already exists for the email address"));
+ .andExpect(status().isOk())
+ .andExpect(jsonPath("$.success").value(true))
+ .andExpect(jsonPath("$.code").value(0))
+ .andExpect(jsonPath("$.messages[0]")
+ .value("If your email address is eligible, you will receive a verification email shortly."))
+ .andExpect(jsonPath("$.redirectUrl").value("/user/registration-pending.html"));
+
+ // No new account is created: no registration event is published and no auto-login occurs.
+ verify(eventPublisher, never()).publishEvent(any(OnRegistrationCompleteEvent.class));
+ verify(userService, never()).authWithoutPassword(any());
}
@Test
@@ -299,42 +307,51 @@ void resendRegistrationToken_success() throws Exception {
.with(csrf()))
.andExpect(status().isOk())
.andExpect(jsonPath("$.success").value(true))
- .andExpect(jsonPath("$.messages[0]").value("Verification Email Resent Successfully!"));
+ .andExpect(jsonPath("$.code").value(0))
+ .andExpect(jsonPath("$.messages[0]")
+ .value("If your account requires verification, a new verification email has been sent."));
verify(userEmailService).sendRegistrationVerificationEmail(eq(unverifiedUser), anyString());
}
@Test
- @DisplayName("POST /user/resendRegistrationToken - account already verified")
- void resendRegistrationToken_alreadyVerified() throws Exception {
- // Given
+ @DisplayName("POST /user/resendRegistrationToken - already-verified account returns the same uniform 200 body")
+ void resendRegistrationToken_alreadyVerified_returnsUniformResponse() throws Exception {
+ // Given - an existing, already-enabled (verified) account
when(userService.findUserByEmail(testUserDto.getEmail())).thenReturn(testUser); // enabled user
- // When & Then
+ // When & Then - same response as the unverified case, and no email is sent
mockMvc.perform(post("/user/resendRegistrationToken")
.contentType(MediaType.APPLICATION_JSON)
.content(objectMapper.writeValueAsString(testUserDto))
.with(csrf()))
- .andExpect(status().isConflict())
- .andExpect(jsonPath("$.success").value(false))
- .andExpect(jsonPath("$.code").value(1))
- .andExpect(jsonPath("$.messages[0]").value("Account is already verified."));
+ .andExpect(status().isOk())
+ .andExpect(jsonPath("$.success").value(true))
+ .andExpect(jsonPath("$.code").value(0))
+ .andExpect(jsonPath("$.messages[0]")
+ .value("If your account requires verification, a new verification email has been sent."));
+
+ verify(userEmailService, never()).sendRegistrationVerificationEmail(any(), anyString());
}
@Test
- @DisplayName("POST /user/resendRegistrationToken - user not found")
- void resendRegistrationToken_userNotFound() throws Exception {
- // Given
+ @DisplayName("POST /user/resendRegistrationToken - unknown email returns the same uniform 200 body (no 500 leak)")
+ void resendRegistrationToken_unknownEmail_returnsUniformResponse() throws Exception {
+ // Given - no account exists for the email
when(userService.findUserByEmail(testUserDto.getEmail())).thenReturn(null);
- // When & Then
+ // When & Then - same uniform 200 response; nothing leaks existence
mockMvc.perform(post("/user/resendRegistrationToken")
.contentType(MediaType.APPLICATION_JSON)
.content(objectMapper.writeValueAsString(testUserDto))
.with(csrf()))
- .andExpect(status().isInternalServerError())
- .andExpect(jsonPath("$.success").value(false))
- .andExpect(jsonPath("$.code").value(2));
+ .andExpect(status().isOk())
+ .andExpect(jsonPath("$.success").value(true))
+ .andExpect(jsonPath("$.code").value(0))
+ .andExpect(jsonPath("$.messages[0]")
+ .value("If your account requires verification, a new verification email has been sent."));
+
+ verify(userEmailService, never()).sendRegistrationVerificationEmail(any(), anyString());
}
}
diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/UserApiTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/UserApiTest.java
index f269140b..165ef7c5 100644
--- a/src/test/java/com/digitalsanctuary/spring/user/api/UserApiTest.java
+++ b/src/test/java/com/digitalsanctuary/spring/user/api/UserApiTest.java
@@ -215,26 +215,32 @@ void shouldRegisterNewUser() throws Exception {
.andExpect(status().isOk())
.andExpect(jsonPath("$.success").value(true))
.andExpect(jsonPath("$.code").value(0))
- .andExpect(jsonPath("$.messages[0]").value("Registration Successful!"));
+ .andExpect(jsonPath("$.messages[0]")
+ .value("If your email address is eligible, you will receive a verification email shortly."));
assertThat(userService.findUserByEmail(testEmail)).isNotNull();
}
@Test
- @DisplayName("Should return 409 Conflict when the user already exists")
- void shouldRejectExistingUser() throws Exception {
+ @DisplayName("Should return the same uniform 200 body for an existing email (anti-enumeration) and create no duplicate")
+ void shouldReturnUniformResponseForExistingUser() throws Exception {
// Register once.
userService.registerNewUserAccount(baseTestUser);
+ Long existingId = userService.findUserByEmail(testEmail).getId();
- // Register again with the same email.
+ // Register again with the same email - response must be indistinguishable from a new registration.
mockMvc.perform(post(URL + "/registration")
.with(csrf())
.contentType(MediaType.APPLICATION_JSON)
.content(json(baseTestUser)))
- .andExpect(status().isConflict())
- .andExpect(jsonPath("$.success").value(false))
- .andExpect(jsonPath("$.code").value(2))
- .andExpect(jsonPath("$.messages[0]").value("An account already exists for the email address"));
+ .andExpect(status().isOk())
+ .andExpect(jsonPath("$.success").value(true))
+ .andExpect(jsonPath("$.code").value(0))
+ .andExpect(jsonPath("$.messages[0]")
+ .value("If your email address is eligible, you will receive a verification email shortly."));
+
+ // No duplicate account was created - the existing account is untouched.
+ assertThat(userService.findUserByEmail(testEmail).getId()).isEqualTo(existingId);
}
@Test
From f863bd105642464bb6be35a00e0e9963d7de2d7f Mon Sep 17 00:00:00 2001
From: Devon Hillard
Date: Mon, 15 Jun 2026 09:58:05 -0600
Subject: [PATCH 05/23] fix(security): require current credential for
auth-method changes (remove/set password, passkey delete/rename)
---
CHANGELOG.md | 2 +
MIGRATION.md | 27 +++
.../spring/user/api/UserAPI.java | 14 ++
.../user/api/WebAuthnManagementAPI.java | 89 ++++++++-
...WebAuthnFeatureEnabledIntegrationTest.java | 60 ++++++-
.../user/api/WebAuthnManagementAPITest.java | 170 ++++++++++++++++--
6 files changed, 338 insertions(+), 24 deletions(-)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index a05cc5d4..d6a93976 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,11 +9,13 @@
### Fixes
- Registration and resend-verification endpoints no longer reveal whether an email is registered or already verified (account-enumeration hardening).
+- Credential-altering operations (remove password, delete/rename passkey) now require the current password when the account has one, preventing a session-only actor from changing authentication methods.
### Breaking Changes
- Reset/verification email links no longer trust `X-Forwarded-Host` by default. Deployments behind a reverse proxy must set `user.security.appUrl` or `user.security.trustedHosts` (see MIGRATION.md). `UserUtils.getAppUrl(HttpServletRequest)` is deprecated for removal.
- Added a UNIQUE, NOT NULL constraint on the `token` column of `password_reset_token` and `verification_token`. This is a schema/DDL change — see MIGRATION.md.
- The registration and resend-verification endpoints now always return HTTP 200 with a uniform generic body. Previously an existing email returned 409 on registration, and resend returned 409 (already verified) or 500 (unknown email). Clients that branched on those status codes or messages must update — the response is now intentionally uniform.
+- `removePassword` and passkey delete/rename now require a `currentPassword` for password-holding accounts; requests omitting it are rejected. Update clients to send the current password. See MIGRATION.md.
### Notes
- Audit-log injection (originally slated here as a JSON-per-line format change) was already resolved in 4.4.0 via field sanitization (CR/LF and `|` stripped). The breaking JSON-per-line conversion was intentionally **not** carried into 5.0.0, as it offered no additional security benefit.
diff --git a/MIGRATION.md b/MIGRATION.md
index f1b6cc0e..27d2dd6b 100644
--- a/MIGRATION.md
+++ b/MIGRATION.md
@@ -100,6 +100,33 @@ Internally the framework still does the correct thing — a brand-new registrati
**Action required:** Clients (web UIs, mobile apps, integrations) must no longer rely on the `409` status (existing/verified account) or the `500` status (unknown email on resend) to detect account existence or verification state. Branch only on the `success` flag for these two endpoints, and present the generic message to end users.
+### Re-authentication required for credential changes
+
+Operations that change *how an account authenticates* now require the user to prove knowledge of the current password **when the account has one**. This closes a gap where a session-only actor (e.g. an unattended or hijacked session) could silently alter the account's authentication methods without re-authenticating (CWE-620 / weak re-authentication).
+
+Affected endpoints (all require `user.webauthn.enabled=true` except where noted):
+
+| Endpoint | Method | What changed | How to send the current password |
+|---|---|---|---|
+| `/user/webauthn/password` | `DELETE` | Removing the password (converting to passkey-only) now requires the current password. | JSON body `{"currentPassword": "..."}` |
+| `/user/webauthn/credentials/{id}` | `DELETE` | Deleting a passkey requires the current password **when the account has a password**. | JSON body `{"currentPassword": "..."}` |
+| `/user/webauthn/credentials/{id}/label` | `PUT` | Renaming a passkey requires the current password **when the account has a password**. The existing body gains a `currentPassword` field. | JSON body `{"label": "...", "currentPassword": "..."}` |
+
+**Behavior when the account has a password:**
+- Missing `currentPassword` → `HTTP 400` with message *"Current password is required to change authentication methods."* — nothing is mutated.
+- Incorrect `currentPassword` → `HTTP 400` with message *"Current password is incorrect."* — nothing is mutated.
+- Correct `currentPassword` → the operation proceeds as before.
+
+`/user/updatePassword` is unchanged: it already required and verified `oldPassword`.
+
+**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:
+- 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` on a passwordless account also cannot require a current password (there is none); this endpoint still rejects accounts that already have a password.
+
+This is a deliberate, documented limitation rather than a half-measure: implementing a true WebAuthn step-up assertion would require significant new challenge/response infrastructure. Consuming applications that need stronger guarantees for passwordless accounts can front these endpoints with their own step-up (e.g. require a fresh passkey assertion) before allowing the call. This will be revisited if/when a step-up mechanism is added to the framework.
+
## Migrating to 4.0.x (Spring Boot 4.0)
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 df38ff5d..f2d0e2dc 100644
--- a/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java
+++ b/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java
@@ -475,6 +475,20 @@ public ResponseEntity registerPasswordlessAccount(@Valid @RequestB
/**
* Sets an initial password for a passwordless account.
*
+ *
+ * This endpoint only applies to passwordless (passkey-only) accounts and rejects the request if a password is already
+ * set (use {@code /user/updatePassword} to change an existing password, which requires the current password). Because
+ * the account has no current password to verify, this credential-altering operation cannot require re-authentication
+ * via a current password, and this library does not yet implement a WebAuthn step-up assertion.
+ *
+ *
+ *
+ * Residual risk: a session-only actor on a passwordless account could set an initial password. This is
+ * a known, documented limitation (see MIGRATION.md "Re-authentication required for credential changes"). It is not a
+ * regression and is bounded: the new password does not displace any existing credential, and consuming applications can
+ * front this endpoint with their own step-up if required.
+ *
+ *
* @param userDetails the authenticated user details
* @param setPasswordDto the set password DTO
* @param request the HTTP servlet request
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 306ff70d..9841d20d 100644
--- a/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java
+++ b/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java
@@ -6,6 +6,7 @@
import org.springframework.http.ResponseEntity;
import org.springframework.security.core.annotation.AuthenticationPrincipal;
import org.springframework.security.core.userdetails.UserDetails;
+import org.springframework.validation.annotation.Validated;
import org.springframework.web.bind.annotation.DeleteMapping;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
@@ -28,7 +29,6 @@
import jakarta.validation.constraints.Size;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
-import org.springframework.validation.annotation.Validated;
/**
* REST API for WebAuthn credential management.
@@ -46,7 +46,22 @@
* GET /user/webauthn/has-credentials - Check if user has any passkeys
* PUT /user/webauthn/credentials/{id}/label - Rename a passkey
* DELETE /user/webauthn/credentials/{id} - Delete a passkey
+ * DELETE /user/webauthn/password - Remove the account password (passkey-only)
*
+ *
+ *
+ * Re-authentication for credential-altering operations: Removing the password and deleting/renaming a
+ * passkey change the account's authentication methods. When the account has a password set, these operations require the
+ * caller to supply the current password ({@code currentPassword}), which is verified before any mutation. This prevents a
+ * session-only actor (e.g. via a hijacked or unattended session) from silently altering how the account authenticates.
+ *
+ *
+ *
+ * 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
+ * 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.
+ *
*/
@Slf4j
@RestController
@@ -107,6 +122,7 @@ public ResponseEntity renameCredential(@PathVariable @NotBlank
@RequestBody @Valid RenameCredentialRequest request,
@AuthenticationPrincipal UserDetails userDetails) {
User user = findAuthenticatedUser(userDetails);
+ requireCurrentPasswordIfSet(user, request.currentPassword());
credentialManagementService.renameCredential(id, request.label(), user);
return ResponseEntity.ok(new GenericResponse("Passkey renamed successfully"));
}
@@ -123,14 +139,24 @@ public ResponseEntity renameCredential(@PathVariable @NotBlank
* If this is the user's last passkey and they have no password, the deletion will be blocked with an error message.
*
*
+ *
+ * Re-authentication: If the account has a password set, the request body must include the current
+ * {@code currentPassword}, which is verified before the passkey is deleted. This prevents a session-only actor from
+ * altering the account's authentication methods. For passwordless (passkey-only) accounts there is no current
+ * credential to verify; see the class-level note on the residual risk for passwordless credential changes.
+ *
+ *
* @param id the credential ID to delete
+ * @param request the (optional) request body carrying the current password; may be {@code null} for passwordless accounts
* @param userDetails the authenticated user details
* @return ResponseEntity with success message or error
*/
@DeleteMapping("/credentials/{id}")
public ResponseEntity deleteCredential(@PathVariable @NotBlank @Size(max = 512) String id,
+ @RequestBody(required = false) CurrentPasswordRequest request,
@AuthenticationPrincipal UserDetails userDetails) {
User user = findAuthenticatedUser(userDetails);
+ requireCurrentPasswordIfSet(user, request != null ? request.currentPassword() : null);
credentialManagementService.deleteCredential(id, user);
return ResponseEntity.ok(new GenericResponse("Passkey deleted successfully"));
}
@@ -139,23 +165,37 @@ public ResponseEntity deleteCredential(@PathVariable @NotBlank
* Remove the user's password, making the account passwordless (passkey-only).
*
*
- * Requires the user to have at least one passkey registered. This ensures
- * the user can still authenticate after the password is removed.
+ * Requires the user to have at least one passkey registered. This ensures the user can still authenticate after the
+ * password is removed.
+ *
+ *
+ *
+ * Re-authentication: Because the account by definition has a password (it is being removed), the
+ * 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
+ * {@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.
*
*
+ * @param body the request body carrying the current password; technically optional at the HTTP layer so that a
+ * missing body produces a clear 400 rather than a framework-level error, but functionally required
* @param userDetails the authenticated user details
* @param request the HTTP servlet request
* @return ResponseEntity with success message or error
*/
@DeleteMapping("/password")
- public ResponseEntity removePassword(@AuthenticationPrincipal UserDetails userDetails,
- HttpServletRequest request) {
+ public ResponseEntity removePassword(@RequestBody(required = false) CurrentPasswordRequest body,
+ @AuthenticationPrincipal UserDetails userDetails, HttpServletRequest request) {
User user = findAuthenticatedUser(userDetails);
if (!userService.hasPassword(user)) {
throw new WebAuthnException("User does not have a password to remove");
}
+ requireCurrentPasswordIfSet(user, body != null ? body.currentPassword() : null);
+
if (!credentialManagementService.hasCredentials(user)) {
throw new WebAuthnException("Cannot remove password. Please register a passkey first.");
}
@@ -181,11 +221,48 @@ private User findAuthenticatedUser(UserDetails userDetails) throws WebAuthnUserN
return user;
}
+ /**
+ * Requires and verifies the current password for a credential-altering operation when the account has a password set.
+ *
+ *
+ * If the account has a password, the supplied {@code currentPassword} must be present and valid (verified via
+ * {@link UserService#checkIfValidOldPassword(User, String)}); otherwise a {@link WebAuthnException} (HTTP 400) is
+ * thrown before any mutation, preventing a session-only actor from changing the account's authentication
+ * methods. If the account is passwordless (passkey-only) there is no current credential to verify, so this check is
+ * a no-op — see the residual-risk note in MIGRATION.md.
+ *
+ *
+ * @param user the authenticated user
+ * @param currentPassword the current password supplied by the client (may be {@code null})
+ * @throws WebAuthnException if the account has a password and the supplied current password is missing or incorrect
+ */
+ private void requireCurrentPasswordIfSet(User user, String currentPassword) {
+ if (!userService.hasPassword(user)) {
+ // Passwordless (passkey-only) account: no current credential exists to verify. See MIGRATION.md residual-risk note.
+ return;
+ }
+ if (currentPassword == null || currentPassword.isBlank()) {
+ throw new WebAuthnException("Current password is required to change authentication methods.");
+ }
+ if (!userService.checkIfValidOldPassword(user, currentPassword)) {
+ throw new WebAuthnException("Current password is incorrect.");
+ }
+ }
+
/**
* Request DTO for renaming credential.
*
* @param label the new label (must not be blank, max 64 chars)
+ * @param currentPassword the current account password, required when the account has a password set (re-authentication)
+ */
+ public record RenameCredentialRequest(@NotBlank @Size(max = 64) String label, String currentPassword) {
+ }
+
+ /**
+ * Request DTO carrying the current account password for credential-altering operations that re-authenticate the user.
+ *
+ * @param currentPassword the current account password, required when the account has a password set
*/
- public record RenameCredentialRequest(@NotBlank @Size(max = 64) String label) {
+ public record CurrentPasswordRequest(String currentPassword) {
}
}
diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnFeatureEnabledIntegrationTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnFeatureEnabledIntegrationTest.java
index b1b76908..bfe33ec1 100644
--- a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnFeatureEnabledIntegrationTest.java
+++ b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnFeatureEnabledIntegrationTest.java
@@ -17,6 +17,7 @@
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.MediaType;
+import org.springframework.security.crypto.password.PasswordEncoder;
import org.springframework.test.context.TestPropertySource;
import org.springframework.test.web.servlet.MockMvc;
import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping;
@@ -36,6 +37,8 @@ class WebAuthnFeatureEnabledIntegrationTest {
private static final String TEST_EMAIL = "webauthn-user@test.com";
+ private static final String TEST_PASSWORD = "currentPassword1!";
+
@Autowired
private MockMvc mockMvc;
@@ -54,6 +57,9 @@ class WebAuthnFeatureEnabledIntegrationTest {
@Autowired
private WebAuthnCredentialManagementService credentialManagementService;
+ @Autowired
+ private PasswordEncoder passwordEncoder;
+
@BeforeEach
void setUp() {
webAuthnCredentialRepository.deleteAll();
@@ -65,7 +71,7 @@ void setUp() {
user.setFirstName("Web");
user.setLastName("Authn");
user.setEnabled(true);
- user.setPassword("encoded-password");
+ user.setPassword(passwordEncoder.encode(TEST_PASSWORD));
User savedUser = userRepository.saveAndFlush(user);
WebAuthnUserEntity userEntity = new WebAuthnUserEntity();
@@ -123,8 +129,56 @@ void shouldReturnValidationResponseForOverlongLabel() throws Exception {
@Test
@DisplayName("should return business error response when service throws WebAuthnException")
void shouldReturnBusinessErrorWhenServiceFails() throws Exception {
- mockMvc.perform(delete("/user/webauthn/credentials/does-not-exist").with(user(TEST_EMAIL).roles("USER")).with(csrf()))
- .andExpect(status().isBadRequest())
+ String payload = "{\"currentPassword\":\"" + TEST_PASSWORD + "\"}";
+ mockMvc.perform(delete("/user/webauthn/credentials/does-not-exist").with(user(TEST_EMAIL).roles("USER")).with(csrf())
+ .contentType(MediaType.APPLICATION_JSON).content(payload)).andExpect(status().isBadRequest())
.andExpect(jsonPath("$.message").value("Credential not found or access denied"));
}
+
+ @Test
+ @DisplayName("should reject passkey deletion when account has password and current password missing")
+ void shouldRejectDeleteWhenCurrentPasswordMissingForPasswordAccount() throws Exception {
+ mockMvc.perform(delete("/user/webauthn/credentials/cred-1").with(user(TEST_EMAIL).roles("USER")).with(csrf()))
+ .andExpect(status().isBadRequest())
+ .andExpect(jsonPath("$.message", containsString("Current password is required")));
+
+ assertThat(webAuthnCredentialRepository.findByIdWithUser("cred-1")).isPresent();
+ }
+
+ @Test
+ @DisplayName("should reject passkey deletion when account has password and current password incorrect")
+ void shouldRejectDeleteWhenCurrentPasswordIncorrectForPasswordAccount() throws Exception {
+ String payload = "{\"currentPassword\":\"wrongPassword!\"}";
+ mockMvc.perform(delete("/user/webauthn/credentials/cred-1").with(user(TEST_EMAIL).roles("USER")).with(csrf())
+ .contentType(MediaType.APPLICATION_JSON).content(payload)).andExpect(status().isBadRequest())
+ .andExpect(jsonPath("$.message", containsString("Current password is incorrect")));
+
+ assertThat(webAuthnCredentialRepository.findByIdWithUser("cred-1")).isPresent();
+ }
+
+ @Test
+ @DisplayName("should reject passkey rename when account has password and current password missing")
+ void shouldRejectRenameWhenCurrentPasswordMissingForPasswordAccount() throws Exception {
+ String payload = "{\"label\":\"New Label\"}";
+ mockMvc.perform(put("/user/webauthn/credentials/cred-1/label").with(user(TEST_EMAIL).roles("USER")).with(csrf())
+ .contentType(MediaType.APPLICATION_JSON).content(payload)).andExpect(status().isBadRequest())
+ .andExpect(jsonPath("$.message", containsString("Current password is required")));
+
+ assertThat(webAuthnCredentialRepository.findByIdWithUser("cred-1"))
+ .isPresent()
+ .hasValueSatisfying(c -> assertThat(c.getLabel()).isEqualTo("My Device"));
+ }
+
+ @Test
+ @DisplayName("should reject passkey rename when account has password and current password incorrect")
+ void shouldRejectRenameWhenCurrentPasswordIncorrectForPasswordAccount() throws Exception {
+ String payload = "{\"label\":\"New Label\",\"currentPassword\":\"wrongPassword!\"}";
+ mockMvc.perform(put("/user/webauthn/credentials/cred-1/label").with(user(TEST_EMAIL).roles("USER")).with(csrf())
+ .contentType(MediaType.APPLICATION_JSON).content(payload)).andExpect(status().isBadRequest())
+ .andExpect(jsonPath("$.message", containsString("Current password is incorrect")));
+
+ assertThat(webAuthnCredentialRepository.findByIdWithUser("cred-1"))
+ .isPresent()
+ .hasValueSatisfying(c -> assertThat(c.getLabel()).isEqualTo("My Device"));
+ }
}
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 ae1b1dc1..b7e8ee9d 100644
--- a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java
+++ b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java
@@ -161,10 +161,12 @@ void shouldThrowNotFoundWhenUserNotFound() {
class RenameCredentialTests {
@Test
- @DisplayName("should rename credential successfully")
- void shouldRenameSuccessfully() {
+ @DisplayName("should rename credential successfully when account is passwordless")
+ void shouldRenameSuccessfullyWhenAccountPasswordless() {
// Given
- WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop");
+ testUser.setPassword(null);
+ when(userService.hasPassword(testUser)).thenReturn(false);
+ WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop", null);
// When
ResponseEntity response = api.renameCredential("cred-1", request, userDetails);
@@ -175,11 +177,61 @@ void shouldRenameSuccessfully() {
verify(credentialManagementService).renameCredential("cred-1", "Work Laptop", testUser);
}
+ @Test
+ @DisplayName("should rename credential successfully when correct current password provided")
+ void shouldRenameSuccessfullyWhenCorrectCurrentPassword() {
+ // Given
+ testUser.setPassword("encodedPassword");
+ when(userService.hasPassword(testUser)).thenReturn(true);
+ when(userService.checkIfValidOldPassword(testUser, "currentPass")).thenReturn(true);
+ WebAuthnManagementAPI.RenameCredentialRequest request =
+ new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop", "currentPass");
+
+ // When
+ ResponseEntity response = api.renameCredential("cred-1", request, userDetails);
+
+ // Then
+ assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK);
+ verify(credentialManagementService).renameCredential("cred-1", "Work Laptop", testUser);
+ }
+
+ @Test
+ @DisplayName("should reject rename when account has password and current password missing")
+ void shouldRejectRenameWhenCurrentPasswordMissing() {
+ // Given
+ testUser.setPassword("encodedPassword");
+ when(userService.hasPassword(testUser)).thenReturn(true);
+ WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop", null);
+
+ // When & Then
+ assertThatThrownBy(() -> api.renameCredential("cred-1", request, userDetails)).isInstanceOf(WebAuthnException.class)
+ .hasMessageContaining("Current password is required");
+ verify(credentialManagementService, never()).renameCredential(any(), any(), any());
+ }
+
+ @Test
+ @DisplayName("should reject rename when account has password and current password incorrect")
+ void shouldRejectRenameWhenCurrentPasswordIncorrect() {
+ // Given
+ testUser.setPassword("encodedPassword");
+ when(userService.hasPassword(testUser)).thenReturn(true);
+ when(userService.checkIfValidOldPassword(testUser, "wrongPass")).thenReturn(false);
+ WebAuthnManagementAPI.RenameCredentialRequest request =
+ new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop", "wrongPass");
+
+ // When & Then
+ assertThatThrownBy(() -> api.renameCredential("cred-1", request, userDetails)).isInstanceOf(WebAuthnException.class)
+ .hasMessageContaining("Current password is incorrect");
+ verify(credentialManagementService, never()).renameCredential(any(), any(), any());
+ }
+
@Test
@DisplayName("should throw when rename fails")
void shouldThrowOnFailure() {
// Given
- WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("New Name");
+ testUser.setPassword(null);
+ when(userService.hasPassword(testUser)).thenReturn(false);
+ WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("New Name", null);
doThrow(new WebAuthnException("Credential not found or access denied")).when(credentialManagementService)
.renameCredential(eq("cred-999"), eq("New Name"), any(User.class));
@@ -192,7 +244,7 @@ void shouldThrowOnFailure() {
@DisplayName("should throw not found when user not found")
void shouldThrowNotFoundWhenUserNotFound() {
// Given
- WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("New Name");
+ WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("New Name", null);
when(userService.findUserByEmail(testUser.getEmail())).thenReturn(null);
// When
@@ -207,10 +259,14 @@ void shouldThrowNotFoundWhenUserNotFound() {
class DeleteCredentialTests {
@Test
- @DisplayName("should delete credential successfully")
- void shouldDeleteSuccessfully() {
+ @DisplayName("should delete credential successfully when account is passwordless")
+ void shouldDeleteSuccessfullyWhenAccountPasswordless() {
+ // Given
+ testUser.setPassword(null);
+ when(userService.hasPassword(testUser)).thenReturn(false);
+
// When
- ResponseEntity response = api.deleteCredential("cred-1", userDetails);
+ ResponseEntity response = api.deleteCredential("cred-1", null, userDetails);
// Then
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK);
@@ -218,15 +274,62 @@ void shouldDeleteSuccessfully() {
verify(credentialManagementService).deleteCredential("cred-1", testUser);
}
+ @Test
+ @DisplayName("should delete credential successfully when correct current password provided")
+ void shouldDeleteSuccessfullyWhenCorrectCurrentPassword() {
+ // Given
+ testUser.setPassword("encodedPassword");
+ when(userService.hasPassword(testUser)).thenReturn(true);
+ when(userService.checkIfValidOldPassword(testUser, "currentPass")).thenReturn(true);
+ WebAuthnManagementAPI.CurrentPasswordRequest request = new WebAuthnManagementAPI.CurrentPasswordRequest("currentPass");
+
+ // When
+ ResponseEntity response = api.deleteCredential("cred-1", request, userDetails);
+
+ // Then
+ assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK);
+ verify(credentialManagementService).deleteCredential("cred-1", testUser);
+ }
+
+ @Test
+ @DisplayName("should reject delete when account has password and current password missing")
+ void shouldRejectDeleteWhenCurrentPasswordMissing() {
+ // Given
+ testUser.setPassword("encodedPassword");
+ when(userService.hasPassword(testUser)).thenReturn(true);
+
+ // When & Then
+ assertThatThrownBy(() -> api.deleteCredential("cred-1", null, userDetails)).isInstanceOf(WebAuthnException.class)
+ .hasMessageContaining("Current password is required");
+ verify(credentialManagementService, never()).deleteCredential(any(), any());
+ }
+
+ @Test
+ @DisplayName("should reject delete when account has password and current password incorrect")
+ void shouldRejectDeleteWhenCurrentPasswordIncorrect() {
+ // Given
+ testUser.setPassword("encodedPassword");
+ when(userService.hasPassword(testUser)).thenReturn(true);
+ when(userService.checkIfValidOldPassword(testUser, "wrongPass")).thenReturn(false);
+ WebAuthnManagementAPI.CurrentPasswordRequest request = new WebAuthnManagementAPI.CurrentPasswordRequest("wrongPass");
+
+ // When & Then
+ assertThatThrownBy(() -> api.deleteCredential("cred-1", request, userDetails)).isInstanceOf(WebAuthnException.class)
+ .hasMessageContaining("Current password is incorrect");
+ verify(credentialManagementService, never()).deleteCredential(any(), any());
+ }
+
@Test
@DisplayName("should throw when delete fails")
void shouldThrowOnFailure() {
// Given
+ testUser.setPassword(null);
+ when(userService.hasPassword(testUser)).thenReturn(false);
doThrow(new WebAuthnException("Cannot delete last passkey")).when(credentialManagementService).deleteCredential(eq("cred-1"),
any(User.class));
// When
- assertThatThrownBy(() -> api.deleteCredential("cred-1", userDetails)).isInstanceOf(WebAuthnException.class)
+ assertThatThrownBy(() -> api.deleteCredential("cred-1", null, userDetails)).isInstanceOf(WebAuthnException.class)
.hasMessageContaining("Cannot delete last passkey");
}
@@ -237,7 +340,7 @@ void shouldThrowNotFoundWhenUserNotFound() {
when(userService.findUserByEmail(testUser.getEmail())).thenReturn(null);
// When
- assertThatThrownBy(() -> api.deleteCredential("cred-1", userDetails))
+ assertThatThrownBy(() -> api.deleteCredential("cred-1", null, userDetails))
.isInstanceOf(WebAuthnUserNotFoundException.class).hasMessageContaining("User not found");
verify(credentialManagementService, never()).deleteCredential(any(), any());
}
@@ -256,16 +359,18 @@ private HttpServletRequest createMockRequest() {
}
@Test
- @DisplayName("should remove password when user has passkeys")
- void shouldRemovePasswordWhenUserHasPasskeys() {
+ @DisplayName("should remove password when user has passkeys and correct current password")
+ void shouldRemovePasswordWhenUserHasPasskeysAndCorrectCurrentPassword() {
// Given
HttpServletRequest mockRequest = createMockRequest();
testUser.setPassword("encodedPassword");
when(userService.hasPassword(testUser)).thenReturn(true);
+ when(userService.checkIfValidOldPassword(testUser, "currentPass")).thenReturn(true);
when(credentialManagementService.hasCredentials(testUser)).thenReturn(true);
+ WebAuthnManagementAPI.CurrentPasswordRequest request = new WebAuthnManagementAPI.CurrentPasswordRequest("currentPass");
// When
- ResponseEntity response = api.removePassword(userDetails, mockRequest);
+ ResponseEntity response = api.removePassword(request, userDetails, mockRequest);
// Then
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK);
@@ -274,6 +379,38 @@ void shouldRemovePasswordWhenUserHasPasskeys() {
verify(eventPublisher).publishEvent(any(AuditEvent.class));
}
+ @Test
+ @DisplayName("should reject removal when current password missing")
+ void shouldRejectRemovalWhenCurrentPasswordMissing() {
+ // Given
+ HttpServletRequest mockRequest = mock(HttpServletRequest.class);
+ testUser.setPassword("encodedPassword");
+ when(userService.hasPassword(testUser)).thenReturn(true);
+
+ // When & Then
+ assertThatThrownBy(() -> api.removePassword(null, userDetails, mockRequest))
+ .isInstanceOf(WebAuthnException.class)
+ .hasMessageContaining("Current password is required");
+ verify(userService, never()).removeUserPassword(any());
+ }
+
+ @Test
+ @DisplayName("should reject removal when current password incorrect")
+ void shouldRejectRemovalWhenCurrentPasswordIncorrect() {
+ // Given
+ HttpServletRequest mockRequest = mock(HttpServletRequest.class);
+ testUser.setPassword("encodedPassword");
+ when(userService.hasPassword(testUser)).thenReturn(true);
+ when(userService.checkIfValidOldPassword(testUser, "wrongPass")).thenReturn(false);
+ WebAuthnManagementAPI.CurrentPasswordRequest request = new WebAuthnManagementAPI.CurrentPasswordRequest("wrongPass");
+
+ // When & Then
+ assertThatThrownBy(() -> api.removePassword(request, userDetails, mockRequest))
+ .isInstanceOf(WebAuthnException.class)
+ .hasMessageContaining("Current password is incorrect");
+ verify(userService, never()).removeUserPassword(any());
+ }
+
@Test
@DisplayName("should reject removal when no passkeys")
void shouldRejectRemovalWhenNoPasskeys() {
@@ -281,10 +418,12 @@ void shouldRejectRemovalWhenNoPasskeys() {
HttpServletRequest mockRequest = mock(HttpServletRequest.class);
testUser.setPassword("encodedPassword");
when(userService.hasPassword(testUser)).thenReturn(true);
+ when(userService.checkIfValidOldPassword(testUser, "currentPass")).thenReturn(true);
when(credentialManagementService.hasCredentials(testUser)).thenReturn(false);
+ WebAuthnManagementAPI.CurrentPasswordRequest request = new WebAuthnManagementAPI.CurrentPasswordRequest("currentPass");
// When & Then
- assertThatThrownBy(() -> api.removePassword(userDetails, mockRequest))
+ assertThatThrownBy(() -> api.removePassword(request, userDetails, mockRequest))
.isInstanceOf(WebAuthnException.class)
.hasMessageContaining("register a passkey first");
verify(userService, never()).removeUserPassword(any());
@@ -297,9 +436,10 @@ void shouldRejectRemovalWhenAlreadyPasswordless() {
HttpServletRequest mockRequest = mock(HttpServletRequest.class);
testUser.setPassword(null);
when(userService.hasPassword(testUser)).thenReturn(false);
+ WebAuthnManagementAPI.CurrentPasswordRequest request = new WebAuthnManagementAPI.CurrentPasswordRequest("anything");
// When & Then
- assertThatThrownBy(() -> api.removePassword(userDetails, mockRequest))
+ assertThatThrownBy(() -> api.removePassword(request, userDetails, mockRequest))
.isInstanceOf(WebAuthnException.class)
.hasMessageContaining("does not have a password");
verify(userService, never()).removeUserPassword(any());
From 5abefa8fae8127daa6bf1652da3db05f98d4d1d4 Mon Sep 17 00:00:00 2001
From: Devon Hillard
Date: Mon, 15 Jun 2026 10:17:37 -0600
Subject: [PATCH 06/23] fix(concurrency): unique role/privilege names +
idempotent setup for multi-node startup
---
CHANGELOG.md | 2 +
MIGRATION.md | 23 ++
db-scripts/mariadb-schema.sql | 16 +-
.../user/persistence/model/Privilege.java | 2 +
.../spring/user/persistence/model/Role.java | 2 +
.../user/roles/RolePrivilegeSetupService.java | 198 +++++++++--
.../SelfProxiedMethodVisibilityTest.java | 12 +-
.../model/RolePrivilegeUniquenessTest.java | 49 +++
.../roles/RolePrivilegeSetupServiceTest.java | 327 +++++++++++-------
.../service/UserUpdateIntegrationTest.java | 19 +-
10 files changed, 481 insertions(+), 169 deletions(-)
create mode 100644 src/test/java/com/digitalsanctuary/spring/user/persistence/model/RolePrivilegeUniquenessTest.java
diff --git a/CHANGELOG.md b/CHANGELOG.md
index d6a93976..2b53ec0f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -10,10 +10,12 @@
### Fixes
- Registration and resend-verification endpoints no longer reveal whether an email is registered or already verified (account-enumeration hardening).
- Credential-altering operations (remove password, delete/rename passkey) now require the current password when the account has one, preventing a session-only actor from changing authentication methods.
+- Role/privilege startup setup is now idempotent and safe under concurrent multi-node startup (handles unique-constraint races by re-reading the existing row).
### Breaking Changes
- Reset/verification email links no longer trust `X-Forwarded-Host` by default. Deployments behind a reverse proxy must set `user.security.appUrl` or `user.security.trustedHosts` (see MIGRATION.md). `UserUtils.getAppUrl(HttpServletRequest)` is deprecated for removal.
- Added a UNIQUE, NOT NULL constraint on the `token` column of `password_reset_token` and `verification_token`. This is a schema/DDL change — see MIGRATION.md.
+- Added UNIQUE, NOT NULL constraints on `role.name` and `privilege.name` (schema/DDL change). See MIGRATION.md.
- The registration and resend-verification endpoints now always return HTTP 200 with a uniform generic body. Previously an existing email returned 409 on registration, and resend returned 409 (already verified) or 500 (unknown email). Clients that branched on those status codes or messages must update — the response is now intentionally uniform.
- `removePassword` and passkey delete/rename now require a `currentPassword` for password-holding accounts; requests omitting it are rejected. Update clients to send the current password. See MIGRATION.md.
diff --git a/MIGRATION.md b/MIGRATION.md
index 27d2dd6b..21b01698 100644
--- a/MIGRATION.md
+++ b/MIGRATION.md
@@ -127,6 +127,29 @@ Affected endpoints (all require `user.webauthn.enabled=true` except where noted)
This is a deliberate, documented limitation rather than a half-measure: implementing a true WebAuthn step-up assertion would require significant new challenge/response infrastructure. Consuming applications that need stronger guarantees for passwordless accounts can front these endpoints with their own step-up (e.g. require a fresh passkey assertion) before allowing the call. This will be revisited if/when a step-up mechanism is added to the framework.
+### Database schema: unique role/privilege names
+
+The `name` column on both the `role` and `privilege` tables now carries a **UNIQUE + NOT NULL** constraint. Role and privilege names were always intended to be unique identifiers (the framework looks them up by name), so this enforces an existing invariant at the schema level. It also makes the startup role/privilege setup safe under concurrent multi-node startup: if two nodes start simultaneously and both try to create the same role/privilege, the unique constraint guarantees only one row is created, and the framework re-reads the winning row instead of failing (first-writer-wins).
+
+**Applications using `spring.jpa.hibernate.ddl-auto=update` (or `create`/`create-drop`):** Hibernate will add the unique index automatically on startup — no manual action required.
+
+**Applications managing schema manually (Flyway, Liquibase, or `ddl-auto=validate`/`none`):** **de-duplicate any existing duplicate role/privilege names first**, then apply the following DDL before upgrading and before starting the application:
+
+```sql
+-- 1. Find duplicates before applying the constraint (must return zero rows):
+SELECT name, COUNT(*) FROM role GROUP BY name HAVING COUNT(*) > 1;
+SELECT name, COUNT(*) FROM privilege GROUP BY name HAVING COUNT(*) > 1;
+-- Resolve any duplicates manually (merge/repoint references, delete extras) before continuing.
+
+-- 2. Apply NOT NULL + UNIQUE:
+ALTER TABLE role ALTER COLUMN name SET NOT NULL;
+ALTER TABLE privilege ALTER COLUMN name SET NOT NULL;
+CREATE UNIQUE INDEX ux_role_name ON role (name);
+CREATE UNIQUE INDEX ux_privilege_name ON privilege (name);
+```
+
+> **Note:** The table names above (`role`, `privilege`) and column name (`name`) are Hibernate's defaults for the `Role` and `Privilege` entities (no `@Table` override). If you have customized Hibernate's physical naming strategy, adjust the identifiers accordingly. The DDL syntax is standard SQL (PostgreSQL / MariaDB / MySQL); for MySQL/MariaDB, `ALTER COLUMN name SET NOT NULL` may need the full column definition, e.g. `MODIFY COLUMN name VARCHAR(255) NOT NULL`.
+
## Migrating to 4.0.x (Spring Boot 4.0)
diff --git a/db-scripts/mariadb-schema.sql b/db-scripts/mariadb-schema.sql
index 5b8f4be2..21717190 100644
--- a/db-scripts/mariadb-schema.sql
+++ b/db-scripts/mariadb-schema.sql
@@ -21,9 +21,10 @@ DROP TABLE IF EXISTS `password_reset_token`;
CREATE TABLE `password_reset_token` (
`id` BIGINT(20) NOT NULL,
`expiry_date` DATETIME(6) DEFAULT NULL,
- `token` VARCHAR(255) DEFAULT NULL,
+ `token` VARCHAR(255) NOT NULL,
`user_id` BIGINT(20) NOT NULL,
PRIMARY KEY (`id`),
+ UNIQUE KEY `ux_password_reset_token_token` (`token`),
KEY `FKns9q9f0f318uaoxiqn6lka9ux` (`user_id`),
CONSTRAINT `FKns9q9f0f318uaoxiqn6lka9ux` FOREIGN KEY (`user_id`) REFERENCES `user_account` (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci;
@@ -32,16 +33,18 @@ DROP TABLE IF EXISTS `privilege`;
CREATE TABLE `privilege` (
`id` BIGINT(20) NOT NULL,
`description` VARCHAR(255) DEFAULT NULL,
- `name` VARCHAR(255) DEFAULT NULL,
- PRIMARY KEY (`id`)
+ `name` VARCHAR(255) NOT NULL,
+ PRIMARY KEY (`id`),
+ UNIQUE KEY `ux_privilege_name` (`name`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci;
DROP TABLE IF EXISTS `role`;
CREATE TABLE `role` (
`id` BIGINT(20) NOT NULL,
`description` VARCHAR(255) DEFAULT NULL,
- `name` VARCHAR(255) DEFAULT NULL,
- PRIMARY KEY (`id`)
+ `name` VARCHAR(255) NOT NULL,
+ PRIMARY KEY (`id`),
+ UNIQUE KEY `ux_role_name` (`name`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci;
DROP TABLE IF EXISTS `roles_privileges`;
@@ -86,9 +89,10 @@ DROP TABLE IF EXISTS `verification_token`;
CREATE TABLE `verification_token` (
`id` BIGINT(20) NOT NULL,
`expiry_date` DATETIME(6) DEFAULT NULL,
- `token` VARCHAR(255) DEFAULT NULL,
+ `token` VARCHAR(255) NOT NULL,
`user_id` BIGINT(20) NOT NULL,
PRIMARY KEY (`id`),
+ UNIQUE KEY `ux_verification_token_token` (`token`),
KEY `FK_VERIFY_USER` (`user_id`),
CONSTRAINT `FK_VERIFY_USER` FOREIGN KEY (`user_id`) REFERENCES `user_account` (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci;
diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Privilege.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Privilege.java
index 9c99ab7e..95dc541e 100644
--- a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Privilege.java
+++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Privilege.java
@@ -2,6 +2,7 @@
import java.io.Serializable;
import java.util.Collection;
+import jakarta.persistence.Column;
import jakarta.persistence.Entity;
import jakarta.persistence.GeneratedValue;
import jakarta.persistence.GenerationType;
@@ -32,6 +33,7 @@ public class Privilege implements Serializable {
private Long id;
/** The name. */
+ @Column(unique = true, nullable = false)
private String name;
/** The description of the role. */
diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Role.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Role.java
index 516414e0..3fd8c264 100644
--- a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Role.java
+++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Role.java
@@ -4,6 +4,7 @@
import java.util.HashSet;
import java.util.Set;
import jakarta.persistence.CascadeType;
+import jakarta.persistence.Column;
import jakarta.persistence.Entity;
import jakarta.persistence.FetchType;
import jakarta.persistence.GeneratedValue;
@@ -49,6 +50,7 @@ public class Role implements Serializable {
private Set privileges = new HashSet<>();
/** The name. */
+ @Column(unique = true, nullable = false)
private String name;
private String description;
diff --git a/src/main/java/com/digitalsanctuary/spring/user/roles/RolePrivilegeSetupService.java b/src/main/java/com/digitalsanctuary/spring/user/roles/RolePrivilegeSetupService.java
index 951348df..6b52daba 100644
--- a/src/main/java/com/digitalsanctuary/spring/user/roles/RolePrivilegeSetupService.java
+++ b/src/main/java/com/digitalsanctuary/spring/user/roles/RolePrivilegeSetupService.java
@@ -4,33 +4,45 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
+import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.ApplicationListener;
+import org.springframework.context.annotation.Lazy;
import org.springframework.context.event.ContextRefreshedEvent;
+import org.springframework.dao.DataIntegrityViolationException;
import org.springframework.stereotype.Component;
+import org.springframework.transaction.annotation.Propagation;
+import org.springframework.transaction.annotation.Transactional;
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 jakarta.transaction.Transactional;
-import lombok.Data;
-import lombok.RequiredArgsConstructor;
+import lombok.Getter;
+import lombok.Setter;
import lombok.extern.slf4j.Slf4j;
/**
* Service that initializes roles and privileges from configuration on application startup.
*
- * Listens for {@link ContextRefreshedEvent} and creates or updates roles and privileges
- * in the database based on the {@link RolesAndPrivilegesConfig} settings. This ensures
- * that the configured role/privilege structure exists before the application handles requests.
+ * Listens for {@link ContextRefreshedEvent} and creates or updates roles and privileges in the database based on the
+ * {@link RolesAndPrivilegesConfig} settings. This ensures that the configured role/privilege structure exists before
+ * the application handles requests.
+ *
+ *
+ * The setup is idempotent and safe under concurrent multi-node startup. Because {@code role.name} and
+ * {@code privilege.name} carry a UNIQUE constraint, a second node inserting the same name will fail with a
+ * {@link DataIntegrityViolationException}. Each insert runs in its own {@link Propagation#REQUIRES_NEW} transaction
+ * (routed through the Spring proxy via the {@link #self} reference) so that a failed insert rolls back only that inner
+ * transaction without poisoning the surrounding flow; the catch handler then re-reads the row the winning node created,
+ * giving first-writer-wins convergence.
*
*/
@Slf4j
-@Data
-@RequiredArgsConstructor
+@Getter
@Component
public class RolePrivilegeSetupService implements ApplicationListener {
/** The already setup flag. */
+ @Setter
private boolean alreadySetup = false;
/** The roles and privileges configuration. */
@@ -42,13 +54,42 @@ public class RolePrivilegeSetupService implements ApplicationListener
+ * Intentionally not {@code @Transactional}: each {@code getOrCreate} call manages its own
+ * transaction boundary so that a unique-constraint violation on one insert cannot mark a shared transaction
+ * rollback-only and poison the subsequent re-read.
+ *
*
* @param event the context refreshed event
*/
@Override
- @Transactional
public void onApplicationEvent(final ContextRefreshedEvent event) {
if (alreadySetup) {
return;
@@ -60,47 +101,142 @@ public void onApplicationEvent(final ContextRefreshedEvent event) {
final String roleName = entry.getKey();
final List privileges = entry.getValue();
if (roleName != null && privileges != null) {
- final Set privilegeSet = new HashSet<>();
for (String privilegeName : privileges) {
- privilegeSet.add(getOrCreatePrivilege(privilegeName));
+ getOrCreatePrivilege(privilegeName);
}
- getOrCreateRole(roleName, privilegeSet);
+ getOrCreateRole(roleName, new HashSet<>(privileges));
}
}
alreadySetup = true;
}
/**
- * Retrieves or creates a privilege by name.
+ * Retrieves or creates a privilege by name, tolerating concurrent creation by another node.
+ *
+ * If the privilege does not yet exist it is inserted in its own {@link Propagation#REQUIRES_NEW} transaction. If a
+ * concurrent node has already inserted a privilege with the same name, the insert fails with a
+ * {@link DataIntegrityViolationException}; that inner transaction is rolled back and the row created by the winning
+ * node is re-read and returned. The exception is never propagated to the caller.
+ *
*
* @param name the name of the privilege
- * @return the privilege
+ * @return the existing or newly created privilege
*/
- @Transactional
- Privilege getOrCreatePrivilege(final String name) {
+ public Privilege getOrCreatePrivilege(final String name) {
Privilege privilege = privilegeRepository.findByName(name);
- if (privilege == null) {
- privilege = new Privilege(name);
- privilege = privilegeRepository.save(privilege);
+ if (privilege != null) {
+ return privilege;
+ }
+ try {
+ return self.insertPrivilege(name);
+ } catch (final DataIntegrityViolationException e) {
+ log.debug("Privilege '{}' was created concurrently; re-reading existing row.", name);
+ final Privilege existing = privilegeRepository.findByName(name);
+ if (existing == null) {
+ throw e;
+ }
+ return existing;
+ }
+ }
+
+ /**
+ * Inserts a new privilege in its own transaction. Must be {@code public} so the Spring proxy can advise it when
+ * invoked through {@link #self}; a package-private/private target is not overridden by the CGLIB proxy subclass and
+ * the {@link Propagation#REQUIRES_NEW} boundary would not take effect.
+ *
+ * @param name the name of the privilege
+ * @return the persisted privilege
+ */
+ @Transactional(propagation = Propagation.REQUIRES_NEW)
+ public Privilege insertPrivilege(final String name) {
+ return privilegeRepository.saveAndFlush(new Privilege(name));
+ }
+
+ /**
+ * Retrieves or creates a role by name and (re)assigns its privileges, tolerating concurrent creation by another
+ * node.
+ *
+ * If the role does not yet exist it is inserted in its own {@link Propagation#REQUIRES_NEW} transaction. If a
+ * concurrent node has already inserted a role with the same name, the insert fails with a
+ * {@link DataIntegrityViolationException}; that inner transaction is rolled back and the existing row is re-read,
+ * has its privileges (re)assigned, and is saved. The exception is never propagated to the caller.
+ *
+ *
+ * @param name the name of the role
+ * @param privilegeNames the names of the privileges associated with the role (resolved to managed entities inside
+ * the insert/update transaction)
+ * @return the existing or newly created role
+ */
+ public Role getOrCreateRole(final String name, final Set privilegeNames) {
+ final Role existing = roleRepository.findByName(name);
+ if (existing != null) {
+ return self.updateRolePrivileges(existing.getId(), privilegeNames);
+ }
+ try {
+ return self.insertRole(name, privilegeNames);
+ } catch (final DataIntegrityViolationException e) {
+ log.debug("Role '{}' was created concurrently; re-reading existing row.", name);
+ final Role concurrent = roleRepository.findByName(name);
+ if (concurrent == null) {
+ throw e;
+ }
+ return self.updateRolePrivileges(concurrent.getId(), privilegeNames);
}
- return privilege;
}
/**
- * Retrieves or creates a role by name and privileges.
+ * Inserts a new role with the given privileges in its own transaction. The privileges are resolved to managed
+ * entities by name within this transaction so the role's cascade does not attempt to persist detached instances.
+ * Must be {@code public} so the Spring proxy can advise it when invoked through {@link #self} (see
+ * {@link #insertPrivilege(String)}).
*
* @param name the name of the role
- * @param privileges the set of privileges associated with the role
- * @return the role
+ * @param privilegeNames the names of the privileges to assign
+ * @return the persisted role
*/
- @Transactional
- Role getOrCreateRole(final String name, final Set privileges) {
- Role role = roleRepository.findByName(name);
- if (role == null) {
- role = new Role(name);
+ @Transactional(propagation = Propagation.REQUIRES_NEW)
+ public Role insertRole(final String name, final Set privilegeNames) {
+ final Role role = new Role(name);
+ role.setPrivileges(resolvePrivileges(privilegeNames));
+ return roleRepository.saveAndFlush(role);
+ }
+
+ /**
+ * Re-reads an existing role by id and (re)assigns its privileges in its own transaction. The privileges are
+ * resolved to managed entities by name within this transaction. Must be {@code public} so the Spring proxy can
+ * advise it when invoked through {@link #self} (see {@link #insertPrivilege(String)}).
+ *
+ * @param roleId the id of the role to update
+ * @param privilegeNames the names of the privileges to assign
+ * @return the updated role
+ */
+ @Transactional(propagation = Propagation.REQUIRES_NEW)
+ public Role updateRolePrivileges(final Long roleId, final Set privilegeNames) {
+ final Role role = roleRepository.findById(roleId).orElseThrow();
+ role.setPrivileges(resolvePrivileges(privilegeNames));
+ return roleRepository.saveAndFlush(role);
+ }
+
+ /**
+ * Resolves a set of privilege names to managed {@link Privilege} entities loaded in the current transaction. Names
+ * with no matching row are skipped (they were already created by {@link #getOrCreatePrivilege(String)} earlier in
+ * the setup flow, so a miss here only happens transiently and is harmless).
+ *
+ * @param privilegeNames the privilege names to resolve
+ * @return the managed privileges
+ */
+ private Set resolvePrivileges(final Set privilegeNames) {
+ final Set privileges = new HashSet<>();
+ for (final String privilegeName : privilegeNames) {
+ final Privilege privilege = privilegeRepository.findByName(privilegeName);
+ if (privilege != null) {
+ privileges.add(privilege);
+ } else {
+ log.warn("Configured privilege '{}' was not found and will be absent from the role's privilege set. "
+ + "This is usually a typo in the user.roles-and-privileges configuration, or a transient miss "
+ + "during concurrent multi-node startup.", privilegeName);
+ }
}
- role.setPrivileges(privileges);
- role = roleRepository.save(role);
- return role;
+ return privileges;
}
}
diff --git a/src/test/java/com/digitalsanctuary/spring/user/architecture/SelfProxiedMethodVisibilityTest.java b/src/test/java/com/digitalsanctuary/spring/user/architecture/SelfProxiedMethodVisibilityTest.java
index 14478290..ba7f19a7 100644
--- a/src/test/java/com/digitalsanctuary/spring/user/architecture/SelfProxiedMethodVisibilityTest.java
+++ b/src/test/java/com/digitalsanctuary/spring/user/architecture/SelfProxiedMethodVisibilityTest.java
@@ -10,6 +10,7 @@
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
import com.digitalsanctuary.spring.user.gdpr.GdprDeletionService;
+import com.digitalsanctuary.spring.user.roles.RolePrivilegeSetupService;
import com.digitalsanctuary.spring.user.service.UserEmailService;
import com.digitalsanctuary.spring.user.service.UserService;
@@ -35,9 +36,9 @@
*
*
*
- * Note: this rule is intentionally scoped to the specific methods invoked via {@code self}. Other package-private
- * {@code @Transactional} helpers (e.g. {@code RolePrivilegeSetupService.getOrCreateRole}) are called via {@code this}
- * from within an already-transactional method and run in the caller's transaction, so they do not need to be proxied.
+ * Note: this rule is intentionally scoped to the specific methods invoked via {@code self}. The
+ * {@code RolePrivilegeSetupService.insert*}/{@code updateRolePrivileges} methods use the same {@code @Lazy self}
+ * pattern (to obtain a {@code REQUIRES_NEW} boundary per insert during startup), so they are included below.
*
*/
@DisplayName("Self-proxied (@Lazy self) transactional methods must be proxyable (public/protected)")
@@ -52,7 +53,10 @@ static List selfProxiedMethods() {
Arguments.of(UserService.class, "persistChangedPassword"),
Arguments.of(UserService.class, "persistInitialPassword"),
Arguments.of(GdprDeletionService.class, "executeUserDeletion"),
- Arguments.of(UserEmailService.class, "createPasswordResetTokenForUser"));
+ Arguments.of(UserEmailService.class, "createPasswordResetTokenForUser"),
+ Arguments.of(RolePrivilegeSetupService.class, "insertPrivilege"),
+ Arguments.of(RolePrivilegeSetupService.class, "insertRole"),
+ Arguments.of(RolePrivilegeSetupService.class, "updateRolePrivileges"));
}
@ParameterizedTest(name = "{0}#{1} is public or protected")
diff --git a/src/test/java/com/digitalsanctuary/spring/user/persistence/model/RolePrivilegeUniquenessTest.java b/src/test/java/com/digitalsanctuary/spring/user/persistence/model/RolePrivilegeUniquenessTest.java
new file mode 100644
index 00000000..ba7b8183
--- /dev/null
+++ b/src/test/java/com/digitalsanctuary/spring/user/persistence/model/RolePrivilegeUniquenessTest.java
@@ -0,0 +1,49 @@
+package com.digitalsanctuary.spring.user.persistence.model;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.jpa.test.autoconfigure.TestEntityManager;
+import org.springframework.dao.DataIntegrityViolationException;
+import com.digitalsanctuary.spring.user.persistence.repository.PrivilegeRepository;
+import com.digitalsanctuary.spring.user.persistence.repository.RoleRepository;
+import com.digitalsanctuary.spring.user.test.annotations.DatabaseTest;
+
+/**
+ * Database-slice tests verifying that the UNIQUE + NOT NULL constraint on the {@code name} column of {@link Role} and
+ * {@link Privilege} is enforced by the schema. Two roles (or two privileges) with the same name must not coexist.
+ */
+@DatabaseTest
+class RolePrivilegeUniquenessTest {
+
+ @Autowired
+ private RoleRepository roleRepository;
+
+ @Autowired
+ private PrivilegeRepository privilegeRepository;
+
+ @Autowired
+ private TestEntityManager entityManager;
+
+ @Test
+ void shouldRejectDuplicateRoleNameWhenFlushed() {
+ roleRepository.saveAndFlush(new Role("ROLE_DUPLICATE"));
+ entityManager.clear();
+
+ Role second = new Role("ROLE_DUPLICATE");
+
+ assertThatThrownBy(() -> roleRepository.saveAndFlush(second))
+ .isInstanceOf(DataIntegrityViolationException.class);
+ }
+
+ @Test
+ void shouldRejectDuplicatePrivilegeNameWhenFlushed() {
+ privilegeRepository.saveAndFlush(new Privilege("DUPLICATE_PRIVILEGE"));
+ entityManager.clear();
+
+ Privilege second = new Privilege("DUPLICATE_PRIVILEGE");
+
+ assertThatThrownBy(() -> privilegeRepository.saveAndFlush(second))
+ .isInstanceOf(DataIntegrityViolationException.class);
+ }
+}
diff --git a/src/test/java/com/digitalsanctuary/spring/user/roles/RolePrivilegeSetupServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/roles/RolePrivilegeSetupServiceTest.java
index bcc51ae3..41ecf678 100644
--- a/src/test/java/com/digitalsanctuary/spring/user/roles/RolePrivilegeSetupServiceTest.java
+++ b/src/test/java/com/digitalsanctuary/spring/user/roles/RolePrivilegeSetupServiceTest.java
@@ -3,14 +3,18 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
-import static org.mockito.ArgumentMatchers.eq;
-import static org.mockito.Mockito.*;
+import static org.mockito.Mockito.lenient;
+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 java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
import org.junit.jupiter.api.BeforeEach;
@@ -22,6 +26,7 @@
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import org.springframework.context.event.ContextRefreshedEvent;
+import org.springframework.dao.DataIntegrityViolationException;
import com.digitalsanctuary.spring.user.persistence.model.Privilege;
import com.digitalsanctuary.spring.user.persistence.model.Role;
@@ -49,10 +54,14 @@ class RolePrivilegeSetupServiceTest {
@BeforeEach
void setUp() {
rolePrivilegeSetupService = new RolePrivilegeSetupService(
- rolesAndPrivilegesConfig,
- roleRepository,
+ rolesAndPrivilegesConfig,
+ roleRepository,
privilegeRepository
);
+ // In production the REQUIRES_NEW insert methods are routed through the Spring proxy. In this unit test there is
+ // no proxy, so point the self-reference at the bean itself: a direct call still exercises the catch/re-read
+ // branch we care about (the transactional propagation is a runtime concern verified by integration tests).
+ rolePrivilegeSetupService.setSelf(rolePrivilegeSetupService);
}
@Nested
@@ -66,23 +75,22 @@ void shouldSetupRolesAndPrivilegesOnFirstContextRefresh() {
Map> rolesAndPrivileges = new HashMap<>();
rolesAndPrivileges.put("ROLE_ADMIN", Arrays.asList("READ_PRIVILEGE", "WRITE_PRIVILEGE", "DELETE_PRIVILEGE"));
rolesAndPrivileges.put("ROLE_USER", Arrays.asList("READ_PRIVILEGE"));
-
+
when(rolesAndPrivilegesConfig.getRolesAndPrivileges()).thenReturn(rolesAndPrivileges);
when(privilegeRepository.findByName(anyString())).thenReturn(null);
- when(privilegeRepository.save(any(Privilege.class))).thenAnswer(invocation -> invocation.getArgument(0));
+ when(privilegeRepository.saveAndFlush(any(Privilege.class))).thenAnswer(invocation -> invocation.getArgument(0));
when(roleRepository.findByName(anyString())).thenReturn(null);
- when(roleRepository.save(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0));
+ when(roleRepository.saveAndFlush(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0));
// When
rolePrivilegeSetupService.onApplicationEvent(contextRefreshedEvent);
// Then
- // READ_PRIVILEGE appears twice (for ROLE_ADMIN and ROLE_USER), so 4 total lookups
- verify(privilegeRepository, times(4)).findByName(anyString());
- // All 4 privileges will be saved (READ is saved twice because findByName returns null each time)
- verify(privilegeRepository, times(4)).save(any(Privilege.class));
+ // 4 privilege creations (READ appears for both roles and findByName always returns null, so it is created
+ // each time it is requested in getOrCreatePrivilege): 3 for ROLE_ADMIN + 1 for ROLE_USER.
+ verify(privilegeRepository, times(4)).saveAndFlush(any(Privilege.class));
verify(roleRepository, times(2)).findByName(anyString());
- verify(roleRepository, times(2)).save(any(Role.class));
+ verify(roleRepository, times(2)).saveAndFlush(any(Role.class));
assertThat(rolePrivilegeSetupService.isAlreadySetup()).isTrue();
}
@@ -123,9 +131,11 @@ void shouldSkipNullRoleNamesInConfiguration() {
Map> rolesAndPrivileges = new HashMap<>();
rolesAndPrivileges.put(null, Arrays.asList("READ_PRIVILEGE"));
rolesAndPrivileges.put("ROLE_USER", Arrays.asList("READ_PRIVILEGE"));
-
+
when(rolesAndPrivilegesConfig.getRolesAndPrivileges()).thenReturn(rolesAndPrivileges);
when(privilegeRepository.findByName("READ_PRIVILEGE")).thenReturn(new Privilege("READ_PRIVILEGE"));
+ when(roleRepository.findByName("ROLE_USER")).thenReturn(null);
+ when(roleRepository.saveAndFlush(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0));
// When
rolePrivilegeSetupService.onApplicationEvent(contextRefreshedEvent);
@@ -142,9 +152,11 @@ void shouldSkipRolesWithNullPrivilegeLists() {
Map> rolesAndPrivileges = new HashMap<>();
rolesAndPrivileges.put("ROLE_ADMIN", null);
rolesAndPrivileges.put("ROLE_USER", Arrays.asList("READ_PRIVILEGE"));
-
+
when(rolesAndPrivilegesConfig.getRolesAndPrivileges()).thenReturn(rolesAndPrivileges);
when(privilegeRepository.findByName("READ_PRIVILEGE")).thenReturn(new Privilege("READ_PRIVILEGE"));
+ when(roleRepository.findByName("ROLE_USER")).thenReturn(null);
+ when(roleRepository.saveAndFlush(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0));
// When
rolePrivilegeSetupService.onApplicationEvent(contextRefreshedEvent);
@@ -167,7 +179,7 @@ void shouldCreateNewPrivilegeWhenNotExists() {
when(privilegeRepository.findByName(privilegeName)).thenReturn(null);
Privilege savedPrivilege = new Privilege(privilegeName);
savedPrivilege.setId(1L);
- when(privilegeRepository.save(any(Privilege.class))).thenReturn(savedPrivilege);
+ when(privilegeRepository.saveAndFlush(any(Privilege.class))).thenReturn(savedPrivilege);
// When
Privilege result = rolePrivilegeSetupService.getOrCreatePrivilege(privilegeName);
@@ -177,7 +189,7 @@ void shouldCreateNewPrivilegeWhenNotExists() {
assertThat(result.getName()).isEqualTo(privilegeName);
assertThat(result.getId()).isEqualTo(1L);
verify(privilegeRepository).findByName(privilegeName);
- verify(privilegeRepository).save(any(Privilege.class));
+ verify(privilegeRepository).saveAndFlush(any(Privilege.class));
}
@Test
@@ -195,31 +207,48 @@ void shouldReturnExistingPrivilegeWhenExists() {
// Then
assertThat(result).isEqualTo(existingPrivilege);
verify(privilegeRepository).findByName(privilegeName);
- verify(privilegeRepository, never()).save(any(Privilege.class));
+ verify(privilegeRepository, never()).saveAndFlush(any(Privilege.class));
}
@Test
- @DisplayName("Should handle multiple privileges efficiently")
- void shouldHandleMultiplePrivilegesEfficiently() {
- // Given
- List privilegeNames = Arrays.asList("READ", "WRITE", "DELETE", "UPDATE");
- when(privilegeRepository.findByName(anyString())).thenReturn(null);
- when(privilegeRepository.save(any(Privilege.class))).thenAnswer(invocation -> {
- Privilege p = invocation.getArgument(0);
- p.setId((long) privilegeNames.indexOf(p.getName()) + 1);
- return p;
- });
+ @DisplayName("Should return existing privilege when a concurrent node inserted it (constraint race)")
+ void shouldReturnExistingPrivilegeWhenConcurrentInsertViolatesConstraint() {
+ // Given: first findByName misses (privilege not yet visible), insert hits the unique constraint because a
+ // concurrent node committed it first, then the re-read finds the winning node's row.
+ String privilegeName = "READ_PRIVILEGE";
+ Privilege concurrentPrivilege = new Privilege(privilegeName);
+ concurrentPrivilege.setId(42L);
+ when(privilegeRepository.findByName(privilegeName))
+ .thenReturn(null)
+ .thenReturn(concurrentPrivilege);
+ when(privilegeRepository.saveAndFlush(any(Privilege.class)))
+ .thenThrow(new DataIntegrityViolationException("unique violation on privilege.name"));
// When
- Set privileges = new HashSet<>();
- for (String name : privilegeNames) {
- privileges.add(rolePrivilegeSetupService.getOrCreatePrivilege(name));
- }
+ Privilege result = rolePrivilegeSetupService.getOrCreatePrivilege(privilegeName);
- // Then
- assertThat(privileges).hasSize(4);
- verify(privilegeRepository, times(4)).findByName(anyString());
- verify(privilegeRepository, times(4)).save(any(Privilege.class));
+ // Then: no exception propagated, the existing row is returned, no duplicate created.
+ assertThat(result).isSameAs(concurrentPrivilege);
+ verify(privilegeRepository, times(2)).findByName(privilegeName);
+ verify(privilegeRepository, times(1)).saveAndFlush(any(Privilege.class));
+ }
+
+ @Test
+ @DisplayName("Should rethrow when insert fails and re-read still finds nothing")
+ void shouldRethrowWhenInsertFailsAndReReadFindsNothing() {
+ // Given: a genuine integrity problem (not a name race) — the row is still absent after the failed insert.
+ String privilegeName = "READ_PRIVILEGE";
+ when(privilegeRepository.findByName(privilegeName)).thenReturn(null);
+ DataIntegrityViolationException failure = new DataIntegrityViolationException("not a name conflict");
+ when(privilegeRepository.saveAndFlush(any(Privilege.class))).thenThrow(failure);
+
+ // When / Then
+ try {
+ rolePrivilegeSetupService.getOrCreatePrivilege(privilegeName);
+ org.assertj.core.api.Assertions.failBecauseExceptionWasNotThrown(DataIntegrityViolationException.class);
+ } catch (DataIntegrityViolationException e) {
+ assertThat(e).isSameAs(failure);
+ }
}
}
@@ -232,26 +261,29 @@ class RoleManagementTests {
void shouldCreateNewRoleWhenNotExists() {
// Given
String roleName = "ROLE_ADMIN";
- Set privileges = new HashSet<>();
- privileges.add(new Privilege("READ_PRIVILEGE"));
- privileges.add(new Privilege("WRITE_PRIVILEGE"));
-
+ Privilege readPriv = new Privilege("READ_PRIVILEGE");
+ readPriv.setId(10L);
+ Privilege writePriv = new Privilege("WRITE_PRIVILEGE");
+ writePriv.setId(11L);
+ when(privilegeRepository.findByName("READ_PRIVILEGE")).thenReturn(readPriv);
+ when(privilegeRepository.findByName("WRITE_PRIVILEGE")).thenReturn(writePriv);
+
when(roleRepository.findByName(roleName)).thenReturn(null);
- when(roleRepository.save(any(Role.class))).thenAnswer(invocation -> {
+ when(roleRepository.saveAndFlush(any(Role.class))).thenAnswer(invocation -> {
Role r = invocation.getArgument(0);
r.setId(1L);
return r;
});
// When
- Role result = rolePrivilegeSetupService.getOrCreateRole(roleName, privileges);
+ Role result = rolePrivilegeSetupService.getOrCreateRole(roleName, new HashSet<>(Arrays.asList("READ_PRIVILEGE", "WRITE_PRIVILEGE")));
// Then
assertThat(result).isNotNull();
assertThat(result.getName()).isEqualTo(roleName);
- assertThat(result.getPrivileges()).isEqualTo(privileges);
+ assertThat(result.getPrivileges()).containsExactlyInAnyOrder(readPriv, writePriv);
verify(roleRepository).findByName(roleName);
- verify(roleRepository).save(any(Role.class));
+ verify(roleRepository).saveAndFlush(any(Role.class));
}
@Test
@@ -264,28 +296,28 @@ void shouldUpdateExistingRolePrivileges() {
Set oldPrivileges = new HashSet<>();
oldPrivileges.add(new Privilege("OLD_PRIVILEGE"));
existingRole.setPrivileges(oldPrivileges);
-
- Set newPrivileges = new HashSet<>();
+
Privilege readPriv = new Privilege("READ_PRIVILEGE");
readPriv.setId(10L);
Privilege writePriv = new Privilege("WRITE_PRIVILEGE");
writePriv.setId(11L);
- newPrivileges.add(readPriv);
- newPrivileges.add(writePriv);
-
+ when(privilegeRepository.findByName("READ_PRIVILEGE")).thenReturn(readPriv);
+ when(privilegeRepository.findByName("WRITE_PRIVILEGE")).thenReturn(writePriv);
+
when(roleRepository.findByName(roleName)).thenReturn(existingRole);
- when(roleRepository.save(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0));
+ when(roleRepository.findById(2L)).thenReturn(Optional.of(existingRole));
+ when(roleRepository.saveAndFlush(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0));
// When
- Role result = rolePrivilegeSetupService.getOrCreateRole(roleName, newPrivileges);
+ Role result = rolePrivilegeSetupService.getOrCreateRole(roleName, new HashSet<>(Arrays.asList("READ_PRIVILEGE", "WRITE_PRIVILEGE")));
// Then
// The role's privileges should be completely replaced with the new set
assertThat(result).isSameAs(existingRole);
- assertThat(result.getPrivileges()).isEqualTo(newPrivileges);
+ assertThat(result.getPrivileges()).containsExactlyInAnyOrder(readPriv, writePriv);
assertThat(result.getPrivileges()).hasSize(2);
verify(roleRepository).findByName(roleName);
- verify(roleRepository).save(existingRole);
+ verify(roleRepository).saveAndFlush(existingRole);
}
@Test
@@ -293,19 +325,47 @@ void shouldUpdateExistingRolePrivileges() {
void shouldHandleRoleWithEmptyPrivilegeSet() {
// Given
String roleName = "ROLE_GUEST";
- Set emptyPrivileges = new HashSet<>();
-
+
when(roleRepository.findByName(roleName)).thenReturn(null);
- when(roleRepository.save(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0));
+ when(roleRepository.saveAndFlush(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0));
// When
- Role result = rolePrivilegeSetupService.getOrCreateRole(roleName, emptyPrivileges);
+ Role result = rolePrivilegeSetupService.getOrCreateRole(roleName, new HashSet<>());
// Then
assertThat(result).isNotNull();
assertThat(result.getName()).isEqualTo(roleName);
assertThat(result.getPrivileges()).isEmpty();
- verify(roleRepository).save(any(Role.class));
+ verify(roleRepository).saveAndFlush(any(Role.class));
+ }
+
+ @Test
+ @DisplayName("Should return existing role when a concurrent node inserted it (constraint race)")
+ void shouldReturnExistingRoleWhenConcurrentInsertViolatesConstraint() {
+ // Given: role not visible on first read, insert hits the unique constraint (concurrent node won), re-read
+ // finds the winning row, and we (re)assign privileges onto it.
+ String roleName = "ROLE_ADMIN";
+ Privilege readPriv = new Privilege("READ_PRIVILEGE");
+ when(privilegeRepository.findByName("READ_PRIVILEGE")).thenReturn(readPriv);
+
+ Role concurrentRole = new Role(roleName);
+ concurrentRole.setId(99L);
+
+ when(roleRepository.findByName(roleName))
+ .thenReturn(null)
+ .thenReturn(concurrentRole);
+ when(roleRepository.findById(99L)).thenReturn(Optional.of(concurrentRole));
+ when(roleRepository.saveAndFlush(any(Role.class)))
+ .thenThrow(new DataIntegrityViolationException("unique violation on role.name"))
+ .thenAnswer(invocation -> invocation.getArgument(0));
+
+ // When
+ Role result = rolePrivilegeSetupService.getOrCreateRole(roleName, new HashSet<>(Arrays.asList("READ_PRIVILEGE")));
+
+ // Then: no exception propagated, the existing row is returned with the desired privileges.
+ assertThat(result).isSameAs(concurrentRole);
+ assertThat(result.getPrivileges()).containsExactly(readPriv);
+ verify(roleRepository, times(2)).findByName(roleName);
}
}
@@ -313,6 +373,39 @@ void shouldHandleRoleWithEmptyPrivilegeSet() {
@DisplayName("Integration Tests")
class IntegrationTests {
+ /**
+ * Wires the mocked repositories to behave like a simple in-memory store: {@code findByName} returns whatever
+ * was previously {@code saveAndFlush}-ed (assigning an id on first save), and {@code findById} returns the
+ * stored row. This lets the role insert/update path resolve privileges to "managed" instances by name, mirroring
+ * production where the privileges were created earlier in the same setup flow.
+ */
+ private void wireInMemoryStore() {
+ final Map privileges = new HashMap<>();
+ final Map roles = new HashMap<>();
+ final Map rolesById = new HashMap<>();
+
+ lenient().when(privilegeRepository.findByName(anyString())).thenAnswer(inv -> privileges.get(inv.getArgument(0)));
+ lenient().when(privilegeRepository.saveAndFlush(any(Privilege.class))).thenAnswer(inv -> {
+ Privilege p = inv.getArgument(0);
+ if (p.getId() == null) {
+ p.setId((long) (privileges.size() + 1));
+ }
+ privileges.put(p.getName(), p);
+ return p;
+ });
+ lenient().when(roleRepository.findByName(anyString())).thenAnswer(inv -> roles.get(inv.getArgument(0)));
+ lenient().when(roleRepository.findById(any())).thenAnswer(inv -> Optional.ofNullable(rolesById.get(inv.getArgument(0))));
+ lenient().when(roleRepository.saveAndFlush(any(Role.class))).thenAnswer(inv -> {
+ Role r = inv.getArgument(0);
+ if (r.getId() == null) {
+ r.setId((long) (roles.size() + 1));
+ }
+ roles.put(r.getName(), r);
+ rolesById.put(r.getId(), r);
+ return r;
+ });
+ }
+
@Test
@DisplayName("Should handle complex role hierarchy setup")
void shouldHandleComplexRoleHierarchySetup() {
@@ -322,27 +415,24 @@ void shouldHandleComplexRoleHierarchySetup() {
rolesAndPrivileges.put("ROLE_ADMIN", Arrays.asList("READ", "WRITE", "DELETE"));
rolesAndPrivileges.put("ROLE_MODERATOR", Arrays.asList("READ", "WRITE"));
rolesAndPrivileges.put("ROLE_USER", Arrays.asList("READ"));
-
+
when(rolesAndPrivilegesConfig.getRolesAndPrivileges()).thenReturn(rolesAndPrivileges);
- when(privilegeRepository.findByName(anyString())).thenReturn(null);
- when(privilegeRepository.save(any(Privilege.class))).thenAnswer(invocation -> invocation.getArgument(0));
- when(roleRepository.findByName(anyString())).thenReturn(null);
- when(roleRepository.save(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0));
+ wireInMemoryStore();
// When
rolePrivilegeSetupService.onApplicationEvent(contextRefreshedEvent);
// Then
- // Verify unique privileges are created (7 unique privileges total, but READ appears 4 times)
+ // 6 unique privileges are persisted exactly once each (READ/WRITE/DELETE are shared across roles).
ArgumentCaptor privilegeCaptor = ArgumentCaptor.forClass(Privilege.class);
- verify(privilegeRepository, times(9)).save(privilegeCaptor.capture()); // Total privilege saves
-
- // Verify all roles are created
+ verify(privilegeRepository, times(6)).saveAndFlush(privilegeCaptor.capture());
+ assertThat(privilegeCaptor.getAllValues()).extracting(Privilege::getName)
+ .containsExactlyInAnyOrder("SUPER_READ", "SUPER_WRITE", "SUPER_DELETE", "READ", "WRITE", "DELETE");
+
+ // All 4 roles are created with their resolved privileges.
ArgumentCaptor roleCaptor = ArgumentCaptor.forClass(Role.class);
- verify(roleRepository, times(4)).save(roleCaptor.capture());
-
- List savedRoles = roleCaptor.getAllValues();
- assertThat(savedRoles).extracting(Role::getName)
+ verify(roleRepository, times(4)).saveAndFlush(roleCaptor.capture());
+ assertThat(roleCaptor.getAllValues()).extracting(Role::getName)
.containsExactlyInAnyOrder("ROLE_SUPER_ADMIN", "ROLE_ADMIN", "ROLE_MODERATOR", "ROLE_USER");
}
@@ -353,30 +443,19 @@ void shouldReuseExistingPrivilegesAcrossRoles() {
Map> rolesAndPrivileges = new HashMap<>();
rolesAndPrivileges.put("ROLE_ADMIN", Arrays.asList("READ", "WRITE"));
rolesAndPrivileges.put("ROLE_USER", Arrays.asList("READ"));
-
- Privilege readPrivilege = new Privilege("READ");
- readPrivilege.setId(1L);
-
+
when(rolesAndPrivilegesConfig.getRolesAndPrivileges()).thenReturn(rolesAndPrivileges);
- // First call returns null (creates), subsequent calls return existing
- when(privilegeRepository.findByName("READ"))
- .thenReturn(null)
- .thenReturn(readPrivilege);
- when(privilegeRepository.findByName("WRITE")).thenReturn(null);
- when(privilegeRepository.save(any(Privilege.class))).thenAnswer(invocation -> invocation.getArgument(0));
- when(roleRepository.findByName(anyString())).thenReturn(null);
- when(roleRepository.save(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0));
+ wireInMemoryStore();
// When
rolePrivilegeSetupService.onApplicationEvent(contextRefreshedEvent);
// Then
- // READ privilege should be looked up twice but only created once
- verify(privilegeRepository, times(2)).findByName("READ");
- // WRITE privilege should be created once
- verify(privilegeRepository, times(1)).findByName("WRITE");
- // Only 2 privileges should be saved (READ once, WRITE once)
- verify(privilegeRepository, times(2)).save(any(Privilege.class));
+ // READ and WRITE are each created exactly once; READ is reused for the second role.
+ ArgumentCaptor privilegeCaptor = ArgumentCaptor.forClass(Privilege.class);
+ verify(privilegeRepository, times(2)).saveAndFlush(privilegeCaptor.capture());
+ assertThat(privilegeCaptor.getAllValues()).extracting(Privilege::getName)
+ .containsExactlyInAnyOrder("READ", "WRITE");
}
@Test
@@ -385,41 +464,24 @@ void shouldHandleDatabaseTransactionProperly() {
// Given
Map> rolesAndPrivileges = new HashMap<>();
rolesAndPrivileges.put("ROLE_TRANSACTIONAL", Arrays.asList("TRANSACT_READ", "TRANSACT_WRITE"));
-
+
when(rolesAndPrivilegesConfig.getRolesAndPrivileges()).thenReturn(rolesAndPrivileges);
- when(privilegeRepository.findByName(anyString())).thenReturn(null);
-
- // Track created privileges to return the same instance when saved
- Map createdPrivileges = new HashMap<>();
- when(privilegeRepository.save(any(Privilege.class))).thenAnswer(invocation -> {
- Privilege p = invocation.getArgument(0);
- p.setId((long) (createdPrivileges.size() + 1)); // Simulate DB generating ID
- createdPrivileges.put(p.getName(), p);
- return p;
- });
-
- when(roleRepository.findByName(anyString())).thenReturn(null);
- when(roleRepository.save(any(Role.class))).thenAnswer(invocation -> {
- Role r = invocation.getArgument(0);
- r.setId(1L);
- return r;
- });
+ wireInMemoryStore();
// When
rolePrivilegeSetupService.onApplicationEvent(contextRefreshedEvent);
// Then
ArgumentCaptor roleCaptor = ArgumentCaptor.forClass(Role.class);
- verify(roleRepository).save(roleCaptor.capture());
-
+ verify(roleRepository).saveAndFlush(roleCaptor.capture());
+
Role savedRole = roleCaptor.getValue();
// The privileges are stored in a Set, so we should have 2 unique privileges
assertThat(savedRole.getPrivileges()).hasSize(2);
- // Verify the privileges have their names set correctly
assertThat(savedRole.getPrivileges())
.extracting(Privilege::getName)
.containsExactlyInAnyOrder("TRANSACT_READ", "TRANSACT_WRITE");
- // Verify the privileges have IDs assigned
+ // Verify the privileges have IDs assigned (they were resolved as persisted, managed entities)
assertThat(savedRole.getPrivileges()).allMatch(p -> p.getId() != null && p.getId() > 0);
}
@@ -430,27 +492,50 @@ void shouldCompleteSetupWithMixedExistingAndNewEntities() {
Map> rolesAndPrivileges = new HashMap<>();
rolesAndPrivileges.put("ROLE_EXISTING", Arrays.asList("EXISTING_PRIV", "NEW_PRIV"));
rolesAndPrivileges.put("ROLE_NEW", Arrays.asList("EXISTING_PRIV"));
-
+
Privilege existingPriv = new Privilege("EXISTING_PRIV");
existingPriv.setId(100L);
Role existingRole = new Role("ROLE_EXISTING");
existingRole.setId(200L);
-
+
when(rolesAndPrivilegesConfig.getRolesAndPrivileges()).thenReturn(rolesAndPrivileges);
- when(privilegeRepository.findByName("EXISTING_PRIV")).thenReturn(existingPriv);
- when(privilegeRepository.findByName("NEW_PRIV")).thenReturn(null);
- when(privilegeRepository.save(any(Privilege.class))).thenAnswer(invocation -> invocation.getArgument(0));
- when(roleRepository.findByName("ROLE_EXISTING")).thenReturn(existingRole);
- when(roleRepository.findByName("ROLE_NEW")).thenReturn(null);
- when(roleRepository.save(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0));
+ // Pre-seed an in-memory store with one existing privilege and one existing role.
+ final Map privileges = new HashMap<>();
+ privileges.put("EXISTING_PRIV", existingPriv);
+ final Map roles = new HashMap<>();
+ roles.put("ROLE_EXISTING", existingRole);
+ final Map rolesById = new HashMap<>();
+ rolesById.put(200L, existingRole);
+
+ when(privilegeRepository.findByName(anyString())).thenAnswer(inv -> privileges.get(inv.getArgument(0)));
+ when(privilegeRepository.saveAndFlush(any(Privilege.class))).thenAnswer(inv -> {
+ Privilege p = inv.getArgument(0);
+ if (p.getId() == null) {
+ p.setId((long) (privileges.size() + 1));
+ }
+ privileges.put(p.getName(), p);
+ return p;
+ });
+ when(roleRepository.findByName(anyString())).thenAnswer(inv -> roles.get(inv.getArgument(0)));
+ when(roleRepository.findById(any())).thenAnswer(inv -> Optional.ofNullable(rolesById.get(inv.getArgument(0))));
+ when(roleRepository.saveAndFlush(any(Role.class))).thenAnswer(inv -> {
+ Role r = inv.getArgument(0);
+ if (r.getId() == null) {
+ r.setId((long) (roles.size() + 1));
+ }
+ roles.put(r.getName(), r);
+ rolesById.put(r.getId(), r);
+ return r;
+ });
// When
rolePrivilegeSetupService.onApplicationEvent(contextRefreshedEvent);
// Then
- verify(privilegeRepository, times(1)).save(any(Privilege.class)); // Only NEW_PRIV
- verify(roleRepository, times(2)).save(any(Role.class)); // Both roles get saved (existing one with updated privileges)
+ verify(privilegeRepository, times(1)).saveAndFlush(any(Privilege.class)); // Only NEW_PRIV created
+ verify(roleRepository, times(2)).saveAndFlush(any(Role.class)); // Both roles saved (existing one updated)
+ verify(roleRepository, times(1)).findById(200L); // existing role re-read via REQUIRES_NEW update path
assertThat(rolePrivilegeSetupService.isAlreadySetup()).isTrue();
}
}
-}
\ No newline at end of file
+}
diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/UserUpdateIntegrationTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/UserUpdateIntegrationTest.java
index 8f2f8b23..b4ec607c 100644
--- a/src/test/java/com/digitalsanctuary/spring/user/service/UserUpdateIntegrationTest.java
+++ b/src/test/java/com/digitalsanctuary/spring/user/service/UserUpdateIntegrationTest.java
@@ -51,16 +51,21 @@ class UserUpdateIntegrationTest {
@BeforeEach
void setUp() {
- // Clean up
+ // Clean up users; do not delete-and-recreate roles. ROLE_USER and ROLE_ADMIN already exist (created at startup
+ // by RolePrivilegeSetupService) and now carry a UNIQUE constraint on name, so reuse the existing rows via
+ // findByName-or-create rather than re-inserting duplicates.
userRepository.deleteAll();
- roleRepository.deleteAll();
- // Create roles
- userRole = new Role("ROLE_USER", "Basic user role");
- userRole = roleRepository.save(userRole);
+ userRole = getOrCreateRole("ROLE_USER", "Basic user role");
+ adminRole = getOrCreateRole("ROLE_ADMIN", "Administrator role");
+ }
- adminRole = new Role("ROLE_ADMIN", "Administrator role");
- adminRole = roleRepository.save(adminRole);
+ private Role getOrCreateRole(final String name, final String description) {
+ final Role existing = roleRepository.findByName(name);
+ if (existing != null) {
+ return existing;
+ }
+ return roleRepository.save(new Role(name, description));
}
@Test
From 70e1f99d576f58d58b475d3224a8d6346899235c Mon Sep 17 00:00:00 2001
From: Devon Hillard
Date: Mon, 15 Jun 2026 10:34:38 -0600
Subject: [PATCH 07/23] perf: lazy role/privilege fetch with EntityGraph on the
auth path
---
CHANGELOG.md | 4 +
MIGRATION.md | 17 +++
.../spring/user/gdpr/GdprExportService.java | 8 +-
.../spring/user/persistence/model/Role.java | 3 +-
.../spring/user/persistence/model/User.java | 2 +-
.../repository/UserRepository.java | 18 +++
.../user/service/DSOAuth2UserService.java | 2 +-
.../user/service/DSOidcUserService.java | 2 +-
.../user/service/DSUserDetailsService.java | 4 +-
.../spring/user/service/UserService.java | 8 +-
.../user/gdpr/GdprExportServiceTest.java | 25 ++++
.../UserRepositoryEntityGraphTest.java | 141 ++++++++++++++++++
...Auth2UserServiceRegistrationGuardTest.java | 6 +-
.../user/service/DSOAuth2UserServiceTest.java | 28 ++--
...SOidcUserServiceRegistrationGuardTest.java | 6 +-
.../user/service/DSOidcUserServiceTest.java | 20 +--
.../service/DSUserDetailsServiceTest.java | 26 ++--
.../service/TokenHashingSecurityTest.java | 2 +-
.../spring/user/service/UserServiceTest.java | 19 +--
19 files changed, 274 insertions(+), 67 deletions(-)
create mode 100644 src/test/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepositoryEntityGraphTest.java
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2b53ec0f..e1acfcb4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,7 +12,11 @@
- Credential-altering operations (remove password, delete/rename passkey) now require the current password when the account has one, preventing a session-only actor from changing authentication methods.
- Role/privilege startup setup is now idempotent and safe under concurrent multi-node startup (handles unique-constraint races by re-reading the existing row).
+### Performance
+- `User` → `roles` and `Role` → `privileges` are now LAZY-fetched; the authentication path loads them via `@EntityGraph` in a single query (`UserRepository.findWithRolesByEmail`), removing the previous two-level eager fetch and the associated N+1 problem while keeping authority resolution correct.
+
### Breaking Changes
+- Role and privilege collections on `User`/`Role` are now LAZY. Consumer code that accesses `user.getRoles()` or `role.getPrivileges()` outside an open transaction/session may now throw `LazyInitializationException`. Load via the authentication path / an `@EntityGraph` query (e.g. `UserRepository.findWithRolesByEmail`), or access the collections within a transaction. See MIGRATION.md.
- Reset/verification email links no longer trust `X-Forwarded-Host` by default. Deployments behind a reverse proxy must set `user.security.appUrl` or `user.security.trustedHosts` (see MIGRATION.md). `UserUtils.getAppUrl(HttpServletRequest)` is deprecated for removal.
- Added a UNIQUE, NOT NULL constraint on the `token` column of `password_reset_token` and `verification_token`. This is a schema/DDL change — see MIGRATION.md.
- Added UNIQUE, NOT NULL constraints on `role.name` and `privilege.name` (schema/DDL change). See MIGRATION.md.
diff --git a/MIGRATION.md b/MIGRATION.md
index 21b01698..32463f94 100644
--- a/MIGRATION.md
+++ b/MIGRATION.md
@@ -150,6 +150,23 @@ CREATE UNIQUE INDEX ux_privilege_name ON privilege (name);
> **Note:** The table names above (`role`, `privilege`) and column name (`name`) are Hibernate's defaults for the `Role` and `Privilege` entities (no `@Table` override). If you have customized Hibernate's physical naming strategy, adjust the identifiers accordingly. The DDL syntax is standard SQL (PostgreSQL / MariaDB / MySQL); for MySQL/MariaDB, `ALTER COLUMN name SET NOT NULL` may need the full column definition, e.g. `MODIFY COLUMN name VARCHAR(255) NOT NULL`.
+### Lazy fetching of roles and privileges
+
+**What changed:** The `roles` collection on `User` and the `privileges` collection on `Role` were previously `FetchType.EAGER`. They are now `FetchType.LAZY`. The authentication path (`DSUserDetailsService.loadUserByUsername`) now loads the full `User` → `roles` → `privileges` graph via a new `@EntityGraph` repository finder, `UserRepository.findWithRolesByEmail(String email)`, which fetches everything in a **single query**.
+
+**Why:** The old two-level eager fetch loaded every role and every privilege on *every* `User` load — even for operations that never touch authorities (token lookups, lockout-counter updates, existence checks) — and caused an N+1 query pattern. Making the collections lazy and loading them explicitly only where they are needed removes that overhead while keeping authentication behavior identical.
+
+**Impact / risk:** Because the collections are now lazy, **any code that accesses `user.getRoles()` (or iterates `role.getPrivileges()`) on a detached entity — i.e. outside an open Hibernate session/transaction — will throw `LazyInitializationException`.** Code that accesses these collections *within* an active transaction (the common case for service methods) is unaffected. The framework's own authentication, OAuth2/OIDC, and GDPR-export paths have been updated to initialize the graph correctly.
+
+**Remediation patterns for consumers** that traverse roles/privileges on a `User` they obtained outside a transaction:
+
+- **Load through the authentication path or the entity-graph finder.** Use `UserRepository.findWithRolesByEmail(email)` (it initializes roles and privileges in one query) instead of the plain `findByEmail(email)` when you need authorities.
+- **Access the collections inside a transaction.** Annotate the method that reads `user.getRoles()` with `@Transactional` so the persistence session is still open when the lazy collection is first touched.
+- **Use a DTO projection.** Map the roles/privileges you need into a DTO while still inside the session, then pass the DTO around the detached boundary.
+- **Initialize before detaching.** If you must hand a `User` to detached code, call `Hibernate.initialize(user.getRolesAsSet())` (and the nested privileges) while the session is open.
+
+The plain `UserRepository.findByEmail(String)` finder is retained unchanged for callers that do not need the authority graph (token lookups, existence checks, lockout counters); it intentionally leaves `roles`/`privileges` uninitialized.
+
## Migrating to 4.0.x (Spring Boot 4.0)
diff --git a/src/main/java/com/digitalsanctuary/spring/user/gdpr/GdprExportService.java b/src/main/java/com/digitalsanctuary/spring/user/gdpr/GdprExportService.java
index 0c89011f..dcc89807 100644
--- a/src/main/java/com/digitalsanctuary/spring/user/gdpr/GdprExportService.java
+++ b/src/main/java/com/digitalsanctuary/spring/user/gdpr/GdprExportService.java
@@ -18,6 +18,7 @@
import com.digitalsanctuary.spring.user.persistence.model.User;
import com.digitalsanctuary.spring.user.persistence.model.VerificationToken;
import com.digitalsanctuary.spring.user.persistence.repository.PasswordResetTokenRepository;
+import com.digitalsanctuary.spring.user.persistence.repository.UserRepository;
import com.digitalsanctuary.spring.user.persistence.repository.VerificationTokenRepository;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
@@ -55,6 +56,7 @@ public class GdprExportService {
private final AuditLogQueryService auditLogQueryService;
private final VerificationTokenRepository verificationTokenRepository;
private final PasswordResetTokenRepository passwordResetTokenRepository;
+ private final UserRepository userRepository;
private final List dataContributors;
private final ApplicationEventPublisher eventPublisher;
private final ObjectMapper objectMapper;
@@ -104,7 +106,11 @@ private GdprExportDTO.ExportMetadata buildMetadata() {
* Builds the user data section.
*/
private GdprExportDTO.UserData buildUserData(User user) {
- List roleNames = user.getRoles().stream()
+ // Roles are now LAZY-fetched and this export runs outside an open persistence session (by design, to avoid
+ // holding a transaction open during slow I/O). Reload the user through the entity-graph finder so roles are
+ // initialized in a single query, avoiding a LazyInitializationException on the detached principal.
+ User userWithRoles = user.getEmail() != null ? userRepository.findWithRolesByEmail(user.getEmail()) : null;
+ List roleNames = (userWithRoles != null ? userWithRoles.getRoles() : List.of()).stream()
.map(Role::getName)
.collect(Collectors.toList());
diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Role.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Role.java
index 3fd8c264..100bacee 100644
--- a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Role.java
+++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Role.java
@@ -44,7 +44,8 @@ public class Role implements Serializable {
private Set users = new HashSet<>();
/** The privileges. */
- @ManyToMany(cascade = {CascadeType.PERSIST, CascadeType.MERGE}, fetch = FetchType.EAGER)
+ @ToString.Exclude
+ @ManyToMany(cascade = {CascadeType.PERSIST, CascadeType.MERGE}, fetch = FetchType.LAZY)
@JoinTable(name = "roles_privileges", joinColumns = @JoinColumn(name = "role_id", referencedColumnName = "id"),
inverseJoinColumns = @JoinColumn(name = "privilege_id", referencedColumnName = "id"))
private Set privileges = new HashSet<>();
diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/User.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/User.java
index 2f9360c9..84c8aeed 100644
--- a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/User.java
+++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/User.java
@@ -111,7 +111,7 @@ public enum Provider {
/** The roles - stored as Set to avoid Hibernate immutable collection issues */
@ToString.Exclude
@EqualsAndHashCode.Exclude
- @ManyToMany(cascade = {CascadeType.PERSIST, CascadeType.MERGE}, fetch = FetchType.EAGER)
+ @ManyToMany(cascade = {CascadeType.PERSIST, CascadeType.MERGE}, fetch = FetchType.LAZY)
@JoinTable(name = "users_roles", joinColumns = @JoinColumn(name = "user_id", referencedColumnName = "id"),
inverseJoinColumns = @JoinColumn(name = "role_id", referencedColumnName = "id"))
private Set roles = new HashSet<>();
diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepository.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepository.java
index 03638a00..eb5ab136 100644
--- a/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepository.java
+++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepository.java
@@ -1,6 +1,7 @@
package com.digitalsanctuary.spring.user.persistence.repository;
import java.util.List;
+import org.springframework.data.jpa.repository.EntityGraph;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.data.jpa.repository.Modifying;
import org.springframework.data.jpa.repository.Query;
@@ -20,6 +21,23 @@ public interface UserRepository extends JpaRepository {
*/
User findByEmail(String email);
+ /**
+ * Find by email, eagerly loading the user's roles and each role's privileges in a single query via an entity graph.
+ *
+ * This is the finder used on the authentication path (see {@code DSUserDetailsService}). Because {@code User.roles}
+ * and {@code Role.privileges} are now {@link jakarta.persistence.FetchType#LAZY}, callers that must traverse
+ * roles/privileges after the persistence session closes (e.g. building Spring Security authorities for a detached
+ * principal) must load the user through this method. The {@code @EntityGraph} ensures the full
+ * User → roles → privileges graph is initialized in one round trip, avoiding both the N+1 problem and a
+ * {@code LazyInitializationException}. The plain {@link #findByEmail(String)} remains for callers (token lookups,
+ * existence checks, lockout counters) that do not need the authority graph.
+ *
+ * @param email the email
+ * @return the user with roles and privileges initialized, or {@code null} if none found
+ */
+ @EntityGraph(attributePaths = {"roles", "roles.privileges"})
+ User findWithRolesByEmail(String email);
+
/**
* Atomically increments the failed login attempt counter for the user with the given email.
*
diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserService.java b/src/main/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserService.java
index 3cd10f5a..5985c558 100644
--- a/src/main/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserService.java
+++ b/src/main/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserService.java
@@ -90,7 +90,7 @@ public User handleOAuthLoginSuccess(String registrationId, OAuth2User oAuth2User
"Unable to retrieve email address from " + registrationId + ". Please ensure you have granted email permissions.");
}
log.debug("handleOAuthLoginSuccess: looking up user with email: {}", user.getEmail());
- User existingUser = userRepository.findByEmail(user.getEmail().toLowerCase());
+ User existingUser = userRepository.findWithRolesByEmail(user.getEmail().toLowerCase());
log.debug("handleOAuthLoginSuccess: existingUser: {}", existingUser);
if (existingUser != null && registrationId != null) {
log.debug("handleOAuthLoginSuccess: existingUser.getProvider(): {}", existingUser.getProvider());
diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/DSOidcUserService.java b/src/main/java/com/digitalsanctuary/spring/user/service/DSOidcUserService.java
index 676c2e25..90fa1b23 100644
--- a/src/main/java/com/digitalsanctuary/spring/user/service/DSOidcUserService.java
+++ b/src/main/java/com/digitalsanctuary/spring/user/service/DSOidcUserService.java
@@ -96,7 +96,7 @@ public User handleOidcLoginSuccess(String registrationId, OidcUser oidcUser) {
// but we normalize again here defensively in case additional sources are added.
String normalizedEmail = user.getEmail().trim().toLowerCase(Locale.ROOT);
log.debug("handleOidcLoginSuccess: looking up user with email: {}", normalizedEmail);
- User existingUser = userRepository.findByEmail(normalizedEmail);
+ User existingUser = userRepository.findWithRolesByEmail(normalizedEmail);
log.debug("handleOidcLoginSuccess: existingUser: {}", existingUser);
if (existingUser != null && registrationId != null) {
log.debug("handleOidcLoginSuccess: existingUser.getProvider(): {}", existingUser.getProvider());
diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/DSUserDetailsService.java b/src/main/java/com/digitalsanctuary/spring/user/service/DSUserDetailsService.java
index 30574462..5742caee 100644
--- a/src/main/java/com/digitalsanctuary/spring/user/service/DSUserDetailsService.java
+++ b/src/main/java/com/digitalsanctuary/spring/user/service/DSUserDetailsService.java
@@ -42,7 +42,9 @@ public class DSUserDetailsService implements UserDetailsService {
@Override
public DSUserDetails loadUserByUsername(final String email) throws UsernameNotFoundException {
log.debug("DSUserDetailsService.loadUserByUsername: called with username: {}", email);
- User dbUser = userRepository.findByEmail(email);
+ // Use the entity-graph finder so roles and privileges are initialized in a single query before the
+ // principal is detached, allowing AuthorityService to build authorities without a LazyInitializationException.
+ User dbUser = userRepository.findWithRolesByEmail(email);
if (dbUser == null) {
throw new UsernameNotFoundException("No user found with email/username: " + email);
}
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 f20b8f79..a808a989 100644
--- a/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java
+++ b/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java
@@ -231,8 +231,6 @@ public String getValue() {
/** The user verification service. */
public final UserVerificationService userVerificationService;
- private final AuthorityService authorityService;
-
/** The user details service. */
private final DSUserDetailsService dsUserDetailsService;
@@ -1036,8 +1034,10 @@ public void authWithoutPassword(User user) {
return;
}
- // Generate authorities from user roles and privileges
- Collection extends GrantedAuthority> authorities = authorityService.getAuthoritiesFromUser(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 extends GrantedAuthority> authorities = userDetails.getAuthorities();
// Authenticate user
authenticateUser(userDetails, authorities);
diff --git a/src/test/java/com/digitalsanctuary/spring/user/gdpr/GdprExportServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/gdpr/GdprExportServiceTest.java
index 03cf8162..1a969283 100644
--- a/src/test/java/com/digitalsanctuary/spring/user/gdpr/GdprExportServiceTest.java
+++ b/src/test/java/com/digitalsanctuary/spring/user/gdpr/GdprExportServiceTest.java
@@ -28,6 +28,7 @@
import com.digitalsanctuary.spring.user.persistence.model.User;
import com.digitalsanctuary.spring.user.persistence.model.VerificationToken;
import com.digitalsanctuary.spring.user.persistence.repository.PasswordResetTokenRepository;
+import com.digitalsanctuary.spring.user.persistence.repository.UserRepository;
import com.digitalsanctuary.spring.user.persistence.repository.VerificationTokenRepository;
import com.digitalsanctuary.spring.user.test.annotations.ServiceTest;
import com.digitalsanctuary.spring.user.test.builders.UserTestDataBuilder;
@@ -48,6 +49,9 @@ class GdprExportServiceTest {
@Mock
private PasswordResetTokenRepository passwordResetTokenRepository;
+ @Mock
+ private UserRepository userRepository;
+
@Mock
private ApplicationEventPublisher eventPublisher;
@@ -102,6 +106,27 @@ void exportsBasicUserData() {
assertThat(export.getUserData().isEnabled()).isTrue();
}
+ @Test
+ @DisplayName("exports role names by reloading the user through the entity-graph finder")
+ void exportsRoleNamesViaEntityGraphFinder() {
+ // Given - roles are LAZY, so the export service reloads the user with roles initialized.
+ User userWithRoles = UserTestDataBuilder.aVerifiedUser()
+ .withId(1L)
+ .withEmail("test@example.com")
+ .withRole("ROLE_USER")
+ .build();
+ when(userRepository.findWithRolesByEmail("test@example.com")).thenReturn(userWithRoles);
+ when(gdprConfig.isConsentTracking()).thenReturn(true);
+ when(auditLogQueryService.findByUser(testUser)).thenReturn(new ArrayList<>());
+ when(auditLogQueryService.findByUserAndAction(any(), any())).thenReturn(new ArrayList<>());
+
+ // When
+ GdprExportDTO export = gdprExportService.exportUserData(testUser);
+
+ // Then
+ assertThat(export.getUserData().getRoles()).containsExactly("ROLE_USER");
+ }
+
@Test
@DisplayName("includes export metadata")
void includesExportMetadata() {
diff --git a/src/test/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepositoryEntityGraphTest.java b/src/test/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepositoryEntityGraphTest.java
new file mode 100644
index 00000000..35d473d8
--- /dev/null
+++ b/src/test/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepositoryEntityGraphTest.java
@@ -0,0 +1,141 @@
+package com.digitalsanctuary.spring.user.persistence.repository;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import java.util.HashSet;
+import java.util.Set;
+import org.hibernate.Hibernate;
+import org.hibernate.SessionFactory;
+import org.hibernate.stat.Statistics;
+import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.jpa.test.autoconfigure.TestEntityManager;
+import com.digitalsanctuary.spring.user.persistence.model.Privilege;
+import com.digitalsanctuary.spring.user.persistence.model.Role;
+import com.digitalsanctuary.spring.user.persistence.model.User;
+import com.digitalsanctuary.spring.user.test.annotations.DatabaseTest;
+import com.digitalsanctuary.spring.user.test.builders.UserTestDataBuilder;
+import jakarta.persistence.EntityManager;
+
+/**
+ * Database-slice tests verifying that {@link UserRepository#findWithRolesByEmail(String)} eagerly loads the
+ * User → roles → privileges graph via {@code @EntityGraph} in a bounded number of queries, while the plain
+ * {@link UserRepository#findByEmail(String)} leaves the now-LAZY collections uninitialized. This is the regression guard
+ * for the EAGER → LAZY switch on {@code User.roles} and {@code Role.privileges}.
+ */
+@DatabaseTest
+class UserRepositoryEntityGraphTest {
+
+ @Autowired
+ private UserRepository userRepository;
+
+ @Autowired
+ private TestEntityManager entityManager;
+
+ private void persistUserWithRolesAndPrivileges(String email) {
+ Privilege read = new Privilege("READ_PRIVILEGE");
+ Privilege write = new Privilege("WRITE_PRIVILEGE");
+ entityManager.persist(read);
+ entityManager.persist(write);
+
+ Role userRole = new Role("ROLE_USER");
+ Set userPrivileges = new HashSet<>();
+ userPrivileges.add(read);
+ userRole.setPrivileges(userPrivileges);
+
+ Role adminRole = new Role("ROLE_ADMIN");
+ Set adminPrivileges = new HashSet<>();
+ adminPrivileges.add(read);
+ adminPrivileges.add(write);
+ adminRole.setPrivileges(adminPrivileges);
+
+ entityManager.persist(userRole);
+ entityManager.persist(adminRole);
+
+ Set roles = new HashSet<>();
+ roles.add(userRole);
+ roles.add(adminRole);
+ User user = UserTestDataBuilder.aUser().withId(null).withEmail(email).build();
+ user.setRolesAsSet(roles);
+ entityManager.persist(user);
+ entityManager.flush();
+ entityManager.clear();
+ }
+
+ private Statistics statistics() {
+ EntityManager em = entityManager.getEntityManager();
+ Statistics stats = em.getEntityManagerFactory().unwrap(SessionFactory.class).getStatistics();
+ stats.setStatisticsEnabled(true);
+ return stats;
+ }
+
+ @Test
+ void shouldInitializeRolesAndPrivilegesWhenLoadedViaEntityGraphFinder() {
+ persistUserWithRolesAndPrivileges("graph@test.com");
+
+ User user = userRepository.findWithRolesByEmail("graph@test.com");
+
+ // Detach so any later access would hit a closed session if the graph were not initialized.
+ entityManager.getEntityManager().detach(user);
+
+ assertThat(user).isNotNull();
+ assertThat(Hibernate.isInitialized(user.getRolesAsSet())).isTrue();
+ for (Role role : user.getRolesAsSet()) {
+ assertThat(Hibernate.isInitialized(role.getPrivileges())).isTrue();
+ }
+ }
+
+ @Test
+ void shouldAccessRolesAndPrivilegesWithoutLazyInitializationExceptionAfterDetach() {
+ persistUserWithRolesAndPrivileges("detached@test.com");
+
+ User user = userRepository.findWithRolesByEmail("detached@test.com");
+ entityManager.getEntityManager().detach(user);
+
+ // Traversing the full graph on a detached entity must not throw LazyInitializationException.
+ assertThatCode(() -> {
+ for (Role role : user.getRolesAsSet()) {
+ role.getName();
+ for (Privilege privilege : role.getPrivileges()) {
+ privilege.getName();
+ }
+ }
+ }).doesNotThrowAnyException();
+
+ assertThat(user.getRolesAsSet())
+ .extracting(Role::getName)
+ .containsExactlyInAnyOrder("ROLE_USER", "ROLE_ADMIN");
+ }
+
+ @Test
+ void shouldNotInitializeRolesWhenLoadedViaPlainFindByEmail() {
+ persistUserWithRolesAndPrivileges("lazy@test.com");
+
+ User user = userRepository.findByEmail("lazy@test.com");
+
+ // The plain finder must leave roles uninitialized, proving the EAGER -> LAZY switch took effect.
+ assertThat(Hibernate.isInitialized(user.getRolesAsSet())).isFalse();
+ }
+
+ @Test
+ void shouldLoadRolesAndPrivilegesInBoundedQueryCountViaEntityGraphFinder() {
+ persistUserWithRolesAndPrivileges("bounded@test.com");
+
+ Statistics stats = statistics();
+ stats.clear();
+
+ User user = userRepository.findWithRolesByEmail("bounded@test.com");
+ // Force full traversal to prove no additional (N+1) queries are issued for privileges.
+ int privilegeCount = 0;
+ for (Role role : user.getRolesAsSet()) {
+ privilegeCount += role.getPrivileges().size();
+ }
+
+ assertThat(privilegeCount).isPositive();
+ // A single entity-graph fetch should not degenerate into a per-role / per-privilege N+1 explosion.
+ // Allow a small bound to tolerate join-table fetch strategy differences across Hibernate versions.
+ assertThat(stats.getPrepareStatementCount())
+ .as("EntityGraph finder should load roles + privileges in a bounded number of queries")
+ .isLessThanOrEqualTo(3);
+ }
+}
diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceRegistrationGuardTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceRegistrationGuardTest.java
index 90c91401..00ec2ec5 100644
--- a/src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceRegistrationGuardTest.java
+++ b/src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceRegistrationGuardTest.java
@@ -77,7 +77,7 @@ void shouldRejectNewOAuth2UserWhenGuardDenies() {
.withLastName("User")
.build();
- when(userRepository.findByEmail("new@gmail.com")).thenReturn(null);
+ when(userRepository.findWithRolesByEmail("new@gmail.com")).thenReturn(null);
doThrow(new RegistrationDeniedException("Domain not allowed"))
.when(userService).enforceRegistrationGuard(eq("new@gmail.com"), eq(RegistrationSource.OAUTH2), anyString());
@@ -99,7 +99,7 @@ void shouldAllowNewOAuth2UserWhenGuardAllows() {
.withLastName("User")
.build();
- when(userRepository.findByEmail("allowed@gmail.com")).thenReturn(null);
+ when(userRepository.findWithRolesByEmail("allowed@gmail.com")).thenReturn(null);
doNothing().when(userService)
.enforceRegistrationGuard(eq("allowed@gmail.com"), eq(RegistrationSource.OAUTH2), anyString());
when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0));
@@ -125,7 +125,7 @@ void shouldNotCallGuardForExistingOAuth2User() {
existingUser.setEmail("existing@gmail.com");
existingUser.setProvider(User.Provider.GOOGLE);
- when(userRepository.findByEmail("existing@gmail.com")).thenReturn(existingUser);
+ when(userRepository.findWithRolesByEmail("existing@gmail.com")).thenReturn(existingUser);
when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0));
User result = service.handleOAuthLoginSuccess("google", googleUser);
diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceTest.java
index b1598f00..4814d6e3 100644
--- a/src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceTest.java
+++ b/src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceTest.java
@@ -90,7 +90,7 @@ void shouldCreateNewUserFromGoogleOAuth2() {
.withLastName("Doe")
.build();
- when(userRepository.findByEmail("john.doe@gmail.com")).thenReturn(null);
+ when(userRepository.findWithRolesByEmail("john.doe@gmail.com")).thenReturn(null);
when(userRepository.save(any(User.class))).thenAnswer(invocation -> {
User savedUser = invocation.getArgument(0);
savedUser.setId(123L);
@@ -143,7 +143,7 @@ void shouldUpdateExistingGoogleUser() {
existingUser.setProvider(User.Provider.GOOGLE);
existingUser.setEnabled(true);
- when(userRepository.findByEmail("existing@gmail.com")).thenReturn(existingUser);
+ when(userRepository.findWithRolesByEmail("existing@gmail.com")).thenReturn(existingUser);
when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0));
// When
@@ -169,7 +169,7 @@ void shouldHandleGoogleUserWithMissingFields() {
.withoutAttribute("family_name")
.build();
- when(userRepository.findByEmail("nolastname@gmail.com")).thenReturn(null);
+ when(userRepository.findWithRolesByEmail("nolastname@gmail.com")).thenReturn(null);
when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0));
// When
@@ -191,14 +191,14 @@ void shouldConvertEmailToLowercase() {
.withEmail("John.Doe@GMAIL.COM")
.build();
- when(userRepository.findByEmail("john.doe@gmail.com")).thenReturn(null);
+ when(userRepository.findWithRolesByEmail("john.doe@gmail.com")).thenReturn(null);
when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0));
// When
service.handleOAuthLoginSuccess("google", googleUser);
// Then
- verify(userRepository).findByEmail("john.doe@gmail.com"); // Lowercase lookup
+ verify(userRepository).findWithRolesByEmail("john.doe@gmail.com"); // Lowercase lookup
}
}
@@ -215,7 +215,7 @@ void shouldAcceptWhenEmailVerifiedBooleanTrue() {
.withAttribute("email_verified", Boolean.TRUE)
.build();
- when(userRepository.findByEmail("verified@gmail.com")).thenReturn(null);
+ when(userRepository.findWithRolesByEmail("verified@gmail.com")).thenReturn(null);
when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0));
// When
@@ -235,7 +235,7 @@ void shouldAcceptWhenEmailVerifiedStringTrue() {
.withAttribute("email_verified", "true")
.build();
- when(userRepository.findByEmail("verified-str@gmail.com")).thenReturn(null);
+ when(userRepository.findWithRolesByEmail("verified-str@gmail.com")).thenReturn(null);
when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0));
// When
@@ -255,7 +255,7 @@ void shouldAcceptWhenEmailVerifiedAbsent() {
.withoutAttribute("email_verified")
.build();
- when(userRepository.findByEmail("noclaim@gmail.com")).thenReturn(null);
+ when(userRepository.findWithRolesByEmail("noclaim@gmail.com")).thenReturn(null);
when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0));
// When
@@ -314,7 +314,7 @@ void shouldCreateNewUserFromFacebookOAuth2() {
.withFullName("Jane Marie Smith")
.build();
- when(userRepository.findByEmail("jane.smith@facebook.com")).thenReturn(null);
+ when(userRepository.findWithRolesByEmail("jane.smith@facebook.com")).thenReturn(null);
when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0));
// When
@@ -363,7 +363,7 @@ void shouldHandleFacebookUserWithoutName() {
.withoutAttribute("name")
.build();
- when(userRepository.findByEmail("noname@facebook.com")).thenReturn(null);
+ when(userRepository.findWithRolesByEmail("noname@facebook.com")).thenReturn(null);
when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0));
// When
@@ -393,7 +393,7 @@ void shouldRejectGoogleLoginForFacebookUser() {
existingUser.setEmail("conflict@example.com");
existingUser.setProvider(User.Provider.FACEBOOK);
- when(userRepository.findByEmail("conflict@example.com")).thenReturn(existingUser);
+ when(userRepository.findWithRolesByEmail("conflict@example.com")).thenReturn(existingUser);
// When/Then
assertThatThrownBy(() -> service.handleOAuthLoginSuccess("google", googleUser))
@@ -413,7 +413,7 @@ void shouldRejectOAuth2LoginForLocalUser() {
existingUser.setEmail("local@example.com");
existingUser.setProvider(User.Provider.LOCAL);
- when(userRepository.findByEmail("local@example.com")).thenReturn(existingUser);
+ when(userRepository.findWithRolesByEmail("local@example.com")).thenReturn(existingUser);
// When/Then
assertThatThrownBy(() -> service.handleOAuthLoginSuccess("google", googleUser))
@@ -434,7 +434,7 @@ void shouldAllowSameProviderReAuthentication() {
existingUser.setProvider(User.Provider.GOOGLE);
existingUser.setFirstName("Existing");
- when(userRepository.findByEmail("same@gmail.com")).thenReturn(existingUser);
+ when(userRepository.findWithRolesByEmail("same@gmail.com")).thenReturn(existingUser);
when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0));
// When - Should not throw exception
@@ -525,7 +525,7 @@ void shouldLoadUserThroughOAuth2RequestFlow() {
DSUserDetails mockUserDetails = mock(DSUserDetails.class);
when(loginHelperService.userLoginHelper(any(User.class), ArgumentMatchers.