Files
coder/coderd/organizationskills_acl.go
Michael Suchacz 2c432a12a3 feat: add organization skill sharing ACL routes (#30611)
## 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.
2026-10-13 16:43:15 +00:00

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)
}