feat: support enterprise oauth transport config for hosted mcp servers - #188
feat: support enterprise oauth transport config for hosted mcp servers#188OchnikBartek wants to merge 1 commit into
Conversation
Coverage Report for CI Build 30365584343Coverage remained the same at 100.0%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
DEENUU1
left a comment
There was a problem hiding this comment.
Thanks for this — the shape is right and you clearly read the layers underneath before writing any code. Splitting _build_oauth / _build_http_transport / _adapt_http_client_factory out keeps _build_raw_mcp_toolset readable, the scope-set namespacing is a neat fix for a cache bug that would otherwise be nearly undiagnosable, and the test that asserts the OAuth object and the transport share the same factory object is exactly the trap I was worried about. Docs are good too — env vars first, factory last.
One blocking problem: the factory never actually works over http. FastMCP's StreamableHttpTransport.connect_session calls the factory with a follow_redirects=True kwarg that the MCP SDK's McpHttpClientFactory protocol doesn't declare, so make_client raises TypeError on the first connect. I reproduced it against a dead port on this branch:
no factory : Client failed to connect: All connection attempts failed
w/ factory : Client failed to connect: _adapt_http_client_factory.<locals>.make_client() got an unexpected keyword argument 'follow_redirects'
Same for the OAuth+factory path, which is the Atlassian case this PR exists for. The sse path is fine (the SDK's sse_client sticks to the declared protocol). Details and a one-line fix inline — I verified the patched version reaches the network on all three paths.
The rest is smaller: one behaviour divergence in transport selection, one about where the new CLI warning lands under the TUI, and a nit.
| def make_client( | ||
| headers: dict[str, str] | None = None, | ||
| timeout: httpx.Timeout | None = None, | ||
| auth: httpx.Auth | None = None, | ||
| ) -> httpx.AsyncClient: | ||
| client = factory(config) | ||
| if headers: | ||
| client.headers.update(headers) | ||
| if timeout is not None: | ||
| client.timeout = timeout | ||
| if auth is not None: | ||
| client.auth = auth | ||
| return client |
There was a problem hiding this comment.
Blocking. This signature is one kwarg short of what FastMCP actually calls, so the whole feature breaks on http (the default, and what Atlassian uses).
StreamableHttpTransport.connect_session does:
http_client = self.httpx_client_factory(
headers=headers,
auth=self.auth,
follow_redirects=True, # type: ignore[call-arg]
**({"timeout": timeout} if timeout else {}),
)follow_redirects isn't in the SDK's McpHttpClientFactory protocol — FastMCP passes it anyway with a type: ignore. pydantic-ai hit the same thing and their _make_httpx_client_factory carries a comment about it. I ran this on your branch against a dead port:
no factory : Client failed to connect: All connection attempts failed
w/ factory : Client failed to connect: _adapt_http_client_factory.<locals>.make_client() got an unexpected keyword argument 'follow_redirects'
And with auth=MCPAuth(kind="oauth") it's the same TypeError. So today, any HTTP server built with a factory fails at connect — and because make_resilient turns that into "server unavailable", the user just sees zero tools with no clue why.
Accepting it also fixes a second thing worth having: a plain httpx.AsyncClient() from a user factory has follow_redirects=False, while the MCP default path (create_mcp_http_client) always sets it to True. Applying the kwarg keeps the two paths behaving the same.
| def make_client( | |
| headers: dict[str, str] | None = None, | |
| timeout: httpx.Timeout | None = None, | |
| auth: httpx.Auth | None = None, | |
| ) -> httpx.AsyncClient: | |
| client = factory(config) | |
| if headers: | |
| client.headers.update(headers) | |
| if timeout is not None: | |
| client.timeout = timeout | |
| if auth is not None: | |
| client.auth = auth | |
| return client | |
| def make_client( | |
| headers: dict[str, str] | None = None, | |
| timeout: httpx.Timeout | None = None, | |
| auth: httpx.Auth | None = None, | |
| # Not in the MCP SDK's `McpHttpClientFactory` protocol, but FastMCP's | |
| # `StreamableHttpTransport` passes it anyway — omitting it is a | |
| # `TypeError` on the first connect. | |
| follow_redirects: bool = True, | |
| ) -> httpx.AsyncClient: | |
| client = factory(config) | |
| if headers: | |
| client.headers.update(headers) | |
| if timeout is not None: | |
| client.timeout = timeout | |
| if auth is not None: | |
| client.auth = auth | |
| client.follow_redirects = follow_redirects | |
| return client |
I applied exactly this locally and re-ran the probe — http, sse and http+oauth all get to "All connection attempts failed", i.e. they reach the network instead of dying in the adapter.
| adapted = _adapt_http_client_factory(factory, cfg) | ||
| auth = httpx.Auth() | ||
| first = adapted() # the OAuth flow calls with no arguments | ||
| second = adapted(headers={"X-H": "1"}, timeout=httpx.Timeout(7.0), auth=auth) |
There was a problem hiding this comment.
This is the test that should have caught the bug above, and it doesn't — because it calls the adapter with the kwargs the protocol declares rather than the kwargs FastMCP actually passes. That's why 13 tests, 100% coverage and green CI all agreed the feature worked.
Worth pinning the real call shape so a future FastMCP change breaks a test instead of a user's connection:
# The exact kwargs FastMCP's StreamableHttpTransport.connect_session passes —
# `follow_redirects` is not in the MCP SDK's declared factory protocol.
third = adapted(headers={}, auth=auth, follow_redirects=True)
assert third.follow_redirects is True
made.append(third)Even better, if it's cheap here: one test that builds an http server with a factory and probes it against a closed port, asserting the error isn't a TypeError. That covers the whole wiring rather than the adapter in isolation.
| """ | ||
| from fastmcp.client.transports import SSETransport, StreamableHttpTransport | ||
|
|
||
| transport_cls = SSETransport if config.transport == "sse" else StreamableHttpTransport |
There was a problem hiding this comment.
Choosing the class from config.transport is the right call — but it only happens when a factory is passed, and that makes the same config mean two different things depending on an unrelated knob.
Without a factory we go through MCPToolset(url, ...), which infers via FastMCP's infer_transport_type_from_url — it returns "sse" only when the path matches /sse(/|?|&|$). So:
transport="sse",url="https://x/events"→ StreamableHttp today, SSE the moment someone setshttp_client_factoryon the registry.transport="http",url="https://x/sse"→ SSE today, StreamableHttp with a factory.
Nobody will connect the switch to the factory when that bites. I'd rather the declared transport always win: build the transport explicitly in both branches and drop the http_client_factory is not None check (passing httpx_client_factory=None is fine — FastMCP treats it as absent). One code path, and transport="sse" means SSE.
If you'd rather not widen the diff, that's fair — but then please say in the _build_http_transport docstring that this path can disagree with the inferred one, so the next person doesn't have to rediscover it.
| try: | ||
| from key_value.aio.stores.disk import DiskStore | ||
| except Exception: | ||
| logger.warning( |
There was a problem hiding this comment.
Right instinct — the silent fallback was the confusing part — but logging.getLogger(__name__) is the wrong channel here, and it can make things worse than staying quiet.
load_mcp_registry() is called from inside running Textual modals (apps/cli/modals/mcp_view.py:178, apps/cli/modals/info_view.py:97). apps.cli.mcp_store isn't in _NOISY_CONSOLE_LOGGERS, so it propagates to root; root has no handler under the TUI, so logging.lastResort writes the record straight to stderr — over the live screen. That's exactly what quiet_console_logging() and the stdio log-file redirect exist to prevent. And the user who needs this message (repeated re-auth) still doesn't get anything actionable in the UI.
The house pattern for CLI-side warnings is the session logger — propagate=False, file handler, no screen corruption:
from apps.cli.debug_log import get_logger
get_logger().warning(
"MCP OAuth tokens will not persist: the disk store backend is not "
"installed (pip install 'py-key-value-aio[disk]'); using in-memory "
"storage, so hosted servers re-authorize on every restart."
)Same for the second logger.warning below. If you want the user to actually see it, the /mcp view already has a place to surface it — but the file log is the minimum and the safe default.
| if self.callback_port is not None and self.callback_port <= 0: | ||
| raise MCPConfigError(f"callback_port must be positive, got {self.callback_port}") |
There was a problem hiding this comment.
nit: while you're validating, the upper bound is worth the same line. callback_port=70000 passes here and then fails much later with a socket error, in the middle of a browser flow, where it's hard to trace back to the config.
| if self.callback_port is not None and self.callback_port <= 0: | |
| raise MCPConfigError(f"callback_port must be positive, got {self.callback_port}") | |
| if self.callback_port is not None and not 0 < self.callback_port <= 65535: | |
| raise MCPConfigError( | |
| f"callback_port must be between 1 and 65535, got {self.callback_port}" | |
| ) |
| # Configuring only the transport would just move the failure from | ||
| # `initialize` to the first token refresh — both sides need the factory. | ||
| assert transport.httpx_client_factory is not None | ||
| assert oauth.httpx_client_factory is transport.httpx_client_factory |
There was a problem hiding this comment.
This assertion is the best line in the PR. Configuring only the transport moves the failure from initialize to the first token refresh, which is a horrible thing to debug months later, and this pins it as an identity check rather than a "not None". Same for test_scoped_token_storage_namespaces_by_scope_set asserting order-insensitivity and the no-scopes pass-through — that pair is what makes the cache-invalidation change safe for people with existing tokens.
DEENUU1
left a comment
There was a problem hiding this comment.
Second pass over the parts I skimmed the first time. One thing worth changing while you're in here, plus a nit folded into the same spot — nothing else new. I re-checked the transport/timeout wiring (pydantic-ai does pass timeout=read_timeout down to the session, so the adapter's client.timeout covers it), the py-key-value-aio>=0.4.5 floor (has PrefixKeysWrapper), the docs heading nesting, and the CLI add-server flow (create-only, so no field-dropping on edit) — all fine.
| if oauth_token_storage is not None: | ||
| storage = oauth_token_storage | ||
| if auth.scopes: | ||
| storage = _scoped_token_storage(storage, auth.scopes) |
There was a problem hiding this comment.
The namespace is keyed on scopes alone, but the thing being namespaced is the whole OAuth cache, and the cached dynamic client registration depends on more than scopes.
PrefixKeysWrapper prefixes every key, so this also namespaces the DCR result (FastMCP stores it via key=_get_client_info_cache_key() in the mcp-oauth-client-info collection). That's good — but the registration is built from client_metadata, which includes redirect_uris=[http://localhost:{callback_port}/callback] and client_name, not just scope. So callback_port and client_name change the registration identity without changing the cache slot.
What that means for someone who already authorized a server and then adds callback_port on its own (which the new docs present as an independent knob, right next to scopes):
auth.scopesis empty → no prefix → the old cachedclient_infois found- the SDK only registers when there is no cached client info (
mcp/client/auth/oauth2.py:if not self.context.client_info:) - so the authorization request goes out with the old
client_idand the newredirect_uri(str(self.context.client_metadata.redirect_uris[0])) - the server sees a redirect URI it never registered
Hedging honestly: this may self-heal. FastMCP's redirect_handler pre-flights the authorization URL and turns a 400 into ClientNotFoundError, which clears the store and retries with a fresh registration. RFC 6749 §4.1.2.1 says a server must not redirect on a bad redirect_uri, so a 400 is likely and many providers will recover after one extra round trip. But a provider that answers 200 with an HTML error page passes FastMCP's status_code not in (200, 302, 303, 307, 308) check, and the user just gets a browser tab on an error with no way out but deleting ~/.pydantic-deep/mcp-oauth by hand. I verified the code path, not a live tenant — no Atlassian access here either.
Since the whole point of this function is "don't silently reuse credentials minted under different terms", I'd key it on everything that goes into client_metadata and drop the dependency on that recovery path:
def _oauth_cache_namespace(auth: MCPAuth) -> str | None:
"""Cache namespace for an OAuth config's tokens *and* client registration.
FastMCP keys both by server URL only, so a config change that alters the
registered client — scopes, the redirect URI's port, the advertised client
name — would otherwise keep reusing credentials minted under the old terms.
``None`` when there is nothing to namespace, so tokens cached before this
existed keep working.
"""
identity = (
",".join(sorted(set(auth.scopes))),
str(auth.callback_port or ""),
auth.client_name or "",
)
if not any(identity):
return None
digest = hashlib.sha256("|".join(identity).encode()).hexdigest()[:16]
return f"oauth-{digest}"and here:
if oauth_token_storage is not None:
storage = oauth_token_storage
namespace = _oauth_cache_namespace(auth)
if namespace is not None:
storage = PrefixKeysWrapper(key_value=storage, prefix=namespace)
oauth_kwargs["token_storage"] = storageTwo notes on the blast radius, both checked:
- No builtin sets
client_name(the Figma entry isMCPAuth(kind="oauth", instructions=...)only), so no shipped config gains a prefix from this. A hand-written config withclient_nameset would re-auth once — the same one-time cost you already documented forscopesin Notes for Reviewers, and arguably a fix: today, changingclient_namesilently has no effect at all, because the old registered name is reused. scopes-only configs would get a different digest than the current one, but nothing is released yet, so no real token is affected.
nit folded in: sorted(set(...)) rather than sorted(...). test_scoped_token_storage_namespaces_by_scope_set says the namespace is a function of the scope set, and with duplicates it currently isn't — ["read:jira", "read:jira"] and ["read:jira"] request identical scopes but land in different slots.
Summary
Hosted OAuth MCP servers (Figma, Atlassian) can now be configured for enterprise
environments declaratively:
MCPAuthgains explicitscopesand a fixedcallback_port, and a newhttp_client_factoryescape hatch threads a customhttpx.AsyncClient(authenticated proxy, OS trust store, mTLS) through both theMCP transport and every step of the OAuth flow. Cached OAuth tokens are
namespaced per scope set, so changing
scopesforces a fresh authorizationinstead of silently reusing a token minted under the old permissions.
Added
scopesandcallback_porttoMCPAuthinpydantic_deep/mcp/config.py—validated, round-tripped through
to_dict/from_dict, forwarded to FastMCP'sOAuth(...)inpydantic_deep/mcp/registry.py.HttpClientFactorytype andhttp_client_factory=kwarg onMCPRegistryand
build_mcp_server. The factory is adapted to the MCP SDK's factory protocol(
_adapt_http_client_factory: fresh client per call, with theheaders/timeout/authkwargs FastMCP passes applied on top) and reaches both the OAuth object and the
transport, so discovery, token exchange, refresh and regular traffic all go
through it.
_scoped_token_storage: wraps the OAuth token store in aPrefixKeysWrapperkeyed by a hash of the sorted scope set.sections in
docs/learn/web-and-mcp.md(env vars first, factory as lastresort), plus
HttpClientFactoryindocs/api/mcp.md.Changed
StreamableHttpTransport/SSETransportexplicitly (chosen fromconfig.transport, not inferred from the URL), becauseMCPToolset(url, ...)has no way to carry an
httpx_client_factoryto the transport it buildsinternally. Configs without a factory build exactly as before.
mcp_oauth_storage()inapps/cli/mcp_store.pylogs a warning on bothfailure paths (missing disk backend, store init error) instead of silently
degrading to in-memory token storage.
Testing
tests/test_mcp.pyandtests/test_cli_mcp.py:field round-trip and validation,
OAuthkwargs forwarding, factory identityasserted on both the OAuth object and the transport, fresh-client-per-call
adapter contract, scope-set namespacing (order-insensitive, pass-through
without scopes), SSE and stdio paths, registry threading, CLI fallback warnings.
make lint,make typecheck(pyright) andmake typecheck-mypy— clean.make test— 2811 passed, 100% coverage.mkdocs build— no warnings.Related Issue
Closes #183
Notes for Reviewers
scopeson a server with an existing cached token triggers a one-timebrowser re-auth: the store key gains a scope-hash prefix, so the old
un-prefixed token is deliberately not found. Configs without
scopeskeepusing their existing tokens unchanged.
httpx.AsyncClientper call — both thetransport and each OAuth step close the client they receive, so a shared
instance would be dead after the first request. This is documented on
HttpClientFactoryand in the docs example.tls_verifyconfig field and an
atlassianbuiltin catalogue entry.Negotiate proxy; the reporter offered to test end-to-end from their side.