Merge pull request #74 from devondragon/chore/upgrade-ds-5.0.1

Upgrade to ds-spring-user-framework 5.0.1

Author: devondragon

build.gradle

@@ -39,7 +39,7 @@ repositories {
 
 dependencies {
     // DigitalSanctuary Spring User Framework
-    implementation 'com.digitalsanctuary:ds-spring-user-framework:4.4.0'
+    implementation 'com.digitalsanctuary:ds-spring-user-framework:5.0.1'
 
     // WebAuthn support (Passkey authentication)
     implementation 'org.springframework.security:spring-security-webauthn'

playwright/tests/auth/registration.spec.ts

@@ -60,7 +60,7 @@ test.describe('Registration', () => {
   });
 
   test.describe('Validation', () => {
-    test('should reject registration with existing email', async ({
+    test('should not reveal account existence when registering with an existing email (anti-enumeration)', async ({
       page,
       registerPage,
       testApiClient,
@@ -87,13 +87,32 @@ test.describe('Registration', () => {
         existingUser.password
       );
       await registerPage.acceptTerms();
-      await registerPage.submit();
 
-      // Registration uses async fetch — wait for the error element to appear
-      // rather than waiting for page navigation (which doesn't happen)
-      const globalError = page.locator('#globalError');
-      const existingAccountError = page.locator('#existingAccountError');
-      await expect(globalError.or(existingAccountError)).toBeVisible({ timeout: 10000 });
+      // SpringUserFramework 5.0.0 (task 4.2): registering an existing email returns the SAME
+      // uniform 200 response as a brand-new registration — it must NOT reveal that the account
+      // already exists. (Prior versions returned 409 and surfaced #globalError /
+      // #existingAccountError, which leaked account existence to an attacker.)
+      const [response] = await Promise.all([
+        page.waitForResponse(
+          (r) =>
+            r.url().endsWith('/user/registration') &&
+            r.request().method() === 'POST'
+        ),
+        registerPage.submit(),
+      ]);
+      expect(response.status()).toBe(200);
+
+      // No error revealing the existing account is shown...
+      await expect(page.locator('#globalError')).toBeHidden();
+      await expect(page.locator('#existingAccountError')).toBeHidden();
+
+      // ...and the flow lands on the generic pending page, indistinguishable from a
+      // new (unverified) registration.
+      await page.waitForURL(/registration-pending/, { timeout: 10000 });
+
+      // The original account is untouched — no duplicate created or overwritten.
+      const userExists = await testApiClient.userExists(existingUser.email);
+      expect(userExists.exists).toBe(true);
     });
 
     test('should reject mismatched passwords', async ({

src/main/java/com/digitalsanctuary/spring/demo/service/CustomUserEmailService.java

@@ -9,6 +9,7 @@
 import org.springframework.stereotype.Service;
 
 import com.digitalsanctuary.spring.user.persistence.repository.PasswordResetTokenRepository;
+import com.digitalsanctuary.spring.user.persistence.repository.UserRepository;
 import com.digitalsanctuary.spring.user.persistence.model.User;
 import com.digitalsanctuary.spring.user.mail.MailService;
 import com.digitalsanctuary.spring.user.service.SessionInvalidationService;
@@ -36,10 +37,11 @@ public CustomUserEmailService(
             MailService mailService,
             UserVerificationService userVerificationService,
             PasswordResetTokenRepository passwordTokenRepository,
+            UserRepository userRepository,
             ApplicationEventPublisher eventPublisher,
             SessionInvalidationService sessionInvalidationService,
             TokenHasher tokenHasher) {
-        super(mailService, userVerificationService, passwordTokenRepository, eventPublisher, sessionInvalidationService, tokenHasher);
+        super(mailService, userVerificationService, passwordTokenRepository, userRepository, eventPublisher, sessionInvalidationService, tokenHasher);
     }
 
     @Override

src/main/java/com/digitalsanctuary/spring/demo/test/api/TestDataController.java

@@ -236,7 +236,7 @@ public ResponseEntity<Map<String, Object>> deleteTestUser(@RequestParam String e
 
         // Let framework listeners clean up their data first (e.g. WebAuthn credentials and user
         // entities, which have a foreign key on the user account)
-        eventPublisher.publishEvent(new UserPreDeleteEvent(this, user));
+        eventPublisher.publishEvent(new UserPreDeleteEvent(this, user.getId(), user.getEmail()));
 
         // Delete related entities first to avoid foreign key constraints
         demoUserProfileRepository.findById(user.getId()).ifPresent(demoUserProfileRepository::delete);

src/main/java/com/digitalsanctuary/spring/demo/user/profile/UserProfileDeletionListener.java

@@ -21,7 +21,7 @@ public class UserProfileDeletionListener {
     @EventListener
     @Transactional // Joins the transaction started by UserService.deleteUserAccount
     public void handleUserPreDelete(UserPreDeleteEvent event) {
-        Long userId = event.getUser().getId();
+        Long userId = event.getUserId();
         log.info("Received UserPreDeleteEvent for userId: {}. Deleting associated DemoUserProfile...", userId);
 
         // Option 1: Delete profile directly (if no further cascades needed from profile)

src/test/java/com/digitalsanctuary/spring/user/api/UserApiIntegrationTestFixed.java

@@ -12,6 +12,7 @@
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc;
 import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.dao.DataAccessException;
 import org.springframework.http.MediaType;
 import org.springframework.test.context.ActiveProfiles;
 import org.springframework.test.context.bean.override.mockito.MockitoBean;
@@ -99,15 +100,35 @@ void tearDown() {
      * would roll back with the test and never actually remove the committed registration row.
      */
     private void deleteTestUserCommitted() {
-        TransactionTemplate tx = new TransactionTemplate(transactionManager);
-        tx.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW);
-        tx.executeWithoutResult(status -> {
-            User existing = userRepository.findByEmail(TEST_EMAIL);
-            if (existing != null) {
-                verificationTokenRepository.deleteByUser(existing);
-                userRepository.delete(existing);
+        // Registration creates the verification token via an @Async listener (the demo app enables @Async on
+        // UserDemoApplication), so the token can be written shortly AFTER the registration request returns. A
+        // single committed delete can race that write: deleteByUser runs before the token row exists, then the
+        // user delete trips FK_VERIFY_USER. Retry the committed cleanup until the async token has settled and
+        // the delete succeeds (bounded so a genuine failure still surfaces).
+        DataAccessException last = null;
+        for (int attempt = 0; attempt < 10; attempt++) {
+            try {
+                TransactionTemplate tx = new TransactionTemplate(transactionManager);
+                tx.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW);
+                tx.executeWithoutResult(status -> {
+                    User existing = userRepository.findByEmail(TEST_EMAIL);
+                    if (existing != null) {
+                        verificationTokenRepository.deleteByUser(existing);
+                        userRepository.delete(existing);
+                    }
+                });
+                return;
+            } catch (DataAccessException ex) {
+                last = ex;
+                try {
+                    Thread.sleep(100);
+                } catch (InterruptedException ie) {
+                    Thread.currentThread().interrupt();
+                    throw new IllegalStateException("Interrupted while cleaning up the committed test user", ie);
+                }
             }
-        });
+        }
+        throw new IllegalStateException("Failed to delete the committed test user after retries", last);
     }
 
     @Test
@@ -118,7 +139,7 @@ void shouldRegisterNewUser() throws Exception {
                 .perform(post(API_BASE_PATH + "/registration").contentType(MediaType.APPLICATION_JSON)
                         .content(objectMapper.writeValueAsString(testUserDto)).with(csrf()))
                 .andExpect(status().isOk()).andExpect(jsonPath("$.success").value(true))
-                .andExpect(jsonPath("$.messages[0]").value("Registration Successful!")).andReturn();
+                .andExpect(jsonPath("$.messages[0]").value("If your email address is eligible, you will receive a verification email shortly.")).andReturn();
 
         // Then - Verify user was created
         User savedUser = userRepository.findByEmail("test@example.com");
@@ -129,16 +150,18 @@ void shouldRegisterNewUser() throws Exception {
     }
 
     @Test
-    @DisplayName("Should return conflict for duplicate email")
-    void shouldReturnConflictForDuplicateEmail() throws Exception {
+    @DisplayName("Should not reveal account existence for duplicate email (anti-enumeration)")
+    void shouldNotRevealAccountExistenceForDuplicateEmail() throws Exception {
         // Given - Register first user
         userService.registerNewUserAccount(testUserDto);
 
         // When - Try to register with same email
+        // Then - anti-enumeration: a duplicate email returns the SAME generic 200 success body as a new
+        // registration, so a caller cannot distinguish an already-registered address from a new one.
         mockMvc.perform(post(API_BASE_PATH + "/registration").contentType(MediaType.APPLICATION_JSON)
-                .content(objectMapper.writeValueAsString(testUserDto)).with(csrf())).andExpect(status().isConflict())
-                .andExpect(jsonPath("$.success").value(false))
-                .andExpect(jsonPath("$.messages[0]").value("An account already exists for the email address"));
+                .content(objectMapper.writeValueAsString(testUserDto)).with(csrf())).andExpect(status().isOk())
+                .andExpect(jsonPath("$.success").value(true))
+                .andExpect(jsonPath("$.messages[0]").value("If your email address is eligible, you will receive a verification email shortly."));
     }
 
     @Test

src/test/java/com/digitalsanctuary/spring/user/api/UserApiSimpleTest.java

@@ -75,8 +75,8 @@ void shouldRegisterNewUser() throws Exception {
         // Then
         result.andExpect(status().isOk())
                 .andExpect(jsonPath("$.success").value(true))
-                .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."));
+
         // Verify user created
         User user = userRepository.findByEmail("simple.test@example.com");
         assertThat(user).isNotNull();
@@ -86,8 +86,8 @@ void shouldRegisterNewUser() throws Exception {
     }
 
     @Test
-    @DisplayName("Should return conflict for duplicate email")
-    void shouldReturnConflictForDuplicateEmail() throws Exception {
+    @DisplayName("Should not reveal account existence for duplicate email (anti-enumeration)")
+    void shouldNotRevealAccountExistenceForDuplicateEmail() throws Exception {
         // Given - Register first user
         UserDto firstUser = new UserDto();
         firstUser.setFirstName("First");
@@ -115,10 +115,11 @@ void shouldReturnConflictForDuplicateEmail() throws Exception {
                 .content(objectMapper.writeValueAsString(duplicateUser))
                 .with(csrf()));
 
-        // Then
-        result.andExpect(status().isConflict())
-                .andExpect(jsonPath("$.success").value(false))
-                .andExpect(jsonPath("$.messages[0]").value("An account already exists for the email address"));
+        // Then - anti-enumeration: a duplicate email returns the SAME generic 200 success body as a new
+        // registration, so a caller cannot distinguish an already-registered address from a new one.
+        result.andExpect(status().isOk())
+                .andExpect(jsonPath("$.success").value(true))
+                .andExpect(jsonPath("$.messages[0]").value("If your email address is eligible, you will receive a verification email shortly."));
     }
 
     @Test

src/test/java/com/digitalsanctuary/spring/user/integration/AuthorityServiceIntegrationTest.java

@@ -61,6 +61,12 @@ void setUp() {
                 userRepository.deleteAll();
                 roleRepository.deleteAll();
                 privilegeRepository.deleteAll();
+                // Force the DELETEs to hit the DB before the role/privilege INSERTs below. The framework seeds the
+                // configured roles and privileges at startup, so without an explicit flush Hibernate's action-queue
+                // ordering would run the new INSERTs before these DELETEs and trip the unique ROLE(NAME) /
+                // PRIVILEGE(NAME) indexes (added in 5.0.0).
+                roleRepository.flush();
+                privilegeRepository.flush();
 
                 // Create privileges as defined in config
                 Privilege loginPrivilege = createAndSavePrivilege("LOGIN_PRIVILEGE", "Allows user login");

src/test/java/com/digitalsanctuary/spring/user/integration/DSUserDetailsServiceIntegrationTest.java

@@ -72,6 +72,10 @@ void setUp() {
         passwordHistoryRepository.deleteAll();
         userRepository.deleteAll();
         roleRepository.deleteAll();
+        // Force the DELETEs to hit the DB before the role INSERTs below. The framework seeds the configured
+        // roles (ROLE_USER/ROLE_ADMIN/...) at startup, so without an explicit flush Hibernate's action-queue
+        // ordering would run the new-role INSERTs before these DELETEs and trip the unique ROLE(NAME) index.
+        roleRepository.flush();
 
         // Create privileges
         Privilege userPrivilege = new Privilege();

src/test/java/com/digitalsanctuary/spring/user/integration/EventSystemIntegrationTest.java

@@ -2,6 +2,8 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.timeout;
 import static org.mockito.Mockito.verify;
@@ -88,38 +90,39 @@ void registrationEvent_triggersEmailService() throws Exception {
             doAnswer(invocation -> {
                 latch.countDown();
                 return null;
-            }).when(userEmailService).sendRegistrationVerificationEmail(any(), any());
+            }).when(userEmailService).sendRegistrationVerificationEmail(anyLong(), anyString());
 
             // When
-            OnRegistrationCompleteEvent event = OnRegistrationCompleteEvent.builder().user(testUser).locale(locale).appUrl(appUrl).build();
+            OnRegistrationCompleteEvent event = OnRegistrationCompleteEvent.builder().userId(testUser.getId()).userEmail(testUser.getEmail())
+                    .userEnabled(testUser.isEnabled()).locale(locale).appUrl(appUrl).build();
             eventPublisher.publishEvent(event);
 
             // Then
             assertThat(latch.await(5, TimeUnit.SECONDS)).isTrue();
-            verify(userEmailService).sendRegistrationVerificationEmail(testUser, appUrl);
+            verify(userEmailService).sendRegistrationVerificationEmail(testUser.getId(), appUrl);
             assertThat(eventCapture.getCapturedEvents()).filteredOn(e -> e instanceof OnRegistrationCompleteEvent).hasSize(1);
         }
 
         @Test
         @DisplayName("Multiple registration events are handled independently")
         void multipleRegistrationEvents_handledIndependently() throws Exception {
             // Given
-            User user1 = UserTestDataBuilder.aUser().withEmail("user1@example.com").build();
-            User user2 = UserTestDataBuilder.aUser().withEmail("user2@example.com").build();
+            User user1 = UserTestDataBuilder.aUser().withId(10L).withEmail("user1@example.com").build();
+            User user2 = UserTestDataBuilder.aUser().withId(20L).withEmail("user2@example.com").build();
             CountDownLatch latch = new CountDownLatch(2);
             doAnswer(invocation -> {
                 latch.countDown();
                 return null;
-            }).when(userEmailService).sendRegistrationVerificationEmail(any(), any());
+            }).when(userEmailService).sendRegistrationVerificationEmail(anyLong(), anyString());
 
             // When
-            eventPublisher.publishEvent(new OnRegistrationCompleteEvent(user1, Locale.ENGLISH, "app1"));
-            eventPublisher.publishEvent(new OnRegistrationCompleteEvent(user2, Locale.FRENCH, "app2"));
+            eventPublisher.publishEvent(new OnRegistrationCompleteEvent(user1.getId(), user1.getEmail(), user1.isEnabled(), Locale.ENGLISH, "app1"));
+            eventPublisher.publishEvent(new OnRegistrationCompleteEvent(user2.getId(), user2.getEmail(), user2.isEnabled(), Locale.FRENCH, "app2"));
 
             // Then
             assertThat(latch.await(5, TimeUnit.SECONDS)).isTrue();
-            verify(userEmailService).sendRegistrationVerificationEmail(user1, "app1");
-            verify(userEmailService).sendRegistrationVerificationEmail(user2, "app2");
+            verify(userEmailService).sendRegistrationVerificationEmail(user1.getId(), "app1");
+            verify(userEmailService).sendRegistrationVerificationEmail(user2.getId(), "app2");
         }
     }
 
@@ -167,14 +170,14 @@ class UserDeletionEventFlowTests {
         @DisplayName("UserPreDeleteEvent is captured correctly")
         void userPreDeleteEvent_capturedCorrectly() {
             // When
-            UserPreDeleteEvent event = new UserPreDeleteEvent(this, testUser);
+            UserPreDeleteEvent event = new UserPreDeleteEvent(this, testUser.getId(), testUser.getEmail());
             eventPublisher.publishEvent(event);
 
             // Then
             assertThat(eventCapture.getCapturedEvents()).filteredOn(e -> e instanceof UserPreDeleteEvent).hasSize(1).first().satisfies(e -> {
                 UserPreDeleteEvent deleteEvent = (UserPreDeleteEvent) e;
-                assertThat(deleteEvent.getUser()).isEqualTo(testUser);
                 assertThat(deleteEvent.getUserId()).isEqualTo(1L);
+                assertThat(deleteEvent.getUserEmail()).isEqualTo(testUser.getEmail());
             });
         }
     }
@@ -217,7 +220,7 @@ void events_processedInOrder() throws Exception {
                 processedEvents.add("registration");
                 latch.countDown();
                 return null;
-            }).when(userEmailService).sendRegistrationVerificationEmail(any(), any());
+            }).when(userEmailService).sendRegistrationVerificationEmail(anyLong(), anyString());
 
             doAnswer(invocation -> {
                 processedEvents.add("login-success");
@@ -226,9 +229,9 @@ void events_processedInOrder() throws Exception {
             }).when(loginAttemptService).loginSucceeded(any());
 
             // When
-            eventPublisher.publishEvent(new OnRegistrationCompleteEvent(testUser, Locale.ENGLISH, "app"));
+            eventPublisher.publishEvent(new OnRegistrationCompleteEvent(testUser.getId(), testUser.getEmail(), testUser.isEnabled(), Locale.ENGLISH, "app"));
             eventPublisher.publishEvent(new AuthenticationSuccessEvent(new UsernamePasswordAuthenticationToken("user", "pass")));
-            eventPublisher.publishEvent(new UserPreDeleteEvent(this, testUser));
+            eventPublisher.publishEvent(new UserPreDeleteEvent(this, testUser.getId(), testUser.getEmail()));
             processedEvents.add("delete");
             latch.countDown();