From 1948a787ae20a8489305957c937ec79a00441ed8 Mon Sep 17 00:00:00 2001 From: Ilker Manap Date: Thu, 29 Oct 2026 12:31:07 +0100 Subject: [PATCH] Fixes from the second review: grants, flags, updates, setup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Revoking a server's tools takes them from every level beneath too, and a level left naming no tool stays closed ([]) while any level above or beneath names tools: an organization's revoke left its teams the copies their first grants took, and going back to no list opened the level to what was above it. A closed level is shown on the page and in the console with an action to open it (store.OpenTools). A key's owner's list is among the levels above a key. - The flag check matches short flags as written again (-m, -hf, -mu were let through), takes a long flag's beginning for it only for the engines that parse with argparse (vLLM, SGLang, ComfyUI: --li is --listen), so llama.cpp's own --chat-template is not refused, and refuses llama.cpp's short spellings (-hfr, -hff, -mm, -jf, …). - A stored model with an adapter by Hugging Face id can be changed again: what it already had is not asked about anew. - The catalog reads a Hugging Face id as the agent does (engines.IsHubID). - The page's form cannot move llmgw:admin to another address. - server init skips the memory check only where the gateway already runs, not wherever a half-finished first run left server.yaml. Co-Authored-By: Claude Opus 5.5 --- internal/agent/engines/pickle_test.go | 14 +++ internal/agent/engines/unsafeenv.go | 36 +++++-- internal/cli/serverinit.go | 10 +- internal/cli/serverinit_choices.go | 20 +++- internal/cli/serverinit_memory_test.go | 8 +- internal/cli/tui_mcp.go | 51 +++++++++- internal/control/catalog/pickle.go | 16 ++-- internal/control/catalog/pickle_test.go | 3 +- internal/control/modelsvc/modelsvc.go | 2 +- internal/store/toolgrants.go | 120 ++++++++++++++++++++++-- internal/store/toolgrants_test.go | 49 ++++++++++ internal/web/admin_mcp.go | 13 ++- internal/web/locales/en/mcp.json | 9 +- internal/web/locales/tr/mcp.json | 9 +- internal/web/templates/admin_mcp.html | 6 +- 15 files changed, 322 insertions(+), 44 deletions(-) diff --git a/internal/agent/engines/pickle_test.go b/internal/agent/engines/pickle_test.go index 2a7c905..69e45d5 100644 --- a/internal/agent/engines/pickle_test.go +++ b/internal/agent/engines/pickle_test.go @@ -41,4 +41,18 @@ func TestAForbiddenFlagIsKnownHoweverItIsSpelled(t *testing.T) { t.Errorf("vllm %v was refused: %v", a, err) } } + // Short flags are matched as written; llama.cpp takes no abbreviations, + // so its --chat-template is not taken for --chat-template-file. + for _, a := range [][]string{{"-m", "x"}, {"-hf", "o/r"}, {"-mu", "http://x"}} { + if UnsafeArgs("llamacpp", a) == nil { + t.Errorf("llamacpp %v was let through", a) + } + } + if err := UnsafeArgs("llamacpp", []string{"--chat-template", "chatml"}); err != nil { + t.Errorf("llama.cpp's own --chat-template was refused: %v", err) + } + // ComfyUI's argparse reads --li as --listen. + if UnsafeArgs("comfyui", []string{"--li", "0.0.0.0"}) == nil { + t.Error("comfyui --li was let through") + } } diff --git a/internal/agent/engines/unsafeenv.go b/internal/agent/engines/unsafeenv.go index 49079f1..157d1d1 100644 --- a/internal/agent/engines/unsafeenv.go +++ b/internal/agent/engines/unsafeenv.go @@ -102,6 +102,13 @@ var flagsNever = map[string]map[string]string{ "--hf-token": "is a credential", "--alias": "is set by the agent", "--model-draft": "loads a file", "-md": "loads a file", "--prompt-cache": "writes a file", "--logdir": "writes files", + // The short spellings of the same. + "-mm": "loads a file; the projector is fetched with the model", "--mmproj-url": "downloads a file", + "-mmu": "downloads a file", "-hfr": "downloads a model", "-hff": "downloads a model", + "--hf-repo-draft": "downloads a model", "-hfd": "downloads a model", "-hfrd": "downloads a model", + "--hf-repo-v": "downloads a model", "-hfv": "downloads a model", "--hf-file-v": "downloads a model", + "-hffv": "downloads a model", "-hft": "is a credential", "-jf": "reads a file", "-a": "is set by the agent", + "--model-vocoder": "loads a file", "-mv": "loads a file", }, "vllm": { "--download-dir": "writes files", "--tokenizer": "reads another repository or path", @@ -165,21 +172,32 @@ var flagsNever = map[string]map[string]string{ // unsafeFlag is why an engine may not be given a flag, or "". func unsafeFlag(engine, flag, _ string) string { name, _, _ := strings.Cut(flag, "=") - if !strings.HasPrefix(name, "--") { + if !strings.HasPrefix(name, "-") { return "" } // Written as the engine would read it: vLLM takes --load_format for - // --load-format, TensorRT-LLM the other way round, and argparse takes - // any unambiguous beginning of a flag for the flag — --trust-remote is - // --trust-remote-code. - dashed := "--" + strings.ReplaceAll(name[2:], "_", "-") + // --load-format, TensorRT-LLM the other way round. Short flags (-m, + // -hf) are matched as written. + norm := func(f string) string { + if strings.HasPrefix(f, "--") { + return "--" + strings.ReplaceAll(f[2:], "_", "-") + } + return f + } + given := norm(name) + // argparse also takes any unambiguous beginning of a long flag for the + // flag — --trust-remote is --trust-remote-code, --li is ComfyUI's + // --listen — so for the engines that parse with it, a beginning of a + // refused flag is refused. llama.cpp and TensorRT-LLM take no such + // thing, and --chat-template there is a flag of its own. + abbreviates := argparseEngines[engine] && strings.HasPrefix(given, "--") && len(given) >= 4 for _, set := range []string{"", engine} { for never, why := range flagsNever[set] { if why == "" { continue } - n := "--" + strings.ReplaceAll(never[2:], "_", "-") - if n == dashed || (len(dashed) > 4 && strings.HasPrefix(n, dashed)) { + n := norm(never) + if n == given || (abbreviates && strings.HasPrefix(n, "--") && strings.HasPrefix(n, given)) { return why } } @@ -187,6 +205,10 @@ func unsafeFlag(engine, flag, _ string) string { return "" } +// argparseEngines read their flags with Python's argparse, which takes a +// long flag's beginning for the flag. +var argparseEngines = map[string]bool{"vllm": true, "sglang": true, "comfyui": true} + // UnsafeArgs refuses the first of a profile's arguments an engine may not be // given, saying why. func UnsafeArgs(engine string, args []string) error { diff --git a/internal/cli/serverinit.go b/internal/cli/serverinit.go index fceb1b9..b68be51 100644 --- a/internal/cli/serverinit.go +++ b/internal/cli/serverinit.go @@ -57,10 +57,12 @@ WantedBy=multi-user.target // it again finishes an install that stopped halfway and changes nothing on // one that did not. type serverInit struct { - etcDir string // /etc/llmgw - stateDir string // /var/lib/llmgw - pkiDir string // where the authority lives - unitPath string // "" leaves systemd alone + // serviceActive says whether llmgw-server runs; nil asks systemctl. + serviceActive func() bool + etcDir string // /etc/llmgw + stateDir string // /var/lib/llmgw + pkiDir string // where the authority lives + unitPath string // "" leaves systemd alone // user owns the state and reads the key; "" means no user is created and // nothing is chowned, which is what a test wants. diff --git a/internal/cli/serverinit_choices.go b/internal/cli/serverinit_choices.go index 214f140..464588e 100644 --- a/internal/cli/serverinit_choices.go +++ b/internal/cli/serverinit_choices.go @@ -5,6 +5,7 @@ import ( "math" "net" "os" + "os/exec" "path" "slices" "strconv" @@ -195,9 +196,10 @@ func (in *serverInit) checkMemory() error { if in.unitPath == "" { return nil // a development run, served by nothing } - // Run again on a machine already set up — to change --host, say, as - // the docs advise — it installs no models it does not already serve. - if _, err := os.Stat(in.configPath()); err == nil { + // Run again on a machine already serving — to change --host, say, as + // the docs advise — it installs no models it does not already serve. A + // first run that stopped half way is checked again: it still would. + if in.alreadyServing() { return nil } meminfo, err := os.ReadFile("/proc/meminfo") @@ -478,3 +480,15 @@ func shellWord(s string) string { } return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" } + +// alreadyServing reports whether this machine's gateway is set up and +// running: a server.yaml, and its unit active. +func (in *serverInit) alreadyServing() bool { + if _, err := os.Stat(in.configPath()); err != nil { + return false + } + if in.serviceActive != nil { + return in.serviceActive() + } + return exec.Command("systemctl", "is-active", "--quiet", "llmgw-server").Run() == nil +} diff --git a/internal/cli/serverinit_memory_test.go b/internal/cli/serverinit_memory_test.go index 298cfc3..a694964 100644 --- a/internal/cli/serverinit_memory_test.go +++ b/internal/cli/serverinit_memory_test.go @@ -39,8 +39,14 @@ func TestSetupRunAgainOnAMachineAlreadySetUpIsNotRefusedItsMemory(t *testing.T) if err := os.WriteFile(cfg, []byte("listen: :8080\n"), 0o600); err != nil { t.Fatal(err) } - in := &serverInit{unitPath: dir + "/llmgw-server.service", etcDir: dir} + in := &serverInit{unitPath: dir + "/llmgw-server.service", etcDir: dir, serviceActive: func() bool { return true }} if err := in.checkMemory(); err != nil { t.Errorf("refused: %v", err) } + // A first run that stopped half way left server.yaml and serves nothing: + // it still would install the models, and is checked. + in.serviceActive = func() bool { return false } + if in.alreadyServing() { + t.Error("a machine whose gateway is not running was taken for one serving") + } } diff --git a/internal/cli/tui_mcp.go b/internal/cli/tui_mcp.go index daa57fe..21894da 100644 --- a/internal/cli/tui_mcp.go +++ b/internal/cli/tui_mcp.go @@ -115,6 +115,17 @@ func (s *consoleMCPMsg) name(scope store.Scope, id, label string) { } } +// closed are the levels whose list names no tool at all. +func (k mcpState) closed() []mcpGrant { + var out []mcpGrant + for _, g := range k.grants { + if len(g.tools) == 0 { + out = append(out, g) + } + } + return out +} + // mcpGrantsOf is who may use a server's tools, and which of them. func (k mcpState) grantsOf(server string) []mcpGrant { var out []mcpGrant @@ -176,7 +187,16 @@ func (m consoleModel) mcpPanel(pw int) (string, string, string) { rows = append(rows, []string{s.Name, state, granted, rate, dash(strings.Join(mcpCredentials(s), ", ")), s.URL}) } b.WriteString(table([]string{"SERVER", "STATE", "GRANTED TO", "RATE", "SENT", "ADDRESS"}, rows, m.row, pw, m.inPanel)) - b.WriteString("\n\n " + sDim.Render("A server's tools are seen only where they are granted; nothing is granted by default.") + "\n\n") + b.WriteString("\n\n " + sDim.Render("A server's tools are seen only where they are granted; nothing is granted by default.") + "\n") + if closed := k.closed(); len(closed) > 0 { + var names []string + for _, g := range closed { + names = append(names, g.label) + } + b.WriteString(" " + sBad.Render("Closed to every tool: ") + strings.Join(names, ", ") + + sDim.Render(" (a server's menu opens them)") + "\n") + } + b.WriteString("\n") b.WriteString(mcpClientHint()) return "MCP servers", b.String(), keys } @@ -290,12 +310,20 @@ 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 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.", + text: "Taken from it and from everything beneath it. Its other grants stay; left naming\n" + + "no tool, it is closed rather than opened to what the levels above allow.", yes: func(m *consoleModel) tea.Cmd { return m.mcpSetTools(s.Name, g.owner, nil) }} return nil }}) } + for _, g := range m.mcp.closed() { + items = append(items, menuItem{"Open " + g.label + ", closed to every tool…", func(m *consoleModel) tea.Cmd { + m.confirmD = &confirmDialog{title: "Open " + g.label, + text: "It then names no tools of its own and sees what the levels above it allow.", + yes: func(m *consoleModel) tea.Cmd { return m.mcpOpen(g.owner) }} + return nil + }}) + } items = append(items, menuItem{"Change…", func(m *consoleModel) tea.Cmd { m.mcpEditForm(s); return nil }}, menuItem{"Replace its credential…", func(m *consoleModel) tea.Cmd { m.mcpCredentialForm(s); return nil }}, @@ -487,3 +515,20 @@ func (m *consoleModel) mcpSetTools(server string, owner store.LimitOwner, tools return actionDoneMsg{title: title, text: text} } } + +// mcpOpen has a closed level name no tools of its own (store.OpenTools). +func (m *consoleModel) mcpOpen(owner store.LimitOwner) tea.Cmd { + h := m.st + return func() tea.Msg { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + s, err := h.get(ctx) + if err == nil { + err = s.OpenTools(ctx, owner.Scope, owner.ID) + } + if err != nil { + return actionDoneMsg{title: "Open", err: err} + } + return actionDoneMsg{title: "Open", text: "Opened: it names no tools of its own now."} + } +} diff --git a/internal/control/catalog/pickle.go b/internal/control/catalog/pickle.go index 9e40cc1..372487c 100644 --- a/internal/control/catalog/pickle.go +++ b/internal/control/catalog/pickle.go @@ -6,6 +6,7 @@ import ( "slices" "strings" + "gitea.mynodes.xyz/ilker/llmgw/internal/agent/engines" "gitea.mynodes.xyz/ilker/llmgw/internal/agent/weights" ) @@ -48,7 +49,7 @@ func (m Model) CheckPickle(except ...string) error { // What an engine downloads itself, by a Hugging Face id, no node looks // at: vLLM reads a model by id as safetensors only, but not an adapter, // and TensorRT-LLM neither. - for _, s := range m.unlookedSources() { + for _, s := range m.UnlookedSources() { if slices.Contains(except, s) { continue } @@ -59,9 +60,9 @@ func (m Model) CheckPickle(except ...string) error { return nil } -// unlookedSources are those an engine fetches by a Hugging Face id, past +// UnlookedSources are those an engine fetches by a Hugging Face id, past // the node's check: adapters, and a TensorRT-LLM model's. -func (m Model) unlookedSources() []string { +func (m Model) UnlookedSources() []string { var out []string for _, a := range m.Adapters { if hubID(a.Source) { @@ -78,14 +79,13 @@ func (m Model) unlookedSources() []string { return out } -// hubID reports whether a source is a Hugging Face id, org/name, rather -// than a URL, a path or a file. +// hubID reports whether a source is a Hugging Face id, as the agent reads +// one (engines.IsHubID) — gpt2 too — and not a file named so, nor a GGUF +// in a repository (org/name:Q4_K_M). func hubID(s string) bool { - if s == "" || strings.Contains(s, "://") || strings.HasPrefix(s, "/") || strings.HasPrefix(s, ".") || - strings.HasPrefix(s, "hf:") || strings.Count(s, "/") != 1 || strings.Contains(s, ":") { + if !engines.IsHubID(s) || strings.Contains(s, ":") { return false } - // A file named so is a file: org/model.gguf is not a repository. ext := strings.ToLower(path.Ext(s)) return ext != ".gguf" && ext != ".safetensors" && !weights.Pickled(s) } diff --git a/internal/control/catalog/pickle_test.go b/internal/control/catalog/pickle_test.go index 61efeeb..9a5d3b4 100644 --- a/internal/control/catalog/pickle_test.go +++ b/internal/control/catalog/pickle_test.go @@ -58,7 +58,8 @@ func TestWhatAnEngineDownloadsByIDIsAskedAbout(t *testing.T) { t.Errorf("vLLM reads a model by id as safetensors only: %v", err) } for s, want := range map[string]bool{"Qwen/Qwen2.5-7B": true, "meta-llama/Llama-3.1-8B-Instruct": true, - "org/model.gguf": false, "o/r:Q4_K_M": false, "/models/x": false, "hf:o/r/f.gguf": false} { + "org/model.gguf": false, "o/r:Q4_K_M": false, "/models/x": false, "hf:o/r/f.gguf": false, + "gpt2": true, "~me/x": false} { if hubID(s) != want { t.Errorf("hubID(%q) = %v", s, !want) } diff --git a/internal/control/modelsvc/modelsvc.go b/internal/control/modelsvc/modelsvc.go index 3f003f8..47fca5c 100644 --- a/internal/control/modelsvc/modelsvc.go +++ b/internal/control/modelsvc/modelsvc.go @@ -409,7 +409,7 @@ func (s *Service) update(ctx context.Context, name, by, what string, change func found := false for _, e := range stored { if e.Name == name { - before := e.PickleSources() + before := append(e.PickleSources(), e.UnlookedSources()...) if err := change(&e); err != nil { return catalog.Model{}, err } diff --git a/internal/store/toolgrants.go b/internal/store/toolgrants.go index 68cf803..ca8370b 100644 --- a/internal/store/toolgrants.go +++ b/internal/store/toolgrants.go @@ -70,12 +70,28 @@ func (s *DB) GrantTools(ctx context.Context, scope Scope, id, server string, too 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. +// RevokeTools takes server's tools away from a level and from every level +// beneath it: taken from an organization, no team, project or key in it +// keeps them by having been given them below. A level left naming nothing +// is closed — it names no tool, so sees none — while any level above or +// beneath it names tools: going back to naming none at all would open it +// to what the levels above allow. With none anywhere, it names none. func (s *DB) RevokeTools(ctx context.Context, scope Scope, id, server string) error { + below, err := s.limitsBelow(ctx, scope, id) + if err != nil { + return err + } + for _, l := range below { + if l.AllowedTools == nil { + continue + } + if kept := withoutServer(l.AllowedTools, server); len(kept) != len(l.AllowedTools) { + l.AllowedTools = kept + if err := s.PutLimits(ctx, l); err != nil { + return err + } + } + } lim, err := s.LimitsFor(ctx, scope, id) if errors.Is(err, ErrNotFound) { return nil @@ -90,8 +106,8 @@ func (s *DB) RevokeTools(ctx context.Context, scope Scope, id, server string) er return err } lim.AllowedTools = nil - for _, a := range above { - if a.AllowedTools != nil { + for _, l := range append(above, below...) { + if l.AllowedTools != nil { lim.AllowedTools = []string{} break } @@ -100,6 +116,88 @@ func (s *DB) RevokeTools(ctx context.Context, scope Scope, id, server string) er return s.PutLimits(ctx, lim) } +// OpenTools has a level name no tools of its own: it then sees what the +// levels above it allow. For a level left closed by RevokeTools, on +// purpose. +func (s *DB) OpenTools(ctx context.Context, scope Scope, id string) error { + lim, err := s.LimitsFor(ctx, scope, id) + if errors.Is(err, ErrNotFound) { + return nil + } + if err != nil { + return err + } + lim.AllowedTools = nil + return s.PutLimits(ctx, lim) +} + +// limitsBelow are the limits set beneath a level: an organization's teams, +// projects and their keys, a team's projects and keys, a project's keys, +// and the member scopes of each. +func (s *DB) limitsBelow(ctx context.Context, scope Scope, id string) ([]Limits, error) { + var owners []LimitOwner + addProject := func(pid string) error { + owners = append(owners, LimitOwner{Scope: ScopeProject, ID: pid}, LimitOwner{Scope: MemberScope(ScopeProject), ID: pid}) + keys, err := s.Keys(ctx, pid) + if err != nil { + return err + } + for _, k := range keys { + owners = append(owners, LimitOwner{Scope: ScopeKey, ID: k.ID}) + } + return nil + } + switch scope { + case ScopeOrg, ScopeTeam: + tree, err := s.Tree(ctx) + if err != nil { + return nil, err + } + for _, o := range tree { + if scope == ScopeOrg && o.ID != id { + continue + } + if scope == ScopeOrg { + owners = append(owners, LimitOwner{Scope: MemberScope(ScopeOrg), ID: o.ID}) + } + for _, t := range o.Teams { + if scope == ScopeTeam && t.ID != id { + continue + } + if scope == ScopeOrg { + owners = append(owners, LimitOwner{Scope: ScopeTeam, ID: t.ID}) + } + owners = append(owners, LimitOwner{Scope: MemberScope(ScopeTeam), ID: t.ID}) + for _, p := range t.Projects { + if err := addProject(p.ID); err != nil { + return nil, err + } + } + } + } + case ScopeProject: + if err := addProject(id); err != nil { + return nil, err + } + owners = owners[1:] // the project itself is not beneath it + } + var out []Limits + for _, o := range owners { + if o.Scope == "" { + continue + } + 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 +} + func withoutServer(tools []string, server string) []string { return slices.DeleteFunc(slices.Clone(tools), func(t string) bool { return strings.HasPrefix(t, server+".") }) } @@ -120,9 +218,15 @@ func (s *DB) limitsAbove(ctx context.Context, scope Scope, id string) ([]Limits, var owners []LimitOwner if scope == ScopeKey { k, err := s.KeyByID(ctx, id) - if err != nil { + if errors.Is(err, ErrNotFound) { return nil, nil } + if err != nil { + return nil, err + } + if k.UserID != "" { + owners = append(owners, LimitOwner{Scope: ScopeUser, ID: k.UserID}) + } scope, id = ScopeProject, k.ProjectID owners = append(owners, LimitOwner{Scope: ScopeProject, ID: id}) } diff --git a/internal/store/toolgrants_test.go b/internal/store/toolgrants_test.go index d6dee0d..fbac754 100644 --- a/internal/store/toolgrants_test.go +++ b/internal/store/toolgrants_test.go @@ -98,3 +98,52 @@ func TestRevokingALevelsLastToolsNeitherClosesNorOpensIt(t *testing.T) { t.Error("withoutServer") } } + +func TestRevokingFromAnOrganizationTakesItFromEverythingBeneath(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 _, ts := range [][]string{tools(ScopeOrg, org), tools(ScopeTeam, team)} { + if ts != nil { + e.ToolGrants = append(e.ToolGrants, ts) + } + } + return e.AllowsTool(tool) + } + _, _ = s.GrantTools(ctx, ScopeOrg, org, "hr", []string{"hr.*"}) + _, _ = s.GrantTools(ctx, ScopeTeam, team, "jira", []string{"jira.*"}) // keeps hr.*, copied + // The organization takes hr's back: the team, which held a copy, loses + // them too; and the organization, naming nothing now, is closed rather + // than opened — its team's jira.* was kept out of reach by it before. + if err := s.RevokeTools(ctx, ScopeOrg, org, "hr"); err != nil { + t.Fatal(err) + } + if sees("hr.search") { + t.Errorf("the team kept hr after its organization's revoke: org %v team %v", tools(ScopeOrg, org), tools(ScopeTeam, team)) + } + if sees("jira.search") { + t.Errorf("the revoke opened the organization to jira: org %v", tools(ScopeOrg, org)) + } + if got := tools(ScopeOrg, org); got == nil || len(got) != 0 { + t.Errorf("the organization is %v; want closed, []", got) + } + // Opened on purpose, it names none and bounds nothing: the team's own + // list decides. + if err := s.OpenTools(ctx, ScopeOrg, org); err != nil { + t.Fatal(err) + } + if tools(ScopeOrg, org) != nil || !sees("jira.search") { + t.Errorf("opened: org %v team %v", tools(ScopeOrg, org), tools(ScopeTeam, team)) + } +} diff --git a/internal/web/admin_mcp.go b/internal/web/admin_mcp.go index cbe9987..311bcba 100644 --- a/internal/web/admin_mcp.go +++ b/internal/web/admin_mcp.go @@ -35,6 +35,7 @@ type MCPAdmin interface { 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 KeyByID(ctx context.Context, id string) (store.Key, error) + OpenTools(ctx context.Context, scope store.Scope, id string) error Tree(ctx context.Context) ([]store.TreeOrg, error) Users(ctx context.Context, state store.UserState) ([]store.User, error) } @@ -194,8 +195,13 @@ func (p *AccountPages) RegisterAdminMCP(mux *http.ServeMux, s MCPAdmin, local fu mux.HandleFunc("POST /admin/mcp/{name}/edit", adminsOnly(func(w http.ResponseWriter, r *http.Request, admin store.User) { l, name := langOf(r), r.PathValue("name") timeout, rate, err := mcpBounds(l, r.FormValue("timeout"), r.FormValue("rate")) + addr := strings.TrimSpace(r.FormValue("url")) + // This cluster's own stays its own, whatever a form sends. + if m, gerr := s.MCPServer(r.Context(), name); gerr == nil && m.URL == adminapi.AdminMCPAddress { + addr = adminapi.AdminMCPAddress + } if err == nil { - req := adminapi.MCPServerRequest{URL: strings.TrimSpace(r.FormValue("url")), + req := adminapi.MCPServerRequest{URL: addr, Description: strings.TrimSpace(r.FormValue("description")), TimeoutSeconds: timeout, RatePerMinute: rate} // An emptied field empties it, rather than leaving it as it was. if req.Description == "" { @@ -298,6 +304,11 @@ func (p *AccountPages) RegisterAdminMCP(mux *http.ServeMux, s MCPAdmin, local fu done(w, r, admin, "mcp grants "+name, notice, err) })) + mux.HandleFunc("POST /admin/mcp/open", adminsOnly(func(w http.ResponseWriter, r *http.Request, admin store.User) { + err := s.OpenTools(r.Context(), store.Scope(r.FormValue("scope")), r.FormValue("id")) + done(w, r, admin, "mcp grants", langOf(r).T("mcp.opened"), 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") diff --git a/internal/web/locales/en/mcp.json b/internal/web/locales/en/mcp.json index d8dae0d..27b8f55 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 then sees them only if the levels above grant them and it names no tools of its own.", + "mcp.confirm_revoke": "Revoke %s from %s and from everything beneath it? A level left naming no tool is closed, rather than opened to what the levels above allow.", "mcp.revoke": "Revoke from %s", "mcp.change": "Change", "mcp.address": "Address", @@ -83,6 +83,9 @@ "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." + "mcp.closed": "Closed to every tool — a server's tools were taken back and it named no other, while a level above or beneath names some:", + "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 back takes them from everything beneath too, and leaves the rest.", + "mcp.opened": "Opened: it names no tools of its own now, and sees what the levels above it allow.", + "mcp.open": "open it", + "mcp.confirm_open": "Open %s? It then names no tools of its own and sees what the levels above it allow." } diff --git a/internal/web/locales/tr/mcp.json b/internal/web/locales/tr/mcp.json index cb491e5..c12e9c1 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ı? Bundan sonra onları yalnızca üst seviyeler veriyorsa ve kendi araç listesi yoksa görür.", + "mcp.confirm_revoke": "%s yetkisi %s üzerinden ve onun altındaki her şeyden geri alınsın mı? Hiç araç adı kalmayan seviye kapanır; üst seviyelerin izin verdiğine açılmaz.", "mcp.revoke": "%s üzerinden geri al", "mcp.change": "Değiştir", "mcp.address": "Adres", @@ -82,6 +82,9 @@ "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." + "mcp.closed": "Tüm araçlara kapalı — bir sunucunun yetkisi geri alındı, başka bir sunucu adı kalmadı, üstte ya da altta liste tanımlayan bir seviye var:", + "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 geri almak altındaki her şeyden de alır, diğerlerini bırakır.", + "mcp.opened": "Açıldı: artık kendi araç listesi yok, üst seviyelerin izin verdiğini görür.", + "mcp.open": "aç", + "mcp.confirm_open": "%s açılsın mı? Kendi araç listesi kalmaz ve üst seviyelerin izin verdiğini görür." } diff --git a/internal/web/templates/admin_mcp.html b/internal/web/templates/admin_mcp.html index ecd46ea..3b00c34 100644 --- a/internal/web/templates/admin_mcp.html +++ b/internal/web/templates/admin_mcp.html @@ -81,7 +81,11 @@ {{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}} +{{if .Closed}}

{{t "mcp.closed"}}

+{{end}}

{{t "mcp.how_grants"}}

{{t "mcp.add"}}