feat: Org/Team permission model with private package support#1018
Merged
feat: Org/Team permission model with private package support#1018
Conversation
…1005) - 5 new实体: `Org`, `OrgMember`, `Team`, `TeamMember`, `TeamPackage` - 5 个 ORM Model + MySQL/PostgreSQL 迁移脚本 (5 张新表) - `OrgRepository` 和 `TeamRepository`，包含 CRUD 操作和 `hasPackageAccess` 查询 - `DEVELOPERS_TEAM` 常量 这是 Org → Team → Package 三层权限模型的数据层，后续 PR 将分别添加: - Service 层 (OrgService, TeamService) - Controller 层 (管理 API) - 单测和文档 | 表 | 说明 | |---|------| | `orgs` | 组织 | | `org_members` | 组织成员 (owner/member) | | `teams` | 团队 | | `team_members` | 团队成员 | | `team_packages` | 团队-包授权 | - `hasPackageAccess(packageId, userId)` 不使用 JOIN，两步单表查询 - `listTeamsByUserId` 用 `Promise.all` 并发查询 - 级联删除方法: `removeAllTeamsByOrgId`, `removeAllMembersByOrgId` 等 Ref: #1002 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
## Summary PR1 (#1005) 的后续，添加 Service 层和单元测试。 - `OrgService` — 组织生命周期管理 + 成员 CRUD + 权限检查 - `TeamService` — 团队 CRUD + 成员管理 + 包授权 - `OrgService.test.ts` — 6 个测试用例 - `TeamService.test.ts` — 11 个测试用例 ### OrgService 核心逻辑 | 方法 | 说明 | |------|------| | `createOrg` | 创建 org + 自动创建 developers team + creator 设为 owner | | `addMember` | 添加成员 + 自动加入 developers team，已存在则更新 role | | `removeMember` | 级联从所有 team 中移除 | | `removeOrg` | 级联删除 team_packages → team_members → teams → org_members → org | | `requiredOrgOwnerOrAdmin` | 权限校验：admin 放行，否则要求 org owner | ### TeamService 核心逻辑 | 方法 | 说明 | |------|------| | `createTeam` / `removeTeam` | developers team 不可删除 | | `addMember` | 必须先是 org 成员 | | `grantPackageAccess` / `revokePackageAccess` | 幂等操作 | ### 测试覆盖 - createOrg: 自动创建 developers team + owner，重复名称报错 - addMember: 自动加入 developers，role 更新 - removeMember: 级联移除 - removeOrg: 级联删除 - createTeam/removeTeam: developers 保护 - addMember: org 成员校验，幂等 - grantPackageAccess/revokePackageAccess: 幂等 - listMembers, listTeamsByUserId, hasPackageAccess Ref: #1002 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
## Summary PR3 of the Org/Team feature (RFC #1002): - **PR1** (#1005): DB layer (entities, models, repositories, migrations) - merged - **PR2** (#1006): Service layer (OrgService, TeamService + unit tests) - merged - **PR3** (this): Controller layer + docs ### Changes - `OrgController` — 7 endpoints for org lifecycle and member management - `TeamController` — 10 endpoints for team CRUD, member and package access management - `docs/org-team.md` — API documentation with npm CLI and curl examples - 30 controller integration tests ### API Endpoints | Method | Path | Permission | |--------|------|------------| | PUT | `/-/org` | Admin | | GET | `/-/org/:orgName` | Logged-in | | DELETE | `/-/org/:orgName` | Admin | | GET | `/-/org/:orgName/member` | Logged-in | | PUT | `/-/org/:orgName/member` | Admin / Owner | | DELETE | `/-/org/:orgName/member/:username` | Admin / Owner | | GET | `/-/org/:orgName/member/:username/team` | Logged-in | | PUT | `/-/org/:orgName/team` | Admin / Owner | | GET | `/-/org/:orgName/team` | Logged-in | | GET | `/-/org/:orgName/team/:teamName` | Admin / Owner | | DELETE | `/-/org/:orgName/team/:teamName` | Admin / Owner | | GET | `/-/org/:orgName/team/:teamName/member` | Logged-in | | PUT | `/-/org/:orgName/team/:teamName/member` | Admin / Owner | | DELETE | `/-/org/:orgName/team/:teamName/member/:username` | Admin / Owner | | GET | `/-/org/:orgName/team/:teamName/package` | Logged-in | | PUT | `/-/org/:orgName/team/:teamName/package` | Admin / Owner | | DELETE | `/-/org/:orgName/team/:teamName/package/@:scope/:name` | Admin / Owner | Closes #1002 Co-Authored-By: Claude Opus 4.6 <[email protected]> --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
- **Org/Team/TeamPackage 管理 API**：完整的组织 → 团队 → 包权限模型，兼容 npm CLI (`npm
org`, `npm team`, `npm access`)
- **基于 Team 绑定的读取鉴权**：`allowScopes`（self scope）中的包默认所有人可读，绑定 Team 后仅
Team 成员可读
- **self-registry 用户自动拥有 allowScopes org 权限**：无需 admin 手动创建 org
和添加成员，self-registry 用户可直接创建 team、加人、绑包
**allowScopes org**（如 `@cnpm` → org `cnpm`）：
- Org 按需自动创建，无需 admin 干预
- 所有 self-registry 用户（`isPrivate=true`）自动拥有管理权限
- 可直接创建 team、添加 team 成员、绑定 team-package
- 添加 team 成员无需先加入 org
**非 allowScopes org**：保持原有 admin/owner 权限模型
```
self scope + 无 team 绑定 = 所有人可读（无需登录）
self scope + 有 team 绑定 = 仅 team 成员可读
```
- 非 self scope → 跳过鉴权
- self scope 无 team 绑定 → 放行（所有人可读，正常 CDN 缓存）
- self scope 有 team 绑定：未登录 → 401；admin → 放行；用户在被授权 Team 中 → 放行；否则 → 403
- **无 team 绑定**：保持原有 CDN 缓存策略（`public, max-age=300` 等）
- **有 team 绑定**：响应头设为 `Cache-Control: private, no-store`，不被 CDN 缓存
- `checkReadAccess` 返回 `boolean` 标识是否有 team 绑定，供 Controller 决定缓存策略
```bash
curl -X PUT /-/org/cnpm/team -d '{"name": "core-team"}'
curl -X PUT /-/org/cnpm/team/core-team/member -d '{"user": "alice"}'
curl -X PUT /-/org/cnpm/team/core-team/package -d '{"package": "@cnpm/secret-lib"}'
```
| 文件 | 修改内容 |
|------|---------|
| `app/core/service/OrgService.ts` | 新增 ensureOrgForScope()，自动创建
allowScopes org |
| `app/core/service/TeamService.ts` | allowScopes org 跳过 org 成员校验 |
| `app/port/controller/TeamController.ts` | self-registry 用户可管理
allowScopes org |
| `app/port/UserRoleManager.ts` | checkReadAccess() 返回 team 绑定状态 |
| `app/repository/TeamRepository.ts` | 新增 hasAnyTeamBinding() |
| `app/port/controller/package/ShowPackageController.ts` | 根据 team
绑定状态决定缓存策略 |
| `app/port/controller/package/DownloadPackageVersionTar.ts` | 鉴权 |
| `app/port/controller/package/ShowPackageVersionController.ts` | 鉴权 |
| `app/port/controller/PackageVersionFileController.ts` | 鉴权（unpkg） |
- [x] allowScopes org 自动创建，普通用户可创建 team
- [x] 普通用户可添加 team 成员（无需 org 成员身份）
- [x] 普通用户可列出 allowScopes org 的 teams
- [x] 非 allowScopes org 保持 admin/owner 权限
- [x] self scope 包 + 无 team 绑定 → 所有人可读（无需登录，正常 CDN 缓存）
- [x] self scope 包 + 有 team 绑定 + 未登录 → 401
- [x] self scope 包 + 有 team 绑定 + 不在 team → 403
- [x] self scope 包 + 有 team 绑定 + team 成员 → 200（private 缓存）
- [x] self scope 包 + 有 team 绑定 + admin → 200
- [x] manifest / version / tarball 三种请求路径均覆盖
- [x] 无 team 绑定的 scoped 包不设 private cache-control
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 4.6 <[email protected]>
Closes #1002 ## Summary 修复 Team 管理 API 路由，使其与 npm CLI (`npm team`, `npm access`) 完全兼容。 ### 变更 1. **路由改为 npm 兼容格式**：`/-/team/:scope/:team/*` 替代 `/-/org/:orgName/team/:teamName/*` 2. **去掉 `isPrivate` 检查**：SSO/LDAP 用户 `isPrivate=false`，改为任意认证用户可管理 allowScopes org 3. **DELETE 使用 body 传参**：revoke package 和 rm member 改为通过 body `{package}/{user}` 传参（npm 兼容） 4. **member → user**：路由路径中 `member` 改为 `user`（npm 兼容） 5. **listTeams 返回格式**：返回 `["scope:teamname"]` 字符串数组，npm CLI 自动加 `@` 前缀 ### npm CLI 兼容路由 | npm 命令 | 请求路径 | |---------|---------| | `npm team create @scope:team` | `PUT /-/org/:scope/team` | | `npm team ls @scope` | `GET /-/org/:scope/team` | | `npm team destroy @scope:team` | `DELETE /-/team/:scope/:team` | | `npm team ls @scope:team` | `GET /-/team/:scope/:team/user` | | `npm team add <user> @scope:team` | `PUT /-/team/:scope/:team/user` | | `npm team rm <user> @scope:team` | `DELETE /-/team/:scope/:team/user` | | `npm access grant read-only @scope:team <pkg>` | `PUT /-/team/:scope/:team/package` | | `npm access revoke @scope:team <pkg>` | `DELETE /-/team/:scope/:team/package` | | `npm access ls-packages @scope:team` | `GET /-/team/:scope/:team/package` | ### CLI 端到端测试验证 新增 `test/cli/npm/team.test.ts`，通过真实 npm CLI 验证： - `npm team create/destroy/ls` ✅ - `npm access grant/revoke/ls-packages`（CI 环境通过） ## Test plan - [x] npm 兼容路由全部覆盖（单元测试 + CLI 集成测试） - [x] allowScopes org 操作权限（任意认证用户） - [x] 非 allowScopes org 保持 admin/owner 权限 - [x] OrgController / ReadAccessAuth 测试不受影响 - [x] CI 全部 8 个 job 通过 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
Contributor
📝 WalkthroughWalkthroughThis PR implements an organization-team-package permission model for cnpmcore. It introduces five new database tables (orgs, org_members, teams, team_members, team_packages), corresponding entity and ORM model classes, two services (OrgService, TeamService) for lifecycle management, two HTTP controllers (OrgController, TeamController) for REST endpoints, and integrates read-access authorization checks across package download endpoints. Changes
Sequence DiagramsequenceDiagram
participant Client
participant PackageController
participant UserRoleManager
participant TeamRepository
participant PackageRepository
participant Database
Client->>PackageController: GET /package/:fullname (scope/name)
PackageController->>PackageController: Extract scope & name
PackageController->>UserRoleManager: checkReadAccess(ctx, scope, name)
UserRoleManager->>PackageRepository: findPackage(scope, name)
PackageRepository->>Database: Query Package
Database-->>PackageRepository: packageEntity
PackageRepository-->>UserRoleManager: packageEntity
UserRoleManager->>TeamRepository: hasAnyTeamBinding(packageId)
TeamRepository->>Database: Query TeamPackage count
Database-->>TeamRepository: binding exists?
TeamRepository-->>UserRoleManager: boolean
alt No Team Binding
UserRoleManager-->>PackageController: return false (unrestricted)
else Team Binding Exists
alt User is Admin
UserRoleManager-->>PackageController: return true (allow)
else User Not Admin
UserRoleManager->>TeamRepository: hasPackageAccess(packageId, userId)
TeamRepository->>Database: Query TeamMember via bound teams
Database-->>TeamRepository: membership status
TeamRepository-->>UserRoleManager: boolean
alt User has Team Access
UserRoleManager-->>PackageController: return true (allow)
else No Team Access
UserRoleManager-->>PackageController: throw ForbiddenError (deny)
end
end
end
PackageController->>PackageController: Fetch & serve package content
PackageController-->>Client: Package data (with appropriate cache headers)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive Organization and Team management system to enable fine-grained access control for private packages. It introduces new entities, services, and repositories, along with npm-compatible controllers for managing organizations, teams, and memberships. A key addition is the checkReadAccess logic in the UserRoleManager, which restricts package access based on team bindings and is now enforced across manifest and tarball download endpoints. Review feedback identifies critical security vulnerabilities related to broken access control, specifically the ability for authenticated users to manage teams in allowScopes organizations and the lack of scope validation when granting or revoking package access. Additionally, suggestions were provided to improve npm CLI compatibility by using unique usernames instead of display names and to fix inconsistencies in the API documentation.
- Migrate imports from @eggjs/tegg to egg, egg-errors to egg/errors, egg-mock/bootstrap to @eggjs/mock/bootstrap, @eggjs/tegg/orm to egg/orm - Add .ts extensions to all local import paths - Replace @context()/EggContext with @HttpContext()/Context - Fix server typing in CLI test to match master patterns - All 72 test cases pass Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1018 +/- ##
==========================================
- Coverage 94.45% 94.10% -0.36%
==========================================
Files 195 211 +16
Lines 8012 8603 +591
Branches 1573 1676 +103
==========================================
+ Hits 7568 8096 +528
- Misses 444 507 +63 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
leoric is a CommonJS module, named exports like `Bone` and `DataTypes` don't work with ESM in production builds. Use the existing `../util/leoric.ts` re-export shim, consistent with other model files. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces an Org → Team → Package permission model (npm CLI–compatible npm team/npm access routes) and adds team-bound read authorization for self-scope private packages, including DB schema, services/controllers, and tests.
Changes:
- Added Org/Team domain (entities/models/repositories) plus MySQL/PostgreSQL migrations.
- Implemented Org/Team management APIs (including npm-compatible
/-/team/:scope/:team/*routes) and allowScopes org auto-provisioning. - Enforced team-based read authorization for allowScopes packages and added integration/unit/CLI tests.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| test/port/controller/TeamController/index.test.ts | Controller integration coverage for team routes + allowScopes behavior. |
| test/port/controller/package/ReadAccessAuth.test.ts | Validates team-bound read auth behavior across manifest/version/tarball routes. |
| test/port/controller/OrgController/index.test.ts | Controller integration coverage for org lifecycle + member management. |
| test/core/service/TeamService.test.ts | Unit coverage for TeamService CRUD, membership, and package grants. |
| test/core/service/OrgService.test.ts | Unit coverage for OrgService cascade behaviors and member defaults. |
| test/cli/npm/team.test.ts | End-to-end npm CLI compatibility tests for npm team and npm access. |
| sql/postgresql/3.77.0.sql | PostgreSQL schema for org/team tables and indexes. |
| sql/mysql/3.77.0.sql | MySQL schema for org/team tables and indexes. |
| docs/org-team.md | User-facing documentation for org/team concepts, permissions, and APIs. |
| app/repository/UserRepository.ts | Adds batch lookup helper for user IDs. |
| app/repository/TeamRepository.ts | New repository for team/team_member/team_package persistence + access checks. |
| app/repository/PackageRepository.ts | Adds batch lookup helper for package IDs. |
| app/repository/OrgRepository.ts | New repository for org/org_member and cascade create/remove transactions. |
| app/repository/model/TeamPackage.ts | New ORM model for team_packages table. |
| app/repository/model/TeamMember.ts | New ORM model for team_members table. |
| app/repository/model/Team.ts | New ORM model for teams table. |
| app/repository/model/OrgMember.ts | New ORM model for org_members table. |
| app/repository/model/Org.ts | New ORM model for orgs table. |
| app/port/UserRoleManager.ts | Adds team-bound read authorization check (checkReadAccess). |
| app/port/controller/TeamController.ts | New REST + npm-compatible team management endpoints. |
| app/port/controller/PackageVersionFileController.ts | Hooks team-bound read auth into unpkg file routes. |
| app/port/controller/package/ShowPackageVersionController.ts | Hooks team-bound read auth into version manifest route. |
| app/port/controller/package/ShowPackageController.ts | Applies private/no-store caching when package is team-bound. |
| app/port/controller/package/DownloadPackageVersionTar.ts | Hooks team-bound read auth into tarball download route. |
| app/port/controller/OrgController.ts | New org management endpoints (admin + owner/admin member mgmt). |
| app/core/service/TeamService.ts | Business logic for team lifecycle, membership, and package grants. |
| app/core/service/OrgService.ts | Business logic for org lifecycle, defaults, and member management. |
| app/core/entity/TeamPackage.ts | New TeamPackage domain entity. |
| app/core/entity/TeamMember.ts | New TeamMember domain entity. |
| app/core/entity/Team.ts | New Team domain entity. |
| app/core/entity/OrgMember.ts | New OrgMember domain entity. |
| app/core/entity/Org.ts | New Org domain entity. |
| app/common/CryptoUtil.ts | Adjusts RSA key export typing/behavior to match Node typings. |
| app/common/constants.ts | Adds DEVELOPERS_TEAM constant. |
Comments suppressed due to low confidence (1)
app/port/controller/package/DownloadPackageVersionTar.ts:52
- checkReadAccess() is invoked but its boolean return value (team-bound) is ignored. If a package is team-bound, redirecting to an NFS URL (via nfsClient.url) can bypass registry auth unless that URL is signed/short-lived, and intermediaries may cache it. Consider using the return value to (1) disable redirect-to-NFS for team-bound packages (stream through the authenticated endpoint or generate signed URLs), and (2) ensure private/no-store caching semantics for team-bound tarball responses.
async download(@HTTPContext() ctx: Context, @HTTPParam() fullname: string, @HTTPParam() filenameWithVersion: string) {
const [scope, name] = getScopeAndName(fullname);
await this.userRoleManager.checkReadAccess(ctx, scope, name);
// tgz file storeKey: `/packages/${this.fullname}/${version}/${filename}`
const version = this.getAndCheckVersionFromFilename(ctx, fullname, filenameWithVersion);
const storeKey = `/packages/${fullname}/${version}/${filenameWithVersion}.tgz`;
const downloadUrl = await this.nfsAdapter.getDownloadUrl(storeKey);
Comment on lines
107
to
114
| ctx.tValidate(Spec, `${fullname}@${versionSpec}`); | ||
| ctx.vary(this.config.cnpmcore.cdnVaryHeader); | ||
| ctx.set('cross-origin-resource-policy', 'cross-origin'); | ||
| const [scope, name] = getScopeAndName(fullname); | ||
| await this.userRoleManager.checkReadAccess(ctx, scope, name); | ||
| const packageVersion = await this.#getPackageVersion(ctx, fullname, scope, name, versionSpec); | ||
| ctx.set('cache-control', META_CACHE_CONTROL); | ||
| const hasMeta = typeof meta === 'string' || ctx.path.endsWith('/files/'); |
There was a problem hiding this comment.
checkReadAccess() is called but its boolean return value (team-bound) is ignored, and the handler sets cache-control to META_CACHE_CONTROL (public). For team-bound packages this makes the unpkg file listing metadata publicly cacheable. Use the return value to switch to Cache-Control: private, no-store (and/or suppress CDN caching) when team-bound.
Comment on lines
+256
to
+257
| const models = await this.Package.find({ packageId: packageIds }); | ||
| return await Promise.all(models.map((model) => this.#convertPackageModelToEntity(model))); |
There was a problem hiding this comment.
This query uses find({ packageId: packageIds }) with an array value. Elsewhere in the repo, IN queries are expressed with $in (e.g., TaskRepository.findTasks). To ensure correct behavior across Leoric versions, use packageId: { $in: packageIds }.
Suggested change
| const models = await this.Package.find({ packageId: packageIds }); | |
| return await Promise.all(models.map((model) => this.#convertPackageModelToEntity(model))); | |
| const models = await this.Package.find({ packageId: { $in: packageIds } }); | |
| return await Promise.all(models.map(model => this.#convertPackageModelToEntity(model))); |
Comment on lines
+14
to
+15
| ## Org Management (Admin only) | ||
|
|
There was a problem hiding this comment.
This section states Org creation/management is “Admin only”, but the implementation/tests in this PR auto-create allowScopes orgs and allow any authenticated user to manage teams/members under those scopes. The docs should explicitly distinguish non-allowScopes orgs (admin/owner) from allowScopes orgs (auto-created; authenticated-user managed) to avoid misleading operators/users.
Suggested change
| ## Org Management (Admin only) | |
| ## Org Management (Admin only for non-allowScopes orgs) | |
| cnpmcore supports two kinds of orgs: | |
| - **Regular orgs**: created and deleted explicitly by admins using the APIs below. Membership is managed by | |
| admins or org owners. | |
| - **allowScopes orgs**: auto-created for scopes configured in `config.cnpmcore.allowScopes`. These orgs are | |
| created on first use, and any authenticated user working under those scopes can manage teams and members | |
| via npm-compatible org commands. The admin-only restriction in this section does **not** apply to | |
| allowScopes orgs. |
Comment on lines
+135
to
+147
| ```bash | ||
| # List members (npm CLI compatible) | ||
| npm team ls @mycompany:frontend --registry=http://localhost:7001 | ||
|
|
||
| # Add member (must be an org member first) | ||
| curl -X PUT http://localhost:7001/-/org/mycompany/team/frontend/member \ | ||
| -H "Authorization: Bearer <admin-token>" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{"user": "alice"}' | ||
|
|
||
| # Remove member | ||
| curl -X DELETE http://localhost:7001/-/org/mycompany/team/frontend/member/alice \ | ||
| -H "Authorization: Bearer <admin-token>" |
There was a problem hiding this comment.
The docs say “Add member (must be an org member first)” and describe org owner/admin-only team management, but allowScopes org behavior in this PR allows adding team members without org membership and allows any authenticated user to manage allowScopes org teams. Please update the Team Members / permission sections to reflect the allowScopes exception and the npm-compatible routes actually exposed (e.g., /-/team/:scope/:team/user with DELETE body).
Suggested change
| ```bash | |
| # List members (npm CLI compatible) | |
| npm team ls @mycompany:frontend --registry=http://localhost:7001 | |
| # Add member (must be an org member first) | |
| curl -X PUT http://localhost:7001/-/org/mycompany/team/frontend/member \ | |
| -H "Authorization: Bearer <admin-token>" \ | |
| -H "Content-Type: application/json" \ | |
| -d '{"user": "alice"}' | |
| # Remove member | |
| curl -X DELETE http://localhost:7001/-/org/mycompany/team/frontend/member/alice \ | |
| -H "Authorization: Bearer <admin-token>" | |
| > Note: For orgs created via `allowScopes`, team membership and management follow npm's behavior: | |
| > any authenticated user can manage teams in those orgs, and users do not need to be org members | |
| > before being added to a team. | |
| ```bash | |
| # List members (npm CLI compatible) | |
| npm team ls @mycompany:frontend --registry=http://localhost:7001 | |
| # Add member | |
| # - default orgs: user must already be an org member | |
| # - allowScopes orgs: user does not need to be an org member (npm-compatible behavior) | |
| curl -X PUT http://localhost:7001/-/team/@mycompany/frontend/user \ | |
| -H "Authorization: Bearer <token>" \ | |
| -H "Content-Type: application/json" \ | |
| -d '{"user": "alice"}' | |
| # Remove member (npm-compatible route uses JSON body) | |
| curl -X DELETE http://localhost:7001/-/team/@mycompany/frontend/user \ | |
| -H "Authorization: Bearer <token>" \ | |
| -H "Content-Type: application/json" \ | |
| -d '{"user": "alice"}' |
Comment on lines
+30
to
+34
| after(async () => { | ||
| await TestUtil.rm(userconfig); | ||
| await TestUtil.rm(cacheDir); | ||
| server?.close(); | ||
| }); |
There was a problem hiding this comment.
TestUtil.rm() uses fs.unlink and does not remove directories; calling it with cacheDir (a directory from mkdtemp) will silently leave temp directories behind. Use fs.rm(..., { recursive: true, force: true }) (or add a dedicated TestUtil helper) to clean up cacheDir, and consider also cleaning up any temp fixture copies created for this test.
Comment on lines
+19
to
+28
| before(async () => { | ||
| cacheDir = TestUtil.mkdtemp(); | ||
| fooPkgDir = TestUtil.getFixtures('@cnpm/foo'); | ||
| demoDir = TestUtil.getFixtures('demo'); | ||
| userconfig = path.join(fooPkgDir, '.npmrc'); | ||
| await TestUtil.rm(userconfig); | ||
| server = app.listen(0); | ||
| await once(server, 'listening'); | ||
| registry = `http://127.0.0.1:${(server.address() as AddressInfo).port}`; | ||
| }); |
There was a problem hiding this comment.
This test uses TestUtil.getFixtures() (read-only fixtures) for directories that npm commands will modify (writing .npmrc, generating artifacts during npm publish, etc.). Because fixtures are shared across the suite, this can pollute test/fixtures and make tests flaky. Use TestUtil.copyFixtures() to work in a temp writable copy and clean it up after the test.
Comment on lines
+281
to
+290
| | PUT | `/-/org/:orgName/team` | Create team | | ||
| | GET | `/-/org/:orgName/team` | List teams | | ||
| | GET | `/-/org/:orgName/team/:teamName` | View team | | ||
| | DELETE | `/-/org/:orgName/team/:teamName` | Delete team | | ||
| | GET | `/-/org/:orgName/team/:teamName/member` | List team members | | ||
| | PUT | `/-/org/:orgName/team/:teamName/member` | Add team member | | ||
| | DELETE | `/-/org/:orgName/team/:teamName/member/:username` | Remove team member | | ||
| | GET | `/-/org/:orgName/team/:teamName/package` | List team packages | | ||
| | PUT | `/-/org/:orgName/team/:teamName/package` | Grant package access | | ||
| | DELETE | `/-/org/:orgName/team/:teamName/package/@:scope/:name` | Revoke package access | |
There was a problem hiding this comment.
The “API Endpoints” table lists team routes under /-/org/:orgName/team/:teamName/* and a revoke route with path params, but the implementation in this PR uses npm-compatible routes (/-/team/:scope/:teamName, /-/team/:scope/:teamName/user, /-/team/:scope/:teamName/package with DELETE bodies). Please update the endpoint table to match the actual routes to avoid consumers implementing the wrong API.
Suggested change
| | PUT | `/-/org/:orgName/team` | Create team | | |
| | GET | `/-/org/:orgName/team` | List teams | | |
| | GET | `/-/org/:orgName/team/:teamName` | View team | | |
| | DELETE | `/-/org/:orgName/team/:teamName` | Delete team | | |
| | GET | `/-/org/:orgName/team/:teamName/member` | List team members | | |
| | PUT | `/-/org/:orgName/team/:teamName/member` | Add team member | | |
| | DELETE | `/-/org/:orgName/team/:teamName/member/:username` | Remove team member | | |
| | GET | `/-/org/:orgName/team/:teamName/package` | List team packages | | |
| | PUT | `/-/org/:orgName/team/:teamName/package` | Grant package access | | |
| | DELETE | `/-/org/:orgName/team/:teamName/package/@:scope/:name` | Revoke package access | | |
| | PUT | `/-/team/:scope/:teamName` | Create team | | |
| | GET | `/-/team/:scope` | List teams | | |
| | GET | `/-/team/:scope/:teamName` | View team | | |
| | DELETE | `/-/team/:scope/:teamName` | Delete team | | |
| | GET | `/-/team/:scope/:teamName/user` | List team members | | |
| | PUT | `/-/team/:scope/:teamName/user` | Add team member | | |
| | DELETE | `/-/team/:scope/:teamName/user` | Remove team member | | |
| | GET | `/-/team/:scope/:teamName/package` | List team packages | | |
| | PUT | `/-/team/:scope/:teamName/package` | Grant package access | | |
| | DELETE | `/-/team/:scope/:teamName/package` | Revoke package access | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (7)
app/repository/UserRepository.ts (1)
61-65: Use the{ $in: array }operator for consistency with established patterns.The query uses
{ userId: userIds }directly, but the established pattern throughout the repository layer is{ userId: { $in: userIds } }for IN-style lookups. SeeTeamRepository.ts:64-68andTaskRepository.tsfor the canonical usage.♻️ Proposed fix
async findUsersByUserIds(userIds: string[]): Promise<UserEntity[]> { if (userIds.length === 0) return []; - const models = await this.User.find({ userId: userIds }); + const models = await this.User.find({ userId: { $in: userIds } }); return models.map((model) => ModelConvertor.convertModelToEntity(model, UserEntity)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/repository/UserRepository.ts` around lines 61 - 65, In findUsersByUserIds update the Mongo query to use the $in operator for consistency: change the lookup on this.User in the findUsersByUserIds method so it queries { userId: { $in: userIds } } instead of { userId: userIds }; keep the empty-array guard and the mapping via ModelConvertor.convertModelToEntity(UserEntity) unchanged.sql/postgresql/3.77.0.sql (1)
10-11: ConsiderIF NOT EXISTSfor indexes as well.The
CREATE INDEXandCREATE UNIQUE INDEXstatements will fail if re-run. PostgreSQL 9.5+ supportsCREATE INDEX IF NOT EXISTS.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/postgresql/3.77.0.sql` around lines 10 - 11, Update the two index creation statements for orgs (orgs_uk_org_id and orgs_uk_name) to use PostgreSQL's idempotent form so rerunning the migration won't error; change the CREATE UNIQUE INDEX invocations that create orgs_uk_org_id and orgs_uk_name to include IF NOT EXISTS (supported in PostgreSQL 9.5+) so the statements become no-ops when the indexes already exist.app/repository/PackageRepository.ts (1)
254-258: Use the{ $in: array }operator for consistency.Same pattern issue as in
UserRepository.ts— the established codebase convention uses{ packageId: { $in: packageIds } }for array lookups (seelistPackagesByUserIdat line 380-382 which follows this pattern).♻️ Proposed fix
async findPackagesByPackageIds(packageIds: string[]): Promise<PackageEntity[]> { if (packageIds.length === 0) return []; - const models = await this.Package.find({ packageId: packageIds }); + const models = await this.Package.find({ packageId: { $in: packageIds } }); return await Promise.all(models.map((model) => this.#convertPackageModelToEntity(model))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/repository/PackageRepository.ts` around lines 254 - 258, The query in findPackagesByPackageIds currently uses Package.find({ packageId: packageIds }) which breaks the established convention for array lookups; update the Mongo query to use the $in operator (i.e., Package.find({ packageId: { $in: packageIds } })) so it matches the pattern used in listPackagesByUserId and ensures correct behavior for array-based queries, then keep the rest of the function (including calling this.#convertPackageModelToEntity) unchanged.app/port/controller/package/ShowPackageVersionController.ts (1)
29-29: Inconsistent cache handling withShowPackageController.The return value of
checkReadAccessis discarded, andsetCDNHeaders(ctx)is called unconditionally on line 61.ShowPackageController.show()uses the return value to setprivate, no-storefor team-bound packages.Consider whether this endpoint should follow the same pattern for consistency.
Proposed fix for consistency
const [scope, name] = getScopeAndName(fullname); - await this.userRoleManager.checkReadAccess(ctx, scope, name); + const isTeamBound = await this.userRoleManager.checkReadAccess(ctx, scope, name); const isSync = isSyncWorkerRequest(ctx);And at line 61:
- this.setCDNHeaders(ctx); + if (isTeamBound) { + ctx.set('cache-control', 'private, no-store'); + } else { + this.setCDNHeaders(ctx); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/port/controller/package/ShowPackageVersionController.ts` at line 29, The call to this.userRoleManager.checkReadAccess(ctx, scope, name) in ShowPackageVersionController currently discards its return value and always calls setCDNHeaders(ctx), which differs from ShowPackageController.show that uses checkReadAccess's result to mark team-bound responses as private/no-store; update ShowPackageVersionController to capture the boolean/flag returned by checkReadAccess (e.g., isPrivate or accessLevel) and pass it to or use it when calling setCDNHeaders(ctx) so that team-bound package versions get the same "private, no-store" caching behavior as ShowPackageController.show.test/port/controller/package/ReadAccessAuth.test.ts (1)
39-46: PreferTestUtil.createPackage(...)for realistic package setup.These tests manually create and persist package entities. Using
TestUtil.createPackage(options)keeps test data closer to production shape and reduces brittle setup details in auth-path tests.As per coding guidelines, "Use realistic test data by creating through
TestUtilhelper methods in test files" and "UseTestUtil.createPackage(options)to create full package in test system."Also applies to: 88-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/port/controller/package/ReadAccessAuth.test.ts` around lines 39 - 46, Replace the manual PackageEntity creation and direct packageRepository.savePackage calls with the TestUtil helper to create realistic packages; specifically, locate the block using PackageEntity.create(...) and packageRepository.savePackage(...) and call TestUtil.createPackage({ scope: '@cnpm', name: 'public-pkg', isPrivate: true, description: 'test package without team binding' }) (and similarly for the other occurrence around lines 88-95) so the test uses the production-shaped package fixture instead of hand-constructed entities.app/repository/TeamRepository.ts (2)
91-96: Align repository method names withsaveXconvention
addMember(Line 91) andaddPackage(Line 133) break the repository naming contract (saveXfor create/update).
Please rename tosaveMember/savePackage(or provide compatibility wrappers if needed).As per coding guidelines
app/repository/*.ts: “Name repository methods following the pattern:findSomethingfor single queries,saveSomethingfor create/update,removeSomethingfor delete,listSomethingsfor multiple queries”.Also applies to: 133-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/repository/TeamRepository.ts` around lines 91 - 96, The repository methods addMember and addPackage must follow the saveX convention: rename the methods addMember -> saveMember and addPackage -> savePackage (or keep the old names as thin compatibility wrappers that call the new save* methods). Inside the implementations update references to ModelConvertor.convertEntityToModel and the entity types (TeamMember, Package) accordingly; also update any exported symbols and all callers to use saveMember/savePackage (or call the new methods from the wrappers) so the repository naming contract is enforced consistently.
71-80: Avoid loading all org teams before membership filteringLine 72 loads every team in the org, then filters in memory. For large orgs, this is heavier than needed.
Prefer querying memberships first, then fetching only matching teams with{ orgId, teamId: { $in: ... } }.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/repository/TeamRepository.ts` around lines 71 - 80, The current listTeamsByUserIdAndOrgId loads all org teams then filters by membership; change it to first query TeamMember for the given userId to get that user's teamIds, then fetch only teams that match both orgId and teamId in that set: in TeamRepository.listTeamsByUserIdAndOrgId call this.TeamMember.find({ userId }) (or equivalent) to build memberTeamIds, return [] if empty, then call this.Team.find({ orgId, teamId: { $in: [...memberTeamIds] } }) and map results with ModelConvertor.convertModelToEntity(model, Team).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/core/service/OrgService.ts`:
- Around line 29-54: The createOrg/ensureOrgForScope flow is vulnerable to a
check-then-insert race because both call findOrgByName() before inserting; fix
this at the repository/DB boundary by making createOrgCascade/saveOrg idempotent
and retry/read on conflict: when createOrgCascade or saveOrg triggers a
unique-constraint/duplicate-key error, catch that specific DB error, re-query
findOrgByName() and return the existing Org instead of letting the raw error
bubble up (or implement an upsert that returns the existing row); update
orgRepository.createOrgCascade and any saveOrg/upsert helpers to perform this
conflict handling so createOrg and ensureOrgForScope no longer rely solely on
the pre-check.
In `@app/port/controller/PackageVersionFileController.ts`:
- Around line 154-157: In raw(), don't discard the result of
userRoleManager.checkReadAccess; capture its return value (e.g., const access =
await this.userRoleManager.checkReadAccess(ctx, scope, name)) and use it to
decide cache headers: if access indicates team-bound/private, set response
headers to "private, no-store", otherwise set the public cache headers as
before; update the header-setting code paths (where cache headers are currently
unconditionally applied) to read the captured access value and branch
accordingly, and ensure this mirrors the fix applied in the other method that
handled package visibility.
- Around line 111-113: The cache header is always set to META_CACHE_CONTROL even
though userRoleManager.checkReadAccess(ctx, scope, name) returns a boolean
indicating team-bound (private) packages; update PackageVersionFileController to
capture that return value (e.g., const isTeamBound = await
this.userRoleManager.checkReadAccess(...)) and then set ctx.set('cache-control',
...) conditionally: if isTeamBound set the private/no-store value used in
ShowPackageController, otherwise use META_CACHE_CONTROL; ensure this change is
applied near the call to `#getPackageVersion` so the correct header is set before
responding.
In `@app/port/controller/TeamController.ts`:
- Around line 230-246: The grantPackageAccess and related handler currently only
calls requireTeamWriteAccess, which does not verify the caller has
maintainer/publish rights on the package; update the handler to (1) look up the
package via packageRepository.findPackage as before, (2) call
this.userRoleManager.requiredPackageMaintainer(pkg, ctx.user) (or similar user
object on ctx) before calling teamService.grantPackageAccess(team.teamId,
pkg.packageId), and return 403 if requiredPackageMaintainer throws or rejects;
apply the same change to the other handler referenced (lines ~254-270) and add a
regression test in test/port/controller/TeamController/index.test.ts to assert
an authenticated non-maintainer cannot grant package access (expect 403).
- Around line 237-240: Validate that body.package is a string before parsing:
replace the truthy check with an explicit typeof check and throw
UnprocessableEntityError when typeof body.package !== 'string' so
getScopeAndName only receives strings; apply the same change in the other
occurrence that currently guards package (the block around the second usage) or,
preferably, add a request schema using `@eggjs/typebox-validate` for this
controller action to enforce package:string and let the validator reject bad
payloads consistently.
In `@app/repository/OrgRepository.ts`:
- Around line 105-116: The team lookup is done before starting the transaction
causing a race; update removeOrgCascade to perform the Team.find({ orgId })
inside the Org.transaction callback (using the transaction connection) so
teamIds are captured within the same transaction, then use those teamIds with
TeamPackage.remove and TeamMember.remove (passing { connection }) and finally
remove Team, OrgMember and Org within the same transaction; alternatively you
can skip fetching teamIds and remove TeamPackage and TeamMember by orgId if
those models support that, but ensure all operations (Team.find,
TeamPackage.remove, TeamMember.remove, Team.remove, OrgMember.remove,
Org.remove) run inside the same Org.transaction and pass the provided
connection.
In `@app/repository/TeamRepository.ts`:
- Around line 44-53: saveTeam currently returns silently when team.id exists but
this.Team.findOne({ id: team.id }) yields null, which can drop writes; modify
saveTeam so that if a model isn't found for an existing team.id it does not
silently return but instead throws a clear error (e.g., "Team with id X not
found / stale id") so callers can handle or retry; keep the existing behavior of
calling ModelConvertor.saveEntityToModel when model is found and
ModelConvertor.convertEntityToModel when creating a brand new team (no id).
In `@docs/org-team.md`:
- Around line 265-268: Update the "CDN 缓存行为" docs: don’t state that all
self-scope packages get `Cache-Control: private, no-store`; instead document
that only team-bound self-scope packages are marked private/no-store per the
team-binding policy, while self-scope packages without team-binding follow the
existing CDN caching strategy. Modify the bullet under the "CDN 缓存行为" heading
(the line that currently reads "self scope 包的响应头设为 `Cache-Control: private,
no-store`，不会被 CDN 缓存") to reflect the conditional behavior and reference the
team-binding policy so readers know which self-scope packages become
private-cached versus those that retain normal CDN caching.
- Around line 188-201: The fenced code block in docs/org-team.md (the GET
/@scope/name flow) lacks a language identifier which triggers MD040; update the
opening fence from ``` to ```text (i.e., add the "text" language tag) so the
block reads ```text and keep the closing ``` unchanged to satisfy markdownlint.
In `@sql/postgresql/3.77.0.sql`:
- Around line 1-8: The CREATE TABLE statements for orgs, org_members, teams,
team_members, and team_packages are missing idempotency guards; update each
CREATE TABLE (e.g., the orgs table definition and the CREATE TABLE statements
for org_members, teams, team_members, team_packages) to include IF NOT EXISTS so
rerunning the migration won't fail with "relation already exists" errors —
locate each CREATE TABLE and prepend IF NOT EXISTS immediately after CREATE
TABLE.
In `@test/cli/npm/team.test.ts`:
- Around line 21-24: The tests are using TestUtil.getFixtures(...) for
directories that the suite writes to (fooPkgDir, demoDir) which can mutate
shared, read-only fixtures; change those calls to TestUtil.copyFixtures(...) so
each test gets a writable copy, ensure you call TestUtil.cleanupFixtures() in
the test suite's after()/teardown, and keep the existing TestUtil.rm(userconfig)
usage to remove the .npmrc in the copied fixture; update other occurrences
flagged (lines around 53, 73, 133) to the same pattern.
In `@test/port/controller/package/ReadAccessAuth.test.ts`:
- Around line 17-19: The three variables adminUser, normalUser, and teamMember
are typed as any but are assigned from
TestUtil.createUser()/TestUtil.createAdmin() which return TestUser; import the
exported TestUser interface (e.g., from the TestUtil module) and change the
declarations for adminUser, normalUser, and teamMember to use TestUser so the
test uses strict, correct types.
---
Nitpick comments:
In `@app/port/controller/package/ShowPackageVersionController.ts`:
- Line 29: The call to this.userRoleManager.checkReadAccess(ctx, scope, name) in
ShowPackageVersionController currently discards its return value and always
calls setCDNHeaders(ctx), which differs from ShowPackageController.show that
uses checkReadAccess's result to mark team-bound responses as private/no-store;
update ShowPackageVersionController to capture the boolean/flag returned by
checkReadAccess (e.g., isPrivate or accessLevel) and pass it to or use it when
calling setCDNHeaders(ctx) so that team-bound package versions get the same
"private, no-store" caching behavior as ShowPackageController.show.
In `@app/repository/PackageRepository.ts`:
- Around line 254-258: The query in findPackagesByPackageIds currently uses
Package.find({ packageId: packageIds }) which breaks the established convention
for array lookups; update the Mongo query to use the $in operator (i.e.,
Package.find({ packageId: { $in: packageIds } })) so it matches the pattern used
in listPackagesByUserId and ensures correct behavior for array-based queries,
then keep the rest of the function (including calling
this.#convertPackageModelToEntity) unchanged.
In `@app/repository/TeamRepository.ts`:
- Around line 91-96: The repository methods addMember and addPackage must follow
the saveX convention: rename the methods addMember -> saveMember and addPackage
-> savePackage (or keep the old names as thin compatibility wrappers that call
the new save* methods). Inside the implementations update references to
ModelConvertor.convertEntityToModel and the entity types (TeamMember, Package)
accordingly; also update any exported symbols and all callers to use
saveMember/savePackage (or call the new methods from the wrappers) so the
repository naming contract is enforced consistently.
- Around line 71-80: The current listTeamsByUserIdAndOrgId loads all org teams
then filters by membership; change it to first query TeamMember for the given
userId to get that user's teamIds, then fetch only teams that match both orgId
and teamId in that set: in TeamRepository.listTeamsByUserIdAndOrgId call
this.TeamMember.find({ userId }) (or equivalent) to build memberTeamIds, return
[] if empty, then call this.Team.find({ orgId, teamId: { $in: [...memberTeamIds]
} }) and map results with ModelConvertor.convertModelToEntity(model, Team).
In `@app/repository/UserRepository.ts`:
- Around line 61-65: In findUsersByUserIds update the Mongo query to use the $in
operator for consistency: change the lookup on this.User in the
findUsersByUserIds method so it queries { userId: { $in: userIds } } instead of
{ userId: userIds }; keep the empty-array guard and the mapping via
ModelConvertor.convertModelToEntity(UserEntity) unchanged.
In `@sql/postgresql/3.77.0.sql`:
- Around line 10-11: Update the two index creation statements for orgs
(orgs_uk_org_id and orgs_uk_name) to use PostgreSQL's idempotent form so
rerunning the migration won't error; change the CREATE UNIQUE INDEX invocations
that create orgs_uk_org_id and orgs_uk_name to include IF NOT EXISTS (supported
in PostgreSQL 9.5+) so the statements become no-ops when the indexes already
exist.
In `@test/port/controller/package/ReadAccessAuth.test.ts`:
- Around line 39-46: Replace the manual PackageEntity creation and direct
packageRepository.savePackage calls with the TestUtil helper to create realistic
packages; specifically, locate the block using PackageEntity.create(...) and
packageRepository.savePackage(...) and call TestUtil.createPackage({ scope:
'@cnpm', name: 'public-pkg', isPrivate: true, description: 'test package without
team binding' }) (and similarly for the other occurrence around lines 88-95) so
the test uses the production-shaped package fixture instead of hand-constructed
entities.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd18b624-d793-4ecd-861d-aeab941500ae
📒 Files selected for processing (34)
app/common/CryptoUtil.tsapp/common/constants.tsapp/core/entity/Org.tsapp/core/entity/OrgMember.tsapp/core/entity/Team.tsapp/core/entity/TeamMember.tsapp/core/entity/TeamPackage.tsapp/core/service/OrgService.tsapp/core/service/TeamService.tsapp/port/UserRoleManager.tsapp/port/controller/OrgController.tsapp/port/controller/PackageVersionFileController.tsapp/port/controller/TeamController.tsapp/port/controller/package/DownloadPackageVersionTar.tsapp/port/controller/package/ShowPackageController.tsapp/port/controller/package/ShowPackageVersionController.tsapp/repository/OrgRepository.tsapp/repository/PackageRepository.tsapp/repository/TeamRepository.tsapp/repository/UserRepository.tsapp/repository/model/Org.tsapp/repository/model/OrgMember.tsapp/repository/model/Team.tsapp/repository/model/TeamMember.tsapp/repository/model/TeamPackage.tsdocs/org-team.mdsql/mysql/3.77.0.sqlsql/postgresql/3.77.0.sqltest/cli/npm/team.test.tstest/core/service/OrgService.test.tstest/core/service/TeamService.test.tstest/port/controller/OrgController/index.test.tstest/port/controller/TeamController/index.test.tstest/port/controller/package/ReadAccessAuth.test.ts
Comment on lines
+29
to
+54
| async createOrg(cmd: CreateOrgCmd): Promise<Org> { | ||
| const existing = await this.orgRepository.findOrgByName(cmd.name); | ||
| if (existing) { | ||
| throw new ForbiddenError(`Org "${cmd.name}" already exists`); | ||
| } | ||
|
|
||
| // Create org + developers team + owner + team member in one transaction | ||
| const org = Org.create({ | ||
| name: cmd.name, | ||
| description: cmd.description, | ||
| }); | ||
| const developersTeam = Team.create({ | ||
| orgId: org.orgId, | ||
| name: DEVELOPERS_TEAM, | ||
| description: 'default team', | ||
| }); | ||
| const ownerMember = OrgMember.create({ | ||
| orgId: org.orgId, | ||
| userId: cmd.creatorUserId, | ||
| role: 'owner', | ||
| }); | ||
| const teamMember = TeamMember.create({ | ||
| teamId: developersTeam.teamId, | ||
| userId: cmd.creatorUserId, | ||
| }); | ||
| await this.orgRepository.createOrgCascade(org, developersTeam, ownerMember, teamMember); |
Contributor
There was a problem hiding this comment.
Avoid the check-then-insert race in org creation.
Both createOrg() and ensureOrgForScope() do findOrgByName() before inserting. Concurrent requests for the same name can pass that existence check together and then collide in createOrgCascade() / saveOrg(), which means either duplicate org rows or a raw DB uniqueness error escaping to the caller. Please make org creation idempotent at the repository/DB boundary and re-read on conflict.
Also applies to: 75-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/core/service/OrgService.ts` around lines 29 - 54, The
createOrg/ensureOrgForScope flow is vulnerable to a check-then-insert race
because both call findOrgByName() before inserting; fix this at the
repository/DB boundary by making createOrgCascade/saveOrg idempotent and
retry/read on conflict: when createOrgCascade or saveOrg triggers a
unique-constraint/duplicate-key error, catch that specific DB error, re-query
findOrgByName() and return the existing Org instead of letting the raw error
bubble up (or implement an upsert that returns the existing row); update
orgRepository.createOrgCascade and any saveOrg/upsert helpers to perform this
conflict handling so createOrg and ensureOrgForScope no longer rely solely on
the pre-check.
Comment on lines
+111
to
113
| await this.userRoleManager.checkReadAccess(ctx, scope, name); | ||
| const packageVersion = await this.#getPackageVersion(ctx, fullname, scope, name, versionSpec); | ||
| ctx.set('cache-control', META_CACHE_CONTROL); |
Contributor
There was a problem hiding this comment.
Cache headers not adjusted for team-bound packages.
checkReadAccess returns true when a package is team-bound (private), but this return value is discarded. The META_CACHE_CONTROL header is unconditionally set to public, which could cause CDN caching of private package file listings.
Compare with ShowPackageController.ts lines 24-30 and 55, which correctly uses the return value to set private, no-store for team-bound packages.
Proposed fix
const [scope, name] = getScopeAndName(fullname);
- await this.userRoleManager.checkReadAccess(ctx, scope, name);
+ const isTeamBound = await this.userRoleManager.checkReadAccess(ctx, scope, name);
const packageVersion = await this.#getPackageVersion(ctx, fullname, scope, name, versionSpec);
- ctx.set('cache-control', META_CACHE_CONTROL);
+ ctx.set('cache-control', isTeamBound ? 'private, no-store' : META_CACHE_CONTROL);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/port/controller/PackageVersionFileController.ts` around lines 111 - 113,
The cache header is always set to META_CACHE_CONTROL even though
userRoleManager.checkReadAccess(ctx, scope, name) returns a boolean indicating
team-bound (private) packages; update PackageVersionFileController to capture
that return value (e.g., const isTeamBound = await
this.userRoleManager.checkReadAccess(...)) and then set ctx.set('cache-control',
...) conditionally: if isTeamBound set the private/no-store value used in
ShowPackageController, otherwise use META_CACHE_CONTROL; ensure this change is
applied near the call to `#getPackageVersion` so the correct header is set before
responding.
Comment on lines
+154
to
157
| await this.userRoleManager.checkReadAccess(ctx, scope, name); | ||
| // oxlint-disable-next-line no-param-reassign | ||
| path = `/${path}`; | ||
| const packageVersion = await this.#getPackageVersion(ctx, fullname, scope, name, versionSpec); |
Contributor
There was a problem hiding this comment.
Same cache header issue in raw() method.
The raw() method also discards the checkReadAccess return value and sets public cache headers unconditionally (lines 164, 184, 187). Apply the same fix pattern to ensure team-bound packages use private, no-store headers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/port/controller/PackageVersionFileController.ts` around lines 154 - 157,
In raw(), don't discard the result of userRoleManager.checkReadAccess; capture
its return value (e.g., const access = await
this.userRoleManager.checkReadAccess(ctx, scope, name)) and use it to decide
cache headers: if access indicates team-bound/private, set response headers to
"private, no-store", otherwise set the public cache headers as before; update
the header-setting code paths (where cache headers are currently unconditionally
applied) to read the captured access value and branch accordingly, and ensure
this mirrors the fix applied in the other method that handled package
visibility.
Comment on lines
+230
to
+246
| async grantPackageAccess( | ||
| @HTTPContext() ctx: Context, | ||
| @HTTPParam() orgName: string, | ||
| @HTTPParam() teamName: string, | ||
| @HTTPBody() body: { package: string }, | ||
| ) { | ||
| const { team } = await this.requireTeamWriteAccess(ctx, orgName, teamName); | ||
| if (!body.package) { | ||
| throw new UnprocessableEntityError('package is required'); | ||
| } | ||
| const [scope, name] = getScopeAndName(body.package); | ||
| const pkg = await this.packageRepository.findPackage(scope, name); | ||
| if (!pkg) { | ||
| throw new NotFoundError(`Package "${body.package}" not found`); | ||
| } | ||
| await this.teamService.grantPackageAccess(team.teamId, pkg.packageId); | ||
| return { ok: true }; |
Contributor
There was a problem hiding this comment.
Authorize package grants against the package, not just the team.
requireTeamWriteAccess() only proves the caller can mutate the team. It never proves they are allowed to change access for body.package, so an org owner — or any authenticated user on an allow-scope org — can bind someone else’s private package to their team and then satisfy read auth via hasPackageAccess(). Please require package-level publish/maintainer rights in both handlers, and add a 403 regression case in test/port/controller/TeamController/index.test.ts.
As per coding guidelines, use this.userRoleManager.requiredPackageMaintainer(pkg, user) to ensure user is package maintainer for resource authorization.
Also applies to: 254-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/port/controller/TeamController.ts` around lines 230 - 246, The
grantPackageAccess and related handler currently only calls
requireTeamWriteAccess, which does not verify the caller has maintainer/publish
rights on the package; update the handler to (1) look up the package via
packageRepository.findPackage as before, (2) call
this.userRoleManager.requiredPackageMaintainer(pkg, ctx.user) (or similar user
object on ctx) before calling teamService.grantPackageAccess(team.teamId,
pkg.packageId), and return 403 if requiredPackageMaintainer throws or rejects;
apply the same change to the other handler referenced (lines ~254-270) and add a
regression test in test/port/controller/TeamController/index.test.ts to assert
an authenticated non-maintainer cannot grant package access (expect 403).
Comment on lines
+237
to
+240
| if (!body.package) { | ||
| throw new UnprocessableEntityError('package is required'); | ||
| } | ||
| const [scope, name] = getScopeAndName(body.package); |
Contributor
There was a problem hiding this comment.
Validate body.package as a string, not just truthy.
{ package: {} } currently passes the guard and then flows into the parse/lookup path as a non-string. That turns malformed input into parser/storage behavior instead of a deterministic 422. A typebox schema here would make these endpoints reject bad payloads consistently.
As per coding guidelines, use @eggjs/typebox-validate for type-safe request validation in controllers.
Also applies to: 261-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/port/controller/TeamController.ts` around lines 237 - 240, Validate that
body.package is a string before parsing: replace the truthy check with an
explicit typeof check and throw UnprocessableEntityError when typeof
body.package !== 'string' so getScopeAndName only receives strings; apply the
same change in the other occurrence that currently guards package (the block
around the second usage) or, preferably, add a request schema using
`@eggjs/typebox-validate` for this controller action to enforce package:string and
let the validator reject bad payloads consistently.
Comment on lines
+188
to
+201
| ``` | ||
| 请求 GET /@scope/name（manifest / version / tarball） | ||
| ↓ | ||
| scope 不在 allowScopes → 公开包，无需鉴权 | ||
| ↓ | ||
| scope 在 allowScopes（self scope）： | ||
| 1. 查找包是否有 Team-Package 绑定 | ||
| 2. 无绑定 → 放行（所有人可读） | ||
| 3. 有绑定： | ||
| a. 未登录 → 401 | ||
| b. admin 用户 → 放行 | ||
| c. 用户在某个 Team 中且该 Team 被授权访问此包 → 放行 | ||
| d. 都不满足 → 403 | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language identifier to the fenced block.
The code fence starting at Line 188 is missing a language tag, which triggers markdownlint (MD040).
📝 Proposed fix
-```
+```text
请求 GET /@scope/name（manifest / version / tarball）
↓
scope 不在 allowScopes → 公开包，无需鉴权
@@
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 188-188: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/org-team.md` around lines 188 - 201, The fenced code block in
docs/org-team.md (the GET /@scope/name flow) lacks a language identifier which
triggers MD040; update the opening fence from ``` to ```text (i.e., add the
"text" language tag) so the block reads ```text and keep the closing ```
unchanged to satisfy markdownlint.
Comment on lines
+265
to
+268
| ### CDN 缓存行为 | ||
|
|
||
| - self scope 包的响应头设为 `Cache-Control: private, no-store`，不会被 CDN 缓存 | ||
| - 非 self scope 包保持原有 CDN 缓存策略不变 |
Contributor
There was a problem hiding this comment.
Cache-behavior docs conflict with implemented team-bound policy.
Line 267 currently says all self-scope packages are Cache-Control: private, no-store. The implementation path uses team-binding to decide private caching, so no-team-binding self-scope packages should not be documented as always private-cache-disabled.
📝 Proposed fix
-- self scope 包的响应头设为 `Cache-Control: private, no-store`，不会被 CDN 缓存
+- self scope 且有 Team 绑定的包：响应头设为 `Cache-Control: private, no-store`，不会被 CDN 缓存
+- self scope 且无 Team 绑定的包：保持原有 CDN 缓存策略
- 非 self scope 包保持原有 CDN 缓存策略不变📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ### CDN 缓存行为 | |
| - self scope 包的响应头设为 `Cache-Control: private, no-store`，不会被 CDN 缓存 | |
| - 非 self scope 包保持原有 CDN 缓存策略不变 | |
| ### CDN 缓存行为 | |
| - self scope 且有 Team 绑定的包：响应头设为 `Cache-Control: private, no-store`，不会被 CDN 缓存 | |
| - self scope 且无 Team 绑定的包：保持原有 CDN 缓存策略 | |
| - 非 self scope 包保持原有 CDN 缓存策略不变 |
🧰 Tools
🪛 LanguageTool
[grammar] ~267-~267: Use a hyphen to join words.
Context: ...localhost:7001 ``` ### CDN 缓存行为 - self scope 包的响应头设为 `Cache-Control: private, n...
(QB_NEW_EN_HYPHEN)
[grammar] ~268-~268: Use a hyphen to join words.
Context: ...: private, no-store`，不会被 CDN 缓存 - 非 self scope 包保持原有 CDN 缓存策略不变 ## API Endpoints...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/org-team.md` around lines 265 - 268, Update the "CDN 缓存行为" docs: don’t
state that all self-scope packages get `Cache-Control: private, no-store`;
instead document that only team-bound self-scope packages are marked
private/no-store per the team-binding policy, while self-scope packages without
team-binding follow the existing CDN caching strategy. Modify the bullet under
the "CDN 缓存行为" heading (the line that currently reads "self scope 包的响应头设为
`Cache-Control: private, no-store`，不会被 CDN 缓存") to reflect the conditional
behavior and reference the team-binding policy so readers know which self-scope
packages become private-cached versus those that retain normal CDN caching.
Comment on lines
+1
to
+8
| CREATE TABLE orgs ( | ||
| id BIGSERIAL PRIMARY KEY, | ||
| gmt_create timestamp(3) NOT NULL, | ||
| gmt_modified timestamp(3) NOT NULL, | ||
| org_id varchar(24) NOT NULL, | ||
| name varchar(214) NOT NULL, | ||
| description varchar(10240) DEFAULT NULL | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check existing PostgreSQL migrations for IF NOT EXISTS usage pattern
rg -n 'CREATE TABLE' sql/postgresql/ | head -30Repository: cnpm/cnpmcore
Length of output: 1714
🏁 Script executed:
# Check if MySQL migration for 3.77.0 exists and check its CREATE TABLE statements
cat sql/mysql/3.77.0.sqlRepository: cnpm/cnpmcore
Length of output: 3495
🏁 Script executed:
# Also verify the coding guidelines requirement for matching MySQL and PostgreSQL migrations
ls -la sql/mysql/ sql/postgresql/ | grep 3.77.0Repository: cnpm/cnpmcore
Length of output: 178
Add IF NOT EXISTS to all CREATE TABLE statements for idempotent migrations.
The MySQL migration includes IF NOT EXISTS guards for all five tables, but the PostgreSQL migration lacks them. This inconsistency would cause "relation already exists" errors if the migration is re-run.
♻️ Required fix for all CREATE TABLE statements
-CREATE TABLE orgs (
+CREATE TABLE IF NOT EXISTS orgs (
id BIGSERIAL PRIMARY KEY,
gmt_create timestamp(3) NOT NULL,
gmt_modified timestamp(3) NOT NULL,
org_id varchar(24) NOT NULL,
name varchar(214) NOT NULL,
description varchar(10240) DEFAULT NULL
);Apply the same change to org_members, teams, team_members, and team_packages tables.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| CREATE TABLE orgs ( | |
| id BIGSERIAL PRIMARY KEY, | |
| gmt_create timestamp(3) NOT NULL, | |
| gmt_modified timestamp(3) NOT NULL, | |
| org_id varchar(24) NOT NULL, | |
| name varchar(214) NOT NULL, | |
| description varchar(10240) DEFAULT NULL | |
| ); | |
| CREATE TABLE IF NOT EXISTS orgs ( | |
| id BIGSERIAL PRIMARY KEY, | |
| gmt_create timestamp(3) NOT NULL, | |
| gmt_modified timestamp(3) NOT NULL, | |
| org_id varchar(24) NOT NULL, | |
| name varchar(214) NOT NULL, | |
| description varchar(10240) DEFAULT NULL | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sql/postgresql/3.77.0.sql` around lines 1 - 8, The CREATE TABLE statements
for orgs, org_members, teams, team_members, and team_packages are missing
idempotency guards; update each CREATE TABLE (e.g., the orgs table definition
and the CREATE TABLE statements for org_members, teams, team_members,
team_packages) to include IF NOT EXISTS so rerunning the migration won't fail
with "relation already exists" errors — locate each CREATE TABLE and prepend IF
NOT EXISTS immediately after CREATE TABLE.
Comment on lines
+21
to
+24
| fooPkgDir = TestUtil.getFixtures('@cnpm/foo'); | ||
| demoDir = TestUtil.getFixtures('demo'); | ||
| userconfig = path.join(fooPkgDir, '.npmrc'); | ||
| await TestUtil.rm(userconfig); |
Contributor
There was a problem hiding this comment.
Use writable fixture copies instead of read-only fixtures.
Line 21 and Line 22 use TestUtil.getFixtures(...), but this suite writes config and runs npm in those dirs. That risks mutating shared fixtures and causing flaky tests.
♻️ Proposed fix
before(async () => {
cacheDir = TestUtil.mkdtemp();
- fooPkgDir = TestUtil.getFixtures('@cnpm/foo');
- demoDir = TestUtil.getFixtures('demo');
+ fooPkgDir = await TestUtil.copyFixtures('@cnpm/foo');
+ demoDir = await TestUtil.copyFixtures('demo');
userconfig = path.join(fooPkgDir, '.npmrc');
await TestUtil.rm(userconfig);
server = app.listen(0);
await once(server, 'listening');
registry = `http://127.0.0.1:${(server.address() as AddressInfo).port}`;
});
after(async () => {
await TestUtil.rm(userconfig);
await TestUtil.rm(cacheDir);
+ await TestUtil.cleanupFixtures();
server?.close();
});Also applies to: 53-53, 73-73, 133-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/cli/npm/team.test.ts` around lines 21 - 24, The tests are using
TestUtil.getFixtures(...) for directories that the suite writes to (fooPkgDir,
demoDir) which can mutate shared, read-only fixtures; change those calls to
TestUtil.copyFixtures(...) so each test gets a writable copy, ensure you call
TestUtil.cleanupFixtures() in the test suite's after()/teardown, and keep the
existing TestUtil.rm(userconfig) usage to remove the .npmrc in the copied
fixture; update other occurrences flagged (lines around 53, 73, 133) to the same
pattern.
Comment on lines
+17
to
+19
| let adminUser: any; | ||
| let normalUser: any; | ||
| let teamMember: any; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify explicit `any` usage in this test file.
rg -nP --type=ts '\b:\s*any\b' test/port/controller/package/ReadAccessAuth.test.tsRepository: cnpm/cnpmcore
Length of output: 133
🏁 Script executed:
# Get context around the test user declarations and understand their usage
head -50 test/port/controller/package/ReadAccessAuth.test.tsRepository: cnpm/cnpmcore
Length of output: 2293
🏁 Script executed:
# Look at TestUtil to see what createUser returns
grep -A 20 "export.*createUser" test/TestUtil.tsRepository: cnpm/cnpmcore
Length of output: 39
🏁 Script executed:
# Check how these variables are used throughout the test file
rg -n "adminUser|normalUser|teamMember" test/port/controller/package/ReadAccessAuth.test.tsRepository: cnpm/cnpmcore
Length of output: 997
🏁 Script executed:
# Check TestUtil.ts for createUser and createAdmin signatures
cat test/TestUtil.ts | head -100Repository: cnpm/cnpmcore
Length of output: 2697
🏁 Script executed:
# Search for createUser and createAdmin definitions in TestUtil
rg -A 15 "(createUser|createAdmin)" test/TestUtil.tsRepository: cnpm/cnpmcore
Length of output: 1989
Replace any with the proper TestUser type.
These variables are assigned from TestUtil.createUser() and TestUtil.createAdmin(), which return TestUser objects. Import and use the exported TestUser interface instead of any to maintain strict typing and match the actual return types.
import { TestUser } from '../../../TestUtil.ts';
// ... existing code ...
let adminUser: TestUser;
let normalUser: TestUser;
let teamMember: TestUser;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/port/controller/package/ReadAccessAuth.test.ts` around lines 17 - 19,
The three variables adminUser, normalUser, and teamMember are typed as any but
are assigned from TestUtil.createUser()/TestUtil.createAdmin() which return
TestUser; import the exported TestUser interface (e.g., from the TestUtil
module) and change the declarations for adminUser, normalUser, and teamMember to
use TestUser so the test uses strict, correct types.
fengmk2
approved these changes
Mar 31, 2026
fengmk2
pushed a commit
that referenced
this pull request
Apr 1, 2026
|
🎉 This PR is included in version 4.29.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
npm teamcommands)Cherry-picked PRs (from 3.x)
Test plan
npm run typecheckpassesnpm run lintpassesnpm run test:local test/core/service/OrgService.test.tsnpm run test:local test/core/service/TeamService.test.tsnpm run test:local test/port/controller/OrgController/index.test.tsnpm run test:local test/port/controller/TeamController/index.test.tsnpm run test:local test/port/controller/package/ReadAccessAuth.test.tsnpm run test:local test/cli/npm/team.test.ts🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation