fix: retry concurrent passwordless registration deadlocks (#368); document step-up session id behavior (#369)

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.

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 @@
 
 		// 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}).
 	 *
 	 * <p>
@@ -375,20 +376,29 @@
 	 * (HTTP 500) instead of being misreported as an existing account while no account was created.
 	 * </p>
 	 *
-	 * @param user the fully built user entity (password already encoded)
+	 * <p>
+	 * 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.
+	 * </p>
+	 *
+	 * @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<User, User> persister) {
 		ConcurrencyFailureException lastFailure = null;
 		for (int attempt = 1; attempt <= REGISTRATION_SERIALIZATION_ATTEMPTS; attempt++) {
 			try {
 				// Each attempt persists a FRESH entity: a rolled-back attempt can leave the passed instance
 				// 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 @@
 
 	/**
 	 * 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 @@
 
 	/**
 	 * Registers a new passwordless user account (no password).
-	 * Uses SERIALIZABLE isolation to prevent race conditions during concurrent registration.
+	 *
+	 * <p>
+	 * 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 <em>different</em> 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.
+	 * </p>
+	 *
+	 * @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 <em>not</em> 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 @@
 		// 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.
+	 *
+	 * <p>
+	 * 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.
+	 * </p>
+	 *
+	 * <p>
+	 * 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.
+	 * </p>
+	 *
+	 * @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());
+		}
 	}
 
 	/**

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<Future<RegistrationOutcome>> 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<Throwable> unexpectedFailures = new ArrayList<>();
+
+			for (Future<RegistrationOutcome> 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<String> emails = new ArrayList<>();
+			final List<Future<RegistrationOutcome>> 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<Throwable> failures = new ArrayList<>();
+			for (Future<RegistrationOutcome> 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<RegistrationOutcome> registrationTask(final String email, final CountDownLatch readyLatch,
 			final CountDownLatch startLatch) {
 		return () -> {
@@ -204,6 +310,27 @@ private Callable<RegistrationOutcome> registrationTask(final String email, final
 		};
 	}
 
+	private Callable<RegistrationOutcome> 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<RegistrationOutcome> future) {
 		try {
 			return future.get(60, TimeUnit.SECONDS);