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 (this PR) 2. #30610 organization skill API routes and the scope-neutral SDK 3. #30611 sharing ACL routes 4. #30612 settings UI: shared skills table, admin page, member section 5. #30613 chats: chatd loading, `/` menu, docs ## What changes This PR adds the data model and authorization for organization skills. Personal skills keep working unchanged: `git diff origin/main -- codersdk site` shows only generated RBAC and API key scope additions (`apikey_scopes_gen.go`, `rbacresources_gen.go`, `rbacresourcesGenerated.ts`, the RBAC and scope unions in `typesGenerated.ts`) and `codersdk.ResourceTypeOrganizationSkill` with its friendly string. No personal route, SDK type or site component changes, and `coderd/userskills_test.go` changes only by mechanical constant renames. **One migration, `000621_skills`** (please review it closely): - renames `user_skills` to `skills` with its constraints, index and triggers, and recreates the cap function and `delete_deleted_user_resources`, whose PL/pgSQL bodies name the table; - adds `organization_id` and `project_id` (`ON DELETE CASCADE`), `enabled` (default true), and `group_acl` / `user_acl` (`jsonb`, default `{}`), with CHECKs for exactly one owner, ACLs that are JSON objects, and ACLs that stay empty unless an organization owns the row, plus partial unique name indexes per owner kind; - replaces the cap trigger with `enforce_skills_per_owner_limit()`: 100 rows per owner, disabled rows included, locking the owner row `FOR NO KEY UPDATE` so unrelated foreign key checks are not blocked; - adds a `BEFORE DELETE` trigger on `organization_members` that removes a departing member from `user_acl` on that organization's skills, so rejoining doesn't restore a direct grant; - adds the `organization_skill` `resource_type` and `api_key_scope` enum values last; nothing in the migration uses them. The down migration undoes the three old migrations in reverse order: it drops organization and project rows and restores `user_skills`, its names, the cap function and `user_id NOT NULL`. Authorization: a new RBAC resource `organization_skill` (create, read, update, delete, share). Owner and org admin get it; site and org auditors get read; members read only through the skill's ACL, which applies only to members of its organization. `Skill.RBACObject()` returns the organization object with both ACLs; project rows stay denied to everyone. New sqlc queries cover organization insert, get, list (SQL-filtered through `GetAuthorizedOrganizationSkillMetadata` and a `regosql` converter), update and delete, with dbauthz wrappers and the audit table entries. No routes yet. ## Commits The first commit reproduces the reviewed code from the old stack; each later commit is a labeled trim or fix. - `baa3150e3b7` feat: add the organization skills data model. Reproduction: every non-site file this commit changes equals the reviewed old `authz` head `1e16b3004c4`, except that the old stack's migrations 000615, 000616 and 000617 are merged into one migration (then `000615_skills`, now `000621_skills`) with one fixture (`testdata/fixtures/000621_skills.up.sql`) and one migration test (now `testMigration000621Skills`). At that commit, `dump.sql` was byte-identical to the old head's. - `1b84e83463a` test(coderd/database): keep only the SQL paths of the org skill matrix. `TestOrganizationSkillAuthorization` becomes `TestOrganizationSkillQueries` instead of being deleted, because coverage showed it is this PR's only run of the `GetAuthorizedOrganizationSkillMetadata` row scan (80% to 65% without it) and of the organization update and delete queries (100% to 0%). Swapping `group_acl` and `user_acl` in the converter makes `GrantedMember` fail. The removed cases are decided by `TestRolePermissions`, `TestSkillRBACObjectOrganizationACL` and the dbauthz method suite. - `e2931b412e9` test(coderd/database): fold the skill cap cases into existing coverage. The user case is covered by `TestUserSkillLimit` and `TestUserSkillLimitConcurrentCreates`; the organization case folds into `ConcurrentOrganizationInsertsHonorCap`, which now also checks that disabled rows count and another organization is unaffected (mutation checked); the project case stays. - `be62aea9021` test(coderd/rbac): drop `TestOrganizationSkillReadScope`, covered by `TestExternalScopeNames` and the generated scope file. - `0c275ba37de` merges main at `454a2fcc697`, which took 000615 to 000618: it renumbers the migration to 000619 and resolves `dump.sql`, `check_constraint.go` and the generated API docs. `8f2b4b87ee2` merges main at `ad85646164e`, which took 000619: it renumbers the migration to 000620. - `5248d56b64f` merges main at `e7b76872098`, which took 000620 (`000620_audit_chat_project_instructions_resource_type`): it renumbers the migration, fixture and migration test to `000621_skills` (step `Migration000621Skills`, version 621), and resolves `enterprise/audit/table.go` (keeping main's `AuditableChatProjectInstructions` entries and the `Skill` entry), `dump.sql` and `models.go` (the `resource_type` enum gets `chat_project_instructions`, then `organization_skill`) and the regenerated `schemas.md`. The up and down SQL are unchanged by every renumbering. ## Size Production +549/-67, tests +728/-33, generated +881/-314; total +2158/-414. 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 #30584 (rename `user_skills` to `skills`), #30585 (skill owners), #30586 (organization skill authorization). ## Validation - Migration test: up keeps the existing personal row and its ID; down restores the `user_skills` names, cap function and `NOT NULL`, and drops organization rows; soft-deleting a user removes their skills after up, after down, and after re-up. - `go test` passed on `be62aea9021`, before the main merges, for `coderd` (skill tests), `coderd/database`, `dbauthz`, `migrations`, `gentest`, `coderd/rbac/...`, `coderd/x/chatd`, `coderd/x/skills`, `enterprise/audit` and `enterprise/coderd`; `make lint/check-scopes` passed; the four personal skills Vitest files pass unchanged. - On `5248d56b64f`: build, vet, `TestMigrate`, `TestMigrationChain`, the `migrations`, `database`, `dbauthz`, `gentest`, `rbac` and `enterprise/audit` packages pass; `make gen` run twice is clean, and `make pre-commit` passes. - 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.
351 lines
10 KiB
Go
351 lines
10 KiB
Go
package coderd
|
|
|
|
import (
|
|
"context"
|
|
"errors"
|
|
"fmt"
|
|
"net/http"
|
|
|
|
"github.com/go-chi/chi/v5"
|
|
"github.com/google/uuid"
|
|
"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/db2sdk"
|
|
"github.com/coder/coder/v2/coderd/httpapi"
|
|
"github.com/coder/coder/v2/coderd/httpmw"
|
|
"github.com/coder/coder/v2/coderd/x/skills"
|
|
"github.com/coder/coder/v2/codersdk"
|
|
)
|
|
|
|
const (
|
|
// personalSkillJSONEscapeExpansion is the maximum expansion for one byte in a JSON string.
|
|
personalSkillJSONEscapeExpansion = 6
|
|
// personalSkillRequestEnvelopeBytes leaves room for the surrounding JSON object.
|
|
personalSkillRequestEnvelopeBytes = 1024
|
|
// maxPersonalSkillRequestBytes allows worst-case JSON string escaping for
|
|
// otherwise valid raw skill content.
|
|
maxPersonalSkillRequestBytes = skills.MaxPersonalSkillSizeBytes*personalSkillJSONEscapeExpansion + personalSkillRequestEnvelopeBytes
|
|
|
|
// These names are raised by trigger functions with USING CONSTRAINT.
|
|
// They are not table CHECK constraints, so dbgen does not emit them in
|
|
// check_constraint.go.
|
|
skillsPerUserLimitConstraint database.CheckConstraint = "skills_per_user_limit"
|
|
userSkillUserDeletedConstraint database.CheckConstraint = "user_skill_user_deleted"
|
|
)
|
|
|
|
// @Summary Create a user skill
|
|
// @ID create-a-user-skill
|
|
// @Security CoderSessionToken
|
|
// @Accept json
|
|
// @Produce json
|
|
// @Tags Users
|
|
// @Param user path string true "User ID, username, or me"
|
|
// @Param request body codersdk.CreateUserSkillRequest true "Create user skill request"
|
|
// @Success 201 {object} codersdk.UserSkill
|
|
// @Router /api/experimental/users/{user}/skills [post]
|
|
// @x-apidocgen {"skip": true}
|
|
func (api *API) postUserSkill(rw http.ResponseWriter, r *http.Request) {
|
|
var (
|
|
ctx = r.Context()
|
|
user = httpmw.UserParam(r)
|
|
auditor = api.Auditor.Load()
|
|
aReq, commitAudit = audit.InitRequest[database.Skill](rw, &audit.RequestParams{
|
|
Audit: *auditor,
|
|
Log: api.Logger,
|
|
Request: r,
|
|
Action: database.AuditActionCreate,
|
|
})
|
|
)
|
|
defer commitAudit()
|
|
|
|
var req codersdk.CreateUserSkillRequest
|
|
if !httpapi.ReadLimit(ctx, rw, r, maxPersonalSkillRequestBytes, &req) {
|
|
return
|
|
}
|
|
|
|
parsedSkill, err := skills.ParsePersonalSkillMarkdown([]byte(req.Content))
|
|
if err != nil {
|
|
writeInvalidUserSkillContent(ctx, rw, err)
|
|
return
|
|
}
|
|
|
|
params := database.InsertUserSkillParams{
|
|
ID: uuid.New(),
|
|
UserID: user.ID,
|
|
Name: parsedSkill.Name,
|
|
Description: parsedSkill.Description,
|
|
Content: req.Content,
|
|
}
|
|
skill, err := api.Database.InsertUserSkill(ctx, params)
|
|
if err != nil {
|
|
if httpapi.IsUnauthorizedError(err) {
|
|
httpapi.Forbidden(rw)
|
|
return
|
|
}
|
|
if database.IsCheckViolation(err, userSkillUserDeletedConstraint) {
|
|
writeCannotCreateUserSkillForDeletedUser(ctx, rw)
|
|
return
|
|
}
|
|
if httpapi.Is404Error(err) {
|
|
httpapi.ResourceNotFound(rw)
|
|
return
|
|
}
|
|
if database.IsCheckViolation(err, skillsPerUserLimitConstraint) {
|
|
writeUserSkillLimitReached(ctx, rw)
|
|
return
|
|
}
|
|
if database.IsUniqueViolation(err, database.UniqueSkillsUserIDNameIndex) {
|
|
httpapi.Write(ctx, rw, http.StatusConflict, codersdk.Response{
|
|
Message: "A skill with that name already exists.",
|
|
Detail: err.Error(),
|
|
})
|
|
return
|
|
}
|
|
httpapi.InternalServerError(rw, err)
|
|
return
|
|
}
|
|
aReq.New = skill
|
|
|
|
httpapi.Write(ctx, rw, http.StatusCreated, db2sdk.UserSkill(skill))
|
|
}
|
|
|
|
// @Summary List user skills
|
|
// @ID list-user-skills
|
|
// @Security CoderSessionToken
|
|
// @Produce json
|
|
// @Tags Users
|
|
// @Param user path string true "User ID, username, or me"
|
|
// @Success 200 {array} codersdk.UserSkillMetadata
|
|
// @Router /api/experimental/users/{user}/skills [get]
|
|
// @x-apidocgen {"skip": true}
|
|
func (api *API) getUserSkills(rw http.ResponseWriter, r *http.Request) { //nolint:revive // Method name matches route.
|
|
ctx := r.Context()
|
|
user := httpmw.UserParam(r)
|
|
|
|
rows, err := api.Database.ListUserSkillMetadataByUserID(ctx, user.ID)
|
|
if err != nil {
|
|
if httpapi.Is404Error(err) {
|
|
httpapi.ResourceNotFound(rw)
|
|
return
|
|
}
|
|
httpapi.InternalServerError(rw, err)
|
|
return
|
|
}
|
|
|
|
httpapi.Write(ctx, rw, http.StatusOK, db2sdk.UserSkillMetadataList(rows))
|
|
}
|
|
|
|
// @Summary Get a user skill by name
|
|
// @ID get-a-user-skill-by-name
|
|
// @Security CoderSessionToken
|
|
// @Produce json
|
|
// @Tags Users
|
|
// @Param user path string true "User ID, username, or me"
|
|
// @Param skillName path string true "Skill name"
|
|
// @Success 200 {object} codersdk.UserSkill
|
|
// @Router /api/experimental/users/{user}/skills/{skillName} [get]
|
|
// @x-apidocgen {"skip": true}
|
|
func (api *API) getUserSkill(rw http.ResponseWriter, r *http.Request) { //nolint:revive // Method name matches route.
|
|
ctx := r.Context()
|
|
user := httpmw.UserParam(r)
|
|
name := chi.URLParam(r, "skillName")
|
|
|
|
skill, err := api.Database.GetUserSkillByUserIDAndName(ctx, database.GetUserSkillByUserIDAndNameParams{
|
|
UserID: user.ID,
|
|
Name: name,
|
|
})
|
|
if err != nil {
|
|
if httpapi.Is404Error(err) {
|
|
httpapi.ResourceNotFound(rw)
|
|
return
|
|
}
|
|
httpapi.InternalServerError(rw, err)
|
|
return
|
|
}
|
|
|
|
httpapi.Write(ctx, rw, http.StatusOK, db2sdk.UserSkill(skill))
|
|
}
|
|
|
|
// @Summary Update a user skill
|
|
// @ID update-a-user-skill
|
|
// @Security CoderSessionToken
|
|
// @Accept json
|
|
// @Produce json
|
|
// @Tags Users
|
|
// @Param user path string true "User ID, username, or me"
|
|
// @Param skillName path string true "Skill name"
|
|
// @Param request body codersdk.UpdateUserSkillRequest true "Update user skill request"
|
|
// @Success 200 {object} codersdk.UserSkill
|
|
// @Router /api/experimental/users/{user}/skills/{skillName} [patch]
|
|
// @x-apidocgen {"skip": true}
|
|
func (api *API) patchUserSkill(rw http.ResponseWriter, r *http.Request) {
|
|
var (
|
|
ctx = r.Context()
|
|
user = httpmw.UserParam(r)
|
|
name = chi.URLParam(r, "skillName")
|
|
auditor = api.Auditor.Load()
|
|
aReq, commitAudit = audit.InitRequest[database.Skill](rw, &audit.RequestParams{
|
|
Audit: *auditor,
|
|
Log: api.Logger,
|
|
Request: r,
|
|
Action: database.AuditActionWrite,
|
|
})
|
|
)
|
|
defer commitAudit()
|
|
|
|
var req codersdk.UpdateUserSkillRequest
|
|
if !httpapi.ReadLimit(ctx, rw, r, maxPersonalSkillRequestBytes, &req) {
|
|
return
|
|
}
|
|
|
|
parsedSkill, err := skills.ParsePersonalSkillMarkdown([]byte(req.Content))
|
|
if err != nil {
|
|
writeInvalidUserSkillContent(ctx, rw, err)
|
|
return
|
|
}
|
|
if parsedSkill.Name != name {
|
|
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
|
|
Message: "Skill name in path does not match frontmatter name.",
|
|
Detail: fmt.Sprintf("path has %q, frontmatter has %q", name, parsedSkill.Name),
|
|
})
|
|
return
|
|
}
|
|
|
|
params := database.UpdateUserSkillByUserIDAndNameParams{
|
|
UserID: user.ID,
|
|
Name: name,
|
|
Description: parsedSkill.Description,
|
|
Content: req.Content,
|
|
}
|
|
|
|
var (
|
|
skill database.Skill
|
|
oldSkill database.Skill
|
|
)
|
|
err = api.Database.InTx(func(tx database.Store) error {
|
|
fetched, err := tx.GetUserSkillByUserIDAndName(ctx, database.GetUserSkillByUserIDAndNameParams{
|
|
UserID: user.ID,
|
|
Name: name,
|
|
})
|
|
if err != nil {
|
|
return xerrors.Errorf("fetch user skill: %w", err)
|
|
}
|
|
|
|
updated, err := tx.UpdateUserSkillByUserIDAndName(ctx, params)
|
|
if err != nil {
|
|
return xerrors.Errorf("update user skill: %w", err)
|
|
}
|
|
oldSkill = fetched
|
|
skill = updated
|
|
return nil
|
|
}, nil)
|
|
if err != nil {
|
|
if httpapi.IsUnauthorizedError(err) {
|
|
httpapi.Forbidden(rw)
|
|
return
|
|
}
|
|
if database.IsCheckViolation(err, userSkillUserDeletedConstraint) {
|
|
writeCannotModifyUserSkillForDeletedUser(ctx, rw)
|
|
return
|
|
}
|
|
if httpapi.Is404Error(err) {
|
|
httpapi.ResourceNotFound(rw)
|
|
return
|
|
}
|
|
httpapi.InternalServerError(rw, err)
|
|
return
|
|
}
|
|
|
|
// Assign audit state after InTx returns so the audit log can never
|
|
// claim a rolled-back update was committed.
|
|
aReq.Old = oldSkill
|
|
aReq.New = skill
|
|
|
|
httpapi.Write(ctx, rw, http.StatusOK, db2sdk.UserSkill(skill))
|
|
}
|
|
|
|
// @Summary Delete a user skill
|
|
// @ID delete-a-user-skill
|
|
// @Security CoderSessionToken
|
|
// @Tags Users
|
|
// @Param user path string true "User ID, username, or me"
|
|
// @Param skillName path string true "Skill name"
|
|
// @Success 204
|
|
// @Router /api/experimental/users/{user}/skills/{skillName} [delete]
|
|
// @x-apidocgen {"skip": true}
|
|
func (api *API) deleteUserSkill(rw http.ResponseWriter, r *http.Request) {
|
|
var (
|
|
ctx = r.Context()
|
|
user = httpmw.UserParam(r)
|
|
name = chi.URLParam(r, "skillName")
|
|
auditor = api.Auditor.Load()
|
|
aReq, commitAudit = audit.InitRequest[database.Skill](rw, &audit.RequestParams{
|
|
Audit: *auditor,
|
|
Log: api.Logger,
|
|
Request: r,
|
|
Action: database.AuditActionDelete,
|
|
})
|
|
)
|
|
defer commitAudit()
|
|
|
|
deleted, err := api.Database.DeleteUserSkillByUserIDAndName(ctx, database.DeleteUserSkillByUserIDAndNameParams{
|
|
UserID: user.ID,
|
|
Name: name,
|
|
})
|
|
if err != nil {
|
|
if httpapi.Is404Error(err) {
|
|
httpapi.ResourceNotFound(rw)
|
|
return
|
|
}
|
|
httpapi.InternalServerError(rw, err)
|
|
return
|
|
}
|
|
aReq.Old = deleted
|
|
|
|
rw.WriteHeader(http.StatusNoContent)
|
|
}
|
|
|
|
func writeCannotCreateUserSkillForDeletedUser(ctx context.Context, rw http.ResponseWriter) {
|
|
httpapi.Write(ctx, rw, http.StatusConflict, codersdk.Response{
|
|
Message: "Cannot create skills for deleted users.",
|
|
Detail: "This user has been deleted and cannot be modified.",
|
|
})
|
|
}
|
|
|
|
func writeCannotModifyUserSkillForDeletedUser(ctx context.Context, rw http.ResponseWriter) {
|
|
httpapi.Write(ctx, rw, http.StatusConflict, codersdk.Response{
|
|
Message: "Cannot modify skills for deleted users.",
|
|
Detail: "This user has been deleted and cannot be modified.",
|
|
})
|
|
}
|
|
|
|
func writeUserSkillLimitReached(ctx context.Context, rw http.ResponseWriter) {
|
|
httpapi.Write(ctx, rw, http.StatusConflict, codersdk.Response{
|
|
Message: "Personal skill limit reached.",
|
|
Detail: fmt.Sprintf(
|
|
"Each user can have at most %d personal skills.",
|
|
skills.MaxPersonalSkillsPerUser,
|
|
),
|
|
})
|
|
}
|
|
|
|
func writeInvalidUserSkillContent(ctx context.Context, rw http.ResponseWriter, err error) {
|
|
message := "Invalid skill content."
|
|
switch {
|
|
case errors.Is(err, skills.ErrInvalidSkillName):
|
|
message = "Invalid skill name."
|
|
case errors.Is(err, skills.ErrSkillBodyRequired):
|
|
message = "Skill body is required."
|
|
case errors.Is(err, skills.ErrSkillTooLarge):
|
|
message = "Skill content is too large."
|
|
case errors.Is(err, skills.ErrSkillDescriptionTooLarge):
|
|
message = "Skill description is too large."
|
|
}
|
|
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
|
|
Message: message,
|
|
Detail: err.Error(),
|
|
})
|
|
}
|