Deprecate constructing the pre-provisioned OAuth clients without an issuer - #3435
Conversation
ClientCredentialsOAuthProvider and PrivateKeyJWTOAuthProvider send fixed credentials to whichever authorization server discovery yields unless `issuer=` names the one they belong to. Leaving it out stays allowed, but the provider now says so at construction with a UserWarning that names the server URL and the keyword to pass, so the choice is visible rather than silent. Nothing else changes: with `issuer=` set there is no warning, and a value that is not an http(s) URL is still a ValueError. The example story and the interaction tests pass `issuer=` (their authorization server is known); the extension tests that exercise the no-issuer path opt in to the warning explicitly.
📚 Documentation preview
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
The warning is already attributed to the caller's constructor line, so it does not need the provider name or server URL; _checked_issuer keeps its single argument.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/mcp/client/auth/extensions/client_credentials.py— nit: the PrivateKeyJWT-side docstring examples in this file still construct providers withoutissuer=, so a user copying the SDK's own documented code now gets the very UserWarning this PR adds (and a pytest suite with warnings-as-errors fails), which the base branch did not do. Fix: addissuer="https://auth.example.com"(or a one-line note about the warning) to every example in this module that omits it — thePrivateKeyJWTOAuthProviderclass docstring Options 1-3 (lines 273-310),static_assertion_provider(174-179), andSignedJWTParameters(208-213); theClientCredentialsOAuthProviderexample already has it. [also at: src/mcp/client/auth/extensions/client_credentials.py:309 - nit: the PrivateKeyJWTOAuthProvider class docstring example constructs the provider withoutissuer=, so anyone…]Extended reasoning...
This PR makes
_checked_issuer(None)emit a UserWarning at construction (src/mcp/client/auth/extensions/client_credentials.py:27-33), and it updated the docs page, the example story, and every in-repo test to passissuer=. But the module's own public docstrings were only updated in their Args sections: thePrivateKeyJWTOAuthProviderclass docstring shows three copy-paste examples (Option 1 ~lines 273-279, Option 2 ~286-292, Option 3 ~304-310), andstatic_assertion_provider(~174-179) andSignedJWTParameters(~208-213) show two more, all constructing the provider withoutissuer=. A developer who copies any of these verbatim — the normal way these docstrings are consumed — now seesUserWarning: Noissuergiven...on every construction, and if their test suite runs withfilterwarnings = ["error"](the convention this repo itself follows) their tests fail outright; on the base branch the same copied code ran silently. TheClientCredentialsOAuthProviderclass example (lines 75-82) already includesissuer=, showing the intended pattern; the five…Verification: nit — triggered whenever a user copies one of this module's own docstring examples verbatim. Mechanism verified:
_checked_issuer()at src/mcp/client/auth/extensions/client_credentials.py:27-33 now emitswarnings.warn("Noissuergiven: ...", stacklevel=3)whenissuer is None, and the PR updated docs/client/oauth-clients.md, the example story, and all in-repo tests to passissuer=—…
…ssuer The warning becomes an MCPDeprecationWarning: omitting `issuer=` on ClientCredentialsOAuthProvider and PrivateKeyJWTOAuthProvider keeps working in 2.x and will be required in 3.0. The message says why (without it the MCP server decides which authorization server receives the credentials) and what to pass. docs/deprecated.md lists it next to the other SDK-level deprecation, and the OAuth clients page points there.
maxisbey
changed the title
There was a problem hiding this comment.
I re-reviewed the rework that turned the construction warning into an MCPDeprecationWarning and found no bugs; since omitting issuer= now formally becomes required in 3.0, a maintainer's eye on that compatibility-contract commitment is still worthwhile.
What was reviewed: the _checked_issuer restructure in src/mcp/client/auth/extensions/client_credentials.py is behavior-preserving (None warns and falls through as before; non-None keeps the http(s) check), and stacklevel=3 correctly attributes the warning to the constructor call site, pinned by the new warning.filename == __file__ test.
Checked every in-repo construction site against the warnings-as-errors pytest config: all tests, the docs_src tutorial, and the example story now pass issuer= or use pytest.warns; the only remaining no-issuer caller is the conformance harness script, which runs outside pytest as the description notes.
Also confirmed the warning message in the new test matches the source string exactly, and that MCPDeprecationWarning was already exported from mcp, so no new public-API surface is added.
Extended reasoning...
Overview
This PR deprecates constructing ClientCredentialsOAuthProvider and PrivateKeyJWTOAuthProvider without issuer=. The only source change is in src/mcp/client/auth/extensions/client_credentials.py: _checked_issuer() now emits an MCPDeprecationWarning (static message, stacklevel=3) when issuer is None, with the non-None validation path unchanged. Docstrings, docs/client/oauth-clients.md, and docs/deprecated.md are updated in the same PR, the example story passes issuer=BASE_URL, and tests across three files gain issuer= so the repo's filterwarnings = ["error"] config stays green. Two new parametrized tests pin the warning text, its attribution to the caller's file, and the unchanged no-issuer exchange behavior. Since my earlier review, commit 04dafcb substantively reworked the change from a plain UserWarning to the SDK's MCPDeprecationWarning with a 3.0-removal commitment, which is why a fresh review body is warranted.
Security risks
The change sits in OAuth client code, but it does not alter any auth decision: the restructured _checked_issuer provably preserves both branches (None still returns None and the flow still follows discovered metadata; non-None still enforces an http(s) scheme). The deprecation itself is security-motivated in the right direction — it nudges callers toward pinning the authorization server that receives their credentials. No credentials, endpoints, or token-handling logic are touched.
Level of scrutiny
Moderate. The mechanical risk is low (a warning plus docs and test plumbing), and I verified the details that could bite: stacklevel=3 walks warn → _checked_issuer → __init__ → caller correctly and is locked down by the warning.filename == __file__ assertion; the test's expected message string matches the source string character-for-character; and every pytest-covered construction site (tests, docs_src/oauth_clients/tutorial002.py, the example story) now passes issuer= or wraps in pytest.warns, so nothing trips warnings-as-errors. The one reason not to approve outright is policy, not correctness: AGENTS.md treats changes to an existing API's observable behavior — including deprecation-shimmed ones — as explicit maintainer design decisions, and this PR commits the 2.x line to making issuer= required in 3.0. That call belongs to a human maintainer.
Other factors
The bug hunt ran to a natural dry-streak finish with no findings. Coverage of the new branch is handled by the two new tests (both the warning branch and the non-None validation branch are exercised). The only remaining no-issuer construction is .github/actions/conformance/client.py, which runs as a plain script where the warning just prints — the PR description acknowledges this. My earlier inline nit about the example README's "five lines" count versus the docstring's "six lines" was not addressed by the latest commit, but it is cosmetic and the open thread already records it, so I did not restate it in the body.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
The PrivateKeyJWTOAuthProvider, static_assertion_provider and SignedJWTParameters examples now show the non-deprecated form, and the client-credentials story stops counting its config lines.
1 participant
Constructing
ClientCredentialsOAuthProviderorPrivateKeyJWTOAuthProviderwithoutissuer=is deprecated: it keeps working in 2.x, warns withMCPDeprecationWarning, and the keyword becomes required in 3.0.Motivation and Context
#3398 added the optional
issuer=keyword to the two pre-provisioned providers: with it, token requests are only ever built from metadata for that authorization server. Without it the provider follows whichever authorization server the MCP server advertises, which is the behaviour these providers have always had. For credentials that were issued by one specific authorization server, the second mode is rarely what anyone wants, and the operator always knows the value to pass, so 3.0 will require it. Until then the constructor says so:MCPDeprecationWarningis the category the SDK already uses for its other deprecations (aUserWarning, so it is shown by default and filterable as one category).stacklevelpoints it at the caller's constructor line. Passingissuer=silences it, and there is no other change in behaviour.How Has This Been Tested?
tests/client/auth/extensions/test_client_credentials.py: a parametrised test asserts the category, the text, and that the warning is attributed to the calling file for both providers, and one test keeps exercising the no-issuer exchange path underpytest.warns. The older tests in that file, the interaction tests and thePrivateKeyJWTOAuthProviderdocs test now passissuer=matching the authorization server they already mock or run. Theoauth_client_credentialsexample story passesissuer=too.docs/deprecated.mdlists the deprecation next to the existing SDK-level one anddocs/client/oauth-clients.mdlinks to it.Breaking Changes
None in 2.x. Code that constructs either provider without
issuer=keeps working and now sees oneMCPDeprecationWarningper call site (test suites running with warnings as errors will want to passissuer=or filter the category). 3.0 will make the keyword required.Types of changes
Checklist
help wanted, or I'm a maintainer)Additional context
The conformance client script constructs these providers from the harness's context, which does not carry an issuer yet, so the client-conformance job will print the warning; that is expected.
AI Disclaimer