Skip to content

Commit eb73267

Browse files
Fix CodeRabbit review comments for Keycloak OAuth provider
Address 11 actionable code review comments from CodeRabbitAI: - Fix FileTokenStorage import and add CLEAR_TOKEN_CACHE feature flag in client example - Remove manual scope splitting in server.py, delegate to KeycloakProviderSettings parser - Merge client-requested scopes with required scopes in registration endpoint - Always append missing required scopes to authorization requests - Fix test mocking to only patch provider's httpx imports, not globally - Move asyncio import to module scope in integration tests - Fix markdown linting issues (convert bold to headings, remove trailing space) - Fix troubleshooting paths in keycloak/README.md - Pin dependency versions in requirements.txt for reproducibility These changes ensure proper scope handling that preserves client intent while enforcing server requirements, improve test isolation, and enhance code quality.
1 parent def60e3 commit eb73267

File tree

7 files changed

+58
-37
lines changed

7 files changed

+58
-37
lines changed

examples/auth/keycloak_auth/README.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,13 @@ Review the realm configuration file: [`keycloak/realm-fastmcp.json`](keycloak/re
1717

1818
Choose one of the following options:
1919

20-
**Option A: Local Keycloak Instance (Recommended for Testing)**
20+
#### Option A: Local Keycloak Instance (Recommended for Testing)
2121

2222
See [keycloak/README.md](keycloak/README.md) for details.
2323

24-
**Note:** The realm will be automatically imported on startup.
24+
**Note:** The realm will be automatically imported on startup.
2525

26-
**Option B: Existing Keycloak Instance**
26+
#### Option B: Existing Keycloak Instance
2727

2828
Manually import the realm:
2929
- Log in to your Keycloak Admin Console

examples/auth/keycloak_auth/client.py

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,17 +9,22 @@
99
import asyncio
1010

1111
from fastmcp import Client
12+
from fastmcp.client.auth.oauth import FileTokenStorage
1213

1314
SERVER_URL = "http://localhost:8000/mcp"
1415

16+
# Set to True to clear any previously stored tokens.
17+
# This is useful if you have just restarted Keycloak and end up seeing
18+
# Keycloak showing this error: "We are sorry... Client not found."
19+
# instead of the login screen
20+
CLEAR_TOKEN_CACHE = False
21+
1522

1623
async def main():
17-
# Uncomment the following lines to clear any previously stored tokens.
18-
# This is useful if you have just restarted Keycloak and end up seeing
19-
# Keycloak showing this error: "We are sorry... Client not found."
20-
# instead of the login screen
21-
# storage = FileTokenStorage("http://localhost:8000/mcp/")
22-
# storage.clear()
24+
if CLEAR_TOKEN_CACHE:
25+
storage = FileTokenStorage(f"{SERVER_URL.rstrip('/')}/")
26+
storage.clear()
27+
print("🧹 Cleared cached OAuth tokens.")
2328

2429
try:
2530
async with Client(SERVER_URL, auth="oauth") as client:

examples/auth/keycloak_auth/keycloak/README.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,8 @@ docker-compose logs -f keycloak
105105
2. **Realm not found**
106106
- Verify realm import: Check admin console at [http://localhost:8080/admin](http://localhost:8080/admin)
107107
- Check realm file exists:
108-
- Linux/macOS: `ls keycloak/realm-fastmcp.json`
109-
- Windows: `dir keycloak\realm-fastmcp.json`
108+
- Linux/macOS: `ls realm-fastmcp.json`
109+
- Windows: `dir realm-fastmcp.json`
110110

111111
3. **Client registration failed**
112112
- Verify the request comes from a trusted host
Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
1-
fastmcp
2-
python-dotenv
1+
fastmcp>=0.1.0
2+
python-dotenv>=1.0.0

examples/auth/keycloak_auth/server.py

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -24,16 +24,18 @@
2424

2525
load_dotenv(".env", override=True)
2626

27+
realm_url = os.getenv(
28+
"FASTMCP_SERVER_AUTH_KEYCLOAK_REALM_URL", "http://localhost:8080/realms/fastmcp"
29+
)
30+
base_url = os.getenv("FASTMCP_SERVER_AUTH_KEYCLOAK_BASE_URL", "http://localhost:8000")
31+
required_scopes = os.getenv(
32+
"FASTMCP_SERVER_AUTH_KEYCLOAK_REQUIRED_SCOPES", "openid,profile"
33+
)
34+
2735
auth = KeycloakAuthProvider(
28-
realm_url=os.getenv(
29-
"FASTMCP_SERVER_AUTH_KEYCLOAK_REALM_URL", "http://localhost:8080/realms/fastmcp"
30-
),
31-
base_url=os.getenv(
32-
"FASTMCP_SERVER_AUTH_KEYCLOAK_BASE_URL", "http://localhost:8000"
33-
),
34-
required_scopes=os.getenv(
35-
"FASTMCP_SERVER_AUTH_KEYCLOAK_REQUIRED_SCOPES", "openid,profile"
36-
).split(","),
36+
realm_url=realm_url,
37+
base_url=base_url,
38+
required_scopes=required_scopes,
3739
)
3840

3941
mcp = FastMCP("Keycloak OAuth Example Server", auth=auth)

src/fastmcp/server/auth/providers/keycloak.py

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -262,12 +262,16 @@ async def register_client_proxy(request):
262262

263263
# Add the server's required scopes to the client registration data
264264
if self.token_verifier.required_scopes:
265+
scopes = parse_scopes(registration_data.get("scope")) or []
266+
merged_scopes = scopes + [
267+
scope
268+
for scope in self.token_verifier.required_scopes
269+
if scope not in scopes
270+
]
265271
logger.info(
266-
f"Adding server-configured required scopes to client registration data: {self.token_verifier.required_scopes}"
267-
)
268-
registration_data["scope"] = " ".join(
269-
self.token_verifier.required_scopes
272+
f"Merging server-configured required scopes with client-requested scopes: {merged_scopes}"
270273
)
274+
registration_data["scope"] = " ".join(merged_scopes)
271275
# Update the body with modified client registration data
272276
body = json.dumps(registration_data).encode("utf-8")
273277

@@ -355,13 +359,20 @@ async def authorize_proxy(request):
355359

356360
# Add server-configured required scopes to the authorization request
357361
query_params = dict(request.query_params)
358-
if "scope" not in query_params and self.token_verifier.required_scopes:
359-
logger.info(
360-
f"Adding server-configured required scopes to authorization request: {self.token_verifier.required_scopes}"
361-
)
362-
query_params["scope"] = " ".join(
363-
self.token_verifier.required_scopes
364-
)
362+
if self.token_verifier.required_scopes:
363+
existing_scopes = parse_scopes(query_params.get("scope")) or []
364+
missing_scopes = [
365+
scope
366+
for scope in self.token_verifier.required_scopes
367+
if scope not in existing_scopes
368+
]
369+
if missing_scopes:
370+
logger.info(
371+
f"Adding server-configured required scopes to authorization request: {missing_scopes}"
372+
)
373+
query_params["scope"] = " ".join(
374+
existing_scopes + missing_scopes
375+
)
365376

366377
# Build authorization request URL for redirecting to Keycloak and including the (potentially modified) query string
367378
authorization_url = str(self.oidc_config.authorization_endpoint)

tests/integration_tests/auth/test_keycloak_provider_integration.py

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
"""Integration tests for Keycloak OAuth provider."""
22

3+
import asyncio
34
import os
45
from unittest.mock import AsyncMock, Mock, patch
56
from urllib.parse import parse_qs, urlparse
@@ -95,7 +96,9 @@ async def test_end_to_end_client_registration_flow(self, mock_keycloak_server):
9596
mcp_http_app = mcp.http_app()
9697

9798
# Mock the actual HTTP client post method
98-
with patch("httpx.AsyncClient.post") as mock_post:
99+
with patch(
100+
"fastmcp.server.auth.providers.keycloak.httpx.AsyncClient.post"
101+
) as mock_post:
99102
# Mock Keycloak's response to client registration
100103
mock_keycloak_response = Mock()
101104
mock_keycloak_response.status_code = 201
@@ -288,7 +291,9 @@ async def test_concurrent_client_registrations(self):
288291
mcp_http_app = mcp.http_app()
289292

290293
# Mock concurrent Keycloak responses
291-
with patch("httpx.AsyncClient") as mock_client_class:
294+
with patch(
295+
"fastmcp.server.auth.providers.keycloak.httpx.AsyncClient"
296+
) as mock_client_class:
292297
mock_client = AsyncMock()
293298
mock_client_class.return_value.__aenter__.return_value = mock_client
294299

@@ -318,8 +323,6 @@ async def test_concurrent_client_registrations(self):
318323
transport=httpx.ASGITransport(app=mcp_http_app),
319324
base_url=TEST_BASE_URL,
320325
) as client:
321-
import asyncio
322-
323326
registration_data = [
324327
{
325328
"redirect_uris": [f"http://localhost:800{i}/callback"],

0 commit comments

Comments
 (0)