mirror of
https://github.com/coder/coder.git
synced 2026-10-16 14:48:56 +08:00
## 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.
192 lines
7.3 KiB
Go
192 lines
7.3 KiB
Go
package coderd
|
|
|
|
import (
|
|
"errors"
|
|
"maps"
|
|
"net/http"
|
|
|
|
"golang.org/x/xerrors"
|
|
|
|
"github.com/coder/coder/v2/coderd/audit"
|
|
"github.com/coder/coder/v2/coderd/database"
|
|
"github.com/coder/coder/v2/coderd/database/dbauthz"
|
|
"github.com/coder/coder/v2/coderd/httpapi"
|
|
"github.com/coder/coder/v2/coderd/httpmw"
|
|
"github.com/coder/coder/v2/coderd/rbac/acl"
|
|
"github.com/coder/coder/v2/codersdk"
|
|
)
|
|
|
|
// @Summary Get an organization skill ACL
|
|
// @ID get-an-organization-skill-acl
|
|
// @Security CoderSessionToken
|
|
// @Produce json
|
|
// @Tags Organizations
|
|
// @Param organization path string true "Organization ID or name"
|
|
// @Param skillName path string true "Skill name"
|
|
// @Success 200 {object} codersdk.OrganizationSkillACL
|
|
// @Router /api/experimental/organizations/{organization}/skills/{skillName}/acl [get]
|
|
// @x-apidocgen {"skip": true}
|
|
func (api *API) getOrganizationSkillACL(rw http.ResponseWriter, r *http.Request) { //nolint:revive // Method name matches route.
|
|
ctx := r.Context()
|
|
skill := httpmw.OrganizationSkillParam(r)
|
|
|
|
users, err := resolveACLUsers(ctx, api.Database, skill.UserACL, func(user codersdk.MinimalUser) codersdk.OrganizationSkillUser {
|
|
return codersdk.OrganizationSkillUser{MinimalUser: user, Role: codersdk.OrganizationSkillRoleRead}
|
|
})
|
|
if err != nil {
|
|
httpapi.InternalServerError(rw, err)
|
|
return
|
|
}
|
|
groups, err := resolveACLGroups(ctx, api.Database, skill.GroupACL, func(group codersdk.Group) codersdk.OrganizationSkillGroup {
|
|
return codersdk.OrganizationSkillGroup{Group: group, Role: codersdk.OrganizationSkillRoleRead}
|
|
})
|
|
if err != nil {
|
|
httpapi.InternalServerError(rw, err)
|
|
return
|
|
}
|
|
httpapi.Write(ctx, rw, http.StatusOK, codersdk.OrganizationSkillACL{
|
|
Users: users,
|
|
Groups: groups,
|
|
})
|
|
}
|
|
|
|
// @Summary Get available organization skill ACL users and groups
|
|
// @ID get-available-organization-skill-acl-users-and-groups
|
|
// @Security CoderSessionToken
|
|
// @Produce json
|
|
// @Tags Organizations
|
|
// @Param organization path string true "Organization ID or name"
|
|
// @Param skillName path string true "Skill name"
|
|
// @Param q query string false "User search query; free-text search also applies to groups"
|
|
// @Param after_id query string false "User after ID" format(uuid)
|
|
// @Param limit query int false "Page limit for users and groups, if 0 returns all candidates"
|
|
// @Param offset query int false "User page offset"
|
|
// @Success 200 {object} codersdk.ACLAvailable
|
|
// @Router /api/experimental/organizations/{organization}/skills/{skillName}/acl/available [get]
|
|
// @x-apidocgen {"skip": true}
|
|
func (api *API) getOrganizationSkillACLAvailable(rw http.ResponseWriter, r *http.Request) { //nolint:revive // Method name matches route.
|
|
api.writeOrganizationACLAvailable(rw, r, httpmw.OrganizationSkillParam(r).OrganizationID.UUID)
|
|
}
|
|
|
|
// @Summary Update an organization skill ACL
|
|
// @ID update-an-organization-skill-acl
|
|
// @Security CoderSessionToken
|
|
// @Accept json
|
|
// @Tags Organizations
|
|
// @Param organization path string true "Organization ID or name"
|
|
// @Param skillName path string true "Skill name"
|
|
// @Param request body codersdk.UpdateOrganizationSkillACLRequest true "Update organization skill ACL request"
|
|
// @Success 204
|
|
// @Router /api/experimental/organizations/{organization}/skills/{skillName}/acl [patch]
|
|
// @x-apidocgen {"skip": true}
|
|
func (api *API) patchOrganizationSkillACL(rw http.ResponseWriter, r *http.Request) {
|
|
var (
|
|
ctx = r.Context()
|
|
skill = httpmw.OrganizationSkillParam(r)
|
|
organizationID = skill.OrganizationID.UUID
|
|
auditor = api.Auditor.Load()
|
|
aReq, commitAudit = audit.InitRequest[database.AuditableOrganizationSkill](rw, &audit.RequestParams{
|
|
Audit: *auditor,
|
|
Log: api.Logger,
|
|
Request: r,
|
|
Action: database.AuditActionWrite,
|
|
OrganizationID: organizationID,
|
|
})
|
|
)
|
|
defer commitAudit()
|
|
aReq.Old = database.AuditableOrganizationSkill{Skill: skill}
|
|
|
|
var req codersdk.UpdateOrganizationSkillACLRequest
|
|
if !httpapi.Read(ctx, rw, r, &req) {
|
|
return
|
|
}
|
|
|
|
var updated database.Skill
|
|
err := api.Database.InTx(func(tx database.Store) error {
|
|
// Lock granted users' memberships before the skill row: a member
|
|
// removal locks its membership row and then, through its trigger, the
|
|
// skill rows, so the reverse order could deadlock. Holding the lock
|
|
// keeps a removal from missing the grant written below.
|
|
if userIDs := activeACLIDs(req.UserRoles); len(userIDs) > 0 {
|
|
//nolint:gocritic // Validation below reports users that are not members.
|
|
_, err := tx.LockOrganizationMembersByUserIDsForShare(dbauthz.AsSystemRestricted(ctx), database.LockOrganizationMembersByUserIDsForShareParams{
|
|
OrganizationID: organizationID,
|
|
UserIds: userIDs,
|
|
})
|
|
if err != nil {
|
|
return xerrors.Errorf("lock organization members: %w", err)
|
|
}
|
|
}
|
|
//nolint:gocritic // The ACL write below reauthorizes the locked row for share.
|
|
current, err := tx.GetOrganizationSkillByIDForUpdate(dbauthz.AsSystemRestricted(ctx), skill.ID)
|
|
if err != nil {
|
|
return xerrors.Errorf("get organization skill for update: %w", err)
|
|
}
|
|
userRoles, groupRoles, validations := validateOrganizationACLUpdate(ctx, tx, organizationID, organizationSkillACLUpdateValidator(req))
|
|
if len(validations) > 0 {
|
|
return &organizationSkillACLValidationError{validations: validations}
|
|
}
|
|
aReq.Old = database.AuditableOrganizationSkill{Skill: current}
|
|
userACL := maps.Clone(current.UserACL)
|
|
groupACL := maps.Clone(current.GroupACL)
|
|
applyACLReadRoles(userACL, userRoles)
|
|
applyACLReadRoles(groupACL, groupRoles)
|
|
updated, err = tx.UpdateOrganizationSkillACLByID(ctx, database.UpdateOrganizationSkillACLByIDParams{
|
|
ID: skill.ID,
|
|
UserACL: userACL,
|
|
GroupACL: groupACL,
|
|
})
|
|
if err != nil {
|
|
return xerrors.Errorf("update organization skill ACL: %w", err)
|
|
}
|
|
return nil
|
|
}, nil)
|
|
if err != nil {
|
|
var validationErr *organizationSkillACLValidationError
|
|
if errors.As(err, &validationErr) {
|
|
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
|
|
Message: "Invalid request to update organization skill ACL.",
|
|
Validations: validationErr.validations,
|
|
})
|
|
return
|
|
}
|
|
// A delete between the middleware read and the row lock stays
|
|
// concealed as 404, matching the update and delete handlers.
|
|
if httpapi.Is404Error(err) {
|
|
httpapi.ResourceNotFound(rw)
|
|
return
|
|
}
|
|
httpapi.InternalServerError(rw, err)
|
|
return
|
|
}
|
|
aReq.New = database.AuditableOrganizationSkill{Skill: updated}
|
|
rw.WriteHeader(http.StatusNoContent)
|
|
}
|
|
|
|
type organizationSkillACLValidationError struct {
|
|
validations []codersdk.ValidationError
|
|
}
|
|
|
|
func (*organizationSkillACLValidationError) Error() string {
|
|
return "invalid organization skill ACL"
|
|
}
|
|
|
|
type organizationSkillACLUpdateValidator codersdk.UpdateOrganizationSkillACLRequest
|
|
|
|
var _ acl.UpdateValidator[codersdk.OrganizationSkillRole] = organizationSkillACLUpdateValidator{}
|
|
|
|
func (v organizationSkillACLUpdateValidator) Users() (map[string]codersdk.OrganizationSkillRole, string) {
|
|
return v.UserRoles, "user_roles"
|
|
}
|
|
|
|
func (v organizationSkillACLUpdateValidator) Groups() (map[string]codersdk.OrganizationSkillRole, string) {
|
|
return v.GroupRoles, "group_roles"
|
|
}
|
|
|
|
func (organizationSkillACLUpdateValidator) ValidateRole(role codersdk.OrganizationSkillRole) error {
|
|
if role == codersdk.OrganizationSkillRoleDeleted || role == codersdk.OrganizationSkillRoleRead {
|
|
return nil
|
|
}
|
|
return xerrors.Errorf("role %q is not a valid organization skill role", role)
|
|
}
|