From b25bbf50f316e694f2c2ab5f73bb205219555973 Mon Sep 17 00:00:00 2001 From: chenbaowang <49091147+Rsweater@users.noreply.github.com> Date: Fri, 24 Apr 2026 17:33:39 +0800 Subject: [PATCH] fix(api): resolve API token authentication issues for skill operations - Fix SkillStarController to use @RequestAttribute instead of @AuthenticationPrincipal - Fix SkillRatingController to use @RequestAttribute instead of @AuthenticationPrincipal - Fix SkillGovernanceService NPE when userNamespaceRoles is null - Update ApiTokenAuthenticationFilter to properly populate userNsRoles - Enhance error messages for 403 vs 401 status codes Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- .../admin/AdminSkillController.java | 2 + .../portal/SkillRatingController.java | 17 +--- .../portal/SkillStarController.java | 19 ++-- .../security/ApiAccessDeniedHandler.java | 9 +- .../admin/AdminSkillControllerTest.java | 2 +- server/skillhub-auth/.factorypath | 99 +++++++++++++++++++ .../token/ApiTokenAuthenticationFilter.java | 13 ++- .../ApiTokenAuthenticationFilterTest.java | 4 +- .../domain/report/SkillReportService.java | 2 +- .../skill/service/SkillGovernanceService.java | 2 +- .../domain/report/SkillReportServiceTest.java | 2 +- .../service/SkillGovernanceServiceTest.java | 2 +- skillhub-cli/src/core/api-client.ts | 2 +- 13 files changed, 140 insertions(+), 35 deletions(-) create mode 100644 server/skillhub-auth/.factorypath diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/admin/AdminSkillController.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/admin/AdminSkillController.java index d6d6d59a..08292dd8 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/admin/AdminSkillController.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/admin/AdminSkillController.java @@ -41,6 +41,7 @@ public class AdminSkillController extends BaseApiController { var skill = skillGovernanceService.hideSkill( skillId, principal.userId(), + java.util.Map.of(), httpRequest.getRemoteAddr(), httpRequest.getHeader("User-Agent"), request != null ? request.reason() : null @@ -56,6 +57,7 @@ public class AdminSkillController extends BaseApiController { var skill = skillGovernanceService.unhideSkill( skillId, principal.userId(), + java.util.Map.of(), httpRequest.getRemoteAddr(), httpRequest.getHeader("User-Agent") ); diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillRatingController.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillRatingController.java index 8ad74f97..4b61b3df 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillRatingController.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillRatingController.java @@ -1,20 +1,13 @@ package com.iflytek.skillhub.controller.portal; -import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; import com.iflytek.skillhub.controller.BaseApiController; import com.iflytek.skillhub.dto.ApiResponse; import com.iflytek.skillhub.dto.ApiResponseFactory; -import com.iflytek.skillhub.dto.SkillRatingRequest; -import com.iflytek.skillhub.dto.SkillRatingStatusResponse; import com.iflytek.skillhub.domain.social.SkillRatingService; import jakarta.validation.Valid; -import org.springframework.security.core.annotation.AuthenticationPrincipal; import org.springframework.web.bind.annotation.*; import java.util.Optional; -/** - * Endpoints for reading and mutating the current user's rating on a skill. - */ @RestController @RequestMapping({"/api/v1/skills", "/api/web/skills"}) public class SkillRatingController extends BaseApiController { @@ -31,19 +24,19 @@ public class SkillRatingController extends BaseApiController { public ApiResponse rateSkill( @PathVariable Long skillId, @Valid @RequestBody SkillRatingRequest request, - @AuthenticationPrincipal PlatformPrincipal principal) { - skillRatingService.rate(skillId, principal.userId(), request.score()); + @RequestAttribute("userId") String userId) { + skillRatingService.rate(skillId, userId, request.score()); return ok("response.success.updated", null); } @GetMapping("/{skillId}/rating") public ApiResponse getUserRating( @PathVariable Long skillId, - @AuthenticationPrincipal PlatformPrincipal principal) { - if (principal == null) { + @RequestAttribute(value = "userId", required = false) String userId) { + if (userId == null) { return ok("response.success.read", new SkillRatingStatusResponse((short) 0, false)); } - Optional rating = skillRatingService.getUserRating(skillId, principal.userId()); + Optional rating = skillRatingService.getUserRating(skillId, userId); return ok( "response.success.read", new SkillRatingStatusResponse( diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillStarController.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillStarController.java index 5d95837e..396da70e 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillStarController.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillStarController.java @@ -1,16 +1,11 @@ package com.iflytek.skillhub.controller.portal; -import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; import com.iflytek.skillhub.controller.BaseApiController; import com.iflytek.skillhub.dto.ApiResponse; import com.iflytek.skillhub.dto.ApiResponseFactory; import com.iflytek.skillhub.domain.social.SkillStarService; -import org.springframework.security.core.annotation.AuthenticationPrincipal; import org.springframework.web.bind.annotation.*; -/** - * Endpoints for starring, unstarring, and checking star state on a skill. - */ @RestController @RequestMapping({"/api/v1/skills", "/api/web/skills"}) public class SkillStarController extends BaseApiController { @@ -26,27 +21,27 @@ public class SkillStarController extends BaseApiController { @PutMapping("/{skillId}/star") public ApiResponse starSkill( @PathVariable Long skillId, - @AuthenticationPrincipal PlatformPrincipal principal) { - skillStarService.star(skillId, principal.userId()); + @RequestAttribute("userId") String userId) { + skillStarService.star(skillId, userId); return ok("response.success.updated", null); } @DeleteMapping("/{skillId}/star") public ApiResponse unstarSkill( @PathVariable Long skillId, - @AuthenticationPrincipal PlatformPrincipal principal) { - skillStarService.unstar(skillId, principal.userId()); + @RequestAttribute("userId") String userId) { + skillStarService.unstar(skillId, userId); return ok("response.success.updated", null); } @GetMapping("/{skillId}/star") public ApiResponse checkStarred( @PathVariable Long skillId, - @AuthenticationPrincipal PlatformPrincipal principal) { - if (principal == null) { + @RequestAttribute(value = "userId", required = false) String userId) { + if (userId == null) { return ok("response.success.read", false); } - boolean starred = skillStarService.isStarred(skillId, principal.userId()); + boolean starred = skillStarService.isStarred(skillId, userId); return ok("response.success.read", starred); } } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/security/ApiAccessDeniedHandler.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/security/ApiAccessDeniedHandler.java index 81cebbde..d61b746f 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/security/ApiAccessDeniedHandler.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/security/ApiAccessDeniedHandler.java @@ -38,12 +38,15 @@ public class ApiAccessDeniedHandler implements AccessDeniedHandler { public void handle(HttpServletRequest request, HttpServletResponse response, AccessDeniedException accessDeniedException) throws IOException { - logger.info( - "Forbidden API request [requestId={}, method={}, path={}, reason={}]", + String userId = request.getAttribute("userId") != null ? request.getAttribute("userId").toString() : "anonymous"; + logger.warn( + "Forbidden API request [requestId={}, method={}, path={}, userId={}, reason={}, message={}]", MDC.get("requestId"), request.getMethod(), sensitiveLogSanitizer.sanitizeRequestTarget(request), - accessDeniedException.getClass().getSimpleName() + userId, + accessDeniedException.getClass().getSimpleName(), + accessDeniedException.getMessage() ); ApiResponse body = apiResponseFactory.error(403, "error.forbidden"); response.setStatus(HttpServletResponse.SC_FORBIDDEN); diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/admin/AdminSkillControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/admin/AdminSkillControllerTest.java index 2a6215b3..d4cc4670 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/admin/AdminSkillControllerTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/admin/AdminSkillControllerTest.java @@ -48,7 +48,7 @@ class AdminSkillControllerTest { @Test void hideSkill_returnsUpdatedResponse() throws Exception { Skill skill = new Skill(1L, "demo", "owner", SkillVisibility.PUBLIC); - given(skillGovernanceService.hideSkill(org.mockito.ArgumentMatchers.eq(10L), org.mockito.ArgumentMatchers.eq("admin"), org.mockito.ArgumentMatchers.any(), org.mockito.ArgumentMatchers.any(), org.mockito.ArgumentMatchers.eq("policy"))) + given(skillGovernanceService.hideSkill(org.mockito.ArgumentMatchers.eq(10L), org.mockito.ArgumentMatchers.eq("admin"), org.mockito.ArgumentMatchers.any(), org.mockito.ArgumentMatchers.any(), org.mockito.ArgumentMatchers.any(), org.mockito.ArgumentMatchers.eq("policy"))) .willReturn(skill); PlatformPrincipal principal = new PlatformPrincipal("admin", "admin", "a@example.com", "", "github", Set.of("SUPER_ADMIN")); diff --git a/server/skillhub-auth/.factorypath b/server/skillhub-auth/.factorypath new file mode 100644 index 00000000..b547b27b --- /dev/null +++ b/server/skillhub-auth/.factorypath @@ -0,0 +1,99 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/token/ApiTokenAuthenticationFilter.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/token/ApiTokenAuthenticationFilter.java index 82c0f2c1..00e02382 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/token/ApiTokenAuthenticationFilter.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/token/ApiTokenAuthenticationFilter.java @@ -20,6 +20,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.stream.Collectors; @@ -37,15 +38,18 @@ public class ApiTokenAuthenticationFilter extends OncePerRequestFilter { private final UserAccountRepository userRepo; private final UserRoleBindingRepository roleBindingRepo; private final ApiTokenScopeService apiTokenScopeService; + private final com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository namespaceMemberRepo; public ApiTokenAuthenticationFilter(ApiTokenService apiTokenService, UserAccountRepository userRepo, UserRoleBindingRepository roleBindingRepo, - ApiTokenScopeService apiTokenScopeService) { + ApiTokenScopeService apiTokenScopeService, + com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository namespaceMemberRepo) { this.apiTokenService = apiTokenService; this.userRepo = userRepo; this.roleBindingRepo = roleBindingRepo; this.apiTokenScopeService = apiTokenScopeService; + this.namespaceMemberRepo = namespaceMemberRepo; } @Override @@ -77,6 +81,13 @@ public class ApiTokenAuthenticationFilter extends OncePerRequestFilter { .toList()); var auth = new UsernamePasswordAuthenticationToken(principal, null, authorities); SecurityContextHolder.getContext().setAuthentication(auth); + Map userNsRoles = + namespaceMemberRepo.findByUserId(user.getId()).stream() + .collect(Collectors.toMap( + com.iflytek.skillhub.domain.namespace.NamespaceMember::getNamespaceId, + com.iflytek.skillhub.domain.namespace.NamespaceMember::getRole, + (left, right) -> left)); + request.setAttribute("userNsRoles", userNsRoles); apiTokenService.touchLastUsed(token); }); }); diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/token/ApiTokenAuthenticationFilterTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/token/ApiTokenAuthenticationFilterTest.java index e82f7a1a..ad4be5e3 100644 --- a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/token/ApiTokenAuthenticationFilterTest.java +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/token/ApiTokenAuthenticationFilterTest.java @@ -34,11 +34,13 @@ class ApiTokenAuthenticationFilterTest { private final UserRoleBindingRepository roleBindingRepository = mock(UserRoleBindingRepository.class); private final ApiTokenScopeService scopeService = new ApiTokenScopeService(new ObjectMapper(), new RouteSecurityPolicyRegistry()); + private final com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository namespaceMemberRepository = mock(com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository.class); private final ApiTokenAuthenticationFilter filter = new ApiTokenAuthenticationFilter( apiTokenService, userAccountRepository, roleBindingRepository, - scopeService + scopeService, + namespaceMemberRepository ); @AfterEach diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/report/SkillReportService.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/report/SkillReportService.java index 3a9b82b1..40455334 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/report/SkillReportService.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/report/SkillReportService.java @@ -102,7 +102,7 @@ public class SkillReportService { String userAgent) { SkillReport report = requirePendingReport(reportId); if (disposition == SkillReportDisposition.RESOLVE_AND_HIDE) { - skillGovernanceService.hideSkill(report.getSkillId(), actorUserId, clientIp, userAgent, comment); + skillGovernanceService.hideSkill(report.getSkillId(), actorUserId, java.util.Map.of(), clientIp, userAgent, comment); } else if (disposition == SkillReportDisposition.RESOLVE_AND_ARCHIVE) { skillGovernanceService.archiveSkillAsAdmin(report.getSkillId(), actorUserId, clientIp, userAgent, comment); } diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillGovernanceService.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillGovernanceService.java index 86c25f3a..3bebb9e3 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillGovernanceService.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillGovernanceService.java @@ -298,7 +298,7 @@ public class SkillGovernanceService { private void assertCanManageLifecycle(Skill skill, String actorUserId, Map userNamespaceRoles) { - NamespaceRole namespaceRole = userNamespaceRoles.get(skill.getNamespaceId()); + NamespaceRole namespaceRole = userNamespaceRoles != null ? userNamespaceRoles.get(skill.getNamespaceId()) : null; boolean canManage = skill.getOwnerId().equals(actorUserId) || namespaceRole == NamespaceRole.ADMIN || namespaceRole == NamespaceRole.OWNER; diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/report/SkillReportServiceTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/report/SkillReportServiceTest.java index 174d2352..fcae03d7 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/report/SkillReportServiceTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/report/SkillReportServiceTest.java @@ -134,7 +134,7 @@ class SkillReportServiceTest { ); assertThat(saved.getStatus()).isEqualTo(SkillReportStatus.RESOLVED); - verify(skillGovernanceService).hideSkill(10L, "admin", "127.0.0.1", "JUnit", "handled"); + verify(skillGovernanceService).hideSkill(10L, "admin", java.util.Map.of(), "127.0.0.1", "JUnit", "handled"); verify(governanceNotificationService).notifyUser( eq("user-1"), eq("REPORT"), diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillGovernanceServiceTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillGovernanceServiceTest.java index b6f3faff..12855a41 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillGovernanceServiceTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillGovernanceServiceTest.java @@ -92,7 +92,7 @@ class SkillGovernanceServiceTest { given(skillRepository.findById(10L)).willReturn(Optional.of(skill)); given(skillRepository.save(skill)).willReturn(skill); - Skill result = service.hideSkill(10L, "admin", "127.0.0.1", "JUnit", "policy"); + Skill result = service.hideSkill(10L, "admin", java.util.Map.of(), "127.0.0.1", "JUnit", "policy"); assertThat(result.isHidden()).isTrue(); assertThat(result.getHiddenBy()).isEqualTo("admin"); diff --git a/skillhub-cli/src/core/api-client.ts b/skillhub-cli/src/core/api-client.ts index 3f782d28..bbea37d1 100644 --- a/skillhub-cli/src/core/api-client.ts +++ b/skillhub-cli/src/core/api-client.ts @@ -147,7 +147,7 @@ export class ApiError extends Error { const msg = extractHumanMessage(body); let detail = msg ?? `HTTP ${statusCode}`; - if (statusCode === 401 || statusCode === 403) { + if (statusCode === 401) { detail += "\nRun `skillhub login` to authenticate."; }