-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add support for serverside oauth #255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 67 commits
682ff1e
331d51e
d283f56
e96d280
1e9dd4c
d535089
031cadf
765efb6
0637bc3
b99633a
9ae1c21
50683b9
2c5f26a
e605994
e7c5f87
83c0c9f
0c1aae9
038fb04
a4e17f3
5f11c60
571913a
d43647f
9d72c1e
a5079af
c4c2608
bc62d73
3852179
874838a
f788d79
152feb9
f37ebc4
c2873fd
fe2c029
3a13f5d
37c5fc4
792d302
6c48b11
a437566
9fee929
d79be8f
8d637b4
8c86bce
31618c1
5ebbc19
50673c6
02d76f3
88edddc
d774be7
a09e958
4e73552
56f694e
60da682
374a0b4
fb5a568
76ddc65
e42dbf5
10e00e7
87571d8
c6f991b
482149e
5230180
8e15abc
0a1a408
3069aa3
16f0688
5ecc7f0
8c251c9
f46dcb1
d3725cf
07f4e3a
1237148
1ad1842
fa068dd
9b5709a
67d568b
16a7efa
91c09a4
0582bf5
8194bce
2c63020
2ea68f2
b0fe041
ba366e3
e1a9fec
af4221f
f2840fe
cda4401
f2cc6ee
23ef519
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -166,4 +166,5 @@ cython_debug/ | |
|
||
# vscode | ||
.vscode/ | ||
.windsurfrules | ||
**/CLAUDE.local.md |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,7 +46,7 @@ default-groups = ["dev", "docs"] | |
|
||
[dependency-groups] | ||
dev = [ | ||
"pyright>=1.1.391", | ||
"pyright>=1.1.396", | ||
"pytest>=8.3.4", | ||
"ruff>=0.8.5", | ||
"trio>=0.26.2", | ||
|
@@ -110,8 +110,12 @@ mcp = { workspace = true } | |
xfail_strict = true | ||
filterwarnings = [ | ||
"error", | ||
# this is a long-standing issue with fastmcp, which is just now being exercised by tests | ||
"ignore:Unclosed:ResourceWarning", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove this. I can help find out the unclosed resources. Is it only reproducible in this PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is reproducible in the PR only because the PR adds integration tests to previously untested flows. I poked at the test failure for a bit, without much success - my preference would be to leave this as-is, and fix the resource leak in tests in a separate PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed the problem and removed the warning suppression in this commit |
||
# This should be fixed on Uvicorn's side. | ||
"ignore::DeprecationWarning:websockets", | ||
"ignore:websockets.server.WebSocketServerProtocol is deprecated:DeprecationWarning", | ||
"ignore:Returning str or bytes.*:DeprecationWarning:mcp.server.lowlevel" | ||
"ignore:Returning str or bytes.*:DeprecationWarning:mcp.server.lowlevel", | ||
# this is a problem in starlette | ||
"ignore:Please use `import python_multipart` instead.:PendingDeprecationWarning", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are using an old version of Starlette. We should bump it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to the above - this is a problem predating this PR. It's only popping up here because we previously didn't have integration tests which exercised this flow in the tests. This PR is already pretty big, and I'd prefer to address the starlette bump in a followup. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Upgraded starlette and removed this warning suppression in this commit |
||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
""" | ||
MCP OAuth server authorization components. | ||
""" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
from typing import Literal | ||
|
||
from pydantic import BaseModel, ValidationError | ||
|
||
ErrorCode = Literal["invalid_request", "invalid_client"] | ||
|
||
|
||
class ErrorResponse(BaseModel): | ||
error: ErrorCode | ||
error_description: str | ||
|
||
|
||
class OAuthError(Exception): | ||
""" | ||
Base class for all OAuth errors. | ||
""" | ||
|
||
error_code: ErrorCode | ||
|
||
def __init__(self, error_description: str): | ||
super().__init__(error_description) | ||
self.error_description = error_description | ||
|
||
def error_response(self) -> ErrorResponse: | ||
return ErrorResponse( | ||
error=self.error_code, | ||
error_description=self.error_description, | ||
) | ||
|
||
|
||
def stringify_pydantic_error(validation_error: ValidationError) -> str: | ||
return "\n".join( | ||
f"{'.'.join(str(loc) for loc in e['loc'])}: {e['msg']}" | ||
for e in validation_error.errors() | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
""" | ||
Request handlers for MCP authorization endpoints. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[non blocking] wonder if its worth noting that this wont scale beyond one instance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's something we can add to the example