From 833270bb31af4d78c329d1cb53f8d9dc2830e147 Mon Sep 17 00:00:00 2001 From: ylhu16 Date: Thu, 30 Jul 2026 14:16:40 +0800 Subject: [PATCH] fix(auth): enforce trusted OAuth identity attributes Signed-off-by: ylhu16 --- docs/02-domain-model.md | 5 +- docs/03-authentication-design.md | 6 +- .../auth/identity/IdentityBindingService.java | 43 ++++-- .../auth/oauth/AccountMergedException.java | 14 ++ .../auth/oauth/GitHubClaimsExtractor.java | 17 ++- .../auth/oauth/OAuthLoginFlowService.java | 4 +- .../oauth/SystemAccountLoginException.java | 18 +++ .../auth/policy/EmailDomainAccessPolicy.java | 2 +- .../identity/IdentityBindingServiceTest.java | 140 +++++++++++++++++- .../auth/oauth/GitHubClaimsExtractorTest.java | 98 ++++++++++++ .../auth/oauth/OAuthLoginFlowServiceTest.java | 24 +++ .../auth/policy/AccessPolicyTest.java | 4 +- 12 files changed, 346 insertions(+), 29 deletions(-) create mode 100644 server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/AccountMergedException.java create mode 100644 server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/SystemAccountLoginException.java create mode 100644 server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/GitHubClaimsExtractorTest.java diff --git a/docs/02-domain-model.md b/docs/02-domain-model.md index 65e92c29c..5aaaa4a64 100644 --- a/docs/02-domain-model.md +++ b/docs/02-domain-model.md @@ -245,6 +245,7 @@ | avatar_url | varchar(512) | | | status | enum | `ACTIVE` / `PENDING` / `DISABLED` / `MERGED` | | merged_to_user_id | varchar(128) | 合并目标用户 ID，仅 MERGED 状态有值 | +| system_account | boolean | 系统服务账号，禁止交互式 Web/OAuth 登录 | | created_at | datetime | | | updated_at | datetime | | @@ -252,8 +253,10 @@ - `ACTIVE`：正常使用 - `PENDING`：等待管理员审批（AccessPolicy 返回 PENDING_APPROVAL 时创建） - `DISABLED`：管理员封禁，登录后拒绝所有操作，返回 403 - - `MERGED`：已合并到其他账号，保留记录不物理删除，登录时自动跳转到合并目标账号 + - `MERGED`：已合并到其他账号，保留记录不物理删除；登录直接拒绝，不向调用方泄露合并目标 - 授权层在每次请求时检查用户状态，非 `ACTIVE` 用户拒绝所有写操作 +- system account 可按独立 Token Policy 使用非交互凭证，但不能通过本地密码或外部 OAuth + 建立普通用户 Session ### identity_binding diff --git a/docs/03-authentication-design.md b/docs/03-authentication-design.md index 2f4b77052..269a85bc2 100644 --- a/docs/03-authentication-design.md +++ b/docs/03-authentication-design.md @@ -94,7 +94,8 @@ astron: - `DENY`：抛出 `OAuth2AccessDeniedException`，由 `failureHandler` 重定向到 `/access-denied` 页面。不创建用户，不建立 Session。 - `PENDING_APPROVAL`：创建 `user_account`（status=`PENDING`），但不建立业务 Session。抛出 `AccountPendingException`，由 `failureHandler` 重定向到 `/pending-approval` 页面（纯静态提示页，无需登录态）。管理员在后台审批后状态变为 `ACTIVE`，用户下次 OAuth 登录才会正常建立 Session。 -安全边界：PENDING / DISABLED 用户绝不会拥有有效的业务 Session，从根源上杜绝"待审批账号已认证"的风险。 +安全边界：PENDING / DISABLED / MERGED 用户和 system account 绝不会通过交互式登录获得 +业务 Session。外部身份命中这些账号时，在更新用户资料或加载角色前直接拒绝。 ### 2.3 扩展性 @@ -361,7 +362,8 @@ public class OAuthClaimsExtractor { 合并操作规则： - 合并操作写入审计日志 - 合并后原 user_account 标记为 `MERGED`，保留记录不物理删除 -- 预留扩展位：未来可配置 `astron.identity.auto-merge-on-verified-email=true` 开启基于已验证邮箱的自动合并 +- 不提供按 email 自动合并；即使 Provider 声明 email 已验证，也不能替代对两个账号控制权 + 的分别证明。未来绑定/合并必须使用显式、可审计的重新认证流程。 ## 5. CLI 认证（OAuth Device Flow + 平台凭证） 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 2a4fae8b3..49fa3dc7f 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 @@ -1,7 +1,11 @@ package com.iflytek.skillhub.auth.identity; import com.iflytek.skillhub.auth.entity.IdentityBinding; +import com.iflytek.skillhub.auth.oauth.AccountDisabledException; +import com.iflytek.skillhub.auth.oauth.AccountMergedException; +import com.iflytek.skillhub.auth.oauth.AccountPendingException; import com.iflytek.skillhub.auth.oauth.OAuthClaims; +import com.iflytek.skillhub.auth.oauth.SystemAccountLoginException; import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; import com.iflytek.skillhub.auth.rbac.PlatformRoleDefaults; import com.iflytek.skillhub.auth.repository.IdentityBindingRepository; @@ -48,8 +52,9 @@ public PlatformPrincipal bindOrCreate(OAuthClaims claims, UserStatus initialStat if (binding != null) { user = userRepo.findById(binding.getUserId()) .orElseThrow(() -> new IllegalStateException("User not found for binding")); + ensureExternalLoginAllowed(user); user.setDisplayName(claims.providerLogin()); - if (claims.email() != null) user.setEmail(claims.email()); + if (trustedEmail(claims) != null) user.setEmail(claims.email()); if (claims.extra().get("avatar_url") != null) { user.setAvatarUrl((String) claims.extra().get("avatar_url")); } @@ -58,7 +63,7 @@ public PlatformPrincipal bindOrCreate(OAuthClaims claims, UserStatus initialStat user = new UserAccount( "usr_" + UUID.randomUUID(), claims.providerLogin(), - claims.email(), + trustedEmail(claims), (String) claims.extra().get("avatar_url") ); user.setStatus(initialStatus); @@ -71,12 +76,7 @@ public PlatformPrincipal bindOrCreate(OAuthClaims claims, UserStatus initialStat bindingRepo.save(binding); } - if (user.getStatus() == UserStatus.PENDING) { - throw new com.iflytek.skillhub.auth.oauth.AccountPendingException(); - } - if (user.getStatus() == UserStatus.DISABLED) { - throw new com.iflytek.skillhub.auth.oauth.AccountDisabledException(); - } + ensureExternalLoginAllowed(user); Set roles = roleBindingRepo.findByUserId(user.getId()).stream() .map(rb -> rb.getRole().getCode()) @@ -97,16 +97,14 @@ public void createPendingUserIfAbsent(OAuthClaims claims) { if (existingBinding != null) { UserAccount existingUser = userRepo.findById(existingBinding.getUserId()) .orElseThrow(() -> new IllegalStateException("User not found for binding")); - if (existingUser.getStatus() == UserStatus.DISABLED) { - throw new com.iflytek.skillhub.auth.oauth.AccountDisabledException(); - } - throw new com.iflytek.skillhub.auth.oauth.AccountPendingException(); + ensureExternalLoginAllowed(existingUser); + throw new AccountPendingException(); } UserAccount user = new UserAccount( "usr_" + UUID.randomUUID(), claims.providerLogin(), - claims.email(), + trustedEmail(claims), (String) claims.extra().get("avatar_url") ); user.setStatus(UserStatus.PENDING); @@ -115,4 +113,23 @@ public void createPendingUserIfAbsent(OAuthClaims claims) { IdentityBinding binding = new IdentityBinding(user.getId(), claims.provider(), claims.subject(), claims.providerLogin()); bindingRepo.save(binding); } + + private String trustedEmail(OAuthClaims claims) { + return claims.emailVerified() ? claims.email() : null; + } + + private void ensureExternalLoginAllowed(UserAccount user) { + if (user.isSystemAccount()) { + throw new SystemAccountLoginException(); + } + if (user.getStatus() == UserStatus.PENDING) { + throw new AccountPendingException(); + } + if (user.getStatus() == UserStatus.DISABLED) { + throw new AccountDisabledException(); + } + if (user.getStatus() == UserStatus.MERGED) { + throw new AccountMergedException(); + } + } } diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/AccountMergedException.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/AccountMergedException.java new file mode 100644 index 000000000..0956be852 --- /dev/null +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/AccountMergedException.java @@ -0,0 +1,14 @@ +package com.iflytek.skillhub.auth.oauth; + +import org.springframework.security.oauth2.core.OAuth2AuthenticationException; +import org.springframework.security.oauth2.core.OAuth2Error; + +/** + * OAuth authentication exception raised when the mapped platform account was merged. + */ +public class AccountMergedException extends OAuth2AuthenticationException { + + public AccountMergedException() { + super(new OAuth2Error("account_merged", "Account was merged", null)); + } +} diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/GitHubClaimsExtractor.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/GitHubClaimsExtractor.java index 51b802dac..b165e1faf 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/GitHubClaimsExtractor.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/GitHubClaimsExtractor.java @@ -9,7 +9,6 @@ import java.util.Comparator; import java.util.List; -import org.springframework.stereotype.Component; import java.util.Map; /** @@ -19,10 +18,14 @@ @Component public class GitHubClaimsExtractor implements OAuthClaimsExtractor { - private final RestClient restClient = RestClient.builder() - .baseUrl("https://api.github.com") - .defaultHeader(HttpHeaders.ACCEPT, MediaType.APPLICATION_JSON_VALUE) - .build(); + private final RestClient restClient; + + public GitHubClaimsExtractor(RestClient.Builder restClientBuilder) { + this.restClient = restClientBuilder + .baseUrl("https://api.github.com") + .defaultHeader(HttpHeaders.ACCEPT, MediaType.APPLICATION_JSON_VALUE) + .build(); + } @Override public String getProvider() { return "github"; } @@ -32,9 +35,7 @@ public OAuthClaims extract(OAuth2UserRequest request, OAuth2User oAuth2User) { Map attrs = oAuth2User.getAttributes(); GitHubEmail primaryEmail = loadPrimaryEmail(request); String email = primaryEmail != null ? primaryEmail.email() : (String) attrs.get("email"); - boolean emailVerified = primaryEmail != null - ? primaryEmail.verified() - : attrs.get("email") != null; + boolean emailVerified = primaryEmail != null && primaryEmail.verified(); return new OAuthClaims( "github", diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowService.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowService.java index e5c7dc3de..ac3d413cb 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowService.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowService.java @@ -98,7 +98,9 @@ public String resolveFailureRedirect(AuthenticationException exception, String r if (exception instanceof AccountPendingException) { return "/pending-approval"; } - if (exception instanceof AccountDisabledException) { + if (exception instanceof AccountDisabledException + || exception instanceof AccountMergedException + || exception instanceof SystemAccountLoginException) { return "/access-denied"; } if (exception instanceof OAuth2AuthenticationException oauth2Exception diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/SystemAccountLoginException.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/SystemAccountLoginException.java new file mode 100644 index 000000000..bab0c4cd3 --- /dev/null +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/SystemAccountLoginException.java @@ -0,0 +1,18 @@ +package com.iflytek.skillhub.auth.oauth; + +import org.springframework.security.oauth2.core.OAuth2AuthenticationException; +import org.springframework.security.oauth2.core.OAuth2Error; + +/** + * OAuth authentication exception raised when an external identity resolves to a system account. + */ +public class SystemAccountLoginException extends OAuth2AuthenticationException { + + public SystemAccountLoginException() { + super(new OAuth2Error( + "system_account_forbidden", + "System accounts cannot use interactive OAuth login", + null + )); + } +} diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/EmailDomainAccessPolicy.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/EmailDomainAccessPolicy.java index d688f2a2e..b65a738d5 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/EmailDomainAccessPolicy.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/policy/EmailDomainAccessPolicy.java @@ -15,7 +15,7 @@ public EmailDomainAccessPolicy(Set allowedDomains) { @Override public AccessDecision evaluate(OAuthClaims claims) { - if (claims.email() == null) return AccessDecision.DENY; + if (claims.email() == null || !claims.emailVerified()) return AccessDecision.DENY; String domain = claims.email().substring(claims.email().indexOf('@') + 1); return allowedDomains.contains(domain.toLowerCase()) ? AccessDecision.ALLOW : AccessDecision.DENY; 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 79e5df746..75dda2de0 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 @@ -11,8 +11,10 @@ import com.iflytek.skillhub.auth.entity.Role; import com.iflytek.skillhub.auth.entity.UserRoleBinding; import com.iflytek.skillhub.auth.oauth.AccountDisabledException; +import com.iflytek.skillhub.auth.oauth.AccountMergedException; import com.iflytek.skillhub.auth.oauth.OAuthClaims; import com.iflytek.skillhub.auth.oauth.AccountPendingException; +import com.iflytek.skillhub.auth.oauth.SystemAccountLoginException; import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; import com.iflytek.skillhub.auth.repository.IdentityBindingRepository; import com.iflytek.skillhub.auth.repository.UserRoleBindingRepository; @@ -131,12 +133,84 @@ void bindOrCreate_existingDisabledUser_throwsAccountDisabled() { when(bindingRepo.findByProviderCodeAndSubject("github", "gh_1")).thenReturn(Optional.of(binding)); when(userRepo.findById("usr_1")).thenReturn(Optional.of(user)); - when(userRepo.save(any(UserAccount.class))).thenAnswer(invocation -> invocation.getArgument(0)); assertThatThrownBy(() -> service.bindOrCreate(claims, UserStatus.ACTIVE)) .isInstanceOf(AccountDisabledException.class); } + @Test + void bindOrCreate_existingMergedUser_throwsBeforeProfileUpdate() { + OAuthClaims claims = new OAuthClaims( + "github", "gh_1", "attacker@example.com", true, "attacker", Map.of() + ); + IdentityBinding binding = new IdentityBinding("usr_1", "github", "gh_1", "alice"); + UserAccount user = new UserAccount("usr_1", "alice", "alice@example.com", null); + user.setStatus(UserStatus.MERGED); + + when(bindingRepo.findByProviderCodeAndSubject("github", "gh_1")).thenReturn(Optional.of(binding)); + when(userRepo.findById("usr_1")).thenReturn(Optional.of(user)); + + assertThatThrownBy(() -> service.bindOrCreate(claims, UserStatus.ACTIVE)) + .isInstanceOf(AccountMergedException.class); + + assertThat(user.getDisplayName()).isEqualTo("alice"); + assertThat(user.getEmail()).isEqualTo("alice@example.com"); + verify(userRepo, never()).save(any(UserAccount.class)); + } + + @Test + void bindOrCreate_existingSystemAccount_throwsBeforeProfileUpdate() { + OAuthClaims claims = new OAuthClaims( + "github", "gh_1", "attacker@example.com", true, "attacker", Map.of() + ); + IdentityBinding binding = new IdentityBinding("system_1", "github", "gh_1", "system"); + UserAccount user = UserAccount.systemAccount("system_1", "system", null, null); + + when(bindingRepo.findByProviderCodeAndSubject("github", "gh_1")).thenReturn(Optional.of(binding)); + when(userRepo.findById("system_1")).thenReturn(Optional.of(user)); + + assertThatThrownBy(() -> service.bindOrCreate(claims, UserStatus.ACTIVE)) + .isInstanceOf(SystemAccountLoginException.class); + + assertThat(user.getDisplayName()).isEqualTo("system"); + assertThat(user.getEmail()).isNull(); + verify(userRepo, never()).save(any(UserAccount.class)); + } + + @Test + void bindOrCreate_unverifiedEmailDoesNotPopulateNewAccount() { + OAuthClaims claims = new OAuthClaims( + "github", "gh_1", "unverified@example.com", false, "alice", Map.of() + ); + when(bindingRepo.findByProviderCodeAndSubject("github", "gh_1")).thenReturn(Optional.empty()); + when(userRepo.save(any(UserAccount.class))).thenAnswer(invocation -> invocation.getArgument(0)); + when(roleBindingRepo.findByUserId(any())).thenReturn(List.of()); + + service.bindOrCreate(claims, UserStatus.ACTIVE); + + ArgumentCaptor userCaptor = ArgumentCaptor.forClass(UserAccount.class); + verify(userRepo).save(userCaptor.capture()); + assertThat(userCaptor.getValue().getEmail()).isNull(); + } + + @Test + void bindOrCreate_unverifiedEmailDoesNotOverwriteExistingEmail() { + OAuthClaims claims = new OAuthClaims( + "github", "gh_1", "unverified@example.com", false, "alice", Map.of() + ); + IdentityBinding binding = new IdentityBinding("usr_1", "github", "gh_1", "alice"); + UserAccount user = new UserAccount("usr_1", "alice", "verified@example.com", null); + + when(bindingRepo.findByProviderCodeAndSubject("github", "gh_1")).thenReturn(Optional.of(binding)); + when(userRepo.findById("usr_1")).thenReturn(Optional.of(user)); + when(userRepo.save(any(UserAccount.class))).thenAnswer(invocation -> invocation.getArgument(0)); + when(roleBindingRepo.findByUserId("usr_1")).thenReturn(List.of()); + + service.bindOrCreate(claims, UserStatus.ACTIVE); + + assertThat(user.getEmail()).isEqualTo("verified@example.com"); + } + @Test void bindOrCreate_returnsExplicitPlatformRolesWhenBindingsExist() { OAuthClaims claims = new OAuthClaims( @@ -179,4 +253,68 @@ void createPendingUserIfAbsent_existingDisabledBinding_throwsAccountDisabled() { assertThatThrownBy(() -> service.createPendingUserIfAbsent(claims)) .isInstanceOf(AccountDisabledException.class); } + + @Test + void createPendingUserIfAbsent_existingPendingBinding_throwsAccountPending() { + OAuthClaims claims = new OAuthClaims( + "github", "gh_1", "alice@example.com", true, "alice", Map.of() + ); + IdentityBinding binding = new IdentityBinding("usr_1", "github", "gh_1", "alice"); + UserAccount user = new UserAccount("usr_1", "alice", "alice@example.com", null); + user.setStatus(UserStatus.PENDING); + + when(bindingRepo.findByProviderCodeAndSubject("github", "gh_1")).thenReturn(Optional.of(binding)); + when(userRepo.findById("usr_1")).thenReturn(Optional.of(user)); + + assertThatThrownBy(() -> service.createPendingUserIfAbsent(claims)) + .isInstanceOf(AccountPendingException.class); + verify(userRepo, never()).save(any(UserAccount.class)); + verify(bindingRepo, never()).save(any(IdentityBinding.class)); + } + + @Test + void createPendingUserIfAbsent_existingMergedBinding_throwsAccountMerged() { + OAuthClaims claims = new OAuthClaims( + "github", "gh_1", "alice@example.com", true, "alice", Map.of() + ); + IdentityBinding binding = new IdentityBinding("usr_1", "github", "gh_1", "alice"); + UserAccount user = new UserAccount("usr_1", "alice", "alice@example.com", null); + user.setStatus(UserStatus.MERGED); + + when(bindingRepo.findByProviderCodeAndSubject("github", "gh_1")).thenReturn(Optional.of(binding)); + when(userRepo.findById("usr_1")).thenReturn(Optional.of(user)); + + assertThatThrownBy(() -> service.createPendingUserIfAbsent(claims)) + .isInstanceOf(AccountMergedException.class); + } + + @Test + void createPendingUserIfAbsent_existingSystemBinding_throwsSystemAccountLogin() { + OAuthClaims claims = new OAuthClaims( + "github", "gh_1", "alice@example.com", true, "alice", Map.of() + ); + IdentityBinding binding = new IdentityBinding("system_1", "github", "gh_1", "system"); + UserAccount user = UserAccount.systemAccount("system_1", "system", null, null); + + when(bindingRepo.findByProviderCodeAndSubject("github", "gh_1")).thenReturn(Optional.of(binding)); + when(userRepo.findById("system_1")).thenReturn(Optional.of(user)); + + assertThatThrownBy(() -> service.createPendingUserIfAbsent(claims)) + .isInstanceOf(SystemAccountLoginException.class); + } + + @Test + void createPendingUserIfAbsent_unverifiedEmailDoesNotPopulateAccount() { + OAuthClaims claims = new OAuthClaims( + "github", "gh_1", "unverified@example.com", false, "alice", Map.of() + ); + when(bindingRepo.findByProviderCodeAndSubject("github", "gh_1")).thenReturn(Optional.empty()); + when(userRepo.save(any(UserAccount.class))).thenAnswer(invocation -> invocation.getArgument(0)); + + service.createPendingUserIfAbsent(claims); + + ArgumentCaptor userCaptor = ArgumentCaptor.forClass(UserAccount.class); + verify(userRepo).save(userCaptor.capture()); + assertThat(userCaptor.getValue().getEmail()).isNull(); + } } diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/GitHubClaimsExtractorTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/GitHubClaimsExtractorTest.java new file mode 100644 index 000000000..35de77b8c --- /dev/null +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/GitHubClaimsExtractorTest.java @@ -0,0 +1,98 @@ +package com.iflytek.skillhub.auth.oauth; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.springframework.test.web.client.match.MockRestRequestMatchers.header; +import static org.springframework.test.web.client.match.MockRestRequestMatchers.requestTo; +import static org.springframework.test.web.client.response.MockRestResponseCreators.withSuccess; + +import java.time.Instant; +import java.util.List; +import java.util.Map; +import org.junit.jupiter.api.Test; +import org.springframework.http.HttpHeaders; +import org.springframework.http.MediaType; +import org.springframework.security.oauth2.client.registration.ClientRegistration; +import org.springframework.security.oauth2.client.userinfo.OAuth2UserRequest; +import org.springframework.security.oauth2.core.AuthorizationGrantType; +import org.springframework.security.oauth2.core.OAuth2AccessToken; +import org.springframework.security.oauth2.core.user.DefaultOAuth2User; +import org.springframework.test.web.client.MockRestServiceServer; +import org.springframework.web.client.RestClient; + +class GitHubClaimsExtractorTest { + + @Test + void extract_doesNotTrustProfileEmailWhenEmailsApiHasNoVerifiedEmail() { + RestClient.Builder restClientBuilder = RestClient.builder(); + MockRestServiceServer server = MockRestServiceServer.bindTo(restClientBuilder).build(); + server.expect(requestTo("https://api.github.com/user/emails")) + .andExpect(header(HttpHeaders.AUTHORIZATION, "Bearer token-123")) + .andRespond(withSuccess( + """ + [{"email":"alice@example.com","primary":true,"verified":false}] + """, + MediaType.APPLICATION_JSON + )); + GitHubClaimsExtractor extractor = new GitHubClaimsExtractor(restClientBuilder); + + OAuthClaims claims = extractor.extract(userRequest(), githubUser("alice@example.com")); + + assertThat(claims.email()).isEqualTo("alice@example.com"); + assertThat(claims.emailVerified()).isFalse(); + server.verify(); + } + + @Test + void extract_usesVerifiedEmailFromEmailsApi() { + RestClient.Builder restClientBuilder = RestClient.builder(); + MockRestServiceServer server = MockRestServiceServer.bindTo(restClientBuilder).build(); + server.expect(requestTo("https://api.github.com/user/emails")) + .andExpect(header(HttpHeaders.AUTHORIZATION, "Bearer token-123")) + .andRespond(withSuccess( + """ + [ + {"email":"secondary@example.com","primary":false,"verified":true}, + {"email":"alice@example.com","primary":true,"verified":true} + ] + """, + MediaType.APPLICATION_JSON + )); + GitHubClaimsExtractor extractor = new GitHubClaimsExtractor(restClientBuilder); + + OAuthClaims claims = extractor.extract(userRequest(), githubUser(null)); + + assertThat(claims.email()).isEqualTo("alice@example.com"); + assertThat(claims.emailVerified()).isTrue(); + server.verify(); + } + + private DefaultOAuth2User githubUser(String email) { + Map attributes = new java.util.HashMap<>(); + attributes.put("id", 42); + attributes.put("login", "alice"); + attributes.put("email", email); + return new DefaultOAuth2User(List.of(), attributes, "login"); + } + + private OAuth2UserRequest userRequest() { + ClientRegistration registration = ClientRegistration.withRegistrationId("github") + .clientId("client-id") + .clientSecret("client-secret") + .authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE) + .redirectUri("{baseUrl}/login/oauth2/code/{registrationId}") + .scope("read:user", "user:email") + .authorizationUri("https://github.com/login/oauth/authorize") + .tokenUri("https://github.com/login/oauth/access_token") + .userInfoUri("https://api.github.com/user") + .userNameAttributeName("login") + .clientName("GitHub") + .build(); + OAuth2AccessToken accessToken = new OAuth2AccessToken( + OAuth2AccessToken.TokenType.BEARER, + "token-123", + Instant.now(), + Instant.now().plusSeconds(3600) + ); + return new OAuth2UserRequest(registration, accessToken); + } +} diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowServiceTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowServiceTest.java index 029ec2944..d51311da5 100644 --- a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowServiceTest.java +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowServiceTest.java @@ -48,6 +48,30 @@ void resolveFailureRedirect_maps_access_denied_to_user_facing_page() { assertThat(redirect).isEqualTo("/access-denied"); } + @Test + void resolveFailureRedirect_mapsMergedAccountToAccessDenied() { + OAuthLoginFlowService service = new OAuthLoginFlowService( + List.of(), + mock(AccessPolicy.class), + mock(IdentityBindingService.class) + ); + + assertThat(service.resolveFailureRedirect(new AccountMergedException(), null)) + .isEqualTo("/access-denied"); + } + + @Test + void resolveFailureRedirect_mapsSystemAccountToAccessDenied() { + OAuthLoginFlowService service = new OAuthLoginFlowService( + List.of(), + mock(AccessPolicy.class), + mock(IdentityBindingService.class) + ); + + assertThat(service.resolveFailureRedirect(new SystemAccountLoginException(), null)) + .isEqualTo("/access-denied"); + } + @Test void consumeReturnTo_clearsUnsafeSessionValue() { OAuthLoginFlowService service = new OAuthLoginFlowService( diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/policy/AccessPolicyTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/policy/AccessPolicyTest.java index 62a40abf5..3bfb7b90a 100644 --- a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/policy/AccessPolicyTest.java +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/policy/AccessPolicyTest.java @@ -37,10 +37,10 @@ void emailDomainPolicy_deniesNullEmail() { } @Test - void emailDomainPolicy_allowsUnverifiedEmailFromMatchingDomain() { + void emailDomainPolicy_deniesUnverifiedEmailFromMatchingDomain() { var policy = new EmailDomainAccessPolicy(Set.of("company.com")); var claims = new OAuthClaims("github", "123", "user@company.com", false, "user", Map.of()); - assertThat(policy.evaluate(claims)).isEqualTo(AccessDecision.ALLOW); + assertThat(policy.evaluate(claims)).isEqualTo(AccessDecision.DENY); } @Test