fix(auth): scope in-session agent MCP calls to the task owner (#1937)
* fix(auth): scope in-session agent MCP calls to the task owner The MCP tools an agent gets automatically inside its own session are relayed over the agent's WebSocket stream, which carries no credential. The backend dispatched them on the raw stream context, so the task service saw no identity — its signal for an internal caller (event bus, pollers, office schedulers) — and served the request unscoped. An agent that supplied another user's task_id or workflow_id was given their data, violating the rule that knowing an ID must not grant access. Resolve the owning user from the stream's own AgentExecution (task -> workspace -> owner) and attach that real stored identity before dispatch, so the existing authorize* checks apply exactly as they do on the PAT-authenticated /mcp endpoint. The owning task never comes from the agent-supplied payload: honoring a payload session_id would let an agent name another user's session and inherit their identity. Tool handlers are unchanged. Auth disabled stays unscoped, unclaimed pre-auth workspaces stay visible to everyone, and a lookup failure denies the dispatch rather than falling back to full access. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(auth): never leave in-session MCP dispatch unscoped Review follow-up. Three paths still returned an identity-free context under enforced auth, and "no identity" is exactly what the task service reads as an internal caller and serves unscoped: - Unowned workspace. CreateWorkspace only stamps an owner for scoped callers, so internal callers keep producing owner_id='' rows after setup. Those streams got full cross-user access. Scope them to a sentinel user ID no account can hold: the service's "empty owner_id or my own ID" rule then limits them to the unowned rows that are public by the compatibility contract, and nothing else. - Task with no workspace. Same treatment, same reasoning. - Missing task or workspace row. A lookup that reports not-found as (nil, nil) fell through to unscoped; treat it as a resolution failure. Also deny, rather than fabricate a member identity, when the owner's account is gone or disabled. Disabling a user revokes their sessions and PATs, so a still-running agent session was the one remaining way into their workspace. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Kandev Agent <agent@kandev.dev> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
João Salavisa
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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,
|
||||
}
|
||||
}
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
@@ -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),
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
@@ -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
|
||||
}
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user