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 (this PR) 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 organization skill CRUD routes and makes the skill SDK types owner-neutral so personal and organization skills share them. **API contract change (experimental API, personal and organization skills):** skill list and get responses gain `enabled`, and `PATCH .../skills/{skillName}` now takes `{content?, enabled?}`, so `content` is optional and a toggle doesn't resend it. A PATCH with neither field returns 400; `content`, when present, is validated as before. In the SDK, `UserSkill`, `UserSkillMetadata`, `CreateUserSkillRequest` and `UpdateUserSkillRequest` become `Skill`, `SkillMetadata`, `CreateSkillRequest` and `UpdateSkillRequest` (`codersdk/userskills.go` moves to `codersdk/skills.go`), and `UpdateSkillRequest.Content` is now `*string`. The personal client method names are unchanged. The site changes are the handwritten edits that rename forces (`api.ts`, `queries/userSkills.ts`, `testHelpers/skills.ts` and three call sites), with no UI change. Organization routes under `/api/experimental/organizations/{organization}/skills`, not gated by an experiment or license, like the personal ones: - `GET` lists with the caller's own permissions, so admins, the site owner and org auditors see every skill and members see only skills shared with them. `GET /{skillName}` goes through the new `httpmw.ExtractOrganizationSkillParam` and admits only read, so a missing skill and a denied one both return 404. - `POST` checks `create` on the organization before reading the body, then seeds `group_acl` with the Everyone group (whose ID is the organization ID) so a new skill reaches every member until an admin narrows it. The 100-skill cap and a duplicate name return 409. - `PATCH` admits only callers who can read the skill, because its response includes the content; `DELETE` admits callers who can read or delete it. A reader who can't change the skill gets a write-specific 403 from the database wrapper, and everyone else gets 404. PATCH locks the row as system, updates as the caller in the same transaction, and audits the locked row only after the transaction commits. - Audit: `database.AuditableOrganizationSkill` records resource type `organization_skill` with the organization ID; personal skills keep `user_skill`. Personal and organization handlers share one create and update path in `coderd/userskills.go` (`createSkill`, `updateSkill`, and a `skillWriteErrors` table per owner and action). ## Commits The first commit reproduces the reviewed code from the old stack; each later commit is a labeled trim or fix. - `49cb2c84276` feat: add organization skill API routes. Reproduction: every file this commit changes, including the SDK layer's site edits, equals the reviewed old `write-routes` head `aa597100f28`; the rest of the tree differs only by #30609's migration merge and main. - `dc893bc60d6` refactor(coderd): share the skill create and update path across owners (production +164/-188). It keeps the organization create check before the body read, the lock as system then update as caller order, and audit assignment after the transaction. No swagger, `codersdk` or `typesGenerated.ts` change; `make gen` is clean. - `f94925425ef` test(coderd): merge duplicate personal and organization skill tests. `TestSkillsCRUD`, `TestSkillValidationAndConflicts` and `TestSkillAudit` are table-driven over Personal, OrgAdmin and SiteOwner, alongside `TestOrganizationSkillAuditsDeniedWrites` and one `ScopedAPIKeys` subtest; `TestPatchUserSkill` is deleted because none of its blocks were unique. Coverage of `userskills.go`, `organizationskills.go` and `httpmw/organizationskillparam.go` is 101 of 117 blocks before and after, the same block set. - `ad0a7a37504` fix(coderd): require read access to patch an organization skill. The organization PATCH route now admits only `read`, because its response carries the skill's content: a key scoped to `organization_skill:update` without read gets 404, like GET, instead of 200 with the content. Readers without update still get 403, and DELETE is unchanged. `TestOrganizationSkillAccess/ScopedAPIKeys` asserts the 404, that the response has no content, and that the skill is unchanged; against the old route it fails with 200. - `69d00ded0b6` and `f67b79343ec` merge the updated #30609 layer, including main; `69d00ded0b6` resolves a conflict in the personal skills page with main. No other changes of their own. - `18373ffd59e` merges the updated #30609 layer (main at `e7b76872098`) and reapplies the `AuditableOrganizationSkill` entry in `enterprise/audit/table.go` alongside main's new `AuditableChatProjectInstructions` entries. ## Size Production +838/-308, tests +733/-322, generated +801/-274; total +2372/-904. 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 #30587 (scope-neutral SDK and personal enabled state), #30588 (read routes), #30589 (write routes and audit). ## Validation - The coverage comparison above, before and after the test merge. - `make gen` leaves no diff after the shared create and update refactor. - On `18373ffd59e`: `make gen` is clean; build, vet, the `coderd` and `enterprise/coderd` skill tests, `rbac`, `httpmw`, `dbauthz` and `enterprise/audit` pass; `pnpm check`, `pnpm lint` and `make pre-commit` 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.
752 lines
28 KiB
Go
752 lines
28 KiB
Go
package coderd_test
|
|
|
|
import (
|
|
"context"
|
|
"database/sql"
|
|
"encoding/json"
|
|
"fmt"
|
|
"net/http"
|
|
"strings"
|
|
"testing"
|
|
|
|
"github.com/google/uuid"
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
|
|
"github.com/coder/coder/v2/coderd/audit"
|
|
"github.com/coder/coder/v2/coderd/coderdtest"
|
|
"github.com/coder/coder/v2/coderd/database"
|
|
"github.com/coder/coder/v2/coderd/database/dbauthz"
|
|
"github.com/coder/coder/v2/coderd/database/dbgen"
|
|
"github.com/coder/coder/v2/coderd/database/dbtestutil"
|
|
"github.com/coder/coder/v2/coderd/rbac"
|
|
"github.com/coder/coder/v2/coderd/util/ptr"
|
|
"github.com/coder/coder/v2/coderd/x/skills"
|
|
"github.com/coder/coder/v2/codersdk"
|
|
"github.com/coder/coder/v2/testutil"
|
|
)
|
|
|
|
// skillOwnerAPI drives the personal or organization skill routes through the
|
|
// same calls, so behavior both owners share is asserted for each of them.
|
|
type skillOwnerAPI struct {
|
|
path string
|
|
resourceType database.ResourceType
|
|
auditOrgID uuid.UUID
|
|
client *codersdk.ExperimentalClient
|
|
create func(context.Context, codersdk.CreateSkillRequest) (codersdk.Skill, error)
|
|
list func(context.Context) ([]codersdk.SkillMetadata, error)
|
|
get func(context.Context, string) (codersdk.Skill, error)
|
|
update func(context.Context, string, codersdk.UpdateSkillRequest) (codersdk.Skill, error)
|
|
delete func(context.Context, string) error
|
|
}
|
|
|
|
func personalSkillAPI(client *codersdk.ExperimentalClient) skillOwnerAPI {
|
|
return skillOwnerAPI{
|
|
path: "/api/experimental/users/me/skills",
|
|
resourceType: database.ResourceTypeUserSkill,
|
|
client: client,
|
|
create: func(ctx context.Context, req codersdk.CreateSkillRequest) (codersdk.Skill, error) {
|
|
return client.CreateUserSkill(ctx, codersdk.Me, req)
|
|
},
|
|
list: func(ctx context.Context) ([]codersdk.SkillMetadata, error) {
|
|
return client.UserSkills(ctx, codersdk.Me)
|
|
},
|
|
get: func(ctx context.Context, name string) (codersdk.Skill, error) {
|
|
return client.UserSkillByName(ctx, codersdk.Me, name)
|
|
},
|
|
update: func(ctx context.Context, name string, req codersdk.UpdateSkillRequest) (codersdk.Skill, error) {
|
|
return client.UpdateUserSkill(ctx, codersdk.Me, name, req)
|
|
},
|
|
delete: func(ctx context.Context, name string) error { return client.DeleteUserSkill(ctx, codersdk.Me, name) },
|
|
}
|
|
}
|
|
|
|
func organizationSkillAPI(client *codersdk.ExperimentalClient, orgID uuid.UUID) skillOwnerAPI {
|
|
return skillOwnerAPI{
|
|
path: fmt.Sprintf("/api/experimental/organizations/%s/skills", orgID),
|
|
resourceType: database.ResourceTypeOrganizationSkill,
|
|
auditOrgID: orgID,
|
|
client: client,
|
|
create: func(ctx context.Context, req codersdk.CreateSkillRequest) (codersdk.Skill, error) {
|
|
return client.CreateOrganizationSkill(ctx, orgID, req)
|
|
},
|
|
list: func(ctx context.Context) ([]codersdk.SkillMetadata, error) {
|
|
return client.OrganizationSkills(ctx, orgID)
|
|
},
|
|
get: func(ctx context.Context, name string) (codersdk.Skill, error) {
|
|
return client.OrganizationSkillByName(ctx, orgID, name)
|
|
},
|
|
update: func(ctx context.Context, name string, req codersdk.UpdateSkillRequest) (codersdk.Skill, error) {
|
|
return client.UpdateOrganizationSkill(ctx, orgID, name, req)
|
|
},
|
|
delete: func(ctx context.Context, name string) error { return client.DeleteOrganizationSkill(ctx, orgID, name) },
|
|
}
|
|
}
|
|
|
|
// skillOwnerCases each start a deployment and return a caller that manages
|
|
// one owner's skills, plus a peer that manages another owner of the same kind.
|
|
var skillOwnerCases = []struct {
|
|
name string
|
|
setup func(t *testing.T, opts *coderdtest.Options) (owner, peer skillOwnerAPI)
|
|
}{
|
|
{
|
|
name: "Personal",
|
|
setup: func(t *testing.T, opts *coderdtest.Options) (skillOwnerAPI, skillOwnerAPI) {
|
|
adminClient := coderdtest.New(t, opts)
|
|
firstUser := coderdtest.CreateFirstUser(t, adminClient)
|
|
ownerClient, _ := coderdtest.CreateAnotherUser(t, adminClient, firstUser.OrganizationID)
|
|
peerClient, _ := coderdtest.CreateAnotherUser(t, adminClient, firstUser.OrganizationID)
|
|
return personalSkillAPI(codersdk.NewExperimentalClient(ownerClient)), personalSkillAPI(codersdk.NewExperimentalClient(peerClient))
|
|
},
|
|
},
|
|
{
|
|
name: "OrgAdmin",
|
|
setup: func(t *testing.T, opts *coderdtest.Options) (skillOwnerAPI, skillOwnerAPI) {
|
|
ownerClient, db := coderdtest.NewWithDatabase(t, opts)
|
|
firstUser := coderdtest.CreateFirstUser(t, ownerClient)
|
|
orgAdminClient, _ := coderdtest.CreateAnotherUser(t, ownerClient, firstUser.OrganizationID, rbac.ScopedRoleOrgAdmin(firstUser.OrganizationID))
|
|
peerOrg := dbgen.Organization(t, db, database.Organization{})
|
|
return organizationSkillAPI(codersdk.NewExperimentalClient(orgAdminClient), firstUser.OrganizationID),
|
|
organizationSkillAPI(codersdk.NewExperimentalClient(ownerClient), peerOrg.ID)
|
|
},
|
|
},
|
|
{
|
|
name: "SiteOwner",
|
|
setup: func(t *testing.T, opts *coderdtest.Options) (skillOwnerAPI, skillOwnerAPI) {
|
|
ownerClient, db := coderdtest.NewWithDatabase(t, opts)
|
|
firstUser := coderdtest.CreateFirstUser(t, ownerClient)
|
|
client := codersdk.NewExperimentalClient(ownerClient)
|
|
peerOrg := dbgen.Organization(t, db, database.Organization{})
|
|
return organizationSkillAPI(client, firstUser.OrganizationID), organizationSkillAPI(client, peerOrg.ID)
|
|
},
|
|
},
|
|
}
|
|
|
|
func TestSkillsCRUD(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
for _, tc := range skillOwnerCases {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
owner, _ := tc.setup(t, nil)
|
|
ctx := testutil.Context(t, testutil.WaitMedium)
|
|
|
|
emptyList, err := owner.list(ctx)
|
|
require.NoError(t, err)
|
|
assert.NotNil(t, emptyList)
|
|
assert.Empty(t, emptyList)
|
|
rawEmptyList := requireRawSkillList(ctx, t, owner)
|
|
assert.NotNil(t, rawEmptyList)
|
|
assert.Empty(t, rawEmptyList)
|
|
|
|
content := userSkillMarkdown("crud-skill", "Initial description", "Use this skill for CRUD tests.")
|
|
created, err := owner.create(ctx, codersdk.CreateSkillRequest{Content: content})
|
|
require.NoError(t, err)
|
|
assert.NotZero(t, created.ID)
|
|
assert.Equal(t, "crud-skill", created.Name)
|
|
assert.Equal(t, "Initial description", created.Description)
|
|
assert.Equal(t, content, created.Content)
|
|
assert.True(t, created.Enabled)
|
|
assert.NotZero(t, created.CreatedAt)
|
|
assert.NotZero(t, created.UpdatedAt)
|
|
|
|
list, err := owner.list(ctx)
|
|
require.NoError(t, err)
|
|
require.Len(t, list, 1)
|
|
assert.Equal(t, created.ID, list[0].ID)
|
|
assert.Equal(t, "crud-skill", list[0].Name)
|
|
assert.Equal(t, "Initial description", list[0].Description)
|
|
assert.True(t, list[0].Enabled)
|
|
rawList := requireRawSkillList(ctx, t, owner)
|
|
require.Len(t, rawList, 1)
|
|
assert.NotContains(t, rawList[0], "content")
|
|
|
|
got, err := owner.get(ctx, "crud-skill")
|
|
require.NoError(t, err)
|
|
assert.Equal(t, created.ID, got.ID)
|
|
assert.Equal(t, content, got.Content)
|
|
|
|
updatedContent := userSkillMarkdown("crud-skill", "Updated description", "Updated body.")
|
|
updated, err := owner.update(ctx, "crud-skill", codersdk.UpdateSkillRequest{Content: ptr.Ref(updatedContent)})
|
|
require.NoError(t, err)
|
|
assert.Equal(t, created.ID, updated.ID)
|
|
assert.Equal(t, "Updated description", updated.Description)
|
|
assert.Equal(t, updatedContent, updated.Content)
|
|
assert.True(t, updated.Enabled)
|
|
|
|
disabled, err := owner.update(ctx, "crud-skill", codersdk.UpdateSkillRequest{Enabled: ptr.Ref(false)})
|
|
require.NoError(t, err)
|
|
assert.False(t, disabled.Enabled)
|
|
assert.Equal(t, updatedContent, disabled.Content)
|
|
assert.Equal(t, "Updated description", disabled.Description)
|
|
list, err = owner.list(ctx)
|
|
require.NoError(t, err)
|
|
require.Len(t, list, 1)
|
|
assert.False(t, list[0].Enabled)
|
|
got, err = owner.get(ctx, "crud-skill")
|
|
require.NoError(t, err)
|
|
assert.False(t, got.Enabled)
|
|
|
|
require.NoError(t, owner.delete(ctx, "crud-skill"))
|
|
_, err = owner.get(ctx, "crud-skill")
|
|
requireSDKErrorStatus(t, err, http.StatusNotFound)
|
|
})
|
|
}
|
|
}
|
|
|
|
func requireRawSkillList(ctx context.Context, t *testing.T, owner skillOwnerAPI) []map[string]json.RawMessage {
|
|
t.Helper()
|
|
res, err := owner.client.Request(ctx, http.MethodGet, owner.path, nil)
|
|
require.NoError(t, err)
|
|
defer res.Body.Close()
|
|
require.Equal(t, http.StatusOK, res.StatusCode)
|
|
var raw []map[string]json.RawMessage
|
|
require.NoError(t, json.NewDecoder(res.Body).Decode(&raw))
|
|
return raw
|
|
}
|
|
|
|
func TestSkillValidationAndConflicts(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
for _, tc := range skillOwnerCases {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
owner, peer := tc.setup(t, nil)
|
|
tests := []struct {
|
|
name string
|
|
content string
|
|
expectedMessage string
|
|
}{
|
|
{
|
|
name: "MissingFrontmatterDelimiters",
|
|
content: "name: missing-frontmatter\n\nBody.",
|
|
expectedMessage: "Invalid skill content.",
|
|
},
|
|
{
|
|
name: "MissingName",
|
|
content: "---\n" +
|
|
"description: Missing name\n" +
|
|
"---\n\nBody.",
|
|
expectedMessage: "Invalid skill name.",
|
|
},
|
|
{
|
|
name: "NonKebabCaseName",
|
|
content: userSkillMarkdown("NotKebab", "Invalid", "Body."),
|
|
expectedMessage: "Invalid skill name.",
|
|
},
|
|
{
|
|
name: "NameTooLong",
|
|
content: userSkillMarkdown(strings.Repeat("a", skills.MaxPersonalSkillNameBytes+1), "Invalid", "Body."),
|
|
expectedMessage: "Invalid skill name.",
|
|
},
|
|
{
|
|
name: "EmptyBody",
|
|
content: userSkillMarkdown("empty-body", "Invalid", " \n"),
|
|
expectedMessage: "Skill body is required.",
|
|
},
|
|
{
|
|
name: "TooLarge",
|
|
content: strings.Repeat("a", skills.MaxPersonalSkillSizeBytes+1),
|
|
expectedMessage: "Skill content is too large.",
|
|
},
|
|
}
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
ctx := testutil.Context(t, testutil.WaitMedium)
|
|
_, err := owner.create(ctx, codersdk.CreateSkillRequest{Content: tt.content})
|
|
sdkErr := requireSDKErrorStatus(t, err, http.StatusBadRequest)
|
|
assert.Equal(t, tt.expectedMessage, sdkErr.Message)
|
|
})
|
|
}
|
|
|
|
t.Run("PatchEmptyBody", func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
ctx := testutil.Context(t, testutil.WaitMedium)
|
|
_, err := owner.create(ctx, codersdk.CreateSkillRequest{Content: userSkillMarkdown("patch-validation", "Valid", "Body.")})
|
|
require.NoError(t, err)
|
|
_, err = owner.update(ctx, "patch-validation", codersdk.UpdateSkillRequest{
|
|
Content: ptr.Ref(userSkillMarkdown("patch-validation", "Invalid", " \n")),
|
|
})
|
|
sdkErr := requireSDKErrorStatus(t, err, http.StatusBadRequest)
|
|
assert.Equal(t, "Skill body is required.", sdkErr.Message)
|
|
})
|
|
|
|
t.Run("PatchNoFields", func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
ctx := testutil.Context(t, testutil.WaitMedium)
|
|
_, err := owner.create(ctx, codersdk.CreateSkillRequest{Content: userSkillMarkdown("patch-no-fields", "Valid", "Body.")})
|
|
require.NoError(t, err)
|
|
_, err = owner.update(ctx, "patch-no-fields", codersdk.UpdateSkillRequest{})
|
|
sdkErr := requireSDKErrorStatus(t, err, http.StatusBadRequest)
|
|
assert.Equal(t, "No skill fields to update.", sdkErr.Message)
|
|
})
|
|
|
|
t.Run("PatchNameMismatch", func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
ctx := testutil.Context(t, testutil.WaitMedium)
|
|
_, err := owner.create(ctx, codersdk.CreateSkillRequest{Content: userSkillMarkdown("old-name", "Old", "Body.")})
|
|
require.NoError(t, err)
|
|
_, err = owner.update(ctx, "old-name", codersdk.UpdateSkillRequest{
|
|
Content: ptr.Ref(userSkillMarkdown("new-name", "New", "Body.")),
|
|
})
|
|
sdkErr := requireSDKErrorStatus(t, err, http.StatusBadRequest)
|
|
assert.Equal(t, "Skill name in path does not match frontmatter name.", sdkErr.Message)
|
|
assert.Equal(t, `path has "old-name", frontmatter has "new-name"`, sdkErr.Detail)
|
|
})
|
|
|
|
t.Run("MissingSkill", func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
ctx := testutil.Context(t, testutil.WaitMedium)
|
|
_, err := owner.get(ctx, "missing-skill")
|
|
requireSDKErrorStatus(t, err, http.StatusNotFound)
|
|
_, err = owner.update(ctx, "missing-skill", codersdk.UpdateSkillRequest{
|
|
Content: ptr.Ref(userSkillMarkdown("missing-skill", "Missing", "Body.")),
|
|
})
|
|
requireSDKErrorStatus(t, err, http.StatusNotFound)
|
|
err = owner.delete(ctx, "missing-skill")
|
|
requireSDKErrorStatus(t, err, http.StatusNotFound)
|
|
})
|
|
|
|
t.Run("SameNameConflictsOnlyWithinOwner", func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
ctx := testutil.Context(t, testutil.WaitMedium)
|
|
sharedContent := userSkillMarkdown("shared-skill", "Shared", "Shared body.")
|
|
_, err := owner.create(ctx, codersdk.CreateSkillRequest{Content: sharedContent})
|
|
require.NoError(t, err)
|
|
_, err = owner.create(ctx, codersdk.CreateSkillRequest{Content: sharedContent})
|
|
sdkErr := requireSDKErrorStatus(t, err, http.StatusConflict)
|
|
assert.Equal(t, "A skill with that name already exists.", sdkErr.Message)
|
|
_, err = peer.create(ctx, codersdk.CreateSkillRequest{Content: sharedContent})
|
|
require.NoError(t, err)
|
|
})
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestUserSkillLimit(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
adminClient := coderdtest.New(t, nil)
|
|
firstUser := coderdtest.CreateFirstUser(t, adminClient)
|
|
ownerClient, _ := coderdtest.CreateAnotherUser(t, adminClient, firstUser.OrganizationID)
|
|
owner := codersdk.NewExperimentalClient(ownerClient)
|
|
ctx := testutil.Context(t, testutil.WaitLong)
|
|
|
|
for i := range skills.MaxPersonalSkillsPerUser {
|
|
name := fmt.Sprintf("limit-skill-%03d", i)
|
|
_, err := owner.CreateUserSkill(ctx, codersdk.Me, codersdk.CreateSkillRequest{
|
|
Content: userSkillMarkdown(name, "Limit", "Body."),
|
|
})
|
|
require.NoError(t, err)
|
|
}
|
|
|
|
_, err := owner.CreateUserSkill(ctx, codersdk.Me, codersdk.CreateSkillRequest{
|
|
Content: userSkillMarkdown("limit-skill-overflow", "Limit", "Body."),
|
|
})
|
|
sdkErr := requireSDKErrorStatus(t, err, http.StatusConflict)
|
|
assert.Equal(t, "Personal skill limit reached.", sdkErr.Message)
|
|
assert.Equal(t,
|
|
fmt.Sprintf("Each user can have at most %d personal skills.", skills.MaxPersonalSkillsPerUser),
|
|
sdkErr.Detail,
|
|
)
|
|
}
|
|
|
|
func TestUserSkillLimitConcurrentCreates(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
adminClient := coderdtest.New(t, nil)
|
|
firstUser := coderdtest.CreateFirstUser(t, adminClient)
|
|
ownerClient, _ := coderdtest.CreateAnotherUser(t, adminClient, firstUser.OrganizationID)
|
|
owner := codersdk.NewExperimentalClient(ownerClient)
|
|
ctx := testutil.Context(t, testutil.WaitLong)
|
|
|
|
for i := range skills.MaxPersonalSkillsPerUser - 1 {
|
|
name := fmt.Sprintf("concurrent-limit-skill-%03d", i)
|
|
_, err := owner.CreateUserSkill(ctx, codersdk.Me, codersdk.CreateSkillRequest{
|
|
Content: userSkillMarkdown(name, "Limit", "Body."),
|
|
})
|
|
require.NoError(t, err)
|
|
}
|
|
|
|
const attempts = 8
|
|
start := make(chan struct{})
|
|
results := make(chan error, attempts)
|
|
for i := range attempts {
|
|
go func() {
|
|
<-start
|
|
name := fmt.Sprintf("concurrent-limit-overflow-%03d", i)
|
|
_, err := owner.CreateUserSkill(ctx, codersdk.Me, codersdk.CreateSkillRequest{
|
|
Content: userSkillMarkdown(name, "Limit", "Body."),
|
|
})
|
|
results <- err
|
|
}()
|
|
}
|
|
close(start)
|
|
|
|
successes := 0
|
|
for range attempts {
|
|
err := <-results
|
|
if err == nil {
|
|
successes++
|
|
continue
|
|
}
|
|
requireSDKErrorStatus(t, err, http.StatusConflict)
|
|
}
|
|
assert.Equal(t, 1, successes)
|
|
|
|
list, err := owner.UserSkills(ctx, codersdk.Me)
|
|
require.NoError(t, err)
|
|
assert.Len(t, list, skills.MaxPersonalSkillsPerUser)
|
|
}
|
|
|
|
func TestUserSkillRequestAllowsEscapedMaxSizeContent(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
adminClient := coderdtest.New(t, nil)
|
|
firstUser := coderdtest.CreateFirstUser(t, adminClient)
|
|
ownerClient, _ := coderdtest.CreateAnotherUser(t, adminClient, firstUser.OrganizationID)
|
|
owner := codersdk.NewExperimentalClient(ownerClient)
|
|
ctx := testutil.Context(t, testutil.WaitMedium)
|
|
|
|
prefix := "---\nname: escaped-limit-skill\ndescription: Escaped\n---\n\n"
|
|
suffix := "\n"
|
|
bodyLen := skills.MaxPersonalSkillSizeBytes - len(prefix) - len(suffix)
|
|
require.Positive(t, bodyLen)
|
|
content := prefix + strings.Repeat(`"`, bodyLen) + suffix
|
|
require.Len(t, []byte(content), skills.MaxPersonalSkillSizeBytes)
|
|
|
|
raw, err := json.Marshal(codersdk.CreateSkillRequest{Content: content})
|
|
require.NoError(t, err)
|
|
require.Greater(t, len(raw), skills.MaxPersonalSkillSizeBytes+1024)
|
|
|
|
created, err := owner.CreateUserSkill(ctx, codersdk.Me, codersdk.CreateSkillRequest{
|
|
Content: content,
|
|
})
|
|
require.NoError(t, err)
|
|
assert.Equal(t, "escaped-limit-skill", created.Name)
|
|
}
|
|
|
|
func TestUserSkillAuthorization(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
adminClient := coderdtest.New(t, nil)
|
|
firstUser := coderdtest.CreateFirstUser(t, adminClient)
|
|
ownerClient, ownerUser := coderdtest.CreateAnotherUser(t, adminClient, firstUser.OrganizationID)
|
|
otherClient, _ := coderdtest.CreateAnotherUser(t, adminClient, firstUser.OrganizationID)
|
|
userAdminClient, _ := coderdtest.CreateAnotherUser(t, adminClient, firstUser.OrganizationID, rbac.RoleUserAdmin())
|
|
admin := codersdk.NewExperimentalClient(adminClient)
|
|
owner := codersdk.NewExperimentalClient(ownerClient)
|
|
other := codersdk.NewExperimentalClient(otherClient)
|
|
userAdmin := codersdk.NewExperimentalClient(userAdminClient)
|
|
ctx := testutil.Context(t, testutil.WaitMedium)
|
|
targetUser := ownerUser.Username
|
|
|
|
_, err := owner.CreateUserSkill(ctx, codersdk.Me, codersdk.CreateSkillRequest{
|
|
Content: userSkillMarkdown("auth-skill", "Auth", "Body."),
|
|
})
|
|
require.NoError(t, err)
|
|
|
|
_, err = other.UserSkills(ctx, targetUser)
|
|
requireSDKErrorStatus(t, err, http.StatusNotFound)
|
|
_, err = other.UserSkillByName(ctx, targetUser, "auth-skill")
|
|
requireSDKErrorStatus(t, err, http.StatusNotFound)
|
|
_, err = other.CreateUserSkill(ctx, targetUser, codersdk.CreateSkillRequest{
|
|
Content: userSkillMarkdown("denied-create", "Denied", "Body."),
|
|
})
|
|
requireSDKErrorStatus(t, err, http.StatusNotFound)
|
|
_, err = other.UpdateUserSkill(ctx, targetUser, "auth-skill", codersdk.UpdateSkillRequest{
|
|
Content: ptr.Ref(userSkillMarkdown("auth-skill", "Denied", "Body.")),
|
|
})
|
|
requireSDKErrorStatus(t, err, http.StatusNotFound)
|
|
err = other.DeleteUserSkill(ctx, targetUser, "auth-skill")
|
|
requireSDKErrorStatus(t, err, http.StatusNotFound)
|
|
|
|
_, err = userAdmin.UserSkills(ctx, targetUser)
|
|
requireSDKErrorStatus(t, err, http.StatusNotFound)
|
|
_, err = userAdmin.UserSkillByName(ctx, targetUser, "auth-skill")
|
|
requireSDKErrorStatus(t, err, http.StatusNotFound)
|
|
_, err = userAdmin.CreateUserSkill(ctx, targetUser, codersdk.CreateSkillRequest{
|
|
Content: userSkillMarkdown("denied-admin-create", "Denied", "Body."),
|
|
})
|
|
requireSDKErrorStatus(t, err, http.StatusForbidden)
|
|
_, err = userAdmin.UpdateUserSkill(ctx, targetUser, "auth-skill", codersdk.UpdateSkillRequest{
|
|
Content: ptr.Ref(userSkillMarkdown("auth-skill", "Denied", "Body.")),
|
|
})
|
|
requireSDKErrorStatus(t, err, http.StatusForbidden)
|
|
err = userAdmin.DeleteUserSkill(ctx, targetUser, "auth-skill")
|
|
requireSDKErrorStatus(t, err, http.StatusNotFound)
|
|
|
|
_, err = admin.CreateUserSkill(ctx, targetUser, codersdk.CreateSkillRequest{
|
|
Content: userSkillMarkdown("admin-created", "Admin create", "Created by admin."),
|
|
})
|
|
requireSDKErrorStatus(t, err, http.StatusForbidden)
|
|
|
|
list, err := admin.UserSkills(ctx, targetUser)
|
|
require.NoError(t, err)
|
|
require.Len(t, list, 1)
|
|
assert.Equal(t, "auth-skill", list[0].Name)
|
|
got, err := admin.UserSkillByName(ctx, targetUser, "auth-skill")
|
|
require.NoError(t, err)
|
|
assert.Equal(t, "auth-skill", got.Name)
|
|
_, err = admin.UpdateUserSkill(ctx, targetUser, "auth-skill", codersdk.UpdateSkillRequest{
|
|
Content: ptr.Ref(userSkillMarkdown("auth-skill", "Admin update", "Updated by admin.")),
|
|
})
|
|
requireSDKErrorStatus(t, err, http.StatusForbidden)
|
|
require.NoError(t, admin.DeleteUserSkill(ctx, targetUser, "auth-skill"))
|
|
}
|
|
|
|
func TestUserSkillSoftDeleteCleanup(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
adminClient, _, api := coderdtest.NewWithAPI(t, nil)
|
|
firstUser := coderdtest.CreateFirstUser(t, adminClient)
|
|
ownerClient, ownerUser := coderdtest.CreateAnotherUser(t, adminClient, firstUser.OrganizationID)
|
|
owner := codersdk.NewExperimentalClient(ownerClient)
|
|
ctx := testutil.Context(t, testutil.WaitMedium)
|
|
|
|
_, err := owner.CreateUserSkill(ctx, codersdk.Me, codersdk.CreateSkillRequest{
|
|
Content: userSkillMarkdown("soft-delete-skill", "Soft delete", "Body."),
|
|
})
|
|
require.NoError(t, err)
|
|
|
|
require.NoError(t, adminClient.DeleteUser(ctx, ownerUser.ID))
|
|
readAuthzCtx := dbauthz.AsSystemRestricted(ctx)
|
|
_, err = api.Database.GetUserSkillByUserIDAndName(
|
|
readAuthzCtx,
|
|
database.GetUserSkillByUserIDAndNameParams{
|
|
UserID: ownerUser.ID,
|
|
Name: "soft-delete-skill",
|
|
},
|
|
)
|
|
require.ErrorIs(t, err, sql.ErrNoRows)
|
|
|
|
createAuthzCtx := dbauthz.As(ctx, rbac.Subject{
|
|
Type: rbac.SubjectTypeUser,
|
|
ID: ownerUser.ID.String(),
|
|
Roles: rbac.RoleIdentifiers{rbac.RoleMember()},
|
|
Scope: rbac.ScopeAll,
|
|
}.WithCachedASTValue())
|
|
_, err = api.Database.InsertUserSkill(
|
|
createAuthzCtx,
|
|
database.InsertUserSkillParams{
|
|
ID: uuid.New(),
|
|
UserID: ownerUser.ID,
|
|
Name: "after-soft-delete",
|
|
Description: "Soft delete",
|
|
Content: userSkillMarkdown("after-soft-delete", "Soft delete", "Body."),
|
|
},
|
|
)
|
|
require.True(t, database.IsCheckViolation(err, database.CheckConstraint("user_skill_user_deleted")))
|
|
require.ErrorContains(t, err, "Cannot create user_skill for deleted user")
|
|
}
|
|
|
|
func TestUserSkillDatabaseConstraints(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
adminClient, _, api := coderdtest.NewWithAPI(t, nil)
|
|
firstUser := coderdtest.CreateFirstUser(t, adminClient)
|
|
_, ownerUser := coderdtest.CreateAnotherUser(t, adminClient, firstUser.OrganizationID)
|
|
tests := []struct {
|
|
name string
|
|
params database.InsertUserSkillParams
|
|
constraint database.CheckConstraint
|
|
}{
|
|
{
|
|
name: "NameFormat",
|
|
params: database.InsertUserSkillParams{
|
|
ID: uuid.New(),
|
|
UserID: ownerUser.ID,
|
|
Name: "not kebab",
|
|
Description: "Invalid",
|
|
Content: userSkillMarkdown("not kebab", "Invalid", "Body."),
|
|
},
|
|
constraint: database.CheckSkillsNameFormat,
|
|
},
|
|
{
|
|
name: "NameSize",
|
|
params: database.InsertUserSkillParams{
|
|
ID: uuid.New(),
|
|
UserID: ownerUser.ID,
|
|
Name: strings.Repeat("a", skills.MaxPersonalSkillNameBytes+1),
|
|
Description: "Invalid",
|
|
Content: userSkillMarkdown("too-long-name", "Invalid", "Body."),
|
|
},
|
|
constraint: database.CheckSkillsNameSize,
|
|
},
|
|
{
|
|
name: "ContentSize",
|
|
params: database.InsertUserSkillParams{
|
|
ID: uuid.New(),
|
|
UserID: ownerUser.ID,
|
|
Name: "content-too-large",
|
|
Description: "Invalid",
|
|
Content: strings.Repeat("a", skills.MaxPersonalSkillSizeBytes+1),
|
|
},
|
|
constraint: database.CheckSkillsContentSize,
|
|
},
|
|
}
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
ctx := testutil.Context(t, testutil.WaitMedium)
|
|
authzCtx := dbauthz.As(ctx, coderdtest.AuthzUserSubject(ownerUser))
|
|
|
|
_, err := api.Database.InsertUserSkill(authzCtx, tt.params)
|
|
require.True(t, database.IsCheckViolation(err, tt.constraint), "expected %s, got %v", tt.constraint, err)
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestUserSkillSchemaConstants(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
_, _, sqlDB := dbtestutil.NewDBWithSQLDB(t)
|
|
|
|
ctx := testutil.Context(t, testutil.WaitMedium)
|
|
var triggerDef string
|
|
require.NoError(t, sqlDB.QueryRowContext(
|
|
ctx,
|
|
`SELECT pg_get_functiondef('enforce_skills_per_owner_limit'::regproc)`,
|
|
).Scan(&triggerDef))
|
|
assert.Contains(t, triggerDef, fmt.Sprintf("skill_limit constant int := %d;", skills.MaxPersonalSkillsPerUser))
|
|
|
|
constraints := map[database.CheckConstraint]string{
|
|
database.CheckSkillsNameSize: fmt.Sprintf("octet_length(name) <= %d", skills.MaxPersonalSkillNameBytes),
|
|
database.CheckSkillsNameFormat: "name ~ '^[a-z0-9]+(-[a-z0-9]+)*$'::text",
|
|
database.CheckSkillsContentSize: fmt.Sprintf("octet_length(content) <= %d", skills.MaxPersonalSkillSizeBytes),
|
|
}
|
|
for constraint, expected := range constraints {
|
|
t.Run(string(constraint), func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
ctx := testutil.Context(t, testutil.WaitMedium)
|
|
var constraintDef string
|
|
require.NoError(t, sqlDB.QueryRowContext(
|
|
ctx,
|
|
`SELECT pg_get_constraintdef(oid) FROM pg_constraint WHERE conname = $1`,
|
|
constraint,
|
|
).Scan(&constraintDef))
|
|
assert.Contains(t, constraintDef, expected)
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestSkillAudit(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
for _, tc := range skillOwnerCases {
|
|
//nolint:paralleltest,tparallel // Subtests share one auditor and run sequentially.
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
auditor := audit.NewMock()
|
|
owner, _ := tc.setup(t, &coderdtest.Options{Auditor: auditor})
|
|
ctx := testutil.Context(t, testutil.WaitMedium)
|
|
auditor.ResetLogs()
|
|
|
|
genName := func(t *testing.T) string {
|
|
return strings.ToLower(strings.ReplaceAll(t.Name(), "/", "-"))
|
|
}
|
|
requireLog := func(t *testing.T, log database.AuditLog, action database.AuditAction, status int, skill codersdk.Skill) {
|
|
t.Helper()
|
|
assert.Equal(t, action, log.Action)
|
|
assert.EqualValues(t, status, log.StatusCode)
|
|
assert.Equal(t, owner.resourceType, log.ResourceType)
|
|
assert.Equal(t, owner.auditOrgID, log.OrganizationID)
|
|
assert.Equal(t, skill.ID, log.ResourceID)
|
|
assert.Equal(t, skill.Name, log.ResourceTarget)
|
|
}
|
|
|
|
t.Run("CreateEmitsLog", func(t *testing.T) {
|
|
auditor.ResetLogs()
|
|
skill, err := owner.create(ctx, codersdk.CreateSkillRequest{Content: userSkillMarkdown(genName(t), "Audit", "Body.")})
|
|
require.NoError(t, err)
|
|
|
|
logs := auditor.AuditLogs()
|
|
require.Len(t, logs, 1)
|
|
requireLog(t, logs[0], database.AuditActionCreate, http.StatusCreated, skill)
|
|
})
|
|
|
|
t.Run("UpdateEmitsLog", func(t *testing.T) {
|
|
name := genName(t)
|
|
skill, err := owner.create(ctx, codersdk.CreateSkillRequest{Content: userSkillMarkdown(name, "Initial", "Body.")})
|
|
require.NoError(t, err)
|
|
auditor.ResetLogs()
|
|
_, err = owner.update(ctx, name, codersdk.UpdateSkillRequest{
|
|
Content: ptr.Ref(userSkillMarkdown(name, "Updated", "Updated body.")),
|
|
})
|
|
require.NoError(t, err)
|
|
|
|
logs := auditor.AuditLogs()
|
|
require.Len(t, logs, 1)
|
|
requireLog(t, logs[0], database.AuditActionWrite, http.StatusOK, skill)
|
|
})
|
|
|
|
t.Run("DeleteEmitsLog", func(t *testing.T) {
|
|
name := genName(t)
|
|
skill, err := owner.create(ctx, codersdk.CreateSkillRequest{Content: userSkillMarkdown(name, "Delete", "Body.")})
|
|
require.NoError(t, err)
|
|
auditor.ResetLogs()
|
|
require.NoError(t, owner.delete(ctx, name))
|
|
|
|
logs := auditor.AuditLogs()
|
|
require.Len(t, logs, 1)
|
|
requireLog(t, logs[0], database.AuditActionDelete, http.StatusNoContent, skill)
|
|
})
|
|
|
|
t.Run("ReadsDoNotEmitLogs", func(t *testing.T) {
|
|
name := genName(t)
|
|
_, err := owner.create(ctx, codersdk.CreateSkillRequest{Content: userSkillMarkdown(name, "Read", "Body.")})
|
|
require.NoError(t, err)
|
|
auditor.ResetLogs()
|
|
|
|
_, err = owner.list(ctx)
|
|
require.NoError(t, err)
|
|
_, err = owner.get(ctx, name)
|
|
require.NoError(t, err)
|
|
assert.Empty(t, auditor.AuditLogs())
|
|
})
|
|
|
|
t.Run("ValidationFailureDoesNotEmitLog", func(t *testing.T) {
|
|
auditor.ResetLogs()
|
|
_, err := owner.create(ctx, codersdk.CreateSkillRequest{Content: userSkillMarkdown("bad-name", "Invalid", " \n")})
|
|
requireSDKErrorStatus(t, err, http.StatusBadRequest)
|
|
assert.Empty(t, auditor.AuditLogs())
|
|
})
|
|
|
|
t.Run("MissingSkillFailuresDoNotEmitLogs", func(t *testing.T) {
|
|
auditor.ResetLogs()
|
|
_, err := owner.update(ctx, "missing-audit-skill", codersdk.UpdateSkillRequest{
|
|
Content: ptr.Ref(userSkillMarkdown("missing-audit-skill", "Missing", "Body.")),
|
|
})
|
|
requireSDKErrorStatus(t, err, http.StatusNotFound)
|
|
err = owner.delete(ctx, "missing-audit-skill")
|
|
requireSDKErrorStatus(t, err, http.StatusNotFound)
|
|
assert.Empty(t, auditor.AuditLogs())
|
|
})
|
|
})
|
|
}
|
|
}
|
|
|
|
func userSkillMarkdown(name string, description string, body string) string {
|
|
return fmt.Sprintf("---\nname: %s\ndescription: %s\n---\n\n%s\n", name, description, body)
|
|
}
|
|
|
|
func requireSDKErrorStatus(t *testing.T, err error, status int, msgAndArgs ...any) *codersdk.Error {
|
|
t.Helper()
|
|
require.Error(t, err, msgAndArgs...)
|
|
sdkErr := coderdtest.SDKError(t, err)
|
|
require.Equal(t, status, sdkErr.StatusCode(), msgAndArgs...)
|
|
return sdkErr
|
|
}
|