Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
33 changes: 26 additions & 7 deletions playwright/tests/auth/registration.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 });

Comment on lines +109 to +112
// 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 ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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");
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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");
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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");
}
}

Expand Down Expand Up @@ -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());
});
}
}
Expand Down Expand Up @@ -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");
Expand All @@ -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();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
Loading