diff --git a/build.gradle b/build.gradle index 13a77f8..b010207 100644 --- a/build.gradle +++ b/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' diff --git a/playwright/tests/auth/registration.spec.ts b/playwright/tests/auth/registration.spec.ts index b2c6ed3..df7415f 100644 --- a/playwright/tests/auth/registration.spec.ts +++ b/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 ({ diff --git a/src/main/java/com/digitalsanctuary/spring/demo/service/CustomUserEmailService.java b/src/main/java/com/digitalsanctuary/spring/demo/service/CustomUserEmailService.java index 98e8083..067701b 100644 --- a/src/main/java/com/digitalsanctuary/spring/demo/service/CustomUserEmailService.java +++ b/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 diff --git a/src/main/java/com/digitalsanctuary/spring/demo/test/api/TestDataController.java b/src/main/java/com/digitalsanctuary/spring/demo/test/api/TestDataController.java index 4fc447b..ffbb08c 100644 --- a/src/main/java/com/digitalsanctuary/spring/demo/test/api/TestDataController.java +++ b/src/main/java/com/digitalsanctuary/spring/demo/test/api/TestDataController.java @@ -236,7 +236,7 @@ public ResponseEntity> 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); diff --git a/src/main/java/com/digitalsanctuary/spring/demo/user/profile/UserProfileDeletionListener.java b/src/main/java/com/digitalsanctuary/spring/demo/user/profile/UserProfileDeletionListener.java index a24e75c..804e060 100644 --- a/src/main/java/com/digitalsanctuary/spring/demo/user/profile/UserProfileDeletionListener.java +++ b/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) diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/UserApiIntegrationTestFixed.java b/src/test/java/com/digitalsanctuary/spring/user/api/UserApiIntegrationTestFixed.java index cf58cfb..bd48b1e 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/UserApiIntegrationTestFixed.java +++ b/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 diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/UserApiSimpleTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/UserApiSimpleTest.java index d999e01..b7e828a 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/UserApiSimpleTest.java +++ b/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 diff --git a/src/test/java/com/digitalsanctuary/spring/user/integration/AuthorityServiceIntegrationTest.java b/src/test/java/com/digitalsanctuary/spring/user/integration/AuthorityServiceIntegrationTest.java index f837612..6535537 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/integration/AuthorityServiceIntegrationTest.java +++ b/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"); diff --git a/src/test/java/com/digitalsanctuary/spring/user/integration/DSUserDetailsServiceIntegrationTest.java b/src/test/java/com/digitalsanctuary/spring/user/integration/DSUserDetailsServiceIntegrationTest.java index 6d06a36..e2547a4 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/integration/DSUserDetailsServiceIntegrationTest.java +++ b/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(); diff --git a/src/test/java/com/digitalsanctuary/spring/user/integration/EventSystemIntegrationTest.java b/src/test/java/com/digitalsanctuary/spring/user/integration/EventSystemIntegrationTest.java index ab5bbc7..f48dc11 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/integration/EventSystemIntegrationTest.java +++ b/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,15 +90,16 @@ 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); } @@ -104,22 +107,22 @@ void registrationEvent_triggersEmailService() throws Exception { @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(); diff --git a/src/test/java/com/digitalsanctuary/spring/user/integration/SecurityConfigurationTest.java b/src/test/java/com/digitalsanctuary/spring/user/integration/SecurityConfigurationTest.java index 218aead..3d73501 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/integration/SecurityConfigurationTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/integration/SecurityConfigurationTest.java @@ -68,6 +68,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 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 (added in 5.0.0). + roleRepository.flush(); // Create role Role userRole = new Role("ROLE_USER");