Add support for serverside oauth by praboud-ant · Pull Request #255 · modelcontextprotocol/python-sdk

praboud-ant · 2025-03-10T18:05:31Z

Motivation and Context

Implements https://spec.modelcontextprotocol.io/specification/draft/basic/authorization/ from the specification.

How Has This Been Tested?

I've added some integration tests as part of this branch. I've also implemented a new MCP server (outside of this repo), and integrated it against the new parts of the the SDK, to play around with the interface a bit and see how it feels.

Tested with inspector:

Breaking Changes

No - this is strictly additive.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update

Checklist

I have read the MCP Documentation
My code follows the repository's style guidelines
New and existing tests pass locally
I have added appropriate error handling
I have added or updated documentation as needed

Additional context

Kludex · 2025-03-10T18:28:50Z

If review wanted, happy to review it.

dsp-ant · 2025-03-11T10:57:37Z

@Kludex its work in progress as far as I undersatnd but feel free to take a review pass.

dsp-ant · 2025-03-11T11:02:21Z

+def validate_scope(requested_scope: str | None, client: OAuthClientInformationFull) -> list[str] | None:
+ if requested_scope is None:
+ return None
+ requested_scopes = requested_scope.split(" ")
+ allowed_scopes = [] if client.scope is None else client.scope.split(" ")
+ for scope in requested_scopes:
+ if scope not in allowed_scopes:
+ raise InvalidRequestError(f"Client was not registered with scope {scope}")
+ return requested_scopes


This feels like a method on OAuthClientInformationFull or the underlying scope model, given we exclusively operate on client.scope here.

[opinion] after hacking around in here, I actually think we should be thinking about OAuthClientInformationFull as a type similar to the ones in mcp.types, and it would be odd to add methods there

dsp-ant · 2025-03-11T11:14:42Z

+ redirect_uri=redirect_uri,
+ )
+
+ response = RedirectResponse(url="", status_code=302, headers={"Cache-Control": "no-store"})


We should probalby not redirect without a URI?

dsp-ant · 2025-03-11T11:24:25Z

+ if "Authorization" not in conn.headers:
+ return None
+
+ auth_header = conn.headers["Authorization"]
+ if not auth_header.startswith("Bearer "):
+ return None
+
+ token = auth_header[7:] # Remove "Bearer " prefix


I think this can be simplified by
Suggested change
if"Authorization"notinconn.headers:
returnNone
auth_header=conn.headers["Authorization"]
ifnotauth_header.startswith("Bearer "):
returnNone
token=auth_header[7:] # Remove "Bearer " prefix
auth_header=conn.headers.get("Authorization")
ifnotauth_headerornotauth_header.startswith("Bearer "):
returnNone
token=auth_header[7:] # Remove "Bearer " prefix

dsp-ant · 2025-03-11T11:25:21Z

+ # Validate the token with the provider
+ auth_info = await self.provider.verify_access_token(token)
+
+ if auth_info.expires_at and auth_info.expires_at < int(time.time()):


We should use datetime comparisions and not rely on timestamps.

dsp-ant · 2025-03-11T11:27:46Z

+ client_id: str
+ client_secret: Optional[str] = None
+ client_id_issued_at: Optional[int] = None
+ client_secret_expires_at: Optional[int] = None


Probably should be parsed as a Timedelta

A timedelta, or a datetime? Don't we want an absolute time here?

dsp-ant · 2025-03-11T11:28:07Z

+ """
+ access_token: str
+ token_type: str
+ expires_in: Optional[int] = None


Also here, probably watn to parse as a timedelta.

dsp-ant · 2025-03-11T11:29:12Z

+ Corresponds to OAuthTokensSchema in src/shared/auth.ts
+ """
+ access_token: str
+ token_type: str


Should we enumerate the potential types here to ensure pydnatic validation?

dsp-ant · 2025-03-11T11:29:33Z

+from httpx._transports.base import AsyncBaseTransport
+from httpx._models import Request, Response
+from httpx._types import AsyncByteStream
+import asyncio


we don't use asyncio for testing.

jerome3o-anthropic

Working through the changes now

jerome3o-anthropic · 2025-03-11T10:49:08Z

+
+class PydanticJSONResponse(JSONResponse):
+ def render(self, content: Any) -> bytes:
+ return content.model_dump_json(exclude_none=True).encode("utf-8")


What is this for?

Lemme add a comment - this uses Pydantic's JSON serialization, since the stock json doesn't know how to serialize eg: AnyHttpUrl

Kludex · 2025-03-15T07:44:33Z

The auth part on the client should be implemented using the httpx.Auth class.

bhosmer-ant

New changes LGTM (and things look relatively undisturbed from the last time it was looked over in some depth) - based on that, tests passing, and your e2e check passing, I'm good with landing it. (would ofc benefit from @jerome3o-anthropic also looking before you push the button, mod available time)

bhosmer-ant · 2025-05-01T13:57:18Z

 )

- # Ensure all streams are properly closed
- async with read_stream, write_stream, read_stream_writer, sse_stream_reader:


ooc was the outer with unnecessary or was it actively messing things up?

I just reverted all the changes to see transport, returning response was not needed at all.
The fun part for the top async with was that it was masking "bad test" and sse tests were just hanging. When reverted all the sse changes notices that some other test randomly fails with " ResourceWarning: Unclosed <MemoryObjectReceiveStream at 106c0dd20>"

I think it's a nice update, I would prefer to have it in a separate PR and test separately. There are a bunch of open PRs addressing different parts of memory leaks, maybe we can just bundle all of them together

bhosmer-ant · 2025-05-01T14:04:02Z

 )


-class TestFastMCPWithAuth:


was this the test that leaked and made the other ones hang?

yes, basically this :
async with aconnect_sse( test_client, "GET", "/sse", headers={"Authorization": authorization} ) as event_source:

jerome3o-anthropic

Thanks for pushing this over the line @ihrpr - it's awesome to see it

I've looked over this, not with a fine-toothed comb but for the general shapes, and it LGTM!

jerome3o-anthropic · 2025-05-01T17:47:41Z

+providing an implementation of the `OAuthServerProvider` protocol.
+
+```
+mcp = FastMCP("My App",


[non blocking] wonder if its worth noting that this wont scale beyond one instance

I think that's something we can add to the example

done

dsp-ant reviewed Mar 11, 2025
View reviewed changes

dsp-ant previously requested changes Mar 11, 2025
View reviewed changes

jerome3o-anthropic reviewed Mar 11, 2025
View reviewed changes

praboud-ant force-pushed the praboud/auth branch from 65db7b6 to 5e7a0bfCompare March 11, 2025 13:52

praboud-ant mentioned this pull request Mar 11, 2025
Jerome/auth #260
Closed
9 tasks

dsp-ant force-pushed the praboud/auth branch from cfab842 to 9e460a9Compare March 14, 2025 21:41

praboud-ant added 21 commits March 19, 2025 11:01

wip
682ff1e

Unwind changes
331d51e

wip
d283f56

Get tests passing
e96d280

Clean up provider interface
1e9dd4c

Lint
d535089

Clean up registration endpoint
031cadf

Lint
765efb6

update token + revoke to use form data
0637bc3

Adjust more things to fit spec
b99633a

Lint
9ae1c21

Remove dup
50683b9

Comment
2c5f26a

Refactor back to authorize()
e605994

Improve validation for /token
e7c5f87

Improve validation for registration
83c0c9f

Improve /authorize validation & add tests
0c1aae9

Hoist oauth token expiration check into bearer auth middleware
038fb04

Add tests for /revoke validation
a4e17f3

Lint + typecheck
5f11c60

Clean up unused error classes
571913a

ihrpr added 11 commits April 30, 2025 21:16

try fixing test
0582bf5

increse timeout
8194bce

fix test
2c63020

test
2ea68f2

fix test
b0fe041

test
ba366e3

test
e1a9fec

skip test
af4221f

Test auth (#609)
f2840fe

remove pyright upgrade and ruff format
cda4401

uv lock
f2cc6ee

bhosmer-ant approved these changes May 1, 2025
View reviewed changes

ihrpr mentioned this pull request May 1, 2025
Auth SSE simple example #610
Merged

jerome3o-anthropic approved these changes May 1, 2025
View reviewed changes

ihrpr approved these changes May 1, 2025
View reviewed changes

Merge branch 'main' into praboud/auth
23ef519

ihrpr enabled auto-merge (squash) May 1, 2025 18:33

ihrpr removed request for Kludex and dsp-ant May 1, 2025 18:34

ihrpr disabled auto-merge May 1, 2025 18:35

ihrpr requested review from Kludex and dsp-ant and removed request for Kludex and dsp-ant May 1, 2025 18:38

ihrpr merged commit 2210c1b into mainMay 1, 2025

ihrpr deleted the praboud/auth branch May 1, 2025 18:43

johnandersen777 mentioned this pull request May 1, 2025
401 Unauthorized #611
Closed

-if"Authorization"notinconn.headers:
-returnNone
-auth_header=conn.headers["Authorization"]
-ifnotauth_header.startswith("Bearer "):
-returnNone
-token=auth_header[7:] # Remove "Bearer " prefix
+auth_header=conn.headers.get("Authorization")
+ifnotauth_headerornotauth_header.startswith("Bearer "):
+returnNone
+token=auth_header[7:] # Remove "Bearer " prefix

		)


		class TestFastMCPWithAuth:

Uh oh!

Conversation

praboud-ant commented Mar 10, 2025• edited by ihrpr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

How Has This Been Tested?

Breaking Changes

Types of changes

Checklist

Additional context

Uh oh!

Kludex commented Mar 10, 2025

Uh oh!

dsp-ant commented Mar 11, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerome3o-anthropic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Kludex commented Mar 15, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bhosmer-ant left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerome3o-anthropic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

praboud-ant commented Mar 10, 2025•
edited by ihrpr
Loading

Kludex commented Mar 15, 2025•
edited
Loading