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.

src/mcp/server/auth/errors.py

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

src/mcp/server/auth/handlers/authorize.py

+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

src/mcp/server/auth/handlers/authorize.py

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

src/mcp/server/auth/handlers/authorize.py

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


We should probalby not redirect without a URI?

src/mcp/server/auth/handlers/register.py

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

src/mcp/server/auth/middleware/bearer_auth.py

+        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" 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

auth_header = conn.headers.get("Authorization")

if not auth_header or not auth_header.startswith("Bearer "):

return None

token = auth_header[7:] # Remove "Bearer " prefix

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

src/mcp/server/auth/middleware/bearer_auth.py

+            # 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

src/mcp/shared/auth.py

+    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

src/mcp/shared/auth.py

+    """
+    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

src/mcp/shared/auth.py

+    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

tests/server/fastmcp/auth/streaming_asgi_transport.py

+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

src/mcp/server/auth/json_response.py

+
+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

src/mcp/server/auth/errors.py

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

src/mcp/server/sse.py

                    )

-        # 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

tests/server/fastmcp/auth/test_auth_integration.py

        )


-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

README.md

+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 5e7a0bf Compare 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 9e460a9 Compare 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 main May 1, 2025
10 checks passed

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

johnandersen777 mentioned this pull request May 1, 2025

401 Unauthorized #611

Closed

		)


		class TestFastMCPWithAuth:

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!

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