diff --git a/internal/cli/tui_mcp.go b/internal/cli/tui_mcp.go
index 791faf6..daa57fe 100644
--- a/internal/cli/tui_mcp.go
+++ b/internal/cli/tui_mcp.go
@@ -33,6 +33,8 @@ type mcpState struct {
grants []mcpGrant
where map[string]string
ids map[string]store.LimitOwner
+ // keys are the IDs of the keys there are, for a grant to one.
+ keys map[string]bool
}
// mcpGrant is one level of the tree that grants tools, and which.
@@ -80,6 +82,13 @@ func (m consoleModel) fetchMCP() tea.Cmd {
msg.name(store.ScopeUser, u.ID, u.Email)
}
}
+ msg.keys = map[string]bool{}
+ if keys, err := s.AllKeys(ctx); err == nil {
+ for _, k := range keys {
+ msg.keys[k.ID] = true
+ msg.where[store.LimitOwner{Scope: store.ScopeKey, ID: k.ID}.CounterKey()] = k.Name
+ }
+ }
msg.grants = []mcpGrant{}
for _, l := range limits {
if l.AllowedTools == nil {
@@ -244,8 +253,8 @@ func (m *consoleModel) mcpAddForm() {
if name == "" || addr == "" {
return nil, fmt.Errorf("a name and an address")
}
- if u, err := url.Parse(addr); err != nil || (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" {
- return nil, fmt.Errorf("the address is http(s)://host/path")
+ if err := adminapi.CheckMCPAddress(addr); err != nil {
+ return nil, err
}
secrets, err := mcpAuth(v[3], strings.TrimSpace(v[4]), v[5])
if err != nil {
@@ -350,8 +359,8 @@ func (m *consoleModel) mcpEditForm(s adminapi.MCPServerView) {
textF("rate", rate, "calls a minute per caller; 0 is no bound"),
},
submit: func(m *consoleModel, v []string) (tea.Cmd, error) {
- if u, err := url.Parse(v[0]); err != nil || (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" {
- return nil, fmt.Errorf("the address is http(s)://host/path")
+ if err := adminapi.CheckMCPAddress(strings.TrimSpace(v[0])); err != nil {
+ return nil, err
}
timeout, err := atoiOr(v[2], 0)
if err != nil || timeout < 0 {
@@ -361,10 +370,18 @@ func (m *consoleModel) mcpEditForm(s adminapi.MCPServerView) {
if err != nil || rate < 0 {
return nil, fmt.Errorf("rate is calls a minute")
}
- if rate == 0 && s.RatePerMinute > 0 {
- rate = -1 // the API's way of taking the bound away
+ req := adminapi.MCPServerRequest{URL: strings.TrimSpace(v[0]), Description: v[1], TimeoutSeconds: timeout,
+ RatePerMinute: rate}
+ // An emptied field empties it, rather than leaving it as it was.
+ if strings.TrimSpace(v[1]) == "" {
+ req.Clear = append(req.Clear, "description")
+ }
+ if timeout == 0 {
+ req.Clear = append(req.Clear, "timeout")
+ }
+ if rate == 0 {
+ req.Clear = append(req.Clear, "rate")
}
- req := adminapi.MCPServerRequest{URL: v[0], Description: v[1], TimeoutSeconds: timeout, RatePerMinute: rate}
return m.call("Change "+s.Name, http.MethodPut, mcpPath(s.Name), req, nil), nil
}})
}
@@ -414,7 +431,7 @@ func (m *consoleModel) mcpGrantForm(s adminapi.MCPServerView) {
}
owner, ok := m.mcp.ids[v[0]+":"+strings.ToLower(which)]
if v[0] == "key" {
- owner, ok = store.LimitOwner{Scope: store.ScopeKey, ID: which}, true
+ owner, ok = store.LimitOwner{Scope: store.ScopeKey, ID: which}, m.mcp.keys[which]
}
if !ok {
return nil, fmt.Errorf("no %s %q", v[0], which)
diff --git a/internal/control/adminapi/mcp.go b/internal/control/adminapi/mcp.go
index abfa1b3..1cfe080 100644
--- a/internal/control/adminapi/mcp.go
+++ b/internal/control/adminapi/mcp.go
@@ -57,6 +57,9 @@ type MCPServerRequest struct {
// RatePerMinute bounds each caller's calls to its tools; a negative
// number takes the bound away.
RatePerMinute int `json:"rate_per_minute,omitempty"`
+ // Clear empties what an empty field above would leave as it is:
+ // "description", "timeout" (back to the default), "rate".
+ Clear []string `json:"clear,omitempty"`
}
// MCPTestView is what the server said when asked: its tools, or why not.
@@ -171,8 +174,8 @@ func MCPServerFrom(ctx context.Context, st MCPStore, name string, req MCPServerR
if req.URL != "" {
m.URL = req.URL
}
- if u, err := url.Parse(m.URL); m.URL != AdminMCPAddress && (err != nil || (u.Scheme != "https" && u.Scheme != "http") || u.Host == "") {
- return m, errors.New("the server's url is an http:// or https:// address, or " + AdminMCPAddress + " for this cluster's own")
+ if err := CheckMCPAddress(m.URL); err != nil {
+ return m, err
}
if req.Description != "" {
m.Description = req.Description
@@ -209,9 +212,39 @@ func MCPServerFrom(ctx context.Context, st MCPStore, name string, req MCPServerR
case req.RatePerMinute < 0:
m.RatePerMinute = 0
}
+ for _, f := range req.Clear {
+ switch f {
+ case "description":
+ m.Description = ""
+ case "timeout":
+ m.Timeout = 0
+ case "rate":
+ m.RatePerMinute = 0
+ default:
+ return m, errors.New("clear names description, timeout or rate, not " + f)
+ }
+ }
return m, nil
}
+// CheckMCPAddress refuses an address a server cannot be reached at: an
+// http(s) one, or this cluster's own. Not one with a user and password in
+// it: a credential is a header, kept sealed, and an address is shown and
+// recorded in the audit log as written.
+func CheckMCPAddress(addr string) error {
+ if addr == AdminMCPAddress {
+ return nil
+ }
+ u, err := url.Parse(addr)
+ if err != nil || (u.Scheme != "https" && u.Scheme != "http") || u.Host == "" {
+ return errors.New("the server's url is an http:// or https:// address, or " + AdminMCPAddress + " for this cluster's own")
+ }
+ if u.User != nil {
+ return errors.New("the server's url has a user and password in it: give the credential as a header, which is kept sealed")
+ }
+ return nil
+}
+
// testMCPServer asks a server for its tools, with its credentials, as /mcp
// would.
func (a *API) testMCPServer(ctx context.Context, m store.MCPServer) MCPTestView {
diff --git a/internal/web/admin_mcp.go b/internal/web/admin_mcp.go
index 15c97b8..cbe9987 100644
--- a/internal/web/admin_mcp.go
+++ b/internal/web/admin_mcp.go
@@ -34,6 +34,7 @@ type MCPAdmin interface {
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
+ KeyByID(ctx context.Context, id string) (store.Key, error)
Tree(ctx context.Context) ([]store.TreeOrg, error)
Users(ctx context.Context, state store.UserState) ([]store.User, error)
}
@@ -193,12 +194,20 @@ 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"))
- if err == nil && rate == 0 {
- rate = -1 // an empty rate takes the bound away
- }
if err == nil {
- err = put(r, admin, name, adminapi.MCPServerRequest{URL: strings.TrimSpace(r.FormValue("url")),
- Description: strings.TrimSpace(r.FormValue("description")), TimeoutSeconds: timeout, RatePerMinute: rate})
+ req := adminapi.MCPServerRequest{URL: strings.TrimSpace(r.FormValue("url")),
+ 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 == "" {
+ req.Clear = append(req.Clear, "description")
+ }
+ if timeout == 0 {
+ req.Clear = append(req.Clear, "timeout")
+ }
+ if rate == 0 {
+ req.Clear = append(req.Clear, "rate")
+ }
+ err = put(r, admin, name, req)
}
done(w, r, admin, "mcp server "+name, l.T("mcp.changed", name), err)
}))
@@ -357,7 +366,12 @@ func mcpResolve(ctx context.Context, s MCPAdmin, l *Lang, scope store.Scope, whi
}
switch scope {
case store.ScopeKey:
- return which, which, nil
+ // A key that is not there would be granted nothing, and said granted.
+ k, err := s.KeyByID(ctx, which)
+ if err != nil {
+ return "", "", errors.New(l.T("mcp.err.no_such", which))
+ }
+ return k.ID, k.Name + " (" + k.Prefix + "…)", nil
case store.ScopeUser:
users, err := s.Users(ctx, "")
if err != nil {
diff --git a/internal/web/admin_mcp_test.go b/internal/web/admin_mcp_test.go
index 0989b91..9b08c4e 100644
--- a/internal/web/admin_mcp_test.go
+++ b/internal/web/admin_mcp_test.go
@@ -12,6 +12,7 @@ import (
"slices"
"strings"
"testing"
+ "time"
"gitea.mynodes.xyz/ilker/llmgw/internal/mcpclient/mcptest"
"gitea.mynodes.xyz/ilker/llmgw/internal/secrets"
@@ -183,3 +184,26 @@ func TestAGrantBelowALevelThatNamesOtherToolsSaysSoAndKeepsWhatItHad(t *testing.
t.Errorf("the team's list is %v; it should keep hr's, which it saw from its organization", l.AllowedTools)
}
}
+
+func TestAnMCPServersFieldsCanBeEmptiedAndItsAddressHoldsNoPassword(t *testing.T) {
+ mux, st, _ := mcpPage(t, mcpAdmin)
+ ctx := context.Background()
+ _ = st.PutMCPServer(ctx, store.MCPServer{Name: "hr", URL: "https://hr/mcp", Description: "people", Enabled: true,
+ Timeout: 30 * time.Second, RatePerMinute: 10})
+ mirrorPost(t, mux, "/admin/mcp/hr/edit", url.Values{"csrf": {"tok"}, "url": {"https://hr/mcp"}, "description": {""},
+ "timeout": {""}, "rate": {""}})
+ if m, _ := st.MCPServer(ctx, "hr"); m.Description != "" || m.Timeout != 0 || m.RatePerMinute != 0 {
+ t.Errorf("emptied fields kept: %+v", m)
+ }
+ body := mirrorPost(t, mux, "/admin/mcp/add", url.Values{"csrf": {"tok"}, "name": {"db"},
+ "url": {"https://admin:hunter2@db.internal/mcp"}, "auth": {"none"}}).Body.String()
+ if _, err := st.MCPServer(ctx, "db"); err == nil || !strings.Contains(body, "user and password in it") {
+ t.Errorf("an address with a password was kept: %s", body)
+ }
+ // A grant to a key that is not there is refused, not said done.
+ body = mirrorPost(t, mux, "/admin/mcp/hr/grant", url.Values{"csrf": {"tok"}, "scope": {"key"}, "which": {"key-nope"},
+ "tools": {"hr.*"}}).Body.String()
+ if !strings.Contains(body, "There is no key-nope") {
+ t.Errorf("a grant to no key: %s", body)
+ }
+}
diff --git a/internal/web/templates/admin_mcp.html b/internal/web/templates/admin_mcp.html
index 3d19c29..ecd46ea 100644
--- a/internal/web/templates/admin_mcp.html
+++ b/internal/web/templates/admin_mcp.html
@@ -57,16 +57,16 @@
{{end}}{{end}}
- {{if not .Local}}
{{t "mcp.change"}}
+ {{if not .Local}}
{{t "mcp.credential"}}