From f952a6e274a70ecb80fa0c459b4c54834696d202 Mon Sep 17 00:00:00 2001 From: tlzhu3 Date: Thu, 12 Mar 2026 19:28:49 +0800 Subject: [PATCH 01/13] feature : add review doc --- docs/review/代码审查报告.md | 363 ++++++++++++++++++++++++++++++++ docs/review/后期优化报告.md | 405 ++++++++++++++++++++++++++++++++++++ 2 files changed, 768 insertions(+) create mode 100644 docs/review/代码审查报告.md create mode 100644 docs/review/后期优化报告.md diff --git a/docs/review/代码审查报告.md b/docs/review/代码审查报告.md new file mode 100644 index 00000000..0bb93718 --- /dev/null +++ b/docs/review/代码审查报告.md @@ -0,0 +1,363 @@ +# SkillHub 代码审查报告 + +## 1. 审查结论 + +当前版本不建议直接作为后续多人并行开发与生产化落地的基线。 + +结论等级：`有条件不通过` + +阻塞原因主要集中在以下几类： + +1. 认证与鉴权存在高风险缺陷，部分接口可被越权使用，设备码登录实现还存在可伪造令牌问题。 +2. 发布状态机与设计文档不一致，`latest` 语义、搜索事件、下载链路会被未审核版本污染。 +3. 上传与存储链路缺少路径安全和流式限制，存在路径穿越、内存耗尽、脏对象遗留等风险。 +4. 自动化质量门禁未闭环，`skillhub-app` 集成测试无法启动，前端 lint 也未通过。 + +## 2. 审查范围 + +本次重点审查了以下内容： + +- 设计文档： + - `docs/01-system-architecture.md` + - `docs/03-authentication-design.md` + - `docs/05-business-flows.md` + - `docs/06-api-design.md` + - `docs/07-skill-protocol.md` + - `docs/09-deployment.md` +- 后端核心代码： + - 认证与安全：`skillhub-auth` + - 发布、查询、下载、评审、提升：`skillhub-domain` + `skillhub-app` + - 搜索：`skillhub-search` + - 存储：`skillhub-storage` +- 前端核心代码： + - `web/src/features/skill/markdown-renderer.tsx` + +本次执行的关键校验： + +- `pnpm run typecheck`：通过 +- `pnpm run lint`：失败 +- `pnpm run build`：通过，但主包体积告警 +- `mvn -q -DskipTests compile`：通过 +- `mvn test`：失败，`skillhub-app` 24 个测试里 19 个错误 + +## 3. 关键发现 + +### [P0] 设备码登录当前会返回可预测的伪令牌，并且存在并发重复签发风险 + +问题位置： + +- `server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/device/DeviceAuthService.java:85-111` +- `server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/SecurityConfig.java:64-75` +- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/DeviceAuthController.java:21-29` + +问题说明： + +- `pollToken()` 在设备码授权成功后直接返回 `token_ + deviceCode`，这不是签名令牌，也不是随机不透明令牌，而是由外部输入可推导得到的占位值。 +- `/api/v1/cli/auth/device/**` 被 `permitAll()` 放开，意味着设备流轮询端点是匿名可访问的，这本身没问题，但前提是返回的必须是正式、不可伪造、可审计、可吊销的令牌。 +- `pollToken()` 采用“先读状态，再写 USED”的非原子流程。两个并发轮询请求可以同时读到 `AUTHORIZED`，从而重复获取访问令牌。 + +影响： + +- 这是认证链路上的核心安全缺陷，风险等级为阻塞上线。 +- 设备授权一旦成功，令牌可被预测，且一次授权可能被多次兑换。 + +修改建议： + +1. 设备流成功后必须调用正式的 token 签发服务，返回随机 opaque token 或签名 JWT。 +2. 设备码消费必须改成 Redis Lua / CAS 方式，保证“仅成功兑换一次”。 +3. 为设备码申请、授权、轮询增加限流和审计。 +4. 补充成功兑换、重复兑换、并发轮询、过期轮询的集成测试。 + +### [P1] 评审与提升流程缺失关键权限校验，已形成越权入口 + +问题位置： + +- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/ReviewController.java:56-64` +- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/ReviewService.java:50-68` +- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/PromotionController.java:54-61` +- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/PromotionService.java:47-78` +- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/ReviewController.java:102-126` +- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/PromotionController.java:86-105` + +问题说明： + +- `submitReview()` 只根据 `skillVersionId` 和当前用户提交，不校验提交人是否为该 skill 的 owner、namespace 管理员或被允许的发布者。 +- `submitPromotion()` 只校验源版本已发布、目标 namespace 是 GLOBAL，不校验申请人是否有权提升该 skill。 +- `listPendingReviews()`、`getReviewDetail()`、`getPromotionDetail()` 缺少明确授权判断，当前只要已登录即可读取相关数据，存在流程信息泄露。 + +与设计文档冲突： + +- `docs/05-business-flows.md:104-119` 明确要求“owner 或 namespace admin”才能发起提升。 +- `docs/06-api-design.md` 中 review / promotion 相关接口的权限边界也比当前实现更严格。 + +影响： + +- 任意登录用户理论上可以替别人的草稿发起评审。 +- 任意登录用户理论上可以为别人的 skill 发起提升申请。 +- 待审核数据与审批详情对无关用户暴露。 + +修改建议： + +1. 在 `ReviewService.submitReview()` 前补 owner / namespace ADMIN/OWNER 校验。 +2. 在 `PromotionService.submitPromotion()` 前补 source skill 所属 namespace 的提交权限校验。 +3. `listPendingReviews()`、`getReviewDetail()`、`getPromotionDetail()` 必须按 namespace 或平台角色做访问控制，未授权时返回 `403`，不能返回空列表掩盖问题。 + +### [P1] 发布流程在审核前就覆盖 `latestVersionId` 并发出 `SkillPublishedEvent`，会污染下载、解析、搜索和标签语义 + +问题位置： + +- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java:143-156` +- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java:217-225` +- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillDownloadService.java:68-75` +- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillQueryService.java:106-124` +- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillQueryService.java:344-351` +- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillTagService.java:44-47` +- `server/skillhub-search/src/main/java/com/iflytek/skillhub/search/event/SearchIndexEventListener.java:26-39` + +问题说明： + +- `publishFromEntries()` 创建的新版本状态是 `PENDING_REVIEW`，但随后立即： + - 更新 `skill.latestVersionId` + - 发出 `SkillPublishedEvent` +- `downloadLatest()`、`resolveLatestVersion()`、`latest` 保留标签、搜索结果里的 latest version 都依赖 `latestVersionId`。 +- 当一个已发布 skill 再上传新版本时，`latestVersionId` 会被指向一个尚未发布的版本，导致“最新版本”不可下载、不可解析或显示错误。 + +与设计文档冲突： + +- `docs/05-business-flows.md:30-45` 的 Phase 2 定义是“发布后直接 `PUBLISHED` 才更新 latest”。 +- `docs/05-business-flows.md:49-55` 的 Phase 3 定义是“`DRAFT -> PENDING_REVIEW -> PUBLISHED`”，审核通过后才进入发布态。 +- 当前代码实际做成了“创建后直接 `PENDING_REVIEW`，但又按已发布版本处理”，状态语义前后矛盾。 + +影响： + +- `latest` 下载链路会在有待审版本时报错。 +- 搜索索引会被过早重建。 +- 详情页、我的技能页、标签查询都会看到未发布版本。 + +修改建议： + +1. 冻结状态机语义： + - 要么 Phase 2：发布即 `PUBLISHED` + - 要么 Phase 3：上传只创建草稿 / 待审，审核通过后才更新 latest +2. `latestVersionId` 必须只表示“最新已发布版本”。 +3. `SkillPublishedEvent` 只能在版本真正进入 `PUBLISHED` 后发出。 +4. 若需要展示“最新草稿”，单独引入 `latestDraftVersionId` 或查询逻辑，不要复用 published 语义字段。 + +### [P1] 上传链路存在路径穿越与压缩包资源耗尽风险 + +问题位置： + +- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillPublishController.java:64-83` +- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/SkillPackageValidator.java:28-81` +- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java:166-189` +- `server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/LocalFileStorageService.java:21-31` +- `server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/LocalFileStorageService.java:61` + +问题说明： + +- Controller 侧对 zip entry 使用 `readAllBytes()`，在验证之前就把每个解压后文件整体读入内存。 +- 包校验器没有校验路径规范化，也没有禁止 `../`、绝对路径、反斜杠路径、重复路径。 +- `LocalFileStorageService.resolve()` 直接 `basePath.resolve(key)`，没有 `normalize()` 和 `startsWith(basePath)` 校验。 +- 攻击者可以构造恶意 zip 路径，例如 `../../outside.txt`，通过 `skills/{skillId}/{versionId}/{filePath}` 最终逃逸本地存储根目录。 + +影响： + +- 本地存储模式下存在任意文件写入风险。 +- 恶意压缩包可造成内存压力甚至进程 OOM。 + +修改建议： + +1. 在解压阶段引入流式校验，限制总解压大小、单文件大小、文件数量。 +2. 对 entry path 做统一规范化，只允许： + - 相对路径 + - `/` 分隔 + - 禁止 `..`、禁止绝对路径、禁止空段 +3. `LocalFileStorageService` 必须在 `normalize()` 后校验目标路径仍位于 `basePath` 内。 +4. 对重复路径在校验阶段直接返回 `400`，不能等数据库唯一键报错。 + +### [P1] `GET /api/v1/skills/**` 过度放开，导致匿名 `500` 与私有元数据泄露 + +问题位置： + +- `server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/SecurityConfig.java:75` +- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillStarController.java:40-45` +- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillRatingController.java:37-46` +- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillController.java:73-94` +- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillTagController.java:29-40` + +问题说明： + +- `SecurityConfig` 直接放开了所有 `GET /api/v1/skills/**`。 +- 但其中至少两类 GET 并不适合匿名访问： + - `GET /api/v1/skills/{skillId}/star` + - `GET /api/v1/skills/{skillId}/rating` + 这两个接口直接解引用 `principal.userId()`，匿名访问会触发空指针，落到全局异常后返回 `500`。 +- 另外： + - `listVersions()` 没有 visibility 校验 + - `listTags()` 没有 visibility 校验 + 这会把 `PRIVATE` / `NAMESPACE_ONLY` skill 的版本信息、标签信息暴露给匿名或无关用户。 + +影响： + +- 安全上属于“鉴权边界与路由策略不一致”。 +- 行为上会出现匿名访问 500，破坏 API 契约。 + +修改建议： + +1. 不要用 `GET /api/v1/skills/**` 一刀切放开，改成白名单到具体公共接口。 +2. 对“需要当前用户上下文”的 GET 接口显式要求认证。 +3. `listVersions()`、`listTags()` 必须补充与详情接口一致的 visibility 校验。 + +### [P1] `skillhub-app` 集成测试当前无法启动，测试信号失真 + +问题位置： + +- `server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/device/DeviceAuthService.java:19-25` +- `server/skillhub-app/src/test/resources/application-test.yml:1-33` +- `server/skillhub-app/target/surefire-reports/com.iflytek.skillhub.controller.AuthControllerTest.txt` + +问题说明： + +- `mvn test` 中，`skillhub-storage`、`skillhub-domain`、`skillhub-auth` 的测试通过，但 `skillhub-app` 的 Spring 上下文无法启动。 +- 直接原因是 `DeviceAuthService` 依赖 `RedisTemplate`，测试环境没有对应 bean。 +- 代码里也没有看到 `verificationUri` 的配置注入实现，当前构造器还要求额外的 `String` 参数，后续即使补齐 Redis bean，也大概率还会继续失败。 + +影响： + +- 目前 controller 层测试的 19 个错误都被同一个装配问题掩盖，真实回归无法被发现。 +- CI 无法提供有效的回归保护。 + +修改建议： + +1. 为设备码服务补全正式配置类： + - `RedisTemplate` + - `@ConfigurationProperties` 形式的 `verificationUri` +2. 测试环境提供 stub / mock bean，确保 app context 可启动。 +3. 优先修复后重新运行 controller 集成测试，再评估剩余问题。 + +### [P2] 存储配置键名与部署文档漂移，S3/MinIO 路径当前并不能真正启用 + +问题位置： + +- `server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/StorageProperties.java:7-15` +- `server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/LocalFileStorageService.java:12-14` +- `server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/S3StorageService.java:19-21` +- `server/skillhub-app/src/main/resources/application.yml:55-58` +- `docker-compose.prod.yml:51-59` +- `docs/09-deployment.md` + +问题说明： + +- 代码使用的配置键是 `skillhub.storage.provider`。 +- `application.yml` 写的是 `skillhub.storage.type`。 +- `docker-compose.prod.yml` 也没有把 S3/MinIO 相关配置注入到后端容器。 + +影响： + +- 即使文档和运维层面按 MinIO/S3 部署，也无法通过当前配置切换到 `S3StorageService`。 +- 实际运行会默认为本地文件存储，和部署文档不一致。 + +修改建议： + +1. 统一配置键，只保留一种： + - 推荐 `skillhub.storage.provider` +2. 修正文档、`application.yml`、`docker-compose.prod.yml`、`StorageProperties` 的一致性。 +3. 增加一个启动时自检日志，打印当前启用的 storage provider。 + +### [P2] 上传大小与文件白名单配置未真正落地，当前仍是硬编码 + +问题位置： + +- `server/skillhub-app/src/main/resources/application.yml:47-64` +- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/SkillPackageValidator.java:12-20` + +问题说明： + +- 配置文件声明： + - `multipart` 允许到 `100MB` + - `skillhub.publish.max-package-size` 为 `100MB` + - `allowed-file-extensions` 可配置 +- 但实际校验器仍然硬编码： + - 总包 `10MB` + - 固定扩展名集合 + +影响： + +- 文档、配置、运行时行为三者不一致。 +- 运维或产品侧修改配置后不会生效，问题定位成本高。 + +修改建议： + +1. 将 `SkillPackageValidator` 改为读取 `@ConfigurationProperties`。 +2. 保持“网关上限”“multipart 上限”“业务校验上限”三层配置含义清晰并一致。 + +### [P2] 资源不存在时多处直接 `orElseThrow()`，会把正常 404 场景放大成 500 + +问题位置： + +- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/PromotionController.java:101-111` +- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/ReviewController.java:60-62` +- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/ReviewController.java:97` +- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/ReviewController.java:123-132` +- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillRatingController.java:30-33` + +问题说明： + +- 多个 controller 直接使用裸 `orElseThrow()`。 +- 当资源不存在时会抛出 `NoSuchElementException`，最后被全局异常处理器转成 `500`。 +- `SkillRatingController` 还存在 `score` 缺失时的空指针风险。 + +影响： + +- API 契约不稳定，客户端会把普通业务错误当成服务故障。 + +修改建议： + +1. 统一抛出领域层 `NotFound/BadRequest` 异常。 +2. 给评分、评审、提升请求增加 `@Valid` 和显式字段校验。 + +### [P2] 前端质量门禁未闭环，lint 未通过，产物主包偏大 + +问题位置： + +- `web/src/features/skill/markdown-renderer.tsx:15-16` + +问题说明： + +- 当前 `eslint` 失败： + - `@ts-ignore` 应改为 `@ts-expect-error` + - `node` 参数未使用 +- `pnpm run build` 虽能通过，但主 JS chunk 约 `751.25 kB`，明显偏大。 + +影响： + +- CI 无法建立严格前端门禁。 +- 首屏加载与缓存更新成本偏高。 + +修改建议： + +1. 先修复 lint 报错，恢复前端静态检查门禁。 +2. 后续按页面级或功能级拆包，优先处理 markdown/highlight、管理页、上传页等非首屏模块。 + +## 4. 与设计文档的主要偏差 + +当前代码与文档的主要偏差有： + +1. `docs/05-business-flows.md` 对 Phase 2 / Phase 3 的发布状态定义，与 `SkillPublishService` 现状不一致。 +2. 文档里定义了幂等、审计、孤儿对象清理、异步事件兜底，但代码里尚未真正实现。 +3. 文档要求的权限边界比当前 review / promotion / skill GET 路由更严格，当前实现明显偏松。 +4. 部署文档强调 MinIO / S3 可切换，但配置层并未打通。 + +## 5. 现阶段是否可进入开发 + +可以继续开发，但不建议直接进入“多人并行开发 + 联调 + 准生产验证”阶段。 + +建议先完成以下最小修复集： + +1. 修复设备码认证实现与 app 测试装配。 +2. 收紧 review / promotion / skills GET 的鉴权边界。 +3. 修正发布状态机，保证 `latestVersionId` 只指向已发布版本。 +4. 修复 zip 路径校验和本地存储路径归一化。 +5. 打通前后端质量门禁：`mvn test`、`pnpm lint`、`pnpm build` 全绿。 + +在这五项完成之前，后续功能开发会持续叠加在一个不稳定基线上，返工概率较高。 diff --git a/docs/review/后期优化报告.md b/docs/review/后期优化报告.md new file mode 100644 index 00000000..b8a07694 --- /dev/null +++ b/docs/review/后期优化报告.md @@ -0,0 +1,405 @@ +# SkillHub 后期优化报告 + +## 1. 总体判断 + +SkillHub 当前已经具备“能跑起来的模块化单体”雏形，分层方向是对的，但距离“可持续迭代、可对外开放、可灰度发布”的工程化状态还有明显差距。 + +后续优化应分成两类： + +1. `阻塞型修复`：先把安全、鉴权、状态机、测试基线修好。 +2. `架构型收敛`：把文档、配置、协议、模块边界收敛成一个长期稳定的实现。 + +下面的建议默认以“后续要继续实际代码开发”为前提，而不是只做文档美化。 + +## 2. 优先级建议 + +### T0：1 周内必须完成 + +1. 修复设备码登录： + - 正式 token 签发 + - 原子消费 + - Redis bean / 配置注入补齐 +2. 修复 review / promotion / skills GET 的权限边界。 +3. 修复发布状态机： + - `latestVersionId` + - `SkillPublishedEvent` + - `latest` 标签 + - 搜索重建触发时机 +4. 修复上传路径安全和解压限制。 +5. 恢复质量门禁： + - `mvn test` + - `pnpm lint` + - `pnpm build` + +### T1：2 到 3 周内完成 + +1. 冻结配置协议与部署方式： + - storage provider + - S3 / MinIO + - publish 限制配置 +2. 完成评审、提升、下载、搜索的权限策略统一。 +3. 落地幂等与审计最小版。 +4. 收敛 API 路径与 DTO 契约。 + +### T2：1 到 2 个迭代完成 + +1. 引入更稳健的异步一致性方案。 +2. 做前端拆包、性能与体验优化。 +3. 增强发布安全能力和安装完整性校验。 +4. 做更强的产品化能力，例如可视化审核、统计分析、推荐等。 + +## 3. 架构优化建议 + +### 3.1 冻结发布状态机 + +当前最需要收敛的是“发布”和“审核”到底是什么关系。 + +建议明确采用下面其中一种，不要混合： + +方案 A：Phase 2 简化模型 + +- 上传成功即 `PUBLISHED` +- 不存在 `PENDING_REVIEW` +- `latestVersionId` 直接指向新版本 + +方案 B：Phase 3 审核模型 + +- 上传只创建 `DRAFT` +- 显式提交后进入 `PENDING_REVIEW` +- 审核通过后才进入 `PUBLISHED` +- 只有 `PUBLISHED` 版本能影响： + - `latestVersionId` + - `latest` 保留标签 + - 搜索索引 + - 下载入口 + +如果选择方案 B，建议新增下面两个边界对象： + +- `PublishedVersionPointer` + - 只负责“当前可安装版本” +- `DraftVersionView` + - 只负责“作者或审核人能看到的最新草稿/待审版本” + +不要再用一个 `latestVersionId` 同时表达“最新上传版本”和“最新已发布版本”。 + +### 3.2 建立统一授权层 + +当前权限判断分散在： + +- `SecurityConfig` +- Controller +- Domain Service +- `VisibilityChecker` +- `ReviewPermissionChecker` + +建议统一成三层： + +1. `Authentication` + - 只负责身份识别 +2. `Resource Access Policy` + - 只负责“谁能看” +3. `Action Authorization Policy` + - 只负责“谁能做什么” + +推荐拆出统一的 policy 组件，例如： + +- `SkillReadPolicy` +- `SkillPublishPolicy` +- `ReviewPolicy` +- `PromotionPolicy` +- `NamespacePolicy` + +Controller 不再自己拼权限，只把上下文交给 policy。 + +### 3.3 重构上传入口为单独的 Package Ingestion 模块 + +建议把当前“解压 + 校验 + 存储 + bundle 构建 + 元数据提取”从 `SkillPublishController` / `SkillPublishService` 中抽离，形成独立应用服务： + +- `SkillPackageIngestionService` + +职责建议如下： + +- 解析 zip stream +- 路径校验 +- 大小限制 +- 重复路径校验 +- 内容类型判断 +- 哈希计算 +- 生成标准化 manifest +- 输出领域对象 `ValidatedSkillPackage` + +这样好处是： + +- Web / CLI 共享同一套上传逻辑 +- 更容易做流式处理 +- 更容易接入病毒扫描、签名验证、内容审核 + +### 3.4 引入存储补偿与异步一致性机制 + +当前对象存储与数据库事务是“两套事务”，必须承认这一点。 + +建议至少补齐两层机制： + +1. 同步补偿 + - 上传过程中一旦数据库失败，立即尝试删除已写入对象 +2. 异步兜底 + - 定时扫描 orphan object + - 定时补建搜索索引 + +如果未来继续扩展，建议走标准化方案： + +- `transactional outbox` +- 后台 worker +- 幂等消费 + +### 3.5 配置体系收敛成强类型配置 + +当前配置散落在： + +- `application.yml` +- `docker-compose*.yml` +- `@ConditionalOnProperty` +- 硬编码常量 + +建议做一次统一收敛： + +- `SkillhubStorageProperties` +- `SkillhubPublishProperties` +- `SkillhubAuthProperties` +- `SkillhubRateLimitProperties` + +并要求： + +1. 所有运行时行为只从配置类读取。 +2. 业务代码禁止继续写死阈值。 +3. 启动时打印关键配置摘要，方便排障。 + +## 4. 设计优化建议 + +### 4.1 API 路径风格统一 + +当前同时存在两种风格： + +- 面向用户的 namespace/slug 坐标 +- 面向内部的数字 ID 路径 + +建议明确区分： + +- 外部公开 API：统一用 `namespace/slug/version/tag` +- 内部管理或后台 API：可以保留数字 ID + +对外协议越稳定，前端、CLI、第三方集成越容易维护。 + +### 4.2 文档与代码冻结同一份状态机和权限矩阵 + +建议补一份真正可执行的“冻结文档”，只包含这些内容： + +1. 版本状态流转表 +2. skill 可见性矩阵 +3. review / promotion 权限矩阵 +4. token scope 矩阵 +5. 公共 GET 白名单 + +这份文档应该成为： + +- 后端开发实现基准 +- 前端联调基准 +- 测试用例来源 + +### 4.3 Token 设计升级为“角色 + scope”双约束 + +当前 API Token 更像“换一种方式拿到完整用户权限”。 + +建议升级成： + +- `identity` +- `platformRoles` +- `scopes` +- `resourceConstraints` + +例如： + +- `skill:read` +- `skill:publish` +- `review:read` +- `review:approve` +- `namespace:team-a` + +并要求 Filter 只解析身份，具体权限由下游 policy 再次判断。 + +### 4.4 设备码登录设计应独立成完整子协议 + +建议把设备流单独文档化并独立实现，最少包括： + +1. device code 生命周期 +2. user code 生命周期 +3. 轮询频率限制 +4. 授权成功后的单次兑换 +5. access token / refresh token +6. 撤销与过期处理 +7. 审计字段 + +不要把它作为“先写个占位版本”长期保留在主干里。 + +## 5. 功能优化建议 + +### 5.1 发布体验 + +建议在当前基础上补齐： + +1. 上传预检接口 + - 只做包校验，不落库 +2. 发布结果详情页 + - 显示版本状态、校验结果、审计信息 +3. 版本差异展示 + - 新旧文件列表 diff +4. bundle 指纹展示 + - 便于 CLI 做一致性校验 + +### 5.2 审核体验 + +建议增加： + +1. review 队列按 namespace / 状态 / 时间过滤 +2. promotion 队列显示来源 skill 与目标 namespace +3. 审核意见模板 +4. 审核历史轨迹 + +### 5.3 安装与下载体验 + +建议补齐： + +1. 下载返回指纹 +2. CLI 安装后校验 hash +3. “latest” 与自定义 tag 的解析提示 +4. 私有 skill 访问失败时更清晰的错误码 + +### 5.4 搜索体验 + +建议按阶段演进： + +1. 先把权限过滤、排序、分页做稳定 +2. 再补关键词高亮、标签过滤、namespace 过滤 +3. 最后再考虑向量搜索或推荐 + +先把 correctness 做扎实，比过早上复杂搜索引擎更重要。 + +## 6. 创新优化建议 + +### 6.1 增加发布安全链 + +可以在上传后增加可插拔安全扫描链： + +- 文件白名单校验 +- 敏感内容扫描 +- 恶意脚本特征扫描 +- 许可证扫描 +- 依赖清单提取 + +这部分可以通过 `PrePublishValidator` 真正扩展，而不是继续 `NoOp`。 + +### 6.2 增加制品签名与可验证安装 + +建议为 bundle 增加： + +- 版本指纹 +- 服务端签名 +- CLI 验签 + +这样 SkillHub 才更像一个可被信任的官方分发源，而不只是文件托管站。 + +### 6.3 增加“来源关系”与“派生关系”图谱 + +当前 promotion 已经有 `sourceSkillId` 雏形，后面可以扩展成： + +- fork / promote / mirror / upstream + +这能支撑： + +- 技能来源追踪 +- 派生链可视化 +- 全局版与团队版差异说明 + +## 7. 迭代优化建议 + +### 7.1 推荐实施顺序 + +第一阶段：安全与正确性收口 + +- 设备码登录 +- review / promotion 鉴权 +- skills GET 白名单 +- 上传路径安全 +- latest 语义修复 + +第二阶段：工程化基线收口 + +- 测试全绿 +- 配置统一 +- 存储 provider 打通 +- 文档与代码对齐 +- 前端 lint 与拆包 + +第三阶段：产品化能力增强 + +- 审核后台 +- 审计日志 +- 幂等与回放 +- 更完整的 CLI 体验 + +第四阶段：平台化能力增强 + +- 签名安装 +- 安全扫描 +- 推荐与搜索增强 +- 可观测性与运营报表 + +### 7.2 每阶段验收标准 + +建议引入明确验收门槛： + +第一阶段验收： + +- 不再存在 P0 / P1 安全问题 +- `mvn test` 通过 +- `pnpm lint && pnpm build` 通过 + +第二阶段验收： + +- 配置切换本地/S3 存储可实测 +- 发布/审核/下载链路与文档一致 +- 私有 skill 权限边界有自动化测试 + +第三阶段验收： + +- review / promotion / audit 有可视化页面 +- CLI 对外可稳定联调 + +## 8. 建议立即补的文档 + +为了让后续开发真正顺利，建议马上追加 4 份冻结文档： + +1. `权限矩阵冻结文档` +2. `发布状态机冻结文档` +3. `上传与存储安全规范` +4. `API Token / Device Flow 协议冻结文档` + +这 4 份文档会比继续补泛化架构图更有实际开发价值。 + +## 9. 最终建议 + +SkillHub 目前最值得保留的是： + +- 模块化单体拆分方向 +- 领域服务初步分层 +- review / promotion / search / storage 的边界意识 + +最需要马上收口的是： + +- 权限模型 +- 发布状态机 +- 上传安全 +- 配置一致性 +- 测试基线 + +只要先把这五件事修稳，后面的架构优化和产品创新都能顺着推进；如果不先修，越往后叠功能，返工就越贵。 From 444a52f802c446051eca2d8d1c1041c885674879 Mon Sep 17 00:00:00 2001 From: tlzhu3 Date: Thu, 12 Mar 2026 21:42:15 +0800 Subject: [PATCH 02/13] fix(auth): complete device flow token exchange --- .../controller/DeviceAuthControllerTest.java | 16 ++++ .../auth/device/DeviceAuthService.java | 76 ++++++++++++++++--- .../auth/device/DeviceAuthServiceTest.java | 55 +++++++++++++- 3 files changed, 134 insertions(+), 13 deletions(-) diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/DeviceAuthControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/DeviceAuthControllerTest.java index 75631a60..f1ede80c 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/DeviceAuthControllerTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/DeviceAuthControllerTest.java @@ -70,4 +70,20 @@ class DeviceAuthControllerTest { .andExpect(jsonPath("$.data.accessToken").isEmpty()) .andExpect(jsonPath("$.data.tokenType").isEmpty()); } + + @Test + void pollToken_returns_access_token_when_authorized() throws Exception { + DeviceTokenResponse response = DeviceTokenResponse.success("sk_device_flow_token"); + + given(deviceAuthService.pollToken("device_abc123")).willReturn(response); + + mockMvc.perform(post("/api/v1/cli/auth/device/token") + .contentType(MediaType.APPLICATION_JSON) + .content("{\"deviceCode\": \"device_abc123\"}")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.code").value(0)) + .andExpect(jsonPath("$.data.accessToken").value("sk_device_flow_token")) + .andExpect(jsonPath("$.data.tokenType").value("Bearer")) + .andExpect(jsonPath("$.data.error").isEmpty()); + } } diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/device/DeviceAuthService.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/device/DeviceAuthService.java index 56ffa0ef..7a60e0df 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/device/DeviceAuthService.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/device/DeviceAuthService.java @@ -1,9 +1,11 @@ package com.iflytek.skillhub.auth.device; +import com.iflytek.skillhub.auth.token.ApiTokenService; import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException; import org.springframework.beans.factory.annotation.Value; import org.springframework.data.redis.core.RedisTemplate; import org.springframework.stereotype.Service; +import org.springframework.util.StringUtils; import java.security.SecureRandom; import java.util.Base64; @@ -13,18 +15,26 @@ import java.util.concurrent.TimeUnit; public class DeviceAuthService { private static final String DEVICE_CODE_PREFIX = "device:code:"; + private static final String DEVICE_CLAIM_PREFIX = "device:claim:"; private static final String USER_CODE_PREFIX = "device:usercode:"; private static final int EXPIRES_IN_SECONDS = 900; private static final int POLL_INTERVAL_SECONDS = 5; private static final String USER_CODE_CHARS = "ABCDEFGHJKLMNPQRSTUVWXYZ23456789"; + private static final long PENDING_CODE_TTL_MINUTES = EXPIRES_IN_SECONDS / 60L; + private static final long USED_CODE_TTL_MINUTES = 1L; + private static final String CLI_DEVICE_TOKEN_NAME = "CLI Device Flow"; + private static final String CLI_DEVICE_SCOPE_JSON = "[\"skill:read\",\"skill:publish\"]"; private final RedisTemplate redisTemplate; + private final ApiTokenService apiTokenService; private final String verificationUri; private final SecureRandom random = new SecureRandom(); public DeviceAuthService(RedisTemplate redisTemplate, + ApiTokenService apiTokenService, @Value("${skillhub.device-auth.verification-uri:/device}") String verificationUri) { this.redisTemplate = redisTemplate; + this.apiTokenService = apiTokenService; this.verificationUri = verificationUri; } @@ -35,9 +45,9 @@ public class DeviceAuthService { DeviceCodeData data = new DeviceCodeData(deviceCode, userCode, DeviceCodeStatus.PENDING, null); redisTemplate.opsForValue().set( - DEVICE_CODE_PREFIX + deviceCode, data, EXPIRES_IN_SECONDS / 60, TimeUnit.MINUTES); + DEVICE_CODE_PREFIX + deviceCode, data, PENDING_CODE_TTL_MINUTES, TimeUnit.MINUTES); redisTemplate.opsForValue().set( - USER_CODE_PREFIX + userCode, deviceCode, EXPIRES_IN_SECONDS / 60, TimeUnit.MINUTES); + USER_CODE_PREFIX + userCode, deviceCode, PENDING_CODE_TTL_MINUTES, TimeUnit.MINUTES); return new DeviceCodeResponse(deviceCode, userCode, verificationUri, EXPIRES_IN_SECONDS, POLL_INTERVAL_SECONDS); } @@ -53,10 +63,20 @@ public class DeviceAuthService { throw new DomainBadRequestException("error.deviceAuth.deviceCode.expired"); } - data.setStatus(DeviceCodeStatus.AUTHORIZED); - data.setUserId(userId); - redisTemplate.opsForValue().set( - DEVICE_CODE_PREFIX + deviceCode, data, EXPIRES_IN_SECONDS / 60, TimeUnit.MINUTES); + switch (data.getStatus()) { + case PENDING -> { + data.setStatus(DeviceCodeStatus.AUTHORIZED); + data.setUserId(userId); + redisTemplate.opsForValue().set( + DEVICE_CODE_PREFIX + deviceCode, data, PENDING_CODE_TTL_MINUTES, TimeUnit.MINUTES); + } + case AUTHORIZED -> { + if (!userId.equals(data.getUserId())) { + throw new DomainBadRequestException("error.deviceAuth.deviceCode.alreadyAuthorized"); + } + } + case USED -> throw new DomainBadRequestException("error.deviceAuth.deviceCode.used"); + } } public DeviceTokenResponse pollToken(String deviceCode) { @@ -68,16 +88,48 @@ public class DeviceAuthService { return switch (data.getStatus()) { case PENDING -> DeviceTokenResponse.pending(); - case AUTHORIZED -> { - data.setStatus(DeviceCodeStatus.USED); - redisTemplate.opsForValue().set( - DEVICE_CODE_PREFIX + deviceCode, data, 1, TimeUnit.MINUTES); - yield DeviceTokenResponse.success(null); - } + case AUTHORIZED -> redeemAuthorizedDeviceCode(deviceCode, data); case USED -> throw new DomainBadRequestException("error.deviceAuth.deviceCode.used"); }; } + private DeviceTokenResponse redeemAuthorizedDeviceCode(String deviceCode, DeviceCodeData data) { + boolean claimed = Boolean.TRUE.equals(redisTemplate.opsForValue().setIfAbsent( + DEVICE_CLAIM_PREFIX + deviceCode, + "claimed", + USED_CODE_TTL_MINUTES, + TimeUnit.MINUTES + )); + if (!claimed) { + throw new DomainBadRequestException("error.deviceAuth.deviceCode.used"); + } + + try { + if (!StringUtils.hasText(data.getUserId())) { + throw new DomainBadRequestException("error.deviceAuth.deviceCode.invalid"); + } + + String token = apiTokenService.createToken( + data.getUserId(), + CLI_DEVICE_TOKEN_NAME, + CLI_DEVICE_SCOPE_JSON + ).rawToken(); + + data.setStatus(DeviceCodeStatus.USED); + redisTemplate.opsForValue().set( + DEVICE_CODE_PREFIX + deviceCode, + data, + USED_CODE_TTL_MINUTES, + TimeUnit.MINUTES + ); + redisTemplate.delete(USER_CODE_PREFIX + data.getUserCode()); + return DeviceTokenResponse.success(token); + } catch (RuntimeException ex) { + redisTemplate.delete(DEVICE_CLAIM_PREFIX + deviceCode); + throw ex; + } + } + private String generateRandomDeviceCode() { byte[] bytes = new byte[32]; random.nextBytes(bytes); diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/device/DeviceAuthServiceTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/device/DeviceAuthServiceTest.java index 993ae314..ac15521a 100644 --- a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/device/DeviceAuthServiceTest.java +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/device/DeviceAuthServiceTest.java @@ -1,5 +1,7 @@ package com.iflytek.skillhub.auth.device; +import com.iflytek.skillhub.auth.entity.ApiToken; +import com.iflytek.skillhub.auth.token.ApiTokenService; import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -26,12 +28,15 @@ class DeviceAuthServiceTest { @Mock private ValueOperations valueOperations; + @Mock + private ApiTokenService apiTokenService; + private DeviceAuthService service; @BeforeEach void setUp() { when(redisTemplate.opsForValue()).thenReturn(valueOperations); - service = new DeviceAuthService(redisTemplate, "https://skillhub.example.com/device"); + service = new DeviceAuthService(redisTemplate, apiTokenService, "https://skillhub.example.com/device"); } @Test @@ -77,6 +82,41 @@ class DeviceAuthServiceTest { assertThat(response.accessToken()).isNull(); } + @Test + void pollToken_returns_access_token_when_authorized() { + // Given + DeviceCodeData data = new DeviceCodeData("device123", "ABCD-1234", DeviceCodeStatus.AUTHORIZED, "42"); + when(valueOperations.get("device:code:device123")).thenReturn(data); + when(valueOperations.setIfAbsent("device:claim:device123", "claimed", 1L, TimeUnit.MINUTES)).thenReturn(true); + when(apiTokenService.createToken("42", "CLI Device Flow", "[\"skill:read\",\"skill:publish\"]")) + .thenReturn(new ApiTokenService.TokenCreateResult("sk_cli_token", mock(ApiToken.class))); + + // When + DeviceTokenResponse response = service.pollToken("device123"); + + // Then + assertThat(response.accessToken()).isEqualTo("sk_cli_token"); + assertThat(response.tokenType()).isEqualTo("Bearer"); + assertThat(response.error()).isNull(); + assertThat(data.getStatus()).isEqualTo(DeviceCodeStatus.USED); + verify(valueOperations).set("device:code:device123", data, 1L, TimeUnit.MINUTES); + verify(redisTemplate).delete("device:usercode:ABCD-1234"); + } + + @Test + void pollToken_rejects_second_exchange_attempt() { + // Given + DeviceCodeData data = new DeviceCodeData("device123", "ABCD-1234", DeviceCodeStatus.AUTHORIZED, "42"); + when(valueOperations.get("device:code:device123")).thenReturn(data); + when(valueOperations.setIfAbsent("device:claim:device123", "claimed", 1L, TimeUnit.MINUTES)).thenReturn(false); + + // When / Then + assertThatThrownBy(() -> service.pollToken("device123")) + .isInstanceOf(DomainBadRequestException.class) + .hasMessageContaining("error.deviceAuth.deviceCode.used"); + verify(apiTokenService, never()).createToken(anyString(), anyString(), anyString()); + } + @Test void pollToken_returns_error_when_expired() { // Given @@ -103,4 +143,17 @@ class DeviceAuthServiceTest { assertThat(data.getUserId()).isEqualTo("42"); verify(valueOperations).set(eq("device:code:device123"), eq(data), eq(15L), eq(TimeUnit.MINUTES)); } + + @Test + void authorizeDeviceCode_rejects_different_user_after_authorization() { + // Given + DeviceCodeData data = new DeviceCodeData("device123", "ABCD-1234", DeviceCodeStatus.AUTHORIZED, "42"); + when(valueOperations.get("device:usercode:ABCD-1234")).thenReturn("device123"); + when(valueOperations.get("device:code:device123")).thenReturn(data); + + // When / Then + assertThatThrownBy(() -> service.authorizeDeviceCode("ABCD-1234", "99")) + .isInstanceOf(DomainBadRequestException.class) + .hasMessageContaining("error.deviceAuth.deviceCode.alreadyAuthorized"); + } } From f99ffbd7eca8891ff1730649db88caffabbac616 Mon Sep 17 00:00:00 2001 From: tlzhu3 Date: Thu, 12 Mar 2026 22:26:44 +0800 Subject: [PATCH 03/13] fix(review): tighten review and promotion access boundaries - derive review namespace from skill ownership instead of trusting request input\n- require namespace membership for review submission and owner or namespace admin rights for promotion submission\n- forbid unauthorized pending-list and detail reads in review and promotion portal endpoints\n- add domain and controller regression tests for submit and read permission boundaries --- .../portal/PromotionController.java | 26 ++- .../controller/portal/ReviewController.java | 45 +++- .../PromotionPortalControllerTest.java | 201 +++++++++++++++++ .../ReviewPortalControllerTest.java | 213 ++++++++++++++++++ .../domain/review/PromotionService.java | 9 +- .../review/ReviewPermissionChecker.java | 73 +++++- .../skillhub/domain/review/ReviewService.java | 14 +- .../domain/review/PromotionServiceTest.java | 83 ++++++- .../review/ReviewPermissionCheckerTest.java | 49 ++++ .../domain/review/ReviewServiceTest.java | 40 +++- 10 files changed, 713 insertions(+), 40 deletions(-) create mode 100644 server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/PromotionPortalControllerTest.java create mode 100644 server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/ReviewPortalControllerTest.java diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/PromotionController.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/PromotionController.java index 270372be..66d276d0 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/PromotionController.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/PromotionController.java @@ -4,10 +4,13 @@ import com.iflytek.skillhub.auth.rbac.RbacService; import com.iflytek.skillhub.controller.BaseApiController; 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.review.PromotionRequest; import com.iflytek.skillhub.domain.review.PromotionRequestRepository; import com.iflytek.skillhub.domain.review.PromotionService; +import com.iflytek.skillhub.domain.review.ReviewPermissionChecker; import com.iflytek.skillhub.domain.review.ReviewTaskStatus; +import com.iflytek.skillhub.domain.shared.exception.DomainForbiddenException; import com.iflytek.skillhub.domain.skill.Skill; import com.iflytek.skillhub.domain.skill.SkillRepository; import com.iflytek.skillhub.domain.skill.SkillVersion; @@ -19,6 +22,7 @@ import org.springframework.data.domain.Page; import org.springframework.data.domain.PageRequest; import org.springframework.web.bind.annotation.*; +import java.util.Map; import java.util.Set; @RestController @@ -32,6 +36,7 @@ public class PromotionController extends BaseApiController { private final NamespaceRepository namespaceRepository; private final UserAccountRepository userAccountRepository; private final RbacService rbacService; + private final ReviewPermissionChecker permissionChecker; public PromotionController(PromotionService promotionService, PromotionRequestRepository promotionRequestRepository, @@ -40,6 +45,7 @@ public class PromotionController extends BaseApiController { NamespaceRepository namespaceRepository, UserAccountRepository userAccountRepository, RbacService rbacService, + ReviewPermissionChecker permissionChecker, ApiResponseFactory responseFactory) { super(responseFactory); this.promotionService = promotionService; @@ -49,15 +55,18 @@ public class PromotionController extends BaseApiController { this.namespaceRepository = namespaceRepository; this.userAccountRepository = userAccountRepository; this.rbacService = rbacService; + this.permissionChecker = permissionChecker; } @PostMapping public ApiResponse submitPromotion( @RequestBody PromotionRequestDto request, - @RequestAttribute("userId") String userId) { + @RequestAttribute("userId") String userId, + @RequestAttribute(value = "userNsRoles", required = false) Map userNsRoles) { PromotionRequest promotion = promotionService.submitPromotion( request.sourceSkillId(), request.sourceVersionId(), - request.targetNamespaceId(), userId); + request.targetNamespaceId(), userId, + userNsRoles != null ? userNsRoles : Map.of()); return ok("response.success.create", toResponse(promotion)); } @@ -89,9 +98,8 @@ public class PromotionController extends BaseApiController { @RequestParam(defaultValue = "20") int size, @RequestAttribute("userId") String userId) { Set platformRoles = rbacService.getUserRoleCodes(userId); - boolean hasAdminRole = platformRoles.contains("SKILL_ADMIN") || platformRoles.contains("SUPER_ADMIN"); - if (!hasAdminRole) { - return ok("response.success.read", PageResponse.from(Page.empty())); + if (!permissionChecker.canListPendingPromotions(platformRoles)) { + throw new DomainForbiddenException("promotion.no_permission"); } Page requests = promotionRequestRepository.findByStatus( ReviewTaskStatus.PENDING, PageRequest.of(page, size)); @@ -99,8 +107,14 @@ public class PromotionController extends BaseApiController { } @GetMapping("/{id}") - public ApiResponse getPromotionDetail(@PathVariable Long id) { + public ApiResponse getPromotionDetail( + @PathVariable Long id, + @RequestAttribute("userId") String userId) { PromotionRequest promotion = promotionRequestRepository.findById(id).orElseThrow(); + Set platformRoles = rbacService.getUserRoleCodes(userId); + if (!permissionChecker.canReadPromotion(promotion, userId, platformRoles)) { + throw new DomainForbiddenException("promotion.no_permission"); + } return ok("response.success.read", toResponse(promotion)); } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/ReviewController.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/ReviewController.java index 742c263a..1b92da5b 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/ReviewController.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/ReviewController.java @@ -6,9 +6,11 @@ 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.review.ReviewService; +import com.iflytek.skillhub.domain.review.ReviewPermissionChecker; import com.iflytek.skillhub.domain.review.ReviewTask; import com.iflytek.skillhub.domain.review.ReviewTaskRepository; import com.iflytek.skillhub.domain.review.ReviewTaskStatus; +import com.iflytek.skillhub.domain.shared.exception.DomainForbiddenException; import com.iflytek.skillhub.domain.skill.Skill; import com.iflytek.skillhub.domain.skill.SkillRepository; import com.iflytek.skillhub.domain.skill.SkillVersion; @@ -34,6 +36,7 @@ public class ReviewController extends BaseApiController { private final NamespaceRepository namespaceRepository; private final UserAccountRepository userAccountRepository; private final RbacService rbacService; + private final ReviewPermissionChecker permissionChecker; public ReviewController(ReviewService reviewService, ReviewTaskRepository reviewTaskRepository, @@ -42,6 +45,7 @@ public class ReviewController extends BaseApiController { NamespaceRepository namespaceRepository, UserAccountRepository userAccountRepository, RbacService rbacService, + ReviewPermissionChecker permissionChecker, ApiResponseFactory responseFactory) { super(responseFactory); this.reviewService = reviewService; @@ -51,16 +55,19 @@ public class ReviewController extends BaseApiController { this.namespaceRepository = namespaceRepository; this.userAccountRepository = userAccountRepository; this.rbacService = rbacService; + this.permissionChecker = permissionChecker; } @PostMapping public ApiResponse submitReview( @RequestBody ReviewTaskRequest request, - @RequestAttribute("userId") String userId) { - SkillVersion sv = skillVersionRepository.findById(request.skillVersionId()) - .orElseThrow(); - Skill skill = skillRepository.findById(sv.getSkillId()).orElseThrow(); - ReviewTask task = reviewService.submitReview(request.skillVersionId(), skill.getNamespaceId(), userId); + @RequestAttribute("userId") String userId, + @RequestAttribute(value = "userNsRoles", required = false) Map userNsRoles) { + ReviewTask task = reviewService.submitReview( + request.skillVersionId(), + userId, + userNsRoles != null ? userNsRoles : Map.of() + ); return ok("response.success.create", toResponse(task)); } @@ -104,7 +111,18 @@ public class ReviewController extends BaseApiController { @RequestParam Long namespaceId, @RequestParam(defaultValue = "0") int page, @RequestParam(defaultValue = "20") int size, - @RequestAttribute("userId") String userId) { + @RequestAttribute("userId") String userId, + @RequestAttribute(value = "userNsRoles", required = false) Map userNsRoles) { + Namespace namespace = namespaceRepository.findById(namespaceId).orElseThrow(); + Set platformRoles = rbacService.getUserRoleCodes(userId); + if (!permissionChecker.canManageNamespaceReviews( + namespaceId, + namespace.getType(), + userNsRoles != null ? userNsRoles : Map.of(), + platformRoles)) { + throw new DomainForbiddenException("review.no_permission"); + } + Page tasks = reviewTaskRepository.findByNamespaceIdAndStatus( namespaceId, ReviewTaskStatus.PENDING, PageRequest.of(page, size)); return ok("response.success.read", PageResponse.from(tasks.map(this::toResponse))); @@ -121,8 +139,21 @@ public class ReviewController extends BaseApiController { } @GetMapping("/{id}") - public ApiResponse getReviewDetail(@PathVariable Long id) { + public ApiResponse getReviewDetail( + @PathVariable Long id, + @RequestAttribute("userId") String userId, + @RequestAttribute(value = "userNsRoles", required = false) Map userNsRoles) { ReviewTask task = reviewTaskRepository.findById(id).orElseThrow(); + Namespace namespace = namespaceRepository.findById(task.getNamespaceId()).orElseThrow(); + Set platformRoles = rbacService.getUserRoleCodes(userId); + if (!permissionChecker.canReadReview( + task, + userId, + namespace.getType(), + userNsRoles != null ? userNsRoles : Map.of(), + platformRoles)) { + throw new DomainForbiddenException("review.no_permission"); + } return ok("response.success.read", toResponse(task)); } diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/PromotionPortalControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/PromotionPortalControllerTest.java new file mode 100644 index 00000000..2e57b0f7 --- /dev/null +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/PromotionPortalControllerTest.java @@ -0,0 +1,201 @@ +package com.iflytek.skillhub.controller; + +import com.iflytek.skillhub.auth.device.DeviceAuthService; +import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; +import com.iflytek.skillhub.auth.rbac.RbacService; +import com.iflytek.skillhub.domain.namespace.Namespace; +import com.iflytek.skillhub.domain.namespace.NamespaceMember; +import com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository; +import com.iflytek.skillhub.domain.namespace.NamespaceRole; +import com.iflytek.skillhub.domain.review.PromotionRequest; +import com.iflytek.skillhub.domain.review.PromotionRequestRepository; +import com.iflytek.skillhub.domain.review.PromotionService; +import com.iflytek.skillhub.domain.review.ReviewPermissionChecker; +import com.iflytek.skillhub.domain.review.ReviewTaskStatus; +import com.iflytek.skillhub.domain.skill.Skill; +import com.iflytek.skillhub.domain.skill.SkillRepository; +import com.iflytek.skillhub.domain.skill.SkillVersion; +import com.iflytek.skillhub.domain.skill.SkillVersionRepository; +import com.iflytek.skillhub.domain.skill.SkillVisibility; +import com.iflytek.skillhub.domain.user.UserAccount; +import com.iflytek.skillhub.domain.user.UserAccountRepository; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.http.MediaType; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; +import org.springframework.security.core.authority.SimpleGrantedAuthority; +import org.springframework.test.context.ActiveProfiles; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.request.RequestPostProcessor; + +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; + +import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.authentication; +import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +@SpringBootTest +@AutoConfigureMockMvc +@ActiveProfiles("test") +class PromotionPortalControllerTest { + + @Autowired + private MockMvc mockMvc; + + @MockBean + private PromotionService promotionService; + + @MockBean + private PromotionRequestRepository promotionRequestRepository; + + @MockBean + private SkillRepository skillRepository; + + @MockBean + private SkillVersionRepository skillVersionRepository; + + @MockBean + private NamespaceMemberRepository namespaceMemberRepository; + + @MockBean + private DeviceAuthService deviceAuthService; + + @MockBean + private com.iflytek.skillhub.domain.namespace.NamespaceRepository namespaceRepository; + + @MockBean + private UserAccountRepository userAccountRepository; + + @MockBean + private RbacService rbacService; + + @MockBean + private ReviewPermissionChecker permissionChecker; + + @Test + void submitPromotion_passesNamespaceRolesToService() throws Exception { + PromotionRequest request = createPromotionRequest(1L, "user-1"); + stubNamespaceRoles("user-1", List.of(new NamespaceMember(5L, "user-1", NamespaceRole.ADMIN))); + given(promotionService.submitPromotion(10L, 20L, 30L, "user-1", Map.of(5L, NamespaceRole.ADMIN))) + .willReturn(request); + stubPromotionResponse(request); + + mockMvc.perform(post("/api/v1/promotions") + .contentType(MediaType.APPLICATION_JSON) + .content("{\"sourceSkillId\":10,\"sourceVersionId\":20,\"targetNamespaceId\":30}") + .with(csrf()) + .with(auth("user-1"))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.code").value(0)) + .andExpect(jsonPath("$.data.id").value(1L)); + } + + @Test + void listPendingPromotions_forbidsRegularUser() throws Exception { + stubNamespaceRoles("user-1", List.of()); + given(rbacService.getUserRoleCodes("user-1")).willReturn(Set.of()); + given(permissionChecker.canListPendingPromotions(Set.of())).willReturn(false); + + mockMvc.perform(get("/api/v1/promotions/pending").with(auth("user-1"))) + .andExpect(status().isForbidden()) + .andExpect(jsonPath("$.code").value(403)); + + verify(promotionRequestRepository, never()).findByStatus(org.mockito.ArgumentMatchers.any(), org.mockito.ArgumentMatchers.any()); + } + + @Test + void getPromotionDetail_allowsSubmitter() throws Exception { + PromotionRequest request = createPromotionRequest(1L, "user-1"); + stubNamespaceRoles("user-1", List.of()); + given(promotionRequestRepository.findById(1L)).willReturn(Optional.of(request)); + given(rbacService.getUserRoleCodes("user-1")).willReturn(Set.of()); + given(permissionChecker.canReadPromotion(request, "user-1", Set.of())).willReturn(true); + stubPromotionResponse(request); + + mockMvc.perform(get("/api/v1/promotions/1").with(auth("user-1"))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.code").value(0)) + .andExpect(jsonPath("$.data.submittedBy").value("user-1")); + } + + @Test + void getPromotionDetail_forbidsUnrelatedUser() throws Exception { + PromotionRequest request = createPromotionRequest(1L, "user-1"); + stubNamespaceRoles("user-9", List.of()); + given(promotionRequestRepository.findById(1L)).willReturn(Optional.of(request)); + given(rbacService.getUserRoleCodes("user-9")).willReturn(Set.of()); + given(permissionChecker.canReadPromotion(request, "user-9", Set.of())).willReturn(false); + + mockMvc.perform(get("/api/v1/promotions/1").with(auth("user-9"))) + .andExpect(status().isForbidden()) + .andExpect(jsonPath("$.code").value(403)); + } + + private void stubPromotionResponse(PromotionRequest request) { + Skill skill = new Skill(5L, "skill-a", request.getSubmittedBy(), SkillVisibility.PUBLIC); + setField(skill, "id", request.getSourceSkillId()); + SkillVersion version = new SkillVersion(request.getSourceSkillId(), "1.0.0", request.getSubmittedBy()); + setField(version, "id", request.getSourceVersionId()); + Namespace sourceNamespace = new Namespace("team-a", "Team A", "owner-1"); + setField(sourceNamespace, "id", 5L); + Namespace targetNamespace = new Namespace("global", "Global", "owner-2"); + setField(targetNamespace, "id", request.getTargetNamespaceId()); + UserAccount submitter = new UserAccount(request.getSubmittedBy(), "Submitter", "submitter@example.com", ""); + + given(skillRepository.findById(request.getSourceSkillId())).willReturn(Optional.of(skill)); + given(skillVersionRepository.findById(request.getSourceVersionId())).willReturn(Optional.of(version)); + given(namespaceRepository.findById(5L)).willReturn(Optional.of(sourceNamespace)); + given(namespaceRepository.findById(request.getTargetNamespaceId())).willReturn(Optional.of(targetNamespace)); + given(userAccountRepository.findById(request.getSubmittedBy())).willReturn(Optional.of(submitter)); + } + + private void stubNamespaceRoles(String userId, List members) { + given(namespaceMemberRepository.findByUserId(userId)).willReturn(members); + } + + private RequestPostProcessor auth(String userId) { + PlatformPrincipal principal = new PlatformPrincipal( + userId, + userId, + userId + "@example.com", + "", + "session", + Set.of() + ); + UsernamePasswordAuthenticationToken authenticationToken = new UsernamePasswordAuthenticationToken( + principal, + null, + List.of(new SimpleGrantedAuthority("ROLE_USER")) + ); + return authentication(authenticationToken); + } + + private PromotionRequest createPromotionRequest(Long id, String submittedBy) { + PromotionRequest request = new PromotionRequest(10L, 20L, 30L, submittedBy); + setField(request, "id", id); + setField(request, "status", ReviewTaskStatus.PENDING); + return request; + } + + private void setField(Object target, String fieldName, Object value) { + try { + java.lang.reflect.Field field = target.getClass().getDeclaredField(fieldName); + field.setAccessible(true); + field.set(target, value); + } catch (Exception e) { + throw new RuntimeException(e); + } + } +} 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 new file mode 100644 index 00000000..44551a8e --- /dev/null +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/ReviewPortalControllerTest.java @@ -0,0 +1,213 @@ +package com.iflytek.skillhub.controller; + +import com.iflytek.skillhub.auth.device.DeviceAuthService; +import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; +import com.iflytek.skillhub.auth.rbac.RbacService; +import com.iflytek.skillhub.domain.namespace.Namespace; +import com.iflytek.skillhub.domain.namespace.NamespaceMember; +import com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository; +import com.iflytek.skillhub.domain.namespace.NamespaceRole; +import com.iflytek.skillhub.domain.review.ReviewPermissionChecker; +import com.iflytek.skillhub.domain.review.ReviewService; +import com.iflytek.skillhub.domain.review.ReviewTask; +import com.iflytek.skillhub.domain.review.ReviewTaskRepository; +import com.iflytek.skillhub.domain.review.ReviewTaskStatus; +import com.iflytek.skillhub.domain.skill.Skill; +import com.iflytek.skillhub.domain.skill.SkillRepository; +import com.iflytek.skillhub.domain.skill.SkillVersion; +import com.iflytek.skillhub.domain.skill.SkillVersionRepository; +import com.iflytek.skillhub.domain.skill.SkillVisibility; +import com.iflytek.skillhub.domain.user.UserAccount; +import com.iflytek.skillhub.domain.user.UserAccountRepository; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.http.MediaType; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; +import org.springframework.security.core.authority.SimpleGrantedAuthority; +import org.springframework.test.context.ActiveProfiles; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.request.RequestPostProcessor; + +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; + +import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.authentication; +import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +@SpringBootTest +@AutoConfigureMockMvc +@ActiveProfiles("test") +class ReviewPortalControllerTest { + + @Autowired + private MockMvc mockMvc; + + @MockBean + private ReviewService reviewService; + + @MockBean + private ReviewTaskRepository reviewTaskRepository; + + @MockBean + private SkillRepository skillRepository; + + @MockBean + private SkillVersionRepository skillVersionRepository; + + @MockBean + private NamespaceMemberRepository namespaceMemberRepository; + + @MockBean + private DeviceAuthService deviceAuthService; + + @MockBean + private com.iflytek.skillhub.domain.namespace.NamespaceRepository namespaceRepository; + + @MockBean + private UserAccountRepository userAccountRepository; + + @MockBean + private RbacService rbacService; + + @MockBean + private ReviewPermissionChecker permissionChecker; + + @Test + void submitReview_passesNamespaceRolesToService() throws Exception { + ReviewTask task = createReviewTask(1L, 20L, "user-1"); + stubNamespaceRoles("user-1", List.of(new NamespaceMember(20L, "user-1", NamespaceRole.MEMBER))); + given(reviewService.submitReview(100L, "user-1", Map.of(20L, NamespaceRole.MEMBER))).willReturn(task); + stubReviewResponse(task); + + mockMvc.perform(post("/api/v1/reviews") + .contentType(MediaType.APPLICATION_JSON) + .content("{\"skillVersionId\":100}") + .with(csrf()) + .with(auth("user-1"))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.code").value(0)) + .andExpect(jsonPath("$.data.id").value(1L)); + } + + @Test + void listPendingReviews_forbidsNamespaceMember() throws Exception { + Namespace namespace = createNamespace(20L, "team-a"); + stubNamespaceRoles("user-1", List.of(new NamespaceMember(20L, "user-1", NamespaceRole.MEMBER))); + given(namespaceRepository.findById(20L)).willReturn(Optional.of(namespace)); + given(rbacService.getUserRoleCodes("user-1")).willReturn(Set.of()); + given(permissionChecker.canManageNamespaceReviews( + 20L, + namespace.getType(), + Map.of(20L, NamespaceRole.MEMBER), + Set.of())).willReturn(false); + + mockMvc.perform(get("/api/v1/reviews/pending") + .param("namespaceId", "20") + .with(auth("user-1"))) + .andExpect(status().isForbidden()) + .andExpect(jsonPath("$.code").value(403)); + + verify(reviewTaskRepository, never()).findByNamespaceIdAndStatus(org.mockito.ArgumentMatchers.any(), org.mockito.ArgumentMatchers.any(), org.mockito.ArgumentMatchers.any()); + } + + @Test + void getReviewDetail_allowsSubmitter() throws Exception { + ReviewTask task = createReviewTask(1L, 20L, "user-1"); + Namespace namespace = createNamespace(20L, "team-a"); + stubNamespaceRoles("user-1", List.of()); + given(reviewTaskRepository.findById(1L)).willReturn(Optional.of(task)); + given(namespaceRepository.findById(20L)).willReturn(Optional.of(namespace)); + given(rbacService.getUserRoleCodes("user-1")).willReturn(Set.of()); + given(permissionChecker.canReadReview(task, "user-1", namespace.getType(), Map.of(), Set.of())).willReturn(true); + stubReviewResponse(task); + + mockMvc.perform(get("/api/v1/reviews/1").with(auth("user-1"))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.code").value(0)) + .andExpect(jsonPath("$.data.submittedBy").value("user-1")); + } + + @Test + void getReviewDetail_forbidsUnrelatedUser() throws Exception { + ReviewTask task = createReviewTask(1L, 20L, "user-1"); + Namespace namespace = createNamespace(20L, "team-a"); + stubNamespaceRoles("user-9", List.of()); + given(reviewTaskRepository.findById(1L)).willReturn(Optional.of(task)); + given(namespaceRepository.findById(20L)).willReturn(Optional.of(namespace)); + given(rbacService.getUserRoleCodes("user-9")).willReturn(Set.of()); + given(permissionChecker.canReadReview(task, "user-9", namespace.getType(), Map.of(), Set.of())).willReturn(false); + + mockMvc.perform(get("/api/v1/reviews/1").with(auth("user-9"))) + .andExpect(status().isForbidden()) + .andExpect(jsonPath("$.code").value(403)); + } + + private void stubReviewResponse(ReviewTask task) { + SkillVersion version = new SkillVersion(30L, "1.0.0", task.getSubmittedBy()); + setField(version, "id", task.getSkillVersionId()); + Skill skill = new Skill(task.getNamespaceId(), "skill-a", task.getSubmittedBy(), SkillVisibility.PUBLIC); + setField(skill, "id", 30L); + UserAccount submitter = new UserAccount(task.getSubmittedBy(), "Submitter", "submitter@example.com", ""); + + given(skillVersionRepository.findById(task.getSkillVersionId())).willReturn(Optional.of(version)); + given(skillRepository.findById(30L)).willReturn(Optional.of(skill)); + given(namespaceRepository.findById(task.getNamespaceId())).willReturn(Optional.of(createNamespace(task.getNamespaceId(), "team-a"))); + given(userAccountRepository.findById(task.getSubmittedBy())).willReturn(Optional.of(submitter)); + } + + private void stubNamespaceRoles(String userId, List members) { + given(namespaceMemberRepository.findByUserId(userId)).willReturn(members); + } + + private RequestPostProcessor auth(String userId) { + PlatformPrincipal principal = new PlatformPrincipal( + userId, + userId, + userId + "@example.com", + "", + "session", + Set.of() + ); + UsernamePasswordAuthenticationToken authenticationToken = new UsernamePasswordAuthenticationToken( + principal, + null, + List.of(new SimpleGrantedAuthority("ROLE_USER")) + ); + return authentication(authenticationToken); + } + + private ReviewTask createReviewTask(Long id, Long namespaceId, String submittedBy) { + ReviewTask task = new ReviewTask(100L, namespaceId, submittedBy); + setField(task, "id", id); + setField(task, "status", ReviewTaskStatus.PENDING); + return task; + } + + private Namespace createNamespace(Long id, String slug) { + Namespace namespace = new Namespace(slug, "Team", "owner-1"); + setField(namespace, "id", id); + return namespace; + } + + private void setField(Object target, String fieldName, Object value) { + try { + java.lang.reflect.Field field = target.getClass().getDeclaredField(fieldName); + field.setAccessible(true); + field.set(target, value); + } catch (Exception e) { + throw new RuntimeException(e); + } + } +} diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/PromotionService.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/PromotionService.java index 01d4d581..919d847f 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/PromotionService.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/PromotionService.java @@ -3,6 +3,7 @@ package com.iflytek.skillhub.domain.review; import com.iflytek.skillhub.domain.event.SkillPublishedEvent; 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.NamespaceType; import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException; import com.iflytek.skillhub.domain.shared.exception.DomainForbiddenException; @@ -15,6 +16,7 @@ import org.springframework.transaction.annotation.Transactional; import java.time.LocalDateTime; import java.util.ConcurrentModificationException; import java.util.List; +import java.util.Map; import java.util.Set; @Service @@ -46,7 +48,8 @@ public class PromotionService { @Transactional public PromotionRequest submitPromotion(Long sourceSkillId, Long sourceVersionId, - Long targetNamespaceId, String userId) { + Long targetNamespaceId, String userId, + Map userNamespaceRoles) { Skill sourceSkill = skillRepository.findById(sourceSkillId) .orElseThrow(() -> new DomainNotFoundException("skill.not_found", sourceSkillId)); @@ -61,6 +64,10 @@ public class PromotionService { throw new DomainBadRequestException("promotion.version_not_published", sourceVersionId); } + if (!permissionChecker.canSubmitPromotion(sourceSkill, userId, userNamespaceRoles)) { + throw new DomainForbiddenException("promotion.submit.no_permission"); + } + Namespace targetNamespace = namespaceRepository.findById(targetNamespaceId) .orElseThrow(() -> new DomainNotFoundException("namespace.not_found", targetNamespaceId)); diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/ReviewPermissionChecker.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/ReviewPermissionChecker.java index 0cc89df1..4983a4a7 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/ReviewPermissionChecker.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/ReviewPermissionChecker.java @@ -2,6 +2,7 @@ package com.iflytek.skillhub.domain.review; import com.iflytek.skillhub.domain.namespace.NamespaceRole; import com.iflytek.skillhub.domain.namespace.NamespaceType; +import com.iflytek.skillhub.domain.skill.Skill; import org.springframework.stereotype.Component; import java.util.Map; @@ -10,6 +11,26 @@ import java.util.Set; @Component public class ReviewPermissionChecker { + public boolean canSubmitReview(Long namespaceId, + Map userNamespaceRoles) { + NamespaceRole role = userNamespaceRoles.get(namespaceId); + return role == NamespaceRole.OWNER + || role == NamespaceRole.ADMIN + || role == NamespaceRole.MEMBER; + } + + public boolean canManageNamespaceReviews(Long namespaceId, + NamespaceType namespaceType, + Map userNamespaceRoles, + Set platformRoles) { + if (namespaceType == NamespaceType.GLOBAL) { + return hasPlatformReviewRole(platformRoles); + } + + NamespaceRole role = userNamespaceRoles.get(namespaceId); + return role == NamespaceRole.OWNER || role == NamespaceRole.ADMIN; + } + /** * Check if a user can review a ReviewTask. * @@ -31,16 +52,37 @@ public class ReviewPermissionChecker { } // Global namespace: only SKILL_ADMIN or SUPER_ADMIN - if (namespaceType == NamespaceType.GLOBAL) { - return platformRoles.contains("SKILL_ADMIN") - || platformRoles.contains("SUPER_ADMIN"); + return canManageNamespaceReviews( + task.getNamespaceId(), + namespaceType, + userNamespaceRoles, + platformRoles + ); + } + + public boolean canReadReview(ReviewTask task, + String userId, + NamespaceType namespaceType, + Map userNamespaceRoles, + Set platformRoles) { + return task.getSubmittedBy().equals(userId) + || canManageNamespaceReviews( + task.getNamespaceId(), + namespaceType, + userNamespaceRoles, + platformRoles + ); + } + + public boolean canSubmitPromotion(Skill sourceSkill, + String userId, + Map userNamespaceRoles) { + if (sourceSkill.getOwnerId().equals(userId)) { + return true; } - // Team namespace: namespace ADMIN or OWNER - NamespaceRole role = userNamespaceRoles.get( - task.getNamespaceId()); - return role == NamespaceRole.ADMIN - || role == NamespaceRole.OWNER; + NamespaceRole role = userNamespaceRoles.get(sourceSkill.getNamespaceId()); + return role == NamespaceRole.OWNER || role == NamespaceRole.ADMIN; } /** @@ -54,6 +96,21 @@ public class ReviewPermissionChecker { if (request.getSubmittedBy().equals(userId)) { return false; } + return hasPlatformReviewRole(platformRoles); + } + + public boolean canListPendingPromotions(Set platformRoles) { + return hasPlatformReviewRole(platformRoles); + } + + public boolean canReadPromotion(PromotionRequest request, + String userId, + Set platformRoles) { + return request.getSubmittedBy().equals(userId) + || canListPendingPromotions(platformRoles); + } + + private boolean hasPlatformReviewRole(Set platformRoles) { return platformRoles.contains("SKILL_ADMIN") || platformRoles.contains("SUPER_ADMIN"); } diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/ReviewService.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/ReviewService.java index fb2482af..8e2f6308 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/ReviewService.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/ReviewService.java @@ -17,7 +17,6 @@ import org.springframework.dao.DataIntegrityViolationException; import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; -import java.time.Instant; import java.time.LocalDateTime; import java.util.ConcurrentModificationException; import java.util.Map; @@ -48,18 +47,27 @@ public class ReviewService { } @Transactional - public ReviewTask submitReview(Long skillVersionId, Long namespaceId, String userId) { + public ReviewTask submitReview(Long skillVersionId, + String userId, + Map userNamespaceRoles) { SkillVersion skillVersion = skillVersionRepository.findById(skillVersionId) .orElseThrow(() -> new DomainNotFoundException("skill_version.not_found", skillVersionId)); + Skill skill = skillRepository.findById(skillVersion.getSkillId()) + .orElseThrow(() -> new DomainNotFoundException("skill.not_found", skillVersion.getSkillId())); + if (skillVersion.getStatus() != SkillVersionStatus.DRAFT) { throw new DomainBadRequestException("review.submit.not_draft", skillVersionId); } + if (!permissionChecker.canSubmitReview(skill.getNamespaceId(), userNamespaceRoles)) { + throw new DomainForbiddenException("review.submit.no_permission"); + } + skillVersion.setStatus(SkillVersionStatus.PENDING_REVIEW); skillVersionRepository.save(skillVersion); - ReviewTask task = new ReviewTask(skillVersionId, namespaceId, userId); + ReviewTask task = new ReviewTask(skillVersionId, skill.getNamespaceId(), userId); try { return reviewTaskRepository.save(task); } catch (DataIntegrityViolationException e) { diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/review/PromotionServiceTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/review/PromotionServiceTest.java index 7670a76d..b1a8ea76 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/review/PromotionServiceTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/review/PromotionServiceTest.java @@ -121,6 +121,7 @@ class PromotionServiceTest { when(skillRepository.findById(SOURCE_SKILL_ID)).thenReturn(Optional.of(sourceSkill)); when(skillVersionRepository.findById(SOURCE_VERSION_ID)).thenReturn(Optional.of(sourceVersion)); + when(permissionChecker.canSubmitPromotion(sourceSkill, USER_ID, Map.of())).thenReturn(true); when(namespaceRepository.findById(TARGET_NAMESPACE_ID)).thenReturn(Optional.of(globalNs)); when(promotionRequestRepository.findBySourceVersionIdAndStatus(SOURCE_VERSION_ID, ReviewTaskStatus.PENDING)) .thenReturn(Optional.empty()); @@ -132,7 +133,7 @@ class PromotionServiceTest { }); PromotionRequest result = promotionService.submitPromotion( - SOURCE_SKILL_ID, SOURCE_VERSION_ID, TARGET_NAMESPACE_ID, USER_ID); + SOURCE_SKILL_ID, SOURCE_VERSION_ID, TARGET_NAMESPACE_ID, USER_ID, Map.of()); assertNotNull(result); assertEquals(SOURCE_SKILL_ID, result.getSourceSkillId()); @@ -147,7 +148,7 @@ class PromotionServiceTest { when(skillRepository.findById(SOURCE_SKILL_ID)).thenReturn(Optional.empty()); assertThrows(DomainNotFoundException.class, - () -> promotionService.submitPromotion(SOURCE_SKILL_ID, SOURCE_VERSION_ID, TARGET_NAMESPACE_ID, USER_ID)); + () -> promotionService.submitPromotion(SOURCE_SKILL_ID, SOURCE_VERSION_ID, TARGET_NAMESPACE_ID, USER_ID, Map.of())); } @Test @@ -156,7 +157,7 @@ class PromotionServiceTest { when(skillVersionRepository.findById(SOURCE_VERSION_ID)).thenReturn(Optional.empty()); assertThrows(DomainNotFoundException.class, - () -> promotionService.submitPromotion(SOURCE_SKILL_ID, SOURCE_VERSION_ID, TARGET_NAMESPACE_ID, USER_ID)); + () -> promotionService.submitPromotion(SOURCE_SKILL_ID, SOURCE_VERSION_ID, TARGET_NAMESPACE_ID, USER_ID, Map.of())); } @Test @@ -169,7 +170,7 @@ class PromotionServiceTest { when(skillVersionRepository.findById(SOURCE_VERSION_ID)).thenReturn(Optional.of(sv)); assertThrows(DomainBadRequestException.class, - () -> promotionService.submitPromotion(SOURCE_SKILL_ID, SOURCE_VERSION_ID, TARGET_NAMESPACE_ID, USER_ID)); + () -> promotionService.submitPromotion(SOURCE_SKILL_ID, SOURCE_VERSION_ID, TARGET_NAMESPACE_ID, USER_ID, Map.of())); } @Test @@ -182,39 +183,99 @@ class PromotionServiceTest { when(skillVersionRepository.findById(SOURCE_VERSION_ID)).thenReturn(Optional.of(sv)); assertThrows(DomainBadRequestException.class, - () -> promotionService.submitPromotion(SOURCE_SKILL_ID, SOURCE_VERSION_ID, TARGET_NAMESPACE_ID, USER_ID)); + () -> promotionService.submitPromotion(SOURCE_SKILL_ID, SOURCE_VERSION_ID, TARGET_NAMESPACE_ID, USER_ID, Map.of())); } @Test void shouldThrowWhenTargetNamespaceNotFound() { - when(skillRepository.findById(SOURCE_SKILL_ID)).thenReturn(Optional.of(createSourceSkill())); + Skill sourceSkill = createSourceSkill(); + when(skillRepository.findById(SOURCE_SKILL_ID)).thenReturn(Optional.of(sourceSkill)); when(skillVersionRepository.findById(SOURCE_VERSION_ID)).thenReturn(Optional.of(createPublishedVersion())); + when(permissionChecker.canSubmitPromotion(sourceSkill, USER_ID, Map.of())).thenReturn(true); when(namespaceRepository.findById(TARGET_NAMESPACE_ID)).thenReturn(Optional.empty()); assertThrows(DomainNotFoundException.class, - () -> promotionService.submitPromotion(SOURCE_SKILL_ID, SOURCE_VERSION_ID, TARGET_NAMESPACE_ID, USER_ID)); + () -> promotionService.submitPromotion(SOURCE_SKILL_ID, SOURCE_VERSION_ID, TARGET_NAMESPACE_ID, USER_ID, Map.of())); } @Test void shouldThrowWhenTargetNamespaceNotGlobal() { - when(skillRepository.findById(SOURCE_SKILL_ID)).thenReturn(Optional.of(createSourceSkill())); + Skill sourceSkill = createSourceSkill(); + when(skillRepository.findById(SOURCE_SKILL_ID)).thenReturn(Optional.of(sourceSkill)); when(skillVersionRepository.findById(SOURCE_VERSION_ID)).thenReturn(Optional.of(createPublishedVersion())); + when(permissionChecker.canSubmitPromotion(sourceSkill, USER_ID, Map.of())).thenReturn(true); when(namespaceRepository.findById(TARGET_NAMESPACE_ID)).thenReturn(Optional.of(createTeamNamespace())); assertThrows(DomainBadRequestException.class, - () -> promotionService.submitPromotion(SOURCE_SKILL_ID, SOURCE_VERSION_ID, TARGET_NAMESPACE_ID, USER_ID)); + () -> promotionService.submitPromotion(SOURCE_SKILL_ID, SOURCE_VERSION_ID, TARGET_NAMESPACE_ID, USER_ID, Map.of())); } @Test void shouldThrowWhenDuplicatePendingExists() { - when(skillRepository.findById(SOURCE_SKILL_ID)).thenReturn(Optional.of(createSourceSkill())); + Skill sourceSkill = createSourceSkill(); + when(skillRepository.findById(SOURCE_SKILL_ID)).thenReturn(Optional.of(sourceSkill)); when(skillVersionRepository.findById(SOURCE_VERSION_ID)).thenReturn(Optional.of(createPublishedVersion())); + when(permissionChecker.canSubmitPromotion(sourceSkill, USER_ID, Map.of())).thenReturn(true); when(namespaceRepository.findById(TARGET_NAMESPACE_ID)).thenReturn(Optional.of(createGlobalNamespace())); when(promotionRequestRepository.findBySourceVersionIdAndStatus(SOURCE_VERSION_ID, ReviewTaskStatus.PENDING)) .thenReturn(Optional.of(createPendingPromotion())); assertThrows(DomainBadRequestException.class, - () -> promotionService.submitPromotion(SOURCE_SKILL_ID, SOURCE_VERSION_ID, TARGET_NAMESPACE_ID, USER_ID)); + () -> promotionService.submitPromotion(SOURCE_SKILL_ID, SOURCE_VERSION_ID, TARGET_NAMESPACE_ID, USER_ID, Map.of())); + } + + @Test + void shouldThrowWhenSubmitterIsNotOwnerOrNamespaceAdmin() { + Skill sourceSkill = createSourceSkill(); + SkillVersion sourceVersion = createPublishedVersion(); + + when(skillRepository.findById(SOURCE_SKILL_ID)).thenReturn(Optional.of(sourceSkill)); + when(skillVersionRepository.findById(SOURCE_VERSION_ID)).thenReturn(Optional.of(sourceVersion)); + when(permissionChecker.canSubmitPromotion( + sourceSkill, + "user-999", + Map.of(sourceSkill.getNamespaceId(), com.iflytek.skillhub.domain.namespace.NamespaceRole.MEMBER))) + .thenReturn(false); + + assertThrows(DomainForbiddenException.class, + () -> promotionService.submitPromotion( + SOURCE_SKILL_ID, + SOURCE_VERSION_ID, + TARGET_NAMESPACE_ID, + "user-999", + Map.of(sourceSkill.getNamespaceId(), com.iflytek.skillhub.domain.namespace.NamespaceRole.MEMBER) + )); + verify(promotionRequestRepository, never()).save(any(PromotionRequest.class)); + } + + @Test + void shouldAllowNamespaceAdminToSubmitPromotionForForeignSkill() { + Skill sourceSkill = createSourceSkill(); + SkillVersion sourceVersion = createPublishedVersion(); + Namespace globalNs = createGlobalNamespace(); + + when(skillRepository.findById(SOURCE_SKILL_ID)).thenReturn(Optional.of(sourceSkill)); + when(skillVersionRepository.findById(SOURCE_VERSION_ID)).thenReturn(Optional.of(sourceVersion)); + when(permissionChecker.canSubmitPromotion( + sourceSkill, + "user-999", + Map.of(sourceSkill.getNamespaceId(), com.iflytek.skillhub.domain.namespace.NamespaceRole.ADMIN))) + .thenReturn(true); + when(namespaceRepository.findById(TARGET_NAMESPACE_ID)).thenReturn(Optional.of(globalNs)); + when(promotionRequestRepository.findBySourceVersionIdAndStatus(SOURCE_VERSION_ID, ReviewTaskStatus.PENDING)) + .thenReturn(Optional.empty()); + when(promotionRequestRepository.save(any(PromotionRequest.class))) + .thenAnswer(inv -> inv.getArgument(0)); + + PromotionRequest result = promotionService.submitPromotion( + SOURCE_SKILL_ID, + SOURCE_VERSION_ID, + TARGET_NAMESPACE_ID, + "user-999", + Map.of(sourceSkill.getNamespaceId(), com.iflytek.skillhub.domain.namespace.NamespaceRole.ADMIN) + ); + + assertNotNull(result); } } diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/review/ReviewPermissionCheckerTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/review/ReviewPermissionCheckerTest.java index f3d43cd9..48a3aadd 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/review/ReviewPermissionCheckerTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/review/ReviewPermissionCheckerTest.java @@ -2,6 +2,8 @@ package com.iflytek.skillhub.domain.review; import com.iflytek.skillhub.domain.namespace.NamespaceRole; import com.iflytek.skillhub.domain.namespace.NamespaceType; +import com.iflytek.skillhub.domain.skill.Skill; +import com.iflytek.skillhub.domain.skill.SkillVisibility; import org.junit.jupiter.api.Test; import java.util.Map; @@ -81,6 +83,53 @@ class ReviewPermissionCheckerTest { // --- canReviewPromotion tests --- + @Test + void memberCanSubmitReview() { + assertTrue(checker.canSubmitReview(10L, Map.of(10L, NamespaceRole.MEMBER))); + } + + @Test + void outsiderCannotSubmitReview() { + assertFalse(checker.canSubmitReview(10L, Map.of())); + } + + @Test + void teamAdminCanManagePendingReviewList() { + assertTrue(checker.canManageNamespaceReviews( + 10L, NamespaceType.TEAM, Map.of(10L, NamespaceRole.ADMIN), Set.of())); + } + + @Test + void submitterCanReadOwnReview() { + ReviewTask task = new ReviewTask(1L, 10L, "user-1"); + assertTrue(checker.canReadReview(task, "user-1", + NamespaceType.TEAM, Map.of(), Set.of())); + } + + @Test + void ownerCanSubmitPromotion() { + Skill sourceSkill = new Skill(10L, "skill-a", "user-1", SkillVisibility.PUBLIC); + assertTrue(checker.canSubmitPromotion(sourceSkill, "user-1", Map.of())); + } + + @Test + void teamAdminCanSubmitPromotionForForeignSkill() { + Skill sourceSkill = new Skill(10L, "skill-a", "user-2", SkillVisibility.PUBLIC); + assertTrue(checker.canSubmitPromotion(sourceSkill, "user-1", + Map.of(10L, NamespaceRole.ADMIN))); + } + + @Test + void submitterCanReadOwnPromotion() { + PromotionRequest req = new PromotionRequest(1L, 1L, 1L, "user-1"); + assertTrue(checker.canReadPromotion(req, "user-1", Set.of())); + } + + @Test + void skillAdminCanListPendingPromotions() { + assertTrue(checker.canListPendingPromotions(Set.of("SKILL_ADMIN"))); + } + @Test void skillAdminCanReviewPromotion() { PromotionRequest req = new PromotionRequest(1L, 1L, 1L, "user-2"); diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/review/ReviewServiceTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/review/ReviewServiceTest.java index 297a7701..c9325329 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/review/ReviewServiceTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/review/ReviewServiceTest.java @@ -104,11 +104,20 @@ class ReviewServiceTest { @Test void shouldSubmitReviewSuccessfully() { SkillVersion sv = createDraftSkillVersion(); + Skill skill = createSkill(); when(skillVersionRepository.findById(SKILL_VERSION_ID)).thenReturn(Optional.of(sv)); + when(skillRepository.findById(SKILL_ID)).thenReturn(Optional.of(skill)); + when(permissionChecker.canSubmitReview( + NAMESPACE_ID, + Map.of(NAMESPACE_ID, NamespaceRole.MEMBER))).thenReturn(true); ReviewTask savedTask = createPendingReviewTask(); when(reviewTaskRepository.save(any(ReviewTask.class))).thenReturn(savedTask); - ReviewTask result = reviewService.submitReview(SKILL_VERSION_ID, NAMESPACE_ID, USER_ID); + ReviewTask result = reviewService.submitReview( + SKILL_VERSION_ID, + USER_ID, + Map.of(NAMESPACE_ID, NamespaceRole.MEMBER) + ); assertNotNull(result); assertEquals(SkillVersionStatus.PENDING_REVIEW, sv.getStatus()); @@ -121,27 +130,50 @@ class ReviewServiceTest { when(skillVersionRepository.findById(SKILL_VERSION_ID)).thenReturn(Optional.empty()); assertThrows(DomainNotFoundException.class, - () -> reviewService.submitReview(SKILL_VERSION_ID, NAMESPACE_ID, USER_ID)); + () -> reviewService.submitReview(SKILL_VERSION_ID, USER_ID, Map.of())); } @Test void shouldThrowWhenStatusNotDraft() { SkillVersion sv = createPendingReviewSkillVersion(); when(skillVersionRepository.findById(SKILL_VERSION_ID)).thenReturn(Optional.of(sv)); + when(skillRepository.findById(SKILL_ID)).thenReturn(Optional.of(createSkill())); assertThrows(DomainBadRequestException.class, - () -> reviewService.submitReview(SKILL_VERSION_ID, NAMESPACE_ID, USER_ID)); + () -> reviewService.submitReview(SKILL_VERSION_ID, USER_ID, Map.of(NAMESPACE_ID, NamespaceRole.MEMBER))); } @Test void shouldThrowOnDuplicateSubmission() { SkillVersion sv = createDraftSkillVersion(); + Skill skill = createSkill(); when(skillVersionRepository.findById(SKILL_VERSION_ID)).thenReturn(Optional.of(sv)); + when(skillRepository.findById(SKILL_ID)).thenReturn(Optional.of(skill)); + when(permissionChecker.canSubmitReview( + NAMESPACE_ID, + Map.of(NAMESPACE_ID, NamespaceRole.MEMBER))).thenReturn(true); when(reviewTaskRepository.save(any(ReviewTask.class))) .thenThrow(new DataIntegrityViolationException("duplicate")); assertThrows(DomainBadRequestException.class, - () -> reviewService.submitReview(SKILL_VERSION_ID, NAMESPACE_ID, USER_ID)); + () -> reviewService.submitReview( + SKILL_VERSION_ID, + USER_ID, + Map.of(NAMESPACE_ID, NamespaceRole.MEMBER) + )); + } + + @Test + void shouldThrowWhenSubmitterLacksNamespaceMembership() { + SkillVersion sv = createDraftSkillVersion(); + Skill skill = createSkill(); + when(skillVersionRepository.findById(SKILL_VERSION_ID)).thenReturn(Optional.of(sv)); + when(skillRepository.findById(SKILL_ID)).thenReturn(Optional.of(skill)); + when(permissionChecker.canSubmitReview(NAMESPACE_ID, Map.of())).thenReturn(false); + + assertThrows(DomainForbiddenException.class, + () -> reviewService.submitReview(SKILL_VERSION_ID, USER_ID, Map.of())); + verify(reviewTaskRepository, never()).save(any(ReviewTask.class)); } } From 7bc152a744e2b049bf14bf7424b0029d641249b1 Mon Sep 17 00:00:00 2001 From: yun-zhi-ztl <15071461069@163.com> Date: Fri, 13 Mar 2026 10:01:01 +0800 Subject: [PATCH 04/13] fix(publish): keep latest pointer aligned with published versions - stop publish submission from advancing skill.latestVersionId or overwriting published-facing skill metadata while a version is still pending review\n- move latest pointer and display metadata promotion into review approval so the public skill record changes only when a version becomes PUBLISHED\n- keep SkillPublishedEvent emission on review approval only, preserving search rebuild semantics for published versions\n- add regression coverage for pending review submissions retaining published metadata and for approval promoting latest pointer plus display fields --- .../skillhub/domain/review/ReviewService.java | 24 ++++- .../skill/service/SkillPublishService.java | 14 +-- .../domain/review/ReviewServiceTest.java | 15 ++- .../service/SkillPublishServiceTest.java | 94 ++++++++++++++++++- 4 files changed, 128 insertions(+), 19 deletions(-) diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/ReviewService.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/ReviewService.java index 8e2f6308..205a2054 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/ReviewService.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/ReviewService.java @@ -1,5 +1,6 @@ package com.iflytek.skillhub.domain.review; +import com.fasterxml.jackson.databind.ObjectMapper; import com.iflytek.skillhub.domain.namespace.Namespace; import com.iflytek.skillhub.domain.namespace.NamespaceRepository; import com.iflytek.skillhub.domain.namespace.NamespaceRole; @@ -12,6 +13,7 @@ import com.iflytek.skillhub.domain.skill.SkillRepository; import com.iflytek.skillhub.domain.skill.SkillVersion; import com.iflytek.skillhub.domain.skill.SkillVersionRepository; import com.iflytek.skillhub.domain.skill.SkillVersionStatus; +import com.iflytek.skillhub.domain.skill.metadata.SkillMetadata; import org.springframework.context.ApplicationEventPublisher; import org.springframework.dao.DataIntegrityViolationException; import org.springframework.stereotype.Service; @@ -31,19 +33,22 @@ public class ReviewService { private final NamespaceRepository namespaceRepository; private final ReviewPermissionChecker permissionChecker; private final ApplicationEventPublisher eventPublisher; + private final ObjectMapper objectMapper; public ReviewService(ReviewTaskRepository reviewTaskRepository, SkillVersionRepository skillVersionRepository, SkillRepository skillRepository, NamespaceRepository namespaceRepository, ReviewPermissionChecker permissionChecker, - ApplicationEventPublisher eventPublisher) { + ApplicationEventPublisher eventPublisher, + ObjectMapper objectMapper) { this.reviewTaskRepository = reviewTaskRepository; this.skillVersionRepository = skillVersionRepository; this.skillRepository = skillRepository; this.namespaceRepository = namespaceRepository; this.permissionChecker = permissionChecker; this.eventPublisher = eventPublisher; + this.objectMapper = objectMapper; } @Transactional @@ -109,6 +114,8 @@ public class ReviewService { Skill skill = skillRepository.findById(skillVersion.getSkillId()) .orElseThrow(() -> new DomainNotFoundException("skill.not_found", skillVersion.getSkillId())); skill.setLatestVersionId(skillVersion.getId()); + applyPublishedMetadata(skill, skillVersion); + skill.setUpdatedBy(reviewerId); skillRepository.save(skill); eventPublisher.publishEvent(new SkillPublishedEvent( @@ -168,4 +175,19 @@ public class ReviewService { skillVersion.setStatus(SkillVersionStatus.DRAFT); skillVersionRepository.save(skillVersion); } + + private void applyPublishedMetadata(Skill skill, SkillVersion skillVersion) { + String metadataJson = skillVersion.getParsedMetadataJson(); + if (metadataJson == null || metadataJson.isBlank()) { + return; + } + + try { + SkillMetadata metadata = objectMapper.readValue(metadataJson, SkillMetadata.class); + skill.setDisplayName(metadata.name()); + skill.setSummary(metadata.description()); + } catch (Exception e) { + throw new IllegalStateException("Failed to deserialize skill metadata", e); + } + } } diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java index 2d6fa0bb..ec693df4 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java @@ -1,7 +1,6 @@ package com.iflytek.skillhub.domain.skill.service; import com.fasterxml.jackson.databind.ObjectMapper; -import com.iflytek.skillhub.domain.event.SkillPublishedEvent; import com.iflytek.skillhub.domain.namespace.Namespace; import com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository; import com.iflytek.skillhub.domain.namespace.NamespaceRepository; @@ -214,17 +213,8 @@ public class SkillPublishService { version.setTotalSize(totalSize); skillVersionRepository.save(version); - // 12. Update skill - skill.setLatestVersionId(version.getId()); - skill.setDisplayName(metadata.name()); - skill.setSummary(metadata.description()); - skill.setUpdatedBy(publisherId); - skillRepository.save(skill); - - // 13. Publish SkillPublishedEvent - eventPublisher.publishEvent(new SkillPublishedEvent(skill.getId(), version.getId(), publisherId)); - - // 14. Return published identifiers + // 12. Return published identifiers. Published-facing skill metadata is + // advanced only when review approval promotes this version to PUBLISHED. return new PublishResult(skill.getId(), skill.getSlug(), version); } diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/review/ReviewServiceTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/review/ReviewServiceTest.java index c9325329..fd6815ae 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/review/ReviewServiceTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/review/ReviewServiceTest.java @@ -1,5 +1,6 @@ package com.iflytek.skillhub.domain.review; +import com.fasterxml.jackson.databind.ObjectMapper; import com.iflytek.skillhub.domain.event.SkillPublishedEvent; import com.iflytek.skillhub.domain.namespace.Namespace; import com.iflytek.skillhub.domain.namespace.NamespaceRepository; @@ -13,6 +14,7 @@ import com.iflytek.skillhub.domain.skill.SkillVersion; import com.iflytek.skillhub.domain.skill.SkillVersionRepository; import com.iflytek.skillhub.domain.skill.SkillVersionStatus; import com.iflytek.skillhub.domain.skill.SkillVisibility; +import com.iflytek.skillhub.domain.skill.metadata.SkillMetadata; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -50,12 +52,14 @@ class ReviewServiceTest { private static final String REVIEWER_ID = "user-200"; private static final Long REVIEW_TASK_ID = 1L; private static final Long SKILL_ID = 30L; + private ObjectMapper objectMapper; @BeforeEach void setUp() { + objectMapper = new ObjectMapper(); reviewService = new ReviewService( reviewTaskRepository, skillVersionRepository, skillRepository, - namespaceRepository, permissionChecker, eventPublisher); + namespaceRepository, permissionChecker, eventPublisher, objectMapper); } private SkillVersion createDraftSkillVersion() { @@ -186,6 +190,12 @@ class ReviewServiceTest { Namespace ns = createTeamNamespace(); SkillVersion sv = createPendingReviewSkillVersion(); Skill skill = createSkill(); + skill.setDisplayName("Published Name"); + skill.setSummary("Published Summary"); + skill.setUpdatedBy("previous-reviewer"); + assertDoesNotThrow(() -> sv.setParsedMetadataJson(objectMapper.writeValueAsString( + new SkillMetadata("Approved Name", "Approved Summary", "1.0.0", "Body", Map.of()) + ))); when(reviewTaskRepository.findById(REVIEW_TASK_ID)).thenReturn(Optional.of(task)); when(namespaceRepository.findById(NAMESPACE_ID)).thenReturn(Optional.of(ns)); @@ -206,6 +216,9 @@ class ReviewServiceTest { assertEquals(SkillVersionStatus.PUBLISHED, sv.getStatus()); assertNotNull(sv.getPublishedAt()); assertEquals(SKILL_VERSION_ID, skill.getLatestVersionId()); + assertEquals("Approved Name", skill.getDisplayName()); + assertEquals("Approved Summary", skill.getSummary()); + assertEquals(REVIEWER_ID, skill.getUpdatedBy()); verify(eventPublisher).publishEvent(any(SkillPublishedEvent.class)); } diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java index 416d441e..f0881441 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java @@ -1,7 +1,6 @@ package com.iflytek.skillhub.domain.skill.service; import com.fasterxml.jackson.databind.ObjectMapper; -import com.iflytek.skillhub.domain.event.SkillPublishedEvent; import com.iflytek.skillhub.domain.namespace.Namespace; import com.iflytek.skillhub.domain.namespace.NamespaceMember; import com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository; @@ -19,7 +18,6 @@ import com.iflytek.skillhub.storage.ObjectStorageService; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import org.springframework.context.ApplicationEventPublisher; @@ -101,6 +99,7 @@ class SkillPublishServiceTest { setId(skill, 1L); SkillVersion version = new SkillVersion(1L, "1.0.0", publisherId); setId(version, 10L); + version.setStatus(SkillVersionStatus.PENDING_REVIEW); when(namespaceRepository.findBySlug(namespaceSlug)).thenReturn(Optional.of(namespace)); when(namespaceMemberRepository.findByNamespaceIdAndUserId(any(), eq(publisherId))).thenReturn(Optional.of(member)); @@ -110,7 +109,6 @@ class SkillPublishServiceTest { when(skillRepository.findByNamespaceIdAndSlug(any(), eq("test-skill"))).thenReturn(Optional.of(skill)); when(skillVersionRepository.findBySkillIdAndVersion(any(), eq("1.0.0"))).thenReturn(Optional.empty()); when(skillVersionRepository.save(any())).thenReturn(version); - when(skillRepository.save(any())).thenReturn(skill); // Act SkillPublishService.PublishResult result = service.publishFromEntries( @@ -125,11 +123,98 @@ class SkillPublishServiceTest { assertEquals(1L, result.skillId()); assertEquals("test-skill", result.slug()); assertEquals("1.0.0", result.version().getVersion()); - verify(eventPublisher).publishEvent(any(SkillPublishedEvent.class)); + assertEquals(SkillVersionStatus.PENDING_REVIEW, result.version().getStatus()); + assertNull(skill.getLatestVersionId()); + verify(eventPublisher, never()).publishEvent(any()); verify(skillFileRepository).saveAll(anyList()); verify(objectStorageService, atLeastOnce()).putObject(anyString(), any(), anyLong(), anyString()); } + @Test + void testPublishFromEntries_ShouldKeepLatestVersionPointingToPublishedVersion() throws Exception { + String namespaceSlug = "test-ns"; + String publisherId = "user-100"; + String skillMdContent = "---\nname: test-skill\ndescription: Test\nversion: 1.1.0\n---\nBody"; + + PackageEntry skillMd = new PackageEntry("SKILL.md", skillMdContent.getBytes(), skillMdContent.length(), "text/markdown"); + List entries = List.of(skillMd); + + Namespace namespace = new Namespace(namespaceSlug, "Test NS", "user-1"); + setId(namespace, 1L); + NamespaceMember member = mock(NamespaceMember.class); + SkillMetadata metadata = new SkillMetadata("test-skill", "Test", "1.1.0", "Body", Map.of()); + + Skill skill = new Skill(1L, "test-skill", publisherId, SkillVisibility.PUBLIC); + setId(skill, 1L); + skill.setLatestVersionId(5L); + SkillVersion version = new SkillVersion(1L, "1.1.0", publisherId); + setId(version, 11L); + version.setStatus(SkillVersionStatus.PENDING_REVIEW); + + when(namespaceRepository.findBySlug(namespaceSlug)).thenReturn(Optional.of(namespace)); + when(namespaceMemberRepository.findByNamespaceIdAndUserId(any(), eq(publisherId))).thenReturn(Optional.of(member)); + when(skillPackageValidator.validate(entries)).thenReturn(ValidationResult.pass()); + when(skillMetadataParser.parse(skillMdContent)).thenReturn(metadata); + when(prePublishValidator.validate(any())).thenReturn(ValidationResult.pass()); + when(skillRepository.findByNamespaceIdAndSlug(any(), eq("test-skill"))).thenReturn(Optional.of(skill)); + when(skillVersionRepository.findBySkillIdAndVersion(any(), eq("1.1.0"))).thenReturn(Optional.empty()); + when(skillVersionRepository.save(any())).thenReturn(version); + + SkillPublishService.PublishResult result = service.publishFromEntries( + namespaceSlug, + entries, + publisherId, + SkillVisibility.PUBLIC + ); + + assertEquals(11L, result.version().getId()); + assertEquals(5L, skill.getLatestVersionId()); + verify(eventPublisher, never()).publishEvent(any()); + } + + @Test + void testPublishFromEntries_ShouldNotOverwritePublishedSkillMetadataBeforeApproval() throws Exception { + String namespaceSlug = "test-ns"; + String publisherId = "user-100"; + String skillMdContent = "---\nname: pending-name\ndescription: Pending Summary\nversion: 1.1.0\n---\nBody"; + + PackageEntry skillMd = new PackageEntry("SKILL.md", skillMdContent.getBytes(), skillMdContent.length(), "text/markdown"); + List entries = List.of(skillMd); + + Namespace namespace = new Namespace(namespaceSlug, "Test NS", "user-1"); + setId(namespace, 1L); + NamespaceMember member = mock(NamespaceMember.class); + SkillMetadata metadata = new SkillMetadata("pending-name", "Pending Summary", "1.1.0", "Body", Map.of()); + + Skill skill = new Skill(1L, "test-skill", publisherId, SkillVisibility.PUBLIC); + setId(skill, 1L); + skill.setDisplayName("Published Name"); + skill.setSummary("Published Summary"); + skill.setUpdatedBy("previous-reviewer"); + skill.setLatestVersionId(5L); + + SkillVersion version = new SkillVersion(1L, "1.1.0", publisherId); + setId(version, 11L); + version.setStatus(SkillVersionStatus.PENDING_REVIEW); + + when(namespaceRepository.findBySlug(namespaceSlug)).thenReturn(Optional.of(namespace)); + when(namespaceMemberRepository.findByNamespaceIdAndUserId(any(), eq(publisherId))).thenReturn(Optional.of(member)); + when(skillPackageValidator.validate(entries)).thenReturn(ValidationResult.pass()); + when(skillMetadataParser.parse(skillMdContent)).thenReturn(metadata); + when(prePublishValidator.validate(any())).thenReturn(ValidationResult.pass()); + when(skillRepository.findByNamespaceIdAndSlug(any(), eq("pending-name"))).thenReturn(Optional.of(skill)); + when(skillVersionRepository.findBySkillIdAndVersion(any(), eq("1.1.0"))).thenReturn(Optional.empty()); + when(skillVersionRepository.save(any())).thenReturn(version); + + service.publishFromEntries(namespaceSlug, entries, publisherId, SkillVisibility.PUBLIC); + + assertEquals("Published Name", skill.getDisplayName()); + assertEquals("Published Summary", skill.getSummary()); + assertEquals("previous-reviewer", skill.getUpdatedBy()); + assertEquals(5L, skill.getLatestVersionId()); + verify(skillRepository, never()).save(skill); + } + @Test void testPublishFromEntries_ShouldSlugifyNameBeforeLookupAndResponse() throws Exception { String namespaceSlug = "test-ns"; @@ -157,7 +242,6 @@ class SkillPublishServiceTest { when(skillRepository.findByNamespaceIdAndSlug(any(), eq("smoke-skill-two"))).thenReturn(Optional.of(skill)); when(skillVersionRepository.findBySkillIdAndVersion(any(), eq("0.2.0"))).thenReturn(Optional.empty()); when(skillVersionRepository.save(any())).thenReturn(version); - when(skillRepository.save(any())).thenReturn(skill); SkillPublishService.PublishResult result = service.publishFromEntries( namespaceSlug, From ddbf92435fe40d4b225c0669d0ea1efefe2d6605 Mon Sep 17 00:00:00 2001 From: yun-zhi-ztl <15071461069@163.com> Date: Fri, 13 Mar 2026 10:20:34 +0800 Subject: [PATCH 05/13] fix(upload): harden package extraction and storage boundaries - add shared package safety policy for path normalization and size limits - stream zip extraction for cli check and publish flows to reject traversal and oversized entries - confine local storage keys to the configured base path and add regression coverage --- .../skillhub/controller/CliController.java | 53 ++++------- .../controller/cli/CliPublishController.java | 46 +++------- .../portal/SkillPublishController.java | 46 +++------- .../support/SkillPackageArchiveExtractor.java | 92 +++++++++++++++++++ .../controller/CliControllerTest.java | 30 ++++++ .../SkillPackageArchiveExtractorTest.java | 61 ++++++++++++ .../skill/validation/SkillPackagePolicy.java | 56 +++++++++++ .../validation/SkillPackageValidator.java | 66 ++++++------- .../validation/SkillPackageValidatorTest.java | 45 +++++++++ .../storage/LocalFileStorageService.java | 10 +- .../storage/LocalFileStorageServiceTest.java | 16 ++++ 11 files changed, 380 insertions(+), 141 deletions(-) create mode 100644 server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/support/SkillPackageArchiveExtractor.java create mode 100644 server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/support/SkillPackageArchiveExtractorTest.java create mode 100644 server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/SkillPackagePolicy.java diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/CliController.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/CliController.java index cf0273ec..5363eab7 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/CliController.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/CliController.java @@ -1,5 +1,6 @@ package com.iflytek.skillhub.controller; +import com.iflytek.skillhub.controller.support.SkillPackageArchiveExtractor; import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; import com.iflytek.skillhub.domain.skill.validation.PackageEntry; import com.iflytek.skillhub.domain.skill.validation.SkillPackageValidator; @@ -14,21 +15,21 @@ import org.springframework.web.bind.annotation.*; import org.springframework.web.multipart.MultipartFile; import java.io.IOException; -import java.util.ArrayList; import java.util.List; -import java.util.zip.ZipEntry; -import java.util.zip.ZipInputStream; @RestController @RequestMapping("/api/v1/cli") public class CliController extends BaseApiController { private final SkillPackageValidator skillPackageValidator; + private final SkillPackageArchiveExtractor skillPackageArchiveExtractor; public CliController(ApiResponseFactory responseFactory, - SkillPackageValidator skillPackageValidator) { + SkillPackageValidator skillPackageValidator, + SkillPackageArchiveExtractor skillPackageArchiveExtractor) { super(responseFactory); this.skillPackageValidator = skillPackageValidator; + this.skillPackageArchiveExtractor = skillPackageArchiveExtractor; } @GetMapping("/whoami") @@ -42,7 +43,18 @@ public class CliController extends BaseApiController { @PostMapping("/check") public ApiResponse check(@RequestParam("file") MultipartFile file) throws IOException { - List entries = extractZipEntries(file); + List entries; + try { + entries = skillPackageArchiveExtractor.extract(file); + } catch (IllegalArgumentException e) { + SkillCheckResponse response = new SkillCheckResponse( + false, + List.of(e.getMessage()), + 0, + 0L + ); + return ok("response.success.validated", response); + } ValidationResult result = skillPackageValidator.validate(entries); SkillCheckResponse response = new SkillCheckResponse( @@ -54,35 +66,4 @@ public class CliController extends BaseApiController { return ok("response.success.validated", response); } - - private List extractZipEntries(MultipartFile file) throws IOException { - List entries = new ArrayList<>(); - - try (ZipInputStream zis = new ZipInputStream(file.getInputStream())) { - ZipEntry zipEntry; - while ((zipEntry = zis.getNextEntry()) != null) { - if (!zipEntry.isDirectory()) { - byte[] content = zis.readAllBytes(); - entries.add(new PackageEntry( - zipEntry.getName(), - content, - content.length, - determineContentType(zipEntry.getName()) - )); - } - zis.closeEntry(); - } - } - - return entries; - } - - private String determineContentType(String filename) { - if (filename.endsWith(".py")) return "text/x-python"; - if (filename.endsWith(".json")) return "application/json"; - if (filename.endsWith(".yaml") || filename.endsWith(".yml")) return "application/x-yaml"; - if (filename.endsWith(".txt")) return "text/plain"; - if (filename.endsWith(".md")) return "text/markdown"; - return "application/octet-stream"; - } } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/cli/CliPublishController.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/cli/CliPublishController.java index 76bae215..953ee4f8 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/cli/CliPublishController.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/cli/CliPublishController.java @@ -1,6 +1,8 @@ package com.iflytek.skillhub.controller.cli; import com.iflytek.skillhub.controller.BaseApiController; +import com.iflytek.skillhub.controller.support.SkillPackageArchiveExtractor; +import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException; import com.iflytek.skillhub.domain.skill.SkillVisibility; import com.iflytek.skillhub.domain.skill.service.SkillPublishService; import com.iflytek.skillhub.domain.skill.validation.PackageEntry; @@ -12,21 +14,21 @@ import org.springframework.web.bind.annotation.*; import org.springframework.web.multipart.MultipartFile; import java.io.IOException; -import java.util.ArrayList; import java.util.List; -import java.util.zip.ZipEntry; -import java.util.zip.ZipInputStream; @RestController @RequestMapping("/api/v1/cli") public class CliPublishController extends BaseApiController { private final SkillPublishService skillPublishService; + private final SkillPackageArchiveExtractor skillPackageArchiveExtractor; public CliPublishController(SkillPublishService skillPublishService, + SkillPackageArchiveExtractor skillPackageArchiveExtractor, ApiResponseFactory responseFactory) { super(responseFactory); this.skillPublishService = skillPublishService; + this.skillPackageArchiveExtractor = skillPackageArchiveExtractor; } @PostMapping("/publish") @@ -39,7 +41,12 @@ public class CliPublishController extends BaseApiController { SkillVisibility skillVisibility = SkillVisibility.valueOf(visibility.toUpperCase()); - List entries = extractZipEntries(file); + List entries; + try { + entries = skillPackageArchiveExtractor.extract(file); + } catch (IllegalArgumentException e) { + throw new DomainBadRequestException("error.skill.publish.package.invalid", e.getMessage()); + } SkillPublishService.PublishResult publishResult = skillPublishService.publishFromEntries( namespace, @@ -60,35 +67,4 @@ public class CliPublishController extends BaseApiController { return ok("response.success.published", response); } - - private List extractZipEntries(MultipartFile file) throws IOException { - List entries = new ArrayList<>(); - - try (ZipInputStream zis = new ZipInputStream(file.getInputStream())) { - ZipEntry zipEntry; - while ((zipEntry = zis.getNextEntry()) != null) { - if (!zipEntry.isDirectory()) { - byte[] content = zis.readAllBytes(); - entries.add(new PackageEntry( - zipEntry.getName(), - content, - content.length, - determineContentType(zipEntry.getName()) - )); - } - zis.closeEntry(); - } - } - - return entries; - } - - private String determineContentType(String filename) { - if (filename.endsWith(".py")) return "text/x-python"; - if (filename.endsWith(".json")) return "application/json"; - if (filename.endsWith(".yaml") || filename.endsWith(".yml")) return "application/x-yaml"; - if (filename.endsWith(".txt")) return "text/plain"; - if (filename.endsWith(".md")) return "text/markdown"; - return "application/octet-stream"; - } } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillPublishController.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillPublishController.java index e984e31e..7d96a749 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillPublishController.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillPublishController.java @@ -1,6 +1,8 @@ package com.iflytek.skillhub.controller.portal; import com.iflytek.skillhub.controller.BaseApiController; +import com.iflytek.skillhub.controller.support.SkillPackageArchiveExtractor; +import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException; import com.iflytek.skillhub.domain.skill.SkillVisibility; import com.iflytek.skillhub.domain.skill.service.SkillPublishService; import com.iflytek.skillhub.domain.skill.validation.PackageEntry; @@ -12,21 +14,21 @@ import org.springframework.web.bind.annotation.*; import org.springframework.web.multipart.MultipartFile; import java.io.IOException; -import java.util.ArrayList; import java.util.List; -import java.util.zip.ZipEntry; -import java.util.zip.ZipInputStream; @RestController @RequestMapping("/api/v1/skills") public class SkillPublishController extends BaseApiController { private final SkillPublishService skillPublishService; + private final SkillPackageArchiveExtractor skillPackageArchiveExtractor; public SkillPublishController(SkillPublishService skillPublishService, + SkillPackageArchiveExtractor skillPackageArchiveExtractor, ApiResponseFactory responseFactory) { super(responseFactory); this.skillPublishService = skillPublishService; + this.skillPackageArchiveExtractor = skillPackageArchiveExtractor; } @PostMapping("/{namespace}/publish") @@ -39,7 +41,12 @@ public class SkillPublishController extends BaseApiController { SkillVisibility skillVisibility = SkillVisibility.valueOf(visibility.toUpperCase()); - List entries = extractZipEntries(file); + List entries; + try { + entries = skillPackageArchiveExtractor.extract(file); + } catch (IllegalArgumentException e) { + throw new DomainBadRequestException("error.skill.publish.package.invalid", e.getMessage()); + } SkillPublishService.PublishResult publishResult = skillPublishService.publishFromEntries( namespace, @@ -60,35 +67,4 @@ public class SkillPublishController extends BaseApiController { return ok("response.success.published", response); } - - private List extractZipEntries(MultipartFile file) throws IOException { - List entries = new ArrayList<>(); - - try (ZipInputStream zis = new ZipInputStream(file.getInputStream())) { - ZipEntry zipEntry; - while ((zipEntry = zis.getNextEntry()) != null) { - if (!zipEntry.isDirectory()) { - byte[] content = zis.readAllBytes(); - entries.add(new PackageEntry( - zipEntry.getName(), - content, - content.length, - determineContentType(zipEntry.getName()) - )); - } - zis.closeEntry(); - } - } - - return entries; - } - - private String determineContentType(String filename) { - if (filename.endsWith(".py")) return "text/x-python"; - if (filename.endsWith(".json")) return "application/json"; - if (filename.endsWith(".yaml") || filename.endsWith(".yml")) return "application/x-yaml"; - if (filename.endsWith(".txt")) return "text/plain"; - if (filename.endsWith(".md")) return "text/markdown"; - return "application/octet-stream"; - } } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/support/SkillPackageArchiveExtractor.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/support/SkillPackageArchiveExtractor.java new file mode 100644 index 00000000..b9193b8e --- /dev/null +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/support/SkillPackageArchiveExtractor.java @@ -0,0 +1,92 @@ +package com.iflytek.skillhub.controller.support; + +import com.iflytek.skillhub.domain.skill.validation.PackageEntry; +import com.iflytek.skillhub.domain.skill.validation.SkillPackagePolicy; +import org.springframework.stereotype.Component; +import org.springframework.web.multipart.MultipartFile; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.zip.ZipEntry; +import java.util.zip.ZipInputStream; + +@Component +public class SkillPackageArchiveExtractor { + + public List extract(MultipartFile file) throws IOException { + if (file.getSize() > SkillPackagePolicy.MAX_TOTAL_PACKAGE_SIZE) { + throw new IllegalArgumentException( + "Package too large: " + file.getSize() + " bytes (max: " + + SkillPackagePolicy.MAX_TOTAL_PACKAGE_SIZE + ")" + ); + } + + List entries = new ArrayList<>(); + long totalSize = 0; + + try (ZipInputStream zis = new ZipInputStream(file.getInputStream())) { + ZipEntry zipEntry; + while ((zipEntry = zis.getNextEntry()) != null) { + if (zipEntry.isDirectory()) { + zis.closeEntry(); + continue; + } + + if (entries.size() >= SkillPackagePolicy.MAX_FILE_COUNT) { + throw new IllegalArgumentException( + "Too many files: more than " + SkillPackagePolicy.MAX_FILE_COUNT + ); + } + + String normalizedPath = SkillPackagePolicy.normalizeEntryPath(zipEntry.getName()); + byte[] content = readEntry(zis, normalizedPath); + totalSize += content.length; + if (totalSize > SkillPackagePolicy.MAX_TOTAL_PACKAGE_SIZE) { + throw new IllegalArgumentException( + "Package too large: " + totalSize + " bytes (max: " + + SkillPackagePolicy.MAX_TOTAL_PACKAGE_SIZE + ")" + ); + } + + entries.add(new PackageEntry( + normalizedPath, + content, + content.length, + determineContentType(normalizedPath) + )); + zis.closeEntry(); + } + } + + return entries; + } + + private byte[] readEntry(ZipInputStream zis, String path) throws IOException { + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + byte[] buffer = new byte[8192]; + long totalRead = 0; + int read; + while ((read = zis.read(buffer)) != -1) { + totalRead += read; + if (totalRead > SkillPackagePolicy.MAX_SINGLE_FILE_SIZE) { + throw new IllegalArgumentException( + "File too large: " + path + " (" + totalRead + " bytes, max: " + + SkillPackagePolicy.MAX_SINGLE_FILE_SIZE + ")" + ); + } + outputStream.write(buffer, 0, read); + } + return outputStream.toByteArray(); + } + + private String determineContentType(String filename) { + if (filename.endsWith(".py")) return "text/x-python"; + if (filename.endsWith(".json")) return "application/json"; + if (filename.endsWith(".yaml") || filename.endsWith(".yml")) return "application/x-yaml"; + if (filename.endsWith(".txt")) return "text/plain"; + if (filename.endsWith(".md")) return "text/markdown"; + return "application/octet-stream"; + } +} diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/CliControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/CliControllerTest.java index 13a781b2..58d721ba 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/CliControllerTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/CliControllerTest.java @@ -132,6 +132,25 @@ class CliControllerTest { .andExpect(jsonPath("$.data.errors").isNotEmpty()); } + @Test + void checkShouldReturnInvalidForPathTraversalEntry() throws Exception { + byte[] zipBytes = createZipWithUnsafePath(); + MockMultipartFile file = new MockMultipartFile( + "file", + "skill.zip", + "application/zip", + zipBytes + ); + + mockMvc.perform(multipart("/api/v1/cli/check").file(file)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.code").value(0)) + .andExpect(jsonPath("$.data.valid").value(false)) + .andExpect(jsonPath("$.data.errors[0]").value(org.hamcrest.Matchers.containsString("escapes package root"))) + .andExpect(jsonPath("$.data.fileCount").value(0)) + .andExpect(jsonPath("$.data.totalSize").value(0)); + } + private byte[] createValidSkillZip() throws Exception { ByteArrayOutputStream baos = new ByteArrayOutputStream(); try (ZipOutputStream zos = new ZipOutputStream(baos)) { @@ -191,4 +210,15 @@ class CliControllerTest { } return baos.toByteArray(); } + + private byte[] createZipWithUnsafePath() throws Exception { + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + try (ZipOutputStream zos = new ZipOutputStream(baos)) { + ZipEntry unsafeEntry = new ZipEntry("../secrets.txt"); + zos.putNextEntry(unsafeEntry); + zos.write("hidden".getBytes()); + zos.closeEntry(); + } + return baos.toByteArray(); + } } diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/support/SkillPackageArchiveExtractorTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/support/SkillPackageArchiveExtractorTest.java new file mode 100644 index 00000000..d5464a60 --- /dev/null +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/support/SkillPackageArchiveExtractorTest.java @@ -0,0 +1,61 @@ +package com.iflytek.skillhub.controller.support; + +import org.junit.jupiter.api.Test; +import org.springframework.mock.web.MockMultipartFile; + +import java.io.ByteArrayOutputStream; +import java.nio.charset.StandardCharsets; +import java.util.zip.ZipEntry; +import java.util.zip.ZipOutputStream; + +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class SkillPackageArchiveExtractorTest { + + private final SkillPackageArchiveExtractor extractor = new SkillPackageArchiveExtractor(); + + @Test + void shouldRejectPathTraversalEntry() throws Exception { + MockMultipartFile file = new MockMultipartFile( + "file", + "skill.zip", + "application/zip", + createZip("../secrets.txt", "hidden") + ); + + IllegalArgumentException error = assertThrows(IllegalArgumentException.class, () -> extractor.extract(file)); + + assertTrue(error.getMessage().contains("escapes package root")); + } + + @Test + void shouldRejectOversizedZipEntry() throws Exception { + byte[] content = new byte[1024 * 1024 + 1]; + MockMultipartFile file = new MockMultipartFile( + "file", + "skill.zip", + "application/zip", + createZip("large.txt", content) + ); + + IllegalArgumentException error = assertThrows(IllegalArgumentException.class, () -> extractor.extract(file)); + + assertTrue(error.getMessage().contains("File too large: large.txt")); + } + + private byte[] createZip(String entryName, String content) throws Exception { + return createZip(entryName, content.getBytes(StandardCharsets.UTF_8)); + } + + private byte[] createZip(String entryName, byte[] content) throws Exception { + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + try (ZipOutputStream zos = new ZipOutputStream(baos)) { + ZipEntry entry = new ZipEntry(entryName); + zos.putNextEntry(entry); + zos.write(content); + zos.closeEntry(); + } + return baos.toByteArray(); + } +} diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/SkillPackagePolicy.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/SkillPackagePolicy.java new file mode 100644 index 00000000..3c0e5498 --- /dev/null +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/SkillPackagePolicy.java @@ -0,0 +1,56 @@ +package com.iflytek.skillhub.domain.skill.validation; + +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Set; + +public final class SkillPackagePolicy { + + public static final int MAX_FILE_COUNT = 100; + public static final long MAX_SINGLE_FILE_SIZE = 1024 * 1024; // 1MB + public static final long MAX_TOTAL_PACKAGE_SIZE = 10 * 1024 * 1024; // 10MB + public static final String SKILL_MD_PATH = "SKILL.md"; + public static final Set ALLOWED_EXTENSIONS = Set.of( + ".md", ".txt", ".json", ".yaml", ".yml", + ".js", ".ts", ".py", ".sh", + ".png", ".jpg", ".svg" + ); + + private SkillPackagePolicy() { + } + + public static String normalizeEntryPath(String rawPath) { + if (rawPath == null) { + throw new IllegalArgumentException("Package entry path is missing"); + } + + String sanitized = rawPath.replace('\\', '/').trim(); + if (sanitized.isEmpty()) { + throw new IllegalArgumentException("Package entry path is empty"); + } + if (sanitized.startsWith("/") || sanitized.startsWith("\\")) { + throw new IllegalArgumentException("Package entry path must be relative: " + rawPath); + } + if (sanitized.contains(":")) { + throw new IllegalArgumentException("Package entry path contains an invalid drive or scheme prefix: " + rawPath); + } + + Path normalized = Paths.get(sanitized).normalize(); + String canonical = normalized.toString().replace('\\', '/'); + if (normalized.isAbsolute() || canonical.isBlank()) { + throw new IllegalArgumentException("Package entry path is invalid: " + rawPath); + } + if (canonical.equals(".") || canonical.equals("..") || canonical.startsWith("../")) { + throw new IllegalArgumentException("Package entry path escapes package root: " + rawPath); + } + if (!sanitized.equals(canonical)) { + throw new IllegalArgumentException("Package entry path must be normalized: " + rawPath); + } + + return canonical; + } + + public static boolean hasAllowedExtension(String path) { + return ALLOWED_EXTENSIONS.stream().anyMatch(path::endsWith); + } +} diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/SkillPackageValidator.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/SkillPackageValidator.java index 57977dc6..950417fc 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/SkillPackageValidator.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/SkillPackageValidator.java @@ -4,21 +4,12 @@ import com.iflytek.skillhub.domain.shared.exception.LocalizedDomainException; import com.iflytek.skillhub.domain.skill.metadata.SkillMetadataParser; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Set; public class SkillPackageValidator { - private static final int MAX_FILE_COUNT = 100; - private static final long MAX_SINGLE_FILE_SIZE = 1024 * 1024; // 1MB - private static final long MAX_TOTAL_PACKAGE_SIZE = 10 * 1024 * 1024; // 10MB - private static final String SKILL_MD_PATH = "SKILL.md"; - private static final Set ALLOWED_EXTENSIONS = Set.of( - ".md", ".txt", ".json", ".yaml", ".yml", - ".js", ".ts", ".py", ".sh", - ".png", ".jpg", ".svg" - ); - private final SkillMetadataParser metadataParser; public SkillPackageValidator(SkillMetadataParser metadataParser) { @@ -27,13 +18,32 @@ public class SkillPackageValidator { public ValidationResult validate(List entries) { List errors = new ArrayList<>(); + Set normalizedPaths = new HashSet<>(); + PackageEntry skillMd = null; + + for (PackageEntry entry : entries) { + String normalizedPath; + try { + normalizedPath = SkillPackagePolicy.normalizeEntryPath(entry.path()); + } catch (IllegalArgumentException e) { + errors.add(e.getMessage()); + continue; + } + + if (!normalizedPaths.add(normalizedPath)) { + errors.add("Duplicate package entry path: " + normalizedPath); + } + + if (!SkillPackagePolicy.hasAllowedExtension(normalizedPath)) { + errors.add("Disallowed file extension: " + normalizedPath); + } + + if (SkillPackagePolicy.SKILL_MD_PATH.equals(normalizedPath) && skillMd == null) { + skillMd = entry; + } + } // 1. Check SKILL.md exists at root - PackageEntry skillMd = entries.stream() - .filter(e -> e.path().equals(SKILL_MD_PATH)) - .findFirst() - .orElse(null); - if (skillMd == null) { errors.add("Missing required file: SKILL.md at root"); return ValidationResult.fail(errors); @@ -51,31 +61,21 @@ public class SkillPackageValidator { } // 3. Check file count - if (entries.size() > MAX_FILE_COUNT) { - errors.add("Too many files: " + entries.size() + " (max: " + MAX_FILE_COUNT + ")"); + if (entries.size() > SkillPackagePolicy.MAX_FILE_COUNT) { + errors.add("Too many files: " + entries.size() + " (max: " + SkillPackagePolicy.MAX_FILE_COUNT + ")"); } - // 4. Check file extensions + // 4. Check single file size for (PackageEntry entry : entries) { - String path = entry.path(); - boolean hasAllowedExtension = ALLOWED_EXTENSIONS.stream() - .anyMatch(path::endsWith); - if (!hasAllowedExtension) { - errors.add("Disallowed file extension: " + path); + if (entry.size() > SkillPackagePolicy.MAX_SINGLE_FILE_SIZE) { + errors.add("File too large: " + entry.path() + " (" + entry.size() + " bytes, max: " + SkillPackagePolicy.MAX_SINGLE_FILE_SIZE + ")"); } } - // 5. Check single file size - for (PackageEntry entry : entries) { - if (entry.size() > MAX_SINGLE_FILE_SIZE) { - errors.add("File too large: " + entry.path() + " (" + entry.size() + " bytes, max: " + MAX_SINGLE_FILE_SIZE + ")"); - } - } - - // 6. Check total package size + // 5. Check total package size long totalSize = entries.stream().mapToLong(PackageEntry::size).sum(); - if (totalSize > MAX_TOTAL_PACKAGE_SIZE) { - errors.add("Package too large: " + totalSize + " bytes (max: " + MAX_TOTAL_PACKAGE_SIZE + ")"); + if (totalSize > SkillPackagePolicy.MAX_TOTAL_PACKAGE_SIZE) { + errors.add("Package too large: " + totalSize + " bytes (max: " + SkillPackagePolicy.MAX_TOTAL_PACKAGE_SIZE + ")"); } return errors.isEmpty() ? ValidationResult.pass() : ValidationResult.fail(errors); diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/validation/SkillPackageValidatorTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/validation/SkillPackageValidatorTest.java index bd0a6542..c36f4f42 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/validation/SkillPackageValidatorTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/validation/SkillPackageValidatorTest.java @@ -176,4 +176,49 @@ class SkillPackageValidatorTest { assertFalse(result.passed()); assertTrue(result.errors().stream().anyMatch(e -> e.contains("Package too large"))); } + + @Test + void testPathTraversalEntryRejected() { + String skillMdContent = """ + --- + name: test-skill + description: A test skill + version: 1.0.0 + --- + Body + """; + + List entries = List.of( + new PackageEntry("SKILL.md", skillMdContent.getBytes(), skillMdContent.length(), "text/markdown"), + new PackageEntry("../secrets.txt", "hidden".getBytes(), 6, "text/plain") + ); + + ValidationResult result = validator.validate(entries); + + assertFalse(result.passed()); + assertTrue(result.errors().stream().anyMatch(e -> e.contains("escapes package root"))); + } + + @Test + void testDuplicateNormalizedPathRejected() { + String skillMdContent = """ + --- + name: test-skill + description: A test skill + version: 1.0.0 + --- + Body + """; + + List entries = List.of( + new PackageEntry("SKILL.md", skillMdContent.getBytes(), skillMdContent.length(), "text/markdown"), + new PackageEntry("docs\\guide.md", "first".getBytes(), 5, "text/markdown"), + new PackageEntry("docs/guide.md", "second".getBytes(), 6, "text/markdown") + ); + + ValidationResult result = validator.validate(entries); + + assertFalse(result.passed()); + assertTrue(result.errors().stream().anyMatch(e -> e.contains("Duplicate package entry path: docs/guide.md"))); + } } diff --git a/server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/LocalFileStorageService.java b/server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/LocalFileStorageService.java index 5fd9ee8d..26889aa0 100644 --- a/server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/LocalFileStorageService.java +++ b/server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/LocalFileStorageService.java @@ -15,7 +15,7 @@ public class LocalFileStorageService implements ObjectStorageService { private final Path basePath; public LocalFileStorageService(StorageProperties properties) { - this.basePath = Paths.get(properties.getLocal().getBasePath()); + this.basePath = Paths.get(properties.getLocal().getBasePath()).toAbsolutePath().normalize(); } @Override @@ -58,5 +58,11 @@ public class LocalFileStorageService implements ObjectStorageService { } catch (IOException e) { throw new UncheckedIOException("Failed to get metadata: " + key, e); } } - private Path resolve(String key) { return basePath.resolve(key); } + private Path resolve(String key) { + Path resolved = basePath.resolve(key).normalize(); + if (!resolved.startsWith(basePath)) { + throw new IllegalArgumentException("Invalid storage key: " + key); + } + return resolved; + } } diff --git a/server/skillhub-storage/src/test/java/com/iflytek/skillhub/storage/LocalFileStorageServiceTest.java b/server/skillhub-storage/src/test/java/com/iflytek/skillhub/storage/LocalFileStorageServiceTest.java index dfaf2b72..d2e98611 100644 --- a/server/skillhub-storage/src/test/java/com/iflytek/skillhub/storage/LocalFileStorageServiceTest.java +++ b/server/skillhub-storage/src/test/java/com/iflytek/skillhub/storage/LocalFileStorageServiceTest.java @@ -61,4 +61,20 @@ class LocalFileStorageServiceTest { assertEquals(content.length, metadata.size()); assertNotNull(metadata.lastModified()); } + + @Test void shouldRejectPathTraversalKeys() { + byte[] content = "data".getBytes(StandardCharsets.UTF_8); + + IllegalArgumentException putError = assertThrows( + IllegalArgumentException.class, + () -> storageService.putObject("../escape.txt", new ByteArrayInputStream(content), content.length, "text/plain") + ); + assertEquals("Invalid storage key: ../escape.txt", putError.getMessage()); + + IllegalArgumentException getError = assertThrows( + IllegalArgumentException.class, + () -> storageService.getObject("..\\escape.txt") + ); + assertEquals("Invalid storage key: ..\\escape.txt", getError.getMessage()); + } } From ec8f7ec8385a593894921e2d74145d15df7b3e3f Mon Sep 17 00:00:00 2001 From: yun-zhi-ztl <15071461069@163.com> Date: Fri, 13 Mar 2026 11:04:52 +0800 Subject: [PATCH 06/13] fix(auth): enforce api token scopes and active-user checks - parse stored api token scopes and attach SCOPE authorities during authentication - reject disabled users from establishing api token sessions and stop touching last-used for inactive accounts - add an api-token-only scope filter that limits tokens to documented publish and token-management endpoints --- .../skillhub/auth/config/SecurityConfig.java | 7 +- .../token/ApiTokenAuthenticationFilter.java | 24 +++- .../auth/token/ApiTokenScopeFilter.java | 81 +++++++++++ .../auth/token/ApiTokenScopeService.java | 132 ++++++++++++++++++ .../ApiTokenAuthenticationFilterTest.java | 95 +++++++++++++ .../auth/token/ApiTokenScopeFilterTest.java | 102 ++++++++++++++ .../auth/token/ApiTokenScopeServiceTest.java | 77 ++++++++++ 7 files changed, 512 insertions(+), 6 deletions(-) create mode 100644 server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/token/ApiTokenScopeFilter.java create mode 100644 server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/token/ApiTokenScopeService.java create mode 100644 server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/token/ApiTokenAuthenticationFilterTest.java create mode 100644 server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/token/ApiTokenScopeFilterTest.java create mode 100644 server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/token/ApiTokenScopeServiceTest.java diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/SecurityConfig.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/SecurityConfig.java index a8c94865..a72455e7 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/SecurityConfig.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/SecurityConfig.java @@ -5,6 +5,7 @@ import com.iflytek.skillhub.auth.oauth.OAuth2LoginFailureHandler; import com.iflytek.skillhub.auth.oauth.OAuth2LoginSuccessHandler; import com.iflytek.skillhub.auth.mock.MockAuthFilter; import com.iflytek.skillhub.auth.token.ApiTokenAuthenticationFilter; +import com.iflytek.skillhub.auth.token.ApiTokenScopeFilter; import org.springframework.beans.factory.ObjectProvider; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @@ -30,6 +31,7 @@ public class SecurityConfig { private final OAuth2LoginSuccessHandler successHandler; private final OAuth2LoginFailureHandler failureHandler; private final ApiTokenAuthenticationFilter apiTokenAuthenticationFilter; + private final ApiTokenScopeFilter apiTokenScopeFilter; private final AuthenticationEntryPoint apiAuthenticationEntryPoint; private final AccessDeniedHandler apiAccessDeniedHandler; private final ObjectProvider mockAuthFilterProvider; @@ -38,6 +40,7 @@ public class SecurityConfig { OAuth2LoginSuccessHandler successHandler, OAuth2LoginFailureHandler failureHandler, ApiTokenAuthenticationFilter apiTokenAuthenticationFilter, + ApiTokenScopeFilter apiTokenScopeFilter, AuthenticationEntryPoint apiAuthenticationEntryPoint, AccessDeniedHandler apiAccessDeniedHandler, ObjectProvider mockAuthFilterProvider) { @@ -45,6 +48,7 @@ public class SecurityConfig { this.successHandler = successHandler; this.failureHandler = failureHandler; this.apiTokenAuthenticationFilter = apiTokenAuthenticationFilter; + this.apiTokenScopeFilter = apiTokenScopeFilter; this.apiAuthenticationEntryPoint = apiAuthenticationEntryPoint; this.apiAccessDeniedHandler = apiAccessDeniedHandler; this.mockAuthFilterProvider = mockAuthFilterProvider; @@ -101,7 +105,8 @@ public class SecurityConfig { .invalidateHttpSession(true) .deleteCookies("SESSION") ) - .addFilterBefore(apiTokenAuthenticationFilter, UsernamePasswordAuthenticationFilter.class); + .addFilterBefore(apiTokenAuthenticationFilter, UsernamePasswordAuthenticationFilter.class) + .addFilterAfter(apiTokenScopeFilter, ApiTokenAuthenticationFilter.class); MockAuthFilter mockAuthFilter = mockAuthFilterProvider.getIfAvailable(); if (mockAuthFilter != null) { 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 2a09417f..01632e9b 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 @@ -16,6 +16,9 @@ import org.springframework.stereotype.Component; import org.springframework.web.filter.OncePerRequestFilter; import java.io.IOException; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; import java.util.Set; import java.util.stream.Collectors; @@ -28,13 +31,16 @@ public class ApiTokenAuthenticationFilter extends OncePerRequestFilter { private final ApiTokenService apiTokenService; private final UserAccountRepository userRepo; private final UserRoleBindingRepository roleBindingRepo; + private final ApiTokenScopeService apiTokenScopeService; public ApiTokenAuthenticationFilter(ApiTokenService apiTokenService, - UserAccountRepository userRepo, - UserRoleBindingRepository roleBindingRepo) { + UserAccountRepository userRepo, + UserRoleBindingRepository roleBindingRepo, + ApiTokenScopeService apiTokenScopeService) { this.apiTokenService = apiTokenService; this.userRepo = userRepo; this.roleBindingRepo = roleBindingRepo; + this.apiTokenScopeService = apiTokenScopeService; } @Override @@ -44,20 +50,28 @@ public class ApiTokenAuthenticationFilter extends OncePerRequestFilter { if (authHeader != null && authHeader.startsWith(BEARER_PREFIX)) { String rawToken = authHeader.substring(BEARER_PREFIX.length()); apiTokenService.validateToken(rawToken).ifPresent(token -> { - apiTokenService.touchLastUsed(token); userRepo.findById(token.getUserId()).ifPresent(user -> { + if (!user.isActive()) { + return; + } Set roles = roleBindingRepo.findByUserId(user.getId()).stream() .map(rb -> rb.getRole().getCode()) .collect(Collectors.toSet()); + Set scopes = apiTokenScopeService.parseScopes(token.getScopeJson()); PlatformPrincipal principal = new PlatformPrincipal( user.getId(), user.getDisplayName(), user.getEmail(), user.getAvatarUrl(), "api_token", roles ); - var authorities = roles.stream() + List authorities = new ArrayList<>(); + authorities.addAll(roles.stream() .map(role -> new SimpleGrantedAuthority("ROLE_" + role)) - .toList(); + .toList()); + authorities.addAll(scopes.stream() + .map(scope -> new SimpleGrantedAuthority("SCOPE_" + scope)) + .toList()); var auth = new UsernamePasswordAuthenticationToken(principal, null, authorities); SecurityContextHolder.getContext().setAuthentication(auth); + apiTokenService.touchLastUsed(token); }); }); } diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/token/ApiTokenScopeFilter.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/token/ApiTokenScopeFilter.java new file mode 100644 index 00000000..c4ff48a5 --- /dev/null +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/token/ApiTokenScopeFilter.java @@ -0,0 +1,81 @@ +package com.iflytek.skillhub.auth.token; + +import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.springframework.security.access.AccessDeniedException; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.security.web.access.AccessDeniedHandler; +import org.springframework.stereotype.Component; +import org.springframework.web.filter.OncePerRequestFilter; + +import java.io.IOException; +import java.util.Set; +import java.util.stream.Collectors; + +@Component +public class ApiTokenScopeFilter extends OncePerRequestFilter { + + private final ApiTokenScopeService apiTokenScopeService; + private final AccessDeniedHandler accessDeniedHandler; + + public ApiTokenScopeFilter(ApiTokenScopeService apiTokenScopeService, + AccessDeniedHandler accessDeniedHandler) { + this.apiTokenScopeService = apiTokenScopeService; + this.accessDeniedHandler = accessDeniedHandler; + } + + @Override + protected void doFilterInternal(HttpServletRequest request, + HttpServletResponse response, + FilterChain filterChain) throws ServletException, IOException { + Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); + if (!isApiTokenAuthentication(authentication)) { + filterChain.doFilter(request, response); + return; + } + + Set tokenScopes = authentication.getAuthorities().stream() + .map(GrantedAuthority::getAuthority) + .filter(authority -> authority.startsWith("SCOPE_")) + .map(authority -> authority.substring("SCOPE_".length())) + .collect(Collectors.toSet()); + + ApiTokenScopeService.AuthorizationDecision decision = apiTokenScopeService.authorize( + request.getMethod(), + request.getRequestURI(), + tokenScopes + ); + + if (decision.allowed()) { + filterChain.doFilter(request, response); + return; + } + + accessDeniedHandler.handle( + request, + response, + new AccessDeniedException(decision.message()) + ); + } + + @Override + protected boolean shouldNotFilter(HttpServletRequest request) { + String path = request.getRequestURI(); + return path == null || (!path.startsWith("/api/v1/") && !path.startsWith("/api/compat/")); + } + + private boolean isApiTokenAuthentication(Authentication authentication) { + if (authentication == null || !authentication.isAuthenticated()) { + return false; + } + + Object principal = authentication.getPrincipal(); + return principal instanceof PlatformPrincipal platformPrincipal + && "api_token".equals(platformPrincipal.oauthProvider()); + } +} diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/token/ApiTokenScopeService.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/token/ApiTokenScopeService.java new file mode 100644 index 00000000..09445f69 --- /dev/null +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/token/ApiTokenScopeService.java @@ -0,0 +1,132 @@ +package com.iflytek.skillhub.auth.token; + +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.ObjectMapper; +import org.springframework.stereotype.Service; +import org.springframework.util.AntPathMatcher; + +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; + +@Service +public class ApiTokenScopeService { + + private static final TypeReference> STRING_LIST = new TypeReference<>() { + }; + + private static final List UNSCOPED_ALLOWED_RULES = List.of( + ScopeRule.allow(null, "/api/v1/health"), + ScopeRule.allow(null, "/api/v1/auth/providers"), + ScopeRule.allow(null, "/api/v1/auth/me"), + ScopeRule.allow(null, "/api/v1/cli/auth/device/**"), + ScopeRule.allow(null, "/api/v1/cli/check"), + ScopeRule.allow("GET", "/api/v1/cli/whoami"), + ScopeRule.allow("GET", "/api/v1/skills"), + ScopeRule.allow("GET", "/api/v1/skills/**"), + ScopeRule.allow("GET", "/api/v1/namespaces"), + ScopeRule.allow("GET", "/api/v1/namespaces/*"), + ScopeRule.allow("GET", "/api/compat/v1/search"), + ScopeRule.allow("GET", "/api/compat/v1/resolve/**"), + ScopeRule.allow("GET", "/api/compat/v1/whoami"), + ScopeRule.allow(null, "/.well-known/**"), + ScopeRule.allow(null, "/actuator/health"), + ScopeRule.allow(null, "/v3/api-docs/**"), + ScopeRule.allow(null, "/swagger-ui/**") + ); + + private static final List REQUIRED_SCOPE_RULES = List.of( + ScopeRule.require(null, "/api/v1/tokens", "token:manage"), + ScopeRule.require(null, "/api/v1/tokens/**", "token:manage"), + ScopeRule.require("POST", "/api/v1/skills/*/publish", "skill:publish"), + ScopeRule.require("POST", "/api/v1/cli/publish", "skill:publish"), + ScopeRule.require("POST", "/api/compat/v1/publish", "skill:publish") + ); + + private final ObjectMapper objectMapper; + private final AntPathMatcher pathMatcher = new AntPathMatcher(); + + public ApiTokenScopeService(ObjectMapper objectMapper) { + this.objectMapper = objectMapper; + } + + public Set parseScopes(String scopeJson) { + if (scopeJson == null || scopeJson.isBlank()) { + return Set.of(); + } + + try { + List scopes = objectMapper.readValue(scopeJson, STRING_LIST); + Set normalized = new LinkedHashSet<>(); + for (String scope : scopes) { + if (scope != null) { + String trimmed = scope.trim(); + if (!trimmed.isEmpty()) { + normalized.add(trimmed); + } + } + } + return Set.copyOf(normalized); + } catch (Exception e) { + return Set.of(); + } + } + + public AuthorizationDecision authorize(String method, String path, Set tokenScopes) { + if (!isApiPath(path)) { + return AuthorizationDecision.allow(); + } + + for (ScopeRule rule : UNSCOPED_ALLOWED_RULES) { + if (rule.matches(method, path, pathMatcher)) { + return AuthorizationDecision.allow(); + } + } + + for (ScopeRule rule : REQUIRED_SCOPE_RULES) { + if (rule.matches(method, path, pathMatcher)) { + if (tokenScopes.contains(rule.requiredScope())) { + return AuthorizationDecision.allow(); + } + return AuthorizationDecision.missingScope(rule.requiredScope()); + } + } + + return AuthorizationDecision.unsupported(path); + } + + private boolean isApiPath(String path) { + return path != null && (path.startsWith("/api/v1/") || path.startsWith("/api/compat/")); + } + + public record AuthorizationDecision(boolean allowed, String requiredScope, String message) { + public static AuthorizationDecision allow() { + return new AuthorizationDecision(true, null, null); + } + + public static AuthorizationDecision missingScope(String requiredScope) { + return new AuthorizationDecision(false, requiredScope, "Missing API token scope: " + requiredScope); + } + + public static AuthorizationDecision unsupported(String path) { + return new AuthorizationDecision(false, null, "API token cannot access endpoint: " + path); + } + } + + private record ScopeRule(String method, String pattern, String requiredScope) { + static ScopeRule allow(String method, String pattern) { + return new ScopeRule(method, pattern, null); + } + + static ScopeRule require(String method, String pattern, String requiredScope) { + return new ScopeRule(method, pattern, requiredScope); + } + + boolean matches(String requestMethod, String requestPath, AntPathMatcher matcher) { + if (method != null && !method.equalsIgnoreCase(requestMethod)) { + return false; + } + return matcher.match(pattern, requestPath); + } + } +} 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 new file mode 100644 index 00000000..a981ffd1 --- /dev/null +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/token/ApiTokenAuthenticationFilterTest.java @@ -0,0 +1,95 @@ +package com.iflytek.skillhub.auth.token; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.iflytek.skillhub.auth.entity.ApiToken; +import com.iflytek.skillhub.auth.entity.Role; +import com.iflytek.skillhub.auth.entity.UserRoleBinding; +import com.iflytek.skillhub.auth.repository.UserRoleBindingRepository; +import com.iflytek.skillhub.domain.user.UserAccount; +import com.iflytek.skillhub.domain.user.UserAccountRepository; +import com.iflytek.skillhub.domain.user.UserStatus; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.springframework.mock.web.MockFilterChain; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.security.core.context.SecurityContextHolder; +import java.util.List; +import java.util.Optional; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +class ApiTokenAuthenticationFilterTest { + + private final ApiTokenService apiTokenService = mock(ApiTokenService.class); + private final UserAccountRepository userAccountRepository = mock(UserAccountRepository.class); + private final UserRoleBindingRepository roleBindingRepository = mock(UserRoleBindingRepository.class); + private final ApiTokenScopeService scopeService = new ApiTokenScopeService(new ObjectMapper()); + private final ApiTokenAuthenticationFilter filter = new ApiTokenAuthenticationFilter( + apiTokenService, + userAccountRepository, + roleBindingRepository, + scopeService + ); + + @AfterEach + void clearSecurityContext() { + SecurityContextHolder.clearContext(); + } + + @Test + void shouldPopulateRoleAndScopeAuthoritiesForActiveUser() throws Exception { + ApiToken token = new ApiToken("user-1", "cli", "sk_test", "hash", "[\"skill:publish\",\"token:manage\"]"); + UserAccount user = new UserAccount("user-1", "Alice", "alice@example.com", ""); + UserRoleBinding binding = mock(UserRoleBinding.class); + Role role = mock(Role.class); + + when(apiTokenService.validateToken("raw-token")).thenReturn(Optional.of(token)); + when(userAccountRepository.findById("user-1")).thenReturn(Optional.of(user)); + when(roleBindingRepository.findByUserId("user-1")).thenReturn(List.of(binding)); + when(binding.getRole()).thenReturn(role); + when(role.getCode()).thenReturn("SKILL_ADMIN"); + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setRequestURI("/api/v1/cli/whoami"); + request.addHeader("Authorization", "Bearer raw-token"); + + filter.doFilter(request, new MockHttpServletResponse(), new MockFilterChain()); + + var authentication = SecurityContextHolder.getContext().getAuthentication(); + assertNotNull(authentication); + assertTrue(authentication.getAuthorities().stream() + .anyMatch(authority -> authority.getAuthority().equals("ROLE_SKILL_ADMIN"))); + assertTrue(authentication.getAuthorities().stream() + .anyMatch(authority -> authority.getAuthority().equals("SCOPE_skill:publish"))); + assertTrue(authentication.getAuthorities().stream() + .anyMatch(authority -> authority.getAuthority().equals("SCOPE_token:manage"))); + verify(apiTokenService).touchLastUsed(token); + } + + @Test + void shouldRejectDisabledUsers() throws Exception { + ApiToken token = new ApiToken("user-2", "cli", "sk_test", "hash", "[\"skill:publish\"]"); + UserAccount user = new UserAccount("user-2", "Bob", "bob@example.com", ""); + user.setStatus(UserStatus.DISABLED); + + when(apiTokenService.validateToken("raw-token")).thenReturn(Optional.of(token)); + when(userAccountRepository.findById("user-2")).thenReturn(Optional.of(user)); + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setRequestURI("/api/v1/cli/publish"); + request.addHeader("Authorization", "Bearer raw-token"); + + filter.doFilter(request, new MockHttpServletResponse(), new MockFilterChain()); + + assertNull(SecurityContextHolder.getContext().getAuthentication()); + verify(apiTokenService, never()).touchLastUsed(token); + } +} diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/token/ApiTokenScopeFilterTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/token/ApiTokenScopeFilterTest.java new file mode 100644 index 00000000..fd50db75 --- /dev/null +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/token/ApiTokenScopeFilterTest.java @@ -0,0 +1,102 @@ +package com.iflytek.skillhub.auth.token; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; +import jakarta.servlet.FilterChain; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; +import org.springframework.security.core.authority.SimpleGrantedAuthority; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.security.web.access.AccessDeniedHandler; + +import java.util.List; +import java.util.Set; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; + +class ApiTokenScopeFilterTest { + + private final ApiTokenScopeService scopeService = new ApiTokenScopeService(new ObjectMapper()); + + @AfterEach + void clearSecurityContext() { + SecurityContextHolder.clearContext(); + } + + @Test + void shouldDenyApiTokenWithoutRequiredScope() throws Exception { + AccessDeniedHandler handler = (request, response, accessDeniedException) -> { + response.sendError(HttpServletResponse.SC_FORBIDDEN, accessDeniedException.getMessage()); + }; + ApiTokenScopeFilter filter = new ApiTokenScopeFilter(scopeService, handler); + + PlatformPrincipal principal = new PlatformPrincipal( + "user-1", + "Alice", + "alice@example.com", + "", + "api_token", + Set.of("SKILL_ADMIN") + ); + var authentication = new UsernamePasswordAuthenticationToken( + principal, + null, + List.of( + new SimpleGrantedAuthority("ROLE_SKILL_ADMIN"), + new SimpleGrantedAuthority("SCOPE_skill:read") + ) + ); + SecurityContextHolder.getContext().setAuthentication(authentication); + + MockHttpServletRequest request = new MockHttpServletRequest("POST", "/api/v1/cli/publish"); + MockHttpServletResponse response = new MockHttpServletResponse(); + FilterChain chain = mock(FilterChain.class); + + filter.doFilter(request, response, chain); + + assertEquals(HttpServletResponse.SC_FORBIDDEN, response.getStatus()); + assertTrue(response.getErrorMessage().contains("Missing API token scope: skill:publish")); + verify(chain, never()).doFilter(request, response); + } + + @Test + void shouldAllowSessionAuthRequestsWithoutScopeChecks() throws Exception { + AccessDeniedHandler handler = mock(AccessDeniedHandler.class); + ApiTokenScopeFilter filter = new ApiTokenScopeFilter(scopeService, handler); + + PlatformPrincipal principal = new PlatformPrincipal( + "user-2", + "Carol", + "carol@example.com", + "", + "github", + Set.of("SUPER_ADMIN") + ); + var authentication = new UsernamePasswordAuthenticationToken( + principal, + null, + List.of(new SimpleGrantedAuthority("ROLE_SUPER_ADMIN")) + ); + SecurityContextHolder.getContext().setAuthentication(authentication); + + MockHttpServletRequest request = new MockHttpServletRequest("POST", "/api/v1/admin/users/user-2/status"); + MockHttpServletResponse response = new MockHttpServletResponse(); + FilterChain chain = mock(FilterChain.class); + + filter.doFilter(request, response, chain); + + verify(chain).doFilter(request, response); + verify(handler, never()).handle(eq(request), eq(response), any()); + } +} diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/token/ApiTokenScopeServiceTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/token/ApiTokenScopeServiceTest.java new file mode 100644 index 00000000..70c73b2d --- /dev/null +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/token/ApiTokenScopeServiceTest.java @@ -0,0 +1,77 @@ +package com.iflytek.skillhub.auth.token; + +import com.fasterxml.jackson.databind.ObjectMapper; +import org.junit.jupiter.api.Test; + +import java.util.Set; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class ApiTokenScopeServiceTest { + + private final ApiTokenScopeService scopeService = new ApiTokenScopeService(new ObjectMapper()); + + @Test + void parseScopesShouldNormalizeJsonArray() { + Set scopes = scopeService.parseScopes("[\"skill:read\", \"skill:publish\", \"skill:read\", \" \"]"); + + assertEquals(Set.of("skill:read", "skill:publish"), scopes); + } + + @Test + void authorizeShouldAllowCliWhoamiWithoutScope() { + ApiTokenScopeService.AuthorizationDecision decision = scopeService.authorize( + "GET", + "/api/v1/cli/whoami", + Set.of() + ); + + assertTrue(decision.allowed()); + } + + @Test + void authorizeShouldRequirePublishScopeForPortalPublish() { + ApiTokenScopeService.AuthorizationDecision denied = scopeService.authorize( + "POST", + "/api/v1/skills/team-a/publish", + Set.of("skill:read") + ); + + assertFalse(denied.allowed()); + assertEquals("skill:publish", denied.requiredScope()); + + ApiTokenScopeService.AuthorizationDecision allowed = scopeService.authorize( + "POST", + "/api/v1/skills/team-a/publish", + Set.of("skill:publish") + ); + + assertTrue(allowed.allowed()); + } + + @Test + void authorizeShouldRequireTokenManageScopeForTokenEndpoints() { + ApiTokenScopeService.AuthorizationDecision decision = scopeService.authorize( + "GET", + "/api/v1/tokens", + Set.of("skill:publish") + ); + + assertFalse(decision.allowed()); + assertEquals("token:manage", decision.requiredScope()); + } + + @Test + void authorizeShouldDenyUnsupportedAuthenticatedEndpoints() { + ApiTokenScopeService.AuthorizationDecision decision = scopeService.authorize( + "GET", + "/api/v1/me/skills", + Set.of("skill:read", "skill:publish") + ); + + assertFalse(decision.allowed()); + assertEquals("API token cannot access endpoint: /api/v1/me/skills", decision.message()); + } +} From ad8bb9c6fd94b9d762ca7ac8ce60256d07fd310b Mon Sep 17 00:00:00 2001 From: yun-zhi-ztl <15071461069@163.com> Date: Fri, 13 Mar 2026 11:12:18 +0800 Subject: [PATCH 07/13] fix(security): whitelist only public skill GET routes - require authentication for skill star and rating GET endpoints before the public skill-read rules - keep documented public skill detail, version, download, resolve, and tag listing endpoints readable anonymously - add regression coverage for anonymous star and rating access denial plus public tag listing --- .../controller/SkillRatingControllerTest.java | 6 +++ .../controller/SkillStarControllerTest.java | 6 +++ .../controller/SkillTagControllerTest.java | 48 +++++++++++++++++++ .../skillhub/auth/config/SecurityConfig.java | 18 ++++++- 4 files changed, 77 insertions(+), 1 deletion(-) create mode 100644 server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/SkillTagControllerTest.java diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/SkillRatingControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/SkillRatingControllerTest.java index 8bc0dceb..7043ac04 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/SkillRatingControllerTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/SkillRatingControllerTest.java @@ -107,4 +107,10 @@ class SkillRatingControllerTest { .content("{\"score\": 4}")) .andExpect(status().isUnauthorized()); } + + @Test + void get_user_rating_unauthenticated_returns_401() throws Exception { + mockMvc.perform(get("/api/v1/skills/10/rating")) + .andExpect(status().isUnauthorized()); + } } diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/SkillStarControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/SkillStarControllerTest.java index de0b5f92..2912fa7f 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/SkillStarControllerTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/SkillStarControllerTest.java @@ -126,4 +126,10 @@ class SkillStarControllerTest { .andExpect(jsonPath("$.timestamp").isNotEmpty()) .andExpect(jsonPath("$.requestId").isNotEmpty()); } + + @Test + void check_starred_unauthenticated_returns_401() throws Exception { + mockMvc.perform(get("/api/v1/skills/10/star")) + .andExpect(status().isUnauthorized()); + } } diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/SkillTagControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/SkillTagControllerTest.java new file mode 100644 index 00000000..2daffdcd --- /dev/null +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/SkillTagControllerTest.java @@ -0,0 +1,48 @@ +package com.iflytek.skillhub.controller; + +import com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository; +import com.iflytek.skillhub.domain.skill.SkillTag; +import com.iflytek.skillhub.domain.skill.service.SkillTagService; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.test.context.ActiveProfiles; +import org.springframework.test.web.servlet.MockMvc; + +import java.util.List; + +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.when; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +@SpringBootTest +@AutoConfigureMockMvc +@ActiveProfiles("test") +class SkillTagControllerTest { + + @Autowired + private MockMvc mockMvc; + + @MockBean + private SkillTagService skillTagService; + + @MockBean + private NamespaceMemberRepository namespaceMemberRepository; + + @Test + void list_tags_is_public() throws Exception { + when(skillTagService.listTags(eq("team"), eq("demo"))) + .thenReturn(List.of(new SkillTag(1L, "latest", 2L, "user-1"))); + + mockMvc.perform(get("/api/v1/skills/team/demo/tags")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.code").value(0)) + .andExpect(jsonPath("$.data[0].tagName").value("latest")) + .andExpect(jsonPath("$.timestamp").isNotEmpty()) + .andExpect(jsonPath("$.requestId").isNotEmpty()); + } +} diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/SecurityConfig.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/SecurityConfig.java index a72455e7..828a6755 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/SecurityConfig.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/SecurityConfig.java @@ -79,7 +79,23 @@ public class SecurityConfig { "/api/compat/v1/search", "/api/compat/v1/resolve/**" ).permitAll() - .requestMatchers(HttpMethod.GET, "/api/v1/skills", "/api/v1/skills/**").permitAll() + .requestMatchers(HttpMethod.GET, "/api/v1/skills/*/star", "/api/v1/skills/*/rating").authenticated() + .requestMatchers( + HttpMethod.GET, + "/api/v1/skills", + "/api/v1/skills/*/*", + "/api/v1/skills/*/*/versions", + "/api/v1/skills/*/*/versions/*", + "/api/v1/skills/*/*/versions/*/files", + "/api/v1/skills/*/*/versions/*/file", + "/api/v1/skills/*/*/resolve", + "/api/v1/skills/*/*/download", + "/api/v1/skills/*/*/versions/*/download", + "/api/v1/skills/*/*/tags", + "/api/v1/skills/*/*/tags/*/files", + "/api/v1/skills/*/*/tags/*/file", + "/api/v1/skills/*/*/tags/*/download" + ).permitAll() .requestMatchers(HttpMethod.GET, "/api/v1/namespaces", "/api/v1/namespaces/*").permitAll() .requestMatchers("/api/v1/admin/**").hasAnyRole("SUPER_ADMIN", "SKILL_ADMIN", "USER_ADMIN", "AUDITOR") .anyRequest().authenticated() From 383bc1edaed7cbd5ea83959f2e70be7058ba5a43 Mon Sep 17 00:00:00 2001 From: yun-zhi-ztl <15071461069@163.com> Date: Fri, 13 Mar 2026 11:46:21 +0800 Subject: [PATCH 08/13] fix(admin): replace compat and admin placeholders with real queries Implement compat search through SkillSearchAppService instead of returning an empty placeholder list, and map search results back to canonical slugs for the compatibility API. Replace hard-coded admin user and audit-log payloads with repository-backed application services. User management now supports paged search and status filters, validates managed statuses and role codes, prevents USER_ADMIN from assigning SUPER_ADMIN, and persists role/status changes against the real repositories. Audit logs now read from the audit_log table through a dedicated query repository/service with filterable pagination. Align admin response DTOs with the frontend contract, add domain not-found handling for localized 404 responses, and cover the new behavior with controller and service regression tests. Verified with targeted skillhub-app tests plus full server mvn test. --- .../compat/ClawHubCompatController.java | 36 ++++- .../controller/admin/AuditLogController.java | 22 +-- .../admin/UserManagementController.java | 26 ++-- .../dto/AdminUserSummaryResponse.java | 11 +- .../skillhub/dto/AuditLogItemResponse.java | 12 +- .../exception/GlobalExceptionHandler.java | 7 + .../repository/AdminUserSearchRepository.java | 74 +++++++++ .../service/AdminAuditLogAppService.java | 97 ++++++++++++ .../skillhub/service/AdminUserAppService.java | 145 +++++++++++++++++ .../src/main/resources/messages.properties | 5 + .../src/main/resources/messages_zh.properties | 5 + .../compat/ClawHubCompatControllerTest.java | 34 +++- .../admin/AuditLogControllerTest.java | 32 +++- .../admin/UserManagementControllerTest.java | 43 ++++- .../service/AdminAuditLogAppServiceTest.java | 44 ++++++ .../service/AdminUserAppServiceTest.java | 147 ++++++++++++++++++ .../repository/UserRoleBindingRepository.java | 4 + .../infra/jpa/UserAccountJpaRepository.java | 3 +- 18 files changed, 700 insertions(+), 47 deletions(-) create mode 100644 server/skillhub-app/src/main/java/com/iflytek/skillhub/repository/AdminUserSearchRepository.java create mode 100644 server/skillhub-app/src/main/java/com/iflytek/skillhub/service/AdminAuditLogAppService.java create mode 100644 server/skillhub-app/src/main/java/com/iflytek/skillhub/service/AdminUserAppService.java create mode 100644 server/skillhub-app/src/test/java/com/iflytek/skillhub/service/AdminAuditLogAppServiceTest.java create mode 100644 server/skillhub-app/src/test/java/com/iflytek/skillhub/service/AdminUserAppServiceTest.java diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/compat/ClawHubCompatController.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/compat/ClawHubCompatController.java index 70717170..de278d66 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/compat/ClawHubCompatController.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/compat/ClawHubCompatController.java @@ -1,28 +1,56 @@ package com.iflytek.skillhub.compat; import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; +import com.iflytek.skillhub.compat.dto.ClawHubSkillItem; import com.iflytek.skillhub.compat.dto.ClawHubResolveResponse; import com.iflytek.skillhub.compat.dto.ClawHubSearchResponse; import com.iflytek.skillhub.compat.dto.ClawHubWhoamiResponse; +import com.iflytek.skillhub.domain.namespace.NamespaceRole; +import com.iflytek.skillhub.service.SkillSearchAppService; import org.springframework.security.core.annotation.AuthenticationPrincipal; import org.springframework.web.bind.annotation.*; import java.util.List; +import java.util.Map; @RestController @RequestMapping("/api/compat/v1") public class ClawHubCompatController { private final CanonicalSlugMapper mapper; + private final SkillSearchAppService skillSearchAppService; - public ClawHubCompatController(CanonicalSlugMapper mapper) { + public ClawHubCompatController(CanonicalSlugMapper mapper, SkillSearchAppService skillSearchAppService) { this.mapper = mapper; + this.skillSearchAppService = skillSearchAppService; } @GetMapping("/search") - public ClawHubSearchResponse search(@RequestParam String q) { - // Return empty results for now (placeholder) - return new ClawHubSearchResponse(List.of()); + public ClawHubSearchResponse search( + @RequestParam String q, + @RequestParam(defaultValue = "0") int page, + @RequestParam(defaultValue = "20") int limit, + @RequestAttribute(value = "userId", required = false) String userId, + @RequestAttribute(value = "userNsRoles", required = false) Map userNsRoles) { + SkillSearchAppService.SearchResponse response = skillSearchAppService.search( + q, + null, + q == null || q.isBlank() ? "newest" : "relevance", + page, + limit, + userId, + userNsRoles + ); + + List items = response.items().stream() + .map(item -> new ClawHubSkillItem( + mapper.toCanonical(item.namespace(), item.slug()), + item.summary(), + item.latestVersion(), + item.starCount())) + .toList(); + + return new ClawHubSearchResponse(items); } @GetMapping("/resolve/{canonicalSlug}") diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/admin/AuditLogController.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/admin/AuditLogController.java index 4426113f..21fa7405 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/admin/AuditLogController.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/admin/AuditLogController.java @@ -5,19 +5,20 @@ import com.iflytek.skillhub.dto.ApiResponse; import com.iflytek.skillhub.dto.ApiResponseFactory; import com.iflytek.skillhub.dto.AuditLogItemResponse; import com.iflytek.skillhub.dto.PageResponse; -import org.springframework.data.domain.PageImpl; +import com.iflytek.skillhub.service.AdminAuditLogAppService; import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.web.bind.annotation.*; -import java.time.Instant; -import java.util.List; - @RestController @RequestMapping("/api/v1/admin/audit-logs") public class AuditLogController extends BaseApiController { - public AuditLogController(ApiResponseFactory responseFactory) { + private final AdminAuditLogAppService adminAuditLogAppService; + + public AuditLogController(AdminAuditLogAppService adminAuditLogAppService, + ApiResponseFactory responseFactory) { super(responseFactory); + this.adminAuditLogAppService = adminAuditLogAppService; } @GetMapping @@ -27,15 +28,6 @@ public class AuditLogController extends BaseApiController { @RequestParam(defaultValue = "20") int size, @RequestParam(required = false) String userId, @RequestParam(required = false) String action) { - List logs = List.of( - new AuditLogItemResponse( - "log-1", "user-1", "CREATE_SKILL", "SKILL", "skill-123", Instant.now(), "192.168.1.1" - ), - new AuditLogItemResponse( - "log-2", "user-2", "UPDATE_NAMESPACE", "NAMESPACE", "ns-456", - Instant.now().minusSeconds(3600), "192.168.1.2" - ) - ); - return ok("response.success.read", PageResponse.from(new PageImpl<>(logs))); + return ok("response.success.read", adminAuditLogAppService.listAuditLogs(page, size, userId, action)); } } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/admin/UserManagementController.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/admin/UserManagementController.java index 8f4bb657..bffd32c5 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/admin/UserManagementController.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/admin/UserManagementController.java @@ -1,6 +1,7 @@ package com.iflytek.skillhub.controller.admin; import com.iflytek.skillhub.controller.BaseApiController; +import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; import com.iflytek.skillhub.dto.AdminUserMutationResponse; import com.iflytek.skillhub.dto.AdminUserRoleUpdateRequest; import com.iflytek.skillhub.dto.AdminUserStatusUpdateRequest; @@ -8,39 +9,42 @@ import com.iflytek.skillhub.dto.AdminUserSummaryResponse; import com.iflytek.skillhub.dto.ApiResponse; import com.iflytek.skillhub.dto.ApiResponseFactory; import com.iflytek.skillhub.dto.PageResponse; +import com.iflytek.skillhub.service.AdminUserAppService; import jakarta.validation.Valid; -import org.springframework.data.domain.PageImpl; import org.springframework.security.access.prepost.PreAuthorize; +import org.springframework.security.core.annotation.AuthenticationPrincipal; import org.springframework.web.bind.annotation.*; -import java.util.List; - @RestController @RequestMapping("/api/v1/admin/users") public class UserManagementController extends BaseApiController { - public UserManagementController(ApiResponseFactory responseFactory) { + private final AdminUserAppService adminUserAppService; + + public UserManagementController(AdminUserAppService adminUserAppService, + ApiResponseFactory responseFactory) { super(responseFactory); + this.adminUserAppService = adminUserAppService; } @GetMapping @PreAuthorize("hasAnyRole('USER_ADMIN', 'SUPER_ADMIN')") public ApiResponse> listUsers( + @RequestParam(required = false) String search, + @RequestParam(required = false) String status, @RequestParam(defaultValue = "0") int page, @RequestParam(defaultValue = "20") int size) { - List users = List.of( - new AdminUserSummaryResponse("user-1", "alice", "USER", "ACTIVE"), - new AdminUserSummaryResponse("user-2", "bob", "USER", "ACTIVE") - ); - return ok("response.success.read", PageResponse.from(new PageImpl<>(users))); + return ok("response.success.read", adminUserAppService.listUsers(search, status, page, size)); } @PutMapping("/{userId}/role") @PreAuthorize("hasAnyRole('USER_ADMIN', 'SUPER_ADMIN')") public ApiResponse updateUserRole( @PathVariable String userId, + @AuthenticationPrincipal PlatformPrincipal principal, @Valid @RequestBody AdminUserRoleUpdateRequest request) { - return ok("response.success.updated", new AdminUserMutationResponse(userId, request.role(), null)); + return ok("response.success.updated", + adminUserAppService.updateUserRole(userId, request.role(), principal.platformRoles())); } @PutMapping("/{userId}/status") @@ -48,6 +52,6 @@ public class UserManagementController extends BaseApiController { public ApiResponse updateUserStatus( @PathVariable String userId, @Valid @RequestBody AdminUserStatusUpdateRequest request) { - return ok("response.success.updated", new AdminUserMutationResponse(userId, null, request.status())); + return ok("response.success.updated", adminUserAppService.updateUserStatus(userId, request.status())); } } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/dto/AdminUserSummaryResponse.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/dto/AdminUserSummaryResponse.java index 3d569967..3cdbbfd2 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/dto/AdminUserSummaryResponse.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/dto/AdminUserSummaryResponse.java @@ -1,9 +1,14 @@ package com.iflytek.skillhub.dto; +import java.time.LocalDateTime; +import java.util.List; + public record AdminUserSummaryResponse( - String userId, + String id, String username, - String role, - String status + String email, + String status, + List platformRoles, + LocalDateTime createdAt ) { } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/dto/AuditLogItemResponse.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/dto/AuditLogItemResponse.java index cd285c8a..699c1b19 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/dto/AuditLogItemResponse.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/dto/AuditLogItemResponse.java @@ -3,12 +3,12 @@ package com.iflytek.skillhub.dto; import java.time.Instant; public record AuditLogItemResponse( - String id, - String userId, + Long id, String action, - String resourceType, - String resourceId, - Instant timestamp, - String ipAddress + String userId, + String username, + String details, + String ipAddress, + Instant timestamp ) { } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/exception/GlobalExceptionHandler.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/exception/GlobalExceptionHandler.java index c1e91628..49625f63 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/exception/GlobalExceptionHandler.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/exception/GlobalExceptionHandler.java @@ -4,6 +4,7 @@ import com.iflytek.skillhub.dto.ApiResponse; import com.iflytek.skillhub.dto.ApiResponseFactory; import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException; import com.iflytek.skillhub.domain.shared.exception.DomainForbiddenException; +import com.iflytek.skillhub.domain.shared.exception.DomainNotFoundException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.slf4j.MDC; @@ -43,6 +44,12 @@ public class GlobalExceptionHandler { apiResponseFactory.error(403, ex.messageCode(), ex.messageArgs())); } + @ExceptionHandler(DomainNotFoundException.class) + public ResponseEntity> handleDomainNotFound(DomainNotFoundException ex) { + return ResponseEntity.status(HttpStatus.NOT_FOUND).body( + apiResponseFactory.error(404, ex.messageCode(), ex.messageArgs())); + } + @ExceptionHandler(MethodArgumentNotValidException.class) public ResponseEntity> handleValidation(MethodArgumentNotValidException ex) { String msg = ex.getBindingResult().getFieldErrors().stream() diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/repository/AdminUserSearchRepository.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/repository/AdminUserSearchRepository.java new file mode 100644 index 00000000..c68bb7ed --- /dev/null +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/repository/AdminUserSearchRepository.java @@ -0,0 +1,74 @@ +package com.iflytek.skillhub.repository; + +import com.iflytek.skillhub.domain.user.UserAccount; +import com.iflytek.skillhub.domain.user.UserStatus; +import jakarta.persistence.EntityManager; +import jakarta.persistence.TypedQuery; +import jakarta.persistence.criteria.CriteriaBuilder; +import jakarta.persistence.criteria.CriteriaQuery; +import jakarta.persistence.criteria.Predicate; +import jakarta.persistence.criteria.Root; +import org.springframework.data.domain.Page; +import org.springframework.data.domain.PageImpl; +import org.springframework.data.domain.Pageable; +import org.springframework.stereotype.Repository; +import org.springframework.util.StringUtils; + +import java.util.ArrayList; +import java.util.List; +import java.util.Locale; + +@Repository +public class AdminUserSearchRepository { + + private final EntityManager entityManager; + + public AdminUserSearchRepository(EntityManager entityManager) { + this.entityManager = entityManager; + } + + public Page search(String search, UserStatus status, Pageable pageable) { + CriteriaBuilder builder = entityManager.getCriteriaBuilder(); + + CriteriaQuery query = builder.createQuery(UserAccount.class); + Root root = query.from(UserAccount.class); + List predicates = buildPredicates(search, status, builder, root); + query.select(root) + .where(predicates.toArray(Predicate[]::new)) + .orderBy(builder.desc(root.get("createdAt"))); + + TypedQuery typedQuery = entityManager.createQuery(query); + typedQuery.setFirstResult((int) pageable.getOffset()); + typedQuery.setMaxResults(pageable.getPageSize()); + List users = typedQuery.getResultList(); + + CriteriaQuery countQuery = builder.createQuery(Long.class); + Root countRoot = countQuery.from(UserAccount.class); + List countPredicates = buildPredicates(search, status, builder, countRoot); + countQuery.select(builder.count(countRoot)) + .where(countPredicates.toArray(Predicate[]::new)); + long total = entityManager.createQuery(countQuery).getSingleResult(); + + return new PageImpl<>(users, pageable, total); + } + + private List buildPredicates( + String search, + UserStatus status, + CriteriaBuilder builder, + Root root) { + List predicates = new ArrayList<>(); + if (StringUtils.hasText(search)) { + String normalized = "%" + search.trim().toLowerCase(Locale.ROOT) + "%"; + predicates.add(builder.or( + builder.like(builder.lower(root.get("id")), normalized), + builder.like(builder.lower(root.get("displayName")), normalized), + builder.like(builder.lower(root.get("email")), normalized) + )); + } + if (status != null) { + predicates.add(builder.equal(root.get("status"), status)); + } + return predicates; + } +} diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/AdminAuditLogAppService.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/AdminAuditLogAppService.java new file mode 100644 index 00000000..d1673617 --- /dev/null +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/AdminAuditLogAppService.java @@ -0,0 +1,97 @@ +package com.iflytek.skillhub.service; + +import com.iflytek.skillhub.dto.AuditLogItemResponse; +import com.iflytek.skillhub.dto.PageResponse; +import org.springframework.jdbc.core.namedparam.MapSqlParameterSource; +import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate; +import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Transactional; +import org.springframework.util.StringUtils; + +import java.sql.Timestamp; +import java.time.Instant; +import java.util.List; + +@Service +public class AdminAuditLogAppService { + + private final NamedParameterJdbcTemplate namedParameterJdbcTemplate; + + public AdminAuditLogAppService(NamedParameterJdbcTemplate namedParameterJdbcTemplate) { + this.namedParameterJdbcTemplate = namedParameterJdbcTemplate; + } + + @Transactional(readOnly = true) + public PageResponse listAuditLogs(int page, int size, String userId, String action) { + MapSqlParameterSource parameters = new MapSqlParameterSource() + .addValue("limit", size) + .addValue("offset", Math.max(page, 0) * size); + + String whereClause = buildWhereClause(parameters, userId, action); + Long total = namedParameterJdbcTemplate.queryForObject( + "SELECT COUNT(*) FROM audit_log al" + whereClause, + parameters, + Long.class + ); + + List items = namedParameterJdbcTemplate.query( + """ + SELECT al.id, + al.action, + al.actor_user_id, + ua.display_name, + al.detail_json, + al.target_type, + al.target_id, + al.client_ip, + al.created_at + FROM audit_log al + LEFT JOIN user_account ua ON ua.id = al.actor_user_id + """ + whereClause + """ + ORDER BY al.created_at DESC + LIMIT :limit OFFSET :offset + """, + parameters, + (rs, rowNum) -> new AuditLogItemResponse( + rs.getLong("id"), + rs.getString("action"), + rs.getString("actor_user_id"), + rs.getString("display_name"), + renderDetails( + rs.getString("detail_json"), + rs.getString("target_type"), + rs.getObject("target_id")), + rs.getString("client_ip"), + toInstant(rs.getTimestamp("created_at"))) + ); + + return new PageResponse<>(items, total == null ? 0 : total, page, size); + } + + private String buildWhereClause(MapSqlParameterSource parameters, String userId, String action) { + StringBuilder clause = new StringBuilder(" WHERE 1 = 1"); + if (StringUtils.hasText(userId)) { + clause.append(" AND al.actor_user_id = :userId"); + parameters.addValue("userId", userId.trim()); + } + if (StringUtils.hasText(action)) { + clause.append(" AND al.action = :action"); + parameters.addValue("action", action.trim()); + } + return clause.toString(); + } + + private String renderDetails(String detailJson, String targetType, Object targetId) { + if (StringUtils.hasText(detailJson)) { + return detailJson; + } + if (!StringUtils.hasText(targetType) && targetId == null) { + return null; + } + return targetType + ":" + targetId; + } + + private Instant toInstant(Timestamp timestamp) { + return timestamp == null ? null : timestamp.toInstant(); + } +} diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/AdminUserAppService.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/AdminUserAppService.java new file mode 100644 index 00000000..5d7a140d --- /dev/null +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/AdminUserAppService.java @@ -0,0 +1,145 @@ +package com.iflytek.skillhub.service; + +import com.iflytek.skillhub.auth.entity.Role; +import com.iflytek.skillhub.auth.entity.UserRoleBinding; +import com.iflytek.skillhub.auth.repository.RoleRepository; +import com.iflytek.skillhub.auth.repository.UserRoleBindingRepository; +import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException; +import com.iflytek.skillhub.domain.shared.exception.DomainForbiddenException; +import com.iflytek.skillhub.domain.shared.exception.DomainNotFoundException; +import com.iflytek.skillhub.domain.user.UserAccount; +import com.iflytek.skillhub.domain.user.UserAccountRepository; +import com.iflytek.skillhub.domain.user.UserStatus; +import com.iflytek.skillhub.dto.AdminUserMutationResponse; +import com.iflytek.skillhub.dto.AdminUserSummaryResponse; +import com.iflytek.skillhub.dto.PageResponse; +import com.iflytek.skillhub.repository.AdminUserSearchRepository; +import org.springframework.data.domain.Page; +import org.springframework.data.domain.PageRequest; +import org.springframework.data.domain.Pageable; +import org.springframework.data.domain.Sort; +import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Transactional; +import org.springframework.util.StringUtils; + +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; + +@Service +public class AdminUserAppService { + + private static final Set MANAGEABLE_STATUSES = Set.of(UserStatus.ACTIVE, UserStatus.DISABLED); + + private final AdminUserSearchRepository adminUserSearchRepository; + private final UserAccountRepository userAccountRepository; + private final UserRoleBindingRepository userRoleBindingRepository; + private final RoleRepository roleRepository; + + public AdminUserAppService( + AdminUserSearchRepository adminUserSearchRepository, + UserAccountRepository userAccountRepository, + UserRoleBindingRepository userRoleBindingRepository, + RoleRepository roleRepository) { + this.adminUserSearchRepository = adminUserSearchRepository; + this.userAccountRepository = userAccountRepository; + this.userRoleBindingRepository = userRoleBindingRepository; + this.roleRepository = roleRepository; + } + + @Transactional(readOnly = true) + public PageResponse listUsers(String search, String status, int page, int size) { + Pageable pageable = PageRequest.of(page, size, Sort.by(Sort.Direction.DESC, "createdAt")); + Page result = adminUserSearchRepository.search( + search, + StringUtils.hasText(status) ? parseStatus(status) : null, + pageable + ); + Map> rolesByUserId = loadRolesByUserId( + result.getContent().stream().map(UserAccount::getId).toList()); + + List items = result.getContent().stream() + .map(user -> new AdminUserSummaryResponse( + user.getId(), + user.getDisplayName(), + user.getEmail(), + user.getStatus().name(), + rolesByUserId.getOrDefault(user.getId(), List.of()), + user.getCreatedAt())) + .toList(); + + return new PageResponse<>(items, result.getTotalElements(), result.getNumber(), result.getSize()); + } + + @Transactional + public AdminUserMutationResponse updateUserRole(String userId, String roleCode, Set actorPlatformRoles) { + UserAccount user = loadUser(userId); + String normalizedRoleCode = normalizeRoleCode(roleCode); + + if ("SUPER_ADMIN".equals(normalizedRoleCode) + && (actorPlatformRoles == null || !actorPlatformRoles.contains("SUPER_ADMIN"))) { + throw new DomainForbiddenException("error.admin.user.role.superAdmin.assignDenied"); + } + + userRoleBindingRepository.deleteByUserId(user.getId()); + + if (!"USER".equals(normalizedRoleCode)) { + Role role = roleRepository.findByCode(normalizedRoleCode) + .orElseThrow(() -> new DomainBadRequestException("error.admin.user.role.invalid", roleCode)); + userRoleBindingRepository.save(new UserRoleBinding(user.getId(), role)); + } + + return new AdminUserMutationResponse(user.getId(), normalizedRoleCode, user.getStatus().name()); + } + + @Transactional + public AdminUserMutationResponse updateUserStatus(String userId, String status) { + UserAccount user = loadUser(userId); + UserStatus nextStatus = parseManageableStatus(status); + user.setStatus(nextStatus); + userAccountRepository.save(user); + return new AdminUserMutationResponse(user.getId(), null, nextStatus.name()); + } + + private UserStatus parseManageableStatus(String status) { + UserStatus parsedStatus = parseStatus(status); + if (!MANAGEABLE_STATUSES.contains(parsedStatus)) { + throw new DomainBadRequestException("error.admin.user.status.unsupported"); + } + return parsedStatus; + } + + private UserStatus parseStatus(String status) { + try { + return UserStatus.valueOf(status.trim().toUpperCase(Locale.ROOT)); + } catch (IllegalArgumentException ex) { + throw new DomainBadRequestException("error.admin.user.status.invalid", status); + } + } + + private String normalizeRoleCode(String roleCode) { + if (!StringUtils.hasText(roleCode)) { + throw new DomainBadRequestException("error.admin.user.role.invalid", roleCode); + } + return roleCode.trim().toUpperCase(Locale.ROOT); + } + + private Map> loadRolesByUserId(List userIds) { + if (userIds.isEmpty()) { + return Map.of(); + } + return userRoleBindingRepository.findByUserIdIn(userIds).stream() + .collect(Collectors.groupingBy( + UserRoleBinding::getUserId, + Collectors.mapping(binding -> binding.getRole().getCode(), + Collectors.collectingAndThen(Collectors.toList(), + roles -> roles.stream().sorted().toList())))); + } + + private UserAccount loadUser(String userId) { + return userAccountRepository.findById(userId) + .orElseThrow(() -> new DomainNotFoundException("error.admin.user.notFound", userId)); + } +} diff --git a/server/skillhub-app/src/main/resources/messages.properties b/server/skillhub-app/src/main/resources/messages.properties index 7cc2c460..c8b31e6b 100644 --- a/server/skillhub-app/src/main/resources/messages.properties +++ b/server/skillhub-app/src/main/resources/messages.properties @@ -71,3 +71,8 @@ error.deviceAuth.userCode.invalid=Invalid or expired user code error.deviceAuth.deviceCode.expired=Device code expired error.deviceAuth.deviceCode.invalid=Device code expired or invalid error.deviceAuth.deviceCode.used=Device code has already been used +error.admin.user.notFound=User not found: {0} +error.admin.user.role.invalid=Invalid role: {0} +error.admin.user.role.superAdmin.assignDenied=Only SUPER_ADMIN can assign SUPER_ADMIN role +error.admin.user.status.invalid=Invalid user status: {0} +error.admin.user.status.unsupported=Only ACTIVE or DISABLED status can be managed here diff --git a/server/skillhub-app/src/main/resources/messages_zh.properties b/server/skillhub-app/src/main/resources/messages_zh.properties index 5192afd1..9480c9df 100644 --- a/server/skillhub-app/src/main/resources/messages_zh.properties +++ b/server/skillhub-app/src/main/resources/messages_zh.properties @@ -71,3 +71,8 @@ error.deviceAuth.userCode.invalid=无效或已过期的用户验证码 error.deviceAuth.deviceCode.expired=设备验证码已过期 error.deviceAuth.deviceCode.invalid=设备验证码无效或已过期 error.deviceAuth.deviceCode.used=设备验证码已被使用 +error.admin.user.notFound=鐢ㄦ埛涓嶅瓨鍦細{0} +error.admin.user.role.invalid=鏃犳晥鐨勮鑹诧細{0} +error.admin.user.role.superAdmin.assignDenied=鍙湁 SUPER_ADMIN 鍙互鍒嗛厤 SUPER_ADMIN 瑙掕壊 +error.admin.user.status.invalid=鏃犳晥鐨勭敤鎴风姸鎬侊細{0} +error.admin.user.status.unsupported=杩欓噷鍙厑璁告寜 ACTIVE 鎴?DISABLED 绠＄悊鐢ㄦ埛鐘舵€? diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/compat/ClawHubCompatControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/compat/ClawHubCompatControllerTest.java index 90e6b164..e9c72230 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/compat/ClawHubCompatControllerTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/compat/ClawHubCompatControllerTest.java @@ -3,6 +3,8 @@ package com.iflytek.skillhub.compat; import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; import com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository; import com.iflytek.skillhub.auth.device.DeviceAuthService; +import com.iflytek.skillhub.dto.SkillSummaryResponse; +import com.iflytek.skillhub.service.SkillSearchAppService; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; @@ -15,7 +17,10 @@ import org.springframework.test.web.servlet.MockMvc; import java.util.List; import java.util.Set; +import java.math.BigDecimal; +import java.time.LocalDateTime; +import static org.mockito.Mockito.when; import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.authentication; import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; @@ -35,13 +40,38 @@ class ClawHubCompatControllerTest { @MockBean private DeviceAuthService deviceAuthService; + @MockBean + private SkillSearchAppService skillSearchAppService; + @Test - void search_returns_200() throws Exception { + void search_returns_mapped_results() throws Exception { + when(skillSearchAppService.search("test", null, "relevance", 0, 20, null, null)) + .thenReturn(new SkillSearchAppService.SearchResponse( + List.of(new SkillSummaryResponse( + 1L, + "my-skill", + "My Skill", + "test summary", + 10L, + 5, + BigDecimal.valueOf(4.5), + 2, + "1.2.0", + "global", + LocalDateTime.of(2026, 3, 13, 9, 0))), + 1, + 0, + 20 + )); + mockMvc.perform(get("/api/compat/v1/search") .param("q", "test")) .andExpect(status().isOk()) .andExpect(jsonPath("$.items").isArray()) - .andExpect(jsonPath("$.items").isEmpty()); + .andExpect(jsonPath("$.items[0].canonicalSlug").value("my-skill")) + .andExpect(jsonPath("$.items[0].description").value("test summary")) + .andExpect(jsonPath("$.items[0].latestVersion").value("1.2.0")) + .andExpect(jsonPath("$.items[0].starCount").value(5)); } @Test diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/admin/AuditLogControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/admin/AuditLogControllerTest.java index becb9cdd..d255616d 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/admin/AuditLogControllerTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/admin/AuditLogControllerTest.java @@ -4,6 +4,9 @@ import com.iflytek.skillhub.TestRedisConfig; import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; import com.iflytek.skillhub.auth.device.DeviceAuthService; import com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository; +import com.iflytek.skillhub.dto.AuditLogItemResponse; +import com.iflytek.skillhub.dto.PageResponse; +import com.iflytek.skillhub.service.AdminAuditLogAppService; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; @@ -17,7 +20,9 @@ import org.springframework.test.web.servlet.MockMvc; import java.util.List; import java.util.Set; +import java.time.Instant; +import static org.mockito.Mockito.when; import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.authentication; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; @@ -38,6 +43,9 @@ class AuditLogControllerTest { @MockBean private DeviceAuthService deviceAuthService; + @MockBean + private AdminAuditLogAppService adminAuditLogAppService; + @Test void listAuditLogs_unauthenticated_returns401() throws Exception { mockMvc.perform(get("/api/v1/admin/audit-logs")) @@ -53,11 +61,27 @@ class AuditLogControllerTest { principal, null, List.of(new SimpleGrantedAuthority("ROLE_AUDITOR")) ); + when(adminAuditLogAppService.listAuditLogs(0, 20, null, null)) + .thenReturn(new PageResponse<>( + List.of(new AuditLogItemResponse( + 1L, + "USER_STATUS_CHANGE", + "user-1", + "alice", + "{\"status\":\"DISABLED\"}", + "127.0.0.1", + Instant.parse("2026-03-13T01:00:00Z"))), + 1, + 0, + 20)); + mockMvc.perform(get("/api/v1/admin/audit-logs").with(authentication(auth))) .andExpect(status().isOk()) .andExpect(jsonPath("$.code").value(0)) .andExpect(jsonPath("$.data.items").isArray()) - .andExpect(jsonPath("$.data.total").value(2)); + .andExpect(jsonPath("$.data.total").value(1)) + .andExpect(jsonPath("$.data.items[0].username").value("alice")) + .andExpect(jsonPath("$.data.items[0].details").value("{\"status\":\"DISABLED\"}")); } @Test @@ -69,6 +93,9 @@ class AuditLogControllerTest { principal, null, List.of(new SimpleGrantedAuthority("ROLE_SUPER_ADMIN")) ); + when(adminAuditLogAppService.listAuditLogs(0, 20, null, null)) + .thenReturn(new PageResponse<>(List.of(), 0, 0, 20)); + mockMvc.perform(get("/api/v1/admin/audit-logs").with(authentication(auth))) .andExpect(status().isOk()) .andExpect(jsonPath("$.data.items").isArray()); @@ -83,6 +110,9 @@ class AuditLogControllerTest { principal, null, List.of(new SimpleGrantedAuthority("ROLE_AUDITOR")) ); + when(adminAuditLogAppService.listAuditLogs(0, 20, "user-1", "CREATE_SKILL")) + .thenReturn(new PageResponse<>(List.of(), 0, 0, 20)); + mockMvc.perform(get("/api/v1/admin/audit-logs") .param("userId", "user-1") .param("action", "CREATE_SKILL") diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/admin/UserManagementControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/admin/UserManagementControllerTest.java index f642cb7c..8379a594 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/admin/UserManagementControllerTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/admin/UserManagementControllerTest.java @@ -4,6 +4,10 @@ import com.iflytek.skillhub.TestRedisConfig; import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; import com.iflytek.skillhub.auth.device.DeviceAuthService; import com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository; +import com.iflytek.skillhub.dto.AdminUserMutationResponse; +import com.iflytek.skillhub.dto.AdminUserSummaryResponse; +import com.iflytek.skillhub.dto.PageResponse; +import com.iflytek.skillhub.service.AdminUserAppService; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; @@ -17,6 +21,7 @@ import org.springframework.test.web.servlet.MockMvc; import java.util.List; import java.util.Set; +import java.time.LocalDateTime; import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.authentication; import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; @@ -25,6 +30,7 @@ import static org.springframework.test.web.servlet.request.MockMvcRequestBuilder import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; import static org.springframework.http.MediaType.APPLICATION_JSON; +import static org.mockito.Mockito.when; @SpringBootTest @AutoConfigureMockMvc @@ -41,6 +47,9 @@ class UserManagementControllerTest { @MockBean private DeviceAuthService deviceAuthService; + @MockBean + private AdminUserAppService adminUserAppService; + @Test void listUsers_unauthenticated_returns401() throws Exception { mockMvc.perform(get("/api/v1/admin/users")) @@ -56,11 +65,27 @@ class UserManagementControllerTest { principal, null, List.of(new SimpleGrantedAuthority("ROLE_USER_ADMIN")) ); + when(adminUserAppService.listUsers(null, null, 0, 20)) + .thenReturn(new PageResponse<>( + List.of(new AdminUserSummaryResponse( + "user-1", + "alice", + "alice@example.com", + "ACTIVE", + List.of("AUDITOR"), + LocalDateTime.of(2026, 3, 13, 9, 0))), + 1, + 0, + 20)); + mockMvc.perform(get("/api/v1/admin/users").with(authentication(auth))) .andExpect(status().isOk()) .andExpect(jsonPath("$.code").value(0)) .andExpect(jsonPath("$.data.items").isArray()) - .andExpect(jsonPath("$.data.total").value(2)); + .andExpect(jsonPath("$.data.total").value(1)) + .andExpect(jsonPath("$.data.items[0].id").value("user-1")) + .andExpect(jsonPath("$.data.items[0].email").value("alice@example.com")) + .andExpect(jsonPath("$.data.items[0].platformRoles[0]").value("AUDITOR")); } @Test @@ -72,6 +97,9 @@ class UserManagementControllerTest { principal, null, List.of(new SimpleGrantedAuthority("ROLE_SUPER_ADMIN")) ); + when(adminUserAppService.listUsers(null, null, 0, 20)) + .thenReturn(new PageResponse<>(List.of(), 0, 0, 20)); + mockMvc.perform(get("/api/v1/admin/users").with(authentication(auth))) .andExpect(status().isOk()) .andExpect(jsonPath("$.data.items").isArray()); @@ -88,6 +116,9 @@ class UserManagementControllerTest { String requestBody = "{\"role\":\"MODERATOR\"}"; + when(adminUserAppService.updateUserRole("user-123", "MODERATOR", Set.of("USER_ADMIN"))) + .thenReturn(new AdminUserMutationResponse("user-123", "MODERATOR", "ACTIVE")); + mockMvc.perform(put("/api/v1/admin/users/user-123/role") .with(authentication(auth)) .with(csrf()) @@ -96,7 +127,8 @@ class UserManagementControllerTest { .andExpect(status().isOk()) .andExpect(jsonPath("$.code").value(0)) .andExpect(jsonPath("$.data.userId").value("user-123")) - .andExpect(jsonPath("$.data.role").value("MODERATOR")); + .andExpect(jsonPath("$.data.role").value("MODERATOR")) + .andExpect(jsonPath("$.data.status").value("ACTIVE")); } @Test @@ -108,7 +140,10 @@ class UserManagementControllerTest { principal, null, List.of(new SimpleGrantedAuthority("ROLE_USER_ADMIN")) ); - String requestBody = "{\"status\":\"BANNED\"}"; + String requestBody = "{\"status\":\"DISABLED\"}"; + + when(adminUserAppService.updateUserStatus("user-123", "DISABLED")) + .thenReturn(new AdminUserMutationResponse("user-123", null, "DISABLED")); mockMvc.perform(put("/api/v1/admin/users/user-123/status") .with(authentication(auth)) @@ -118,6 +153,6 @@ class UserManagementControllerTest { .andExpect(status().isOk()) .andExpect(jsonPath("$.code").value(0)) .andExpect(jsonPath("$.data.userId").value("user-123")) - .andExpect(jsonPath("$.data.status").value("BANNED")); + .andExpect(jsonPath("$.data.status").value("DISABLED")); } } diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/AdminAuditLogAppServiceTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/AdminAuditLogAppServiceTest.java new file mode 100644 index 00000000..5f2d64cc --- /dev/null +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/AdminAuditLogAppServiceTest.java @@ -0,0 +1,44 @@ +package com.iflytek.skillhub.service; + +import com.iflytek.skillhub.dto.AuditLogItemResponse; +import com.iflytek.skillhub.dto.PageResponse; +import org.junit.jupiter.api.Test; +import org.springframework.jdbc.core.RowMapper; +import org.springframework.jdbc.core.namedparam.MapSqlParameterSource; +import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate; + +import java.time.Instant; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; + +class AdminAuditLogAppServiceTest { + + private final NamedParameterJdbcTemplate jdbcTemplate = mock(NamedParameterJdbcTemplate.class); + private final AdminAuditLogAppService service = new AdminAuditLogAppService(jdbcTemplate); + + @Test + void listAuditLogs_returnsJdbcBackedPage() { + when(jdbcTemplate.queryForObject(contains("COUNT(*)"), any(MapSqlParameterSource.class), eq(Long.class))) + .thenReturn(1L); + when(jdbcTemplate.query(contains("FROM audit_log"), any(MapSqlParameterSource.class), any(RowMapper.class))) + .thenReturn(List.of(new AuditLogItemResponse( + 1L, + "USER_STATUS_CHANGE", + "user-1", + "alice", + "{\"status\":\"DISABLED\"}", + "127.0.0.1", + Instant.parse("2026-03-13T01:00:00Z") + ))); + + PageResponse response = service.listAuditLogs(0, 20, "user-1", "USER_STATUS_CHANGE"); + + assertThat(response.total()).isEqualTo(1); + assertThat(response.items()).hasSize(1); + verify(jdbcTemplate).queryForObject(contains("al.actor_user_id = :userId"), any(MapSqlParameterSource.class), eq(Long.class)); + verify(jdbcTemplate).query(contains("al.action = :action"), any(MapSqlParameterSource.class), any(RowMapper.class)); + } +} diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/AdminUserAppServiceTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/AdminUserAppServiceTest.java new file mode 100644 index 00000000..03b7b853 --- /dev/null +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/AdminUserAppServiceTest.java @@ -0,0 +1,147 @@ +package com.iflytek.skillhub.service; + +import com.iflytek.skillhub.auth.entity.Role; +import com.iflytek.skillhub.auth.entity.UserRoleBinding; +import com.iflytek.skillhub.auth.repository.RoleRepository; +import com.iflytek.skillhub.auth.repository.UserRoleBindingRepository; +import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException; +import com.iflytek.skillhub.domain.shared.exception.DomainForbiddenException; +import com.iflytek.skillhub.domain.shared.exception.DomainNotFoundException; +import com.iflytek.skillhub.domain.user.UserAccount; +import com.iflytek.skillhub.domain.user.UserAccountRepository; +import com.iflytek.skillhub.domain.user.UserStatus; +import com.iflytek.skillhub.dto.PageResponse; +import com.iflytek.skillhub.repository.AdminUserSearchRepository; +import org.junit.jupiter.api.Test; +import org.springframework.data.domain.PageImpl; +import org.springframework.data.domain.PageRequest; +import org.springframework.data.domain.Sort; +import org.springframework.test.util.ReflectionTestUtils; + +import java.time.LocalDateTime; +import java.util.List; +import java.util.Optional; +import java.util.Set; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.*; + +class AdminUserAppServiceTest { + + private final AdminUserSearchRepository adminUserSearchRepository = mock(AdminUserSearchRepository.class); + private final UserRoleBindingRepository userRoleBindingRepository = mock(UserRoleBindingRepository.class); + private final RoleRepository roleRepository = mock(RoleRepository.class); + private final UserAccountRepository userAccountRepository = mock(UserAccountRepository.class); + private final AdminUserAppService service = new AdminUserAppService( + adminUserSearchRepository, + userAccountRepository, + userRoleBindingRepository, + roleRepository + ); + + @Test + void listUsers_returnsPagedUsersFromRepository() { + UserAccount user = user("user-1", "alice", "alice@example.com", UserStatus.ACTIVE); + PageRequest pageable = PageRequest.of(0, 20, Sort.by(Sort.Direction.DESC, "createdAt")); + when(adminUserSearchRepository.search("ali", UserStatus.ACTIVE, pageable)) + .thenReturn(new PageImpl<>(List.of(user), pageable, 1)); + when(userRoleBindingRepository.findByUserIdIn(List.of("user-1"))) + .thenReturn(List.of(new UserRoleBinding("user-1", role("AUDITOR")))); + + PageResponse response = service.listUsers("ali", "ACTIVE", 0, 20); + + assertThat(response.total()).isEqualTo(1); + assertThat(response.items()).hasSize(1); + assertThat(response.items().get(0)).extracting("id", "username", "email", "status") + .containsExactly("user-1", "alice", "alice@example.com", "ACTIVE"); + assertThat(response.items().get(0)).extracting("platformRoles") + .isEqualTo(List.of("AUDITOR")); + } + + @Test + void listUsers_withInvalidStatus_throwsBadRequest() { + assertThrows(DomainBadRequestException.class, () -> service.listUsers(null, "BANNED", 0, 20)); + } + + @Test + void updateUserRole_nonSuperAdminCannotAssignSuperAdmin() { + when(userAccountRepository.findById("user-1")) + .thenReturn(Optional.of(user("user-1", "alice", "alice@example.com", UserStatus.ACTIVE))); + + assertThrows(DomainForbiddenException.class, + () -> service.updateUserRole("user-1", "SUPER_ADMIN", Set.of("USER_ADMIN"))); + } + + @Test + void updateUserRole_replacesExistingBindings() { + when(userAccountRepository.findById("user-1")) + .thenReturn(Optional.of(user("user-1", "alice", "alice@example.com", UserStatus.ACTIVE))); + when(roleRepository.findByCode("AUDITOR")).thenReturn(Optional.of(role("AUDITOR"))); + + var response = service.updateUserRole("user-1", "AUDITOR", Set.of("SUPER_ADMIN")); + + verify(userRoleBindingRepository).deleteByUserId("user-1"); + verify(userRoleBindingRepository).save(any(UserRoleBinding.class)); + assertThat(response.userId()).isEqualTo("user-1"); + assertThat(response.role()).isEqualTo("AUDITOR"); + assertThat(response.status()).isEqualTo("ACTIVE"); + } + + @Test + void updateUserRole_userPseudoRoleClearsBindingsWithoutSavingNewRole() { + when(userAccountRepository.findById("user-1")) + .thenReturn(Optional.of(user("user-1", "alice", "alice@example.com", UserStatus.ACTIVE))); + + var response = service.updateUserRole("user-1", "USER", Set.of("SUPER_ADMIN")); + + verify(userRoleBindingRepository).deleteByUserId("user-1"); + verify(userRoleBindingRepository, never()).save(any(UserRoleBinding.class)); + assertThat(response.role()).isEqualTo("USER"); + } + + @Test + void updateUserStatus_rejectsUnsupportedStatuses() { + when(userAccountRepository.findById("user-1")) + .thenReturn(Optional.of(user("user-1", "alice", "alice@example.com", UserStatus.ACTIVE))); + + assertThrows(DomainBadRequestException.class, () -> service.updateUserStatus("user-1", "MERGED")); + } + + @Test + void updateUserStatus_updatesPersistedStatus() { + UserAccount user = user("user-1", "alice", "alice@example.com", UserStatus.ACTIVE); + when(userAccountRepository.findById("user-1")).thenReturn(Optional.of(user)); + when(userAccountRepository.save(user)).thenReturn(user); + + var response = service.updateUserStatus("user-1", "DISABLED"); + + verify(userAccountRepository).save(user); + assertThat(user.getStatus()).isEqualTo(UserStatus.DISABLED); + assertThat(response.status()).isEqualTo("DISABLED"); + } + + @Test + void updateUserStatus_withUnknownUser_throwsNotFound() { + when(userAccountRepository.findById("missing")).thenReturn(Optional.empty()); + + assertThrows(DomainNotFoundException.class, () -> service.updateUserStatus("missing", "DISABLED")); + } + + private UserAccount user(String id, String displayName, String email, UserStatus status) { + UserAccount user = new UserAccount(id, displayName, email, null); + user.setStatus(status); + ReflectionTestUtils.setField(user, "createdAt", LocalDateTime.of(2026, 3, 13, 9, 0)); + ReflectionTestUtils.setField(user, "updatedAt", LocalDateTime.of(2026, 3, 13, 9, 0)); + return user; + } + + private Role role(String code) { + Role role = new Role(); + ReflectionTestUtils.setField(role, "code", code); + ReflectionTestUtils.setField(role, "name", code); + return role; + } +} diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/repository/UserRoleBindingRepository.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/repository/UserRoleBindingRepository.java index 76aeaf6b..def4ad3b 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/repository/UserRoleBindingRepository.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/repository/UserRoleBindingRepository.java @@ -3,9 +3,13 @@ package com.iflytek.skillhub.auth.repository; import com.iflytek.skillhub.auth.entity.UserRoleBinding; import org.springframework.data.jpa.repository.JpaRepository; import org.springframework.stereotype.Repository; + +import java.util.Collection; import java.util.List; @Repository public interface UserRoleBindingRepository extends JpaRepository { List findByUserId(String userId); + List findByUserIdIn(Collection userIds); + void deleteByUserId(String userId); } diff --git a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/UserAccountJpaRepository.java b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/UserAccountJpaRepository.java index 4f6e9eb7..c7dcc3ca 100644 --- a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/UserAccountJpaRepository.java +++ b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/UserAccountJpaRepository.java @@ -3,9 +3,10 @@ package com.iflytek.skillhub.infra.jpa; import com.iflytek.skillhub.domain.user.UserAccount; import com.iflytek.skillhub.domain.user.UserAccountRepository; import org.springframework.data.jpa.repository.JpaRepository; +import org.springframework.data.jpa.repository.JpaSpecificationExecutor; import org.springframework.stereotype.Repository; @Repository public interface UserAccountJpaRepository - extends JpaRepository, UserAccountRepository { + extends JpaRepository, JpaSpecificationExecutor, UserAccountRepository { } From 556d556724706d8556599cd5419e659492f32d37 Mon Sep 17 00:00:00 2001 From: yun-zhi-ztl <15071461069@163.com> Date: Fri, 13 Mar 2026 11:49:57 +0800 Subject: [PATCH 09/13] fix(token): align revoke endpoint with 204 contract Change DELETE /api/v1/tokens/{id} to return HTTP 204 No Content so the backend matches the existing OpenAPI contract and the frontend delete flow no longer rejects successful revocations. Add a controller regression test that verifies the endpoint returns 204 with an empty body and still delegates the revoke call to ApiTokenService. Verified with the targeted TokenControllerTest plus full server mvn test. --- .../skillhub/controller/TokenController.java | 6 +- .../controller/TokenControllerTest.java | 64 +++++++++++++++++++ 2 files changed, 67 insertions(+), 3 deletions(-) create mode 100644 server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/TokenControllerTest.java diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/TokenController.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/TokenController.java index d1b8420b..cb2af098 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/TokenController.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/TokenController.java @@ -4,11 +4,11 @@ import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; import com.iflytek.skillhub.auth.token.ApiTokenService; import com.iflytek.skillhub.dto.ApiResponse; import com.iflytek.skillhub.dto.ApiResponseFactory; -import com.iflytek.skillhub.dto.MessageResponse; import com.iflytek.skillhub.dto.TokenCreateRequest; import com.iflytek.skillhub.dto.TokenCreateResponse; import com.iflytek.skillhub.dto.TokenSummaryResponse; import jakarta.validation.Valid; +import org.springframework.http.ResponseEntity; import org.springframework.security.core.annotation.AuthenticationPrincipal; import org.springframework.web.bind.annotation.*; @@ -59,10 +59,10 @@ public class TokenController extends BaseApiController { } @DeleteMapping("/{id}") - public ApiResponse revoke( + public ResponseEntity revoke( @AuthenticationPrincipal PlatformPrincipal principal, @PathVariable Long id) { apiTokenService.revokeToken(id, principal.userId()); - return ok("response.success.revoked", new MessageResponse("Token revoked")); + return ResponseEntity.noContent().build(); } } diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/TokenControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/TokenControllerTest.java new file mode 100644 index 00000000..52b98554 --- /dev/null +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/TokenControllerTest.java @@ -0,0 +1,64 @@ +package com.iflytek.skillhub.controller; + +import com.iflytek.skillhub.TestRedisConfig; +import com.iflytek.skillhub.auth.device.DeviceAuthService; +import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; +import com.iflytek.skillhub.auth.token.ApiTokenService; +import com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.context.annotation.Import; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; +import org.springframework.security.core.authority.SimpleGrantedAuthority; +import org.springframework.test.context.ActiveProfiles; +import org.springframework.test.web.servlet.MockMvc; + +import java.util.List; +import java.util.Set; + +import static org.mockito.Mockito.verify; +import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.authentication; +import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +@SpringBootTest +@AutoConfigureMockMvc +@ActiveProfiles("test") +@Import(TestRedisConfig.class) +class TokenControllerTest { + + @Autowired + private MockMvc mockMvc; + + @MockBean + private NamespaceMemberRepository namespaceMemberRepository; + + @MockBean + private DeviceAuthService deviceAuthService; + + @MockBean + private ApiTokenService apiTokenService; + + @Test + void revoke_returns204NoContent() throws Exception { + PlatformPrincipal principal = new PlatformPrincipal( + "user-42", "tester", "tester@example.com", "", "github", Set.of("USER") + ); + var auth = new UsernamePasswordAuthenticationToken( + principal, null, List.of(new SimpleGrantedAuthority("ROLE_USER")) + ); + + mockMvc.perform(delete("/api/v1/tokens/7") + .with(authentication(auth)) + .with(csrf())) + .andExpect(status().isNoContent()) + .andExpect(content().string("")); + + verify(apiTokenService).revokeToken(7L, "user-42"); + } +} From 9021ba754efab5402353a3030701b856e627b53c Mon Sep 17 00:00:00 2001 From: yun-zhi-ztl <15071461069@163.com> Date: Fri, 13 Mar 2026 11:54:47 +0800 Subject: [PATCH 10/13] fix(web): clear lint gate and split route bundles Remove the markdown renderer ts-ignore workaround by applying prose styling at the container level, which clears the remaining lint failure without changing rendered behavior. Convert top-level pages to route-level lazy imports with a shared suspense fallback so the main bundle is no longer forced to include every page upfront. Verified with pnpm run lint, pnpm run typecheck, and pnpm run build; the entry chunk dropped from roughly 770 kB to 338.77 kB after the split. --- web/src/app/router.tsx | 80 ++++++++++++++++---- web/src/features/skill/markdown-renderer.tsx | 16 ++-- 2 files changed, 71 insertions(+), 25 deletions(-) diff --git a/web/src/app/router.tsx b/web/src/app/router.tsx index 819ac1d8..ff0e6f94 100644 --- a/web/src/app/router.tsx +++ b/web/src/app/router.tsx @@ -1,22 +1,72 @@ +import { lazy, Suspense, type ComponentType } from 'react' import { createRouter, createRoute, createRootRoute, redirect } from '@tanstack/react-router' import { Layout } from './layout' -import { HomePage } from '@/pages/home' -import { LoginPage } from '@/pages/login' -import { DashboardPage } from '@/pages/dashboard' -import { SearchPage } from '@/pages/search' -import { NamespacePage } from '@/pages/namespace' -import { SkillDetailPage } from '@/pages/skill-detail' -import { PublishPage } from '@/pages/dashboard/publish' -import { MySkillsPage } from '@/pages/dashboard/my-skills' -import { MyNamespacesPage } from '@/pages/dashboard/my-namespaces' -import { NamespaceMembersPage } from '@/pages/dashboard/namespace-members' -import { ReviewsPage } from '@/pages/dashboard/reviews' -import { ReviewDetailPage } from '@/pages/dashboard/review-detail' -import { DeviceAuthPage } from '@/pages/device' -import { AdminUsersPage } from '@/pages/admin/users' -import { AuditLogPage } from '@/pages/admin/audit-log' import { getCurrentUser } from '@/api/client' +function createLazyRouteComponent(load: () => Promise<{ default: ComponentType }>) { + const LazyComponent = lazy(load) + + return function LazyRouteComponent(props: Record) { + return ( + + Loading... + + } + > + + + ) + } +} + +const HomePage = createLazyRouteComponent(() => + import('@/pages/home').then((module) => ({ default: module.HomePage })), +) +const LoginPage = createLazyRouteComponent(() => + import('@/pages/login').then((module) => ({ default: module.LoginPage })), +) +const DashboardPage = createLazyRouteComponent(() => + import('@/pages/dashboard').then((module) => ({ default: module.DashboardPage })), +) +const SearchPage = createLazyRouteComponent(() => + import('@/pages/search').then((module) => ({ default: module.SearchPage })), +) +const NamespacePage = createLazyRouteComponent(() => + import('@/pages/namespace').then((module) => ({ default: module.NamespacePage })), +) +const SkillDetailPage = createLazyRouteComponent(() => + import('@/pages/skill-detail').then((module) => ({ default: module.SkillDetailPage })), +) +const PublishPage = createLazyRouteComponent(() => + import('@/pages/dashboard/publish').then((module) => ({ default: module.PublishPage })), +) +const MySkillsPage = createLazyRouteComponent(() => + import('@/pages/dashboard/my-skills').then((module) => ({ default: module.MySkillsPage })), +) +const MyNamespacesPage = createLazyRouteComponent(() => + import('@/pages/dashboard/my-namespaces').then((module) => ({ default: module.MyNamespacesPage })), +) +const NamespaceMembersPage = createLazyRouteComponent(() => + import('@/pages/dashboard/namespace-members').then((module) => ({ default: module.NamespaceMembersPage })), +) +const ReviewsPage = createLazyRouteComponent(() => + import('@/pages/dashboard/reviews').then((module) => ({ default: module.ReviewsPage })), +) +const ReviewDetailPage = createLazyRouteComponent(() => + import('@/pages/dashboard/review-detail').then((module) => ({ default: module.ReviewDetailPage })), +) +const DeviceAuthPage = createLazyRouteComponent(() => + import('@/pages/device').then((module) => ({ default: module.DeviceAuthPage })), +) +const AdminUsersPage = createLazyRouteComponent(() => + import('@/pages/admin/users').then((module) => ({ default: module.AdminUsersPage })), +) +const AuditLogPage = createLazyRouteComponent(() => + import('@/pages/admin/audit-log').then((module) => ({ default: module.AuditLogPage })), +) + const rootRoute = createRootRoute({ component: Layout, }) diff --git a/web/src/features/skill/markdown-renderer.tsx b/web/src/features/skill/markdown-renderer.tsx index 7480c2bb..32b919e5 100644 --- a/web/src/features/skill/markdown-renderer.tsx +++ b/web/src/features/skill/markdown-renderer.tsx @@ -7,17 +7,13 @@ interface MarkdownRendererProps { } export function MarkdownRenderer({ content, className }: MarkdownRendererProps) { + const containerClassName = [className, 'prose prose-sm max-w-none dark:prose-invert'] + .filter(Boolean) + .join(' ') + return ( -
-
, - }} - > - {content} - +
+ {content}
) } From 644fc6aa431d9d85bdac06d298e0a7103c6a6b7e Mon Sep 17 00:00:00 2001 From: yun-zhi-ztl <15071461069@163.com> Date: Fri, 13 Mar 2026 13:20:58 +0800 Subject: [PATCH 11/13] fix --- .gitignore | 1 + docs/review/代码审查报告.md | 363 -------------------------------- docs/review/后期优化报告.md | 405 ------------------------------------ 3 files changed, 1 insertion(+), 768 deletions(-) delete mode 100644 docs/review/代码审查报告.md delete mode 100644 docs/review/后期优化报告.md diff --git a/.gitignore b/.gitignore index 2129c847..ed773263 100644 --- a/.gitignore +++ b/.gitignore @@ -65,3 +65,4 @@ __pycache__/ # Superpowers (AI planning artifacts) docs/superpowers/ +docs/review/ diff --git a/docs/review/代码审查报告.md b/docs/review/代码审查报告.md deleted file mode 100644 index 0bb93718..00000000 --- a/docs/review/代码审查报告.md +++ /dev/null @@ -1,363 +0,0 @@ -# SkillHub 代码审查报告 - -## 1. 审查结论 - -当前版本不建议直接作为后续多人并行开发与生产化落地的基线。 - -结论等级：`有条件不通过` - -阻塞原因主要集中在以下几类： - -1. 认证与鉴权存在高风险缺陷，部分接口可被越权使用，设备码登录实现还存在可伪造令牌问题。 -2. 发布状态机与设计文档不一致，`latest` 语义、搜索事件、下载链路会被未审核版本污染。 -3. 上传与存储链路缺少路径安全和流式限制，存在路径穿越、内存耗尽、脏对象遗留等风险。 -4. 自动化质量门禁未闭环，`skillhub-app` 集成测试无法启动，前端 lint 也未通过。 - -## 2. 审查范围 - -本次重点审查了以下内容： - -- 设计文档： - - `docs/01-system-architecture.md` - - `docs/03-authentication-design.md` - - `docs/05-business-flows.md` - - `docs/06-api-design.md` - - `docs/07-skill-protocol.md` - - `docs/09-deployment.md` -- 后端核心代码： - - 认证与安全：`skillhub-auth` - - 发布、查询、下载、评审、提升：`skillhub-domain` + `skillhub-app` - - 搜索：`skillhub-search` - - 存储：`skillhub-storage` -- 前端核心代码： - - `web/src/features/skill/markdown-renderer.tsx` - -本次执行的关键校验： - -- `pnpm run typecheck`：通过 -- `pnpm run lint`：失败 -- `pnpm run build`：通过，但主包体积告警 -- `mvn -q -DskipTests compile`：通过 -- `mvn test`：失败，`skillhub-app` 24 个测试里 19 个错误 - -## 3. 关键发现 - -### [P0] 设备码登录当前会返回可预测的伪令牌，并且存在并发重复签发风险 - -问题位置： - -- `server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/device/DeviceAuthService.java:85-111` -- `server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/SecurityConfig.java:64-75` -- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/DeviceAuthController.java:21-29` - -问题说明： - -- `pollToken()` 在设备码授权成功后直接返回 `token_ + deviceCode`，这不是签名令牌，也不是随机不透明令牌，而是由外部输入可推导得到的占位值。 -- `/api/v1/cli/auth/device/**` 被 `permitAll()` 放开，意味着设备流轮询端点是匿名可访问的，这本身没问题，但前提是返回的必须是正式、不可伪造、可审计、可吊销的令牌。 -- `pollToken()` 采用“先读状态，再写 USED”的非原子流程。两个并发轮询请求可以同时读到 `AUTHORIZED`，从而重复获取访问令牌。 - -影响： - -- 这是认证链路上的核心安全缺陷，风险等级为阻塞上线。 -- 设备授权一旦成功，令牌可被预测，且一次授权可能被多次兑换。 - -修改建议： - -1. 设备流成功后必须调用正式的 token 签发服务，返回随机 opaque token 或签名 JWT。 -2. 设备码消费必须改成 Redis Lua / CAS 方式，保证“仅成功兑换一次”。 -3. 为设备码申请、授权、轮询增加限流和审计。 -4. 补充成功兑换、重复兑换、并发轮询、过期轮询的集成测试。 - -### [P1] 评审与提升流程缺失关键权限校验，已形成越权入口 - -问题位置： - -- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/ReviewController.java:56-64` -- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/ReviewService.java:50-68` -- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/PromotionController.java:54-61` -- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/PromotionService.java:47-78` -- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/ReviewController.java:102-126` -- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/PromotionController.java:86-105` - -问题说明： - -- `submitReview()` 只根据 `skillVersionId` 和当前用户提交，不校验提交人是否为该 skill 的 owner、namespace 管理员或被允许的发布者。 -- `submitPromotion()` 只校验源版本已发布、目标 namespace 是 GLOBAL，不校验申请人是否有权提升该 skill。 -- `listPendingReviews()`、`getReviewDetail()`、`getPromotionDetail()` 缺少明确授权判断，当前只要已登录即可读取相关数据，存在流程信息泄露。 - -与设计文档冲突： - -- `docs/05-business-flows.md:104-119` 明确要求“owner 或 namespace admin”才能发起提升。 -- `docs/06-api-design.md` 中 review / promotion 相关接口的权限边界也比当前实现更严格。 - -影响： - -- 任意登录用户理论上可以替别人的草稿发起评审。 -- 任意登录用户理论上可以为别人的 skill 发起提升申请。 -- 待审核数据与审批详情对无关用户暴露。 - -修改建议： - -1. 在 `ReviewService.submitReview()` 前补 owner / namespace ADMIN/OWNER 校验。 -2. 在 `PromotionService.submitPromotion()` 前补 source skill 所属 namespace 的提交权限校验。 -3. `listPendingReviews()`、`getReviewDetail()`、`getPromotionDetail()` 必须按 namespace 或平台角色做访问控制，未授权时返回 `403`，不能返回空列表掩盖问题。 - -### [P1] 发布流程在审核前就覆盖 `latestVersionId` 并发出 `SkillPublishedEvent`，会污染下载、解析、搜索和标签语义 - -问题位置： - -- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java:143-156` -- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java:217-225` -- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillDownloadService.java:68-75` -- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillQueryService.java:106-124` -- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillQueryService.java:344-351` -- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillTagService.java:44-47` -- `server/skillhub-search/src/main/java/com/iflytek/skillhub/search/event/SearchIndexEventListener.java:26-39` - -问题说明： - -- `publishFromEntries()` 创建的新版本状态是 `PENDING_REVIEW`，但随后立即： - - 更新 `skill.latestVersionId` - - 发出 `SkillPublishedEvent` -- `downloadLatest()`、`resolveLatestVersion()`、`latest` 保留标签、搜索结果里的 latest version 都依赖 `latestVersionId`。 -- 当一个已发布 skill 再上传新版本时，`latestVersionId` 会被指向一个尚未发布的版本，导致“最新版本”不可下载、不可解析或显示错误。 - -与设计文档冲突： - -- `docs/05-business-flows.md:30-45` 的 Phase 2 定义是“发布后直接 `PUBLISHED` 才更新 latest”。 -- `docs/05-business-flows.md:49-55` 的 Phase 3 定义是“`DRAFT -> PENDING_REVIEW -> PUBLISHED`”，审核通过后才进入发布态。 -- 当前代码实际做成了“创建后直接 `PENDING_REVIEW`，但又按已发布版本处理”，状态语义前后矛盾。 - -影响： - -- `latest` 下载链路会在有待审版本时报错。 -- 搜索索引会被过早重建。 -- 详情页、我的技能页、标签查询都会看到未发布版本。 - -修改建议： - -1. 冻结状态机语义： - - 要么 Phase 2：发布即 `PUBLISHED` - - 要么 Phase 3：上传只创建草稿 / 待审，审核通过后才更新 latest -2. `latestVersionId` 必须只表示“最新已发布版本”。 -3. `SkillPublishedEvent` 只能在版本真正进入 `PUBLISHED` 后发出。 -4. 若需要展示“最新草稿”，单独引入 `latestDraftVersionId` 或查询逻辑，不要复用 published 语义字段。 - -### [P1] 上传链路存在路径穿越与压缩包资源耗尽风险 - -问题位置： - -- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillPublishController.java:64-83` -- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/SkillPackageValidator.java:28-81` -- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java:166-189` -- `server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/LocalFileStorageService.java:21-31` -- `server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/LocalFileStorageService.java:61` - -问题说明： - -- Controller 侧对 zip entry 使用 `readAllBytes()`，在验证之前就把每个解压后文件整体读入内存。 -- 包校验器没有校验路径规范化，也没有禁止 `../`、绝对路径、反斜杠路径、重复路径。 -- `LocalFileStorageService.resolve()` 直接 `basePath.resolve(key)`，没有 `normalize()` 和 `startsWith(basePath)` 校验。 -- 攻击者可以构造恶意 zip 路径，例如 `../../outside.txt`，通过 `skills/{skillId}/{versionId}/{filePath}` 最终逃逸本地存储根目录。 - -影响： - -- 本地存储模式下存在任意文件写入风险。 -- 恶意压缩包可造成内存压力甚至进程 OOM。 - -修改建议： - -1. 在解压阶段引入流式校验，限制总解压大小、单文件大小、文件数量。 -2. 对 entry path 做统一规范化，只允许： - - 相对路径 - - `/` 分隔 - - 禁止 `..`、禁止绝对路径、禁止空段 -3. `LocalFileStorageService` 必须在 `normalize()` 后校验目标路径仍位于 `basePath` 内。 -4. 对重复路径在校验阶段直接返回 `400`，不能等数据库唯一键报错。 - -### [P1] `GET /api/v1/skills/**` 过度放开，导致匿名 `500` 与私有元数据泄露 - -问题位置： - -- `server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/SecurityConfig.java:75` -- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillStarController.java:40-45` -- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillRatingController.java:37-46` -- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillController.java:73-94` -- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillTagController.java:29-40` - -问题说明： - -- `SecurityConfig` 直接放开了所有 `GET /api/v1/skills/**`。 -- 但其中至少两类 GET 并不适合匿名访问： - - `GET /api/v1/skills/{skillId}/star` - - `GET /api/v1/skills/{skillId}/rating` - 这两个接口直接解引用 `principal.userId()`，匿名访问会触发空指针，落到全局异常后返回 `500`。 -- 另外： - - `listVersions()` 没有 visibility 校验 - - `listTags()` 没有 visibility 校验 - 这会把 `PRIVATE` / `NAMESPACE_ONLY` skill 的版本信息、标签信息暴露给匿名或无关用户。 - -影响： - -- 安全上属于“鉴权边界与路由策略不一致”。 -- 行为上会出现匿名访问 500，破坏 API 契约。 - -修改建议： - -1. 不要用 `GET /api/v1/skills/**` 一刀切放开，改成白名单到具体公共接口。 -2. 对“需要当前用户上下文”的 GET 接口显式要求认证。 -3. `listVersions()`、`listTags()` 必须补充与详情接口一致的 visibility 校验。 - -### [P1] `skillhub-app` 集成测试当前无法启动，测试信号失真 - -问题位置： - -- `server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/device/DeviceAuthService.java:19-25` -- `server/skillhub-app/src/test/resources/application-test.yml:1-33` -- `server/skillhub-app/target/surefire-reports/com.iflytek.skillhub.controller.AuthControllerTest.txt` - -问题说明： - -- `mvn test` 中，`skillhub-storage`、`skillhub-domain`、`skillhub-auth` 的测试通过，但 `skillhub-app` 的 Spring 上下文无法启动。 -- 直接原因是 `DeviceAuthService` 依赖 `RedisTemplate`，测试环境没有对应 bean。 -- 代码里也没有看到 `verificationUri` 的配置注入实现，当前构造器还要求额外的 `String` 参数，后续即使补齐 Redis bean，也大概率还会继续失败。 - -影响： - -- 目前 controller 层测试的 19 个错误都被同一个装配问题掩盖，真实回归无法被发现。 -- CI 无法提供有效的回归保护。 - -修改建议： - -1. 为设备码服务补全正式配置类： - - `RedisTemplate` - - `@ConfigurationProperties` 形式的 `verificationUri` -2. 测试环境提供 stub / mock bean，确保 app context 可启动。 -3. 优先修复后重新运行 controller 集成测试，再评估剩余问题。 - -### [P2] 存储配置键名与部署文档漂移，S3/MinIO 路径当前并不能真正启用 - -问题位置： - -- `server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/StorageProperties.java:7-15` -- `server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/LocalFileStorageService.java:12-14` -- `server/skillhub-storage/src/main/java/com/iflytek/skillhub/storage/S3StorageService.java:19-21` -- `server/skillhub-app/src/main/resources/application.yml:55-58` -- `docker-compose.prod.yml:51-59` -- `docs/09-deployment.md` - -问题说明： - -- 代码使用的配置键是 `skillhub.storage.provider`。 -- `application.yml` 写的是 `skillhub.storage.type`。 -- `docker-compose.prod.yml` 也没有把 S3/MinIO 相关配置注入到后端容器。 - -影响： - -- 即使文档和运维层面按 MinIO/S3 部署，也无法通过当前配置切换到 `S3StorageService`。 -- 实际运行会默认为本地文件存储，和部署文档不一致。 - -修改建议： - -1. 统一配置键，只保留一种： - - 推荐 `skillhub.storage.provider` -2. 修正文档、`application.yml`、`docker-compose.prod.yml`、`StorageProperties` 的一致性。 -3. 增加一个启动时自检日志，打印当前启用的 storage provider。 - -### [P2] 上传大小与文件白名单配置未真正落地，当前仍是硬编码 - -问题位置： - -- `server/skillhub-app/src/main/resources/application.yml:47-64` -- `server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/validation/SkillPackageValidator.java:12-20` - -问题说明： - -- 配置文件声明： - - `multipart` 允许到 `100MB` - - `skillhub.publish.max-package-size` 为 `100MB` - - `allowed-file-extensions` 可配置 -- 但实际校验器仍然硬编码： - - 总包 `10MB` - - 固定扩展名集合 - -影响： - -- 文档、配置、运行时行为三者不一致。 -- 运维或产品侧修改配置后不会生效，问题定位成本高。 - -修改建议： - -1. 将 `SkillPackageValidator` 改为读取 `@ConfigurationProperties`。 -2. 保持“网关上限”“multipart 上限”“业务校验上限”三层配置含义清晰并一致。 - -### [P2] 资源不存在时多处直接 `orElseThrow()`，会把正常 404 场景放大成 500 - -问题位置： - -- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/PromotionController.java:101-111` -- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/ReviewController.java:60-62` -- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/ReviewController.java:97` -- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/ReviewController.java:123-132` -- `server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/portal/SkillRatingController.java:30-33` - -问题说明： - -- 多个 controller 直接使用裸 `orElseThrow()`。 -- 当资源不存在时会抛出 `NoSuchElementException`，最后被全局异常处理器转成 `500`。 -- `SkillRatingController` 还存在 `score` 缺失时的空指针风险。 - -影响： - -- API 契约不稳定，客户端会把普通业务错误当成服务故障。 - -修改建议： - -1. 统一抛出领域层 `NotFound/BadRequest` 异常。 -2. 给评分、评审、提升请求增加 `@Valid` 和显式字段校验。 - -### [P2] 前端质量门禁未闭环，lint 未通过，产物主包偏大 - -问题位置： - -- `web/src/features/skill/markdown-renderer.tsx:15-16` - -问题说明： - -- 当前 `eslint` 失败： - - `@ts-ignore` 应改为 `@ts-expect-error` - - `node` 参数未使用 -- `pnpm run build` 虽能通过，但主 JS chunk 约 `751.25 kB`，明显偏大。 - -影响： - -- CI 无法建立严格前端门禁。 -- 首屏加载与缓存更新成本偏高。 - -修改建议： - -1. 先修复 lint 报错，恢复前端静态检查门禁。 -2. 后续按页面级或功能级拆包，优先处理 markdown/highlight、管理页、上传页等非首屏模块。 - -## 4. 与设计文档的主要偏差 - -当前代码与文档的主要偏差有： - -1. `docs/05-business-flows.md` 对 Phase 2 / Phase 3 的发布状态定义，与 `SkillPublishService` 现状不一致。 -2. 文档里定义了幂等、审计、孤儿对象清理、异步事件兜底，但代码里尚未真正实现。 -3. 文档要求的权限边界比当前 review / promotion / skill GET 路由更严格，当前实现明显偏松。 -4. 部署文档强调 MinIO / S3 可切换，但配置层并未打通。 - -## 5. 现阶段是否可进入开发 - -可以继续开发，但不建议直接进入“多人并行开发 + 联调 + 准生产验证”阶段。 - -建议先完成以下最小修复集： - -1. 修复设备码认证实现与 app 测试装配。 -2. 收紧 review / promotion / skills GET 的鉴权边界。 -3. 修正发布状态机，保证 `latestVersionId` 只指向已发布版本。 -4. 修复 zip 路径校验和本地存储路径归一化。 -5. 打通前后端质量门禁：`mvn test`、`pnpm lint`、`pnpm build` 全绿。 - -在这五项完成之前，后续功能开发会持续叠加在一个不稳定基线上，返工概率较高。 diff --git a/docs/review/后期优化报告.md b/docs/review/后期优化报告.md deleted file mode 100644 index b8a07694..00000000 --- a/docs/review/后期优化报告.md +++ /dev/null @@ -1,405 +0,0 @@ -# SkillHub 后期优化报告 - -## 1. 总体判断 - -SkillHub 当前已经具备“能跑起来的模块化单体”雏形，分层方向是对的，但距离“可持续迭代、可对外开放、可灰度发布”的工程化状态还有明显差距。 - -后续优化应分成两类： - -1. `阻塞型修复`：先把安全、鉴权、状态机、测试基线修好。 -2. `架构型收敛`：把文档、配置、协议、模块边界收敛成一个长期稳定的实现。 - -下面的建议默认以“后续要继续实际代码开发”为前提，而不是只做文档美化。 - -## 2. 优先级建议 - -### T0：1 周内必须完成 - -1. 修复设备码登录： - - 正式 token 签发 - - 原子消费 - - Redis bean / 配置注入补齐 -2. 修复 review / promotion / skills GET 的权限边界。 -3. 修复发布状态机： - - `latestVersionId` - - `SkillPublishedEvent` - - `latest` 标签 - - 搜索重建触发时机 -4. 修复上传路径安全和解压限制。 -5. 恢复质量门禁： - - `mvn test` - - `pnpm lint` - - `pnpm build` - -### T1：2 到 3 周内完成 - -1. 冻结配置协议与部署方式： - - storage provider - - S3 / MinIO - - publish 限制配置 -2. 完成评审、提升、下载、搜索的权限策略统一。 -3. 落地幂等与审计最小版。 -4. 收敛 API 路径与 DTO 契约。 - -### T2：1 到 2 个迭代完成 - -1. 引入更稳健的异步一致性方案。 -2. 做前端拆包、性能与体验优化。 -3. 增强发布安全能力和安装完整性校验。 -4. 做更强的产品化能力，例如可视化审核、统计分析、推荐等。 - -## 3. 架构优化建议 - -### 3.1 冻结发布状态机 - -当前最需要收敛的是“发布”和“审核”到底是什么关系。 - -建议明确采用下面其中一种，不要混合： - -方案 A：Phase 2 简化模型 - -- 上传成功即 `PUBLISHED` -- 不存在 `PENDING_REVIEW` -- `latestVersionId` 直接指向新版本 - -方案 B：Phase 3 审核模型 - -- 上传只创建 `DRAFT` -- 显式提交后进入 `PENDING_REVIEW` -- 审核通过后才进入 `PUBLISHED` -- 只有 `PUBLISHED` 版本能影响： - - `latestVersionId` - - `latest` 保留标签 - - 搜索索引 - - 下载入口 - -如果选择方案 B，建议新增下面两个边界对象： - -- `PublishedVersionPointer` - - 只负责“当前可安装版本” -- `DraftVersionView` - - 只负责“作者或审核人能看到的最新草稿/待审版本” - -不要再用一个 `latestVersionId` 同时表达“最新上传版本”和“最新已发布版本”。 - -### 3.2 建立统一授权层 - -当前权限判断分散在： - -- `SecurityConfig` -- Controller -- Domain Service -- `VisibilityChecker` -- `ReviewPermissionChecker` - -建议统一成三层： - -1. `Authentication` - - 只负责身份识别 -2. `Resource Access Policy` - - 只负责“谁能看” -3. `Action Authorization Policy` - - 只负责“谁能做什么” - -推荐拆出统一的 policy 组件，例如： - -- `SkillReadPolicy` -- `SkillPublishPolicy` -- `ReviewPolicy` -- `PromotionPolicy` -- `NamespacePolicy` - -Controller 不再自己拼权限，只把上下文交给 policy。 - -### 3.3 重构上传入口为单独的 Package Ingestion 模块 - -建议把当前“解压 + 校验 + 存储 + bundle 构建 + 元数据提取”从 `SkillPublishController` / `SkillPublishService` 中抽离，形成独立应用服务： - -- `SkillPackageIngestionService` - -职责建议如下： - -- 解析 zip stream -- 路径校验 -- 大小限制 -- 重复路径校验 -- 内容类型判断 -- 哈希计算 -- 生成标准化 manifest -- 输出领域对象 `ValidatedSkillPackage` - -这样好处是： - -- Web / CLI 共享同一套上传逻辑 -- 更容易做流式处理 -- 更容易接入病毒扫描、签名验证、内容审核 - -### 3.4 引入存储补偿与异步一致性机制 - -当前对象存储与数据库事务是“两套事务”，必须承认这一点。 - -建议至少补齐两层机制： - -1. 同步补偿 - - 上传过程中一旦数据库失败，立即尝试删除已写入对象 -2. 异步兜底 - - 定时扫描 orphan object - - 定时补建搜索索引 - -如果未来继续扩展，建议走标准化方案： - -- `transactional outbox` -- 后台 worker -- 幂等消费 - -### 3.5 配置体系收敛成强类型配置 - -当前配置散落在： - -- `application.yml` -- `docker-compose*.yml` -- `@ConditionalOnProperty` -- 硬编码常量 - -建议做一次统一收敛： - -- `SkillhubStorageProperties` -- `SkillhubPublishProperties` -- `SkillhubAuthProperties` -- `SkillhubRateLimitProperties` - -并要求： - -1. 所有运行时行为只从配置类读取。 -2. 业务代码禁止继续写死阈值。 -3. 启动时打印关键配置摘要，方便排障。 - -## 4. 设计优化建议 - -### 4.1 API 路径风格统一 - -当前同时存在两种风格： - -- 面向用户的 namespace/slug 坐标 -- 面向内部的数字 ID 路径 - -建议明确区分： - -- 外部公开 API：统一用 `namespace/slug/version/tag` -- 内部管理或后台 API：可以保留数字 ID - -对外协议越稳定，前端、CLI、第三方集成越容易维护。 - -### 4.2 文档与代码冻结同一份状态机和权限矩阵 - -建议补一份真正可执行的“冻结文档”，只包含这些内容： - -1. 版本状态流转表 -2. skill 可见性矩阵 -3. review / promotion 权限矩阵 -4. token scope 矩阵 -5. 公共 GET 白名单 - -这份文档应该成为： - -- 后端开发实现基准 -- 前端联调基准 -- 测试用例来源 - -### 4.3 Token 设计升级为“角色 + scope”双约束 - -当前 API Token 更像“换一种方式拿到完整用户权限”。 - -建议升级成： - -- `identity` -- `platformRoles` -- `scopes` -- `resourceConstraints` - -例如： - -- `skill:read` -- `skill:publish` -- `review:read` -- `review:approve` -- `namespace:team-a` - -并要求 Filter 只解析身份，具体权限由下游 policy 再次判断。 - -### 4.4 设备码登录设计应独立成完整子协议 - -建议把设备流单独文档化并独立实现，最少包括： - -1. device code 生命周期 -2. user code 生命周期 -3. 轮询频率限制 -4. 授权成功后的单次兑换 -5. access token / refresh token -6. 撤销与过期处理 -7. 审计字段 - -不要把它作为“先写个占位版本”长期保留在主干里。 - -## 5. 功能优化建议 - -### 5.1 发布体验 - -建议在当前基础上补齐： - -1. 上传预检接口 - - 只做包校验，不落库 -2. 发布结果详情页 - - 显示版本状态、校验结果、审计信息 -3. 版本差异展示 - - 新旧文件列表 diff -4. bundle 指纹展示 - - 便于 CLI 做一致性校验 - -### 5.2 审核体验 - -建议增加： - -1. review 队列按 namespace / 状态 / 时间过滤 -2. promotion 队列显示来源 skill 与目标 namespace -3. 审核意见模板 -4. 审核历史轨迹 - -### 5.3 安装与下载体验 - -建议补齐： - -1. 下载返回指纹 -2. CLI 安装后校验 hash -3. “latest” 与自定义 tag 的解析提示 -4. 私有 skill 访问失败时更清晰的错误码 - -### 5.4 搜索体验 - -建议按阶段演进： - -1. 先把权限过滤、排序、分页做稳定 -2. 再补关键词高亮、标签过滤、namespace 过滤 -3. 最后再考虑向量搜索或推荐 - -先把 correctness 做扎实，比过早上复杂搜索引擎更重要。 - -## 6. 创新优化建议 - -### 6.1 增加发布安全链 - -可以在上传后增加可插拔安全扫描链： - -- 文件白名单校验 -- 敏感内容扫描 -- 恶意脚本特征扫描 -- 许可证扫描 -- 依赖清单提取 - -这部分可以通过 `PrePublishValidator` 真正扩展，而不是继续 `NoOp`。 - -### 6.2 增加制品签名与可验证安装 - -建议为 bundle 增加： - -- 版本指纹 -- 服务端签名 -- CLI 验签 - -这样 SkillHub 才更像一个可被信任的官方分发源，而不只是文件托管站。 - -### 6.3 增加“来源关系”与“派生关系”图谱 - -当前 promotion 已经有 `sourceSkillId` 雏形，后面可以扩展成： - -- fork / promote / mirror / upstream - -这能支撑： - -- 技能来源追踪 -- 派生链可视化 -- 全局版与团队版差异说明 - -## 7. 迭代优化建议 - -### 7.1 推荐实施顺序 - -第一阶段：安全与正确性收口 - -- 设备码登录 -- review / promotion 鉴权 -- skills GET 白名单 -- 上传路径安全 -- latest 语义修复 - -第二阶段：工程化基线收口 - -- 测试全绿 -- 配置统一 -- 存储 provider 打通 -- 文档与代码对齐 -- 前端 lint 与拆包 - -第三阶段：产品化能力增强 - -- 审核后台 -- 审计日志 -- 幂等与回放 -- 更完整的 CLI 体验 - -第四阶段：平台化能力增强 - -- 签名安装 -- 安全扫描 -- 推荐与搜索增强 -- 可观测性与运营报表 - -### 7.2 每阶段验收标准 - -建议引入明确验收门槛： - -第一阶段验收： - -- 不再存在 P0 / P1 安全问题 -- `mvn test` 通过 -- `pnpm lint && pnpm build` 通过 - -第二阶段验收： - -- 配置切换本地/S3 存储可实测 -- 发布/审核/下载链路与文档一致 -- 私有 skill 权限边界有自动化测试 - -第三阶段验收： - -- review / promotion / audit 有可视化页面 -- CLI 对外可稳定联调 - -## 8. 建议立即补的文档 - -为了让后续开发真正顺利，建议马上追加 4 份冻结文档： - -1. `权限矩阵冻结文档` -2. `发布状态机冻结文档` -3. `上传与存储安全规范` -4. `API Token / Device Flow 协议冻结文档` - -这 4 份文档会比继续补泛化架构图更有实际开发价值。 - -## 9. 最终建议 - -SkillHub 目前最值得保留的是： - -- 模块化单体拆分方向 -- 领域服务初步分层 -- review / promotion / search / storage 的边界意识 - -最需要马上收口的是： - -- 权限模型 -- 发布状态机 -- 上传安全 -- 配置一致性 -- 测试基线 - -只要先把这五件事修稳，后面的架构优化和产品创新都能顺着推进；如果不先修，越往后叠功能，返工就越贵。 From 8cfb6b6384378539b5ea7e678f4682f0c66bafc1 Mon Sep 17 00:00:00 2001 From: yun-zhi-ztl <15071461069@163.com> Date: Fri, 13 Mar 2026 14:13:30 +0800 Subject: [PATCH 12/13] fix(ci): stabilize openapi sdk validation --- scripts/check-openapi-generated.sh | 32 ++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/scripts/check-openapi-generated.sh b/scripts/check-openapi-generated.sh index 2cc21ef5..c111ab0f 100755 --- a/scripts/check-openapi-generated.sh +++ b/scripts/check-openapi-generated.sh @@ -6,7 +6,18 @@ ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" SERVER_DIR="$ROOT_DIR/server" WEB_DIR="$ROOT_DIR/web" API_LOG="${TMPDIR:-/tmp}/skillhub-openapi-check.log" +BUILD_LOG="${TMPDIR:-/tmp}/skillhub-openapi-build.log" SERVER_PID="" +OPENAPI_URL="http://127.0.0.1:8080/v3/api-docs" + +print_log_tail() { + local log_file="$1" + + if [[ -f "$log_file" ]]; then + echo "--- Last 50 lines of $log_file ---" >&2 + tail -n 50 "$log_file" >&2 || true + fi +} cleanup() { if [[ -n "$SERVER_PID" ]] && kill -0 "$SERVER_PID" 2>/dev/null; then @@ -21,6 +32,15 @@ trap cleanup EXIT cd "$ROOT_DIR" docker compose up -d --wait postgres redis +( + cd "$SERVER_DIR" + ./mvnw -pl skillhub-app -am -DskipTests install +) >"$BUILD_LOG" 2>&1 || { + echo "Failed to prepare backend modules. See $BUILD_LOG" >&2 + print_log_tail "$BUILD_LOG" + exit 1 +} + ( cd "$SERVER_DIR" SPRING_PROFILES_ACTIVE=local ./mvnw -pl skillhub-app spring-boot:run @@ -28,14 +48,22 @@ docker compose up -d --wait postgres redis SERVER_PID=$! for _ in $(seq 1 90); do - if curl -fsS "http://127.0.0.1:8080/v3/api-docs" >/dev/null 2>&1; then + if curl -fsS "$OPENAPI_URL" >/dev/null 2>&1; then break fi + + if ! kill -0 "$SERVER_PID" 2>/dev/null; then + echo "Backend exited before exposing /v3/api-docs. See $API_LOG" >&2 + print_log_tail "$API_LOG" + exit 1 + fi + sleep 2 done -if ! curl -fsS "http://127.0.0.1:8080/v3/api-docs" >/dev/null 2>&1; then +if ! curl -fsS "$OPENAPI_URL" >/dev/null 2>&1; then echo "Backend did not expose /v3/api-docs. See $API_LOG" >&2 + print_log_tail "$API_LOG" exit 1 fi From 142685610aa89c10fe302c57efd8ddf269dfd3fd Mon Sep 17 00:00:00 2001 From: yun-zhi-ztl Date: Sat, 14 Mar 2026 17:52:56 +0800 Subject: [PATCH 13/13] fix(test): update ReviewPermissionCheckerTest for stricter self-review policy After merge, HEAD's ReviewPermissionChecker prohibits all self-review (including SKILL_ADMIN/SUPER_ADMIN). Updated tests to reflect this stricter security model from A2. --- .../domain/review/ReviewPermissionCheckerTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/review/ReviewPermissionCheckerTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/review/ReviewPermissionCheckerTest.java index 189a7d1f..6ba7a4de 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/review/ReviewPermissionCheckerTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/review/ReviewPermissionCheckerTest.java @@ -26,18 +26,18 @@ class ReviewPermissionCheckerTest { } @Test - void skillAdminCanReviewOwnSubmission() { + void skillAdminCannotReviewOwnSubmission() { String userId = "user-1"; ReviewTask task = new ReviewTask(1L, 10L, userId); - assertTrue(checker.canReview(task, userId, + assertFalse(checker.canReview(task, userId, NamespaceType.TEAM, Map.of(), Set.of("SKILL_ADMIN"))); } @Test - void superAdminCanReviewOwnSubmission() { + void superAdminCannotReviewOwnSubmission() { String userId = "user-1"; ReviewTask task = new ReviewTask(1L, 10L, userId); - assertTrue(checker.canReview(task, userId, + assertFalse(checker.canReview(task, userId, NamespaceType.TEAM, Map.of(), Set.of("SUPER_ADMIN"))); }