From b7cfbe193ba6936fc2d85035abc2bff25ac33faa Mon Sep 17 00:00:00 2001 From: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> Date: Thu, 8 Oct 2026 13:56:23 +0800 Subject: [PATCH] fix(auth): serialize account merges and revoke secondary tokens Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> --- docs/03-authentication-design.md | 3 + .../AccountMergeFlowIntegrationTest.java | 138 ++++++++++++++++++ .../auth/merge/AccountMergeService.java | 24 ++- .../auth/merge/AccountMergeServiceTest.java | 11 +- .../domain/user/UserAccountRepository.java | 1 + .../infra/jpa/UserAccountJpaRepository.java | 9 ++ 6 files changed, 173 insertions(+), 13 deletions(-) diff --git a/docs/03-authentication-design.md b/docs/03-authentication-design.md index d847dd64..c55c2cc2 100644 --- a/docs/03-authentication-design.md +++ b/docs/03-authentication-design.md @@ -438,6 +438,9 @@ public class OAuthClaimsExtractor { 发起账号创建 30 分钟内有效的请求；待合并账号在自己的已认证会话中查看合并目标并批准； 最后发起账号在批准后 30 分钟内确认迁移；任一账号可在完成前撤销请求。 请求 ID 只用于定位请求，不是第二个账号的所有权凭据。 +确认合并时，对两个账号行按 ID 顺序加锁并重新检查状态，防止多个已批准请求并发完成。 +待合并账号已签发的 API Token 在合并时吊销，不转移给主账号；否则旧 Token 会继承主账号权限。 +主账号原有 Token 不变，需要自动化访问的调用方应重新签发 Token。 账号合并页面当前仍未开放，`/settings/accounts` 保持重定向；上述流程由 API 强制执行。 - 一期 GitHub-only：不需要自动合并，每个 Provider 登录独立创建用户 diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/AccountMergeFlowIntegrationTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/AccountMergeFlowIntegrationTest.java index 6242ae69..07f43521 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/AccountMergeFlowIntegrationTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/AccountMergeFlowIntegrationTest.java @@ -15,6 +15,7 @@ import com.iflytek.skillhub.auth.merge.AccountMergeRequest; import com.iflytek.skillhub.auth.merge.AccountMergeRequestRepository; import com.iflytek.skillhub.auth.merge.AccountMergeService; import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; +import com.iflytek.skillhub.auth.token.ApiTokenService; import com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository; import com.iflytek.skillhub.domain.user.UserAccount; import com.iflytek.skillhub.domain.user.UserAccountRepository; @@ -72,6 +73,7 @@ class AccountMergeFlowIntegrationTest { @Autowired private LocalCredentialRepository localCredentialRepository; @Autowired private AccountMergeRequestRepository mergeRequestRepository; @Autowired private AccountMergeService mergeService; + @Autowired private ApiTokenService apiTokenService; @Autowired private JdbcTemplate jdbcTemplate; @Autowired private PlatformTransactionManager transactionManager; @MockBean private NamespaceMemberRepository namespaceMemberRepository; @@ -85,6 +87,8 @@ class AccountMergeFlowIntegrationTest { userAccountRepository.save(new UserAccount(primaryId, "Primary", null, null)); userAccountRepository.save(new UserAccount(secondaryId, "Secondary", null, null)); localCredentialRepository.save(new LocalCredential(secondaryId, secondaryUsername, "hash")); + String secondaryToken = apiTokenService.createToken(secondaryId, "secondary-automation", "[]").rawToken(); + String primaryToken = apiTokenService.createToken(primaryId, "primary-automation", "[]").rawToken(); String initiateResponse = mockMvc.perform(post("/api/v1/account/merge/initiate") .with(authentication(auth(primaryId))) @@ -124,6 +128,60 @@ class AccountMergeFlowIntegrationTest { assertThat(userAccountRepository.findById(secondaryId).orElseThrow().getStatus()).isEqualTo(UserStatus.MERGED); assertThat(localCredentialRepository.findByUsernameIgnoreCase(secondaryUsername).orElseThrow().getUserId()) .isEqualTo(primaryId); + assertThat(apiTokenService.validateToken(secondaryToken)).isEmpty(); + assertThat(apiTokenService.validateToken(primaryToken)).isPresent(); + mockMvc.perform(get("/api/v1/auth/me").header("Authorization", "Bearer " + secondaryToken)) + .andExpect(status().isUnauthorized()); + } + + @Test + void directApiCallsCannotSpoofAnotherAccountOrBypassCsrf() throws Exception { + String suffix = UUID.randomUUID().toString(); + String primaryId = "merge-primary-" + suffix; + String secondaryId = "merge-secondary-" + suffix; + String outsiderId = "merge-outsider-" + suffix; + String secondaryUsername = "merge-" + suffix; + userAccountRepository.save(new UserAccount(primaryId, "Primary", null, null)); + userAccountRepository.save(new UserAccount(secondaryId, "Secondary", null, null)); + userAccountRepository.save(new UserAccount(outsiderId, "Outsider", null, null)); + localCredentialRepository.save(new LocalCredential(secondaryId, secondaryUsername, "hash")); + + String initiateResponse = mockMvc.perform(post("/api/v1/account/merge/initiate") + .with(authentication(auth(primaryId))).with(csrf()) + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(java.util.Map.of( + "primaryUserId", outsiderId, "secondaryIdentifier", secondaryUsername)))) + .andExpect(status().isOk()) + .andReturn().getResponse().getContentAsString(); + long requestId = objectMapper.readTree(initiateResponse).path("data").path("mergeRequestId").asLong(); + AccountMergeRequest request = mergeRequestRepository.findById(requestId).orElseThrow(); + assertThat(request.getPrimaryUserId()).isEqualTo(primaryId); + String spoofedBody = objectMapper.writeValueAsString(java.util.Map.of( + "mergeRequestId", requestId, "secondaryUserId", secondaryId, "primaryUserId", primaryId)); + + mockMvc.perform(get("/api/v1/account/merge/requests/{id}", requestId)) + .andExpect(status().isUnauthorized()); + mockMvc.perform(get("/api/v1/account/merge/requests/{id}", requestId) + .with(authentication(auth(outsiderId)))) + .andExpect(status().isNotFound()); + mockMvc.perform(post("/api/v1/account/merge/verify") + .with(authentication(auth(outsiderId))).with(csrf()) + .contentType(MediaType.APPLICATION_JSON).content(spoofedBody)) + .andExpect(status().isNotFound()); + mockMvc.perform(post("/api/v1/account/merge/confirm") + .with(authentication(auth(outsiderId))).with(csrf()) + .contentType(MediaType.APPLICATION_JSON).content(spoofedBody)) + .andExpect(status().isNotFound()); + mockMvc.perform(post("/api/v1/account/merge/cancel") + .with(authentication(auth(outsiderId))).with(csrf()) + .contentType(MediaType.APPLICATION_JSON).content(spoofedBody)) + .andExpect(status().isNotFound()); + mockMvc.perform(post("/api/v1/account/merge/verify") + .with(authentication(auth(secondaryId))) + .contentType(MediaType.APPLICATION_JSON).content(spoofedBody)) + .andExpect(status().is4xxClientError()); + assertThat(mergeRequestRepository.findById(requestId).orElseThrow().getStatus()) + .isEqualTo(AccountMergeRequest.STATUS_PENDING); } @Test @@ -246,6 +304,86 @@ class AccountMergeFlowIntegrationTest { } } + @Test + void twoApprovedDestinationsCannotBothMergeTheSameAccount() throws Exception { + String suffix = UUID.randomUUID().toString(); + String firstPrimaryId = "merge-first-" + suffix; + String secondPrimaryId = "merge-second-" + suffix; + String secondaryId = "merge-secondary-" + suffix; + String secondaryUsername = "merge-" + suffix; + userAccountRepository.save(new UserAccount(firstPrimaryId, "First", null, null)); + userAccountRepository.save(new UserAccount(secondPrimaryId, "Second", null, null)); + userAccountRepository.save(new UserAccount(secondaryId, "Secondary", null, null)); + localCredentialRepository.save(new LocalCredential(secondaryId, secondaryUsername, "hash")); + AccountMergeRequest first = new AccountMergeRequest( + firstPrimaryId, secondaryId, null, Instant.now().plusSeconds(1800)); + first.setStatus(AccountMergeRequest.STATUS_VERIFIED); + AccountMergeRequest second = new AccountMergeRequest( + secondPrimaryId, secondaryId, null, Instant.now().plusSeconds(1800)); + second.setStatus(AccountMergeRequest.STATUS_VERIFIED); + long firstId = mergeRequestRepository.save(first).getId(); + long secondId = mergeRequestRepository.save(second).getId(); + + CountDownLatch locked = new CountDownLatch(1); + CountDownLatch releaseLock = new CountDownLatch(1); + try (var executor = Executors.newVirtualThreadPerTaskExecutor()) { + Future lockHolder = executor.submit(() -> new TransactionTemplate(transactionManager).execute(status -> { + jdbcTemplate.execute("set local lock_timeout = '5s'"); + jdbcTemplate.queryForObject( + "select id from user_account where id = ? for update", String.class, secondaryId); + locked.countDown(); + try { + releaseLock.await(); + } catch (InterruptedException exception) { + Thread.currentThread().interrupt(); + throw new IllegalStateException(exception); + } + return null; + })); + if (!locked.await(10, TimeUnit.SECONDS)) { + releaseLock.countDown(); + lockHolder.cancel(true); + throw new AssertionError("Could not acquire the account row lock"); + } + Future firstConfirm = executor.submit(() -> confirmOrReject(firstPrimaryId, firstId)); + Future secondConfirm = executor.submit(() -> confirmOrReject(secondPrimaryId, secondId)); + int waiting = 0; + try { + for (int attempt = 0; attempt < 50 && waiting < 2; attempt++) { + waiting = jdbcTemplate.queryForObject( + "select count(*) from pg_stat_activity where wait_event_type = 'Lock' " + + "and query like '%user_account%'", Integer.class); + if (waiting < 2) { + Thread.sleep(100); + } + } + assertThat(waiting).isGreaterThanOrEqualTo(2); + } finally { + releaseLock.countDown(); + } + lockHolder.get(); + assertThat(firstConfirm.get()).isNotEqualTo(secondConfirm.get()); + } + + String winner = localCredentialRepository.findByUsernameIgnoreCase(secondaryUsername) + .orElseThrow().getUserId(); + assertThat(winner).isIn(firstPrimaryId, secondPrimaryId); + assertThat(userAccountRepository.findById(secondaryId).orElseThrow().getMergedToUserId()) + .isEqualTo(winner); + assertThat(List.of(firstId, secondId).stream() + .filter(id -> AccountMergeRequest.STATUS_COMPLETED.equals( + mergeRequestRepository.findById(id).orElseThrow().getStatus())).count()).isEqualTo(1); + } + + private boolean confirmOrReject(String primaryUserId, long requestId) { + try { + mergeService.confirm(primaryUserId, requestId); + return true; + } catch (com.iflytek.skillhub.auth.exception.AuthFlowException expected) { + return false; + } + } + @Test void secondaryCancellationAfterApprovalBlocksConfirmation() throws Exception { String suffix = UUID.randomUUID().toString(); diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/merge/AccountMergeService.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/merge/AccountMergeService.java index c80b9de3..b5de4dbe 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/merge/AccountMergeService.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/merge/AccountMergeService.java @@ -157,13 +157,22 @@ public class AccountMergeService { throw new AuthFlowException(HttpStatus.BAD_REQUEST, "error.auth.merge.tokenExpired"); } - UserAccount primaryUser = loadActiveUser(primaryUserId); - UserAccount secondaryUser = userAccountRepository.findById(request.getSecondaryUserId()) - .orElseThrow(() -> new AuthFlowException(HttpStatus.NOT_FOUND, "error.auth.merge.secondaryNotFound")); + String secondaryUserId = request.getSecondaryUserId(); + String firstUserId = primaryUserId.compareTo(secondaryUserId) < 0 ? primaryUserId : secondaryUserId; + String secondUserId = primaryUserId.compareTo(secondaryUserId) < 0 ? secondaryUserId : primaryUserId; + UserAccount firstUser = userAccountRepository.findLockedById(firstUserId) + .orElseThrow(() -> new AuthFlowException(HttpStatus.NOT_FOUND, "error.auth.merge.requestNotFound")); + UserAccount secondUser = userAccountRepository.findLockedById(secondUserId) + .orElseThrow(() -> new AuthFlowException(HttpStatus.NOT_FOUND, "error.auth.merge.requestNotFound")); + UserAccount primaryUser = firstUser.getId().equals(primaryUserId) ? firstUser : secondUser; + UserAccount secondaryUser = firstUser.getId().equals(secondaryUserId) ? firstUser : secondUser; + if (primaryUser.getStatus() != UserStatus.ACTIVE) { + throw new AuthFlowException(HttpStatus.BAD_REQUEST, "error.auth.merge.primaryNotActive"); + } validateMergePair(primaryUser, secondaryUser); migrateIdentityBindings(primaryUser.getId(), secondaryUser.getId()); - migrateApiTokens(primaryUser.getId(), secondaryUser.getId()); + revokeSecondaryApiTokens(secondaryUser.getId()); migrateUserRoles(primaryUser.getId(), secondaryUser.getId()); migrateNamespaceMemberships(primaryUser.getId(), secondaryUser.getId()); migrateLocalCredential(primaryUser.getId(), secondaryUser.getId()); @@ -249,12 +258,11 @@ public class AccountMergeService { identityBindingRepository.saveAll(bindings); } - private void migrateApiTokens(String primaryUserId, String secondaryUserId) { + private void revokeSecondaryApiTokens(String secondaryUserId) { List tokens = apiTokenRepository.findByUserId(secondaryUserId); for (ApiToken token : tokens) { - token.setUserId(primaryUserId); - if ("USER".equals(token.getSubjectType())) { - token.setSubjectId(primaryUserId); + if (token.getRevokedAt() == null) { + token.setRevokedAt(currentTime()); } } apiTokenRepository.saveAll(tokens); diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/merge/AccountMergeServiceTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/merge/AccountMergeServiceTest.java index 070c9073..08a6d480 100644 --- a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/merge/AccountMergeServiceTest.java +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/merge/AccountMergeServiceTest.java @@ -163,7 +163,7 @@ class AccountMergeServiceTest { } @Test - void confirm_migratesBindingsRolesTokensAndMemberships() throws Exception { + void confirm_migratesBindingsRolesAndMembershipsButRevokesSecondaryTokens() throws Exception { UserAccount primary = new UserAccount("usr_primary", "primary", "primary@example.com", null); UserAccount secondary = new UserAccount("usr_secondary", "secondary", "", null); AccountMergeRequest request = request("usr_primary", "usr_secondary", "encoded"); @@ -176,8 +176,8 @@ class AccountMergeServiceTest { NamespaceMember secondaryMembership = new NamespaceMember(1L, "usr_secondary", NamespaceRole.ADMIN); given(mergeRequestRepository.findByIdAndPrimaryUserId(7L, "usr_primary")).willReturn(Optional.of(request)); - given(userAccountRepository.findById("usr_primary")).willReturn(Optional.of(primary)); - given(userAccountRepository.findById("usr_secondary")).willReturn(Optional.of(secondary)); + given(userAccountRepository.findLockedById("usr_primary")).willReturn(Optional.of(primary)); + given(userAccountRepository.findLockedById("usr_secondary")).willReturn(Optional.of(secondary)); given(mergeRequestRepository.save(any(AccountMergeRequest.class))).willAnswer(invocation -> invocation.getArgument(0)); given(identityBindingRepository.findByUserId("usr_secondary")).willReturn(List.of(binding)); given(apiTokenRepository.findByUserId("usr_secondary")).willReturn(List.of(token)); @@ -191,8 +191,9 @@ class AccountMergeServiceTest { service.confirm("usr_primary", 7L); assertThat(binding.getUserId()).isEqualTo("usr_primary"); - assertThat(token.getUserId()).isEqualTo("usr_primary"); - assertThat(token.getSubjectId()).isEqualTo("usr_primary"); + assertThat(token.getUserId()).isEqualTo("usr_secondary"); + assertThat(token.getSubjectId()).isEqualTo("usr_secondary"); + assertThat(token.getRevokedAt()).isEqualTo(Instant.parse("2026-03-18T00:00:00Z")); assertThat(secondaryMembership.getUserId()).isEqualTo("usr_primary"); assertThat(secondary.getStatus()).isEqualTo(com.iflytek.skillhub.domain.user.UserStatus.MERGED); assertThat(secondary.getMergedToUserId()).isEqualTo("usr_primary"); diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/user/UserAccountRepository.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/user/UserAccountRepository.java index c0f4f295..4b1fd567 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/user/UserAccountRepository.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/user/UserAccountRepository.java @@ -11,6 +11,7 @@ import java.util.Optional; */ public interface UserAccountRepository { Optional findById(String id); + Optional findLockedById(String id); List findByIdIn(List ids); Optional findByEmailIgnoreCase(String email); Page search(String keyword, UserStatus status, Pageable pageable); diff --git a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/UserAccountJpaRepository.java b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/UserAccountJpaRepository.java index f2d4835c..d2ea1367 100644 --- a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/UserAccountJpaRepository.java +++ b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/UserAccountJpaRepository.java @@ -3,14 +3,18 @@ package com.iflytek.skillhub.infra.jpa; import com.iflytek.skillhub.domain.user.UserAccount; import com.iflytek.skillhub.domain.user.UserAccountRepository; import com.iflytek.skillhub.domain.user.UserStatus; +import jakarta.persistence.LockModeType; import org.springframework.data.domain.Page; import org.springframework.data.domain.Pageable; +import org.springframework.data.jpa.repository.Lock; import org.springframework.data.jpa.repository.JpaRepository; import org.springframework.data.jpa.repository.JpaSpecificationExecutor; import org.springframework.data.jpa.repository.Query; import org.springframework.data.repository.query.Param; import org.springframework.stereotype.Repository; +import java.util.Optional; + /** * JPA-backed user-account repository that provides filtered admin search over account records. */ @@ -18,6 +22,11 @@ import org.springframework.stereotype.Repository; public interface UserAccountJpaRepository extends JpaRepository, JpaSpecificationExecutor, UserAccountRepository { + @Override + @Lock(LockModeType.PESSIMISTIC_WRITE) + @Query("select user from UserAccount user where user.id = :id") + Optional findLockedById(@Param("id") String id); + @Override @Query(""" SELECT u