From 44451bacd9d78900673398b6b4a1bfb3320bb1da Mon Sep 17 00:00:00 2001 From: Ilker Manap Date: Wed, 28 Oct 2026 22:11:07 +0100 Subject: [PATCH] Granting or taking back a server's tools leaves the rest as it was A tool is seen where every level above that names tools names it (store Effective.AllowsTool). The console and /admin/mcp wrote a level's list as given, so a team that saw its organization's hr.* through having no list was given jira.* and then saw neither; and revoking a level's last server left [] in the store, closed to every tool, shown nowhere. store.GrantTools and RevokeTools now do both screens' work: a level's first list starts from the nearest list above it; taking a server away leaves the others, and a level left naming nothing goes back to no list, or stays closed when a level above names tools, so it opens to nothing it was kept from. GrantTools says which levels above still keep the granted tools out of reach, and both screens say so. The page shows closed levels and member scopes by name, and offers no revoke for a * grant it would not change. The console calls the store instead of keys limits with a list built from what it had loaded, which overwrote changes made since. Found in review. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 9 ++ docs/console.md | 3 +- docs/install.md | 14 ++- internal/cli/tui_mcp.go | 64 +++++++---- internal/store/toolgrants.go | 159 ++++++++++++++++++++++++++ internal/store/toolgrants_test.go | 100 ++++++++++++++++ internal/web/admin_mcp.go | 100 ++++++++++------ internal/web/admin_mcp_test.go | 18 +++ internal/web/locales/en/mcp.json | 10 +- internal/web/locales/tr/mcp.json | 10 +- internal/web/templates/admin_mcp.html | 6 +- 11 files changed, 422 insertions(+), 71 deletions(-) create mode 100644 internal/store/toolgrants.go create mode 100644 internal/store/toolgrants_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 4be7b8f..03c8596 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,15 @@ APIs change between minors without a deprecation period. redacted to the same text shared one answer — with the first caller's address put back in it. Such requests are now never answered from the cache nor kept in it. +- **Granting a server's tools could take others away; revoking could close + a level to every tool.** A level with no tool list sees what the levels + above grant; giving it its first list made it see only that list. And a + level left with an empty list was closed to every tool, which the screens + did not show. Now a first list starts from the nearest list above, taking + a server away leaves the rest or goes back to no list, the screens say + when a level above keeps granted tools out of reach and which levels are + closed, and the console reads and writes the level at once rather than + from what it loaded. - **Similar questions about different pictures got each other's answer.** A question with a picture, a sound or a file in it is reused only when it is the same request. diff --git a/docs/console.md b/docs/console.md index 7aed363..5d4daff 100644 --- a/docs/console.md +++ b/docs/console.md @@ -181,7 +181,8 @@ its name, which is its tools' prefix, its address, and a bearer token or another header, kept sealed in the store. On a row: **Test, and list its tools**, with its credential, as a call would; **Grant its tools to…** a team, project, organization, person or key — all of them, `jira.*`, or some -by name — beside whatever else that level is granted; **Revoke** a grant; +by name — beside whatever else that level is granted, and saying when a +level above keeps them out of reach; **Revoke** a grant; **Change…** its address, timeout or rate; **Replace its credential…**; **Turn off** (its tools are listed nowhere and every call refused) or on; **Remove…**. A server's tools are seen nowhere until granted. The screen diff --git a/docs/install.md b/docs/install.md index 94aa1f0..de8cf47 100644 --- a/docs/install.md +++ b/docs/install.md @@ -2606,10 +2606,20 @@ llmgw keys limits --scope team --id Acme/support --tools 'jira.*' - **Who may call what:** tools are named `server.tool` and granted as patterns (`jira.*`, `db.query`) at any level of the tenancy tree, as models are. - - A level that names any narrows what is above it. + - A level that names any narrows what is above it: a tool is seen where + every level above that names tools names it too. A team under an + organization that names `hr.*` sees no `jira.*` whatever the team is + given, until the organization names it as well. - Unlike models, **none granted anywhere is none**: a tool reaches into a company system, so it is granted, never assumed. - - The console's limits form has the same field. + - `keys limits --tools` writes a level's list whole, every server's + together; an empty list (`--tools ''`) closes the level to every tool. + The console's **MCP servers** screen and the `/admin/mcp` page grant + and take back one server's tools: a level given its first list starts + from the nearest list above it, so it keeps what it saw; one left + naming none goes back to naming none, or stays closed when a level + above names tools, so taking one server away opens it to no other. They + say when a level above keeps the granted tools out of reach. - **The server is told who calls:** `X-Llmgw-Key`, and `X-Llmgw-User` and `X-Llmgw-Project` where there are some. It is called with llmgw's credentials, never the caller's key. diff --git a/internal/cli/tui_mcp.go b/internal/cli/tui_mcp.go index a02439f..ee46e13 100644 --- a/internal/cli/tui_mcp.go +++ b/internal/cli/tui_mcp.go @@ -82,13 +82,13 @@ func (m consoleModel) fetchMCP() tea.Cmd { } msg.grants = []mcpGrant{} for _, l := range limits { - if len(l.AllowedTools) == 0 { + if l.AllowedTools == nil { continue } o := store.LimitOwner{Scope: l.Scope, ID: l.ScopeID} label := msg.where[o.CounterKey()] if label == "" { - label = string(l.Scope) + " " + l.ScopeID + label = l.ScopeID } msg.grants = append(msg.grants, mcpGrant{owner: o, label: string(l.Scope) + " " + label, tools: l.AllowedTools}) } @@ -100,7 +100,10 @@ func (m consoleModel) fetchMCP() tea.Cmd { func (s *consoleMCPMsg) name(scope store.Scope, id, label string) { o := store.LimitOwner{Scope: scope, ID: id} s.where[o.CounterKey()] = label - s.ids[string(scope)+":"+label] = o + s.ids[string(scope)+":"+strings.ToLower(label)] = o + if m := store.MemberScope(scope); m != "" { + s.where[store.LimitOwner{Scope: m, ID: id}.CounterKey()] = label + } } // mcpGrantsOf is who may use a server's tools, and which of them. @@ -278,8 +281,8 @@ func (m consoleModel) mcpMenu(s adminapi.MCPServerView) *menuDialog { items = append(items, menuItem{"Revoke from " + g.label + " (" + strings.Join(g.tools, ", ") + ")…", func(m *consoleModel) tea.Cmd { m.confirmD = &confirmDialog{title: "Revoke " + s.Name + " from " + g.label, - text: "It and everything below it then no longer see " + strings.Join(g.tools, ", ") + - ",\nunless another level grants them.", + text: "It then sees " + strings.Join(g.tools, ", ") + " only if the levels above grant them\n" + + "and it names no tools of its own. Its other grants stay.", yes: func(m *consoleModel) tea.Cmd { return m.mcpSetTools(s.Name, g.owner, nil) }} return nil }}) @@ -397,8 +400,8 @@ func (m *consoleModel) mcpGrantForm(s adminapi.MCPServerView) { return } m.openForm(&formDialog{title: "Grant " + s.Name + "'s tools", - note: "It and everything below it then see them, unless a level below grants\n" + - "tools and not these. Other grants at that level stay.", + note: "Seen there, and below, where every level above that names tools names these too.\n" + + "A level given its first grant keeps what it saw from above; other grants stay.", fields: []formField{ choiceF("to", []string{"team", "project", "org", "user", "key"}, "team"), textF("which", "", "ORG/TEAM/PROJECT, ORG/TEAM, ORG, an e-mail, or a key's ID"), @@ -409,7 +412,7 @@ func (m *consoleModel) mcpGrantForm(s adminapi.MCPServerView) { if which == "" { return nil, fmt.Errorf("which %s?", v[0]) } - owner, ok := m.mcp.ids[v[0]+":"+which] + owner, ok := m.mcp.ids[v[0]+":"+strings.ToLower(which)] if v[0] == "key" { owner, ok = store.LimitOwner{Scope: store.ScopeKey, ID: which}, true } @@ -432,25 +435,38 @@ func (m *consoleModel) mcpGrantForm(s adminapi.MCPServerView) { }}) } -// mcpSetTools makes a level's grants of a server's tools these, leaving its -// grants of other servers' as they are. None revokes them. +// mcpSetTools grants a level these of a server's tools, or with none takes +// them away, in the store (store.GrantTools, store.RevokeTools): read and +// written at once, so another administrator's change since this screen +// was loaded is kept. func (m *consoleModel) mcpSetTools(server string, owner store.LimitOwner, tools []string) tea.Cmd { - var keep []string - for _, g := range m.mcp.grants { - if g.owner != owner { - continue - } - for _, t := range g.tools { - if !strings.HasPrefix(t, server+".") { - keep = append(keep, t) - } - } - } - all := append(keep, tools...) + h, where := m.st, m.mcp.where title := "Grant " + server if len(tools) == 0 { title = "Revoke " + server } - return m.runCLI(title, "keys", "limits", "--scope="+string(owner.Scope), "--id="+owner.ID, - "--tools="+strings.Join(all, ",")) + return func() tea.Msg { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + s, err := h.get(ctx) + if err != nil { + return actionDoneMsg{title: title, err: err} + } + if len(tools) == 0 { + if err := s.RevokeTools(ctx, owner.Scope, owner.ID, server); err != nil { + return actionDoneMsg{title: title, err: err} + } + return actionDoneMsg{title: title, text: server + "'s tools taken back there; its other grants stay."} + } + g, err := s.GrantTools(ctx, owner.Scope, owner.ID, server, tools) + if err != nil { + return actionDoneMsg{title: title, err: err} + } + text := "Granted " + strings.Join(tools, ", ") + "." + for _, b := range g.BlockedBy { + text += "\n\nBut " + string(b.Scope) + " " + or(where[b.CounterKey()], b.ID) + " names tools and not these:\n" + + server + "'s stay out of reach there until it grants them too." + } + return actionDoneMsg{title: title, text: text} + } } diff --git a/internal/store/toolgrants.go b/internal/store/toolgrants.go new file mode 100644 index 0000000..68cf803 --- /dev/null +++ b/internal/store/toolgrants.go @@ -0,0 +1,159 @@ +package store + +import ( + "context" + "errors" + "slices" + "strings" +) + +// Granting one MCP server's tools at a level of the tree, and taking them +// back, as the console and the admin page do. A tool is usable only where +// every level above that names tools names it too (Effective.AllowsTool), +// which makes adding a list where there was none a narrowing: a team that +// saw its organization's jira.* through having no list of its own would see +// only hr.* once given hr.*. So a level given its first list starts from +// the nearest list above it, and keeps what it had. + +// ToolGrant is what granting came to: the levels above whose lists do not +// name the tools granted, so that they stay out of reach until those grant +// them too. +type ToolGrant struct { + BlockedBy []LimitOwner +} + +// GrantTools makes a level's grants of server's tools these, keeping its +// grants of other servers' and the rest of its limits. +func (s *DB) GrantTools(ctx context.Context, scope Scope, id, server string, tools []string) (ToolGrant, error) { + above, err := s.limitsAbove(ctx, scope, id) + if err != nil { + return ToolGrant{}, err + } + lim, err := s.LimitsFor(ctx, scope, id) + switch { + case errors.Is(err, ErrNotFound): + lim = Limits{Scope: scope, ScopeID: id} + case err != nil: + return ToolGrant{}, err + } + if lim.AllowedTools == nil { + // Its first list: what it saw through the levels above, kept. + for _, a := range above { + if a.AllowedTools != nil { + lim.AllowedTools = slices.Clone(a.AllowedTools) + break + } + } + } + keep := withoutServer(lim.AllowedTools, server) + for _, t := range tools { + if !slices.Contains(keep, t) { + keep = append(keep, t) + } + } + lim.AllowedTools = keep + if err := s.PutLimits(ctx, lim); err != nil { + return ToolGrant{}, err + } + var g ToolGrant + for _, a := range above { + if a.AllowedTools == nil { + continue + } + for _, t := range tools { + if !matchesTool(a.AllowedTools, sampleTool(t)) { + g.BlockedBy = append(g.BlockedBy, LimitOwner{Scope: a.Scope, ID: a.ScopeID}) + break + } + } + } + return g, nil +} + +// RevokeTools takes a level's grants of server's tools away, keeping the +// rest. A level left naming no tools no longer names any — it then sees +// what the levels above let it — unless a level above names tools: then +// it is left naming none, closed to every tool, so that taking one +// server's tools away does not open it to the others above. +func (s *DB) RevokeTools(ctx context.Context, scope Scope, id, server string) error { + lim, err := s.LimitsFor(ctx, scope, id) + if errors.Is(err, ErrNotFound) { + return nil + } + if err != nil { + return err + } + lim.AllowedTools = withoutServer(lim.AllowedTools, server) + if len(lim.AllowedTools) == 0 { + above, err := s.limitsAbove(ctx, scope, id) + if err != nil { + return err + } + lim.AllowedTools = nil + for _, a := range above { + if a.AllowedTools != nil { + lim.AllowedTools = []string{} + break + } + } + } + return s.PutLimits(ctx, lim) +} + +func withoutServer(tools []string, server string) []string { + return slices.DeleteFunc(slices.Clone(tools), func(t string) bool { return strings.HasPrefix(t, server+".") }) +} + +// sampleTool is a tool a pattern grants, for asking whether a list above +// grants it too: jira.* stands for any of jira's. +func sampleTool(pattern string) string { + if before, ok := strings.CutSuffix(pattern, "*"); ok { + return before + "x" + } + return pattern +} + +// limitsAbove are the limits of the levels above one, nearest first: a +// project's team and organization, a team's organization, a key's project +// and those above it. None for an organization or a person. +func (s *DB) limitsAbove(ctx context.Context, scope Scope, id string) ([]Limits, error) { + var owners []LimitOwner + if scope == ScopeKey { + k, err := s.KeyByID(ctx, id) + if err != nil { + return nil, nil + } + scope, id = ScopeProject, k.ProjectID + owners = append(owners, LimitOwner{Scope: ScopeProject, ID: id}) + } + if scope == ScopeTeam || scope == ScopeProject { + tree, err := s.Tree(ctx) + if err != nil { + return nil, err + } + for _, o := range tree { + for _, t := range o.Teams { + if scope == ScopeTeam && t.ID == id { + owners = append(owners, LimitOwner{Scope: ScopeOrg, ID: o.ID}) + } + for _, p := range t.Projects { + if scope == ScopeProject && p.ID == id { + owners = append(owners, LimitOwner{Scope: ScopeTeam, ID: t.ID}, LimitOwner{Scope: ScopeOrg, ID: o.ID}) + } + } + } + } + } + var out []Limits + for _, o := range owners { + l, err := s.LimitsFor(ctx, o.Scope, o.ID) + if errors.Is(err, ErrNotFound) { + continue + } + if err != nil { + return nil, err + } + out = append(out, l) + } + return out, nil +} diff --git a/internal/store/toolgrants_test.go b/internal/store/toolgrants_test.go new file mode 100644 index 0000000..d6dee0d --- /dev/null +++ b/internal/store/toolgrants_test.go @@ -0,0 +1,100 @@ +package store + +import ( + "context" + "path/filepath" + "slices" + "testing" +) + +func TestGrantingToolsAtALevelKeepsWhatItSawFromAbove(t *testing.T) { + ctx := context.Background() + s, err := OpenSQLite(ctx, filepath.Join(t.TempDir(), "s.db")) + if err != nil { + t.Fatal(err) + } + defer s.Close() + org, _ := s.CreateOrg(ctx, "Acme") + team, _ := s.CreateTeam(ctx, org, "eng") + tools := func(scope Scope, id string) []string { + l, _ := s.LimitsFor(ctx, scope, id) + return l.AllowedTools + } + sees := func(tool string) bool { + e := Effective{} + for _, l := range []struct { + sc Scope + id string + }{{ScopeOrg, org}, {ScopeTeam, team}} { + if ts := tools(l.sc, l.id); ts != nil { + e.ToolGrants = append(e.ToolGrants, ts) + } + } + return e.AllowsTool(tool) + } + + // The organization grants hr's tools; the team names none and sees them. + if _, err := s.GrantTools(ctx, ScopeOrg, org, "hr", []string{"hr.*"}); err != nil { + t.Fatal(err) + } + if !sees("hr.search") { + t.Fatal("the team does not see what its organization grants") + } + // Granting jira's to the team keeps hr's, and says the organization + // still keeps jira's out of reach. + g, err := s.GrantTools(ctx, ScopeTeam, team, "jira", []string{"jira.*"}) + if err != nil { + t.Fatal(err) + } + if !sees("hr.search") { + t.Errorf("granting jira's tools to the team took hr's away: %v", tools(ScopeTeam, team)) + } + if len(g.BlockedBy) != 1 || g.BlockedBy[0].ID != org { + t.Errorf("the organization, which does not grant jira's tools, should be named: %+v", g) + } + if sees("jira.search") { + t.Error("the team sees jira's tools its organization does not grant") + } + // The organization grants them too: now the team sees both. + _, _ = s.GrantTools(ctx, ScopeOrg, org, "jira", []string{"jira.*"}) + if !sees("jira.search") || !sees("hr.get") { + t.Errorf("org %v team %v", tools(ScopeOrg, org), tools(ScopeTeam, team)) + } + // Taking jira's back from the team leaves it hr's. + if err := s.RevokeTools(ctx, ScopeTeam, team, "jira"); err != nil { + t.Fatal(err) + } + if sees("jira.search") || !sees("hr.get") { + t.Errorf("after the revoke: team %v", tools(ScopeTeam, team)) + } +} + +func TestRevokingALevelsLastToolsNeitherClosesNorOpensIt(t *testing.T) { + ctx := context.Background() + s, err := OpenSQLite(ctx, filepath.Join(t.TempDir(), "s.db")) + if err != nil { + t.Fatal(err) + } + defer s.Close() + org, _ := s.CreateOrg(ctx, "Acme") + team, _ := s.CreateTeam(ctx, org, "eng") + + // Nothing above names tools: the team's last grant taken leaves it + // naming none at all, not closed to every tool. + _, _ = s.GrantTools(ctx, ScopeTeam, team, "jira", []string{"jira.*"}) + _ = s.RevokeTools(ctx, ScopeTeam, team, "jira") + if l, _ := s.LimitsFor(ctx, ScopeTeam, team); l.AllowedTools != nil { + t.Errorf("left %v; want no list", l.AllowedTools) + } + // The organization grants every tool; the team was kept to jira's. + // Taking jira's away must not open it to everything above. + _ = s.PutLimits(ctx, Limits{Scope: ScopeOrg, ScopeID: org, AllowedTools: []string{"*"}}) + _ = s.PutLimits(ctx, Limits{Scope: ScopeTeam, ScopeID: team, AllowedTools: []string{"jira.*"}}) + _ = s.RevokeTools(ctx, ScopeTeam, team, "jira") + if l, _ := s.LimitsFor(ctx, ScopeTeam, team); l.AllowedTools == nil || len(l.AllowedTools) != 0 { + t.Errorf("left %v; want an empty list, closed", l.AllowedTools) + } + if !slices.Equal(withoutServer([]string{"a.x", "b.*", "a.*"}, "a"), []string{"b.*"}) { + t.Error("withoutServer") + } +} diff --git a/internal/web/admin_mcp.go b/internal/web/admin_mcp.go index c111d65..9ae5574 100644 --- a/internal/web/admin_mcp.go +++ b/internal/web/admin_mcp.go @@ -21,7 +21,8 @@ import ( func init() { // Named from the grant's scope in the template. - registerKeys("mcp.scope.org", "mcp.scope.team", "mcp.scope.project", "mcp.scope.user", "mcp.scope.key") + registerKeys("mcp.scope.org", "mcp.scope.team", "mcp.scope.project", "mcp.scope.user", "mcp.scope.key", + "mcp.scope.org-member", "mcp.scope.team-member", "mcp.scope.project-member") } // MCPAdmin is where the servers, their sealed credentials and the tree's @@ -31,6 +32,8 @@ type MCPAdmin interface { AllLimits(ctx context.Context) (map[string]store.Limits, error) LimitsFor(ctx context.Context, scope store.Scope, id string) (store.Limits, error) PutLimits(ctx context.Context, l store.Limits) error + GrantTools(ctx context.Context, scope store.Scope, id, server string, tools []string) (store.ToolGrant, error) + RevokeTools(ctx context.Context, scope store.Scope, id, server string) error Tree(ctx context.Context) ([]store.TreeOrg, error) Users(ctx context.Context, state store.UserState) ([]store.User, error) } @@ -46,6 +49,9 @@ type mcpPageData struct { // Tools are what a test found, for the server Tested. Tested string Tools []string + // Closed are the levels that name no tool at all, and so see none + // whatever is granted above them. + Closed []mcpGrantRow Notice string Error string } @@ -66,6 +72,9 @@ type mcpRow struct { type mcpGrantRow struct { Scope, ID, Label string Tools []string + // Revocable is false for a grant of every tool, "*", which taking this + // server's away would not change: it is changed with the level's limits. + Revocable bool } // RegisterAdminMCP mounts /admin/mcp, for administrators only: a server's @@ -81,6 +90,11 @@ func (p *AccountPages) RegisterAdminMCP(mux *http.ServeMux, s MCPAdmin, local fu d.Error = l.Err(err) } grants := mcpGrants(ctx, s) + for _, g := range grants { + if len(g.Tools) == 0 { + d.Closed = append(d.Closed, g) + } + } for _, m := range list { row := mcpRow{MCPServerView: adminapi.MCPServerView{Name: m.Name, URL: m.URL, Description: m.Description, Enabled: m.Enabled, TimeoutSeconds: int(m.Timeout.Seconds()), RatePerMinute: m.RatePerMinute}, @@ -95,13 +109,16 @@ func (p *AccountPages) RegisterAdminMCP(mux *http.ServeMux, s MCPAdmin, local fu slices.Sort(row.Plain) for _, g := range grants { var mine []string + revocable := false for _, t := range g.Tools { if strings.HasPrefix(t, m.Name+".") || t == "*" { mine = append(mine, t) + revocable = revocable || t != "*" } } if len(mine) > 0 { - row.Grants = append(row.Grants, mcpGrantRow{Scope: g.Scope, ID: g.ID, Label: g.Label, Tools: mine}) + row.Grants = append(row.Grants, mcpGrantRow{Scope: g.Scope, ID: g.ID, Label: g.Label, Tools: mine, + Revocable: revocable}) } } d.Servers = append(d.Servers, row) @@ -254,16 +271,25 @@ func (p *AccountPages) RegisterAdminMCP(mux *http.ServeMux, s MCPAdmin, local fu if err == nil { id, label, err = mcpResolve(r.Context(), s, l, scope, which) } + notice := l.T("mcp.granted", name, label) if err == nil { - err = mcpSetTools(r.Context(), s, name, scope, id, tools) + var g store.ToolGrant + if g, err = s.GrantTools(r.Context(), scope, id, name, tools); err == nil && len(g.BlockedBy) > 0 { + names := mcpNames(r.Context(), s) + var by []string + for _, o := range g.BlockedBy { + by = append(by, l.T("mcp.scope."+string(o.Scope))+" "+or(names[o.CounterKey()], o.ID)) + } + notice += " " + l.T("mcp.blocked_above", strings.Join(by, ", "), name) + } } - done(w, r, admin, "mcp grants "+name, l.T("mcp.granted", name, label), err) + done(w, r, admin, "mcp grants "+name, notice, err) })) mux.HandleFunc("POST /admin/mcp/{name}/revoke", adminsOnly(func(w http.ResponseWriter, r *http.Request, admin store.User) { l, name := langOf(r), r.PathValue("name") scope, id := store.Scope(r.FormValue("scope")), r.FormValue("id") - err := mcpSetTools(r.Context(), s, name, scope, id, nil) + err := s.RevokeTools(r.Context(), scope, id, name) done(w, r, admin, "mcp grants "+name, l.T("mcp.revoked", name), err) })) } @@ -275,26 +301,10 @@ func mcpGrants(ctx context.Context, s MCPAdmin) []mcpGrantRow { if err != nil { return nil } - names := map[string]string{} - if tree, err := s.Tree(ctx); err == nil { - for _, o := range tree { - names[store.LimitOwner{Scope: store.ScopeOrg, ID: o.ID}.CounterKey()] = o.Name - for _, t := range o.Teams { - names[store.LimitOwner{Scope: store.ScopeTeam, ID: t.ID}.CounterKey()] = o.Name + "/" + t.Name - for _, pr := range t.Projects { - names[store.LimitOwner{Scope: store.ScopeProject, ID: pr.ID}.CounterKey()] = o.Name + "/" + t.Name + "/" + pr.Name - } - } - } - } - if users, err := s.Users(ctx, ""); err == nil { - for _, u := range users { - names[store.LimitOwner{Scope: store.ScopeUser, ID: u.ID}.CounterKey()] = u.Email - } - } + names := mcpNames(ctx, s) var out []mcpGrantRow for key, lim := range limits { - if len(lim.AllowedTools) == 0 { + if lim.AllowedTools == nil { continue } label := names[key] @@ -307,6 +317,35 @@ func mcpGrants(ctx context.Context, s MCPAdmin) []mcpGrantRow { return out } +// mcpNames are the tree's levels and the people by what they are called, +// keyed as limits are; a member scope is named as the level it is set on. +func mcpNames(ctx context.Context, s MCPAdmin) map[string]string { + names := map[string]string{} + name := func(scope store.Scope, id, label string) { + names[store.LimitOwner{Scope: scope, ID: id}.CounterKey()] = label + if m := store.MemberScope(scope); m != "" { + names[store.LimitOwner{Scope: m, ID: id}.CounterKey()] = label + } + } + if tree, err := s.Tree(ctx); err == nil { + for _, o := range tree { + name(store.ScopeOrg, o.ID, o.Name) + for _, t := range o.Teams { + name(store.ScopeTeam, t.ID, o.Name+"/"+t.Name) + for _, pr := range t.Projects { + name(store.ScopeProject, pr.ID, o.Name+"/"+t.Name+"/"+pr.Name) + } + } + } + } + if users, err := s.Users(ctx, ""); err == nil { + for _, u := range users { + name(store.ScopeUser, u.ID, u.Email) + } + } + return names +} + // mcpResolve is the ID of the level a form names: a path in the tree, a // person's e-mail, or a key's ID; and how to say it. func mcpResolve(ctx context.Context, s MCPAdmin, l *Lang, scope store.Scope, which string) (string, string, error) { @@ -369,21 +408,6 @@ func mcpCheckTools(l *Lang, server string, tools []string) error { return nil } -// mcpSetTools makes a level's grants of a server's tools these, leaving its -// other grants and its other limits as they are; none revokes them. -func mcpSetTools(ctx context.Context, s MCPAdmin, server string, scope store.Scope, id string, tools []string) error { - lim, err := s.LimitsFor(ctx, scope, id) - switch { - case errors.Is(err, store.ErrNotFound): - lim = store.Limits{Scope: scope, ScopeID: id} - case err != nil: - return err - } - keep := slices.DeleteFunc(slices.Clone(lim.AllowedTools), func(t string) bool { return strings.HasPrefix(t, server+".") }) - lim.AllowedTools = append(keep, tools...) - return s.PutLimits(ctx, lim) -} - // mcpSecrets turns the form's credential into what the server is sent. func mcpSecrets(l *Lang, kind, header, value string) (map[string]string, error) { header, value = strings.TrimSpace(header), strings.TrimSpace(value) diff --git a/internal/web/admin_mcp_test.go b/internal/web/admin_mcp_test.go index bf024af..0989b91 100644 --- a/internal/web/admin_mcp_test.go +++ b/internal/web/admin_mcp_test.go @@ -165,3 +165,21 @@ func TestTheMCPPageSpeaksTurkishWhenAsked(t *testing.T) { } } } + +func TestAGrantBelowALevelThatNamesOtherToolsSaysSoAndKeepsWhatItHad(t *testing.T) { + mux, st, _ := mcpPage(t, mcpAdmin) + ctx := context.Background() + org, _ := st.CreateOrg(ctx, "Acme") + team, _ := st.CreateTeam(ctx, org, "eng") + _ = st.PutLimits(ctx, store.Limits{Scope: store.ScopeOrg, ScopeID: org, AllowedTools: []string{"hr.*"}}) + _ = st.PutMCPServer(ctx, store.MCPServer{Name: "jira", URL: "https://jira/mcp", Enabled: true}) + + body := mirrorPost(t, mux, "/admin/mcp/jira/grant", url.Values{"csrf": {"tok"}, "scope": {"team"}, "which": {"Acme/eng"}, + "tools": {"jira.*"}}).Body.String() + if !strings.Contains(body, "But organization Acme names tools and not these") { + t.Errorf("the grant should say the organization keeps jira out of reach: %s", body) + } + if l, _ := st.LimitsFor(ctx, store.ScopeTeam, team); !slices.Equal(l.AllowedTools, []string{"hr.*", "jira.*"}) { + t.Errorf("the team's list is %v; it should keep hr's, which it saw from its organization", l.AllowedTools) + } +} diff --git a/internal/web/locales/en/mcp.json b/internal/web/locales/en/mcp.json index 1d7a754..d8dae0d 100644 --- a/internal/web/locales/en/mcp.json +++ b/internal/web/locales/en/mcp.json @@ -30,7 +30,7 @@ "mcp.which_hint": "ORG/TEAM, ORG/TEAM/PROJECT, ORG, an e-mail or a key's ID", "mcp.tools": "Tools", "mcp.grant_button": "Grant", - "mcp.confirm_revoke": "Revoke %s from %s? It and everything below it then no longer see those tools, unless another level grants them.", + "mcp.confirm_revoke": "Revoke %s from %s? It then sees them only if the levels above grant them and it names no tools of its own.", "mcp.revoke": "Revoke from %s", "mcp.change": "Change", "mcp.address": "Address", @@ -78,5 +78,11 @@ "mcp.err.header": "Which header carries the credential?", "mcp.err.auth": "The credential is a bearer token, another header or none.", "mcp.err.timeout": "The timeout is whole seconds.", - "mcp.err.rate": "The rate is calls a minute." + "mcp.err.rate": "The rate is calls a minute.", + "mcp.scope.org-member": "each member of the organization", + "mcp.scope.team-member": "each member of the team", + "mcp.scope.project-member": "each member of the project", + "mcp.blocked_above": "But %s names tools and not these: %s's tools stay out of reach here until it grants them too.", + "mcp.closed": "Closed to every tool, whatever is granted above:", + "mcp.how_grants": "A tool is seen where it is granted and every level above that names tools names it too. A level given its first grant keeps what it saw from above; taking a server's tools away leaves the rest." } diff --git a/internal/web/locales/tr/mcp.json b/internal/web/locales/tr/mcp.json index f9b3b89..cb491e5 100644 --- a/internal/web/locales/tr/mcp.json +++ b/internal/web/locales/tr/mcp.json @@ -30,7 +30,7 @@ "mcp.which_hint": "ORG/TAKIM, ORG/TAKIM/PROJE, ORG, bir e-posta ya da anahtarın kimliği", "mcp.tools": "Araçlar", "mcp.grant_button": "Yetki ver", - "mcp.confirm_revoke": "%s yetkisi %s üzerinden geri alınsın mı? Başka bir seviye vermedikçe orası ve altındakiler bu araçları artık görmez.", + "mcp.confirm_revoke": "%s yetkisi %s üzerinden geri alınsın mı? Bundan sonra onları yalnızca üst seviyeler veriyorsa ve kendi araç listesi yoksa görür.", "mcp.revoke": "%s üzerinden geri al", "mcp.change": "Değiştir", "mcp.address": "Adres", @@ -77,5 +77,11 @@ "mcp.err.header": "Kimlik bilgisini hangi başlık taşıyor?", "mcp.err.auth": "Kimlik bilgisi bir bearer token, başka bir başlık ya da hiçbiri olabilir.", "mcp.err.timeout": "Zaman aşımı tam saniye olmalı.", - "mcp.err.rate": "Hız dakikadaki çağrı sayısıdır." + "mcp.err.rate": "Hız dakikadaki çağrı sayısıdır.", + "mcp.scope.org-member": "organizasyonun her üyesi", + "mcp.scope.team-member": "takımın her üyesi", + "mcp.scope.project-member": "projenin her üyesi", + "mcp.blocked_above": "Ama %s araç listesi tanımlıyor ve bunlar onda yok: o da yetki verene kadar %s araçları burada erişilemez.", + "mcp.closed": "Yukarıda ne verilirse verilsin tüm araçlara kapalı:", + "mcp.how_grants": "Bir araç, yetki verildiği yerde ve araç listesi tanımlayan her üst seviye de onu tanımlıyorsa görünür. İlk kez yetki verilen bir seviye yukarıdan gördüklerini korur; bir sunucunun yetkisini almak diğerlerini bırakır." } diff --git a/internal/web/templates/admin_mcp.html b/internal/web/templates/admin_mcp.html index 0028194..3d19c29 100644 --- a/internal/web/templates/admin_mcp.html +++ b/internal/web/templates/admin_mcp.html @@ -51,11 +51,11 @@ {{$name := .Name}} - {{range .Grants}} + {{range .Grants}}{{if .Revocable}}
- {{end}} + {{end}}{{end}} {{if not .Local}}

{{t "mcp.change"}}

@@ -81,6 +81,8 @@ {{end}} {{else}}

{{t "mcp.none"}}

{{end}} +{{if .Closed}}

{{t "mcp.closed"}} {{range $i, $c := .Closed}}{{if $i}}, {{end}}{{t (printf "mcp.scope.%s" $c.Scope)}} {{$c.Label}}{{end}}.

{{end}} +

{{t "mcp.how_grants"}}

{{t "mcp.add"}}