diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index 3c65e3ca0f..0dea966832 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -168,7 +168,8 @@ and can leak ACP subprocesses. **Remote SSH executor platforms:** Treat supported remote OS/arch values as an end-to-end contract. Platform probe/normalization, lifecycle support checks, agentctl helper resolution, platform default shell, SSH readiness endpoints, frontend response types, and tests must stay aligned. Preserve raw unsupported platform details in user-facing errors, but use normalized values for supported-platform matching. Keep shell defaults platform-aware: Darwin defaults to `zsh`, Linux defaults to `bash`, unless an explicit shell is saved. **Opt-in authentication & per-user scoping** (`internal/auth/`): auth is OFF by default; the global middleware (`auth/httpmw`, installed after CORS in `backendapp.buildHTTPServer`) then injects a synthetic admin identity for the pre-auth single user, so behavior is unchanged. Enablement is the `features.auth` runtime flag (`KANDEV_FEATURES_AUTH`, Settings > System > Feature Toggles) — the auth service derives its mode from `cfg.Features.Auth` (`disabled` / `setup` = flag-on-no-admin / `enabled` = flag-on-admin-exists); there is no separate `auth.mode` setting. When enabled, requests authenticate via a `kandev_session` cookie (opaque token, SHA-256 at rest, DB-backed) or a `kandev_pat_*` bearer token. Scoping rules: -- **Identity travels in the request context** (`authn.IdentityFromContext`). No identity = internal caller (pollers, event bus, MCP stream) = unscoped. Synthetic identity = auth disabled = unscoped. +- **Identity travels in the request context** (`authn.IdentityFromContext`). No identity = internal caller (pollers, event bus, office schedulers) = unscoped. Synthetic identity = auth disabled = unscoped. +- **In-session agent MCP is scoped to the task owner.** The MCP tools an agent gets inside its own session are relayed over the agent's WebSocket stream, which carries no credential of its own. `internal/mcp/scope` resolves the stream's task → workspace → owner and attaches that user's real identity before dispatch (`lifecycle.Manager.SetMCPIdentityScoper`), so the same `authorize*` checks apply as for the PAT-authenticated `/mcp` endpoint. The owning task comes from the `AgentExecution`, never from the agent-supplied payload — do not "improve" this by reading `session_id`/`task_id` out of the request. Tool handlers stay identity-agnostic. Under enforced auth every dispatch is scoped to *somebody*: a resolvable active owner gets their identity; an unowned workspace gets a sentinel user ID that reaches unowned rows only; anything unresolvable (missing task/workspace row, or an owner whose account was deleted or disabled) is **denied**. Never return an identity-free context from this path — the task service reads that as an internal caller and grants everything. - **Workspaces are per-user** (`workspaces.owner_id`); the task service filters/denies at the service layer with `*NotFound` sentinels (no existence leak). Unowned rows (`owner_id=''`) stay visible until the setup wizard claims them for the admin. - **New user-facing service entry points must apply scoping** — call the `authorize*` helpers in `task/service/service_access.go` (or the same pattern) when adding routes that read or mutate workspace-scoped data. - **WS**: clients carry their identity; dispatched actions and subscriptions are scoped; workspace-carrying events route via `Hub.BroadcastToWorkspace`. A new `hub.Broadcast` (global) call site needs a `//ws:global` justification comment. diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager.go b/apps/backend/internal/agent/runtime/lifecycle/manager.go index 7d0df05f8d..4a6cf77d95 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager.go @@ -349,6 +349,22 @@ func (m *Manager) SetMCPHandler(handler agentctl.MCPHandler) { m.streamManager.mcpHandler = handler } +// SetMCPIdentityScoper installs the per-user scoping hook for in-session MCP +// tool calls. +// +// Unlike the external /mcp endpoint — where the agent presents a personal +// access token and the auth middleware resolves the identity — MCP requests +// relayed over an agent's own stream carry no credential. Without this hook +// they reach the task service with no identity, which that service reads as an +// internal caller and serves unscoped, so an agent supplying another user's +// task_id or workflow_id would be given their data. +// +// Set once during startup wiring, before agents start making MCP calls. Leave +// unset to keep dispatch unscoped (single-user instances). +func (m *Manager) SetMCPIdentityScoper(scoper MCPIdentityScoper) { + m.streamManager.mcpIdentityScoper = scoper +} + // SetSessionAccessChecker installs the per-user session visibility check used // by GetOrEnsureExecution and EnsurePassthroughExecution. The checker must // return nil for contexts without a request identity (internal callers). Set diff --git a/apps/backend/internal/agent/runtime/lifecycle/mcp_identity.go b/apps/backend/internal/agent/runtime/lifecycle/mcp_identity.go new file mode 100644 index 0000000000..1bc3e8b0f5 --- /dev/null +++ b/apps/backend/internal/agent/runtime/lifecycle/mcp_identity.go @@ -0,0 +1,61 @@ +package lifecycle + +import ( + "context" + + "go.uber.org/zap" + + agentctl "github.com/kandev/kandev/internal/agent/runtime/agentctl" + "github.com/kandev/kandev/internal/common/logger" + ws "github.com/kandev/kandev/pkg/websocket" +) + +// MCPIdentityScoper attaches the identity of the user who owns taskID to ctx, +// so the in-session MCP tools an agent gets automatically are authorized as +// that user instead of running unscoped. Returning an error denies the +// dispatch — see internal/mcp/scope for the production implementation and why +// it fails closed rather than falling back to full access. +type MCPIdentityScoper func(ctx context.Context, taskID string) (context.Context, error) + +// taskScopedMCPHandler scopes every MCP request on one agent stream to the +// owner of that stream's task. +// +// The task ID comes from the AgentExecution this stream belongs to, never from +// the request payload: an agent controls its own payloads, so honoring a +// payload session_id would let it name another user's session and inherit +// their identity — turning the scoping fix into a privilege escalation. +type taskScopedMCPHandler struct { + inner agentctl.MCPHandler + scope MCPIdentityScoper + taskID string + logger *logger.Logger +} + +func (h *taskScopedMCPHandler) Dispatch(ctx context.Context, msg *ws.Message) (*ws.Message, error) { + scoped, err := h.scope(ctx, h.taskID) + if err != nil { + h.logger.Error("denying in-session MCP request: cannot resolve the task owner", + zap.String("task_id", h.taskID), + zap.String("action", msg.Action), + zap.Error(err)) + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, + "failed to resolve the session owner", nil) + } + return h.inner.Dispatch(scoped, msg) +} + +// mcpHandlerFor returns the MCP handler for one execution's stream, scoped to +// the task's owning user when per-user scoping has been wired. Without a +// scoper (tests, or the runtime tier used standalone) the handler is passed +// through unchanged. +func (sm *StreamManager) mcpHandlerFor(execution *AgentExecution) agentctl.MCPHandler { + if sm.mcpHandler == nil || sm.mcpIdentityScoper == nil || execution.TaskID == "" { + return sm.mcpHandler + } + return &taskScopedMCPHandler{ + inner: sm.mcpHandler, + scope: sm.mcpIdentityScoper, + taskID: execution.TaskID, + logger: sm.logger, + } +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/mcp_identity_test.go b/apps/backend/internal/agent/runtime/lifecycle/mcp_identity_test.go new file mode 100644 index 0000000000..3bd4037f36 --- /dev/null +++ b/apps/backend/internal/agent/runtime/lifecycle/mcp_identity_test.go @@ -0,0 +1,162 @@ +package lifecycle + +import ( + "context" + "encoding/json" + "errors" + "testing" + + "github.com/kandev/kandev/internal/auth/authn" + "github.com/kandev/kandev/internal/common/logger" + ws "github.com/kandev/kandev/pkg/websocket" +) + +// recordingMCPHandler captures the context its dispatch ran under so tests can +// assert which identity (if any) reached the tool handlers. +type recordingMCPHandler struct { + gotCtx context.Context + calls int +} + +func (h *recordingMCPHandler) Dispatch(ctx context.Context, msg *ws.Message) (*ws.Message, error) { + h.gotCtx = ctx + h.calls++ + return ws.NewResponse(msg.ID, msg.Action, map[string]interface{}{"ok": true}) +} + +func newMCPStreamManager(t *testing.T, inner *recordingMCPHandler, scoper MCPIdentityScoper) *StreamManager { + t.Helper() + log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"}) + if err != nil { + t.Fatalf("logger: %v", err) + } + sm := NewStreamManager(log, StreamCallbacks{}, inner, nil) + sm.mcpIdentityScoper = scoper + return sm +} + +func mcpRequest(t *testing.T, payload map[string]interface{}) *ws.Message { + t.Helper() + data, err := json.Marshal(payload) + if err != nil { + t.Fatalf("marshal payload: %v", err) + } + return &ws.Message{ID: "req-1", Type: ws.MessageTypeRequest, Action: "mcp.list_tasks", Payload: data} +} + +// TestMCPHandlerForScopesToExecutionTask is the core wiring assertion: the +// identity handed to the tool handlers comes from the execution that owns the +// stream. +func TestMCPHandlerForScopesToExecutionTask(t *testing.T) { + inner := &recordingMCPHandler{} + var scopedTaskIDs []string + sm := newMCPStreamManager(t, inner, func(ctx context.Context, taskID string) (context.Context, error) { + scopedTaskIDs = append(scopedTaskIDs, taskID) + return authn.WithIdentity(ctx, authn.Identity{UserID: "owner-of-" + taskID, Role: authn.RoleMember}), nil + }) + + handler := sm.mcpHandlerFor(&AgentExecution{ID: "exec-1", TaskID: "task-a"}) + resp, err := handler.Dispatch(context.Background(), mcpRequest(t, map[string]interface{}{})) + if err != nil { + t.Fatalf("Dispatch: %v", err) + } + if resp.Type != ws.MessageTypeResponse { + t.Fatalf("response type = %q, want response", resp.Type) + } + + if len(scopedTaskIDs) != 1 || scopedTaskIDs[0] != "task-a" { + t.Fatalf("scoped task IDs = %v, want [task-a]", scopedTaskIDs) + } + identity, ok := authn.IdentityFromContext(inner.gotCtx) + if !ok { + t.Fatal("tool handlers received no identity") + } + if identity.UserID != "owner-of-task-a" { + t.Errorf("UserID = %q, want owner-of-task-a", identity.UserID) + } +} + +// TestMCPHandlerForIgnoresPayloadSessionID is the privilege-escalation pin. The +// payload is agent-controlled, so scoping must key off the execution's task +// even when the request names a different session or task. +func TestMCPHandlerForIgnoresPayloadSessionID(t *testing.T) { + inner := &recordingMCPHandler{} + var scopedTaskIDs []string + sm := newMCPStreamManager(t, inner, func(ctx context.Context, taskID string) (context.Context, error) { + scopedTaskIDs = append(scopedTaskIDs, taskID) + return ctx, nil + }) + + handler := sm.mcpHandlerFor(&AgentExecution{ID: "exec-1", TaskID: "task-a"}) + _, err := handler.Dispatch(context.Background(), mcpRequest(t, map[string]interface{}{ + "session_id": "session-of-victim", + "task_id": "task-victim", + })) + if err != nil { + t.Fatalf("Dispatch: %v", err) + } + + if len(scopedTaskIDs) != 1 || scopedTaskIDs[0] != "task-a" { + t.Errorf("scoped task IDs = %v, want [task-a] — payload IDs must not steer scoping", scopedTaskIDs) + } +} + +// TestMCPHandlerForDeniesWhenScopingFails pins fail-closed behavior: an +// unresolvable owner must not fall through to the unscoped handlers. +func TestMCPHandlerForDeniesWhenScopingFails(t *testing.T) { + inner := &recordingMCPHandler{} + sm := newMCPStreamManager(t, inner, func(context.Context, string) (context.Context, error) { + return nil, errors.New("db unavailable") + }) + + handler := sm.mcpHandlerFor(&AgentExecution{ID: "exec-1", TaskID: "task-a"}) + resp, err := handler.Dispatch(context.Background(), mcpRequest(t, map[string]interface{}{})) + if err != nil { + t.Fatalf("Dispatch: %v", err) + } + + if resp.Type != ws.MessageTypeError { + t.Errorf("response type = %q, want error", resp.Type) + } + if inner.calls != 0 { + t.Errorf("inner handler ran %d times, want 0 — the request must be denied", inner.calls) + } +} + +// TestMCPHandlerForPassesThroughWithoutScoper keeps single-user instances and +// isolated tests on the original unwrapped handler. +func TestMCPHandlerForPassesThroughWithoutScoper(t *testing.T) { + inner := &recordingMCPHandler{} + sm := newMCPStreamManager(t, inner, nil) + + if got := sm.mcpHandlerFor(&AgentExecution{ID: "exec-1", TaskID: "task-a"}); got != inner { + t.Errorf("handler = %T, want the unwrapped inner handler", got) + } +} + +// TestMCPHandlerForPassesThroughWithoutTaskID covers executions with no task +// (there is no owner to resolve, so wrapping would deny every call). +func TestMCPHandlerForPassesThroughWithoutTaskID(t *testing.T) { + inner := &recordingMCPHandler{} + sm := newMCPStreamManager(t, inner, func(ctx context.Context, _ string) (context.Context, error) { + return ctx, nil + }) + + if got := sm.mcpHandlerFor(&AgentExecution{ID: "exec-1"}); got != inner { + t.Errorf("handler = %T, want the unwrapped inner handler", got) + } +} + +func TestSetMCPIdentityScoperReachesStreamManager(t *testing.T) { + log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"}) + if err != nil { + t.Fatalf("logger: %v", err) + } + m := &Manager{streamManager: NewStreamManager(log, StreamCallbacks{}, &recordingMCPHandler{}, nil)} + + m.SetMCPIdentityScoper(func(ctx context.Context, _ string) (context.Context, error) { return ctx, nil }) + + if m.streamManager.mcpIdentityScoper == nil { + t.Error("SetMCPIdentityScoper did not reach the stream manager") + } +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/streams.go b/apps/backend/internal/agent/runtime/lifecycle/streams.go index 2d6d3caeb1..e914987f34 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/streams.go +++ b/apps/backend/internal/agent/runtime/lifecycle/streams.go @@ -31,6 +31,10 @@ type StreamManager struct { logger *logger.Logger callbacks StreamCallbacks mcpHandler agentctl.MCPHandler + // mcpIdentityScoper scopes in-session MCP dispatches to the owner of the + // stream's task. Nil leaves dispatch unscoped (single-user instances and + // isolated tests); set via Manager.SetMCPIdentityScoper. + mcpIdentityScoper MCPIdentityScoper // stopCh is the Manager-owned shutdown signal. The retry/backoff and // connected `<-ws.Done() / <-stop>` select read from it so they drain on // Manager.Stop. May be nil when isolated tests don't care about external @@ -282,7 +286,7 @@ func (sm *StreamManager) connectUpdatesStream(execution *AgentExecution, ready c if sm.callbacks.OnAgentEvent != nil { sm.callbacks.OnAgentEvent(execution, event) } - }, sm.mcpHandler, func(disconnectErr error) { + }, sm.mcpHandlerFor(execution), func(disconnectErr error) { // WebSocket dropped — signal promptDoneCh so SendPrompt doesn't hang forever. // Only signal on unexpected errors (not normal close). if disconnectErr != nil { @@ -324,7 +328,7 @@ func (sm *StreamManager) connectUpdatesStream(execution *AgentExecution, ready c // closing this stream is expected, not an error. func (sm *StreamManager) connectMCPStream(execution *AgentExecution) { ctx := sm.streamContext(execution) - err := execution.agentctl.StreamUpdates(ctx, func(agentctl.AgentEvent) {}, sm.mcpHandler, func(disconnectErr error) { + err := execution.agentctl.StreamUpdates(ctx, func(agentctl.AgentEvent) {}, sm.mcpHandlerFor(execution), func(disconnectErr error) { if disconnectErr != nil { sm.logger.Debug("passthrough MCP stream disconnected", zap.String("execution_id", execution.ID), diff --git a/apps/backend/internal/auth/service_credentials.go b/apps/backend/internal/auth/service_credentials.go index bbb8ffdda8..93a977a294 100644 --- a/apps/backend/internal/auth/service_credentials.go +++ b/apps/backend/internal/auth/service_credentials.go @@ -121,6 +121,28 @@ func (s *Service) ResolveBearer(ctx context.Context, token string) (authn.Identi return authn.Identity{UserID: user.ID, Role: roleOf(user), TokenID: record.ID}, true } +// IdentityForUser resolves a stored user ID to the identity that user carries +// on an authenticated request, without needing one of their credentials. +// +// It exists for callers that already know *whose* work they are performing but +// have no request to authenticate: in-session agent MCP calls arrive over the +// agent's own WebSocket stream, so the owning user is derived from the stream's +// task rather than from a cookie or PAT (see internal/mcp/scope). The returned +// identity is real — never synthetic — so per-user scoping applies to it. +// +// Returns false while authentication is disabled (callers keep the pre-auth +// unscoped behavior) and when the account is missing or no longer active. +func (s *Service) IdentityForUser(ctx context.Context, userID string) (authn.Identity, bool) { + if s == nil || userID == "" || s.Mode() == ModeDisabled { + return authn.Identity{}, false + } + user, err := s.activeUser(ctx, userID) + if err != nil { + return authn.Identity{}, false + } + return authn.Identity{UserID: user.ID, Role: roleOf(user)}, true +} + // ListSessions returns the user's sessions for the account page. func (s *Service) ListSessions(ctx context.Context, userID string) ([]*store.Session, error) { return s.store.ListSessionsByUser(ctx, userID) diff --git a/apps/backend/internal/auth/service_identity_test.go b/apps/backend/internal/auth/service_identity_test.go new file mode 100644 index 0000000000..fe0b69b0a1 --- /dev/null +++ b/apps/backend/internal/auth/service_identity_test.go @@ -0,0 +1,101 @@ +package auth + +import ( + "context" + "testing" + + "github.com/kandev/kandev/internal/auth/authn" + usermodels "github.com/kandev/kandev/internal/user/models" +) + +// IdentityForUser backs in-session agent MCP scoping (internal/mcp/scope), +// where the owning user is known from the agent's stream but no credential is +// presented. + +func TestIdentityForUserReturnsRealStoredIdentity(t *testing.T) { + f := newServiceFixture(t, false) + setupEnabled(t, f) + member, err := f.svc.AdminCreateUser(context.Background(), "member@x.dev", "memberpass123", "Member", usermodels.RoleMember) + if err != nil { + t.Fatalf("create member: %v", err) + } + + identity, ok := f.svc.IdentityForUser(context.Background(), member.ID) + if !ok { + t.Fatal("expected the member's identity to resolve") + } + if identity.UserID != member.ID { + t.Errorf("UserID = %q, want %q", identity.UserID, member.ID) + } + if identity.Role != authn.RoleMember { + t.Errorf("Role = %q, want member", identity.Role) + } + if identity.Synthetic { + t.Error("identity must not be synthetic — synthetic identities are treated as unscoped") + } + // No credential was presented, so neither credential handle is set. + if identity.SessionID != "" || identity.TokenID != "" { + t.Errorf("unexpected credential handles: session=%q token=%q", identity.SessionID, identity.TokenID) + } +} + +func TestIdentityForUserCarriesAdminRole(t *testing.T) { + f := newServiceFixture(t, false) + setupEnabled(t, f) + admin, err := f.svc.AdminCreateUser(context.Background(), "admin2@x.dev", "adminpass123", "Admin Two", usermodels.RoleAdmin) + if err != nil { + t.Fatalf("create admin: %v", err) + } + + identity, ok := f.svc.IdentityForUser(context.Background(), admin.ID) + if !ok { + t.Fatal("expected the admin's identity to resolve") + } + if identity.Role != authn.RoleAdmin { + t.Errorf("Role = %q, want admin", identity.Role) + } +} + +// TestIdentityForUserFalseWhenAuthDisabled keeps the opt-out path unscoped. +func TestIdentityForUserFalseWhenAuthDisabled(t *testing.T) { + f := newServiceFixture(t, false) + + if _, ok := f.svc.IdentityForUser(context.Background(), "some-user"); ok { + t.Error("auth disabled must not produce a scoping identity") + } +} + +func TestIdentityForUserFalseForUnknownUser(t *testing.T) { + f := newServiceFixture(t, false) + setupEnabled(t, f) + + if _, ok := f.svc.IdentityForUser(context.Background(), "no-such-user"); ok { + t.Error("an unknown user must not resolve") + } +} + +func TestIdentityForUserFalseForDisabledUser(t *testing.T) { + f := newServiceFixture(t, false) + setupEnabled(t, f) + ctx := context.Background() + member, err := f.svc.AdminCreateUser(ctx, "gone@x.dev", "memberpass123", "Gone", usermodels.RoleMember) + if err != nil { + t.Fatalf("create member: %v", err) + } + if _, err := f.svc.AdminSetRoleStatus(ctx, member.ID, usermodels.RoleMember, usermodels.StatusDisabled); err != nil { + t.Fatalf("disable member: %v", err) + } + + if _, ok := f.svc.IdentityForUser(ctx, member.ID); ok { + t.Error("a disabled user must not resolve") + } +} + +func TestIdentityForUserFalseForEmptyUserID(t *testing.T) { + f := newServiceFixture(t, false) + setupEnabled(t, f) + + if _, ok := f.svc.IdentityForUser(context.Background(), ""); ok { + t.Error("an empty user ID must not resolve") + } +} diff --git a/apps/backend/internal/backendapp/helpers.go b/apps/backend/internal/backendapp/helpers.go index 315fa9d5cc..d6548008ad 100644 --- a/apps/backend/internal/backendapp/helpers.go +++ b/apps/backend/internal/backendapp/helpers.go @@ -53,6 +53,7 @@ import ( "github.com/kandev/kandev/internal/jira" "github.com/kandev/kandev/internal/linear" mcphandlers "github.com/kandev/kandev/internal/mcp/handlers" + mcpscope "github.com/kandev/kandev/internal/mcp/scope" mcpserver "github.com/kandev/kandev/internal/mcp/server" notificationcontroller "github.com/kandev/kandev/internal/notifications/controller" notificationhandlers "github.com/kandev/kandev/internal/notifications/handlers" @@ -1359,6 +1360,20 @@ func registerMCPAndDebugRoutes( p.lifecycleMgr.SetMCPHandler(p.gateway.Dispatcher) p.log.Debug("MCP handler configured for agent lifecycle manager") + // In-session MCP calls reach this same dispatcher over the agent's own WS + // stream, which carries no credential — so scope them to the user who owns + // the stream's task. Without this the handlers run with no identity, which + // the task service treats as an internal caller and serves unscoped. + if p.authSvc != nil { + p.lifecycleMgr.SetMCPIdentityScoper(mcpscope.NewResolver( + p.taskRepo, + p.authSvc, + func() bool { return p.authSvc.Mode() != auth.ModeDisabled }, + p.log, + ).Scope) + p.log.Debug("In-session MCP dispatch scoped to task owner") + } + // External MCP endpoint — exposes config tools + create_task to external coding // agents (Claude Code, Cursor, etc.) at /mcp on the backend HTTP server. registerExternalMCP(p) diff --git a/apps/backend/internal/mcp/handlers/mcp_identity_scope_test.go b/apps/backend/internal/mcp/handlers/mcp_identity_scope_test.go new file mode 100644 index 0000000000..bb16a31833 --- /dev/null +++ b/apps/backend/internal/mcp/handlers/mcp_identity_scope_test.go @@ -0,0 +1,307 @@ +package handlers + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/kandev/kandev/internal/auth/authn" + mcpscope "github.com/kandev/kandev/internal/mcp/scope" + "github.com/kandev/kandev/internal/task/models" + sqliterepo "github.com/kandev/kandev/internal/task/repository/sqlite" + "github.com/kandev/kandev/internal/task/service" + v1 "github.com/kandev/kandev/pkg/api/v1" + ws "github.com/kandev/kandev/pkg/websocket" +) + +// In-session MCP tool calls arrive over the agent's WebSocket stream, which +// carries no credential of its own. Before the stream dispatch was scoped to +// the task owner, the task service saw "no identity" — its signal for an +// internal caller — and granted unscoped access, so an agent that supplied +// another user's task_id/workflow_id was served their data. +// +// These tests drive the same handler→service path production uses, with the +// stream context scoped by mcpscope.Resolver the way the lifecycle stream +// manager scopes it. + +type scopedOwner struct { + userID string + workspaceID string + workflowID string + taskID string + sessionID string +} + +// stubIdentityLookup stands in for *auth.Service. The real implementation is +// covered by TestIdentityForUser* in internal/auth. +type stubIdentityLookup map[string]authn.Identity + +func (s stubIdentityLookup) IdentityForUser(_ context.Context, userID string) (authn.Identity, bool) { + identity, ok := s[userID] + return identity, ok +} + +func seedOwnedTask(t *testing.T, svc *service.Service, repo *sqliterepo.Repository, suffix, ownerID string) scopedOwner { + t.Helper() + ctx := context.Background() + now := time.Now().UTC() + owned := scopedOwner{ + userID: ownerID, + workspaceID: "ws-" + suffix, + workflowID: "wf-" + suffix, + taskID: "task-" + suffix, + sessionID: "session-" + suffix, + } + require.NoError(t, repo.CreateWorkspace(ctx, &models.Workspace{ + ID: owned.workspaceID, + Name: "Workspace " + suffix, + OwnerID: ownerID, + CreatedAt: now, + UpdatedAt: now, + })) + require.NoError(t, repo.CreateWorkflow(ctx, &models.Workflow{ + ID: owned.workflowID, + WorkspaceID: owned.workspaceID, + Name: "Board " + suffix, + CreatedAt: now, + UpdatedAt: now, + })) + require.NoError(t, repo.CreateTask(ctx, &models.Task{ + ID: owned.taskID, + WorkspaceID: owned.workspaceID, + WorkflowID: owned.workflowID, + Title: "Task " + suffix, + State: v1.TaskStateInProgress, + CreatedAt: now, + UpdatedAt: now, + })) + require.NoError(t, repo.CreateTaskSession(ctx, &models.TaskSession{ + ID: owned.sessionID, + TaskID: owned.taskID, + IsPrimary: true, + State: models.TaskSessionStateRunning, + StartedAt: now, + UpdatedAt: now, + })) + _, err := svc.CreateMessage(ctx, &service.CreateMessageRequest{ + TaskSessionID: owned.sessionID, + TaskID: owned.taskID, + AuthorType: "user", + Content: "secret plan for " + suffix, + }) + require.NoError(t, err) + return owned +} + +// scopeFixture wires the production dispatch chain: MCP handlers registered on +// a real dispatcher, reached through a stream context scoped to one task's +// owner. +type scopeFixture struct { + dispatcher *ws.Dispatcher + scope func(ctx context.Context, taskID string) (context.Context, error) + userA scopedOwner + userB scopedOwner + // unowned is a workspace with an empty owner_id — created before auth was + // enabled, or since by an internal (unscoped) caller, because + // CreateWorkspace only stamps an owner for scoped callers. + unowned scopedOwner +} + +func newScopeFixture(t *testing.T, authEnabled bool) *scopeFixture { + t.Helper() + return newScopeFixtureWith(t, authEnabled, stubIdentityLookup{ + "user-a": {UserID: "user-a", Role: authn.RoleMember}, + "user-b": {UserID: "user-b", Role: authn.RoleAdmin}, + }) +} + +// newScopeFixtureWithoutAccounts models both owners having been disabled or +// deleted while their agents kept running. +func newScopeFixtureWithoutAccounts(t *testing.T) *scopeFixture { + t.Helper() + return newScopeFixtureWith(t, true, stubIdentityLookup{}) +} + +func newScopeFixtureWith(t *testing.T, authEnabled bool, identities stubIdentityLookup) *scopeFixture { + t.Helper() + svc, repo := newTestTaskService(t) + userA := seedOwnedTask(t, svc, repo, "a", "user-a") + userB := seedOwnedTask(t, svc, repo, "b", "user-b") + unowned := seedOwnedTask(t, svc, repo, "unowned", "") + + h := &Handlers{taskSvc: svc, logger: testLogger(t).WithFields()} + dispatcher := ws.NewDispatcher() + dispatcher.RegisterFunc(ws.ActionMCPListTasks, h.handleListTasks) + dispatcher.RegisterFunc(ws.ActionMCPGetTaskConversation, h.handleGetTaskConversation) + + resolver := mcpscope.NewResolver(repo, identities, func() bool { return authEnabled }, testLogger(t)) + return &scopeFixture{ + dispatcher: dispatcher, + scope: resolver.Scope, + userA: userA, + userB: userB, + unowned: unowned, + } +} + +// dispatchAs runs one MCP request over the stream of the agent session that +// belongs to streamTaskID. +func (f *scopeFixture) dispatchAs(t *testing.T, streamTaskID, action string, payload map[string]interface{}) *ws.Message { + t.Helper() + ctx, err := f.scope(context.Background(), streamTaskID) + require.NoError(t, err) + resp, err := f.dispatcher.Dispatch(ctx, makeWSMessage(t, action, payload)) + require.NoError(t, err) + require.NotNil(t, resp) + return resp +} + +// TestInSessionMCPListTasksDeniesForeignWorkflow is the regression pin for the +// leak: user A's agent naming user B's workflow_id must not be served B's +// tasks, even though the agent knows the ID. +func TestInSessionMCPListTasksDeniesForeignWorkflow(t *testing.T) { + f := newScopeFixture(t, true) + + resp := f.dispatchAs(t, f.userA.taskID, ws.ActionMCPListTasks, map[string]interface{}{ + "workflow_id": f.userB.workflowID, + }) + + assert.Equal(t, ws.MessageTypeError, resp.Type, "user A's agent must not read user B's workflow") +} + +func TestInSessionMCPListTasksAllowsOwnWorkflow(t *testing.T) { + f := newScopeFixture(t, true) + + resp := f.dispatchAs(t, f.userA.taskID, ws.ActionMCPListTasks, map[string]interface{}{ + "workflow_id": f.userA.workflowID, + }) + + require.Equal(t, ws.MessageTypeResponse, resp.Type) + var payload map[string]interface{} + require.NoError(t, json.Unmarshal(resp.Payload, &payload)) + assert.Equal(t, float64(1), payload["total"]) +} + +// TestInSessionMCPConversationDeniesForeignTask covers the read that exposes +// message content rather than task metadata. +func TestInSessionMCPConversationDeniesForeignTask(t *testing.T) { + f := newScopeFixture(t, true) + + resp := f.dispatchAs(t, f.userA.taskID, ws.ActionMCPGetTaskConversation, map[string]interface{}{ + "task_id": f.userB.taskID, + "limit": 10, + }) + + assert.Equal(t, ws.MessageTypeError, resp.Type, "user A's agent must not read user B's conversation") +} + +func TestInSessionMCPConversationAllowsOwnTask(t *testing.T) { + f := newScopeFixture(t, true) + + resp := f.dispatchAs(t, f.userA.taskID, ws.ActionMCPGetTaskConversation, map[string]interface{}{ + "task_id": f.userA.taskID, + "limit": 10, + }) + + require.Equal(t, ws.MessageTypeResponse, resp.Type) + var payload map[string]interface{} + require.NoError(t, json.Unmarshal(resp.Payload, &payload)) + assert.Equal(t, f.userA.sessionID, payload["session_id"]) + assert.Equal(t, float64(1), payload["total"]) +} + +// TestInSessionMCPUnscopedWhenAuthDisabled pins the opt-out path: with the +// auth feature off, dispatch stays unscoped exactly as it did before per-user +// scoping existed, so single-user instances keep cross-workspace access. +func TestInSessionMCPUnscopedWhenAuthDisabled(t *testing.T) { + f := newScopeFixture(t, false) + + resp := f.dispatchAs(t, f.userA.taskID, ws.ActionMCPListTasks, map[string]interface{}{ + "workflow_id": f.userB.workflowID, + }) + + assert.Equal(t, ws.MessageTypeResponse, resp.Type) +} + +// TestInSessionMCPUnownedStreamCannotReadOwnedWorkflow closes the gap where an +// agent whose own workspace has no owner was handed an identity-free context. +// The task service reads "no identity" as an internal caller and serves +// everything, so such a stream could name any user's workflow. It must reach +// only unowned rows. +func TestInSessionMCPUnownedStreamCannotReadOwnedWorkflow(t *testing.T) { + f := newScopeFixture(t, true) + + resp := f.dispatchAs(t, f.unowned.taskID, ws.ActionMCPListTasks, map[string]interface{}{ + "workflow_id": f.userB.workflowID, + }) + + assert.Equal(t, ws.MessageTypeError, resp.Type, + "an unowned-workspace agent must not read an owned workflow") +} + +// TestInSessionMCPUnownedStreamReadsOwnUnownedWorkflow is the other half: the +// pre-auth compatibility contract keeps unowned rows readable, so bounding the +// scope must not lock the agent out of its own workspace. +func TestInSessionMCPUnownedStreamReadsOwnUnownedWorkflow(t *testing.T) { + f := newScopeFixture(t, true) + + resp := f.dispatchAs(t, f.unowned.taskID, ws.ActionMCPListTasks, map[string]interface{}{ + "workflow_id": f.unowned.workflowID, + }) + + require.Equal(t, ws.MessageTypeResponse, resp.Type) + var payload map[string]interface{} + require.NoError(t, json.Unmarshal(resp.Payload, &payload)) + assert.Equal(t, float64(1), payload["total"]) +} + +// TestInSessionMCPUnownedStreamReadsOwnConversation guards the message-content +// path for the same stream. +func TestInSessionMCPUnownedStreamReadsOwnConversation(t *testing.T) { + f := newScopeFixture(t, true) + + resp := f.dispatchAs(t, f.unowned.taskID, ws.ActionMCPGetTaskConversation, map[string]interface{}{ + "task_id": f.unowned.taskID, + "limit": 10, + }) + + require.Equal(t, ws.MessageTypeResponse, resp.Type) + var payload map[string]interface{} + require.NoError(t, json.Unmarshal(resp.Payload, &payload)) + assert.Equal(t, float64(1), payload["total"]) +} + +// TestInSessionMCPDeniesWhenOwnerAccountInactive covers an admin disabling a +// user while their agent is still running: revoking sessions and PATs must also +// cut off the in-session MCP surface. +func TestInSessionMCPDeniesWhenOwnerAccountInactive(t *testing.T) { + f := newScopeFixtureWithoutAccounts(t) + + _, err := f.scope(context.Background(), f.userA.taskID) + + require.Error(t, err, "an inactive owner must deny the dispatch, not scope to them") +} + +// TestInSessionMCPIgnoresPayloadSessionForScoping is the privilege-escalation +// pin: scoping keys off the stream's own task, so naming another user's task +// in the payload cannot swap in their identity. Both requests below claim +// user B's workflow; only the one whose *stream* belongs to B is served. +func TestInSessionMCPIgnoresPayloadSessionForScoping(t *testing.T) { + f := newScopeFixture(t, true) + + fromA := f.dispatchAs(t, f.userA.taskID, ws.ActionMCPListTasks, map[string]interface{}{ + "workflow_id": f.userB.workflowID, + "task_id": f.userB.taskID, + "session_id": f.userB.sessionID, + }) + assert.Equal(t, ws.MessageTypeError, fromA.Type) + + fromB := f.dispatchAs(t, f.userB.taskID, ws.ActionMCPListTasks, map[string]interface{}{ + "workflow_id": f.userB.workflowID, + }) + assert.Equal(t, ws.MessageTypeResponse, fromB.Type) +} diff --git a/apps/backend/internal/mcp/scope/scope.go b/apps/backend/internal/mcp/scope/scope.go new file mode 100644 index 0000000000..beda4e68cc --- /dev/null +++ b/apps/backend/internal/mcp/scope/scope.go @@ -0,0 +1,162 @@ +// Package scope resolves the authenticated identity that owns an agent +// session's task, so the MCP tools an agent gets automatically inside its own +// session carry the same per-user scope an HTTP request from that user would. +// +// Why this exists: an agent can reach Kandev's MCP two ways. The external +// /mcp endpoint authenticates the caller with a personal access token, so the +// global auth middleware puts a real identity on the request context and the +// task service's authorize* checks apply. In-session MCP calls instead arrive +// over the agent's own WebSocket stream, which carries no credential — the +// dispatch context had no identity at all, which the task service reads as +// "internal caller" (event bus, pollers, office schedulers) and therefore +// serves unscoped. An agent that supplied another user's task_id or +// workflow_id was given their data. +// +// The owning user is derived from the stream's own execution +// (task → workspace → owner), never from the agent-supplied request payload, +// so an agent cannot widen its scope by naming a different session. +package scope + +import ( + "context" + "fmt" + + "go.uber.org/zap" + + "github.com/kandev/kandev/internal/auth/authn" + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/task/models" +) + +// TaskOwnerLookup reads the rows needed to find a task's owning user. +// Implemented by the task SQLite repository. +type TaskOwnerLookup interface { + GetTask(ctx context.Context, id string) (*models.Task, error) + GetWorkspace(ctx context.Context, id string) (*models.Workspace, error) +} + +// IdentityLookup resolves a stored user ID to the real identity that user +// carries on an authenticated request. Implemented by *auth.Service. +// +// ok=false means "no active account for this user" — the account is missing or +// has been disabled. Callers must deny rather than substitute an identity. +type IdentityLookup interface { + IdentityForUser(ctx context.Context, userID string) (authn.Identity, bool) +} + +// unownedScopeUserID is the owner attached when the stream's own workspace has +// no owner: workspaces created before auth was enabled, and any created since +// by an internal (unscoped) caller, since CreateWorkspace only stamps an owner +// for scoped callers. +// +// Such a stream must not fall back to a no-identity context — the task service +// reads that as an internal caller and grants everything. Instead we scope to +// an ID no stored account can hold. The service's visibility rule is "an empty +// owner_id, or my own user ID", so this yields exactly the intended reach: the +// unowned rows that are public by the pre-auth compatibility contract, and no +// owned row belonging to anyone else. The colon keeps it from colliding with a +// UUID or with the default-user row. +const unownedScopeUserID = "kandev:unowned-workspace" + +// Resolver attaches task-owner identity to in-session MCP dispatch contexts. +type Resolver struct { + tasks TaskOwnerLookup + identities IdentityLookup + // enforced reports whether per-user authentication is on. Kept as a + // callback because the auth mode is resolved at runtime (feature toggle) + // rather than fixed at wiring time. + enforced func() bool + logger *logger.Logger +} + +// NewResolver builds the resolver. enforced must report false while +// authentication is disabled so single-user instances keep today's behavior; +// identities must be non-nil for any enforced use, or every dispatch is denied. +func NewResolver( + tasks TaskOwnerLookup, + identities IdentityLookup, + enforced func() bool, + log *logger.Logger, +) *Resolver { + return &Resolver{ + tasks: tasks, + identities: identities, + enforced: enforced, + logger: log.WithFields(zap.String("component", "mcp-scope")), + } +} + +// Scope returns ctx carrying the identity of the user who owns taskID. +// +// The only case that stays fully unscoped is authentication being disabled, +// which preserves single-user behavior exactly. Under enforced auth every +// dispatch is scoped to somebody: +// +// - a resolvable owner with an active account → that user's real identity +// - no owner (unowned workspace, or a task with no workspace) → +// unownedScopeUserID, which reaches unowned rows and nothing else +// - anything unresolvable (missing task/workspace row, lookup failure, or an +// owner whose account is gone or disabled) → an error, which the caller +// turns into a denied dispatch +// +// The last case is deliberate: returning ctx unchanged would hand the agent an +// identity-free context, which the task service reads as an internal caller and +// serves unscoped — the exact bug this package closes. "We cannot tell who owns +// this stream" must deny foreign data, not grant all of it. +func (r *Resolver) Scope(ctx context.Context, taskID string) (context.Context, error) { + if r == nil || taskID == "" || r.enforced == nil || !r.enforced() { + return ctx, nil + } + // A context that already carries an identity came from a credentialed + // caller; never replace or widen it. + if _, ok := authn.IdentityFromContext(ctx); ok { + return ctx, nil + } + ownerID, err := r.ownerOf(ctx, taskID) + if err != nil { + return nil, err + } + if ownerID == "" { + r.logger.Warn("in-session MCP stream has no workspace owner; limiting it to unowned rows", + zap.String("task_id", taskID)) + return authn.WithIdentity(ctx, authn.Identity{UserID: unownedScopeUserID, Role: authn.RoleMember}), nil + } + if r.identities == nil { + return nil, fmt.Errorf("resolve owner of task %s: no identity lookup wired", taskID) + } + // A disabled or deleted account must not keep serving its agent: disabling + // a user revokes their sessions and PATs, so an in-flight agent session is + // the one remaining way in. Deny instead of minting an identity for them. + identity, ok := r.identities.IdentityForUser(ctx, ownerID) + if !ok { + return nil, fmt.Errorf("resolve owner of task %s: user %s has no active account", taskID, ownerID) + } + return authn.WithIdentity(ctx, identity), nil +} + +// ownerOf returns the user ID owning taskID's workspace, or "" when the task +// has no workspace or that workspace has no owner. +// +// A missing task or workspace row is an error, not an empty owner: an agent +// whose task or workspace was deleted mid-session must be denied rather than +// fall through to unscoped access. +func (r *Resolver) ownerOf(ctx context.Context, taskID string) (string, error) { + task, err := r.tasks.GetTask(ctx, taskID) + if err != nil { + return "", fmt.Errorf("resolve owner of task %s: %w", taskID, err) + } + if task == nil { + return "", fmt.Errorf("resolve owner of task %s: task not found", taskID) + } + if task.WorkspaceID == "" { + return "", nil + } + workspace, err := r.tasks.GetWorkspace(ctx, task.WorkspaceID) + if err != nil { + return "", fmt.Errorf("resolve owner of workspace %s: %w", task.WorkspaceID, err) + } + if workspace == nil { + return "", fmt.Errorf("resolve owner of workspace %s: workspace not found", task.WorkspaceID) + } + return workspace.OwnerID, nil +} diff --git a/apps/backend/internal/mcp/scope/scope_test.go b/apps/backend/internal/mcp/scope/scope_test.go new file mode 100644 index 0000000000..6b6a1a3330 --- /dev/null +++ b/apps/backend/internal/mcp/scope/scope_test.go @@ -0,0 +1,287 @@ +package scope + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/google/uuid" + + "github.com/kandev/kandev/internal/auth/authn" + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/task/models" + userstore "github.com/kandev/kandev/internal/user/store" +) + +type fakeTaskOwnerLookup struct { + tasks map[string]*models.Task + workspaces map[string]*models.Workspace + taskErr error + wsErr error +} + +func (f *fakeTaskOwnerLookup) GetTask(_ context.Context, id string) (*models.Task, error) { + if f.taskErr != nil { + return nil, f.taskErr + } + return f.tasks[id], nil +} + +func (f *fakeTaskOwnerLookup) GetWorkspace(_ context.Context, id string) (*models.Workspace, error) { + if f.wsErr != nil { + return nil, f.wsErr + } + return f.workspaces[id], nil +} + +type fakeIdentityLookup map[string]authn.Identity + +func (f fakeIdentityLookup) IdentityForUser(_ context.Context, userID string) (authn.Identity, bool) { + identity, ok := f[userID] + return identity, ok +} + +func testLogger(t *testing.T) *logger.Logger { + t.Helper() + log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"}) + if err != nil { + t.Fatalf("logger: %v", err) + } + return log +} + +// ownedFixture is one task in one workspace owned by "user-a". +func ownedFixture() *fakeTaskOwnerLookup { + return &fakeTaskOwnerLookup{ + tasks: map[string]*models.Task{"task-1": {ID: "task-1", WorkspaceID: "ws-1"}}, + workspaces: map[string]*models.Workspace{"ws-1": {ID: "ws-1", OwnerID: "user-a"}}, + } +} + +func newResolver(t *testing.T, tasks TaskOwnerLookup, identities IdentityLookup, enforced bool) *Resolver { + t.Helper() + return NewResolver(tasks, identities, func() bool { return enforced }, testLogger(t)) +} + +func identityOf(t *testing.T, ctx context.Context) authn.Identity { + t.Helper() + identity, ok := authn.IdentityFromContext(ctx) + if !ok { + t.Fatal("expected an identity on the scoped context") + } + return identity +} + +func TestScopeAttachesRealOwnerIdentity(t *testing.T) { + identities := fakeIdentityLookup{"user-a": {UserID: "user-a", Role: authn.RoleAdmin}} + r := newResolver(t, ownedFixture(), identities, true) + + ctx, err := r.Scope(context.Background(), "task-1") + if err != nil { + t.Fatalf("Scope: %v", err) + } + + identity := identityOf(t, ctx) + if identity.UserID != "user-a" { + t.Errorf("UserID = %q, want user-a", identity.UserID) + } + if identity.Role != authn.RoleAdmin { + t.Errorf("Role = %q, want the owner's stored role (admin)", identity.Role) + } + if identity.Synthetic { + t.Error("identity must be real, not synthetic — synthetic reads as unscoped") + } +} + +// TestScopeNoOpWhenAuthDisabled pins the opt-out path: single-user instances +// keep the pre-auth unscoped dispatch. +func TestScopeNoOpWhenAuthDisabled(t *testing.T) { + r := newResolver(t, ownedFixture(), fakeIdentityLookup{}, false) + + ctx, err := r.Scope(context.Background(), "task-1") + if err != nil { + t.Fatalf("Scope: %v", err) + } + if _, ok := authn.IdentityFromContext(ctx); ok { + t.Error("auth disabled must leave the context unscoped") + } +} + +// TestScopeUnownedWorkspaceStaysBounded covers a workspace with no owner — +// created before auth was enabled, or since by an internal (unscoped) caller. +// Such rows stay publicly visible by the compatibility contract, but the stream +// must NOT be handed an identity-free context: that reads as an internal caller +// and grants every user's data. It gets an ID no account can hold, so the +// service's "empty owner_id or my own ID" rule limits it to unowned rows. +func TestScopeUnownedWorkspaceStaysBounded(t *testing.T) { + lookup := ownedFixture() + lookup.workspaces["ws-1"].OwnerID = "" + r := newResolver(t, lookup, fakeIdentityLookup{}, true) + + ctx, err := r.Scope(context.Background(), "task-1") + if err != nil { + t.Fatalf("Scope: %v", err) + } + + identity := identityOf(t, ctx) + if identity.UserID != unownedScopeUserID { + t.Errorf("UserID = %q, want the unowned sentinel %q", identity.UserID, unownedScopeUserID) + } + if identity.Synthetic { + t.Error("the sentinel must not be synthetic — synthetic reads as unscoped") + } + if identity.Role == authn.RoleAdmin { + t.Error("the sentinel must not carry the admin role") + } +} + +// TestScopeTaskWithoutWorkspaceStaysBounded is the same reasoning for a task +// that has no workspace at all. +func TestScopeTaskWithoutWorkspaceStaysBounded(t *testing.T) { + lookup := ownedFixture() + lookup.tasks["task-1"].WorkspaceID = "" + r := newResolver(t, lookup, fakeIdentityLookup{}, true) + + ctx, err := r.Scope(context.Background(), "task-1") + if err != nil { + t.Fatalf("Scope: %v", err) + } + + if got := identityOf(t, ctx).UserID; got != unownedScopeUserID { + t.Errorf("UserID = %q, want the unowned sentinel %q", got, unownedScopeUserID) + } +} + +// TestUnownedScopeUserIDCannotBeAStoredAccount pins the sentinel's one +// requirement: it must not collide with a real user ID (UUIDs, or the +// default-user row promoted by the setup wizard). +func TestUnownedScopeUserIDCannotBeAStoredAccount(t *testing.T) { + if unownedScopeUserID == "" { + t.Fatal("the sentinel must be non-empty, or callerScope reports unscoped") + } + if !strings.Contains(unownedScopeUserID, ":") { + t.Errorf("sentinel %q should keep a colon so it cannot be a UUID", unownedScopeUserID) + } + if _, err := uuid.Parse(unownedScopeUserID); err == nil { + t.Errorf("sentinel %q parses as a UUID and could collide with a real account", unownedScopeUserID) + } + if unownedScopeUserID == userstore.DefaultUserID { + t.Error("sentinel collides with the default-user row") + } +} + +func TestScopeNoOpForEmptyTaskID(t *testing.T) { + r := newResolver(t, ownedFixture(), fakeIdentityLookup{}, true) + + ctx, err := r.Scope(context.Background(), "") + if err != nil { + t.Fatalf("Scope: %v", err) + } + if _, ok := authn.IdentityFromContext(ctx); ok { + t.Error("no task ID means no owner to resolve") + } +} + +// TestScopePreservesExistingIdentity guards the external /mcp path and any +// other credentialed caller: an identity already on the context is never +// replaced or widened. +func TestScopePreservesExistingIdentity(t *testing.T) { + identities := fakeIdentityLookup{"user-a": {UserID: "user-a", Role: authn.RoleAdmin}} + r := newResolver(t, ownedFixture(), identities, true) + caller := authn.Identity{UserID: "user-b", Role: authn.RoleMember, TokenID: "pat-1"} + + ctx, err := r.Scope(authn.WithIdentity(context.Background(), caller), "task-1") + if err != nil { + t.Fatalf("Scope: %v", err) + } + + if got := identityOf(t, ctx); got != caller { + t.Errorf("identity = %+v, want the original caller %+v", got, caller) + } +} + +// TestScopeFailsClosedOnTaskLookupError is the fail-closed pin: if we cannot +// tell who owns the stream, denying the dispatch is correct — returning an +// unscoped context would grant every user's data. +func TestScopeFailsClosedOnTaskLookupError(t *testing.T) { + lookup := ownedFixture() + lookup.taskErr = errors.New("db unavailable") + r := newResolver(t, lookup, fakeIdentityLookup{}, true) + + if _, err := r.Scope(context.Background(), "task-1"); err == nil { + t.Fatal("expected an error so the dispatch is denied") + } +} + +func TestScopeFailsClosedOnWorkspaceLookupError(t *testing.T) { + lookup := ownedFixture() + lookup.wsErr = errors.New("db unavailable") + r := newResolver(t, lookup, fakeIdentityLookup{}, true) + + if _, err := r.Scope(context.Background(), "task-1"); err == nil { + t.Fatal("expected an error so the dispatch is denied") + } +} + +// TestScopeDeniesWhenOwnerAccountInactive covers a deleted or disabled owner. +// Disabling a user revokes their sessions and PATs, so a still-running agent +// session would otherwise be the one remaining way into their workspace — +// deny the dispatch instead of minting an identity on their behalf. +func TestScopeDeniesWhenOwnerAccountInactive(t *testing.T) { + r := newResolver(t, ownedFixture(), fakeIdentityLookup{}, true) + + if _, err := r.Scope(context.Background(), "task-1"); err == nil { + t.Fatal("expected an error so the dispatch is denied for an inactive owner") + } +} + +// TestScopeFailsClosedOnMissingTaskRow and its workspace sibling cover a +// lookup that reports "not found" as (nil, nil) rather than an error. An +// unscoped context here would let a stream whose task was deleted mid-session +// read everything. +func TestScopeFailsClosedOnMissingTaskRow(t *testing.T) { + lookup := &fakeTaskOwnerLookup{ + tasks: map[string]*models.Task{}, + workspaces: map[string]*models.Workspace{}, + } + r := newResolver(t, lookup, fakeIdentityLookup{}, true) + + if _, err := r.Scope(context.Background(), "task-gone"); err == nil { + t.Fatal("expected an error for a missing task row") + } +} + +func TestScopeFailsClosedOnMissingWorkspaceRow(t *testing.T) { + lookup := ownedFixture() + delete(lookup.workspaces, "ws-1") + r := newResolver(t, lookup, fakeIdentityLookup{}, true) + + if _, err := r.Scope(context.Background(), "task-1"); err == nil { + t.Fatal("expected an error for a missing workspace row") + } +} + +// TestScopeFailsClosedWithoutIdentityLookup guards the wiring: a resolver with +// no identity lookup must deny, not fall back to unscoped. +func TestScopeFailsClosedWithoutIdentityLookup(t *testing.T) { + r := NewResolver(ownedFixture(), nil, func() bool { return true }, testLogger(t)) + + if _, err := r.Scope(context.Background(), "task-1"); err == nil { + t.Fatal("expected an error when no identity lookup is wired") + } +} + +// TestScopeNoOpWithoutEnforcedCallback keeps a partially wired resolver from +// scoping (and therefore from denying) anything. +func TestScopeNoOpWithoutEnforcedCallback(t *testing.T) { + r := NewResolver(ownedFixture(), fakeIdentityLookup{}, nil, testLogger(t)) + + ctx, err := r.Scope(context.Background(), "task-1") + if err != nil { + t.Fatalf("Scope: %v", err) + } + if _, ok := authn.IdentityFromContext(ctx); ok { + t.Error("a resolver with no enforcement check must stay a no-op") + } +} diff --git a/apps/backend/internal/task/service/service_access.go b/apps/backend/internal/task/service/service_access.go index dc63669c92..4f773e1913 100644 --- a/apps/backend/internal/task/service/service_access.go +++ b/apps/backend/internal/task/service/service_access.go @@ -13,8 +13,10 @@ import ( // Every user-facing service entry point calls one of the authorize* helpers. // Scoping keys off the request-context identity placed there by the auth // middleware: -// - no identity in ctx → internal caller (event bus, pollers, MCP stream, -// office schedulers) → unscoped, exactly as before the auth feature +// - no identity in ctx → internal caller (event bus, pollers, office +// schedulers) → unscoped, exactly as before the auth feature. In-session +// agent MCP calls used to land here too; they now arrive with the task +// owner's identity attached by internal/mcp/scope. // - synthetic identity → auth disabled → unscoped (today's behavior) // - real identity → workspaces are visible only to their owner; // rows with an empty owner_id (created pre-auth) stay visible to everyone diff --git a/docs/decisions/2026-07-24-opt-in-authentication.md b/docs/decisions/2026-07-24-opt-in-authentication.md index 77dc84fadf..56d31fe9ec 100644 --- a/docs/decisions/2026-07-24-opt-in-authentication.md +++ b/docs/decisions/2026-07-24-opt-in-authentication.md @@ -25,9 +25,19 @@ a login screen or any behavioral change. 2. **Synthetic identity in disabled mode.** The global HTTP middleware (and WS gateway) inject `Identity{default-user, admin, Synthetic}` when auth is off. Downstream code branches on identity, never on mode — internal - callers (event bus, pollers, MCP stream) carry no identity and are + callers (event bus, pollers, office schedulers) carry no identity and are unscoped. This keeps disabled-mode behavior byte-identical and makes every - consumer identity-aware in one step. + consumer identity-aware in one step. In-session agent MCP calls arrive over + the agent's credential-less WebSocket stream, so they initially fell into + the same unscoped bucket; `internal/mcp/scope` now resolves the stream's + task owner and attaches that real identity before dispatch (the owning task + comes from the `AgentExecution`, never from the agent-supplied payload). + Because "no identity" means full access, that path never returns an + identity-free context under enforced auth: an unowned workspace is scoped to + a sentinel user ID that reaches unowned rows only, and an unresolvable owner + — deleted task/workspace row, or an account since deleted or disabled — + denies the dispatch. Disabling a user revokes their sessions and PATs, so + their still-running agent session must lose this surface too. 3. **Opaque DB-backed credentials, not JWTs.** Sessions (`kandev_session` HttpOnly SameSite=Lax cookie) and personal access tokens (`kandev_pat_*`) are 256-bit random values stored as SHA-256 digests. Instant revocation