From b24cc58338d7daf45b6d5e863ac7c3c8f0b13695 Mon Sep 17 00:00:00 2001 From: dongmucat <70678707+dongmucat@users.noreply.github.com> Date: Thu, 9 Apr 2026 18:11:47 +0800 Subject: [PATCH 1/6] fix(search): include permitted skills in clawhub explore (#258) --- .../service/SkillSearchAppService.java | 36 ++++++++++---- .../service/SkillSearchAppServiceTest.java | 47 +++++++++++++++++-- .../search/SearchVisibilityScope.java | 12 ++++- .../PostgresFullTextQueryService.java | 6 +++ .../PostgresFullTextQueryServiceTest.java | 34 ++++++++++++++ 5 files changed, 121 insertions(+), 14 deletions(-) diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SkillSearchAppService.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SkillSearchAppService.java index 4b3f3204..57a1145a 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SkillSearchAppService.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SkillSearchAppService.java @@ -1,8 +1,9 @@ package com.iflytek.skillhub.service; -import com.iflytek.skillhub.domain.namespace.NamespaceRole; +import com.iflytek.skillhub.auth.rbac.RbacService; import com.iflytek.skillhub.domain.namespace.Namespace; import com.iflytek.skillhub.domain.namespace.NamespaceRepository; +import com.iflytek.skillhub.domain.namespace.NamespaceRole; import com.iflytek.skillhub.domain.namespace.NamespaceService; import com.iflytek.skillhub.domain.skill.Skill; import com.iflytek.skillhub.domain.skill.SkillRepository; @@ -12,13 +13,12 @@ import com.iflytek.skillhub.search.SearchQuery; import com.iflytek.skillhub.search.SearchQueryService; import com.iflytek.skillhub.search.SearchResult; import com.iflytek.skillhub.search.SearchVisibilityScope; -import org.springframework.stereotype.Service; - import java.util.List; import java.util.Map; import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; +import org.springframework.stereotype.Service; /** * Application service that assembles discovery responses from search matches. @@ -36,18 +36,21 @@ public class SkillSearchAppService { private final NamespaceRepository namespaceRepository; private final NamespaceService namespaceService; private final SkillLifecycleProjectionService skillLifecycleProjectionService; + private final RbacService rbacService; public SkillSearchAppService( SearchQueryService searchQueryService, SkillRepository skillRepository, NamespaceRepository namespaceRepository, NamespaceService namespaceService, - SkillLifecycleProjectionService skillLifecycleProjectionService) { + SkillLifecycleProjectionService skillLifecycleProjectionService, + RbacService rbacService) { this.searchQueryService = searchQueryService; this.skillRepository = skillRepository; this.namespaceRepository = namespaceRepository; this.namespaceService = namespaceService; this.skillLifecycleProjectionService = skillLifecycleProjectionService; + this.rbacService = rbacService; } public record SearchResponse( @@ -93,21 +96,36 @@ public class SkillSearchAppService { } private SearchVisibilityScope buildVisibilityScope(String userId, Map userNsRoles) { - if (userId == null || userNsRoles == null) { + if (userId == null) { return SearchVisibilityScope.anonymous(); } - Set memberNamespaceIds = userNsRoles.keySet(); - Set adminNamespaceIds = userNsRoles.entrySet().stream() + Map normalizedRoles = userNsRoles != null ? userNsRoles : Map.of(); + Set memberNamespaceIds = normalizedRoles.keySet(); + Set adminNamespaceIds = normalizedRoles.entrySet().stream() .filter(e -> e.getValue() == NamespaceRole.ADMIN) .map(Map.Entry::getKey) .collect(java.util.stream.Collectors.toSet()); - adminNamespaceIds.addAll(userNsRoles.entrySet().stream() + adminNamespaceIds.addAll(normalizedRoles.entrySet().stream() .filter(e -> e.getValue() == NamespaceRole.OWNER) .map(Map.Entry::getKey) .toList()); - return new SearchVisibilityScope(userId, memberNamespaceIds, adminNamespaceIds); + Set platformRoles = rbacService.getUserRoleCodes(userId); + + return new SearchVisibilityScope( + userId, + memberNamespaceIds, + adminNamespaceIds, + hasPlatformWideReadAccess(platformRoles) + ); + } + + private boolean hasPlatformWideReadAccess(Set platformRoles) { + if (platformRoles == null || platformRoles.isEmpty()) { + return false; + } + return platformRoles.contains("SUPER_ADMIN"); } private SearchResponse searchVisibleSkills( diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/SkillSearchAppServiceTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/SkillSearchAppServiceTest.java index 98496db0..57710c88 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/SkillSearchAppServiceTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/SkillSearchAppServiceTest.java @@ -1,5 +1,6 @@ package com.iflytek.skillhub.service; +import com.iflytek.skillhub.auth.rbac.RbacService; import com.iflytek.skillhub.domain.namespace.Namespace; import com.iflytek.skillhub.domain.namespace.NamespaceRole; import com.iflytek.skillhub.domain.namespace.NamespaceRepository; @@ -7,21 +8,23 @@ import com.iflytek.skillhub.domain.namespace.NamespaceStatus; import com.iflytek.skillhub.domain.namespace.NamespaceService; import com.iflytek.skillhub.domain.skill.Skill; import com.iflytek.skillhub.domain.skill.SkillRepository; -import com.iflytek.skillhub.domain.skill.VisibilityChecker; import com.iflytek.skillhub.domain.skill.SkillVersionRepository; import com.iflytek.skillhub.domain.skill.SkillVisibility; import com.iflytek.skillhub.domain.skill.service.SkillLifecycleProjectionService; +import com.iflytek.skillhub.search.SearchQuery; import com.iflytek.skillhub.search.SearchQueryService; import com.iflytek.skillhub.search.SearchResult; -import org.mockito.ArgumentCaptor; +import com.iflytek.skillhub.search.SearchVisibilityScope; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; +import org.mockito.ArgumentCaptor; import org.mockito.junit.jupiter.MockitoExtension; import java.util.List; import java.util.Map; +import java.util.Set; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -49,6 +52,9 @@ class SkillSearchAppServiceTest { @Mock private NamespaceService namespaceService; + @Mock + private RbacService rbacService; + private SkillSearchAppService service; @BeforeEach @@ -58,7 +64,8 @@ class SkillSearchAppServiceTest { skillRepository, namespaceRepository, namespaceService, - new SkillLifecycleProjectionService(skillVersionRepository) + new SkillLifecycleProjectionService(skillVersionRepository), + rbacService ); } @@ -190,6 +197,40 @@ class SkillSearchAppServiceTest { assertEquals(List.of("code-generation", "official"), captor.getValue().labelSlugs()); } + @Test + void search_shouldIncludeMemberNamespacesInVisibilityScope() { + when(searchQueryService.search(any())) + .thenReturn(new SearchResult(List.of(), 0, 0, 20)); + when(rbacService.getUserRoleCodes("user-9")).thenReturn(Set.of("USER")); + + service.search("skill", null, "newest", 0, 20, "user-9", Map.of(7L, NamespaceRole.MEMBER)); + + ArgumentCaptor captor = ArgumentCaptor.forClass(SearchQuery.class); + verify(searchQueryService).search(captor.capture()); + + SearchVisibilityScope scope = captor.getValue().visibilityScope(); + assertEquals("user-9", scope.userId()); + assertEquals(Set.of(7L), scope.memberNamespaceIds()); + assertEquals(Set.of(), scope.adminNamespaceIds()); + assertEquals(false, scope.platformWideAccess()); + } + + @Test + void search_shouldGrantPlatformWideAccessToSuperAdmin() { + when(searchQueryService.search(any())) + .thenReturn(new SearchResult(List.of(), 0, 0, 20)); + when(rbacService.getUserRoleCodes("admin-1")).thenReturn(Set.of("SUPER_ADMIN", "USER")); + + service.search("skill", null, "newest", 0, 20, "admin-1", Map.of()); + + ArgumentCaptor captor = ArgumentCaptor.forClass(SearchQuery.class); + verify(searchQueryService).search(captor.capture()); + + SearchVisibilityScope scope = captor.getValue().visibilityScope(); + assertEquals("admin-1", scope.userId()); + assertEquals(true, scope.platformWideAccess()); + } + private void setField(Object target, String fieldName, Object value) { try { java.lang.reflect.Field field = target.getClass().getDeclaredField(fieldName); diff --git a/server/skillhub-search/src/main/java/com/iflytek/skillhub/search/SearchVisibilityScope.java b/server/skillhub-search/src/main/java/com/iflytek/skillhub/search/SearchVisibilityScope.java index 4f435e35..288ae5d2 100644 --- a/server/skillhub-search/src/main/java/com/iflytek/skillhub/search/SearchVisibilityScope.java +++ b/server/skillhub-search/src/main/java/com/iflytek/skillhub/search/SearchVisibilityScope.java @@ -8,9 +8,17 @@ import java.util.Set; public record SearchVisibilityScope( String userId, Set memberNamespaceIds, - Set adminNamespaceIds + Set adminNamespaceIds, + boolean platformWideAccess ) { + public SearchVisibilityScope( + String userId, + Set memberNamespaceIds, + Set adminNamespaceIds) { + this(userId, memberNamespaceIds, adminNamespaceIds, false); + } + public static SearchVisibilityScope anonymous() { - return new SearchVisibilityScope(null, Set.of(), Set.of()); + return new SearchVisibilityScope(null, Set.of(), Set.of(), false); } } diff --git a/server/skillhub-search/src/main/java/com/iflytek/skillhub/search/postgres/PostgresFullTextQueryService.java b/server/skillhub-search/src/main/java/com/iflytek/skillhub/search/postgres/PostgresFullTextQueryService.java index 8ec3cd37..e6888827 100644 --- a/server/skillhub-search/src/main/java/com/iflytek/skillhub/search/postgres/PostgresFullTextQueryService.java +++ b/server/skillhub-search/src/main/java/com/iflytek/skillhub/search/postgres/PostgresFullTextQueryService.java @@ -105,6 +105,7 @@ public class PostgresFullTextQueryService implements SearchQueryService { Set adminNamespaceIds = query.visibilityScope().adminNamespaceIds().isEmpty() ? Set.of(-1L) : query.visibilityScope().adminNamespaceIds(); + boolean platformWideAccess = query.visibilityScope().platformWideAccess(); StringBuilder sql = new StringBuilder(); sql.append("SELECT d.skill_id "); @@ -117,7 +118,9 @@ public class PostgresFullTextQueryService implements SearchQueryService { sql.append("AND (d.visibility = 'PUBLIC' "); if (query.visibilityScope().userId() != null) { sql.append("OR (d.visibility = 'NAMESPACE_ONLY' AND d.namespace_id IN :memberNamespaceIds) "); + sql.append("OR (d.visibility = 'NAMESPACE_ONLY' AND :platformWideAccess = TRUE) "); sql.append("OR (d.visibility = 'PRIVATE' AND (d.namespace_id IN :adminNamespaceIds OR d.owner_id = :userId)) "); + sql.append("OR (d.visibility = 'PRIVATE' AND :platformWideAccess = TRUE) "); } sql.append(") "); @@ -128,6 +131,7 @@ public class PostgresFullTextQueryService implements SearchQueryService { sql.append("AND (n.status <> 'ARCHIVED' "); if (query.visibilityScope().userId() != null) { sql.append("OR d.namespace_id IN :memberNamespaceIds "); + sql.append("OR :platformWideAccess = TRUE "); } sql.append(") "); @@ -192,6 +196,7 @@ public class PostgresFullTextQueryService implements SearchQueryService { if (query.visibilityScope().userId() != null) { nativeQuery.setParameter("memberNamespaceIds", memberNamespaceIds); nativeQuery.setParameter("adminNamespaceIds", adminNamespaceIds); + nativeQuery.setParameter("platformWideAccess", platformWideAccess); nativeQuery.setParameter("userId", query.visibilityScope().userId()); } @@ -238,6 +243,7 @@ public class PostgresFullTextQueryService implements SearchQueryService { if (query.visibilityScope().userId() != null) { countQuery.setParameter("memberNamespaceIds", memberNamespaceIds); countQuery.setParameter("adminNamespaceIds", adminNamespaceIds); + countQuery.setParameter("platformWideAccess", platformWideAccess); countQuery.setParameter("userId", query.visibilityScope().userId()); } diff --git a/server/skillhub-search/src/test/java/com/iflytek/skillhub/search/postgres/PostgresFullTextQueryServiceTest.java b/server/skillhub-search/src/test/java/com/iflytek/skillhub/search/postgres/PostgresFullTextQueryServiceTest.java index 8fb6c448..ea33092b 100644 --- a/server/skillhub-search/src/test/java/com/iflytek/skillhub/search/postgres/PostgresFullTextQueryServiceTest.java +++ b/server/skillhub-search/src/test/java/com/iflytek/skillhub/search/postgres/PostgresFullTextQueryServiceTest.java @@ -392,6 +392,40 @@ class PostgresFullTextQueryServiceTest { assertThat(sqlCaptor.getAllValues().getFirst()).contains("OR d.namespace_id IN :memberNamespaceIds"); } + @Test + void platformWideAccessShouldBypassNamespaceVisibilityRestrictions() { + EntityManager entityManager = mock(EntityManager.class); + Query nativeQuery = mock(Query.class); + Query countQuery = mock(Query.class); + when(entityManager.createNativeQuery(anyString())) + .thenReturn(nativeQuery) + .thenReturn(countQuery); + when(nativeQuery.setParameter(anyString(), org.mockito.ArgumentMatchers.any())).thenReturn(nativeQuery); + when(countQuery.setParameter(anyString(), org.mockito.ArgumentMatchers.any())).thenReturn(countQuery); + when(nativeQuery.getResultList()).thenReturn(List.of()); + when(countQuery.getSingleResult()).thenReturn(0L); + + PostgresFullTextQueryService service = new PostgresFullTextQueryService(entityManager); + + service.search(new SearchQuery( + null, + null, + new SearchVisibilityScope("admin-1", Set.of(), Set.of(), true), + "newest", + 0, + 12 + )); + + ArgumentCaptor sqlCaptor = ArgumentCaptor.forClass(String.class); + verify(entityManager, org.mockito.Mockito.times(2)).createNativeQuery(sqlCaptor.capture()); + assertThat(sqlCaptor.getAllValues().getFirst()) + .contains("OR (d.visibility = 'NAMESPACE_ONLY' AND :platformWideAccess = TRUE)") + .contains("OR (d.visibility = 'PRIVATE' AND :platformWideAccess = TRUE)") + .contains("OR :platformWideAccess = TRUE"); + verify(nativeQuery).setParameter("platformWideAccess", true); + verify(countQuery).setParameter("platformWideAccess", true); + } + @Test void maliciousKeywordShouldBeBoundAsParameterInsteadOfInlinedIntoSql() { EntityManager entityManager = mock(EntityManager.class); From cd7c1ba38462e1651c2238be537a844ec72b9578 Mon Sep 17 00:00:00 2001 From: dongmucat <70678707+dongmucat@users.noreply.github.com> Date: Thu, 9 Apr 2026 18:12:02 +0800 Subject: [PATCH 2/6] fix(review): correct review queue totals (#265) --- .../service/ReviewPortalAppService.java | 13 +- .../NamespaceWorkflowContractTest.java | 8 ++ .../ReviewPortalControllerTest.java | 111 +++++++++++++++--- 3 files changed, 115 insertions(+), 17 deletions(-) diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/ReviewPortalAppService.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/ReviewPortalAppService.java index c9fa8b15..47d8eed5 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/ReviewPortalAppService.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/ReviewPortalAppService.java @@ -119,6 +119,7 @@ public class ReviewPortalAppService { Map userNsRoles) { ReviewTaskStatus reviewStatus = ReviewTaskStatus.valueOf(status.toUpperCase()); Map namespaceRoles = normalizeRoles(userNsRoles); + Set platformRoles = platformRoles(userId); Pageable pageable = buildReviewPageable(reviewStatus, page, size, sortDirection); Page tasks; @@ -131,11 +132,14 @@ public class ReviewPortalAppService { userId, namespace.getType(), namespaceRoles, - platformRoles(userId))) { + platformRoles)) { throw new DomainForbiddenException("review.no_permission"); } tasks = reviewTaskRepository.findByNamespaceIdAndStatus(namespaceId, reviewStatus, pageable); } else { + if (!hasPlatformReviewRole(platformRoles)) { + throw new DomainForbiddenException("review.no_permission"); + } tasks = reviewTaskRepository.findByStatus(reviewStatus, pageable); } @@ -146,7 +150,7 @@ public class ReviewPortalAppService { return PageResponse.from(new PageImpl<>( governanceQueryRepository.getReviewTaskResponses(visibleItems), tasks.getPageable(), - visibleItems.size() + tasks.getTotalElements() )); } @@ -233,6 +237,11 @@ public class ReviewPortalAppService { return rbacService.getUserRoleCodes(userId); } + private boolean hasPlatformReviewRole(Set platformRoles) { + return platformRoles.contains("SKILL_ADMIN") + || platformRoles.contains("SUPER_ADMIN"); + } + private Map normalizeRoles(Map userNsRoles) { return userNsRoles != null ? userNsRoles : Map.of(); } diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/NamespaceWorkflowContractTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/NamespaceWorkflowContractTest.java index 8c796f22..af279dc4 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/NamespaceWorkflowContractTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/NamespaceWorkflowContractTest.java @@ -12,6 +12,8 @@ import com.iflytek.skillhub.domain.namespace.NamespaceRole; import com.iflytek.skillhub.domain.namespace.NamespaceService; import com.iflytek.skillhub.domain.namespace.NamespaceStatus; import com.iflytek.skillhub.domain.namespace.NamespaceType; +import com.iflytek.skillhub.domain.user.UserAccount; +import com.iflytek.skillhub.domain.user.UserAccountRepository; import com.iflytek.skillhub.dto.NamespaceCandidateUserResponse; import com.iflytek.skillhub.service.NamespaceMemberCandidateService; import org.junit.jupiter.api.Test; @@ -26,6 +28,7 @@ import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.request.RequestPostProcessor; import java.util.List; +import java.util.Optional; import java.util.Set; import static org.mockito.ArgumentMatchers.any; @@ -66,6 +69,9 @@ class NamespaceWorkflowContractTest { @MockBean private NamespaceMemberCandidateService namespaceMemberCandidateService; + @MockBean + private UserAccountRepository userAccountRepository; + @MockBean private DeviceAuthService deviceAuthService; @@ -92,6 +98,8 @@ class NamespaceWorkflowContractTest { .willReturn(new org.springframework.data.domain.PageImpl<>(List.of(adminMember))); given(namespaceMemberService.updateMemberRole(7L, "user-admin", NamespaceRole.ADMIN, "owner-1")) .willReturn(adminMember); + given(userAccountRepository.findById("user-admin")) + .willReturn(Optional.of(new UserAccount("user-admin", "Admin", "admin@example.com", null))); mockMvc.perform(post("/api/web/namespaces") .with(csrf()) diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/ReviewPortalControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/ReviewPortalControllerTest.java index bd4914d2..a69ec302 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/ReviewPortalControllerTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/ReviewPortalControllerTest.java @@ -41,6 +41,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.stream.IntStream; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.never; @@ -221,6 +222,7 @@ class ReviewPortalControllerTest { @Test void listReviews_appliesRequestedTimeSortDirection() throws Exception { stubNamespaceRoles("admin", List.of()); + given(rbacService.getUserRoleCodes("admin")).willReturn(Set.of("SKILL_ADMIN")); PageRequest pageable = PageRequest.of( 1, 5, @@ -244,6 +246,35 @@ class ReviewPortalControllerTest { verify(reviewTaskRepository).findByStatus(ReviewTaskStatus.APPROVED, pageable); } + @Test + void listReviews_preservesRepositoryTotalForDefaultPageSize() throws Exception { + assertReviewTotalPreservedForDefaultPageSize(ReviewTaskStatus.PENDING); + } + + @Test + void listApprovedReviews_preservesRepositoryTotalForDefaultPageSize() throws Exception { + assertReviewTotalPreservedForDefaultPageSize(ReviewTaskStatus.APPROVED); + } + + @Test + void listRejectedReviews_preservesRepositoryTotalForDefaultPageSize() throws Exception { + assertReviewTotalPreservedForDefaultPageSize(ReviewTaskStatus.REJECTED); + } + + @Test + void listReviews_forbidsGlobalQueueForNonPlatformReviewer() throws Exception { + stubNamespaceRoles("namespace-admin", List.of()); + given(rbacService.getUserRoleCodes("namespace-admin")).willReturn(Set.of("NAMESPACE_ADMIN")); + + mockMvc.perform(get("/api/v1/reviews") + .param("status", "PENDING") + .with(auth("namespace-admin"))) + .andExpect(status().isForbidden()) + .andExpect(jsonPath("$.code").value(403)); + + verify(reviewTaskRepository, never()).findByStatus(org.mockito.ArgumentMatchers.any(), org.mockito.ArgumentMatchers.any()); + } + @Test void downloadReviewVersion_streamsZipForAuthorizedReviewer() throws Exception { stubNamespaceRoles("admin", List.of()); @@ -264,21 +295,7 @@ class ReviewPortalControllerTest { } private void stubReviewResponse(ReviewTask task) { - given(governanceQueryRepository.getReviewTaskResponse(task)).willReturn(new ReviewTaskResponse( - task.getId(), - task.getSkillVersionId(), - "team-a", - "skill-a", - "1.0.0", - task.getStatus().name(), - task.getSubmittedBy(), - "Submitter", - task.getReviewedBy(), - null, - task.getReviewComment(), - task.getSubmittedAt(), - task.getReviewedAt() - )); + given(governanceQueryRepository.getReviewTaskResponse(task)).willReturn(toReviewResponse(task)); } private void stubNamespaceRoles(String userId, List members) { @@ -309,12 +326,76 @@ class ReviewPortalControllerTest { return task; } + private ReviewTask createReviewTask(Long id, Long namespaceId, String submittedBy, ReviewTaskStatus status) { + ReviewTask task = createReviewTask(id, namespaceId, submittedBy); + setField(task, "status", status); + return task; + } + private Namespace createNamespace(Long id, String slug) { Namespace namespace = new Namespace(slug, "Team", "owner-1"); setField(namespace, "id", id); return namespace; } + private ReviewTaskResponse toReviewResponse(ReviewTask task) { + return new ReviewTaskResponse( + task.getId(), + task.getSkillVersionId(), + "team-a", + "skill-a", + "1.0.0", + task.getStatus().name(), + task.getSubmittedBy(), + "Submitter", + task.getReviewedBy(), + null, + task.getReviewComment(), + task.getSubmittedAt(), + task.getReviewedAt() + ); + } + + private void assertReviewTotalPreservedForDefaultPageSize(ReviewTaskStatus status) throws Exception { + stubNamespaceRoles("admin", List.of()); + Namespace namespace = createNamespace(20L, "team-a"); + List tasks = IntStream.rangeClosed(1, 20) + .mapToObj(index -> createReviewTask((long) index, 20L, "submitter-" + index, status)) + .toList(); + List responses = tasks.stream() + .map(this::toReviewResponse) + .toList(); + PageRequest pageable = PageRequest.of( + 0, + 20, + Sort.by( + new Sort.Order(Sort.Direction.DESC, status == ReviewTaskStatus.PENDING ? "submittedAt" : "reviewedAt"), + new Sort.Order(Sort.Direction.DESC, "id") + ) + ); + + given(reviewTaskRepository.findByStatus(status, pageable)) + .willReturn(new PageImpl<>(tasks, pageable, 42)); + given(namespaceRepository.findById(20L)).willReturn(Optional.of(namespace)); + given(rbacService.getUserRoleCodes("admin")).willReturn(Set.of("SKILL_ADMIN")); + tasks.forEach(task -> given(reviewService.canViewReview( + task, + "admin", + namespace.getType(), + Map.of(), + Set.of("SKILL_ADMIN"))).willReturn(true)); + given(governanceQueryRepository.getReviewTaskResponses(tasks)).willReturn(responses); + + mockMvc.perform(get("/api/v1/reviews") + .param("status", status.name()) + .with(auth("admin"))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.code").value(0)) + .andExpect(jsonPath("$.data.total").value(42)) + .andExpect(jsonPath("$.data.size").value(20)) + .andExpect(jsonPath("$.data.items.length()").value(20)); + } + private void setField(Object target, String fieldName, Object value) { try { java.lang.reflect.Field field = target.getClass().getDeclaredField(fieldName); From 4cc22d0099b824764176a7261563ebfc5042ccfe Mon Sep 17 00:00:00 2001 From: dongmucat <70678707+dongmucat@users.noreply.github.com> Date: Thu, 9 Apr 2026 18:12:24 +0800 Subject: [PATCH 3/6] fix(auth): avoid extra session rotation after oauth success (#245) --- .../skillhub/auth/oauth/OAuth2LoginSuccessHandler.java | 2 +- .../skillhub/auth/oauth/OAuth2LoginHandlersTest.java | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/OAuth2LoginSuccessHandler.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/OAuth2LoginSuccessHandler.java index b87c0018..0a27e6bd 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/OAuth2LoginSuccessHandler.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/OAuth2LoginSuccessHandler.java @@ -35,7 +35,7 @@ public class OAuth2LoginSuccessHandler extends SavedRequestAwareAuthenticationSu if (authentication.getPrincipal() instanceof OAuth2User oAuth2User) { PlatformPrincipal principal = (PlatformPrincipal) oAuth2User.getAttributes().get("platformPrincipal"); if (principal != null) { - platformSessionService.attachToAuthenticatedSession(principal, authentication, request, true); + platformSessionService.attachToAuthenticatedSession(principal, authentication, request); } } String returnTo = oauthLoginFlowService.consumeReturnTo(request.getSession(false)); diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/OAuth2LoginHandlersTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/OAuth2LoginHandlersTest.java index 592151fa..6bf8c098 100644 --- a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/OAuth2LoginHandlersTest.java +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/OAuth2LoginHandlersTest.java @@ -6,6 +6,7 @@ import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.Authentication; +import org.springframework.security.core.context.SecurityContext; import org.springframework.security.oauth2.core.OAuth2AuthenticationException; import org.springframework.security.oauth2.core.OAuth2Error; import org.springframework.security.oauth2.core.user.DefaultOAuth2User; @@ -52,11 +53,15 @@ class OAuth2LoginHandlersTest { handler.onAuthenticationSuccess(request, response, authentication); + SecurityContext securityContext = (SecurityContext) session.getAttribute( + HttpSessionSecurityContextRepository.SPRING_SECURITY_CONTEXT_KEY + ); assertThat(response.getRedirectedUrl()).isEqualTo("/dashboard/publish"); - assertThat(request.getSession(false).getId()).isNotEqualTo(originalSessionId); + assertThat(request.getSession(false).getId()).isEqualTo(originalSessionId); assertThat(session.getAttribute(OAuthLoginRedirectSupport.SESSION_RETURN_TO_ATTRIBUTE)).isNull(); assertThat(session.getAttribute("platformPrincipal")).isEqualTo(principal); - assertThat(session.getAttribute(HttpSessionSecurityContextRepository.SPRING_SECURITY_CONTEXT_KEY)).isNotNull(); + assertThat(securityContext).isNotNull(); + assertThat(securityContext.getAuthentication()).isSameAs(authentication); } @Test From 0497f8654fc7daabff9a2225d61ab3aedad00081 Mon Sep 17 00:00:00 2001 From: dongmucat <70678707+dongmucat@users.noreply.github.com> Date: Thu, 9 Apr 2026 18:13:08 +0800 Subject: [PATCH 4/6] fix(validation): avoid token false positives in pre-publish check (#253) --- .../validation/BasicPrePublishValidator.java | 75 +++++++++++++++++-- .../BasicPrePublishValidatorTest.java | 67 +++++++++++++++++ 2 files changed, 137 insertions(+), 5 deletions(-) diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/BasicPrePublishValidator.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/BasicPrePublishValidator.java index c61e420b..3709bece 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/BasicPrePublishValidator.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/BasicPrePublishValidator.java @@ -16,6 +16,12 @@ import java.util.regex.Pattern; @Component public class BasicPrePublishValidator implements PrePublishValidator { + private static final Pattern ASSIGNMENT_WITH_SENSITIVE_KEY = Pattern.compile( + "(?i)(api[_-]?key|access[_-]?key|secret|password|token)\\s*[:=]\\s*(.+)$" + ); + private static final Pattern QUOTED_LITERAL = Pattern.compile("^(['\"])(.*)\\1$"); + private static final Pattern IDENTIFIER = Pattern.compile("[A-Za-z_][A-Za-z0-9_]*"); + private static final Pattern BARE_LITERAL = Pattern.compile("[A-Za-z0-9_\\-]{12,}"); private static final Pattern PLACEHOLDER_VALUE = Pattern.compile( "(?i).*(your|example|sample|placeholder|changeme|replace|dummy|mock|test|fake|todo|xxx|redacted).*" ); @@ -23,10 +29,7 @@ public class BasicPrePublishValidator implements PrePublishValidator { new SecretRule(Pattern.compile("(AKIA[0-9A-Z]{16})"), 1, "cloud access key"), new SecretRule(Pattern.compile("(ghp_[A-Za-z0-9]{20,})"), 1, "GitHub token"), new SecretRule(Pattern.compile("(sk-[A-Za-z0-9]{20,})"), 1, "API key"), - new SecretRule( - Pattern.compile("(?i)(api[_-]?key|access[_-]?key|secret|password|token)\\s*[:=]\\s*['\\\"]?([A-Za-z0-9_\\-]{12,})"), - 2, - "secret or token") + new SecretRule(ASSIGNMENT_WITH_SENSITIVE_KEY, 0, "secret or token") ); @Override @@ -46,7 +49,10 @@ public class BasicPrePublishValidator implements PrePublishValidator { if (!matcher.find()) { continue; } - String matchedValue = matcher.group(rule.valueGroup()); + String matchedValue = extractMatchedValue(line, matcher, rule); + if (matchedValue == null) { + continue; + } if (isPlaceholderValue(matchedValue)) { continue; } @@ -87,5 +93,64 @@ public class BasicPrePublishValidator implements PrePublishValidator { || value.chars().allMatch(ch -> ch == 'x' || ch == 'X' || ch == '*' || ch == '-'); } + private String extractMatchedValue(String line, Matcher matcher, SecretRule rule) { + if (rule.valueGroup() > 0) { + return matcher.group(rule.valueGroup()); + } + + Matcher assignmentMatcher = ASSIGNMENT_WITH_SENSITIVE_KEY.matcher(line); + if (!assignmentMatcher.find()) { + return null; + } + + String rawValue = assignmentMatcher.group(2).trim(); + if (rawValue.isBlank()) { + return null; + } + + Matcher quotedLiteralMatcher = QUOTED_LITERAL.matcher(rawValue); + if (quotedLiteralMatcher.matches()) { + return quotedLiteralMatcher.group(2); + } + + rawValue = stripInlineComment(rawValue); + if (rawValue.isBlank()) { + return null; + } + + quotedLiteralMatcher = QUOTED_LITERAL.matcher(rawValue); + if (quotedLiteralMatcher.matches()) { + return quotedLiteralMatcher.group(2); + } + + if (looksLikeExpression(rawValue) || IDENTIFIER.matcher(rawValue).matches()) { + return null; + } + + return BARE_LITERAL.matcher(rawValue).matches() ? rawValue : null; + } + + private String stripInlineComment(String rawValue) { + int hashIndex = rawValue.indexOf('#'); + if (hashIndex >= 0) { + return rawValue.substring(0, hashIndex).trim(); + } + return rawValue; + } + + private boolean looksLikeExpression(String rawValue) { + return rawValue.contains("(") + || rawValue.contains(")") + || rawValue.contains(".") + || rawValue.contains("[") + || rawValue.contains("]") + || rawValue.contains("{") + || rawValue.contains("}") + || rawValue.contains(",") + || rawValue.contains(" ") + || rawValue.contains("+") + || rawValue.contains("/"); + } + private record SecretRule(Pattern pattern, int valueGroup, String label) {} } diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/validation/BasicPrePublishValidatorTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/validation/BasicPrePublishValidatorTest.java index a40eb5c3..f5f2ad00 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/validation/BasicPrePublishValidatorTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/validation/BasicPrePublishValidatorTest.java @@ -98,4 +98,71 @@ class BasicPrePublishValidatorTest { assertTrue(result.passed()); } + + @Test + void shouldAllowFunctionCallAssignedToTokenVariable() { + PackageEntry script = new PackageEntry( + "scripts/f2e_mock.py", + """ + token = extract_group_token_value(response, group_choice.group_id) + if token: + return token + """.getBytes(StandardCharsets.UTF_8), + 97, + "text/x-python" + ); + + ValidationResult result = validator.validate(new PrePublishValidator.SkillPackageContext( + List.of(script), + new SkillMetadata("Safe Skill", "desc", "1.0.0", "body", Map.of()), + "user-1", + 1L + )); + + assertTrue(result.passed()); + } + + @Test + void shouldAllowIdentifierAssignedToSecretNamedVariable() { + PackageEntry envTemplate = new PackageEntry( + "config.env", + """ + token=generated_token_value + api_key=current_api_key + """.getBytes(StandardCharsets.UTF_8), + 46, + "text/plain" + ); + + ValidationResult result = validator.validate(new PrePublishValidator.SkillPackageContext( + List.of(envTemplate), + new SkillMetadata("Safe Skill", "desc", "1.0.0", "body", Map.of()), + "user-1", + 1L + )); + + assertTrue(result.passed()); + } + + @Test + void shouldRejectQuotedSecretWithTrailingComment() { + PackageEntry script = new PackageEntry( + "scripts/publish.py", + """ + token = "ghp_abcdefghijklmnopqrstuvwxyz1234" # do not commit real token + """.getBytes(StandardCharsets.UTF_8), + 76, + "text/x-python" + ); + + ValidationResult result = validator.validate(new PrePublishValidator.SkillPackageContext( + List.of(script), + new SkillMetadata("Secret Skill", "desc", "1.0.0", "body", Map.of()), + "user-1", + 1L + )); + + assertFalse(result.passed()); + assertTrue(result.errors().stream().anyMatch(error -> error.contains("scripts/publish.py"))); + } } From 689e698b89ded12a0ee2e2da4fb380c4438b3a68 Mon Sep 17 00:00:00 2001 From: wowo Date: Thu, 9 Apr 2026 18:45:14 +0800 Subject: [PATCH 5/6] feat(ci): add PR batch test deployment workflow (#275) * feat(ci): add PR batch test deployment workflow * fix(ci): support local PR batch rehearsal --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .github/workflows/pr-batch-test-deploy.yml | 161 +++++++++++++++++++ docs/pr-batch-test-runtime.md | 81 ++++++++++ scripts/deploy-test-runtime.sh | 128 +++++++++++++++ scripts/prepare-pr-batch.sh | 174 +++++++++++++++++++++ scripts/skillhub-test-deploy-remote.sh | 135 ++++++++++++++++ 5 files changed, 679 insertions(+) create mode 100644 .github/workflows/pr-batch-test-deploy.yml create mode 100644 docs/pr-batch-test-runtime.md create mode 100755 scripts/deploy-test-runtime.sh create mode 100755 scripts/prepare-pr-batch.sh create mode 100644 scripts/skillhub-test-deploy-remote.sh diff --git a/.github/workflows/pr-batch-test-deploy.yml b/.github/workflows/pr-batch-test-deploy.yml new file mode 100644 index 00000000..f61cdef6 --- /dev/null +++ b/.github/workflows/pr-batch-test-deploy.yml @@ -0,0 +1,161 @@ +name: PR Batch Test Deploy + +on: + workflow_dispatch: + inputs: + pr_numbers: + description: "Comma/newline separated PR numbers to merge onto the base branch" + required: true + type: string + base_ref: + description: "Base branch to build from" + required: false + default: main + type: string + deploy_channel: + description: "Floating image tag used by the shared HK test machine" + required: false + default: manual-test-hk + type: string + +concurrency: + group: pr-batch-test-runtime + cancel-in-progress: false + +permissions: + contents: read + packages: write + pull-requests: read + +env: + DOCKER_PLATFORM: linux/amd64 + +jobs: + build-and-deploy: + name: Build And Deploy Manual Test Batch + runs-on: ubuntu-latest + timeout-minutes: 120 + + steps: + - name: Check out repository + uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Ensure helper scripts are executable + run: chmod +x scripts/prepare-pr-batch.sh scripts/deploy-test-runtime.sh + + - name: Validate deploy secrets + env: + TEST_RUNTIME_SSH_HOST: ${{ secrets.TEST_RUNTIME_SSH_HOST }} + TEST_RUNTIME_SSH_KEY: ${{ secrets.TEST_RUNTIME_SSH_KEY }} + run: | + [[ -n "${TEST_RUNTIME_SSH_HOST}" ]] || { echo "::error::Missing secret TEST_RUNTIME_SSH_HOST"; exit 1; } + [[ -n "${TEST_RUNTIME_SSH_KEY}" ]] || { echo "::error::Missing secret TEST_RUNTIME_SSH_KEY"; exit 1; } + + - name: Set up Docker Buildx + uses: docker/setup-buildx-action@v3 + + - name: Log in to GHCR + uses: docker/login-action@v3 + with: + registry: ghcr.io + username: ${{ github.actor }} + password: ${{ secrets.GITHUB_TOKEN }} + + - name: Merge selected PRs onto base ref + id: batch + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + scripts/prepare-pr-batch.sh \ + --pr-list "${{ inputs.pr_numbers }}" \ + --base-ref "${{ inputs.base_ref }}" \ + --deploy-channel "${{ inputs.deploy_channel }}" + + - name: Build and push backend image + uses: docker/build-push-action@v6 + with: + context: ./server + file: ./server/Dockerfile + platforms: ${{ env.DOCKER_PLATFORM }} + push: true + provenance: false + sbom: false + tags: | + ghcr.io/${{ github.repository_owner }}/skillhub-server:${{ steps.batch.outputs.deploy_tag }} + ghcr.io/${{ github.repository_owner }}/skillhub-server:${{ steps.batch.outputs.immutable_tag }} + cache-from: type=gha,scope=manual-test-server + cache-to: type=gha,mode=max,scope=manual-test-server + + - name: Build and push frontend image + uses: docker/build-push-action@v6 + with: + context: ./web + file: ./web/Dockerfile + platforms: ${{ env.DOCKER_PLATFORM }} + push: true + provenance: false + sbom: false + tags: | + ghcr.io/${{ github.repository_owner }}/skillhub-web:${{ steps.batch.outputs.deploy_tag }} + ghcr.io/${{ github.repository_owner }}/skillhub-web:${{ steps.batch.outputs.immutable_tag }} + cache-from: type=gha,scope=manual-test-web + cache-to: type=gha,mode=max,scope=manual-test-web + + - name: Build and push scanner image + uses: docker/build-push-action@v6 + with: + context: ./scanner + file: ./scanner/Dockerfile + platforms: ${{ env.DOCKER_PLATFORM }} + push: true + provenance: false + sbom: false + tags: | + ghcr.io/${{ github.repository_owner }}/skillhub-scanner:${{ steps.batch.outputs.deploy_tag }} + ghcr.io/${{ github.repository_owner }}/skillhub-scanner:${{ steps.batch.outputs.immutable_tag }} + cache-from: type=gha,scope=manual-test-scanner + cache-to: type=gha,mode=max,scope=manual-test-scanner + + - name: Prepare deploy key + id: ssh + env: + TEST_RUNTIME_SSH_KEY: ${{ secrets.TEST_RUNTIME_SSH_KEY }} + run: | + key_file="${RUNNER_TEMP}/test-runtime.key" + printf '%s\n' "${TEST_RUNTIME_SSH_KEY}" > "${key_file}" + chmod 600 "${key_file}" + echo "key_file=${key_file}" >> "${GITHUB_OUTPUT}" + + - name: Deploy batch images to HK test runtime + env: + TEST_RUNTIME_SSH_HOST: ${{ secrets.TEST_RUNTIME_SSH_HOST }} + TEST_RUNTIME_SSH_USER: ${{ secrets.TEST_RUNTIME_SSH_USER }} + TEST_RUNTIME_SSH_PORT: ${{ secrets.TEST_RUNTIME_SSH_PORT }} + run: | + ssh_port="${TEST_RUNTIME_SSH_PORT:-22}" + ssh_user="${TEST_RUNTIME_SSH_USER:-skillhub-deploy}" + scripts/deploy-test-runtime.sh \ + --host "${TEST_RUNTIME_SSH_HOST}" \ + --user "${ssh_user}" \ + --port "${ssh_port}" \ + --key-file "${{ steps.ssh.outputs.key_file }}" \ + --deploy-tag "${{ steps.batch.outputs.deploy_tag }}" \ + --immutable-tag "${{ steps.batch.outputs.immutable_tag }}" \ + --merged-sha "${{ steps.batch.outputs.merged_sha }}" \ + --pr-csv "${{ steps.batch.outputs.pr_csv }}" \ + --run-url "https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}" + + - name: Publish final summary + run: | + { + echo "### HK manual test runtime updated" + echo + echo "- URL: \`https://skill.xf-yun.com.cn\`" + echo "- Base ref: \`${{ steps.batch.outputs.base_ref }}\`" + echo "- Floating tag: \`${{ steps.batch.outputs.deploy_tag }}\`" + echo "- Immutable tag: \`${{ steps.batch.outputs.immutable_tag }}\`" + echo "- Merged SHA: \`${{ steps.batch.outputs.merged_sha }}\`" + echo "- PR list: \`${{ steps.batch.outputs.pr_csv }}\`" + } >> "${GITHUB_STEP_SUMMARY}" diff --git a/docs/pr-batch-test-runtime.md b/docs/pr-batch-test-runtime.md new file mode 100644 index 00000000..6f6d1c88 --- /dev/null +++ b/docs/pr-batch-test-runtime.md @@ -0,0 +1,81 @@ +# PR Batch Test Runtime + +This repository includes a manual GitHub Actions workflow that builds a +synthetic test image set from multiple PRs and deploys it to the shared +Hong Kong manual-test machine. + +Workflow file: + +- `.github/workflows/pr-batch-test-deploy.yml` + +## What the workflow does + +When you trigger the workflow manually, it: + +1. checks out the repository and fetches the selected base branch +2. parses the PR list you provide and deduplicates it while preserving order +3. verifies that every PR is still open and targets the chosen base branch +4. merges the selected PR heads onto the base branch in the exact order you supplied +5. fails fast if any PR conflicts with the base branch or with an earlier PR in the batch +6. builds `server`, `web`, and `scanner` images for `linux/amd64` +7. pushes both a floating tag and an immutable tag to GHCR +8. SSHes into the HK test machine as a dedicated deploy user +9. calls a root-owned deployment wrapper through `sudo` +10. updates `/opt/skillhub-runtime/.env.release` and runs `docker compose pull && docker compose up -d` + +The floating tag is the shared environment channel. By default it is +`manual-test-hk`. Each run also pushes an immutable tag for traceability: + +- floating tag example: `manual-test-hk` +- immutable tag example: `manual-test-hk-128-3d4a8e7f9a1b` + +The runtime always deploys the floating tag, so the same test URL keeps +working while still letting maintainers look up the exact image version +used by a given run. + +## Required GitHub secrets + +Add these repository or environment secrets before using the workflow: + +- `TEST_RUNTIME_SSH_HOST`: test machine hostname or IP +- `TEST_RUNTIME_SSH_KEY`: private key content used by GitHub Actions + +Optional secrets: + +- `TEST_RUNTIME_SSH_USER`: defaults to `skillhub-deploy` +- `TEST_RUNTIME_SSH_PORT`: defaults to `22` + +The remote machine should expose a root-owned deployment command at: + +- `/usr/local/bin/skillhub-test-deploy` + +The dedicated deploy user is expected to have passwordless sudo access to +that command only. + +## Recommended usage + +Open the workflow in GitHub Actions and fill in: + +- `pr_numbers`: a comma-separated or newline-separated list such as `123, 124, 130` +- `base_ref`: usually `main` +- `deploy_channel`: keep the default `manual-test-hk` for the shared test machine + +The merge order matters. If PR `124` depends on `123`, list `123` first. + +## Runtime metadata on the server + +After deployment, the workflow writes a small metadata file here: + +- `/opt/skillhub-runtime/manual-test-deployment.txt` + +It records: + +- deploy time +- floating tag +- immutable tag +- merged synthetic SHA +- PR list +- GitHub Actions run URL + +This makes it easy for testers and maintainers to confirm which batch is +currently deployed. diff --git a/scripts/deploy-test-runtime.sh b/scripts/deploy-test-runtime.sh new file mode 100755 index 00000000..47a2f3cc --- /dev/null +++ b/scripts/deploy-test-runtime.sh @@ -0,0 +1,128 @@ +#!/usr/bin/env bash + +set -euo pipefail + +usage() { + cat <<'EOF' +Usage: scripts/deploy-test-runtime.sh [options] + +Options: + --host Remote SSH host + --user Remote SSH user. Default: skillhub-deploy + --port Remote SSH port. Default: 22 + --key-file SSH private key for deployment + --deploy-tag Floating image tag to deploy + --immutable-tag Immutable image tag for traceability + --merged-sha Synthetic merge commit SHA + --pr-csv Comma-separated PR numbers + --run-url GitHub Actions run URL +EOF +} + +ssh_host="" +ssh_user="skillhub-deploy" +ssh_port="22" +ssh_key_file="" +deploy_tag="" +immutable_tag="" +merged_sha="" +pr_csv="" +run_url="" + +while [[ $# -gt 0 ]]; do + case "$1" in + --host) + [[ $# -ge 2 ]] || { echo "Missing value for --host" >&2; exit 1; } + ssh_host="$2" + shift 2 + ;; + --user) + [[ $# -ge 2 ]] || { echo "Missing value for --user" >&2; exit 1; } + ssh_user="$2" + shift 2 + ;; + --port) + [[ $# -ge 2 ]] || { echo "Missing value for --port" >&2; exit 1; } + ssh_port="$2" + shift 2 + ;; + --key-file) + [[ $# -ge 2 ]] || { echo "Missing value for --key-file" >&2; exit 1; } + ssh_key_file="$2" + shift 2 + ;; + --deploy-tag) + [[ $# -ge 2 ]] || { echo "Missing value for --deploy-tag" >&2; exit 1; } + deploy_tag="$2" + shift 2 + ;; + --immutable-tag) + [[ $# -ge 2 ]] || { echo "Missing value for --immutable-tag" >&2; exit 1; } + immutable_tag="$2" + shift 2 + ;; + --merged-sha) + [[ $# -ge 2 ]] || { echo "Missing value for --merged-sha" >&2; exit 1; } + merged_sha="$2" + shift 2 + ;; + --pr-csv) + [[ $# -ge 2 ]] || { echo "Missing value for --pr-csv" >&2; exit 1; } + pr_csv="$2" + shift 2 + ;; + --run-url) + [[ $# -ge 2 ]] || { echo "Missing value for --run-url" >&2; exit 1; } + run_url="$2" + shift 2 + ;; + --help|-h) + usage + exit 0 + ;; + *) + echo "Unsupported argument: $1" >&2 + usage >&2 + exit 1 + ;; + esac +done + +[[ -n "${ssh_host}" ]] || { echo "--host is required" >&2; exit 1; } +[[ -n "${ssh_key_file}" ]] || { echo "--key-file is required" >&2; exit 1; } +[[ -n "${deploy_tag}" ]] || { echo "--deploy-tag is required" >&2; exit 1; } +[[ -n "${immutable_tag}" ]] || { echo "--immutable-tag is required" >&2; exit 1; } + +ssh_opts=( + -i "${ssh_key_file}" + -o BatchMode=yes + -o IdentitiesOnly=yes + -o StrictHostKeyChecking=accept-new + -o ServerAliveInterval=15 + -o ServerAliveCountMax=3 + -o TCPKeepAlive=yes + -o ConnectTimeout=10 + -p "${ssh_port}" +) + +ssh "${ssh_opts[@]}" "${ssh_user}@${ssh_host}" bash -s -- \ + "${deploy_tag}" \ + "${immutable_tag}" \ + "${merged_sha}" \ + "${pr_csv}" \ + "${run_url}" <<'EOF' +set -euo pipefail + +deploy_tag="$1" +immutable_tag="$2" +merged_sha="$3" +pr_csv="$4" +run_url="${5:-}" + +sudo /usr/local/bin/skillhub-test-deploy \ + --deploy-tag "${deploy_tag}" \ + --immutable-tag "${immutable_tag}" \ + --merged-sha "${merged_sha}" \ + --pr-csv "${pr_csv}" \ + --run-url "${run_url}" +EOF diff --git a/scripts/prepare-pr-batch.sh b/scripts/prepare-pr-batch.sh new file mode 100755 index 00000000..0b5f56a3 --- /dev/null +++ b/scripts/prepare-pr-batch.sh @@ -0,0 +1,174 @@ +#!/usr/bin/env bash + +set -euo pipefail + +usage() { + cat <<'EOF' +Usage: scripts/prepare-pr-batch.sh --pr-list "123,456" [options] + +Options: + --base-ref Base branch to merge onto. Default: main + --deploy-channel Floating image tag for the shared test runtime. + Default: manual-test-hk +EOF +} + +base_ref="main" +deploy_channel="manual-test-hk" +pr_input="" + +while [[ $# -gt 0 ]]; do + case "$1" in + --base-ref) + [[ $# -ge 2 ]] || { echo "Missing value for --base-ref" >&2; exit 1; } + base_ref="$2" + shift 2 + ;; + --deploy-channel) + [[ $# -ge 2 ]] || { echo "Missing value for --deploy-channel" >&2; exit 1; } + deploy_channel="$2" + shift 2 + ;; + --pr-list) + [[ $# -ge 2 ]] || { echo "Missing value for --pr-list" >&2; exit 1; } + pr_input="$2" + shift 2 + ;; + --help|-h) + usage + exit 0 + ;; + *) + echo "Unsupported argument: $1" >&2 + usage >&2 + exit 1 + ;; + esac +done + +: "${GH_TOKEN:?GH_TOKEN is required}" + +if [[ -z "${pr_input}" ]]; then + echo "--pr-list is required" >&2 + exit 1 +fi + +normalized_input="$(printf '%s' "${pr_input}" | tr ',;\r\n\t' ' ')" + +declare -a pr_numbers=() + +for token in ${normalized_input}; do + if [[ ! "${token}" =~ ^[0-9]+$ ]]; then + echo "Invalid PR number: ${token}" >&2 + exit 1 + fi + + already_seen=false + if [[ "${#pr_numbers[@]}" -gt 0 ]]; then + for existing in "${pr_numbers[@]}"; do + if [[ "${existing}" == "${token}" ]]; then + already_seen=true + break + fi + done + fi + + if [[ "${already_seen}" == "true" ]]; then + continue + fi + + pr_numbers+=("${token}") +done + +if [[ "${#pr_numbers[@]}" -eq 0 ]]; then + echo "No PR numbers were parsed from --pr-list" >&2 + exit 1 +fi + +sanitized_channel="$( + printf '%s' "${deploy_channel}" | + tr '[:upper:]' '[:lower:]' | + sed -E 's/[^a-z0-9._-]+/-/g; s/^-+//; s/-+$//; s/-{2,}/-/g' +)" + +if [[ -z "${sanitized_channel}" ]]; then + echo "Deploy channel resolved to an empty tag" >&2 + exit 1 +fi + +git config user.name "github-actions[bot]" +git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + +git fetch --no-tags origin "${base_ref}" +git checkout -B manual-test-batch "origin/${base_ref}" + +summary_file="${RUNNER_TEMP:-/tmp}/manual-test-batch-summary.md" +current_pr="" +trap 'status=$?; if [[ $status -ne 0 && -n "${current_pr}" ]]; then echo "Failed while merging PR #${current_pr}" >&2; fi' EXIT + +{ + echo "### Manual Test Batch" + echo + echo "- Base ref: \`${base_ref}\`" + echo "- Deploy channel: \`${sanitized_channel}\`" + echo "- Selected PRs:" +} > "${summary_file}" + +for pr in "${pr_numbers[@]}"; do + current_pr="${pr}" + + IFS=$'\t' read -r state pr_base is_draft title url <&2 + exit 1 + fi + + if [[ "${pr_base}" != "${base_ref}" ]]; then + echo "PR #${pr} targets ${pr_base}, expected ${base_ref}" >&2 + exit 1 + fi + + git fetch --no-tags origin "pull/${pr}/head:refs/remotes/origin/manual-test-pr-${pr}" + git merge --no-ff --no-edit \ + -m "Merge PR #${pr} for manual test batch" \ + "refs/remotes/origin/manual-test-pr-${pr}" + + if [[ "${is_draft}" == "true" ]]; then + title="${title} [draft]" + fi + + echo " - #${pr} ${title} (${url})" >> "${summary_file}" +done + +merged_sha="$(git rev-parse HEAD)" +short_sha="$(git rev-parse --short=12 HEAD)" +run_token="${GITHUB_RUN_NUMBER:-manual}" +immutable_tag="${sanitized_channel}-${run_token}-${short_sha}" +pr_csv="$(IFS=,; echo "${pr_numbers[*]}")" + +if [[ -n "${GITHUB_OUTPUT:-}" ]]; then + { + echo "base_ref=${base_ref}" + echo "deploy_tag=${sanitized_channel}" + echo "immutable_tag=${immutable_tag}" + echo "merged_sha=${merged_sha}" + echo "short_sha=${short_sha}" + echo "pr_csv=${pr_csv}" + echo "summary_file=${summary_file}" + } >> "${GITHUB_OUTPUT}" +fi + +{ + echo "- Merged SHA: \`${merged_sha}\`" + echo "- Floating tag: \`${sanitized_channel}\`" + echo "- Immutable tag: \`${immutable_tag}\`" +} >> "${summary_file}" + +if [[ -n "${GITHUB_STEP_SUMMARY:-}" ]]; then + cat "${summary_file}" >> "${GITHUB_STEP_SUMMARY}" +fi diff --git a/scripts/skillhub-test-deploy-remote.sh b/scripts/skillhub-test-deploy-remote.sh new file mode 100644 index 00000000..39663624 --- /dev/null +++ b/scripts/skillhub-test-deploy-remote.sh @@ -0,0 +1,135 @@ +#!/usr/bin/env bash + +set -euo pipefail + +usage() { + cat <<'EOF' +Usage: /usr/local/bin/skillhub-test-deploy [options] + +Options: + --deploy-tag Floating image tag to deploy + --immutable-tag Immutable image tag for traceability + --merged-sha Synthetic merge commit SHA + --pr-csv Comma-separated PR numbers + --run-url GitHub Actions run URL +EOF +} + +runtime_dir="/opt/skillhub-runtime" +deploy_tag="" +immutable_tag="" +merged_sha="" +pr_csv="" +run_url="" + +while [[ $# -gt 0 ]]; do + case "$1" in + --deploy-tag) + [[ $# -ge 2 ]] || { echo "Missing value for --deploy-tag" >&2; exit 1; } + deploy_tag="$2" + shift 2 + ;; + --immutable-tag) + [[ $# -ge 2 ]] || { echo "Missing value for --immutable-tag" >&2; exit 1; } + immutable_tag="$2" + shift 2 + ;; + --merged-sha) + [[ $# -ge 2 ]] || { echo "Missing value for --merged-sha" >&2; exit 1; } + merged_sha="$2" + shift 2 + ;; + --pr-csv) + [[ $# -ge 2 ]] || { echo "Missing value for --pr-csv" >&2; exit 1; } + pr_csv="$2" + shift 2 + ;; + --run-url) + [[ $# -ge 2 ]] || { echo "Missing value for --run-url" >&2; exit 1; } + run_url="$2" + shift 2 + ;; + --help|-h) + usage + exit 0 + ;; + *) + echo "Unsupported argument: $1" >&2 + usage >&2 + exit 1 + ;; + esac +done + +[[ -n "${deploy_tag}" ]] || { echo "--deploy-tag is required" >&2; exit 1; } +[[ -n "${immutable_tag}" ]] || { echo "--immutable-tag is required" >&2; exit 1; } + +if [[ ! "${deploy_tag}" =~ ^[a-z0-9._-]+$ ]]; then + echo "Invalid deploy tag: ${deploy_tag}" >&2 + exit 1 +fi + +if [[ ! "${immutable_tag}" =~ ^[a-z0-9._-]+$ ]]; then + echo "Invalid immutable tag: ${immutable_tag}" >&2 + exit 1 +fi + +if [[ -n "${merged_sha}" && ! "${merged_sha}" =~ ^[0-9a-f]{7,64}$ ]]; then + echo "Invalid merged SHA: ${merged_sha}" >&2 + exit 1 +fi + +if [[ -n "${pr_csv}" && ! "${pr_csv}" =~ ^[0-9]+(,[0-9]+)*$ ]]; then + echo "Invalid PR list: ${pr_csv}" >&2 + exit 1 +fi + +if [[ -n "${run_url}" && ! "${run_url}" =~ ^https://github\.com/.+/actions/runs/[0-9]+$ ]]; then + echo "Invalid run URL: ${run_url}" >&2 + exit 1 +fi + +set_env_value() { + key="$1" + value="$2" + tmp=".env.release.tmp" + + if grep -q "^${key}=" .env.release; then + sed "s|^${key}=.*|${key}=${value}|" .env.release > "${tmp}" + else + cp .env.release "${tmp}" + printf '%s=%s\n' "${key}" "${value}" >> "${tmp}" + fi + + mv "${tmp}" .env.release +} + +cd "${runtime_dir}" + +test -f .env.release +test -f compose.release.yml + +cp .env.release ".env.release.bak.$(date +%Y%m%d%H%M%S)" + +set_env_value "SKILLHUB_VERSION" "${deploy_tag}" + +cat > manual-test-deployment.txt </dev/null +curl -fsS "http://127.0.0.1:${web_port}/nginx-health" >/dev/null From 3e1b5738aa5241034ca3e08df92fc58065751681 Mon Sep 17 00:00:00 2001 From: dongmucat <70678707+dongmucat@users.noreply.github.com> Date: Fri, 10 Apr 2026 10:01:05 +0800 Subject: [PATCH 6/6] fix(storage): honor forcePathStyle for s3 presigner (#251) --- .../skillhub/storage/S3StorageService.java | 14 ++++- .../storage/S3StorageServiceTest.java | 56 +++++++++++++++++++ 2 files changed, 67 insertions(+), 3 deletions(-) create mode 100644 server/skillhub-storage/src/test/java/com/iflytek/skillhub/storage/S3StorageServiceTest.java diff --git a/server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/S3StorageService.java b/server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/S3StorageService.java index 50062e85..a0efbbd0 100644 --- a/server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/S3StorageService.java +++ b/server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/S3StorageService.java @@ -11,6 +11,7 @@ import software.amazon.awssdk.core.sync.RequestBody; import software.amazon.awssdk.http.apache.ApacheHttpClient; import software.amazon.awssdk.regions.Region; import software.amazon.awssdk.services.s3.S3Client; +import software.amazon.awssdk.services.s3.S3Configuration; import software.amazon.awssdk.services.s3.model.*; import software.amazon.awssdk.services.s3.presigner.S3Presigner; import software.amazon.awssdk.services.s3.presigner.model.GetObjectPresignRequest; @@ -54,17 +55,24 @@ public class S3StorageService implements ObjectStorageService { builder.endpointOverride(URI.create(properties.getEndpoint())); } this.s3Client = builder.build(); + this.s3Presigner = buildPresigner(); + ensureBucketExists(); + } + + S3Presigner buildPresigner() { var presignerBuilder = S3Presigner.builder() .region(Region.of(properties.getRegion())) .credentialsProvider(StaticCredentialsProvider.create( - AwsBasicCredentials.create(properties.getAccessKey(), properties.getSecretKey()))); + AwsBasicCredentials.create(properties.getAccessKey(), properties.getSecretKey()))) + .serviceConfiguration(S3Configuration.builder() + .pathStyleAccessEnabled(properties.isForcePathStyle()) + .build()); if (properties.getPublicEndpoint() != null && !properties.getPublicEndpoint().isBlank()) { presignerBuilder.endpointOverride(URI.create(properties.getPublicEndpoint())); } else if (properties.getEndpoint() != null && !properties.getEndpoint().isBlank()) { presignerBuilder.endpointOverride(URI.create(properties.getEndpoint())); } - this.s3Presigner = presignerBuilder.build(); - ensureBucketExists(); + return presignerBuilder.build(); } private void ensureBucketExists() { diff --git a/server/skillhub-storage/src/test/java/com/iflytek/skillhub/storage/S3StorageServiceTest.java b/server/skillhub-storage/src/test/java/com/iflytek/skillhub/storage/S3StorageServiceTest.java new file mode 100644 index 00000000..8ca8cae7 --- /dev/null +++ b/server/skillhub-storage/src/test/java/com/iflytek/skillhub/storage/S3StorageServiceTest.java @@ -0,0 +1,56 @@ +package com.iflytek.skillhub.storage; + +import org.junit.jupiter.api.Test; +import software.amazon.awssdk.services.s3.model.GetObjectRequest; +import software.amazon.awssdk.services.s3.presigner.model.GetObjectPresignRequest; + +import java.net.URI; +import java.time.Duration; + +import static org.assertj.core.api.Assertions.assertThat; + +class S3StorageServiceTest { + + @Test + void shouldUsePathStylePresignedUrlWhenForcePathStyleEnabled() { + URI presignedUrl = presignGetObjectUrl(true); + + assertThat(presignedUrl.getHost()).isEqualTo("s3.us-east-1.amazonaws.com"); + assertThat(presignedUrl.getPath()).isEqualTo("/test-bucket/artifacts/package.tgz"); + } + + @Test + void shouldUseHostStylePresignedUrlWhenForcePathStyleDisabled() { + URI presignedUrl = presignGetObjectUrl(false); + + assertThat(presignedUrl.getHost()).isEqualTo("test-bucket.s3.us-east-1.amazonaws.com"); + assertThat(presignedUrl.getPath()).isEqualTo("/artifacts/package.tgz"); + } + + private URI presignGetObjectUrl(boolean forcePathStyle) { + S3StorageService storageService = new S3StorageService(createProperties(forcePathStyle)); + try (var presigner = storageService.buildPresigner()) { + var request = presigner.presignGetObject( + GetObjectPresignRequest.builder() + .signatureDuration(Duration.ofMinutes(10)) + .getObjectRequest(GetObjectRequest.builder() + .bucket("test-bucket") + .key("artifacts/package.tgz") + .build()) + .build() + ); + return URI.create(request.url().toString()); + } + } + + private S3StorageProperties createProperties(boolean forcePathStyle) { + S3StorageProperties properties = new S3StorageProperties(); + properties.setRegion("us-east-1"); + properties.setBucket("test-bucket"); + properties.setAccessKey("test-access-key"); + properties.setSecretKey("test-secret-key"); + properties.setEndpoint("https://s3.us-east-1.amazonaws.com"); + properties.setForcePathStyle(forcePathStyle); + return properties; + } +}