Merge pull request #786 from iflytek/fix/issue-613-oauth-concurrency-20260831

fix(auth): converge concurrent identity binding

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<String> 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) {

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<UserAccount> 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> T execute(TransactionCallback<T> action) {
+            return action.doInTransaction(null);
+        }
+    }
 }