mirror of
https://github.com/iflytek/skillhub.git
synced 2026-10-09 03:17:52 +00:00
fix(auth): serialize account merges and revoke secondary tokens
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
This commit is contained in:
parent
7a53782985
commit
b7cfbe193b
6 changed files with 173 additions and 13 deletions
|
|
@ -438,6 +438,9 @@ public class OAuthClaimsExtractor {
|
|||
发起账号创建 30 分钟内有效的请求;待合并账号在自己的已认证会话中查看合并目标并批准;
|
||||
最后发起账号在批准后 30 分钟内确认迁移;任一账号可在完成前撤销请求。
|
||||
请求 ID 只用于定位请求,不是第二个账号的所有权凭据。
|
||||
确认合并时,对两个账号行按 ID 顺序加锁并重新检查状态,防止多个已批准请求并发完成。
|
||||
待合并账号已签发的 API Token 在合并时吊销,不转移给主账号;否则旧 Token 会继承主账号权限。
|
||||
主账号原有 Token 不变,需要自动化访问的调用方应重新签发 Token。
|
||||
账号合并页面当前仍未开放,`/settings/accounts` 保持重定向;上述流程由 API 强制执行。
|
||||
|
||||
- 一期 GitHub-only:不需要自动合并,每个 Provider 登录独立创建用户
|
||||
|
|
|
|||
|
|
@ -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<Boolean> firstConfirm = executor.submit(() -> confirmOrReject(firstPrimaryId, firstId));
|
||||
Future<Boolean> 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();
|
||||
|
|
|
|||
|
|
@ -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<ApiToken> 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);
|
||||
|
|
|
|||
|
|
@ -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");
|
||||
|
|
|
|||
|
|
@ -11,6 +11,7 @@ import java.util.Optional;
|
|||
*/
|
||||
public interface UserAccountRepository {
|
||||
Optional<UserAccount> findById(String id);
|
||||
Optional<UserAccount> findLockedById(String id);
|
||||
List<UserAccount> findByIdIn(List<String> ids);
|
||||
Optional<UserAccount> findByEmailIgnoreCase(String email);
|
||||
Page<UserAccount> search(String keyword, UserStatus status, Pageable pageable);
|
||||
|
|
|
|||
|
|
@ -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<UserAccount, String>, JpaSpecificationExecutor<UserAccount>, UserAccountRepository {
|
||||
|
||||
@Override
|
||||
@Lock(LockModeType.PESSIMISTIC_WRITE)
|
||||
@Query("select user from UserAccount user where user.id = :id")
|
||||
Optional<UserAccount> findLockedById(@Param("id") String id);
|
||||
|
||||
@Override
|
||||
@Query("""
|
||||
SELECT u
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue