Repository navigation
Let an embedder choose the upstream credential for each call - #187
Conversation
WithUpstreamCredentials asks the embedder for the credential on every upstream contact (list, call, forward) and again just before a call executes, so a held call released later uses the credential as it stands then. Errors reach the caller instead of falling back, and with the option set the stored OAuth overlay never applies. Upstream MCP sessions are keyed by the credential's session key, so callers never share one, and a 401 clears that session and tells the credential's owner first.
A 401 usually has no JSON-RPC body, so a call or tool listing that met one failed with "unexpected end of JSON input". It now says the upstream rejected the credential (HTTP 401).
… read A harness lists a server's tools and reads its instructions before calling anything, and an error there is swallowed: Claude Code showed the server connected with no tools and never said why. The credential function can now return an UpstreamUnavailable: a tools/list is answered with the tools it carries, an initialize adds its instructions, and any other contact fails with its message. The upstream is never contacted. A new initialize purpose lets the embedder look the caller up when a session starts.
cachafla
left a comment
There was a problem hiding this comment.
Nice work on this 👏 The provider design reads well, and the execute re-resolve does the right thing for held calls. I left three things I'd fix before we tag it:
1. ReportStuck fails for a caller with no connected account (internal/invocation/hooks.go:278)
ReportStuck calls ResolveContext(ctx, server) with no purpose, so it defaults to PurposeCall. It only needs the server's name and never contacts the upstream. With WithUpstreamCredentials set, AA refuses a caller with no connected account (an error or UpstreamUnavailable), and ReportStuck turns that into "unknown server". So the person who's stuck can't report it, and we ask the embedder for a token (maybe renewing it) for nothing.
I'd suggest looking the server up without going through credential() here, or resolving with a purpose that skips the credential (PurposeInitialize would work).
2. The provider also runs for stdio servers (internal/mcp/client.go:349)
Resolver.credential() calls the provider for every upstream, including ones with a Command and no BaseURL. The changelog and the option's doc say "every path that resolves an HTTP upstream". An embedder that refuses callers with no connected account would also block local servers that don't need a token at all.
I'd suggest skipping the provider when BaseURL is empty. Alternatively, document that it covers every server and add the transport to UpstreamContact so the embedder can tell them apart.
3. Notice adds a credential lookup on every initialize, even without the option (internal/api/handlers.go:1789, internal/invocation/service.go:2095)
The handler calls Notice on every initialize, and Notice resolves the server whether or not a provider is set. Without the option, that goes through the RefreshingOAuthCredentialStore overlay: an extra DB read per initialize, plus an OAuth refresh against the provider when the stored token is close to expiry. That breaks the "without it, nothing changes" promise in the changelog.
I'd suggest returning "" early from Notice when no provider is set. The resolver could expose that, or the service could keep a flag.
Note: I didn't run the tests locally, this is from reading the diff and the surrounding code at 05a4e50.
# Conflicts: # CHANGELOG.md # internal/mcp/client.go
…credential From review: ReportStuck resolved the server through the credential provider, so a caller the embedder refuses (no account connected yet) couldn't report being stuck, and the embedder was asked for a token it never sent. The resolver's Lookup finds a server as configured, with no credential, and the report uses it. The provider ran for every upstream, stdio servers included, though the option's doc says HTTP: a provider refusing a caller would have blocked a local server that needs no token. It now covers upstreams with a BaseURL. Notice resolved the server on every initialize whether or not a provider was set, so without the option each session start read the stored OAuth credential and could renew it. It returns "" when no provider is set. After merging main, a cached session is keyed by tenant and credential together, and the 401 test looks it up that way.
|
Thanks, all three fixed in 3e07756, and main merged in (the session key now combines the tenant with the credential's key). 1. 2. stdio servers. The provider is asked only about upstreams with a 3.
|
cachafla
left a comment
There was a problem hiding this comment.
Thanks @kam-validmind, all three are addressed in 3e07756 👏
ReportStucknow uses the newResolver.Lookup(internal/invocation/hooks.go:280), so it never asks the provider for a credential. Covered byTestStuckReportAsksNoCredential.Resolver.credentialreturns stdio upstreams (noBaseURL) as configured before calling the provider, and the changelog says so. Covered byTestProviderSkipsStdioUpstreams.Noticereturns""whenHasProvider()is false, before any read. Covered byTestNoticeWithoutProviderReadsNothing.
CI is green (Go, Baseline). Ready for Andres's approval 🚀
The short version
Atryum sits between AI agents and MCP servers. When an agent calls a tool, Atryum forwards the call to the MCP server (the "upstream") and attaches a credential, usually a bearer token.
Today Atryum attaches the same token for every caller of a server. This PR lets the program that embeds Atryum, in our case AA, choose the token for each call, based on who is calling.
Nothing changes for anyone who doesn't use the new option.
Why AA needs this
Many hosted MCP servers, such as Shortcut's, sign people in with OAuth. You don't paste a token; each person signs in with their own account. AA wants every agent's calls to run as the right account:
That way an agent can only do what its account is allowed to do, and the server's logs show who really did what.
Atryum couldn't support that, for four reasons:
oauth_credentialsis keyed by server name), so everyone shares it.What this PR adds
1. A new option:
WithUpstreamCredentialsThe embedder gives Atryum a function. Atryum calls it every time it is about to contact an upstream, and it tells the function:
list(listing tools),call(a tool call, before the decision),forward(other MCP methods) orexecute(right before the tool runs).The function returns what to send:
BearerTokenHeadersSessionKeyOnUnauthorized401.If the function returns an error, Atryum stops and passes the error back to the agent. It never quietly falls back to some other stored token.
2. Asking again just before the tool runs
Right before Atryum runs a call (in
finishExecution), it calls the function again with the purposeexecute. This matters most for held calls:If the function refuses at this point (for example, the person disconnected their account while the call was waiting), the call is marked failed with that message, and the server never sees the request.
3. One MCP session per credential
MCP over HTTP can use a session: the server hands back an
Mcp-Session-Id, and later requests send it. Atryum caches it. Before, the cache was keyed by server name. Now it's keyed bySessionKey, which defaults to the server name, so nothing changes without the option.flowchart TD accTitle: Session cache before and after accDescr: Before, all callers shared one session per server. After, each credential has its own. subgraph Before A1["Sam's call"] --> S1["Session for 'shortcut'"] B1["Priya's call"] --> S1 end subgraph After A2["Sam's call"] --> S2["Session 'shortcut/sam'"] B2["Priya's call"] --> S3["Session 'shortcut/priya'"] endWhen the upstream answers 401 Unauthorized, Atryum now:
OnUnauthorizedbefore returning the error, so the embedder can mark the token for renewal.4. The old stored-token path is switched off when the option is set
With
WithUpstreamCredentialsset, Atryum's ownoauth_credentialslookup is never used, on any path. A leftover token in that table can't leak into a call.5. A way to say "not available to you yet"
A harness lists a server's tools, and reads its instructions, before it calls anything, and it swallows an error there. Claude Code showed a server whose listing failed as connected with no tools and never said why. So the credential function may now return an
UpstreamUnavailable{Message, Tools, Instructions}as its error:tools/listis answered with theToolsit carries (AA sends one placeholder whose description explains and links), instead of an error;initializeaddsInstructionsto the session's instructions, through a newinitializepurpose that lets the function look the caller up without sending or renewing anything;Message;Without the option, or with no
Toolsto show, nothing changes.Where to look in the code
pkg/atryum/upstream_credentials.goUpstreamContact,UpstreamCredential, the purpose constants), and the adapter that turns them into an internal provider.pkg/atryum/options.go,pkg/atryum/atryum.gointernal/mcp/purpose.gocontext, so no function signatures had to change.internal/mcp/client.goCredentialProvideron the resolver;UpstreamgainsSessionKey,OnUnauthorizedandResolvedByProvider; session cache keyed bysessionKey(); 401 handling.internal/invocation/service.gofinishExecution.internal/api/handlers.goforwardpurpose.CHANGELOG.mdUnreleasedentry.Two small behaviours to know about:
tools/listortools/call), the method isn't forwarded. That's what Atryum already did when resolving a server failed.ListAllskips a server the function refuses, instead of failing the whole list.Tests
New tests:
internal/mcp/credential_provider_test.gooauth_credentialstoken.ListAllskips that server.OnUnauthorizedexactly once, before the error comes back.internal/invocation/credential_provider_test.gocallthenexecute(with the invocation ID), and the upstream only ever sees theexecutetoken.execute, the call fails with its message and the upstream receives nothing.To run them:
The whole suite (
go test ./...) passes on top ofmain.How AA uses it
AA's PR, validmind/agent-authority#152, passes a function to
WithUpstreamCredentials. For each call it:<server>/<account id>.If no account is connected, it returns nothing for
call, so AA's decider refuses the call with a "connect your account" link, and it returns an error for the other purposes. AA's end-to-end test runs two agents through this proxy against a fake OAuth server and checks that each call ran as its own user.AA pins Atryum to a released tag, so this needs to be merged and tagged before AA's PR can merge.
How this was made
Written with Claude Code, from an AA feature request: "AA MCP doesn't seem to have an OAuth flow; in Atryum you can put in a server URL and it talks you through doing an OAuth flow." It went through a spec, a plan and a task list. The person's decisions:
This option was the smallest Atryum change that let AA do the rest itself. The full contract is in AA's
specs/001-mcp-upstream-oauth/contracts/atryum-upstream-credentials.md.