From 8ee5bc70ad03538eedeee7b3aac4deba82cc5457 Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Fri, 21 Aug 2026 16:27:08 -0600 Subject: [PATCH] fix: retry concurrent passwordless registration deadlocks (#368); document step-up session id behavior (#369) Passwordless registration had the same SERIALIZABLE-isolation deadlock as password registration but none of the hardening: registerPasswordlessAccount ran a single @Transactional(SERIALIZABLE) save with no retry and no DataIntegrityViolationException translation, so a concurrent-insert deadlock (including between DIFFERENT emails contending on index gap locks) surfaced as a generic 500 "System Error" on POST /user/registration/passwordless. Mirror the password path: generalize persistWithSerializationRetry to take the proxied persist call, add persistNewPasswordlessAccount (@Transactional(SERIALIZABLE), DataIntegrityViolationException -> 409, no password history), and make registerPasswordlessAccount NOT_SUPPORTED so each retry runs in its own fresh short transaction. Exhausted retries still propagate honestly rather than being misreported as an existing account. Tests: 4 mock-based UserServiceTest cases (retry-then-succeed, DIV->409, exhausted->propagate, unrelated-exception-not-swallowed) and passwordless same-email / different-email cases in AbstractConcurrentRegistrationTest, which run against real MariaDB and PostgreSQL via Testcontainers. Full suite: 1280 tests, 0 failures. #369: document in CONFIG.md that step-up re-assertion preserves the session id (intended: principal unchanged, no fixation vector) while the CSRF token rotates, and that a client retrying after step-up must refresh its CSRF token. Claude-Session: https://claude.ai/code/session_01KL5qvKVGQLLQDjHToy34vj --- CONFIG.md | 2 + .../spring/user/service/UserService.java | 115 +++++++++++++--- .../AbstractConcurrentRegistrationTest.java | 127 ++++++++++++++++++ .../spring/user/service/UserServiceTest.java | 94 +++++++++++++ 4 files changed, 319 insertions(+), 19 deletions(-) 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