-
Notifications
You must be signed in to change notification settings - Fork 7
feat(mcp): delegated per-user token resolution for type=user (#317) #327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| package mcp | ||
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "errors" | ||
| "io" | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "sync/atomic" | ||
| "testing" | ||
|
|
||
| "github.com/initializ/forge/forge-core/auth" | ||
| "github.com/initializ/forge/forge-core/types" | ||
| ) | ||
|
|
||
| // delegatedServer is a fake platform token endpoint for the type=user | ||
| // path (#317): it echoes the requested subject into the token, counts | ||
| // calls, and can 403 a named subject to simulate "no grant yet". | ||
| func delegatedServer(t *testing.T, calls *atomic.Int32, denySubject string) *httptest.Server { | ||
| t.Helper() | ||
| return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if calls != nil { | ||
| calls.Add(1) | ||
| } | ||
| body, _ := io.ReadAll(r.Body) | ||
| var in struct{ Server, Subject string } | ||
| _ = json.Unmarshal(body, &in) | ||
| if in.Subject == "" { | ||
| t.Errorf("delegated request missing subject: %s", body) | ||
| } | ||
| if in.Subject == denySubject { | ||
| w.WriteHeader(http.StatusForbidden) | ||
| return | ||
| } | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _ = json.NewEncoder(w).Encode(map[string]any{ | ||
| "access_token": "tok-for-" + in.Subject, | ||
| "expires_in": 3600, | ||
| }) | ||
| })) | ||
| } | ||
|
|
||
| // TestDelegatedTokenSource_PerSubjectFetchAndCache: distinct users get | ||
| // distinct tokens (subject sent in the body), and the same user's token is | ||
| // cached — one fetch per subject. | ||
| func TestDelegatedTokenSource_PerSubjectFetchAndCache(t *testing.T) { | ||
| var calls atomic.Int32 | ||
| srv := delegatedServer(t, &calls, "") | ||
| defer srv.Close() | ||
|
|
||
| d := newDelegatedTokenSource(PlatformSourceConfig{ | ||
| TokenEndpoint: srv.URL, AgentIdentity: "agent-cred", Ref: "atlassian", HTTPClient: srv.Client(), | ||
| }) | ||
| ctx := context.Background() | ||
|
|
||
| a, err := d.TokenForSubject(ctx, "alice@corp.com") | ||
| if err != nil || a != "tok-for-alice@corp.com" { | ||
| t.Fatalf("alice: %q %v", a, err) | ||
| } | ||
| b, err := d.TokenForSubject(ctx, "bob@corp.com") | ||
| if err != nil || b != "tok-for-bob@corp.com" { | ||
| t.Fatalf("bob: %q %v", b, err) | ||
| } | ||
| if a == b { | ||
| t.Fatal("distinct users must get distinct tokens") | ||
| } | ||
| // alice again — served from the per-subject cache, no new fetch. | ||
| if _, err := d.TokenForSubject(ctx, "alice@corp.com"); err != nil { | ||
| t.Fatalf("alice cached: %v", err) | ||
| } | ||
| if n := calls.Load(); n != 2 { | ||
| t.Errorf("platform hit %d times, want 2 (one per distinct subject; alice cached)", n) | ||
| } | ||
| } | ||
|
|
||
| // TestDelegatedTokenSource_NoGrantIsAuthRequired: a platform 403 for a | ||
| // subject (no grant yet) surfaces as ErrNoToken — auth-required, lazy — | ||
| // not a hard error. | ||
| func TestDelegatedTokenSource_NoGrantIsAuthRequired(t *testing.T) { | ||
| srv := delegatedServer(t, nil, "carol@corp.com") | ||
| defer srv.Close() | ||
| d := newDelegatedTokenSource(PlatformSourceConfig{ | ||
| TokenEndpoint: srv.URL, AgentIdentity: "c", Ref: "r", HTTPClient: srv.Client(), | ||
| }) | ||
| if _, err := d.TokenForSubject(context.Background(), "carol@corp.com"); !errors.Is(err, ErrNoToken) { | ||
| t.Fatalf("no grant must be ErrNoToken (auth-required), got: %v", err) | ||
| } | ||
| } | ||
|
|
||
| // TestBuildAuthFn_User_ResolvesSubjectFromContext: the type=user authFn | ||
| // pulls the requesting user from the request ctx and resolves that user's | ||
| // token; with no user in ctx it fails lazily with ErrNoToken. | ||
| func TestBuildAuthFn_User_ResolvesSubjectFromContext(t *testing.T) { | ||
| srv := delegatedServer(t, nil, "") | ||
| defer srv.Close() | ||
|
|
||
| spec := types.MCPServer{ | ||
| Name: "atlassian-read", URL: "https://mcp.atlassian.com/mcp", | ||
| Auth: &types.MCPAuth{Type: "user", Ref: "mcp.atlassian"}, | ||
| } | ||
| deps := ServerDeps{ | ||
| HTTPClient: srv.Client(), | ||
| Platform: &types.PlatformConfig{TokenEndpoint: srv.URL, AgentIdentity: "agent-cred"}, | ||
| } | ||
| authFn := buildAuthFn(spec, deps) | ||
|
|
||
| // With an authenticated user in ctx → that user's token. | ||
| ctx := auth.WithIdentity(context.Background(), &auth.Identity{Email: "dave@corp.com"}) | ||
| tok, err := authFn(ctx) | ||
| if err != nil || tok != "tok-for-dave@corp.com" { | ||
| t.Fatalf("resolved token = %q err=%v, want dave's token", tok, err) | ||
| } | ||
|
|
||
| // No user in ctx → lazy auth-required. | ||
| if _, err := authFn(context.Background()); !errors.Is(err, ErrNoToken) { | ||
| t.Fatalf("no user in ctx must fail lazily with ErrNoToken, got: %v", err) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,58 +78,160 @@ func (p *platformTokenSource) Token(ctx context.Context) (string, error) { | |
| } | ||
|
|
||
| func (p *platformTokenSource) fetch(ctx context.Context) (string, time.Duration, error) { | ||
| endpoint := expandEnvVars(p.endpoint) | ||
| identity := expandEnvVars(p.identity) | ||
| // Agent-principal: no subject in the body (§19 contract). | ||
| tok, ttl, status, err := doPlatformTokenRequest(ctx, p.client, p.endpoint, p.identity, p.ref, "") | ||
| if err != nil { | ||
| return "", 0, err | ||
| } | ||
| if status != http.StatusOK { | ||
| return "", 0, fmt.Errorf("%w: platform token endpoint returned %d for server %q", ErrProtocolError, status, p.ref) | ||
| } | ||
| return tok, ttl, nil | ||
| } | ||
|
|
||
| // doPlatformTokenRequest POSTs the platform token endpoint (§19 contract) | ||
| // and returns the access token + TTL on 200, or the raw status on non-200 | ||
| // so the caller can classify (agent-principal vs delegated read a non-200 | ||
| // differently). Only transport / parse problems return a non-nil err. The | ||
| // endpoint + identity are ${VAR}-expanded at request time so a rotated pod | ||
| // secret applies without a restart. | ||
| // | ||
| // Body: {"server": ref} for agent-principal; {"server": ref, "subject": s} | ||
| // for the delegated (per-user) path. access_token only — any refresh_token | ||
| // is ignored (invariant 8). | ||
| func doPlatformTokenRequest(ctx context.Context, client *http.Client, rawEndpoint, rawIdentity, ref, subject string) (token string, ttl time.Duration, status int, err error) { | ||
| endpoint := expandEnvVars(rawEndpoint) | ||
| identity := expandEnvVars(rawIdentity) | ||
| if endpoint == "" { | ||
| return "", 0, fmt.Errorf("%w: platform.token_endpoint is empty (or its env var is unset)", ErrProtocolError) | ||
| return "", 0, 0, fmt.Errorf("%w: platform.token_endpoint is empty (or its env var is unset)", ErrProtocolError) | ||
| } | ||
| if identity == "" { | ||
| return "", 0, fmt.Errorf("%w: platform.agent_identity is empty (or its env var is unset) — the platform did not materialize the agent credential", ErrProtocolError) | ||
| return "", 0, 0, fmt.Errorf("%w: platform.agent_identity is empty (or its env var is unset) — the platform did not materialize the agent credential", ErrProtocolError) | ||
| } | ||
|
|
||
| body, err := json.Marshal(map[string]string{"server": p.ref}) | ||
| payload := map[string]string{"server": ref} | ||
| if subject != "" { | ||
| payload["subject"] = subject | ||
| } | ||
| body, err := json.Marshal(payload) | ||
| if err != nil { | ||
| return "", 0, err | ||
| return "", 0, 0, err | ||
| } | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint, strings.NewReader(string(body))) | ||
| if err != nil { | ||
| return "", 0, err | ||
| return "", 0, 0, err | ||
| } | ||
| req.Header.Set("Content-Type", "application/json") | ||
| req.Header.Set("Accept", "application/json") | ||
| req.Header.Set("Authorization", "Bearer "+identity) | ||
|
|
||
| client := p.client | ||
| if client == nil { | ||
| client = &http.Client{Timeout: 15 * time.Second} | ||
| } | ||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| return "", 0, fmt.Errorf("%w: platform token request failed: %v", ErrTransportUnavailable, err) | ||
| return "", 0, 0, fmt.Errorf("%w: platform token request failed: %v", ErrTransportUnavailable, err) | ||
| } | ||
| defer func() { _ = resp.Body.Close() }() | ||
| data, _ := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) | ||
| if resp.StatusCode != http.StatusOK { | ||
| return "", 0, fmt.Errorf("%w: platform token endpoint returned %d for server %q: %s", | ||
| ErrProtocolError, resp.StatusCode, p.ref, strings.TrimSpace(string(data))) | ||
| return "", 0, resp.StatusCode, nil // caller classifies the non-200 | ||
| } | ||
| var out struct { | ||
| AccessToken string `json:"access_token"` | ||
| ExpiresIn int `json:"expires_in"` | ||
| // A refresh_token here would violate invariant 8 — the platform | ||
| // must never send one; if it does, it is deliberately ignored. | ||
| } | ||
| if err := json.Unmarshal(data, &out); err != nil { | ||
| return "", 0, fmt.Errorf("%w: parsing platform token response: %v", ErrProtocolError, err) | ||
| return "", 0, resp.StatusCode, fmt.Errorf("%w: parsing platform token response: %v", ErrProtocolError, err) | ||
| } | ||
| if out.AccessToken == "" { | ||
| return "", 0, fmt.Errorf("%w: platform token response carried no access_token", ErrProtocolError) | ||
| return "", 0, resp.StatusCode, fmt.Errorf("%w: platform token response carried no access_token", ErrProtocolError) | ||
| } | ||
| ttl := defaultPlatformTokenTTL | ||
| ttl = defaultPlatformTokenTTL | ||
| if out.ExpiresIn > 0 { | ||
| ttl = time.Duration(out.ExpiresIn) * time.Second | ||
| } | ||
| return out.AccessToken, ttl, nil | ||
| return out.AccessToken, ttl, resp.StatusCode, nil | ||
| } | ||
|
|
||
| // delegatedTokenSource is the managed resolver for auth.type=user (#317): | ||
| // a per-REQUESTING-USER access token from the platform token endpoint | ||
| // (delegated body {server, subject}), cached per subject so distinct users | ||
| // never share a token. A platform 401/403/404 means "no grant for this | ||
| // user yet" → ErrNoToken (auth-required), keeping the server lazy and | ||
| // non-blocking until the platform-side consent flow produces the grant. | ||
| // The refresh token never reaches the agent (invariant 8). | ||
| type delegatedTokenSource struct { | ||
| endpoint string | ||
| identity string | ||
| ref string | ||
| client *http.Client | ||
|
|
||
| mu sync.Mutex | ||
| cache map[string]cachedToken // key: subject | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: this per-subject cache is unbounded and never evicts. Entries are added per distinct user and only read-checked for expiry — expired entries are never deleted, and there's no size cap. A long-running agent serving many distinct users grows the map with distinct-subject cardinality (a slow leak). Cheap fix:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bounded it: evict the stale entry on the fast-path read, and sweep expired entries on each write — so one-shot users can't grow the map unbounded and tokens aren't held past use, with no background goroutine. Per-subject singleflight (finding 3) I've left as the noted follow-up. |
||
| } | ||
|
|
||
| type cachedToken struct { | ||
| token string | ||
| expiresAt time.Time | ||
| } | ||
|
|
||
| func newDelegatedTokenSource(cfg PlatformSourceConfig) *delegatedTokenSource { | ||
| return &delegatedTokenSource{ | ||
| endpoint: cfg.TokenEndpoint, | ||
| identity: cfg.AgentIdentity, | ||
| ref: cfg.Ref, | ||
| client: cfg.HTTPClient, | ||
| } | ||
| } | ||
|
|
||
| // TokenForSubject returns a valid access token for the requesting user, | ||
| // fetching from the platform when the per-subject cache is empty/expiring. | ||
| func (d *delegatedTokenSource) TokenForSubject(ctx context.Context, subject string) (string, error) { | ||
| if subject == "" { | ||
| return "", fmt.Errorf("%w: delegated token requires a requesting-user subject", ErrNoToken) | ||
| } | ||
| // Fast path: a cached, unexpired token for this subject. The lock is | ||
| // NOT held across the network fetch, so a slow fetch for user A never | ||
| // blocks user B — the multi-user path is the whole point of #317. | ||
| d.mu.Lock() | ||
| if c, ok := d.cache[subject]; ok { | ||
| if c.token != "" && time.Now().Before(c.expiresAt.Add(-platformTokenSkew)) { | ||
| d.mu.Unlock() | ||
| return c.token, nil | ||
| } | ||
| // Evict the stale entry — don't hold a sensitive token past use | ||
| // (#327 review finding 2). | ||
| delete(d.cache, subject) | ||
| } | ||
| d.mu.Unlock() | ||
|
|
||
| tok, ttl, status, err := doPlatformTokenRequest(ctx, d.client, d.endpoint, d.identity, d.ref, subject) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| if status != http.StatusOK { | ||
| if status == http.StatusUnauthorized || status == http.StatusForbidden || status == http.StatusNotFound { | ||
| return "", fmt.Errorf("%w: no platform grant for subject %q on server %q yet — awaiting the delegated consent flow (#317)", ErrNoToken, subject, d.ref) | ||
| } | ||
| return "", fmt.Errorf("%w: platform token endpoint returned %d for server %q (subject %q)", ErrProtocolError, status, d.ref, subject) | ||
| } | ||
|
|
||
| d.mu.Lock() | ||
| if d.cache == nil { | ||
| d.cache = map[string]cachedToken{} | ||
| } | ||
| // Opportunistic sweep so the map can't grow unbounded with one-shot | ||
| // users' stale tokens — a background-free TTL bound (#327 review). | ||
| now := time.Now() | ||
| for s, c := range d.cache { | ||
| if !now.Before(c.expiresAt) { | ||
| delete(d.cache, s) | ||
| } | ||
| } | ||
| d.cache[subject] = cachedToken{token: tok, expiresAt: now.Add(ttl)} | ||
| d.mu.Unlock() | ||
| return tok, nil | ||
| } | ||
|
|
||
| // expandEnvVars resolves ${VAR}/$VAR against the process env at USE time — | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Medium (isolation-boundary clarity): "each user gets their own token" needs the connection-sharing caveat.
This is true at the token level — but the PR's own scope note says the per-user connection lifecycle is a follow-up: MCP
initializeruns at connection setup, and today the per-request token rides a shared connection. Against a session-stateful MCP server (identity bound atinitialize, not re-checked per call), the per-call token swap may not re-scope the session, so users could effectively act as the connection-establisher.An operator reads the docs, not the PR body, and "each user gets their own token" reads as complete isolation. Suggest adding: "This resolves a per-user token on each call; the underlying MCP connection is currently shared. Full per-subject connection isolation is a follow-up — for a session-stateful MCP server, confirm it re-authorizes per call before relying on this for isolation." Keeps operators from over-trusting isolation this slice doesn't yet fully provide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the caveat. The docs now spell out that this is per-user token-level isolation per call, the connection is shared, and a session-stateful server may not re-scope on the per-call token — confirm per-request re-auth before relying on it for hard isolation. This is exactly what the per-user connection lifecycle follow-up closes (starting on it next).