diff --git a/CONFIG.md b/CONFIG.md
index 156a1ce..87ed8d2 100644
--- a/CONFIG.md
+++ b/CONFIG.md
@@ -156,6 +156,8 @@ user:
**Enabling step-up also enables factor merging** (`setMfaEnabled(true)` on authentication processing filters), without which re-authenticating replaces the session's authorities instead of merging them. If your application registers its own `AbstractAuthenticationProcessingFilter`, see the warning in `MfaFilterMergingConfiguration`.
+**Session id and CSRF token on re-assertion.** Because a step-up ceremony merges a new factor onto the already-authenticated principal rather than running a fresh authentication, the servlet session id (`JSESSIONID`) is **not** rotated on re-assertion, while the CSRF token **is** rotated (`CsrfAuthenticationStrategy` still runs). This is intended: the principal is unchanged and the session was already authenticated, so there is no pre-authentication session id being upgraded and thus no session-fixation vector to close by rotating it. The client consequence is the CSRF rotation, not the session id: a client that stays on the page and retries the gated operation after step-up must first refresh its CSRF token, or the retry fails CSRF validation. Expose the current token to such clients (for example a `GET` endpoint that returns `CsrfToken`) and read it before the retry.
+
**Accounts with no passkey** (OAuth-only, for example) cannot satisfy `WEBAUTHN` step-up. For them `setPassword` remains governed by `allowInitialPasswordSetWithoutStepUp` below, exactly as before.
**Reserved authority names.** Do not name a role or privilege `FACTOR_*` in `user.roles-and-privileges`. Spring Security uses that prefix for factor authorities, and a plain authority with such a name is indistinguishable from a real factor by name: it satisfies MFA enforcement without the factor ever being completed, and it shadows the genuine factor in a step-up freshness check. Startup fails when such a name is configured while MFA or step-up is enabled, and logs an error otherwise.
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 d42406d..d16d16b 100644
--- a/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java
+++ b/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java
@@ -6,6 +6,7 @@
import java.util.Collection;
import java.util.List;
import java.util.Optional;
+import java.util.function.Function;
import java.util.stream.Collectors;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Value;
@@ -354,15 +355,15 @@ public User registerNewUserAccount(final UserDto newUserDto) {
// Persist through the proxy so the SERIALIZABLE transaction actually applies (a direct
// this.persistNewUserAccount(...) self-invocation would bypass the proxy and run no transaction).
- User saved = persistWithSerializationRetry(user);
+ User saved = persistWithSerializationRetry(user, p -> self.persistNewUserAccount(p));
// authWithoutPassword(saved);
timeLogger.end();
return saved;
}
/**
- * Invokes {@link #persistNewUserAccount(User)} through the proxy, retrying when the SERIALIZABLE
- * transaction fails to serialize (deadlock / lock-acquisition failure,
+ * Invokes a proxied {@code @Transactional(SERIALIZABLE)} persist method through the proxy, retrying
+ * when the transaction fails to serialize (deadlock / lock-acquisition failure,
* {@link ConcurrencyFailureException}).
*
*
@@ -375,12 +376,21 @@ public User registerNewUserAccount(final UserDto newUserDto) {
* (HTTP 500) instead of being misreported as an existing account while no account was created.
*
*
- * @param user the fully built user entity (password already encoded)
+ *
+ * The {@code persister} is a call through {@link #self} to the appropriate proxied persist method
+ * ({@link #persistNewUserAccount(User)} for password accounts,
+ * {@link #persistNewPasswordlessAccount(User)} for passwordless accounts); routing through the
+ * proxy is what makes the SERIALIZABLE transaction actually apply on each attempt.
+ *
+ *
+ * @param prototype the fully built user entity (password already encoded, or {@code null} for
+ * passwordless)
+ * @param persister the proxied persist call to run for each attempt
* @return the saved user entity
* @throws UserAlreadyExistException if an account with the same email already exists
* @throws ConcurrencyFailureException if every attempt fails to serialize
*/
- private User persistWithSerializationRetry(final User prototype) {
+ private User persistWithSerializationRetry(final User prototype, final Function persister) {
ConcurrencyFailureException lastFailure = null;
for (int attempt = 1; attempt <= REGISTRATION_SERIALIZATION_ATTEMPTS; attempt++) {
try {
@@ -388,7 +398,7 @@ private User persistWithSerializationRetry(final User prototype) {
// carrying persistence state (a generated id, Hibernate-managed collections such as
// passwordHistoryEntries), and re-saving that instance fails with optimistic-locking or
// orphan-delete errors instead of performing a clean INSERT.
- return self.persistNewUserAccount(copyForInsert(prototype));
+ return persister.apply(copyForInsert(prototype));
} catch (ConcurrencyFailureException e) {
lastFailure = e;
log.warn("UserService.persistWithSerializationRetry: serialization failure on attempt {}/{} for email {}: {}",
@@ -412,8 +422,10 @@ private User persistWithSerializationRetry(final User prototype) {
/**
* Copies the registration-relevant fields onto a new transient {@link User} for a persist attempt.
- * Only the fields set by {@link #registerNewUserAccount(UserDto)} are copied; everything else keeps
- * its entity default, exactly as on a first attempt.
+ * Only the fields set by the registration entry points ({@link #registerNewUserAccount(UserDto)} and
+ * {@link #registerPasswordlessAccount(PasswordlessRegistrationDto)}) are copied; everything else
+ * keeps its entity default, exactly as on a first attempt. The password is copied verbatim, which is
+ * {@code null} for a passwordless account.
*
* @param prototype the user carrying the registration data
* @return a fresh transient copy safe to persist
@@ -985,13 +997,32 @@ protected void persistInitialPassword(final User user, final String encodedPassw
/**
* Registers a new passwordless user account (no password).
- * Uses SERIALIZABLE isolation to prevent race conditions during concurrent registration.
+ *
+ *
+ * The DB write runs with {@link Isolation#SERIALIZABLE} isolation to close the
+ * duplicate-registration race when two requests register the same email concurrently: a losing
+ * duplicate insert ({@link DataIntegrityViolationException}) is translated into a
+ * {@link UserAlreadyExistException} (HTTP 409). A serialization failure
+ * ({@link CannotAcquireLockException} / {@link ConcurrencyFailureException}) — which can also be
+ * caused by a concurrent registration of a different email deadlocking on index gap locks —
+ * is retried in a fresh transaction (see {@link #persistWithSerializationRetry}); exhausted retries
+ * propagate the failure rather than misreporting it as an existing account. This mirrors the
+ * hardening on {@link #registerNewUserAccount(UserDto)} so that concurrent passwordless
+ * registrations no longer surface a raw deadlock as a generic 500 System Error.
+ *
+ *
+ * @implNote This method is {@link Propagation#NOT_SUPPORTED}: it holds no transaction (and no pooled
+ * connection) across the retry loop, delegating each attempt to a short, separate
+ * SERIALIZABLE transaction. As a result it does not enlist in a caller's
+ * transaction — a consumer's outer {@code @Transactional} is suspended and the
+ * registration commits independently, so an outer rollback will not undo the persisted
+ * user.
*
* @param dto the passwordless registration data
* @return the newly created user entity
* @throws UserAlreadyExistException if an account with the same email already exists
*/
- @Transactional(isolation = Isolation.SERIALIZABLE)
+ @Transactional(propagation = Propagation.NOT_SUPPORTED)
public User registerPasswordlessAccount(final PasswordlessRegistrationDto dto) {
TimeLogger timeLogger = new TimeLogger(log, "UserService.registerPasswordlessAccount");
log.debug("UserService.registerPasswordlessAccount: called for email: {}", dto != null ? dto.getEmail() : null);
@@ -1001,26 +1032,72 @@ public User registerPasswordlessAccount(final PasswordlessRegistrationDto dto) {
// into the REGISTRATION_DENIED response.
evaluateRegistrationGuard(dto.getEmail(), RegistrationSource.PASSWORDLESS, null);
- if (emailExists(dto.getEmail())) {
- log.debug("UserService.registerPasswordlessAccount: email already exists: {}", dto.getEmail());
- throw new UserAlreadyExistException(
- "There is an account with that email address: " + dto.getEmail());
- }
-
User user = new User();
user.setFirstName(dto.getFirstName());
user.setLastName(dto.getLastName());
user.setPassword(null);
user.setEmail(dto.getEmail().toLowerCase());
- user.setRoles(Arrays.asList(roleRepository.findByName(USER_ROLE_NAME)));
if (!sendRegistrationVerificationEmail) {
user.setEnabled(true);
}
- user = userRepository.save(user);
+ // Persist through the proxy so the SERIALIZABLE transaction actually applies, retrying a
+ // serialization failure (deadlock) in a fresh transaction rather than surfacing it as a 500.
+ User saved = persistWithSerializationRetry(user, p -> self.persistNewPasswordlessAccount(p));
timeLogger.end();
- return user;
+ return saved;
+ }
+
+ /**
+ * Persists a new passwordless user account inside a short, serializable transaction.
+ *
+ *
+ * This is the DB-only, passwordless counterpart of {@link #persistNewUserAccount(User)}: it records
+ * no password history because the account has no password. It runs with {@link Isolation#SERIALIZABLE}
+ * to close the duplicate-registration race. The {@link #emailExists} pre-check handles the common
+ * case, but a concurrent insert can still fail at commit: a unique-constraint violation
+ * ({@link DataIntegrityViolationException}) is translated into a {@link UserAlreadyExistException}
+ * (HTTP 409), while a serialization failure ({@link CannotAcquireLockException} /
+ * {@link ConcurrencyFailureException}) propagates unchanged so the caller's retry
+ * ({@link #persistWithSerializationRetry}) can distinguish a same-email race from a different-email
+ * deadlock. Unrelated failures are never swallowed.
+ *
+ *
+ *
+ * Internal seam: it MUST be invoked through the Spring proxy (via {@link #self}) so the transaction
+ * applies, and is {@code protected} (not package-private) so the CGLIB proxy subclass — generated in
+ * a different package — can override and advise it. See {@link #persistNewUserAccount(User)} for the
+ * full rationale.
+ *
+ *
+ * @param user the fully built passwordless user entity (no password)
+ * @return the saved user entity
+ * @throws UserAlreadyExistException if an account with the same email already exists
+ */
+ @Transactional(isolation = Isolation.SERIALIZABLE)
+ protected User persistNewPasswordlessAccount(final User user) {
+ if (emailExists(user.getEmail())) {
+ log.debug("UserService.persistNewPasswordlessAccount: email already exists: {}", user.getEmail());
+ throw new UserAlreadyExistException(
+ "There is an account with that email address: " + user.getEmail());
+ }
+
+ user.setRoles(Arrays.asList(roleRepository.findByName(USER_ROLE_NAME)));
+
+ try {
+ return userRepository.save(user);
+ } catch (DataIntegrityViolationException e) {
+ // A concurrent registration of the SAME email won the race: the unique-email constraint was
+ // violated. Translate to a 409 instead of a 500. A ConcurrencyFailureException (deadlock /
+ // serialization failure) is deliberately NOT translated here: it can be caused by a concurrent
+ // registration of a DIFFERENT email, so it propagates to the retry in
+ // persistWithSerializationRetry, whose fresh-transaction pre-check distinguishes the two cases.
+ log.debug("UserService.persistNewPasswordlessAccount: concurrent duplicate registration detected for email {}: {}",
+ user.getEmail(), e.getClass().getSimpleName());
+ throw new UserAlreadyExistException(
+ "There is an account with that email address: " + user.getEmail());
+ }
}
/**
diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/AbstractConcurrentRegistrationTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/AbstractConcurrentRegistrationTest.java
index 5703971..2b9f72f 100644
--- a/src/test/java/com/digitalsanctuary/spring/user/service/AbstractConcurrentRegistrationTest.java
+++ b/src/test/java/com/digitalsanctuary/spring/user/service/AbstractConcurrentRegistrationTest.java
@@ -1,6 +1,7 @@
package com.digitalsanctuary.spring.user.service;
import static org.assertj.core.api.Assertions.assertThat;
+import com.digitalsanctuary.spring.user.dto.PasswordlessRegistrationDto;
import com.digitalsanctuary.spring.user.dto.UserDto;
import com.digitalsanctuary.spring.user.exceptions.UserAlreadyExistException;
import com.digitalsanctuary.spring.user.persistence.model.User;
@@ -181,6 +182,111 @@ void shouldRegisterEveryUserWhenThreadsRaceDifferentEmails() throws InterruptedE
}
}
+ @RepeatedTest(value = 3, name = "{displayName} [run {currentRepetition}/{totalRepetitions}]")
+ @DisplayName("should serialize concurrent duplicate passwordless registration into exactly one user and one UserAlreadyExistException")
+ void shouldSerializeConcurrentDuplicatePasswordlessRegistrationWhenTwoThreadsRaceSameEmail() throws InterruptedException {
+ final String email = "pwless-race-" + System.nanoTime() + "@test.com";
+
+ final int threadCount = 2;
+ final CountDownLatch readyLatch = new CountDownLatch(threadCount);
+ final CountDownLatch startLatch = new CountDownLatch(1);
+ final ExecutorService executor = Executors.newFixedThreadPool(threadCount);
+
+ try {
+ final List> futures = new ArrayList<>();
+ for (int i = 0; i < threadCount; i++) {
+ futures.add(executor.submit(passwordlessRegistrationTask(email, readyLatch, startLatch)));
+ }
+
+ assertThat(readyLatch.await(30, TimeUnit.SECONDS))
+ .as("both passwordless registration threads should reach the start gate")
+ .isTrue();
+ startLatch.countDown();
+
+ final AtomicInteger successCount = new AtomicInteger();
+ final AtomicInteger alreadyExistCount = new AtomicInteger();
+ final List unexpectedFailures = new ArrayList<>();
+
+ for (Future future : futures) {
+ final RegistrationOutcome outcome = collect(future);
+ if (outcome.user != null) {
+ successCount.incrementAndGet();
+ } else if (outcome.error instanceof UserAlreadyExistException) {
+ alreadyExistCount.incrementAndGet();
+ } else {
+ unexpectedFailures.add(outcome.error);
+ }
+ }
+
+ assertThat(unexpectedFailures)
+ .as("neither thread should fail with a raw serialization/constraint exception (it must be "
+ + "translated to UserAlreadyExistException, not surfaced as a 500)")
+ .isEmpty();
+ assertThat(successCount.get())
+ .as("exactly one thread should successfully register the passwordless user")
+ .isEqualTo(1);
+ assertThat(alreadyExistCount.get())
+ .as("the losing thread should fail with the handled UserAlreadyExistException")
+ .isEqualTo(1);
+
+ final long rowCount = userRepository.findAll().stream()
+ .filter(u -> email.toLowerCase().equals(u.getEmail()))
+ .count();
+ assertThat(rowCount)
+ .as("the database must contain EXACTLY ONE user row for the raced email")
+ .isEqualTo(1);
+ } finally {
+ executor.shutdownNow();
+ }
+ }
+
+ @RepeatedTest(value = 3, name = "{displayName} [run {currentRepetition}/{totalRepetitions}]")
+ @DisplayName("should register every passwordless user when threads race with different emails")
+ void shouldRegisterEveryPasswordlessUserWhenThreadsRaceDifferentEmails() throws InterruptedException {
+ // Distinct emails cannot conflict logically, but their SERIALIZABLE transactions can still deadlock on
+ // index gap locks. Before the passwordless serialization retry existed, that deadlock surfaced as a
+ // generic 500 System Error on POST /user/registration/passwordless (issue #368).
+ final int threadCount = 6;
+ final CountDownLatch readyLatch = new CountDownLatch(threadCount);
+ final CountDownLatch startLatch = new CountDownLatch(1);
+ final ExecutorService executor = Executors.newFixedThreadPool(threadCount);
+
+ try {
+ final List emails = new ArrayList<>();
+ final List> futures = new ArrayList<>();
+ for (int i = 0; i < threadCount; i++) {
+ final String email = "pwless-distinct-" + i + "-" + System.nanoTime() + "@test.com";
+ emails.add(email);
+ futures.add(executor.submit(passwordlessRegistrationTask(email, readyLatch, startLatch)));
+ }
+
+ assertThat(readyLatch.await(30, TimeUnit.SECONDS))
+ .as("all passwordless registration threads should reach the start gate")
+ .isTrue();
+ startLatch.countDown();
+
+ final List failures = new ArrayList<>();
+ for (Future future : futures) {
+ final RegistrationOutcome outcome = collect(future);
+ if (outcome.user == null) {
+ failures.add(outcome.error);
+ }
+ }
+
+ assertThat(failures)
+ .as("every distinct-email passwordless registration must succeed — a deadlock between them "
+ + "must be retried, never surfaced as a 500")
+ .isEmpty();
+ for (String email : emails) {
+ assertThat(userRepository.findByEmail(email.toLowerCase()))
+ .as("passwordless user row should exist for %s", email)
+ .isNotNull();
+ }
+ } finally {
+ executor.shutdownNow();
+ }
+ }
+
private Callable registrationTask(final String email, final CountDownLatch readyLatch,
final CountDownLatch startLatch) {
return () -> {
@@ -204,6 +310,27 @@ private Callable registrationTask(final String email, final
};
}
+ private Callable passwordlessRegistrationTask(final String email, final CountDownLatch readyLatch,
+ final CountDownLatch startLatch) {
+ return () -> {
+ final PasswordlessRegistrationDto dto = new PasswordlessRegistrationDto();
+ dto.setFirstName("Race");
+ dto.setLastName("Condition");
+ dto.setEmail(email);
+
+ readyLatch.countDown();
+ if (!startLatch.await(30, TimeUnit.SECONDS)) {
+ throw new IllegalStateException("start gate was never opened");
+ }
+
+ try {
+ return RegistrationOutcome.success(userService.registerPasswordlessAccount(dto));
+ } catch (Throwable t) {
+ return RegistrationOutcome.failure(t);
+ }
+ };
+ }
+
private RegistrationOutcome collect(final Future future) {
try {
return future.get(60, TimeUnit.SECONDS);
diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java
index ec54c9d..12206e3 100644
--- a/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java
+++ b/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java
@@ -1118,6 +1118,100 @@ void shouldThrowWhenEmailExists() {
.isInstanceOf(UserAlreadyExistException.class)
.hasMessageContaining("There is an account with that email address");
}
+
+ @Test
+ @DisplayName("retries and succeeds when a serialization failure is transient (different-email deadlock)")
+ void shouldRetryAndSucceedWhenSerializationFailureIsTransient() {
+ // Given: pre-check passes; the first SERIALIZABLE attempt deadlocks (e.g. against a concurrent
+ // registration of a DIFFERENT email), the retry succeeds. Before the retry existed this
+ // surfaced as a raw deadlock -> generic 500 System Error on POST /user/registration/passwordless.
+ PasswordlessRegistrationDto dto = new PasswordlessRegistrationDto();
+ dto.setFirstName("Race");
+ dto.setLastName("Condition");
+ dto.setEmail("passwordless-deadlock@example.com");
+
+ Role userRole = RoleTestDataBuilder.aUserRole().build();
+ when(roleRepository.findByName(USER_ROLE_NAME)).thenReturn(userRole);
+ when(userRepository.findByEmail(anyString())).thenReturn(null);
+ when(userRepository.save(any(User.class)))
+ .thenThrow(new CannotAcquireLockException("deadlock"))
+ .thenAnswer(invocation -> invocation.getArgument(0));
+
+ // When
+ User saved = userService.registerPasswordlessAccount(dto);
+
+ // Then
+ assertThat(saved).isNotNull();
+ assertThat(saved.getEmail()).isEqualTo("passwordless-deadlock@example.com");
+ assertThat(saved.getPassword()).isNull();
+ verify(userRepository, org.mockito.Mockito.times(2)).save(any(User.class));
+ verify(passwordEncoder, never()).encode(anyString());
+ }
+
+ @Test
+ @DisplayName("translates DataIntegrityViolationException from save into UserAlreadyExistException")
+ void shouldTranslateDataIntegrityViolationToUserAlreadyExist() {
+ // Given: the pre-check passes but a concurrent SAME-email registration wins the race, so the
+ // insert violates the unique-email constraint. That must become a 409, not a 500.
+ PasswordlessRegistrationDto dto = new PasswordlessRegistrationDto();
+ dto.setFirstName("Race");
+ dto.setLastName("Condition");
+ dto.setEmail("passwordless-dup@example.com");
+
+ Role userRole = RoleTestDataBuilder.aUserRole().build();
+ when(roleRepository.findByName(USER_ROLE_NAME)).thenReturn(userRole);
+ when(userRepository.findByEmail(anyString())).thenReturn(null);
+ when(userRepository.save(any(User.class)))
+ .thenThrow(new DataIntegrityViolationException("unique constraint violation"));
+
+ // When & Then
+ assertThatThrownBy(() -> userService.registerPasswordlessAccount(dto))
+ .isInstanceOf(UserAlreadyExistException.class)
+ .hasMessageContaining("There is an account with that email address");
+ }
+
+ @Test
+ @DisplayName("propagates ConcurrencyFailureException when retries are exhausted")
+ void shouldPropagateConcurrencyFailureWhenRetriesExhausted() {
+ // Given: every SERIALIZABLE attempt deadlocks. The failure must surface honestly (a 500 the
+ // caller can retry), never a fake "already exists" 409 while no account was created.
+ PasswordlessRegistrationDto dto = new PasswordlessRegistrationDto();
+ dto.setFirstName("Race");
+ dto.setLastName("Condition");
+ dto.setEmail("passwordless-persistent-deadlock@example.com");
+
+ Role userRole = RoleTestDataBuilder.aUserRole().build();
+ when(roleRepository.findByName(USER_ROLE_NAME)).thenReturn(userRole);
+ when(userRepository.findByEmail(anyString())).thenReturn(null);
+ when(userRepository.save(any(User.class)))
+ .thenThrow(new CannotAcquireLockException("persistent deadlock"));
+
+ // When & Then
+ assertThatThrownBy(() -> userService.registerPasswordlessAccount(dto))
+ .isInstanceOf(org.springframework.dao.ConcurrencyFailureException.class);
+ verify(userRepository, org.mockito.Mockito.times(5)).save(any(User.class));
+ }
+
+ @Test
+ @DisplayName("does not swallow unrelated runtime exceptions from save")
+ void shouldNotSwallowUnrelatedExceptions() {
+ // Given: an unrelated failure must propagate, not be translated to a 409.
+ PasswordlessRegistrationDto dto = new PasswordlessRegistrationDto();
+ dto.setFirstName("Race");
+ dto.setLastName("Condition");
+ dto.setEmail("passwordless-unrelated@example.com");
+
+ Role userRole = RoleTestDataBuilder.aUserRole().build();
+ when(roleRepository.findByName(USER_ROLE_NAME)).thenReturn(userRole);
+ when(userRepository.findByEmail(anyString())).thenReturn(null);
+ when(userRepository.save(any(User.class)))
+ .thenThrow(new IllegalStateException("unrelated failure"));
+
+ // When & Then
+ assertThatThrownBy(() -> userService.registerPasswordlessAccount(dto))
+ .isInstanceOf(IllegalStateException.class)
+ .hasMessageContaining("unrelated failure");
+ }
}
@Nested