diff --git a/CHANGELOG.md b/CHANGELOG.md index dfb37920..3e8905a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ All notable changes to GoClaw are documented here. For full documentation, see [ ### Added +- **Skill agent manage grants** — Adds per-agent skill edit/delete grants with + backend checks, HTTP/WS support, SQLite and PostgreSQL schema updates, and web + dashboard controls for granting and revoking manage access. + - **Packages Update Flow (Phase 2a: pip + npm)** — closes #900 (Phase 2a). Extends Phase 1 update infrastructure to pip and npm package sources. `/v1/packages/updates` now returns mixed-source results with an `availability: {github, pip, npm}` map. @@ -77,6 +81,12 @@ All notable changes to GoClaw are documented here. For full documentation, see [ ### Fixed +- **Skill grant tenant isolation.** Agent skill grants now validate both the + skill and agent tenant scope before insert, revoke, grant listing, or + can-manage checks. Visibility auto-promote/auto-demote updates are scoped to + the calling tenant or system skills so one tenant cannot mutate another + tenant's skill. + - **Agent provider switching.** Saving an agent after changing provider/model now handles cleared ChatGPT OAuth routing config without writing SQL NULL into NOT NULL JSON config columns. diff --git a/cmd/gateway_github_installer.go b/cmd/gateway_github_installer.go index 2be97534..41b2fc14 100644 --- a/cmd/gateway_github_installer.go +++ b/cmd/gateway_github_installer.go @@ -18,7 +18,7 @@ import ( // GOCLAW_PACKAGES_GITHUB_TOKEN optional PAT (boosts rate limit, enables private repos) // GOCLAW_PACKAGES_MAX_ASSET_SIZE_MB default 200 // GOCLAW_PACKAGES_GITHUB_ALLOWED_ORGS comma-separated allowlist (empty = all allowed) -// GOCLAW_PACKAGES_GITHUB_BIN_DIR default /app/data/.runtime/bin +// GOCLAW_PACKAGES_GITHUB_BIN_DIR default {runtimeDir}/bin // GOCLAW_PACKAGES_GITHUB_MANIFEST default {BIN_DIR}/../github-packages.json func initGitHubInstaller() { cfg := &skills.GitHubPackagesConfig{ diff --git a/docs/14-skills-runtime.md b/docs/14-skills-runtime.md index c41f4c44..ac8fe740 100644 --- a/docs/14-skills-runtime.md +++ b/docs/14-skills-runtime.md @@ -217,7 +217,7 @@ github:owner/repo[@tag] ``` Admin-only, SHA256-verified, ELF-validated, with a release-picker UI. Binaries -land in `/app/data/.runtime/bin/` (on `$PATH`). See +land in `{runtimeDir}/bin/` (on `$PATH`). See [`docs/packages-github.md`](./packages-github.md) for syntax, configuration, security posture, and troubleshooting (especially musl/glibc compatibility). diff --git a/docs/journals/260518-1741-beta-skill-grants-tenant-scope.md b/docs/journals/260518-1741-beta-skill-grants-tenant-scope.md new file mode 100644 index 00000000..29489bcc --- /dev/null +++ b/docs/journals/260518-1741-beta-skill-grants-tenant-scope.md @@ -0,0 +1,43 @@ +# Beta Skill Grants Tenant-Scope Hardening + +**Date**: 2026-05-18 17:41 +**Severity**: High +**Component**: Skills grants, tenant isolation, skill management UI/API +**Status**: Resolved + +## What Happened + +This beta ship started from PR #14, which added per-agent skill manage grants so an authorized agent can edit/delete skills even when `owner_id` no longer matches its current actor identity. The review caught the ugly part: the grant path could mutate skill visibility or grant records across tenant boundaries, and the UI/API exposed `owner_id` in skill responses where clients did not need it. Issue digitopvn/goclaw#15 tracks the hardening fallout. + +## The Brutal Truth + +This was a permission feature shipped close to a tenant-isolation fault line. That is always where mistakes hurt most. The exhausting part is that the baseline feature was useful, but the first version trusted grant inputs too much. A beta is exactly where this should be caught, but it still feels bad because cross-tenant mutation risk is not a cosmetic bug. + +## Technical Details + +- PG now validates grant scope before insert/update in `internal/store/pg/skills_grants.go:19` and `verifySkillGrantScope` rejects mismatched agent tenants as `agent not found`. +- SQLite mirrors the same check in `internal/store/sqlitestore/skills_grants.go:25`. +- Cleanup migration `migrations/000067_skill_agent_grants_scope_cleanup.up.sql:1` deletes `skill_agent_grants` rows where `sag.tenant_id <> a.tenant_id` or non-system skill tenant differs. +- `internal/store/skill_store.go:19` hides `OwnerID` with `json:"-"`, stopping owner IDs from leaking through skill responses/UI. +- Changelog updated under `CHANGELOG.md`. + +## What We Tried + +- Kept PR #14's per-agent `can_manage` model because it directly solves agent-owned skill maintenance without pretending `owner_id` is stable agent identity. +- Rejected UI-only filtering because it would not protect HTTP, WebSocket, import, or future callers. +- Added store-layer verification in both PG and SQLite because that is the point every grant mutation must cross. + +## Root Cause Analysis + +We shipped the first feature around ownership semantics without enforcing the tenant relationship at the same boundary as the write. The fundamental mistake was treating authorization and scope as already resolved by callers. In a multi-tenant system, that assumption is how one tenant ends up mutating another tenant's visibility state. + +## Lessons Learned + +Grant writes must prove both sides of the relationship: the skill and the agent. Do not rely on caller discipline. Also, response structs should not expose security-sensitive ownership fields just because the database row has them. + +## Next Steps + +- Owner: maintainers. Monitor digitopvn/goclaw#15 through beta. +- Owner: release lead. Verify beta users receive migration `000067` and no stale cross-tenant rows survive upgrade. +- Owner: reviewers. Treat future grant/import/export changes as tenant-scope sensitive by default. +- Tests/build are passing for this ship; keep PG, SQLite, and Web UI build checks required before promoting beyond beta. diff --git a/docs/packages-github.md b/docs/packages-github.md index d8e6fbb7..9749fd67 100644 --- a/docs/packages-github.md +++ b/docs/packages-github.md @@ -24,7 +24,7 @@ Examples: 4. Verifies SHA256 if the publisher ships `checksums.txt` / `SHA256SUMS` 5. Validates ELF magic bytes + 64-bit class + machine matches runtime arch 6. Extracts archive safely (tar.gz / zip / raw binary) with path-traversal + zip-bomb guards -7. Installs to `/app/data/.runtime/bin/` (prepended to `$PATH`) +7. Installs to `{runtimeDir}/bin/` (prepended to `$PATH`) 8. Persists a manifest for later listing + uninstall ## Usage @@ -69,7 +69,7 @@ token in `config.json`. | `GOCLAW_PACKAGES_GITHUB_TOKEN` | `""` | Optional PAT: rate 60/hr → 5000/hr + private repo access | | `GOCLAW_PACKAGES_MAX_ASSET_SIZE_MB` | `200` | Applies to both download cap and 2× uncompressed cap | | `GOCLAW_PACKAGES_GITHUB_ALLOWED_ORGS` | `""` | Comma-separated allowlist (empty = all orgs allowed) | -| `GOCLAW_PACKAGES_GITHUB_BIN_DIR` | `/app/data/.runtime/bin` | Where extracted binaries land | +| `GOCLAW_PACKAGES_GITHUB_BIN_DIR` | `{runtimeDir}/bin` | Where extracted binaries land | | `GOCLAW_PACKAGES_GITHUB_MANIFEST` | `{bin_dir}/../github-packages.json` | Manifest path | Token scopes: @@ -133,7 +133,7 @@ the release. Do not force-install; report upstream. ## Limitations (Phase 1) - Linux-only (Lite/Desktop editions not yet supported) -- Docker edition only (runtime dir `/app/data/.runtime/bin`) +- Docker and bare-metal gateway editions (default runtime dir resolves to `/app/data/.runtime/bin` in Docker or `/var/lib/goclaw/data/.runtime/bin` on bare-metal Linux) - Installs all top-level executables in an archive (no interactive picker if archive contains multiple binaries) - No version history / rollback — re-installing replaces in place diff --git a/docs/project-changelog.md b/docs/project-changelog.md index 6afba041..9384a8b9 100644 --- a/docs/project-changelog.md +++ b/docs/project-changelog.md @@ -6,6 +6,19 @@ Significant changes, features, and fixes in reverse chronological order. ## 2026-05-18 +### Packages: GitHub installer runtime path + +**Fixes** + +- Fixed GitHub Releases package installs on bare-metal gateways by defaulting the GitHub binary directory to `{runtimeDir}/bin` instead of Docker-only `/app/data/.runtime/bin`. +- The fix covers installs such as `github:nextlevelbuilder/goclaw-cli@v0.4.1` on the VPS, where `/app` is not writable or present. + +**Tests** + +- Added default-path regression coverage and made Unix-socket apk helper tests skip cleanly on Windows environments that cannot bind Unix sockets. + +--- + ### Tools: built-in wait delay **Features** @@ -56,6 +69,24 @@ Significant changes, features, and fixes in reverse chronological order. ## 2026-05-17 +### Skills: agent manage grants + +**Fixes** + +- Added explicit per-agent skill manage grants so agents can edit/delete skills they were authorized to maintain even when `owner_id` no longer matches their current actor identity. +- Auto-granted manage permission to the creating/publishing agent for new managed skills. + +**UI** + +- Show custom skill owner IDs in the Skills table. +- Added Skills page controls to grant agent skill access and edit/delete permission. + +**Tests** + +- Added PG/SQLite grant coverage and verified Go builds plus Web UI build. + +--- + ### Agents: provider switch save fix **Fixes** diff --git a/internal/gateway/methods/skills.go b/internal/gateway/methods/skills.go index ac5349d1..740f699b 100644 --- a/internal/gateway/methods/skills.go +++ b/internal/gateway/methods/skills.go @@ -17,7 +17,7 @@ import ( // skillOwnerGetter is an optional interface for stores that can return a skill's owner ID. type skillOwnerGetter interface { - GetSkillOwnerID(id uuid.UUID) (string, bool) + GetSkillOwnerID(ctx context.Context, id uuid.UUID) (string, bool) } // SkillsMethods handles skills.list, skills.get, skills.update. @@ -219,7 +219,7 @@ func (m *SkillsMethods) handleUpdate(ctx context.Context, client *gateway.Client client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrUnauthorized, i18n.T(locale, i18n.MsgPermissionDenied, "skills.update"))) return } - if ownerID, found := ownerGetter.GetSkillOwnerID(skillID); found && ownerID != client.UserID() { + if ownerID, found := ownerGetter.GetSkillOwnerID(ctx, skillID); found && ownerID != client.UserID() { client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrUnauthorized, i18n.T(locale, i18n.MsgPermissionDenied, "skills.update"))) return } diff --git a/internal/http/skills.go b/internal/http/skills.go index 52078ca3..ee1e17dc 100644 --- a/internal/http/skills.go +++ b/internal/http/skills.go @@ -37,7 +37,7 @@ type SkillsHandler struct { msgBus *bus.MessageBus tenantCfgStore store.SkillTenantConfigStore tenantStore store.TenantStore - db *sql.DB // for export/import direct queries + db *sql.DB // for export/import direct queries uploadLocks sync.Map // per-slug mutex; bounded by validated slug set, entries are tiny (*sync.Mutex) } @@ -92,6 +92,7 @@ func (h *SkillsHandler) RegisterRoutes(mux *http.ServeMux) { mux.HandleFunc("PUT /v1/skills/{id}", h.adminMiddleware(h.handleUpdate)) mux.HandleFunc("DELETE /v1/skills/{id}", h.adminMiddleware(h.handleDelete)) // Skill grants (admin+) + mux.HandleFunc("GET /v1/skills/{id}/grants/agent", h.adminMiddleware(h.handleListAgentGrants)) mux.HandleFunc("POST /v1/skills/{id}/grants/agent", h.adminMiddleware(h.handleGrantAgent)) mux.HandleFunc("DELETE /v1/skills/{id}/grants/agent/{agentID}", h.adminMiddleware(h.handleRevokeAgent)) mux.HandleFunc("POST /v1/skills/{id}/grants/user", h.adminMiddleware(h.handleGrantUser)) diff --git a/internal/http/skills_grants.go b/internal/http/skills_grants.go index d359b94b..eccebcad 100644 --- a/internal/http/skills_grants.go +++ b/internal/http/skills_grants.go @@ -33,6 +33,25 @@ func (h *SkillsHandler) handleListAgentSkills(w http.ResponseWriter, r *http.Req writeJSON(w, http.StatusOK, map[string]any{"skills": skills}) } +func (h *SkillsHandler) handleListAgentGrants(w http.ResponseWriter, r *http.Request) { + locale := store.LocaleFromContext(r.Context()) + idStr := r.PathValue("id") + skillID, err := uuid.Parse(idStr) + if err != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidID, "skill")}) + return + } + + grants, err := h.skills.ListAgentGrantsForSkill(r.Context(), skillID) + if err != nil { + slog.Error("failed to list skill agent grants", "skill_id", skillID, "error", err) + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgFailedToList, "skill grants")}) + return + } + + writeJSON(w, http.StatusOK, map[string]any{"grants": grants}) +} + func (h *SkillsHandler) handleGrantAgent(w http.ResponseWriter, r *http.Request) { locale := store.LocaleFromContext(r.Context()) userID := store.UserIDFromContext(r.Context()) @@ -53,8 +72,9 @@ func (h *SkillsHandler) handleGrantAgent(w http.ResponseWriter, r *http.Request) } var req struct { - AgentID string `json:"agent_id"` - Version int `json:"version"` + AgentID string `json:"agent_id"` + Version int `json:"version"` + CanManage *bool `json:"can_manage"` } if !bindJSON(w, r, locale, &req) { return @@ -70,8 +90,14 @@ func (h *SkillsHandler) handleGrantAgent(w http.ResponseWriter, r *http.Request) req.Version = 1 } - if err := h.skills.GrantToAgent(r.Context(), skillID, agentID, req.Version, userID); err != nil { - writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) + var grantErr error + if req.CanManage == nil { + grantErr = h.skills.GrantToAgent(r.Context(), skillID, agentID, req.Version, userID) + } else { + grantErr = h.skills.GrantToAgent(r.Context(), skillID, agentID, req.Version, userID, *req.CanManage) + } + if grantErr != nil { + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": grantErr.Error()}) return } diff --git a/internal/http/skills_upload_test.go b/internal/http/skills_upload_test.go index f8dca60a..f8ae2935 100644 --- a/internal/http/skills_upload_test.go +++ b/internal/http/skills_upload_test.go @@ -528,7 +528,7 @@ func (s *skillManageStoreStub) StoreMissingDeps(_ context.Context, id uuid.UUID, s.skills[id] = skill return nil } -func (s *skillManageStoreStub) GrantToAgent(context.Context, uuid.UUID, uuid.UUID, int, string) error { +func (s *skillManageStoreStub) GrantToAgent(context.Context, uuid.UUID, uuid.UUID, int, string, ...bool) error { return nil } func (s *skillManageStoreStub) RevokeFromAgent(context.Context, uuid.UUID, uuid.UUID) error { @@ -541,6 +541,12 @@ func (s *skillManageStoreStub) RevokeFromUser(context.Context, uuid.UUID, string func (s *skillManageStoreStub) ListWithGrantStatus(context.Context, uuid.UUID) ([]store.SkillWithGrantStatus, error) { return nil, nil } +func (s *skillManageStoreStub) ListAgentGrantsForSkill(context.Context, uuid.UUID) ([]store.SkillAgentGrantInfo, error) { + return nil, nil +} +func (s *skillManageStoreStub) AgentCanManageSkill(context.Context, uuid.UUID, uuid.UUID) (bool, error) { + return false, nil +} func (s *skillManageStoreStub) GetSkillFilePath(context.Context, uuid.UUID) (string, string, int, bool, bool) { return "", "", 0, false, false } diff --git a/internal/skills/apk_helper_call_test.go b/internal/skills/apk_helper_call_test.go index 382bb40e..67da4eab 100644 --- a/internal/skills/apk_helper_call_test.go +++ b/internal/skills/apk_helper_call_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "net" + "runtime" "strings" "sync/atomic" "testing" @@ -42,6 +43,9 @@ func servePkgHelper(t *testing.T, sockPath, respJSON string) func() { ln, err := net.Listen("unix", sockPath) if err != nil { + if runtime.GOOS == "windows" { + t.Skipf("unix sockets are not available in this Windows test environment: %v", err) + } t.Fatalf("servePkgHelper: listen %q: %v", sockPath, err) } diff --git a/internal/skills/github_installer.go b/internal/skills/github_installer.go index 504f92e2..f44bdad3 100644 --- a/internal/skills/github_installer.go +++ b/internal/skills/github_installer.go @@ -57,7 +57,7 @@ func ParseGitHubSpec(s string) (*GitHubSpec, error) { // Token is sourced from env var only (never config.json plaintext). type GitHubPackagesConfig struct { Token string // optional GitHub personal access token - BinDir string // where to install binaries (default /app/data/.runtime/bin) + BinDir string // where to install binaries (default {runtimeDir}/bin) ManifestPath string // manifest file path (default {BinDir}/../github-packages.json) AllowedOrgs []string // lowercase list; empty = all allowed MaxAssetSizeMB int // default 200 @@ -66,7 +66,7 @@ type GitHubPackagesConfig struct { // Defaults fills in zero-valued fields. func (c *GitHubPackagesConfig) Defaults() { if c.BinDir == "" { - c.BinDir = "/app/data/.runtime/bin" + c.BinDir = filepath.Join(packageRuntimeDir(), "bin") } if c.ManifestPath == "" { c.ManifestPath = filepath.Join(filepath.Dir(c.BinDir), "github-packages.json") diff --git a/internal/skills/github_installer_test.go b/internal/skills/github_installer_test.go index 3d226fce..aec3874b 100644 --- a/internal/skills/github_installer_test.go +++ b/internal/skills/github_installer_test.go @@ -2,17 +2,18 @@ package skills import ( "errors" + "path/filepath" "strings" "testing" ) func TestParseGitHubSpec(t *testing.T) { cases := []struct { - in string - ok bool - owner string - repo string - tag string + in string + ok bool + owner string + repo string + tag string }{ {"github:cli/cli@v2.45.0", true, "cli", "cli", "v2.45.0"}, {"github:cli/cli", true, "cli", "cli", ""}, @@ -123,11 +124,20 @@ func TestAllowedOrg(t *testing.T) { } func TestConfigDefaults(t *testing.T) { + runtimeDir := t.TempDir() + t.Setenv("RUNTIME_DIR", runtimeDir) + c := &GitHubPackagesConfig{} c.Defaults() if c.BinDir == "" || c.ManifestPath == "" || c.MaxAssetSizeMB != 200 { t.Errorf("unexpected defaults: %+v", c) } + if want := filepath.Join(runtimeDir, "bin"); c.BinDir != want { + t.Errorf("BinDir = %q, want %q", c.BinDir, want) + } + if want := filepath.Join(runtimeDir, "github-packages.json"); c.ManifestPath != want { + t.Errorf("ManifestPath = %q, want %q", c.ManifestPath, want) + } if c.MaxAssetBytes() != 200*1024*1024 { t.Errorf("MaxAssetBytes wrong: %d", c.MaxAssetBytes()) } diff --git a/internal/skills/runtime_check.go b/internal/skills/runtime_check.go index b39298ca..56b5e70f 100644 --- a/internal/skills/runtime_check.go +++ b/internal/skills/runtime_check.go @@ -4,6 +4,7 @@ import ( "context" "os" "os/exec" + "path/filepath" "strings" "time" ) @@ -65,7 +66,7 @@ func CheckRuntimes() *RuntimeStatus { // Check github-bin runtime directory (where GitHub-installed binaries live). ghInfo := RuntimeInfo{Name: "github-bin"} - binDir := "/app/data/.runtime/bin" + binDir := filepath.Join(packageRuntimeDir(), "bin") if gh := DefaultGitHubInstaller(); gh != nil && gh.Config != nil && gh.Config.BinDir != "" { binDir = gh.Config.BinDir } diff --git a/internal/store/pg/skills_grants.go b/internal/store/pg/skills_grants.go index 5a42f698..22f8947a 100644 --- a/internal/store/pg/skills_grants.go +++ b/internal/store/pg/skills_grants.go @@ -2,6 +2,7 @@ package pg import ( "context" + "database/sql" "fmt" "log/slog" "time" @@ -14,38 +15,48 @@ import ( // GrantToAgent grants a skill to an agent with version pinning. // Auto-promotes visibility from 'private' to 'internal' so the skill // becomes accessible via ListAccessible for granted agents. -// Validates the agent belongs to the requesting tenant (prevents cross-tenant grant injection). -func (s *PGSkillStore) GrantToAgent(ctx context.Context, skillID, agentID uuid.UUID, version int, grantedBy string) error { +// Validates both the skill and agent belong to the requesting tenant. +func (s *PGSkillStore) GrantToAgent(ctx context.Context, skillID, agentID uuid.UUID, version int, grantedBy string, canManage ...bool) error { if err := store.ValidateUserID(grantedBy); err != nil { return err } tid := tenantIDForInsert(ctx) - - // Verify agent belongs to the requesting tenant. - var agentTenantID uuid.UUID - if err := s.db.QueryRowContext(ctx, - "SELECT tenant_id FROM agents WHERE id = $1", agentID, - ).Scan(&agentTenantID); err != nil { - return fmt.Errorf("agent not found") - } - if agentTenantID != tid { - return fmt.Errorf("agent not found") + if err := s.verifySkillGrantScope(ctx, skillID, agentID, tid); err != nil { + return err } - _, err := s.db.ExecContext(ctx, - `INSERT INTO skill_agent_grants (id, skill_id, agent_id, pinned_version, granted_by, created_at, tenant_id) - VALUES ($1, $2, $3, $4, $5, $6, $7) - ON CONFLICT (skill_id, agent_id) DO UPDATE SET pinned_version = EXCLUDED.pinned_version`, - store.GenNewID(), skillID, agentID, version, grantedBy, time.Now(), tid, - ) + now := time.Now() + var err error + if len(canManage) > 0 { + _, err = s.db.ExecContext(ctx, + `INSERT INTO skill_agent_grants (id, skill_id, agent_id, pinned_version, granted_by, can_manage, created_at, tenant_id) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8) + ON CONFLICT (skill_id, agent_id) DO UPDATE SET + pinned_version = EXCLUDED.pinned_version, + granted_by = EXCLUDED.granted_by, + can_manage = EXCLUDED.can_manage`, + store.GenNewID(), skillID, agentID, version, grantedBy, canManage[0], now, tid, + ) + } else { + _, err = s.db.ExecContext(ctx, + `INSERT INTO skill_agent_grants (id, skill_id, agent_id, pinned_version, granted_by, created_at, tenant_id) + VALUES ($1, $2, $3, $4, $5, $6, $7) + ON CONFLICT (skill_id, agent_id) DO UPDATE SET + pinned_version = EXCLUDED.pinned_version, + granted_by = EXCLUDED.granted_by`, + store.GenNewID(), skillID, agentID, version, grantedBy, now, tid, + ) + } if err != nil { return err } // Auto-promote: private → internal (so ListAccessible query includes it for granted agents) _, err = s.db.ExecContext(ctx, - `UPDATE skills SET visibility = 'internal', updated_at = NOW() WHERE id = $1 AND visibility = 'private'`, - skillID) + `UPDATE skills + SET visibility = 'internal', updated_at = NOW() + WHERE id = $1 AND visibility = 'private' AND (is_system = true OR tenant_id = $2)`, + skillID, tid) if err != nil { slog.Warn("skill_grants: failed to auto-promote visibility", "skill_id", skillID, "error", err) // Non-fatal: grant was already created successfully @@ -58,6 +69,10 @@ func (s *PGSkillStore) GrantToAgent(ctx context.Context, skillID, agentID uuid.U // RevokeFromAgent revokes a skill grant from an agent. // Auto-demotes visibility from 'internal' back to 'private' when no agent grants remain. func (s *PGSkillStore) RevokeFromAgent(ctx context.Context, skillID, agentID uuid.UUID) error { + tid := tenantIDForInsert(ctx) + if err := s.verifySkillInGrantScope(ctx, skillID, tid); err != nil { + return err + } tClause, tArgs, _, err := scopeClause(ctx, 3) if err != nil { return err @@ -74,9 +89,9 @@ func (s *PGSkillStore) RevokeFromAgent(ctx context.Context, skillID, agentID uui // avoiding a race window between COUNT and UPDATE. _, err = s.db.ExecContext(ctx, `UPDATE skills SET visibility = 'private', updated_at = NOW() - WHERE id = $1 AND visibility = 'internal' + WHERE id = $1 AND visibility = 'internal' AND (is_system = true OR tenant_id = $2) AND NOT EXISTS (SELECT 1 FROM skill_agent_grants WHERE skill_id = $1)`, - skillID) + skillID, tid) if err != nil { slog.Warn("skill_grants: failed to auto-demote visibility", "skill_id", skillID, "error", err) } @@ -85,6 +100,37 @@ func (s *PGSkillStore) RevokeFromAgent(ctx context.Context, skillID, agentID uui return nil } +func (s *PGSkillStore) verifySkillGrantScope(ctx context.Context, skillID, agentID, tenantID uuid.UUID) error { + if err := s.verifySkillInGrantScope(ctx, skillID, tenantID); err != nil { + return err + } + + var agentTenantID uuid.UUID + if err := s.db.QueryRowContext(ctx, + "SELECT tenant_id FROM agents WHERE id = $1", agentID, + ).Scan(&agentTenantID); err != nil { + return fmt.Errorf("agent not found") + } + if agentTenantID != tenantID { + return fmt.Errorf("agent not found") + } + return nil +} + +func (s *PGSkillStore) verifySkillInGrantScope(ctx context.Context, skillID, tenantID uuid.UUID) error { + var skillTenantID uuid.UUID + var isSystem bool + if err := s.db.QueryRowContext(ctx, + "SELECT tenant_id, is_system FROM skills WHERE id = $1", skillID, + ).Scan(&skillTenantID, &isSystem); err != nil { + return fmt.Errorf("skill not found") + } + if !isSystem && skillTenantID != tenantID { + return fmt.Errorf("skill not found") + } + return nil +} + // ListAgentGrants returns all skill grants for an agent. func (s *PGSkillStore) ListAgentGrants(ctx context.Context, agentID uuid.UUID) ([]SkillGrantInfo, error) { tClause, tArgs, _, err := scopeClause(ctx, 2) @@ -101,6 +147,47 @@ func (s *PGSkillStore) ListAgentGrants(ctx context.Context, agentID uuid.UUID) ( return result, nil } +// ListAgentGrantsForSkill returns all agent grants for one skill. +func (s *PGSkillStore) ListAgentGrantsForSkill(ctx context.Context, skillID uuid.UUID) ([]store.SkillAgentGrantInfo, error) { + if err := s.verifySkillInGrantScope(ctx, skillID, tenantIDForInsert(ctx)); err != nil { + return nil, err + } + tClause, tArgs, _, err := scopeClause(ctx, 2) + if err != nil { + return nil, err + } + var result []store.SkillAgentGrantInfo + err = pkgSqlxDB.SelectContext(ctx, &result, + "SELECT agent_id, pinned_version, granted_by, can_manage FROM skill_agent_grants WHERE skill_id = $1"+tClause+" ORDER BY created_at DESC", + append([]any{skillID}, tArgs...)...) + if err != nil { + return nil, err + } + return result, nil +} + +// AgentCanManageSkill reports whether an agent has explicit edit/delete rights for a skill. +func (s *PGSkillStore) AgentCanManageSkill(ctx context.Context, skillID, agentID uuid.UUID) (bool, error) { + if err := s.verifySkillInGrantScope(ctx, skillID, tenantIDForInsert(ctx)); err != nil { + return false, err + } + tClause, tArgs, _, err := scopeClause(ctx, 3) + if err != nil { + return false, err + } + var canManage bool + err = s.db.QueryRowContext(ctx, + "SELECT can_manage FROM skill_agent_grants WHERE skill_id = $1 AND agent_id = $2"+tClause, + append([]any{skillID, agentID}, tArgs...)...).Scan(&canManage) + if err == sql.ErrNoRows { + return false, nil + } + if err != nil { + return false, err + } + return canManage, nil +} + // GrantToUser grants a skill to a user (for internal visibility skills). func (s *PGSkillStore) GrantToUser(ctx context.Context, skillID uuid.UUID, userID, grantedBy string) error { if err := store.ValidateUserID(userID); err != nil { @@ -210,16 +297,19 @@ func (s *PGSkillStore) ListWithGrantStatus(ctx context.Context, agentID uuid.UUI return nil, err } tenantCond := "" + grantTenantCond := "" if tc != "" { tenantCond = fmt.Sprintf(" AND (s.is_system = true OR s.tenant_id = $%d)", 2) + grantTenantCond = fmt.Sprintf(" AND sag.tenant_id = $%d", 2) } rows, err := s.db.QueryContext(ctx, `SELECT s.id, s.name, s.slug, COALESCE(s.description, ''), s.visibility, s.version, (sag.id IS NOT NULL) AS granted, + COALESCE(sag.can_manage, false) AS can_manage, sag.pinned_version, s.is_system FROM skills s - LEFT JOIN skill_agent_grants sag ON s.id = sag.skill_id AND sag.agent_id = $1 + LEFT JOIN skill_agent_grants sag ON s.id = sag.skill_id AND sag.agent_id = $1`+grantTenantCond+` WHERE s.status = 'active'`+tenantCond+` ORDER BY s.name`, append([]any{agentID}, tcArgs...)...) if err != nil { @@ -230,7 +320,7 @@ func (s *PGSkillStore) ListWithGrantStatus(ctx context.Context, agentID uuid.UUI var result []store.SkillWithGrantStatus for rows.Next() { var r store.SkillWithGrantStatus - if err := rows.Scan(&r.ID, &r.Name, &r.Slug, &r.Description, &r.Visibility, &r.Version, &r.Granted, &r.PinnedVer, &r.IsSystem); err != nil { + if err := rows.Scan(&r.ID, &r.Name, &r.Slug, &r.Description, &r.Visibility, &r.Version, &r.Granted, &r.CanManage, &r.PinnedVer, &r.IsSystem); err != nil { slog.Warn("skill_grants: scan error in ListWithGrantStatus", "error", err) continue } diff --git a/internal/store/skill_store.go b/internal/store/skill_store.go index 478725ee..eeac4720 100644 --- a/internal/store/skill_store.go +++ b/internal/store/skill_store.go @@ -16,7 +16,7 @@ type SkillInfo struct { Source string `json:"source" db:"-"` Description string `json:"description" db:"description"` Visibility string `json:"visibility,omitempty" db:"visibility"` - OwnerID string `json:"owner_id,omitempty" db:"owner_id"` + OwnerID string `json:"-" db:"owner_id"` Tags []string `json:"tags,omitempty" db:"tags"` Version int `json:"version,omitempty" db:"version"` IsSystem bool `json:"is_system,omitempty" db:"is_system"` @@ -89,10 +89,19 @@ type SkillWithGrantStatus struct { Visibility string `json:"visibility" db:"visibility"` Version int `json:"version" db:"version"` Granted bool `json:"granted" db:"granted"` + CanManage bool `json:"can_manage" db:"can_manage"` PinnedVer *int `json:"pinned_version,omitempty" db:"pinned_version"` IsSystem bool `json:"is_system" db:"is_system"` } +// SkillAgentGrantInfo is a grant row for one skill across agents. +type SkillAgentGrantInfo struct { + AgentID uuid.UUID `json:"agent_id" db:"agent_id"` + PinnedVersion int `json:"pinned_version" db:"pinned_version"` + GrantedBy string `json:"granted_by" db:"granted_by"` + CanManage bool `json:"can_manage" db:"can_manage"` +} + // SkillManageStore extends SkillStore with CRUD, ownership, and grant operations // needed by HTTP upload handlers and agent tools (skill_manage, publish_skill). // Implemented by both PGSkillStore and SQLiteSkillStore. @@ -119,11 +128,13 @@ type SkillManageStore interface { ListSystemSkillDirs(ctx context.Context) map[string]string StoreMissingDeps(ctx context.Context, id uuid.UUID, missing []string) error // Grants - GrantToAgent(ctx context.Context, skillID, agentID uuid.UUID, version int, grantedBy string) error + GrantToAgent(ctx context.Context, skillID, agentID uuid.UUID, version int, grantedBy string, canManage ...bool) error RevokeFromAgent(ctx context.Context, skillID, agentID uuid.UUID) error GrantToUser(ctx context.Context, skillID uuid.UUID, userID, grantedBy string) error RevokeFromUser(ctx context.Context, skillID uuid.UUID, userID string) error ListWithGrantStatus(ctx context.Context, agentID uuid.UUID) ([]SkillWithGrantStatus, error) + ListAgentGrantsForSkill(ctx context.Context, skillID uuid.UUID) ([]SkillAgentGrantInfo, error) + AgentCanManageSkill(ctx context.Context, skillID, agentID uuid.UUID) (bool, error) // Files GetSkillFilePath(ctx context.Context, id uuid.UUID) (filePath string, slug string, version int, isSystem bool, ok bool) } diff --git a/internal/store/sqlitestore/schema.go b/internal/store/sqlitestore/schema.go index b06ba9b3..0266d667 100644 --- a/internal/store/sqlitestore/schema.go +++ b/internal/store/sqlitestore/schema.go @@ -16,7 +16,7 @@ var schemaSQL string // SchemaVersion is the current SQLite schema version. // Bump this when adding new migration steps below. -const SchemaVersion = 34 +const SchemaVersion = 36 // migrations maps version → SQL to apply when upgrading FROM that version. // schema.sql always represents the LATEST full schema (for fresh DBs). @@ -596,6 +596,20 @@ CREATE INDEX IF NOT EXISTS idx_ws_activity_retention ON workstation_activity(c // Version 33 → 34: per-agent ordered provider/model fallback config. 33: `ALTER TABLE agents ADD COLUMN model_fallback TEXT NOT NULL DEFAULT '{}';`, + // Version 34 → 35: agent skill grants can optionally allow skill management. + 34: `ALTER TABLE skill_agent_grants ADD COLUMN can_manage INTEGER NOT NULL DEFAULT 0;`, + + // Version 35 → 36: remove legacy cross-tenant skill-agent grant rows. + 35: `DELETE FROM skill_agent_grants +WHERE id IN ( + SELECT sag.id + FROM skill_agent_grants sag + JOIN skills s ON sag.skill_id = s.id + JOIN agents a ON sag.agent_id = a.id + WHERE sag.tenant_id <> a.tenant_id + OR (s.is_system = 0 AND sag.tenant_id <> s.tenant_id) +);`, + // Version 23 → 24: vault_documents scope/ownership consistency triggers. // Mirrors PG migration 000055 CHECK constraint; SQLite cannot add CHECK via // ALTER TABLE so we use BEFORE INSERT + BEFORE UPDATE triggers instead. @@ -975,6 +989,8 @@ func idempotentColumnMigration(version int) (string, string, bool) { return "webhooks", "encrypted_secret", true case 33: return "agents", "model_fallback", true + case 34: + return "skill_agent_grants", "can_manage", true default: return "", "", false } diff --git a/internal/store/sqlitestore/schema.sql b/internal/store/sqlitestore/schema.sql index b3d749c5..553e71ee 100644 --- a/internal/store/sqlitestore/schema.sql +++ b/internal/store/sqlitestore/schema.sql @@ -408,6 +408,7 @@ CREATE TABLE IF NOT EXISTS skill_agent_grants ( agent_id TEXT NOT NULL REFERENCES agents(id) ON DELETE CASCADE, pinned_version INT NOT NULL, granted_by VARCHAR(255) NOT NULL, + can_manage INTEGER NOT NULL DEFAULT 0, tenant_id TEXT NOT NULL REFERENCES tenants(id), created_at TEXT DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), UNIQUE(skill_id, agent_id) diff --git a/internal/store/sqlitestore/skills_grants.go b/internal/store/sqlitestore/skills_grants.go index 344189c8..d9dba9b6 100644 --- a/internal/store/sqlitestore/skills_grants.go +++ b/internal/store/sqlitestore/skills_grants.go @@ -4,6 +4,8 @@ package sqlitestore import ( "context" + "database/sql" + "fmt" "log/slog" "time" @@ -20,27 +22,48 @@ type SkillGrantInfo struct { } // GrantToAgent grants a skill to an agent with version pinning. -func (s *SQLiteSkillStore) GrantToAgent(ctx context.Context, skillID, agentID uuid.UUID, version int, grantedBy string) error { +func (s *SQLiteSkillStore) GrantToAgent(ctx context.Context, skillID, agentID uuid.UUID, version int, grantedBy string, canManage ...bool) error { if err := store.ValidateUserID(grantedBy); err != nil { return err } - // Upsert grant. id := store.GenNewID() - _, err := s.db.ExecContext(ctx, - `INSERT INTO skill_agent_grants (id, skill_id, agent_id, pinned_version, granted_by, created_at, tenant_id) - VALUES (?, ?, ?, ?, ?, ?, ?) - ON CONFLICT (skill_id, agent_id) DO UPDATE SET pinned_version = excluded.pinned_version`, - id, skillID, agentID, version, grantedBy, time.Now().UTC(), tenantIDForInsert(ctx), - ) + now := time.Now().UTC() + tid := tenantIDForInsert(ctx) + if err := s.verifySkillGrantScope(ctx, skillID, agentID, tid); err != nil { + return err + } + var err error + if len(canManage) > 0 { + _, err = s.db.ExecContext(ctx, + `INSERT INTO skill_agent_grants (id, skill_id, agent_id, pinned_version, granted_by, can_manage, created_at, tenant_id) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + ON CONFLICT (skill_id, agent_id) DO UPDATE SET + pinned_version = excluded.pinned_version, + granted_by = excluded.granted_by, + can_manage = excluded.can_manage`, + id, skillID, agentID, version, grantedBy, canManage[0], now, tid, + ) + } else { + _, err = s.db.ExecContext(ctx, + `INSERT INTO skill_agent_grants (id, skill_id, agent_id, pinned_version, granted_by, created_at, tenant_id) + VALUES (?, ?, ?, ?, ?, ?, ?) + ON CONFLICT (skill_id, agent_id) DO UPDATE SET + pinned_version = excluded.pinned_version, + granted_by = excluded.granted_by`, + id, skillID, agentID, version, grantedBy, now, tid, + ) + } if err != nil { return err } // Auto-promote: private → internal. _, err = s.db.ExecContext(ctx, - `UPDATE skills SET visibility = 'internal', updated_at = ? WHERE id = ? AND visibility = 'private'`, - time.Now().UTC(), skillID) + `UPDATE skills + SET visibility = 'internal', updated_at = ? + WHERE id = ? AND visibility = 'private' AND (is_system = 1 OR tenant_id = ?)`, + time.Now().UTC(), skillID, tid) if err != nil { slog.Warn("skill_grants: failed to auto-promote visibility", "skill_id", skillID, "error", err) } @@ -51,6 +74,10 @@ func (s *SQLiteSkillStore) GrantToAgent(ctx context.Context, skillID, agentID uu // RevokeFromAgent revokes a skill grant from an agent. func (s *SQLiteSkillStore) RevokeFromAgent(ctx context.Context, skillID, agentID uuid.UUID) error { + tid := tenantIDForInsert(ctx) + if err := s.verifySkillInGrantScope(ctx, skillID, tid); err != nil { + return err + } tClause, tArgs, err := scopeClause(ctx) if err != nil { return err @@ -65,9 +92,9 @@ func (s *SQLiteSkillStore) RevokeFromAgent(ctx context.Context, skillID, agentID // Auto-demote: internal → private when no grants remain. _, err = s.db.ExecContext(ctx, `UPDATE skills SET visibility = 'private', updated_at = ? - WHERE id = ? AND visibility = 'internal' + WHERE id = ? AND visibility = 'internal' AND (is_system = 1 OR tenant_id = ?) AND NOT EXISTS (SELECT 1 FROM skill_agent_grants WHERE skill_id = ?)`, - time.Now().UTC(), skillID, skillID) + time.Now().UTC(), skillID, tid, skillID) if err != nil { slog.Warn("skill_grants: failed to auto-demote visibility", "skill_id", skillID, "error", err) } @@ -76,6 +103,37 @@ func (s *SQLiteSkillStore) RevokeFromAgent(ctx context.Context, skillID, agentID return nil } +func (s *SQLiteSkillStore) verifySkillGrantScope(ctx context.Context, skillID, agentID, tenantID uuid.UUID) error { + if err := s.verifySkillInGrantScope(ctx, skillID, tenantID); err != nil { + return err + } + + var agentTenantID uuid.UUID + if err := s.db.QueryRowContext(ctx, + "SELECT tenant_id FROM agents WHERE id = ?", agentID, + ).Scan(&agentTenantID); err != nil { + return fmt.Errorf("agent not found") + } + if agentTenantID != tenantID { + return fmt.Errorf("agent not found") + } + return nil +} + +func (s *SQLiteSkillStore) verifySkillInGrantScope(ctx context.Context, skillID, tenantID uuid.UUID) error { + var skillTenantID uuid.UUID + var isSystem bool + if err := s.db.QueryRowContext(ctx, + "SELECT tenant_id, is_system FROM skills WHERE id = ?", skillID, + ).Scan(&skillTenantID, &isSystem); err != nil { + return fmt.Errorf("skill not found") + } + if !isSystem && skillTenantID != tenantID { + return fmt.Errorf("skill not found") + } + return nil +} + // ListAgentGrants returns all skill grants for an agent. func (s *SQLiteSkillStore) ListAgentGrants(ctx context.Context, agentID uuid.UUID) ([]SkillGrantInfo, error) { tClause, tArgs, err := scopeClause(ctx) @@ -102,6 +160,57 @@ func (s *SQLiteSkillStore) ListAgentGrants(ctx context.Context, agentID uuid.UUI return result, rows.Err() } +// ListAgentGrantsForSkill returns all agent grants for one skill. +func (s *SQLiteSkillStore) ListAgentGrantsForSkill(ctx context.Context, skillID uuid.UUID) ([]store.SkillAgentGrantInfo, error) { + if err := s.verifySkillInGrantScope(ctx, skillID, tenantIDForInsert(ctx)); err != nil { + return nil, err + } + tClause, tArgs, err := scopeClause(ctx) + if err != nil { + return nil, err + } + rows, err := s.db.QueryContext(ctx, + "SELECT agent_id, pinned_version, granted_by, can_manage FROM skill_agent_grants WHERE skill_id = ?"+tClause+" ORDER BY created_at DESC", + append([]any{skillID}, tArgs...)...) + if err != nil { + return nil, err + } + defer rows.Close() + + var result []store.SkillAgentGrantInfo + for rows.Next() { + var g store.SkillAgentGrantInfo + if err := rows.Scan(&g.AgentID, &g.PinnedVersion, &g.GrantedBy, &g.CanManage); err != nil { + slog.Warn("skill_grants: scan error in ListAgentGrantsForSkill", "error", err) + continue + } + result = append(result, g) + } + return result, rows.Err() +} + +// AgentCanManageSkill reports whether an agent has explicit edit/delete rights for a skill. +func (s *SQLiteSkillStore) AgentCanManageSkill(ctx context.Context, skillID, agentID uuid.UUID) (bool, error) { + if err := s.verifySkillInGrantScope(ctx, skillID, tenantIDForInsert(ctx)); err != nil { + return false, err + } + tClause, tArgs, err := scopeClause(ctx) + if err != nil { + return false, err + } + var canManage bool + err = s.db.QueryRowContext(ctx, + "SELECT can_manage FROM skill_agent_grants WHERE skill_id = ? AND agent_id = ?"+tClause, + append([]any{skillID, agentID}, tArgs...)...).Scan(&canManage) + if err == sql.ErrNoRows { + return false, nil + } + if err != nil { + return false, err + } + return canManage, nil +} + // GrantToUser grants a skill to a user. func (s *SQLiteSkillStore) GrantToUser(ctx context.Context, skillID uuid.UUID, userID, grantedBy string) error { if err := store.ValidateUserID(userID); err != nil { @@ -201,23 +310,27 @@ func (s *SQLiteSkillStore) ListWithGrantStatus(ctx context.Context, agentID uuid return nil, err } tenantCond := "" + grantTenantCond := "" if len(tArgs) > 0 { tenantCond = " AND (s.is_system = 1 OR s.tenant_id = ?)" + grantTenantCond = " AND sag.tenant_id = ?" } _ = tClause queryArgs := []any{agentID} if len(tArgs) > 0 { queryArgs = append(queryArgs, tArgs...) + queryArgs = append(queryArgs, tArgs...) } rows, err := s.db.QueryContext(ctx, `SELECT s.id, s.name, s.slug, COALESCE(s.description, ''), s.visibility, s.version, (sag.id IS NOT NULL) AS granted, + COALESCE(sag.can_manage, 0) AS can_manage, sag.pinned_version, s.is_system FROM skills s - LEFT JOIN skill_agent_grants sag ON s.id = sag.skill_id AND sag.agent_id = ? + LEFT JOIN skill_agent_grants sag ON s.id = sag.skill_id AND sag.agent_id = ?`+grantTenantCond+` WHERE s.status = 'active'`+tenantCond+` ORDER BY s.name`, queryArgs...) if err != nil { @@ -229,7 +342,7 @@ func (s *SQLiteSkillStore) ListWithGrantStatus(ctx context.Context, agentID uuid for rows.Next() { var r store.SkillWithGrantStatus if err := rows.Scan(&r.ID, &r.Name, &r.Slug, &r.Description, &r.Visibility, - &r.Version, &r.Granted, &r.PinnedVer, &r.IsSystem); err != nil { + &r.Version, &r.Granted, &r.CanManage, &r.PinnedVer, &r.IsSystem); err != nil { slog.Warn("skill_grants: scan error in ListWithGrantStatus", "error", err) continue } diff --git a/internal/store/sqlitestore/skills_test.go b/internal/store/sqlitestore/skills_test.go index d72ba7c9..3e640ca2 100644 --- a/internal/store/sqlitestore/skills_test.go +++ b/internal/store/sqlitestore/skills_test.go @@ -4,10 +4,13 @@ package sqlitestore import ( "context" + "database/sql" "path/filepath" "reflect" "testing" + "github.com/google/uuid" + "github.com/nextlevelbuilder/goclaw/internal/store" ) @@ -67,7 +70,119 @@ func TestSQLiteSkillStore_CreateSkillManaged_PersistsArchivedDependencyState(t * } } +func TestSQLiteSkillStore_GrantToAgentRejectsCrossTenantSkill(t *testing.T) { + _, skillStore, db := newTestSQLiteSkillStoreWithDB(t) + tenantA, agentA := seedSQLiteTenantAgent(t, db) + tenantB, _ := seedSQLiteTenantAgent(t, db) + ctxA := store.WithTenantID(context.Background(), tenantA) + ctxB := store.WithTenantID(context.Background(), tenantB) + + skillID, err := skillStore.CreateSkillManaged(ctxB, store.SkillCreateParams{ + Name: "Tenant B Skill", + Slug: "tenant-b-skill-" + tenantB.String()[:8], + OwnerID: "user-1", + Visibility: "private", + FilePath: filepath.Join(t.TempDir(), "tenant-b-skill", "1"), + }) + if err != nil { + t.Fatalf("CreateSkillManaged error: %v", err) + } + + if err := skillStore.GrantToAgent(ctxA, skillID, agentA, 1, "user-1", true); err == nil { + t.Fatal("GrantToAgent allowed tenant A to grant tenant B skill") + } + + grants, err := skillStore.ListAgentGrantsForSkill(ctxB, skillID) + if err != nil { + t.Fatalf("ListAgentGrantsForSkill error: %v", err) + } + if len(grants) != 0 { + t.Fatalf("cross-tenant grant was inserted: %+v", grants) + } + + got, ok := skillStore.GetSkillByID(ctxB, skillID) + if !ok { + t.Fatal("GetSkillByID returned !ok") + } + if got.Visibility != "private" { + t.Fatalf("cross-tenant grant changed visibility to %q, want private", got.Visibility) + } +} + +func TestSQLiteSkillStore_RevokeFromAgentDoesNotDemoteCrossTenantSkill(t *testing.T) { + _, skillStore, db := newTestSQLiteSkillStoreWithDB(t) + tenantA, agentA := seedSQLiteTenantAgent(t, db) + tenantB, _ := seedSQLiteTenantAgent(t, db) + ctxA := store.WithTenantID(context.Background(), tenantA) + ctxB := store.WithTenantID(context.Background(), tenantB) + + skillID, err := skillStore.CreateSkillManaged(ctxB, store.SkillCreateParams{ + Name: "Tenant B Skill", + Slug: "tenant-b-revoke-skill-" + tenantB.String()[:8], + OwnerID: "user-1", + Visibility: "internal", + FilePath: filepath.Join(t.TempDir(), "tenant-b-revoke-skill", "1"), + }) + if err != nil { + t.Fatalf("CreateSkillManaged error: %v", err) + } + + if err := skillStore.RevokeFromAgent(ctxA, skillID, agentA); err == nil { + t.Fatal("RevokeFromAgent allowed tenant A to revoke tenant B skill") + } + + got, ok := skillStore.GetSkillByID(ctxB, skillID) + if !ok { + t.Fatal("GetSkillByID returned !ok") + } + if got.Visibility != "internal" { + t.Fatalf("cross-tenant revoke demoted visibility to %q, want internal", got.Visibility) + } +} + +func TestSQLiteSkillStore_ListWithGrantStatusIgnoresForeignTenantGrant(t *testing.T) { + _, skillStore, db := newTestSQLiteSkillStoreWithDB(t) + tenantA, _ := seedSQLiteTenantAgent(t, db) + tenantB, agentB := seedSQLiteTenantAgent(t, db) + ctxA := store.WithTenantID(context.Background(), tenantA) + + skillID := uuid.New() + if _, err := db.Exec( + `INSERT INTO skills (id, name, slug, owner_id, visibility, version, status, file_path, is_system, tenant_id) + VALUES (?, 'System Skill', ?, 'system', 'internal', 1, 'active', ?, 1, ?)`, + skillID.String(), "system-grant-status-"+skillID.String()[:8], filepath.Join(t.TempDir(), "system-skill", "1"), store.MasterTenantID.String(), + ); err != nil { + t.Fatalf("insert system skill: %v", err) + } + if _, err := db.Exec( + `INSERT INTO skill_agent_grants (id, skill_id, agent_id, pinned_version, granted_by, can_manage, tenant_id) + VALUES (?, ?, ?, 1, 'tenant-b-admin', 1, ?)`, + uuid.New().String(), skillID.String(), agentB.String(), tenantB.String(), + ); err != nil { + t.Fatalf("insert foreign tenant grant: %v", err) + } + + skills, err := skillStore.ListWithGrantStatus(ctxA, agentB) + if err != nil { + t.Fatalf("ListWithGrantStatus error: %v", err) + } + for _, skill := range skills { + if skill.ID == skillID { + if skill.Granted || skill.CanManage { + t.Fatalf("foreign tenant grant leaked into tenant A status: granted=%v canManage=%v", skill.Granted, skill.CanManage) + } + return + } + } + t.Fatalf("system skill %s not returned for tenant A", skillID) +} + func newTestSQLiteSkillStore(t *testing.T) (context.Context, *SQLiteSkillStore) { + ctx, skillStore, _ := newTestSQLiteSkillStoreWithDB(t) + return ctx, skillStore +} + +func newTestSQLiteSkillStoreWithDB(t *testing.T) (context.Context, *SQLiteSkillStore, *sql.DB) { t.Helper() db, err := OpenDB(filepath.Join(t.TempDir(), "skills.db")) @@ -79,5 +194,26 @@ func newTestSQLiteSkillStore(t *testing.T) (context.Context, *SQLiteSkillStore) t.Fatalf("EnsureSchema error: %v", err) } - return store.WithTenantID(context.Background(), store.MasterTenantID), NewSQLiteSkillStore(db, t.TempDir()) + return store.WithTenantID(context.Background(), store.MasterTenantID), NewSQLiteSkillStore(db, t.TempDir()), db +} + +func seedSQLiteTenantAgent(t *testing.T, db *sql.DB) (uuid.UUID, uuid.UUID) { + t.Helper() + + tenantID := uuid.New() + agentID := uuid.New() + if _, err := db.Exec( + `INSERT INTO tenants (id, name, slug, status) VALUES (?, ?, ?, 'active')`, + tenantID.String(), "tenant-"+tenantID.String()[:8], "t"+tenantID.String()[:8], + ); err != nil { + t.Fatalf("insert tenant: %v", err) + } + if _, err := db.Exec( + `INSERT INTO agents (id, tenant_id, agent_key, agent_type, status, provider, model, owner_id) + VALUES (?, ?, ?, 'predefined', 'active', 'test', 'test-model', 'user-1')`, + agentID.String(), tenantID.String(), "agent-"+agentID.String()[:8], + ); err != nil { + t.Fatalf("insert agent: %v", err) + } + return tenantID, agentID } diff --git a/internal/tools/publish_skill.go b/internal/tools/publish_skill.go index dd3f50a7..227033ce 100644 --- a/internal/tools/publish_skill.go +++ b/internal/tools/publish_skill.go @@ -170,7 +170,7 @@ func (t *PublishSkillTool) Execute(ctx context.Context, args map[string]any) *Re // Auto-grant to calling agent (granted-by = owner, same as CreateSkillManaged) agentID := store.AgentIDFromContext(ctx) if agentID != uuid.Nil { - if err := t.skills.GrantToAgent(ctx, id, agentID, version, ownerID); err != nil { + if err := t.skills.GrantToAgent(ctx, id, agentID, version, ownerID, true); err != nil { slog.Warn("publish_skill: auto-grant failed", "error", err) } } diff --git a/internal/tools/skill_manage.go b/internal/tools/skill_manage.go index 16b70545..aabcd6b1 100644 --- a/internal/tools/skill_manage.go +++ b/internal/tools/skill_manage.go @@ -51,6 +51,29 @@ func isOwnerOfSkill(ctx context.Context, skills store.SkillManageStore, slug str return ownerID == actorID || ownerID == userID || ownerID == senderID } +func canManageSkill(ctx context.Context, skills store.SkillManageStore, info *store.SkillInfo) bool { + if isOwnerOfSkill(ctx, skills, info.Slug) { + return true + } + if info.ID == "" { + return false + } + skillID, err := uuid.Parse(info.ID) + if err != nil { + return false + } + agentID := store.AgentIDFromContext(ctx) + if agentID == uuid.Nil { + return false + } + ok, err := skills.AgentCanManageSkill(ctx, skillID, agentID) + if err != nil { + slog.Warn("skill_manage: manage grant check failed", "skill", info.Slug, "agent_id", agentID, "error", err) + return false + } + return ok +} + // tenantSkillsDir returns the skills-store directory scoped to the calling agent's tenant. func (t *SkillManageTool) tenantSkillsDir(ctx context.Context) string { tid := store.TenantIDFromContext(ctx) @@ -211,7 +234,7 @@ func (t *SkillManageTool) executeCreate(ctx context.Context, args map[string]any granted := false agentID := store.AgentIDFromContext(ctx) if agentID != uuid.Nil { - if err := t.skills.GrantToAgent(ctx, id, agentID, version, ownerID); err != nil { + if err := t.skills.GrantToAgent(ctx, id, agentID, version, ownerID, true); err != nil { slog.Warn("skill_manage: auto-grant failed", "error", err) } else { granted = true @@ -278,7 +301,7 @@ func (t *SkillManageTool) executePatch(ctx context.Context, args map[string]any) // where DM owners got the raw channel sender) // A DM user merged to "viettx" with Telegram ID "386246614" matches all // three of their skills regardless of when they were created. - if !isOwnerOfSkill(ctx, t.skills, slug) { + if !canManageSkill(ctx, t.skills, info) { return ErrorResult(fmt.Sprintf("cannot manage skill %q: you are not the owner", slug)) } @@ -392,7 +415,7 @@ func (t *SkillManageTool) executeDelete(ctx context.Context, args map[string]any // Ownership check: only the skill owner can delete. // Same three-identity match as the patch flow above (#915). - if !isOwnerOfSkill(ctx, t.skills, slug) { + if !canManageSkill(ctx, t.skills, info) { return ErrorResult(fmt.Sprintf("cannot manage skill %q: you are not the owner", slug)) } diff --git a/internal/upgrade/version.go b/internal/upgrade/version.go index ab3b06d7..240912b1 100644 --- a/internal/upgrade/version.go +++ b/internal/upgrade/version.go @@ -2,4 +2,4 @@ package upgrade // RequiredSchemaVersion is the schema migration version this binary requires. // Bump this whenever adding a new SQL migration file. -const RequiredSchemaVersion uint = 65 +const RequiredSchemaVersion uint = 67 diff --git a/migrations/000066_skill_agent_manage_grants.down.sql b/migrations/000066_skill_agent_manage_grants.down.sql new file mode 100644 index 00000000..6ff8efba --- /dev/null +++ b/migrations/000066_skill_agent_manage_grants.down.sql @@ -0,0 +1,2 @@ +ALTER TABLE skill_agent_grants + DROP COLUMN IF EXISTS can_manage; diff --git a/migrations/000066_skill_agent_manage_grants.up.sql b/migrations/000066_skill_agent_manage_grants.up.sql new file mode 100644 index 00000000..8cf33e67 --- /dev/null +++ b/migrations/000066_skill_agent_manage_grants.up.sql @@ -0,0 +1,2 @@ +ALTER TABLE skill_agent_grants + ADD COLUMN IF NOT EXISTS can_manage BOOLEAN NOT NULL DEFAULT FALSE; diff --git a/migrations/000067_skill_agent_grants_scope_cleanup.down.sql b/migrations/000067_skill_agent_grants_scope_cleanup.down.sql new file mode 100644 index 00000000..cfa54c36 --- /dev/null +++ b/migrations/000067_skill_agent_grants_scope_cleanup.down.sql @@ -0,0 +1 @@ +-- Irreversible cleanup migration. diff --git a/migrations/000067_skill_agent_grants_scope_cleanup.up.sql b/migrations/000067_skill_agent_grants_scope_cleanup.up.sql new file mode 100644 index 00000000..f024264f --- /dev/null +++ b/migrations/000067_skill_agent_grants_scope_cleanup.up.sql @@ -0,0 +1,8 @@ +DELETE FROM skill_agent_grants sag +USING skills s, agents a +WHERE sag.skill_id = s.id + AND sag.agent_id = a.id + AND ( + sag.tenant_id <> a.tenant_id + OR (s.is_system = false AND sag.tenant_id <> s.tenant_id) + ); diff --git a/tests/integration/v3_skills_store_test.go b/tests/integration/v3_skills_store_test.go index 5d9cd130..0ae48a34 100644 --- a/tests/integration/v3_skills_store_test.go +++ b/tests/integration/v3_skills_store_test.go @@ -271,6 +271,9 @@ func TestStoreSkill_GrantToAgent(t *testing.T) { if !g.Granted { t.Error("expected Granted=true for granted skill") } + if g.CanManage { + t.Error("expected CanManage=false by default") + } found = true break } @@ -295,6 +298,37 @@ func TestStoreSkill_GrantToAgent(t *testing.T) { t.Error("granted skill not found in ListAccessible") } + if err := s.GrantToAgent(ctx, skillID, agentID, 1, "test-owner", true); err != nil { + t.Fatalf("GrantToAgent can_manage: %v", err) + } + canManage, err := s.AgentCanManageSkill(ctx, skillID, agentID) + if err != nil { + t.Fatalf("AgentCanManageSkill: %v", err) + } + if !canManage { + t.Error("expected AgentCanManageSkill=true after manage grant") + } + if err := s.GrantToAgent(ctx, skillID, agentID, 1, "test-owner"); err != nil { + t.Fatalf("GrantToAgent preserve can_manage: %v", err) + } + canManage, err = s.AgentCanManageSkill(ctx, skillID, agentID) + if err != nil { + t.Fatalf("AgentCanManageSkill after preserve grant: %v", err) + } + if !canManage { + t.Error("expected omitted can_manage grant update to preserve existing manage permission") + } + if err := s.GrantToAgent(ctx, skillID, agentID, 1, "test-owner", false); err != nil { + t.Fatalf("GrantToAgent can_manage false: %v", err) + } + canManage, err = s.AgentCanManageSkill(ctx, skillID, agentID) + if err != nil { + t.Fatalf("AgentCanManageSkill after false grant: %v", err) + } + if canManage { + t.Error("expected explicit can_manage=false to revoke manage permission") + } + // Revoke if err := s.RevokeFromAgent(ctx, skillID, agentID); err != nil { t.Fatalf("RevokeFromAgent: %v", err) @@ -312,6 +346,101 @@ func TestStoreSkill_GrantToAgent(t *testing.T) { } } +func TestStoreSkill_GrantToAgentRejectsCrossTenantSkill(t *testing.T) { + db := testDB(t) + tenantA, agentA := seedTenantAgent(t, db) + tenantB, _ := seedTenantAgent(t, db) + ctxA := tenantCtx(tenantA) + ctxB := tenantCtx(tenantB) + s := newSkillStore(t) + + skillB := seedSkill(t, s, ctxB, "grant-cross-tenant-"+tenantB.String()[:8], "Tenant B Skill") + + if err := s.GrantToAgent(ctxA, skillB, agentA, 1, "test-owner", true); err == nil { + t.Fatal("GrantToAgent allowed tenant A to grant tenant B skill") + } + + grants, err := s.ListAgentGrantsForSkill(ctxB, skillB) + if err != nil { + t.Fatalf("ListAgentGrantsForSkill: %v", err) + } + if len(grants) != 0 { + t.Fatalf("cross-tenant grant was inserted: %+v", grants) + } + + got, ok := s.GetSkillByID(ctxB, skillB) + if !ok { + t.Fatal("GetSkillByID for tenant B skill returned false") + } + if got.Visibility != "private" { + t.Fatalf("cross-tenant grant changed visibility to %q, want private", got.Visibility) + } +} + +func TestStoreSkill_RevokeFromAgentDoesNotDemoteCrossTenantSkill(t *testing.T) { + db := testDB(t) + tenantA, agentA := seedTenantAgent(t, db) + tenantB, _ := seedTenantAgent(t, db) + ctxA := tenantCtx(tenantA) + ctxB := tenantCtx(tenantB) + s := newSkillStore(t) + + skillB := seedSkill(t, s, ctxB, "revoke-cross-tenant-"+tenantB.String()[:8], "Tenant B Skill") + if err := s.UpdateSkill(ctxB, skillB, map[string]any{"visibility": "internal"}); err != nil { + t.Fatalf("UpdateSkill: %v", err) + } + + if err := s.RevokeFromAgent(ctxA, skillB, agentA); err == nil { + t.Fatal("RevokeFromAgent allowed tenant A to revoke tenant B skill") + } + + got, ok := s.GetSkillByID(ctxB, skillB) + if !ok { + t.Fatal("GetSkillByID for tenant B skill returned false") + } + if got.Visibility != "internal" { + t.Fatalf("cross-tenant revoke demoted visibility to %q, want internal", got.Visibility) + } +} + +func TestStoreSkill_ListWithGrantStatusIgnoresForeignTenantGrant(t *testing.T) { + db := testDB(t) + tenantA, _ := seedTenantAgent(t, db) + tenantB, agentB := seedTenantAgent(t, db) + ctxA := tenantCtx(tenantA) + s := newSkillStore(t) + + skillID := uuid.New() + if _, err := db.Exec( + `INSERT INTO skills (id, name, slug, owner_id, visibility, version, status, file_path, is_system, tenant_id) + VALUES ($1, 'System Skill', $2, 'system', 'internal', 1, 'active', $3, true, $4)`, + skillID, "system-grant-status-"+skillID.String()[:8], "/tmp/skills/system-skill/1", store.MasterTenantID, + ); err != nil { + t.Fatalf("insert system skill: %v", err) + } + if _, err := db.Exec( + `INSERT INTO skill_agent_grants (id, skill_id, agent_id, pinned_version, granted_by, can_manage, tenant_id) + VALUES ($1, $2, $3, 1, 'tenant-b-admin', true, $4)`, + uuid.New(), skillID, agentB, tenantB, + ); err != nil { + t.Fatalf("insert foreign tenant grant: %v", err) + } + + skills, err := s.ListWithGrantStatus(ctxA, agentB) + if err != nil { + t.Fatalf("ListWithGrantStatus error: %v", err) + } + for _, skill := range skills { + if skill.ID == skillID { + if skill.Granted || skill.CanManage { + t.Fatalf("foreign tenant grant leaked into tenant A status: granted=%v canManage=%v", skill.Granted, skill.CanManage) + } + return + } + } + t.Fatalf("system skill %s not returned for tenant A", skillID) +} + func TestStoreSkill_TenantIsolation(t *testing.T) { db := testDB(t) tenantA, _ := seedTenantAgent(t, db) diff --git a/ui/web/src/i18n/locales/en/skills.json b/ui/web/src/i18n/locales/en/skills.json index a5f1dce3..c8df5947 100644 --- a/ui/web/src/i18n/locales/en/skills.json +++ b/ui/web/src/i18n/locales/en/skills.json @@ -11,11 +11,31 @@ "description": "Description", "source": "Source", "author": "Author", + "owner": "Owner", "visibility": "Visibility", "status": "Status", "actions": "Actions" }, "noDescription": "No description", + "owner": "Owner", + "unknownOwner": "Unknown", + "grants": { + "title": "Agent access for {{name}}", + "manage": "Manage agent access", + "current": "Current agent grants", + "none": "No agent grants yet.", + "add": "Add grant", + "update": "Update grant", + "grant": "Grant", + "save": "Save", + "revoke": "Revoke grant", + "selectAgent": "Select agent", + "allowManage": "Allow this agent to edit or delete the skill", + "canManage": "Can edit", + "loadFailed": "Failed to load grants", + "saveFailed": "Failed to save grant", + "revokeFailed": "Failed to revoke grant" + }, "visibility": { "clickToCycle": "Click to change visibility" }, diff --git a/ui/web/src/i18n/locales/vi/skills.json b/ui/web/src/i18n/locales/vi/skills.json index 9f35cecb..5833c4c4 100644 --- a/ui/web/src/i18n/locales/vi/skills.json +++ b/ui/web/src/i18n/locales/vi/skills.json @@ -13,7 +13,8 @@ "author": "Tác giả", "visibility": "Hiển thị", "status": "Trạng thái", - "actions": "Thao tác" + "actions": "Thao tác", + "owner": "Chủ sở hữu" }, "noDescription": "Không có mô tả", "visibility": { @@ -125,5 +126,24 @@ "rescanUpdated": "Đã cập nhật {{count}} skill", "rescanNoChanges": "Tất cả skill đã cập nhật", "rescanFailed": "Không thể quét lại dependencies" + }, + "owner": "Chủ sở hữu", + "unknownOwner": "Không rõ", + "grants": { + "title": "Quyền agent cho {{name}}", + "manage": "Quản lý quyền agent", + "current": "Grant agent hiện tại", + "none": "Chưa có grant agent.", + "add": "Thêm grant", + "update": "Cập nhật grant", + "grant": "Grant", + "save": "Lưu", + "revoke": "Thu hồi grant", + "selectAgent": "Chọn agent", + "allowManage": "Cho phép agent này sửa hoặc xóa skill", + "canManage": "Được sửa", + "loadFailed": "Không thể tải grant", + "saveFailed": "Không thể lưu grant", + "revokeFailed": "Không thể thu hồi grant" } } diff --git a/ui/web/src/i18n/locales/zh/skills.json b/ui/web/src/i18n/locales/zh/skills.json index 1ec658e9..b3c0444d 100644 --- a/ui/web/src/i18n/locales/zh/skills.json +++ b/ui/web/src/i18n/locales/zh/skills.json @@ -13,7 +13,8 @@ "author": "作者", "visibility": "可见性", "status": "状态", - "actions": "操作" + "actions": "操作", + "owner": "所有者" }, "noDescription": "暂无描述", "visibility": { @@ -125,5 +126,24 @@ "rescanUpdated": "已更新 {{count}} 个技能", "rescanNoChanges": "所有技能已是最新", "rescanFailed": "重新扫描依赖失败" + }, + "owner": "所有者", + "unknownOwner": "未知", + "grants": { + "title": "{{name}} 的 Agent 权限", + "manage": "管理 Agent 权限", + "current": "当前 Agent 授权", + "none": "暂无 Agent 授权。", + "add": "添加授权", + "update": "更新授权", + "grant": "授权", + "save": "保存", + "revoke": "撤销授权", + "selectAgent": "选择 Agent", + "allowManage": "允许此 Agent 编辑或删除 Skill", + "canManage": "可编辑", + "loadFailed": "无法加载授权", + "saveFailed": "无法保存授权", + "revokeFailed": "无法撤销授权" } } diff --git a/ui/web/src/pages/skills/hooks/use-skills.ts b/ui/web/src/pages/skills/hooks/use-skills.ts index a842c67e..8dc3ac9a 100644 --- a/ui/web/src/pages/skills/hooks/use-skills.ts +++ b/ui/web/src/pages/skills/hooks/use-skills.ts @@ -7,7 +7,7 @@ import { queryKeys } from "@/lib/query-keys"; import { toast } from "@/stores/use-toast-store"; import i18next from "i18next"; import { userFriendlyError } from "@/lib/error-utils"; -import type { SkillInfo, SkillFile, SkillVersions } from "@/types/skill"; +import type { SkillInfo, SkillFile, SkillVersions, SkillAgentGrant } from "@/types/skill"; export type { SkillInfo, SkillFile, SkillVersions }; @@ -105,6 +105,34 @@ export function useSkills() { [http, invalidate], ); + const listAgentGrants = useCallback( + async (id: string) => { + const res = await http.get<{ grants: SkillAgentGrant[] }>(`/v1/skills/${id}/grants/agent`); + return res.grants ?? []; + }, + [http], + ); + + const grantSkillToAgent = useCallback( + async (id: string, agentId: string, version: number, canManage: boolean) => { + await http.post<{ ok: string }>(`/v1/skills/${id}/grants/agent`, { + agent_id: agentId, + version, + can_manage: canManage, + }); + await invalidate(); + }, + [http, invalidate], + ); + + const revokeSkillFromAgent = useCallback( + async (id: string, agentId: string) => { + await http.delete<{ ok: string }>(`/v1/skills/${id}/grants/agent/${agentId}`); + await invalidate(); + }, + [http, invalidate], + ); + const getSkillVersions = useCallback( async (id: string) => { return http.get(`/v1/skills/${id}/versions`); @@ -220,6 +248,7 @@ export function useSkills() { return { skills, loading, refresh: invalidate, getSkill, uploadSkill, updateSkill, deleteSkill, + listAgentGrants, grantSkillToAgent, revokeSkillFromAgent, getSkillVersions, getSkillFiles, getSkillFileContent, rescanDeps, installDeps, installSingleDep, toggleSkill, setTenantConfig, deleteTenantConfig, }; diff --git a/ui/web/src/pages/skills/skill-agent-grants-dialog.tsx b/ui/web/src/pages/skills/skill-agent-grants-dialog.tsx new file mode 100644 index 00000000..399d0481 --- /dev/null +++ b/ui/web/src/pages/skills/skill-agent-grants-dialog.tsx @@ -0,0 +1,187 @@ +import { useEffect, useMemo, useState } from "react"; +import { useTranslation } from "react-i18next"; +import { Plus, Trash2, ShieldCheck } from "lucide-react"; +import { + Dialog, + DialogContent, + DialogHeader, + DialogTitle, +} from "@/components/ui/dialog"; +import { Button } from "@/components/ui/button"; +import { Badge } from "@/components/ui/badge"; +import { Label } from "@/components/ui/label"; +import { Switch } from "@/components/ui/switch"; +import { + Select, + SelectContent, + SelectItem, + SelectTrigger, + SelectValue, +} from "@/components/ui/select"; +import { useAgents } from "@/pages/agents/hooks/use-agents"; +import type { SkillAgentGrant, SkillInfo } from "@/types/skill"; + +interface SkillAgentGrantsDialogProps { + skill: SkillInfo; + onClose: () => void; + onLoad: (skillId: string) => Promise; + onGrant: (skillId: string, agentId: string, version: number, canManage: boolean) => Promise; + onRevoke: (skillId: string, agentId: string) => Promise; +} + +export function SkillAgentGrantsDialog({ + skill, + onClose, + onLoad, + onGrant, + onRevoke, +}: SkillAgentGrantsDialogProps) { + const { t } = useTranslation("skills"); + const { agents } = useAgents(); + const [grants, setGrants] = useState([]); + const [agentId, setAgentId] = useState(""); + const [canManage, setCanManage] = useState(false); + const [loading, setLoading] = useState(false); + const [error, setError] = useState(""); + + useEffect(() => { + if (!skill.id) return; + setLoading(true); + setError(""); + onLoad(skill.id) + .then(setGrants) + .catch((err) => setError(err instanceof Error ? err.message : t("grants.loadFailed"))) + .finally(() => setLoading(false)); + }, [skill.id, onLoad, t]); + + const agentNames = useMemo(() => { + const map = new Map(); + for (const agent of agents) map.set(agent.id, agent.display_name || agent.agent_key); + return map; + }, [agents]); + + const selectedGrant = grants.find((grant) => grant.agent_id === agentId); + + useEffect(() => { + setCanManage(selectedGrant?.can_manage ?? false); + }, [selectedGrant]); + + const handleGrant = async () => { + if (!skill.id || !agentId) return; + setLoading(true); + setError(""); + try { + await onGrant(skill.id, agentId, skill.version ?? 1, canManage); + setGrants((current) => { + const next: SkillAgentGrant = { + agent_id: agentId, + pinned_version: skill.version ?? 1, + granted_by: "", + can_manage: canManage, + }; + if (current.some((grant) => grant.agent_id === agentId)) { + return current.map((grant) => (grant.agent_id === agentId ? next : grant)); + } + return [...current, next]; + }); + setAgentId(""); + setCanManage(false); + } catch (err) { + setError(err instanceof Error ? err.message : t("grants.saveFailed")); + } finally { + setLoading(false); + } + }; + + const handleRevoke = async (grant: SkillAgentGrant) => { + if (!skill.id) return; + setLoading(true); + setError(""); + try { + await onRevoke(skill.id, grant.agent_id); + setGrants((current) => current.filter((item) => item.agent_id !== grant.agent_id)); + if (agentId === grant.agent_id) setAgentId(""); + } catch (err) { + setError(err instanceof Error ? err.message : t("grants.revokeFailed")); + } finally { + setLoading(false); + } + }; + + return ( + !open && onClose()}> + + + {t("grants.title", { name: skill.name })} + + +
+
+ + {grants.length === 0 ? ( +

{t("grants.none")}

+ ) : ( +
+ {grants.map((grant) => ( +
+
+

{agentNames.get(grant.agent_id) || grant.agent_id}

+
+ v{grant.pinned_version} + {grant.can_manage && ( + + {t("grants.canManage")} + + )} +
+
+ +
+ ))} +
+ )} +
+ +
+ + + + +
+ + {error &&

{error}

} +
+ + + ); +} diff --git a/ui/web/src/pages/skills/skill-table-row.tsx b/ui/web/src/pages/skills/skill-table-row.tsx index 71a16a46..97bc6bbb 100644 --- a/ui/web/src/pages/skills/skill-table-row.tsx +++ b/ui/web/src/pages/skills/skill-table-row.tsx @@ -1,5 +1,5 @@ import { useTranslation } from "react-i18next"; -import { Zap, Pencil, Trash2 } from "lucide-react"; +import { Zap, Pencil, Trash2, Users } from "lucide-react"; import { Button } from "@/components/ui/button"; import { Badge } from "@/components/ui/badge"; import { Switch } from "@/components/ui/switch"; @@ -20,6 +20,7 @@ interface SkillTableRowProps { toggling: string | null; onView: (name: string) => void; onEdit: (skill: SkillInfo) => void; + onManageGrants: (skill: SkillInfo) => void; onDelete: (skill: SkillInfo) => void; onToggle: (skill: SkillInfo, enabled: boolean) => void; onCycleVisibility: (skill: SkillInfo) => void; @@ -30,7 +31,7 @@ interface SkillTableRowProps { /** Single row in the skills table with inline status, visibility, and action controls. */ export function SkillTableRow({ skill, tab, hasTenantScope, toggling, - onView, onEdit, onDelete, onToggle, onCycleVisibility, + onView, onEdit, onManageGrants, onDelete, onToggle, onCycleVisibility, onSetTenantConfig, onDeleteTenantConfig, }: SkillTableRowProps) { const { t } = useTranslation("skills"); @@ -132,6 +133,11 @@ export function SkillTableRow({ + {!skill.is_system && ( + + )} {!skill.is_system && (