From 3a29f6d7500cab0ac3703a9a76a76e2e8e16ebb1 Mon Sep 17 00:00:00 2001 From: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> Date: Mon, 31 Aug 2026 16:30:33 +0800 Subject: [PATCH] fix(auth): converge concurrent identity binding Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> --- .../auth/identity/IdentityBindingService.java | 91 +++++++++++++++++-- .../identity/IdentityBindingServiceTest.java | 75 ++++++++++++++- 2 files changed, 156 insertions(+), 10 deletions(-) diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/IdentityBindingService.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/IdentityBindingService.java index e34530ab..a195c9e7 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/IdentityBindingService.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/IdentityBindingService.java @@ -16,8 +16,13 @@ import com.iflytek.skillhub.domain.user.UserAccount; import com.iflytek.skillhub.domain.user.UserAccountRepository; import com.iflytek.skillhub.domain.user.UserStatus; import org.springframework.context.ApplicationEventPublisher; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.dao.DataIntegrityViolationException; import org.springframework.stereotype.Service; -import org.springframework.transaction.annotation.Transactional; +import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.TransactionDefinition; +import org.springframework.transaction.support.TransactionOperations; +import org.springframework.transaction.support.TransactionTemplate; import java.util.UUID; import java.util.Set; import java.util.stream.Collectors; @@ -34,21 +39,46 @@ public class IdentityBindingService { private final UserRoleBindingRepository roleBindingRepo; private final GlobalNamespaceMembershipService globalNamespaceMembershipService; private final ApplicationEventPublisher eventPublisher; + private final TransactionOperations transactions; + @Autowired public IdentityBindingService(IdentityBindingRepository bindingRepo, UserAccountRepository userRepo, UserRoleBindingRepository roleBindingRepo, GlobalNamespaceMembershipService globalNamespaceMembershipService, - ApplicationEventPublisher eventPublisher) { + ApplicationEventPublisher eventPublisher, + PlatformTransactionManager transactionManager) { + this(bindingRepo, userRepo, roleBindingRepo, globalNamespaceMembershipService, eventPublisher, + requiresNewTransactions(transactionManager)); + } + + IdentityBindingService(IdentityBindingRepository bindingRepo, + UserAccountRepository userRepo, + UserRoleBindingRepository roleBindingRepo, + GlobalNamespaceMembershipService globalNamespaceMembershipService, + ApplicationEventPublisher eventPublisher, + TransactionOperations transactions) { this.bindingRepo = bindingRepo; this.userRepo = userRepo; this.roleBindingRepo = roleBindingRepo; this.globalNamespaceMembershipService = globalNamespaceMembershipService; this.eventPublisher = eventPublisher; + this.transactions = transactions; } - @Transactional public PlatformPrincipal bindOrCreate(OAuthClaims claims, UserStatus initialStatus) { + try { + return transactions.execute(status -> bindOrCreateInTransaction(claims, initialStatus)); + } catch (DataIntegrityViolationException conflict) { + PlatformPrincipal winner = transactions.execute(status -> resolveConcurrentWinner(claims)); + if (winner != null) { + return winner; + } + throw conflict; + } + } + + private PlatformPrincipal bindOrCreateInTransaction(OAuthClaims claims, UserStatus initialStatus) { IdentityBinding binding = bindingRepo .findByProviderCodeAndSubject(claims.provider(), claims.subject()) .orElse(null); @@ -73,14 +103,15 @@ public class IdentityBindingService { ); user.setStatus(initialStatus); user = userRepo.save(user); + + binding = new IdentityBinding(user.getId(), claims.provider(), claims.subject(), claims.providerLogin()); + // Force the unique identity coordinate to be checked before membership or events run. + bindingRepo.saveAndFlush(binding); if (initialStatus == UserStatus.ACTIVE) { globalNamespaceMembershipService.ensureMember(user.getId()); eventPublisher.publishEvent( new UserActivatedEvent(user.getId(), claims.providerLogin(), claims.email())); } - - binding = new IdentityBinding(user.getId(), claims.provider(), claims.subject(), claims.providerLogin()); - bindingRepo.save(binding); } ensureExternalLoginAllowed(user); @@ -96,8 +127,16 @@ public class IdentityBindingService { ); } - @Transactional public void createPendingUserIfAbsent(OAuthClaims claims) { + try { + transactions.executeWithoutResult(status -> createPendingUserInTransaction(claims)); + } catch (DataIntegrityViolationException conflict) { + transactions.executeWithoutResult(status -> handleConcurrentPendingWinner(claims, conflict)); + throw conflict; + } + } + + private void createPendingUserInTransaction(OAuthClaims claims) { IdentityBinding existingBinding = bindingRepo .findByProviderCodeAndSubject(claims.provider(), claims.subject()) .orElse(null); @@ -118,7 +157,43 @@ public class IdentityBindingService { user = userRepo.save(user); IdentityBinding binding = new IdentityBinding(user.getId(), claims.provider(), claims.subject(), claims.providerLogin()); - bindingRepo.save(binding); + bindingRepo.saveAndFlush(binding); + } + + private PlatformPrincipal resolveConcurrentWinner(OAuthClaims claims) { + IdentityBinding binding = bindingRepo + .findByProviderCodeAndSubject(claims.provider(), claims.subject()) + .orElse(null); + if (binding == null) { + return null; + } + UserAccount user = userRepo.findById(binding.getUserId()) + .orElseThrow(() -> new IllegalStateException("User not found for binding")); + ensureExternalLoginAllowed(user); + Set roles = roleBindingRepo.findByUserId(user.getId()).stream() + .map(rb -> rb.getRole().getCode()) + .collect(Collectors.toSet()); + roles = PlatformRoleDefaults.withDefaultUserRole(roles); + return new PlatformPrincipal( + user.getId(), user.getDisplayName(), user.getEmail(), + user.getAvatarUrl(), claims.provider(), roles + ); + } + + private void handleConcurrentPendingWinner(OAuthClaims claims, DataIntegrityViolationException conflict) { + IdentityBinding binding = bindingRepo + .findByProviderCodeAndSubject(claims.provider(), claims.subject()) + .orElseThrow(() -> conflict); + UserAccount user = userRepo.findById(binding.getUserId()) + .orElseThrow(() -> new IllegalStateException("User not found for binding")); + ensureExternalLoginAllowed(user); + throw new AccountPendingException(); + } + + private static TransactionOperations requiresNewTransactions(PlatformTransactionManager transactionManager) { + TransactionTemplate template = new TransactionTemplate(transactionManager); + template.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW); + return template; } private String trustedEmail(OAuthClaims claims) { diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/IdentityBindingServiceTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/IdentityBindingServiceTest.java index c72c8022..2f5e3b0e 100644 --- a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/IdentityBindingServiceTest.java +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/IdentityBindingServiceTest.java @@ -33,6 +33,9 @@ import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import org.springframework.context.ApplicationEventPublisher; +import org.springframework.dao.DataIntegrityViolationException; +import org.springframework.transaction.support.TransactionCallback; +import org.springframework.transaction.support.TransactionOperations; import org.springframework.test.util.ReflectionTestUtils; @ExtendWith(MockitoExtension.class) @@ -58,7 +61,7 @@ class IdentityBindingServiceTest { @BeforeEach void setUp() { service = new IdentityBindingService(bindingRepo, userRepo, roleBindingRepo, - globalNamespaceMembershipService, eventPublisher); + globalNamespaceMembershipService, eventPublisher, new ImmediateTransactionOperations()); } @Test @@ -80,7 +83,7 @@ class IdentityBindingServiceTest { ArgumentCaptor userCaptor = ArgumentCaptor.forClass(UserAccount.class); verify(userRepo).save(userCaptor.capture()); verify(globalNamespaceMembershipService).ensureMember(userCaptor.getValue().getId()); - verify(bindingRepo).save(any(IdentityBinding.class)); + verify(bindingRepo).saveAndFlush(any(IdentityBinding.class)); assertThat(principal.displayName()).isEqualTo("alice"); assertThat(principal.oauthProvider()).isEqualTo("github"); } @@ -361,4 +364,72 @@ class IdentityBindingServiceTest { verify(userRepo).save(userCaptor.capture()); assertThat(userCaptor.getValue().getEmail()).isNull(); } + + @Test + void bindOrCreate_concurrentInsertReturnsWinningPrincipal() { + OAuthClaims claims = new OAuthClaims( + "github", "gh_1", "alice@example.com", true, "alice", Map.of() + ); + IdentityBinding winningBinding = new IdentityBinding("usr_winner", "github", "gh_1", "alice"); + UserAccount winningUser = new UserAccount("usr_winner", "alice", "alice@example.com", null); + winningUser.setStatus(UserStatus.ACTIVE); + + when(bindingRepo.findByProviderCodeAndSubject("github", "gh_1")) + .thenReturn(Optional.empty(), Optional.of(winningBinding)); + when(userRepo.save(any(UserAccount.class))).thenAnswer(invocation -> invocation.getArgument(0)); + when(bindingRepo.saveAndFlush(any(IdentityBinding.class))) + .thenThrow(new DataIntegrityViolationException("duplicate binding")); + when(userRepo.findById("usr_winner")).thenReturn(Optional.of(winningUser)); + when(roleBindingRepo.findByUserId("usr_winner")).thenReturn(List.of()); + + PlatformPrincipal principal = service.bindOrCreate(claims, UserStatus.ACTIVE); + + assertThat(principal.userId()).isEqualTo("usr_winner"); + assertThat(principal.platformRoles()).containsExactly("USER"); + verify(globalNamespaceMembershipService, never()).ensureMember(any()); + verify(eventPublisher, never()).publishEvent(any()); + } + + @Test + void bindOrCreate_rethrowsUnexpectedIntegrityFailureWhenNoWinnerExists() { + OAuthClaims claims = new OAuthClaims( + "github", "gh_1", "alice@example.com", true, "alice", Map.of() + ); + when(bindingRepo.findByProviderCodeAndSubject("github", "gh_1")) + .thenReturn(Optional.empty(), Optional.empty()); + when(userRepo.save(any(UserAccount.class))).thenAnswer(invocation -> invocation.getArgument(0)); + when(bindingRepo.saveAndFlush(any(IdentityBinding.class))) + .thenThrow(new DataIntegrityViolationException("unrelated integrity failure")); + + assertThatThrownBy(() -> service.bindOrCreate(claims, UserStatus.ACTIVE)) + .isInstanceOf(DataIntegrityViolationException.class) + .hasMessageContaining("unrelated integrity failure"); + } + + @Test + void createPendingUserIfAbsent_concurrentInsertUsesWinningAccountState() { + OAuthClaims claims = new OAuthClaims( + "github", "gh_1", "alice@example.com", true, "alice", Map.of() + ); + IdentityBinding winningBinding = new IdentityBinding("usr_winner", "github", "gh_1", "alice"); + UserAccount winningUser = new UserAccount("usr_winner", "alice", "alice@example.com", null); + winningUser.setStatus(UserStatus.PENDING); + + when(bindingRepo.findByProviderCodeAndSubject("github", "gh_1")) + .thenReturn(Optional.empty(), Optional.of(winningBinding)); + when(userRepo.save(any(UserAccount.class))).thenAnswer(invocation -> invocation.getArgument(0)); + when(bindingRepo.saveAndFlush(any(IdentityBinding.class))) + .thenThrow(new DataIntegrityViolationException("duplicate binding")); + when(userRepo.findById("usr_winner")).thenReturn(Optional.of(winningUser)); + + assertThatThrownBy(() -> service.createPendingUserIfAbsent(claims)) + .isInstanceOf(AccountPendingException.class); + } + + private static final class ImmediateTransactionOperations implements TransactionOperations { + @Override + public T execute(TransactionCallback action) { + return action.doInTransaction(null); + } + } }