diff --git a/pkg/modules/mcp/dispatcher.go b/pkg/modules/mcp/dispatcher.go index c9162c785..3d479b197 100644 --- a/pkg/modules/mcp/dispatcher.go +++ b/pkg/modules/mcp/dispatcher.go @@ -95,12 +95,13 @@ var crud = defaultCRUD // skip the unmarshal round-trip the SDK has already performed against the // input schema). // -// Errors fall into two categories: -// - ErrToolNotFound / ErrNoUserInContext / JSON-unmarshal errors are -// dispatcher-level failures the caller should translate into an -// IsError=true tool result. We return them as errors here (rather than -// constructing a *mcp.CallToolResult) so the dispatcher stays -// SDK-agnostic; the thin AddTool handler does the wrapping. +// Errors fall into three categories: +// - ErrToolNotFound / ErrNoUserInContext / ErrScopeDenied / +// JSON-unmarshal errors are dispatcher-level failures the caller should +// translate into an IsError=true tool result. We return them as errors +// here (rather than constructing a *mcp.CallToolResult) so the +// dispatcher stays SDK-agnostic; the thin AddTool handler does the +// wrapping. // - Errors returned by handler.Do* (model-layer permission denials, // validation failures, etc.) are propagated as-is. The tool handler // wraps them with SetError per the SDK's convention that domain @@ -111,6 +112,16 @@ func Dispatch(ctx context.Context, toolName string, rawArgs json.RawMessage) (an return nil, fmt.Errorf("%w: %s", ErrToolNotFound, toolName) } + // Scope check first — never allocate a wrapper or touch model state + // for a tool the caller isn't authorized to invoke. This guards + // against the (rare) case where the per-session tool registration in + // newServer registered a tool the current request's token doesn't + // have a scope for: the SDK caches the *Server across requests, but + // the API token is per-HTTP-request. + if !tokenAuthorizes(TokenFromContext(ctx), ref.resource.Name, ref.op) { + return nil, fmt.Errorf("%w: %s", ErrScopeDenied, toolName) + } + // Allocate a fresh wrapper for this call so concurrent dispatches // don't share state through the prototype stored in r.Inputs. wrapperProto, ok := ref.resource.Inputs[ref.op] @@ -143,6 +154,11 @@ func DispatchTyped(ctx context.Context, toolName string, wrapper any) (any, erro if !ok { return nil, fmt.Errorf("%w: %s", ErrToolNotFound, toolName) } + // Scope check mirrors Dispatch — see the comment there for why this + // is necessary even when newServer already filtered the tool set. + if !tokenAuthorizes(TokenFromContext(ctx), ref.resource.Name, ref.op) { + return nil, fmt.Errorf("%w: %s", ErrScopeDenied, toolName) + } return dispatchPrepared(ctx, ref, wrapper) } diff --git a/pkg/modules/mcp/dispatcher_test.go b/pkg/modules/mcp/dispatcher_test.go index e66898a05..9d04fc072 100644 --- a/pkg/modules/mcp/dispatcher_test.go +++ b/pkg/modules/mcp/dispatcher_test.go @@ -22,6 +22,7 @@ import ( "errors" "testing" + "code.vikunja.io/api/pkg/models" "code.vikunja.io/api/pkg/user" "code.vikunja.io/api/pkg/web" "code.vikunja.io/api/pkg/web/handler" @@ -119,10 +120,20 @@ func (i *stubInput) ReadAllParams() (string, int, int) { return i.Search, i.Page, i.PerPage } +// newAuthedCtx returns a context with a test user and an API token that +// authorizes every (resource, op) on the "stubs" resource — sufficient for +// the dispatcher's wiring tests. Scope-denied scenarios are covered in +// scope_test.go with explicitly narrower tokens. func newAuthedCtx(t *testing.T) context.Context { t.Helper() u := &user.User{ID: 42} - return WithUser(context.Background(), u) + token := &models.APIToken{ + APIPermissions: models.APIPermissions{ + "stubs": []string{"create", "read_one", "read_all", "update", "delete"}, + }, + } + ctx := WithUser(context.Background(), u) + return WithToken(ctx, token) } // installStubCRUD swaps the dispatcher's Do* function set with test doubles @@ -171,7 +182,18 @@ func TestDispatchNoUser(t *testing.T) { Inputs: map[Op]any{OpReadOne: &stubInput{}}, })) - _, err := Dispatch(context.Background(), "stubs_read_one", json.RawMessage(`{"id":1}`)) + // Attach an authorising token but no user — the scope check passes, + // the user lookup inside dispatchPrepared fails. Ordering matters: the + // scope check runs first so callers without a token never reach the + // user check. + token := &models.APIToken{ + APIPermissions: models.APIPermissions{ + "stubs": []string{"read_one"}, + }, + } + ctx := WithToken(context.Background(), token) + + _, err := Dispatch(ctx, "stubs_read_one", json.RawMessage(`{"id":1}`)) require.Error(t, err) assert.ErrorIs(t, err, ErrNoUserInContext) } diff --git a/pkg/modules/mcp/mcp.go b/pkg/modules/mcp/mcp.go index 3058f6e1b..8f599895c 100644 --- a/pkg/modules/mcp/mcp.go +++ b/pkg/modules/mcp/mcp.go @@ -42,33 +42,40 @@ import ( const routePrefix = "/api/v1/mcp" // newServer constructs a fresh *mcp.Server with Vikunja's implementation -// metadata and the static set of registered tools. The SDK's -// NewStreamableHTTPHandler accepts a factory (getServer) that may return -// the same server across sessions; we return a new one per session for now -// so future per-session state (e.g. scope-filtered tool sets, see Task 6) -// has a clean place to live. +// metadata and the per-session set of registered tools. The SDK calls the +// factory passed to NewStreamableHTTPHandler exactly once per session +// (when no Mcp-Session-Id matches an existing session, i.e. at the +// initialize request); the returned *mcp.Server is cached for the +// lifetime of that session. // -// RegisterResources is idempotent and is called here so production startup -// doesn't need to know about a separate init step — the first incoming MCP -// request triggers registration on demand. -func newServer() *mcp.Server { +// Per-token tool filtering happens here: we pull the API token from the +// request context (placed there by the Echo entry handler in Handler) and +// register only the tools the token's scopes authorise. tools/list then +// returns the filtered subset naturally; tools/call is additionally +// re-checked in the dispatcher. +// +// RegisterResources is idempotent and called here so production startup +// doesn't need to know about a separate init step — the first incoming +// MCP request triggers registration on demand. +func newServer(req *http.Request) *mcp.Server { RegisterResources() srv := mcp.NewServer(&mcp.Implementation{ Name: "vikunja", Version: version.Version, }, nil) - installTools(srv) + // The token may legitimately be nil if a future code path forgets to + // attach one — installToolsForToken treats that as "no tools allowed". + // In the production flow Handler rejects unauthenticated requests + // before reaching the SDK, so this is purely defensive. + token := TokenFromContext(req.Context()) + installToolsForToken(srv, token) return srv } // streamableHandler is package-level so the SDK can manage its internal -// session map across requests. The factory returned to the SDK still -// builds a fresh *mcp.Server per session so we can attach per-session -// state later without churning the handler. -var streamableHandler = mcp.NewStreamableHTTPHandler( - func(_ *http.Request) *mcp.Server { return newServer() }, - nil, -) +// session map across requests. The factory returns a fresh *mcp.Server +// per session, scoped to the requesting token's permissions. +var streamableHandler = mcp.NewStreamableHTTPHandler(newServer, nil) // Handler is the Echo entry point for the MCP endpoint. It: // diff --git a/pkg/modules/mcp/resources.go b/pkg/modules/mcp/resources.go index 43c720f9e..ddadcd036 100644 --- a/pkg/modules/mcp/resources.go +++ b/pkg/modules/mcp/resources.go @@ -28,6 +28,17 @@ import ( "github.com/modelcontextprotocol/go-sdk/mcp" ) +// Approach for scope-filtered tools/list (Task 6): the SDK calls the +// getServer factory in NewStreamableHTTPHandler exactly once per session +// (at the initialize request, when no Mcp-Session-Id matches an existing +// session) and caches the returned *mcp.Server for the lifetime of that +// session. There is no filter callback in mcp.ServerOptions, so we build a +// per-session *mcp.Server that only registers the tools the requesting +// token's APIPermissions allows. tools/list then naturally returns the +// allowed subset. The dispatcher additionally re-checks scopes on every +// tools/call as a defence-in-depth measure (the same session could in +// principle be reused across requests carrying different tokens). + // resources.go owns the central list of MCP-exposed resources. Each entry // declares: the resource name (matches the API-token scope group), the // model's EmptyStruct, the set of supported ops, and the per-op input @@ -74,19 +85,22 @@ func registerProjects() error { }) } -// installTools walks the registry and binds each enabled (resource, op) -// pair to a tool on the given server. Per-op wrapper types are known at -// compile time, so a per-resource installer is the cleanest way to keep the -// SDK's compile-time type parameter happy while the registry stays -// data-driven elsewhere. +// installToolsForToken walks the registry and binds each (resource, op) +// pair to a tool on the given server, but only if the token authorises that +// (group, permission) combination. Per-op wrapper types are known at compile +// time, so a per-resource installer is the cleanest way to keep the SDK's +// compile-time type parameter happy while the registry stays data-driven +// elsewhere. // -// Called from newServer (mcp.go); every fresh MCP session gets the full -// tool set. Per-token scope filtering is layered on top in Task 6. -func installTools(srv *mcp.Server) { - installProjectsTools(srv) +// Called from newServer (mcp.go) at session-init time. A nil token (which +// should never happen in production because the entry handler rejects +// unauthenticated requests) yields a server with no tools — defensive, the +// dispatcher would also reject the call. +func installToolsForToken(srv *mcp.Server, token *models.APIToken) { + installProjectsToolsForToken(srv, token) } -func installProjectsTools(srv *mcp.Server) { +func installProjectsToolsForToken(srv *mcp.Server, token *models.APIToken) { r, ok := lookupResource("projects") if !ok { // Defensive: RegisterResources must run before installTools. @@ -95,19 +109,19 @@ func installProjectsTools(srv *mcp.Server) { panic("mcp: projects resource not registered") } - if r.Ops&OpCreate != 0 { + if r.Ops&OpCreate != 0 && tokenAuthorizes(token, r.Name, OpCreate) { addTool[*ProjectCreateInput](srv, r, OpCreate, "Create a new project") } - if r.Ops&OpReadOne != 0 { + if r.Ops&OpReadOne != 0 && tokenAuthorizes(token, r.Name, OpReadOne) { addTool[*ReadOneInput](srv, r, OpReadOne, "Fetch a single project by id") } - if r.Ops&OpReadAll != 0 { + if r.Ops&OpReadAll != 0 && tokenAuthorizes(token, r.Name, OpReadAll) { addTool[*ReadAllInput](srv, r, OpReadAll, "List the projects the caller has access to") } - if r.Ops&OpUpdate != 0 { + if r.Ops&OpUpdate != 0 && tokenAuthorizes(token, r.Name, OpUpdate) { addTool[*ProjectUpdateInput](srv, r, OpUpdate, "Update an existing project") } - if r.Ops&OpDelete != 0 { + if r.Ops&OpDelete != 0 && tokenAuthorizes(token, r.Name, OpDelete) { addTool[*DeleteInput](srv, r, OpDelete, "Delete a project by id") } } diff --git a/pkg/modules/mcp/scope.go b/pkg/modules/mcp/scope.go new file mode 100644 index 000000000..0ef276b9a --- /dev/null +++ b/pkg/modules/mcp/scope.go @@ -0,0 +1,48 @@ +// Vikunja is a to-do list application to facilitate your life. +// Copyright 2018-present Vikunja and contributors. All rights reserved. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package mcp + +import ( + "errors" + "slices" + + "code.vikunja.io/api/pkg/models" +) + +// ErrScopeDenied is returned by the dispatcher when the token attached to +// the call context does not have the (resource, op) scope required to invoke +// the tool. The AddTool wrapper renders this as an IsError tool result so +// the client sees a structured failure rather than a JSON-RPC protocol error. +var ErrScopeDenied = errors.New("mcp: tool not authorized for this token") + +// tokenAuthorizes returns true iff the token's APIPermissions map contains +// op.Permission() under the given resource's scope group. This is the +// (group, permission) lookup that gates both tools/list visibility and +// tools/call invocation; it intentionally duplicates rather than shares +// CanDoAPIRoute's logic because MCP doesn't have a path/method to match — +// the registry already owns the (resource, op) → (group, permission) mapping. +// +// A nil token or nil APIPermissions returns false (slices.Contains on a nil +// slice is also false, so the second case is naturally handled). Defensive +// checks here keep the dispatcher's "fail closed" contract even if the entry +// handler somehow forgets to attach a token. +func tokenAuthorizes(token *models.APIToken, resourceName string, op Op) bool { + if token == nil { + return false + } + return slices.Contains(token.APIPermissions[resourceName], op.Permission()) +} diff --git a/pkg/modules/mcp/scope_test.go b/pkg/modules/mcp/scope_test.go new file mode 100644 index 000000000..52aa1aff9 --- /dev/null +++ b/pkg/modules/mcp/scope_test.go @@ -0,0 +1,194 @@ +// Vikunja is a to-do list application to facilitate your life. +// Copyright 2018-present Vikunja and contributors. All rights reserved. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package mcp + +import ( + "encoding/json" + "testing" + + "code.vikunja.io/api/pkg/models" + "code.vikunja.io/api/pkg/user" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestTokenAuthorizes_PermissionPresent(t *testing.T) { + token := &models.APIToken{ + APIPermissions: models.APIPermissions{ + "projects": []string{"read_one", "read_all"}, + }, + } + + r := &Resource{Name: "projects"} + + assert.True(t, tokenAuthorizes(token, r.Name, OpReadOne)) + assert.True(t, tokenAuthorizes(token, r.Name, OpReadAll)) +} + +func TestTokenAuthorizes_PermissionAbsent(t *testing.T) { + token := &models.APIToken{ + APIPermissions: models.APIPermissions{ + "projects": []string{"read_one"}, + }, + } + + r := &Resource{Name: "projects"} + + assert.False(t, tokenAuthorizes(token, r.Name, OpCreate)) + assert.False(t, tokenAuthorizes(token, r.Name, OpUpdate)) + assert.False(t, tokenAuthorizes(token, r.Name, OpDelete)) +} + +func TestTokenAuthorizes_NoGroup(t *testing.T) { + token := &models.APIToken{ + APIPermissions: models.APIPermissions{ + "mcp": []string{"access"}, + }, + } + + assert.False(t, tokenAuthorizes(token, "projects", OpReadOne)) + assert.False(t, tokenAuthorizes(token, "projects", OpCreate)) +} + +func TestTokenAuthorizes_NilPermissionsMap(t *testing.T) { + // A token with nil APIPermissions should never authorize anything. + token := &models.APIToken{APIPermissions: nil} + + assert.False(t, tokenAuthorizes(token, "projects", OpReadOne)) +} + +func TestTokenAuthorizes_NilToken(t *testing.T) { + // Defensive: a nil token (should never happen in practice because the + // entry handler always sets one) must not panic. + assert.False(t, tokenAuthorizes(nil, "projects", OpReadOne)) +} + +func TestTokenAuthorizes_FullScopes(t *testing.T) { + token := &models.APIToken{ + APIPermissions: models.APIPermissions{ + "projects": []string{"create", "read_one", "read_all", "update", "delete"}, + }, + } + + for _, op := range AllOps() { + assert.Truef(t, tokenAuthorizes(token, "projects", op), "op %s should be authorized", op.ToolSuffix()) + } +} + +func TestDispatchScopeDenied(t *testing.T) { + resetRegistry(t) + installStubCRUD(t) + tracker := &stubTracker{} + require.NoError(t, Register(Resource{ + Name: "stubs", + EmptyStruct: tracker.empty, + Ops: OpCreate | OpReadOne, + Inputs: map[Op]any{ + OpCreate: &stubInput{}, + OpReadOne: &stubInput{}, + }, + })) + + // Token has read_one but not create. + token := &models.APIToken{ + APIPermissions: models.APIPermissions{ + "stubs": []string{"read_one"}, + }, + } + ctx := WithToken(newAuthedCtx(t), token) + + _, err := Dispatch(ctx, "stubs_create", json.RawMessage(`{"title":"x"}`)) + require.Error(t, err) + require.ErrorIs(t, err, ErrScopeDenied) + // The denied call must not have invoked Do*. + assert.Nil(t, tracker.last, "Do* must not run for a denied scope") +} + +func TestDispatchScopeDenied_NoTokenInContext(t *testing.T) { + // Without a token in context, the scope check has nothing to authorize + // against. The dispatcher should treat a missing token as denied + // (defensive — the entry handler always sets one in production). + resetRegistry(t) + installStubCRUD(t) + tracker := &stubTracker{} + require.NoError(t, Register(Resource{ + Name: "stubs", + EmptyStruct: tracker.empty, + Ops: OpReadOne, + Inputs: map[Op]any{OpReadOne: &stubInput{}}, + })) + + // User in context but no token — the scope check must still deny. + u := &user.User{ID: 42} + ctx := WithUser(t.Context(), u) + _, err := Dispatch(ctx, "stubs_read_one", json.RawMessage(`{"id":1}`)) + require.Error(t, err) + require.ErrorIs(t, err, ErrScopeDenied) + assert.Nil(t, tracker.last) +} + +func TestDispatchTypedScopeDenied(t *testing.T) { + // DispatchTyped is the path AddTool handlers take; the same scope check + // must apply there. + resetRegistry(t) + installStubCRUD(t) + tracker := &stubTracker{} + require.NoError(t, Register(Resource{ + Name: "stubs", + EmptyStruct: tracker.empty, + Ops: OpDelete, + Inputs: map[Op]any{OpDelete: &stubInput{}}, + })) + + token := &models.APIToken{ + APIPermissions: models.APIPermissions{ + "stubs": []string{"read_one"}, // delete not allowed + }, + } + ctx := WithToken(newAuthedCtx(t), token) + + _, err := DispatchTyped(ctx, "stubs_delete", &stubInput{ID: 1}) + require.Error(t, err) + require.ErrorIs(t, err, ErrScopeDenied) + assert.Nil(t, tracker.last) +} + +func TestDispatchScopeAllowed(t *testing.T) { + // Positive control: with the right scope, dispatch reaches the stub. + resetRegistry(t) + installStubCRUD(t) + tracker := &stubTracker{} + require.NoError(t, Register(Resource{ + Name: "stubs", + EmptyStruct: tracker.empty, + Ops: OpReadOne, + Inputs: map[Op]any{OpReadOne: &stubInput{}}, + })) + + token := &models.APIToken{ + APIPermissions: models.APIPermissions{ + "stubs": []string{"read_one"}, + }, + } + ctx := WithToken(newAuthedCtx(t), token) + + _, err := Dispatch(ctx, "stubs_read_one", json.RawMessage(`{"id":1}`)) + require.NoError(t, err) + require.NotNil(t, tracker.last) + assert.Equal(t, "ReadOne", tracker.last.called) +} diff --git a/pkg/webtests/mcp_scopes_test.go b/pkg/webtests/mcp_scopes_test.go new file mode 100644 index 000000000..bb664d889 --- /dev/null +++ b/pkg/webtests/mcp_scopes_test.go @@ -0,0 +1,157 @@ +// Vikunja is a to-do list application to facilitate your life. +// Copyright 2018-present Vikunja and contributors. All rights reserved. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package webtests + +import ( + "testing" + + "code.vikunja.io/api/pkg/db" + "code.vikunja.io/api/pkg/models" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Token 10 has {mcp:access, projects:[read_one, read_all]} — a partial scope +// for the scope-filtered tools/list and tools/call tests. +const mcpMixedScopeToken = "tk_mcp_mixed_scope_token_test_00mcpmixed02" + +// toolNamesFromList extracts the "name" field from every tool in a tools/list +// result payload. +func toolNamesFromList(t *testing.T, resp map[string]any) map[string]bool { + t.Helper() + result, ok := resp["result"].(map[string]any) + require.True(t, ok, "response missing result: %v", resp) + tools, ok := result["tools"].([]any) + require.True(t, ok, "response missing tools array: %v", result) + names := make(map[string]bool, len(tools)) + for _, raw := range tools { + tool, isMap := raw.(map[string]any) + require.Truef(t, isMap, "tool entry not an object: %v", raw) + name, _ := tool["name"].(string) + names[name] = true + } + return names +} + +func TestMCP_Scopes_ToolsListMixed(t *testing.T) { + // Token 10: projects:[read_one, read_all] — should see exactly those two + // project tools and no others. + c := newMCPClient(t, mcpMixedScopeToken) + resp := c.rpc("tools/list", map[string]any{}) + names := toolNamesFromList(t, resp) + + assert.Truef(t, names["projects_read_one"], "expected projects_read_one in: %v", names) + assert.Truef(t, names["projects_read_all"], "expected projects_read_all in: %v", names) + + assert.Falsef(t, names["projects_create"], "projects_create must be filtered out: %v", names) + assert.Falsef(t, names["projects_update"], "projects_update must be filtered out: %v", names) + assert.Falsef(t, names["projects_delete"], "projects_delete must be filtered out: %v", names) +} + +func TestMCP_Scopes_ToolsListMcpOnly(t *testing.T) { + // Token 9: only {mcp:access} — no project scopes, so no project tools + // must show in tools/list. + c := newMCPClient(t, mcpOnlyToken) + resp := c.rpc("tools/list", map[string]any{}) + names := toolNamesFromList(t, resp) + + for _, want := range []string{ + "projects_create", + "projects_read_one", + "projects_read_all", + "projects_update", + "projects_delete", + } { + assert.Falsef(t, names[want], "%s must be filtered out for an mcp-only token: %v", want, names) + } +} + +func TestMCP_Scopes_ToolsListFullScopes(t *testing.T) { + // Token 11: mcp:access + projects:* — should see all five project tools. + c := newMCPClient(t, mcpFullProjectsToken) + resp := c.rpc("tools/list", map[string]any{}) + names := toolNamesFromList(t, resp) + + for _, want := range []string{ + "projects_create", + "projects_read_one", + "projects_read_all", + "projects_update", + "projects_delete", + } { + assert.Truef(t, names[want], "expected %s in: %v", want, names) + } +} + +func TestMCP_Scopes_CallCreateForbidden(t *testing.T) { + // Token 10 lacks projects:create. Calling projects_create must come back + // as an error response without writing to the database. The SDK may + // return either a JSON-RPC protocol error (tool not found, because the + // tool wasn't registered for this session's server) or a tool result + // with isError=true (if the dispatcher's defensive scope check ran). + // Both are valid — what matters is that no DB write happened. + projectsBefore := countProjects(t) + + c := newMCPClient(t, mcpMixedScopeToken) + resp := c.rpc("tools/call", map[string]any{ + "name": "projects_create", + "arguments": map[string]any{"title": "should not be created"}, + }) + + // Either a JSON-RPC error or a tool result with isError=true is + // acceptable; what matters is no DB write. + if _, hasErr := resp["error"]; !hasErr { + result, ok := resp["result"].(map[string]any) + require.Truef(t, ok, "missing result: %v", resp) + isErr, _ := result["isError"].(bool) + assert.Truef(t, isErr, "expected isError for forbidden create: %v", result) + } + + projectsAfter := countProjects(t) + assert.Equal(t, projectsBefore, projectsAfter, "no project should be created when scope is denied") +} + +func TestMCP_Scopes_CallNonexistentTool(t *testing.T) { + // An unknown tool name must result in an error tool call result (or a + // JSON-RPC error from the SDK saying "tool not found"). Either way, the + // caller sees a failure, not a JSON-parse 500. + c := newMCPClient(t, mcpFullProjectsToken) + resp := c.rpc("tools/call", map[string]any{ + "name": "nonexistent_tool", + "arguments": map[string]any{}, + }) + + if _, hasErr := resp["error"]; hasErr { + return // SDK returned a JSON-RPC error — acceptable. + } + result, ok := resp["result"].(map[string]any) + require.Truef(t, ok, "missing both error and result: %v", resp) + isErr, _ := result["isError"].(bool) + assert.Truef(t, isErr, "expected isError for nonexistent tool: %v", result) +} + +// countProjects returns the number of rows in the projects table. Used to +// verify that a denied-scope tool call did not mutate the database. +func countProjects(t *testing.T) int64 { + t.Helper() + s := db.NewSession() + defer s.Close() + n, err := s.Count(&models.Project{}) + require.NoError(t, err) + return n +} diff --git a/pkg/webtests/mcp_test.go b/pkg/webtests/mcp_test.go index 50e6b8e78..f1adc69cb 100644 --- a/pkg/webtests/mcp_test.go +++ b/pkg/webtests/mcp_test.go @@ -137,10 +137,10 @@ func TestMCP_InitializeWithMCPToken(t *testing.T) { } func TestMCP_ToolsListReturnsRegisteredResources(t *testing.T) { - // Task 5 wires the `projects` resource into the registry, so the live - // SDK server should now expose its five tools to any token with - // mcp:access. Task 6 will add per-token scope filtering and is when - // the mcp-only token starts seeing a narrower list. + // Per Task 6, an mcp-only token (no projects scope) sees zero project + // tools in tools/list — the per-session tool registration filters by + // the requesting token's (group, permission) scopes. Tools/list visibility + // for tokens with project scopes is covered in mcp_scopes_test.go. e, err := setupTestEnv() require.NoError(t, err) @@ -174,24 +174,18 @@ func TestMCP_ToolsListReturnsRegisteredResources(t *testing.T) { require.True(t, ok, "response missing result: %s", listRec.Body.String()) tools, ok := result["tools"].([]any) require.True(t, ok, "response missing tools array: %s", listRec.Body.String()) - require.Len(t, tools, 5, "expected exactly the 5 project tools, got: %v", tools) - names := make(map[string]bool, len(tools)) + // No project tools because the token has no projects:* scopes. + projectToolCount := 0 for _, raw := range tools { tool, isMap := raw.(map[string]any) require.True(t, isMap, "tool entry should be an object: %v", raw) name, _ := tool["name"].(string) - names[name] = true - } - for _, want := range []string{ - "projects_create", - "projects_read_one", - "projects_read_all", - "projects_update", - "projects_delete", - } { - assert.Truef(t, names[want], "tools/list missing %s; got %v", want, names) + if strings.HasPrefix(name, "projects_") { + projectToolCount++ + } } + assert.Zero(t, projectToolCount, "mcp-only token must see zero project tools, got %v", tools) } func TestMCP_SessionRoundTrip(t *testing.T) {