mirror of
https://github.com/coder/coder.git
synced 2026-10-16 14:48:56 +08:00
Michael Suchacz
2c432a12a3
## Stack Part of [CODAGT-1341](https://linear.app/codercom/issue/CODAGT-1341/support-organization-level-skills-in-coder-agents). This stack adds organization skills to Coder Agents: admins publish a skill once per organization, share it with groups or members, and chats in that organization load it next to personal and workspace skills. It replaces the 21-PR draft stack #30584 to #30604 with five PRs, bottom to top: 1. #30609 data model: one migration, queries, RBAC and dbauthz 2. #30610 organization skill API routes and the scope-neutral SDK 3. **#30611** sharing ACL routes (this PR) 4. #30612 settings UI: shared skills table, admin page, member section 5. #30613 chats: chatd loading, `/` menu, docs ## What changes This PR adds the routes that let admins choose who can read an organization skill. It reuses the MCP server ACL machinery rather than adding a second implementation. Routes under `/api/experimental/organizations/{organization}/skills/{skillName}`, all behind `httpmw.ExtractOrganizationSkillParam` with `share`, so a caller without share gets 404: - `GET /acl` returns the resolved user and group grants. - `GET /acl/available` returns the organization's users and groups that can be granted, with the same `q`, `after_id`, `limit` and `offset` paging as the MCP endpoint. - `PATCH /acl` takes a sparse `UpdateOrganizationSkillACLRequest`: only the listed principals change, `read` grants and `""` removes. It returns 204 and writes a `write` audit entry with the old and new ACL. **Concurrency (please review closely):** the PATCH transaction first locks the granted users' `organization_members` rows `FOR KEY SHARE` (new query `LockOrganizationMembersByUserIDsForShare`, run as system), then locks the skill row `FOR UPDATE`, then validates the update on the transaction handle. Member removal takes the same locks in the same order (its membership row, then the skill rows through the #30609 trigger), so the two can't deadlock, and a removal can't miss a grant written by a concurrent PATCH. A skill deleted between the middleware read and the row lock stays a 404. Grants are users and groups, the same as MCP server configs (Mike's decision, for parity). Only the `read` role exists; any other role is a validation error, as are users or groups outside the organization. Supporting changes: - `coderd/mcp_acl.go`: the MCP ACL handlers' resolution, candidate listing and validation move into generic helpers (`writeOrganizationACLAvailable`, `validateOrganizationACLUpdate`, `resolveACLUsers`, `resolveACLGroups`) shared by both resources; `acl.Validate` accepts a named `acl.Role` constraint that now includes `codersdk.OrganizationSkillRole`. - `codersdk/skills.go`: `OrganizationSkillRole`, `OrganizationSkillACL` with its user and group entries, `UpdateOrganizationSkillACLRequest`, and client methods. - New sqlc query `UpdateOrganizationSkillACLByID` with its dbauthz wrapper (requires `share`). ## Commits The first commit reproduces the reviewed code from the old stack; each later commit is a labeled trim or fix. - `b1c85560b69` feat: add organization skill sharing ACL routes. Reproduction: every file this commit changes equals the reviewed old `acl-routes` head `80c1c3594e6`; the rest of the tree differs only by #30609's migration merge and main. - `b5d2ee265c8` test(coderd): merge duplicate organization skill ACL route tests (+49/-111). The removed cases repeated `TestMCPServerConfigACL`, `TestMCPServerConfigACLAvailable` or `TestRolePermissions`; with the MCP tests included, the covered block set of `organizationskills_acl.go`, `mcp_acl.go` and `rbac/acl/updatevalidator.go` is identical before and after. Kept: `TestOrganizationSkillACL` (`OrgAdminManagesACL`, `ReadersGetNotFound`, `SparseUpdateChangesReaders` with the audit diff, `RejectsUnsupportedRole`, `ScopedAPIKeys`), `TestOrganizationSkillACLConcurrentDelete`, and the lock order tests `TestOrganizationSkillACLMemberRemoval` (`RemovalBeforeGrant`, `RemovalAfterGrant`). `OrgAdminManagesACL` now also checks that users from another organization are not candidates (mutation checked). Sharing the param extractor with MCP was skipped: it would save about 10 lines of security-sensitive code. - `e09c4dba66b` Merge the updated org-api layer into sharing. Brings in #30610's `ad0a7a37504`; the merge has no changes of its own. - `1d3817dc639` and `c8c88f84d18` merge the updated #30610 layer, including main; no changes of their own. - `c4d22c47990` merges the updated #30610 layer (main at `e7b76872098`); no changes of its own. ## Size Production +431/-102, tests +377/-0, generated +918/-0; total +1726/-102. This is above the 500-line reviewer-friendly limit on purpose: Mike asked for 4 to 5 PRs instead of the 21 small ones this stack replaces. ## Replaces #30591 (ACL queries), #30592 (ACL routes). ## Validation - `go build` and `go vet` passed; `make gen` exited 0 with no diff. - `go test` passed for `coderd` (skill, MCP and ACL tests), `coderd/database`, `dbauthz`, `migrations`, `gentest`, `coderd/rbac/...`, `coderd/httpmw`, `coderd/x/skills/...`, `coderd/x/chatd/...`, and the audit and skill tests in `enterprise/audit` and `enterprise/coderd`. - The covered block comparison above, before and after the test merge. - On `c4d22c47990`: the same checks as #30610's head, plus `go test ./coderd/ -run 'Skill|MCP|ACL'`, pass. - Remote UAT round 7 through Coder Agents passed on the whole stack; see #30613's Validation for scope and findings. > Xum opened this PR on behalf of @ibetitsmike.