feat(controller): allow L2 humans to update worker skills within their teams - #1212
feat(controller): allow L2 humans to update worker skills within their teams#1212LUOSENGWA wants to merge 1 commit into
Conversation
a2fc047 to
5c22b59
Compare
5c22b59 to
02843ef
Compare
shiyiyue1102
left a comment
There was a problem hiding this comment.
Thanks for working on L2 self-service management. Allowing L2 users to view and edit non-sensitive Worker, Skill, and MCP configuration within their own teams is a reasonable and valuable direction. The concern is not that these operations must remain admin-only, but that the current permission boundary treats every L2 user and every configuration field in the same way.
Please separate the permission model into at least two levels:
- By default, L2 users may view and edit non-sensitive configuration for Workers in their
accessibleTeams. - Sensitive operations and data require an explicit FullAccess-like capability granted by an admin. This includes revealing credentials, managing arbitrary external Skill sources, and binding credentials to external endpoints.
Editing an MCP address itself can remain a normal L2 capability, but the Worker Gateway bearer token must not automatically be attached to an arbitrary user-provided URL. It should only be attached to trusted Gateway endpoints, or credentials should be managed through a separate credentialRef.
Similarly, assigning a Skill from the public catalog can be a default L2 capability, while adding an arbitrary remote registry and accessing its credentials should require the elevated permission.
Please avoid using the generic Worker ActionUpdate as the permission boundary for all these capabilities. Separate actions make it possible to authorize normal configuration, external sources, and sensitive access independently.
Before continuing with the implementation, please open a GitHub Issue describing the L2 permission matrix, sensitive-field classification, FullAccess assignment model, and credential handling. Link the Issue to this PR so other community members can participate in the design discussion.
感谢补充 L2 自助管理能力。允许 L2 用户在自己的 accessibleTeams 范围内查看和编辑非敏感的 Worker、Skill 和 MCP 配置,是合理且有价值的方向。当前问题并不是这些操作必须继续由 Admin 独占,而是现有权限边界把所有 L2 用户和所有配置字段都按同一种权限处理了。
建议至少拆分成两级权限:
- L2 默认可以查看和编辑本团队 Worker 的非敏感配置。
- 敏感操作和数据需要由 Admin 显式授予类似 FullAccess 的能力,包括查看凭据、管理任意外部 Skill Source,以及向外部地址绑定凭据。
编辑 MCP 地址本身可以作为普通 L2 能力,但不能把 Worker Gateway Bearer Token 自动附加到任意用户填写的 URL。Bearer Token 只能发送给可信 Gateway 地址,或者通过独立的 credentialRef 管理。
同样,从公共 Catalog 为 Worker 分配 Skill 可以默认开放给 L2;新增任意远程注册表以及访问其凭据,则应要求更高权限。
请不要使用通用的 Worker ActionUpdate 统一承载这些能力。应将普通配置、外部数据源和敏感数据访问拆成独立 Action,才能分别授权。
继续实现前,请创建一个 GitHub Issue,说明 L2 权限矩阵、敏感字段分类、FullAccess 分配方式和凭据处理规则,并关联到本 PR,让社区其他同学也能参与设计讨论。
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Adds a code-level L2-human boundary on PUT /api/v1/workers/{name}: team-scoped access (404 cross-team/standalone, probe-resistant) plus a field whitelist limited to skills/remoteSkills/mcpServers, with authorizer pass-through and thorough handler tests. The mechanics are well implemented, but the maintainer's outstanding CHANGES_REQUESTED design review (permission levels, sensitive-field classification, separate actions instead of generic ActionUpdate, design Issue first) is unresolved, and our code-level check confirms the credential concern behind it is concrete.
Findings
- [Critical] resource_handler.go:257-262 — the whitelist permits L2-set
mcpServers, andagentconfig.GenerateMcporterConfiginjectsAuthorization: Bearer <gatewayKey>into every MCP entry using the URL verbatim, so an L2 human can currently point a worker MCP entry at an arbitrary URL and receive the gateway consumer key there (credential exfiltration path). Same class of risk applies to arbitraryremoteSkillsregistries with credentials. - [Warning] resource_handler.go:896 — the forbidden-field list is fail-open: future fields added to
UpdateWorkerRequestbecome L2-writable by default; suggest a reflection-based sync test or inverting to an allowlist. - [Info] The whitelist is complete against the current
UpdateWorkerRequeststruct (all 13 non-whitelisted fields covered), the 404-vs-403 probe-resistance matches the read path, and the authorizer/handler layering is consistent with the project-write pattern — good.
Suggestions
Holding off on approval pending the design discussion requested by the maintainer (L2 permission matrix Issue + action separation). The inline findings are meant as concrete evidence for that discussion, not as a separate approval path.
Automated review by github-manager-bot
| if req.Skills != nil { | ||
| worker.Spec.Skills = req.Skills | ||
| } | ||
| if req.RemoteSkills != nil { |
There was a problem hiding this comment.
L2-controlled remoteSkills / mcpServers reach the worker verbatim. Note that agentconfig.GenerateMcporterConfig (internal/agentconfig/mcporter.go) uses each MCP server URL as-is and unconditionally injects Authorization: Bearer <gatewayKey> — the same consumer key used for LLM access. With this PR an L2 human can register an MCP entry pointing at an arbitrary external URL, so the worker would deliver the gateway consumer key to an attacker-controlled endpoint (credential exfiltration). This matches the maintainer's review: bearer attachment must be restricted to trusted gateway endpoints or moved to an explicit credentialRef, and arbitrary remote skill registries should require an elevated (FullAccess-like) permission.
| // may only touch the self-service fields (skills / remoteSkills / | ||
| // mcpServers); everything else (model, image, identity, resources, ...) is | ||
| // the team owner's domain. Returns (0, "") when the update is allowed. | ||
| func (h *ResourceHandler) checkHumanWorkerUpdate(ctx context.Context, caller *authpkg.CallerIdentity, name string, req *UpdateWorkerRequest) (int, string) { |
There was a problem hiding this comment.
The field policy here is fail-open: it enumerates the forbidden fields, so any field added to UpdateWorkerRequest in the future becomes L2-writable by default until someone remembers to extend this list. Consider a reflection-based test that asserts every non-whitelisted field of UpdateWorkerRequest appears in the forbidden list (or invert the check to enumerate the allowed fields), so the security boundary degrades to deny-by-default when the request type grows.
02843ef to
42335c2
Compare
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of the amended head (42335c2): the security findings from the previous review are both addressed. remoteSkills and mcpServers are now closed to default L2 callers (only public-catalog skills remains L2-writable), closing the gateway-key exfiltration path through GenerateMcporterConfig — verified against main: the generator attaches Authorization: Bearer <gatewayKey> to every MCP entry using the URL verbatim, so keeping mcpServers behind the elevated capability (pending #1220) is the right call. The fail-open field-policy concern is resolved by TestL2WorkerUpdateFieldPolicyCoversAllRequestFields, the reflection-based pin that fails if any future UpdateWorkerRequest field lacks an explicit policy decision. The new design doc and the linked design issue #1220 respond to the maintainer's request for a design discussion. Two user-facing doc tables are stale (see inline), and the PR description still describes the old whitelist.
Findings
- [Warning] docs/usage/resource-management.md:190 — L2 row lists
skills,remoteSkills,mcpServers; code now allows onlyskills(remoteSkills/mcpServers → 400 pending #1220) - [Warning] docs/zh-cn/usage/resource-management.md:190 — same stale L2 row in the zh-cn doc
Suggestions
- Update both usage-doc tables (and the paragraphs under them) to "
skillsonly" with a pointer to the #1220 elevated-capability design; also refresh the PR body's field-whitelist bullet so reviewers don't get conflicting signals. - Verified against main@ac22c88:
findTeamMembersignature matches the call site (first-matching-team semantics, consistent with the read path);TeamMatches/CallerFromContext/CallerKeyForTestall exist;GenerateMcporterConfigbearer injection confirmed. The 404-vs-403 probe resistance and admin/leader regression coverage look consistent with the project-write pattern. - Final approval still rests on the maintainer's CHANGES_REQUESTED design review and the #1220 discussion; no APPROVE from this bot at this stage.
Automated review by github-manager-bot
| |------|-------|--------| | ||
| | admin / manager | any worker | all fields | | ||
| | team leader | workers in their team | all fields | | ||
| | L2 human (`permissionLevel: 2`) | workers in `accessibleTeams` only | `skills`, `remoteSkills`, `mcpServers` | |
There was a problem hiding this comment.
This row is stale relative to the tightened implementation: checkHumanWorkerUpdate now rejects remoteSkills and mcpServers for default L2 callers (400, elevated capability pending #1220), so only skills is L2-writable. The design doc (docs/design/l2-worker-scoped-write.md) states this correctly, but this user-facing table still advertises skills, remoteSkills, mcpServers — and the paragraph below the table repeats it. Suggest: skills (public-catalog assignment) only, with a note that remoteSkills/mcpServers require the elevated capability once #1220 lands.
| |------|------|------| | ||
| | admin / manager | 任意 Worker | 全部字段 | | ||
| | 团队 Leader | 本团队 Worker | 全部字段 | | ||
| | L2 人类用户(`permissionLevel: 2`) | 仅 `accessibleTeams` 内的 Worker | `skills`、`remoteSkills`、`mcpServers` | |
There was a problem hiding this comment.
Same staleness as the English doc: the L2 row lists skills, remoteSkills, mcpServers, but the code now allows only skills for default L2 (remoteSkills/mcpServers → 400 pending the #1220 elevated-capability design). The surrounding sentence ("L2 人类用户可自主管理…远程注册表技能、MCP 服务器") needs the same correction.
42335c2 to
8393875
Compare
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of the amended head (8393875): the security lockdown from the previous revision is intact and both stale doc tables flagged in the previous review are fixed. The maintainer's design-level CHANGES_REQUESTED items (separate permission actions instead of generic ActionUpdate, credentialRef, FullAccess-style tiering per #1220) remain open, so the approval decision is left to the maintainer.
Verified:
- L2 field policy intact — only
skills(public-catalog assignment) is L2-writable;remoteSkillsandmcpServersare rejected with 400, with the gateway-key exfiltration rationale documented incheckHumanWorkerUpdate. The deny-by-default reflection pin (TestL2WorkerUpdateFieldPolicyCoversAllRequestFields) still guards the policy. - Team scope + W8 probe-resistance (cross-team/standalone → 404) unchanged.
- [Warning → fixed]
docs/usage/resource-management.md:190-192and the zh-cn counterpart now list onlyskillsfor L2 and explain whyremoteSkills/mcpServersstay gated until #1220.
CI note: integration-tests (controller-cr, SHARD_C_TESTS, qwenpaw, qwenpaw) is failing on this PR's latest run while the same shard passes on #1210 — please investigate whether the failure is PR-specific or flaky.
Local verification: go test ./internal/server/ ./internal/auth/ green on this head.
Automated review by github-manager-bot
Worker skill/MCP self-service updates for team-scoped humans
Summary
PUT /api/v1/workers/{name}was only reachable by admins, managers, and team leaders. A team-scoped human (aHumanCR withpermissionLevel: 2) could read the workers in theiraccessibleTeamsand drive projects there, but could not adjust the capabilities of the workers they coordinate: enabling a built-in skill, a remote registry skill, or an MCP server required escalating to an admin for every change.This PR opens the existing update endpoint to that caller class with a code-level boundary:
accessibleTeams(same team-membership lookup the list endpoints use). Out-of-scope workers — cross-team or standalone — are hidden from team-scoped humans on the update path the same way the read path hides them (404), so a scoped human cannot probe worker existence or learn a worker's owning team.skills,remoteSkills, andmcpServersmay be set. Any other field in the body (model,image,soul,agents,runtime,package,expose,channelPolicy,resources,containerManaged,state, …) is rejected with400naming the offending fields. Ownership, persona, image, network, and lifecycle remain the team owner's domain.The layering matches the existing project write endpoints: the middleware cannot resolve
worker -> team, so the authorizer change (ActionUpdateonworkerfor humans →requireSameTeam) is a pass-through for the scoped request, and the handler is the single enforcement point. The request type also gainsremoteSkills, which the CRD and deployer already support but the API could not set even for admins.What's included
internal/auth/authorizer.go—ActionUpdateonworkerfor team-scoped humans routes throughrequireSameTeaminstead of a hard deny (teamless humans are still rejected at the middleware).internal/server/resource_handler.go—UpdateWorkerenforces the boundary for human callers: team scope (404 cross-team and standalone, probe-resistant) + field whitelist (400, offending fields named). AppliesremoteSkillsfor all callers.internal/server/types.go—UpdateWorkerRequest.remoteSkills.docs/design/l2-worker-scoped-write.md— design contract.docs/usage/resource-management.md(+ zh-cn) — role matrix for worker updates.Data boundary
remoteSkillsalready exists in the Worker CRD and is consumed by the deployer's skill-push path; this PR only makes it settable through the API.PUT /api/v1/workers/{name}is unchanged (covered by regression tests).404(never403), so the endpoint does not reveal worker names or team ownership to team-scoped humans.Tests
internal/auth/authorizer_test.go—TestAuthorizer_HumanScoped: in-scope and unresolved-teamupdate workerallowed at the authorizer layer, cross-team denied; create/delete/wake/sleep on workers still denied.internal/server/resource_handler_l2_update_test.go(new, 8 cases) — in-scope skills/remoteSkills/mcpServers update applies and returns 200; cross-team worker 404; standalone worker 404; off-whitelist fields 400 with field names; empty body no-op 200; admin full update unchanged; team leader update unchanged; teamless human hidden (404 at the handler; the middleware rejects it first).go test ./internal/server/ ./internal/auth/green; fullgo test ./...run: 19 packages ok, the only failure is a pre-existing environment issue ininternal/executor(unzip binary absent in the test container) untouched by this diff.gofmt/go vetclean.Related
docs/design/l2-worker-scoped-write.md.团队人类用户的 Worker 技能/MCP 自助更新
摘要
PUT /api/v1/workers/{name}此前只有 admin、manager、团队 Leader 可调。团队范围的人类用户(permissionLevel: 2的 Human CR)能读到自己accessibleTeams内的 worker、能驱动项目,却无法调整自己协调的 worker 的能力配置——启用一个内置技能、远程注册表技能或 MCP 服务器,每次都要升级到 admin。本 PR 在现有更新端点上为该角色打开入口,代码级双层边界:
accessibleTeams内某团队的成员(与列表端点同一套成员查询)。越权 worker(跨团队或独立)写路径与读路径同样隐藏(404),L2 无法探测 worker 存在性或所属团队,端点保持防探测。skills、remoteSkills、mcpServers。body 触碰任何其他字段(model、image、soul、agents、runtime、package、expose、channelPolicy、resources、containerManaged、state等)即400拒绝并点名。归属、人设、镜像、网络、生命周期仍属团队所有者权限。分层与既有项目写端点一致:中间件无法解析
worker -> team,authorizer 改动(人类对 worker 的ActionUpdate走requireSameTeam)对无团队上下文的请求是放行,真正强制点在 handler。请求结构体顺带补上remoteSkills——CRD 和 deployer 早已支持,但 API 此前连 admin 都改不了。包含内容
internal/auth/authorizer.go— 人类对 worker 的ActionUpdate改走requireSameTeam(无团队的 L2 仍在中间件被拒)。internal/server/resource_handler.go—UpdateWorker对人类调用方强制边界:团队范围(跨团队与独立 worker 均 404,防探测)+ 字段白名单(400 点名)。对全部调用方应用remoteSkills。internal/server/types.go—UpdateWorkerRequest.remoteSkills。docs/design/l2-worker-scoped-write.md— 设计契约。docs/usage/resource-management.md(+ zh-cn)— Worker 更新的角色权限矩阵。数据边界
remoteSkills在 Worker CRD 中已存在且被 deployer 的技能推送链路消费,本 PR 只是让它可通过 API 设置。PUT /api/v1/workers/{name}行为不变(有回归测试覆盖)。404(而非403),不向 L2 泄露 worker 名称或团队归属。测试
internal/auth/authorizer_test.go—TestAuthorizer_HumanScoped:本团队与无团队上下文的update worker在 authorizer 层放行,跨团队拒绝;worker 的 create/delete/wake/sleep 仍拒绝。internal/server/resource_handler_l2_update_test.go(新,8 用例)— 本团队 skills/remoteSkills/mcpServers 更新生效返回 200;跨团队 worker 404;独立 worker 404;白名单外字段 400 且点名;空 body 无操作 200;admin 全字段更新不变;团队 Leader 更新行为不变;无团队 L2 隐藏(handler 404,中间件先行拒绝)。go test ./internal/server/ ./internal/auth/全绿;全量go test ./...:19 包 ok,唯一失败为internal/executor的预存环境问题(测试容器缺 unzip),与本 diff 无关。gofmt/go vet干净。相关
docs/design/l2-worker-scoped-write.md。