Implement OAuth endpoints#5
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jhernand The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
size/L
## Walkthrough
The changes introduce OAuth authentication support for the MCP server, including a new middleware to handle OAuth endpoints and enforce authentication. The server is now run as a Starlette application with middleware support, and documentation has been updated to describe the new OAuth configuration options.
## Changes
| File(s) | Change Summary |
|--------------------|--------------------------------------------------------------------------------------------------------------------|
| README.md | Moved "Ask about your clusters" section earlier; added "Authorization Options" detailing four prioritized authentication methods including bearer token, OAuth flow, and offline tokens with environment variable configurations. |
| server.py | Refactored imports to use `fastmcp` package; replaced direct request context with dependency injection for headers; added conditional OAuth middleware integration based on environment variables; switched server startup to Starlette app with Uvicorn on port 8000. |
| oauth.py | Added new Starlette middleware class implementing OAuth endpoints (`/.well-known/oauth-protected-resource`, `/.well-known/oauth-authorization-server`, `/oauth/register`), enforcing authorization header presence for other requests and simulating dynamic client registration. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Client
participant Server (Starlette+MCP)
participant OAuth Server
Client->>Server (/.well-known/oauth-authorization-server): Request metadata
Server->>OAuth Server: Proxy metadata request
OAuth Server-->>Server: Return metadata
Server-->>Client: Return modified metadata
Client->>Server (/oauth/register): Register client
Server-->>Client: Return fixed client ID
Client->>Server (other endpoints): API request with/without Authorization header
alt Authorization header present
Server->>Server: Allow request to proceed
else No Authorization header
Server-->>Client: Return 401 Unauthorized
endPossibly related PRs
Suggested labels
Poem
|
|
@eranco74 please take a look. Note that it still needs some work to avoid having the URLs as constants, and to make authentication optional. |
|
@jhernand this is great! |
jhernand
force-pushed
the
implement_oauth_endpoints
branch
from
7789052 to
bdec3d8
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@eranco74 added a |
|
@eranco74 please don't merge yet, I need to make the registration URL configurable. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
README.md (1)
72-87: Improve grammar and punctuation for better readability.The documentation is helpful, but could benefit from minor grammatical improvements.
Apply this diff to improve readability:
-The server can implement a subset of the OAuth protocol that MCP clients like the one in VScode -use to authenticate. To enable it set the environment variable `OAUTH_ENABLED` to true. Then when -you try to connect the MCP client will open a browser where you can provide your credentials, and +The server can implement a subset of the OAuth protocol that MCP clients like the one in VScode +use to authenticate. To enable it, set the environment variable `OAUTH_ENABLED` to true. Then, when +you try to connect, the MCP client will open a browser where you can provide your credentials, and the MCP client will request an access token that will then be used by the server to authenticate to the assisted installer REST API. -If you use this mechanism then you don't need to provide the `OFFLINE_TOKEN`. +If you use this mechanism, then you don't need to provide the `OFFLINE_TOKEN`. -Additionaly you can configure the OAuth authorization server and client identifier to use with the +Additionally, you can configure the OAuth authorization server and client identifier to use with the `OAUTH_URL` and `OAUTH_CLIENT` environment variables. The defaults are the following:server.py (4)
311-311: Add blank line before class definition.return InventoryClient(get_access_token()).update_host(host_id, infraenv_id, host_role=role).to_str() + class OAuthMiddleware(starlette.middleware.base.BaseHTTPMiddleware):
423-428: Consider making CORS settings configurable for production security.The current CORS configuration allows all origins with credentials, which could be a security risk in production environments.
Consider making CORS settings configurable via environment variables:
# Add configuration options cors_origins = os.getenv("CORS_ORIGINS", "*").split(",") cors_allow_credentials = os.getenv("CORS_ALLOW_CREDENTIALS", "true").lower() == "true" # Apply configuration app.add_middleware( starlette.middleware.cors.CORSMiddleware, allow_origins=cors_origins, allow_methods=["GET", "POST", "OPTIONS"], allow_credentials=cors_allow_credentials, )
400-400: Add blank lines after class definition.) + if __name__ == "__main__":
406-406: Consider using proper logging instead of print statement.Debug print statements should be replaced with proper logging for production code.
+import logging + +# At module level +logger = logging.getLogger(__name__) + # In main block - print(f"oauth_enabled: {oauth_enabled}") + logger.info(f"OAuth enabled: {oauth_enabled}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)server.py(4 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~75-~75: Possible missing comma found.
Context: ...n VScode use to authenticate. To enable it set the environment variable `OAUTH_ENA...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~76-~76: Possible missing comma found.
Context: ..._ENABLED` to true. Then when you try to connect the MCP client will open a browser wher...
(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~80-~80: Consider adding a comma.
Context: ...ler REST API. If you use this mechanism then you don't need to provide the `OFFLINE_...
(IF_THEN_COMMA)
[uncategorized] ~81-~81: Possible missing comma found.
Context: ...t need to provide the OFFLINE_TOKEN. Additionaly you can configure the OAuth authorizati...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 Flake8 (7.2.0)
server.py
[error] 311-311: expected 2 blank lines, found 1
(E302)
[error] 400-400: expected 2 blank lines after class or function definition, found 1
(E305)
🪛 Pylint (3.3.7)
server.py
[error] 364-364: Using variable 'oauth_url' before assignment
(E0601)
🔇 Additional comments (1)
server.py (1)
17-17: Good adaptation to the new FastMCP architecture.The changes correctly utilize the FastMCP dependency injection pattern for header retrieval and remove the redundant host parameter.
Also applies to: 38-39, 59-64
jhernand
force-pushed
the
implement_oauth_endpoints
branch
from
bdec3d8 to
023c53b
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
README.md (1)
72-94: Documentation looks comprehensive with minor grammar improvements needed.The OAuth support documentation effectively explains the feature and configuration options. However, there are a few punctuation issues to address for better readability.
Apply these grammar improvements:
-The server can implement a subset of the OAuth protocol that MCP clients like the one in VScode -use to authenticate. To enable it set the environment variable `OAUTH_ENABLED` to true. Then when -you try to connect the MCP client will open a browser where you can provide your credentials, and +The server can implement a subset of the OAuth protocol that MCP clients like the one in VSCode +use to authenticate. To enable it, set the environment variable `OAUTH_ENABLED` to true. Then, when +you try to connect, the MCP client will open a browser where you can provide your credentials, and-If you use this mechanism then you don't need to provide the `OFFLINE_TOKEN`. +If you use this mechanism, then you don't need to provide the `OFFLINE_TOKEN`.-Additionaly you can configure the OAuth authorization server and client identifier to use with the +Additionally, you can configure the OAuth authorization server and client identifier to use with theserver.py (3)
311-311: Fix formatting: Add required blank lines before class definition.+ class OAuthMiddleware(starlette.middleware.base.BaseHTTPMiddleware):
396-402: Consider input validation for OAuth registration endpoint.The OAuth registration endpoint accepts any JSON input without validation. While this may be intentional for a "fake" endpoint, consider adding basic validation to ensure the request contains expected fields.
async def oauth_register(self, request: starlette.requests.Request) -> starlette.responses.Response: """ This method implements the OAuth dynamic client registration endpoint. It responds to all requests with a fixed client identifier. """ - body = await request.json() + try: + body = await request.json() + if "redirect_uris" not in body: + return starlette.responses.JSONResponse( + status_code=400, + content={"error": "redirect_uris is required"} + ) + except Exception: + return starlette.responses.JSONResponse( + status_code=400, + content={"error": "Invalid JSON"} + ) return starlette.responses.JSONResponse( content={ "client_id": self._oauth_client, "redirect_uris": body["redirect_uris"], }, )
404-404: Fix formatting: Add required blank lines after class definition.) + if __name__ == "__main__":
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)server.py(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jhernand
PR: openshift-assisted/assisted-service-mcp#5
File: server.py:0-0
Timestamp: 2025-06-26T18:46:49.998Z
Learning: When constructing URLs for OAuth endpoints in middleware, use environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place, as the URL in the request may not be the URL that clients need to use.
server.py (1)
Learnt from: jhernand
PR: openshift-assisted/assisted-service-mcp#5
File: server.py:0-0
Timestamp: 2025-06-26T18:46:49.998Z
Learning: When constructing URLs for OAuth endpoints in middleware, use environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place, as the URL in the request may not be the URL that clients need to use.
🧬 Code Graph Analysis (1)
server.py (1)
service_client/assisted_service_api.py (1)
InventoryClient(11-129)
🪛 Flake8 (7.2.0)
server.py
[error] 311-311: expected 2 blank lines, found 1
(E302)
[error] 404-404: expected 2 blank lines after class or function definition, found 1
(E305)
🪛 LanguageTool
README.md
[uncategorized] ~75-~75: A comma might be missing here.
Context: ...n VScode use to authenticate. To enable it set the environment variable `OAUTH_ENA...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~76-~76: A comma might be missing here.
Context: ..._ENABLED` to true. Then when you try to connect the MCP client will open a browser wher...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[typographical] ~80-~80: Consider adding a comma.
Context: ...ler REST API. If you use this mechanism then you don't need to provide the `OFFLINE_...
(IF_THEN_COMMA)
[uncategorized] ~81-~81: Possible missing comma found.
Context: ...t need to provide the OFFLINE_TOKEN. Additionaly you can configure the OAuth authorizati...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (4)
server.py (4)
3-17: Import changes and FastMCP initialization look good.The transition from
mcp.server.fastmcptofastmcpand the addition of Starlette/Uvicorn imports align well with the new middleware-based architecture.
38-64: Function refactoring correctly uses new dependency system.The updates to
get_offline_token()andget_access_token()properly usefastmcp.server.dependencies.get_http_headers()instead of direct MCP request context access, which is the correct approach for the new architecture.
405-440: OAuth middleware integration implemented correctly.The main execution block properly:
- Creates a Starlette application from the MCP instance
- Conditionally adds OAuth middleware based on environment variables (aligning with retrieved learnings about using environment variables for URL construction)
- Configures CORS middleware appropriately
- Uses Uvicorn for serving
The use of
SELF_URLenvironment variable correctly addresses the learning about reverse proxy scenarios.
311-402: OAuth middleware implementation provides necessary functionality.The
OAuthMiddlewareclass effectively:
- Proxies OAuth metadata from the real authorization server while modifying necessary fields
- Implements a fake dynamic client registration endpoint
- Enforces authorization header presence for protected endpoints
This implementation aligns well with the PR objectives of enabling OAuth flow for MCP clients without requiring manual token provision.
jhernand
force-pushed
the
implement_oauth_endpoints
branch
from
023c53b to
d682cad
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (1)
72-94: Fix grammatical issues in the OAuth documentation.The documentation content is comprehensive and accurate, but there are several grammar and punctuation issues that should be addressed for better readability.
Apply these fixes to improve the grammar:
-The server can implement a subset of the OAuth protocol that MCP clients like the one in VScode -use to authenticate. To enable it set the environment variable `OAUTH_ENABLED` to true. Then when -you try to connect the MCP client will open a browser where you can provide your credentials, and +The server can implement a subset of the OAuth protocol that MCP clients like the one in VSCode +use to authenticate. To enable it, set the environment variable `OAUTH_ENABLED` to true. Then when +you try to connect, the MCP client will open a browser where you can provide your credentials, and the MCP client will request an access token that will then be used by the server to authenticate to the assisted installer REST API. -If you use this mechanism then you don't need to provide the `OFFLINE_TOKEN`. +If you use this mechanism, then you don't need to provide the `OFFLINE_TOKEN`. -Additionaly you can configure the OAuth authorization server and client identifier to use with the +Additionally, you can configure the OAuth authorization server and client identifier to use with theserver.py (2)
365-369: Consider adding timeout and error handling for external requests.The external HTTP request to the OAuth metadata endpoint should include timeout configuration and proper error handling for robustness.
# Get the metadata from the real authorization service: -response = requests.get( - url=f"{self._oauth_url}/.well-known/oauth-authorization-server", -) +try: + response = requests.get( + url=f"{self._oauth_url}/.well-known/oauth-authorization-server", + timeout=10, + ) +except requests.RequestException as e: + return starlette.responses.Response( + content=f"Failed to retrieve OAuth metadata: {e}", + status_code=502, + )
433-436: Review CORS configuration for production security.The current CORS configuration allows all origins (
allow_origins="*"), which may be too permissive for production environments. Consider making this configurable.# Add the CORS middleware: +cors_origins = os.getenv("CORS_ORIGINS", "*").split(",") app.add_middleware( starlette.middleware.cors.CORSMiddleware, - allow_origins="*", + allow_origins=cors_origins, allow_methods="*", allow_credentials=True, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)server.py(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jhernand
PR: openshift-assisted/assisted-service-mcp#5
File: server.py:0-0
Timestamp: 2025-06-26T18:46:49.998Z
Learning: When constructing URLs for OAuth endpoints in middleware, use environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place, as the URL in the request may not be the URL that clients need to use.
server.py (1)
Learnt from: jhernand
PR: openshift-assisted/assisted-service-mcp#5
File: server.py:0-0
Timestamp: 2025-06-26T18:46:49.998Z
Learning: When constructing URLs for OAuth endpoints in middleware, use environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place, as the URL in the request may not be the URL that clients need to use.
🧬 Code Graph Analysis (1)
server.py (1)
service_client/assisted_service_api.py (1)
InventoryClient(11-129)
🪛 Flake8 (7.2.0)
server.py
[error] 311-311: expected 2 blank lines, found 1
(E302)
[error] 403-403: expected 2 blank lines after class or function definition, found 1
(E305)
🪛 LanguageTool
README.md
[uncategorized] ~75-~75: A comma might be missing here.
Context: ...n VScode use to authenticate. To enable it set the environment variable `OAUTH_ENA...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~76-~76: A comma might be missing here.
Context: ..._ENABLED` to true. Then when you try to connect the MCP client will open a browser wher...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[typographical] ~80-~80: Consider adding a comma.
Context: ...ler REST API. If you use this mechanism then you don't need to provide the `OFFLINE_...
(IF_THEN_COMMA)
[uncategorized] ~81-~81: Possible missing comma found.
Context: ...t need to provide the OFFLINE_TOKEN. Additionaly you can configure the OAuth authorizati...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (4)
server.py (4)
1-17: Import changes look good.The transition from
mcp.server.fastmcpto the standalonefastmcppackage is properly implemented with all necessary Starlette and Uvicorn imports added for the new middleware functionality.
38-64: Approve header access method changes.The functions correctly use
fastmcp.server.dependencies.get_http_headers()instead of direct request object access, which is the proper way to access HTTP headers in the updated fastmcp framework.
311-401: OAuth middleware implementation is well-designed.The middleware correctly implements the OAuth flow requirements:
- Properly handles OAuth metadata endpoint by proxying to the real authorization server
- Correctly replaces the registration endpoint URL using the
SELF_URLenvironment variable (following the retrieved learning)- Appropriately limits supported scopes to prevent authorization server rejections
- Implements fake dynamic client registration as intended
- Enforces authorization header presence for protected endpoints
The security model is appropriate for the use case - requiring only the presence of an authorization header to trigger the OAuth flow rather than validating the token at the middleware level.
403-439: Main execution block implementation is excellent.The restructuring to use Starlette application with middleware support is well-implemented:
- Properly creates HTTP app for SSE transport
- Correctly implements conditional OAuth middleware based on environment variable
- Uses appropriate environment variable defaults that match the documentation
- Properly configures CORS middleware for browser-based OAuth flows
- Uses Uvicorn as the ASGI server appropriately
The debug print statement helps with troubleshooting OAuth configuration.
jhernand
force-pushed
the
implement_oauth_endpoints
branch
from
d682cad to
ea51a8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server.py (1)
365-370: Add timeout and error handling for external HTTP request.The external request to the OAuth authorization server lacks timeout and proper error handling, which could cause the middleware to hang or fail ungracefully.
# Get the metadata from the real authorization service: - response = requests.get( - url=f"{self._oauth_url}/.well-known/oauth-authorization-server", - ) - response.raise_for_status() + try: + response = requests.get( + url=f"{self._oauth_url}/.well-known/oauth-authorization-server", + timeout=10, + ) + response.raise_for_status() + except requests.RequestException as e: + return starlette.responses.JSONResponse( + content={"error": "Failed to fetch OAuth metadata"}, + status_code=503, + )
🧹 Nitpick comments (3)
server.py (3)
310-311: Add missing blank lines for PEP 8 compliance.Static analysis indicates missing blank lines before the class definition.
+ + class OAuthMiddleware(starlette.middleware.base.BaseHTTPMiddleware):
402-403: Add missing blank lines for PEP 8 compliance.Static analysis indicates missing blank lines after the class definition.
) + if __name__ == "__main__":
429-435: Consider restricting CORS configuration for production.The current CORS configuration allows all origins with credentials, which may be too permissive for production environments.
Consider making CORS configuration environment-driven:
# Add the CORS middleware: + cors_origins = os.getenv("CORS_ORIGINS", "*").split(",") app.add_middleware( starlette.middleware.cors.CORSMiddleware, - allow_origins="*", + allow_origins=cors_origins, allow_methods="*", allow_credentials=True, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(2 hunks)server.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jhernand
PR: openshift-assisted/assisted-service-mcp#5
File: server.py:0-0
Timestamp: 2025-06-26T18:46:49.998Z
Learning: When constructing URLs for OAuth endpoints in middleware, use environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place, as the URL in the request may not be the URL that clients need to use.
server.py (1)
Learnt from: jhernand
PR: openshift-assisted/assisted-service-mcp#5
File: server.py:0-0
Timestamp: 2025-06-26T18:46:49.998Z
Learning: When constructing URLs for OAuth endpoints in middleware, use environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place, as the URL in the request may not be the URL that clients need to use.
🧬 Code Graph Analysis (1)
server.py (1)
service_client/assisted_service_api.py (1)
InventoryClient(11-129)
🪛 Flake8 (7.2.0)
server.py
[error] 311-311: expected 2 blank lines, found 1
(E302)
[error] 403-403: expected 2 blank lines after class or function definition, found 1
(E305)
🔇 Additional comments (3)
server.py (3)
4-13: Import changes look good.The new imports properly support the OAuth middleware and Starlette-based architecture changes.
38-39: Header access implementation is correct.The changes properly use the new fastmcp dependency injection system for accessing HTTP headers.
Also applies to: 59-65
404-438: OAuth configuration follows best practices.The use of environment variables for OAuth configuration correctly follows the established pattern of using
SELF_URLfor endpoint construction, which properly handles reverse proxy scenarios.
| async def oauth_register(self, request: starlette.requests.Request) -> starlette.responses.Response: | ||
| """ | ||
| This method implements the OAuth dynamic client registration endpoint. It responds to all requests | ||
| with a fixed client identifier. | ||
| """ | ||
| body = await request.json() | ||
| return starlette.responses.JSONResponse( | ||
| content={ | ||
| "client_id": self._oauth_client, | ||
| "redirect_uris": body["redirect_uris"], | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for OAuth registration request.
The method assumes the request body contains "redirect_uris" without validation, which could cause a KeyError if the field is missing.
async def oauth_register(self, request: starlette.requests.Request) -> starlette.responses.Response:
"""
This method implements the OAuth dynamic client registration endpoint. It responds to all requests
with a fixed client identifier.
"""
- body = await request.json()
+ try:
+ body = await request.json()
+ if "redirect_uris" not in body:
+ return starlette.responses.JSONResponse(
+ content={"error": "redirect_uris is required"},
+ status_code=400,
+ )
+ except Exception:
+ return starlette.responses.JSONResponse(
+ content={"error": "Invalid JSON in request body"},
+ status_code=400,
+ )
return starlette.responses.JSONResponse(
content={
"client_id": self._oauth_client,
"redirect_uris": body["redirect_uris"],
},
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def oauth_register(self, request: starlette.requests.Request) -> starlette.responses.Response: | |
| """ | |
| This method implements the OAuth dynamic client registration endpoint. It responds to all requests | |
| with a fixed client identifier. | |
| """ | |
| body = await request.json() | |
| return starlette.responses.JSONResponse( | |
| content={ | |
| "client_id": self._oauth_client, | |
| "redirect_uris": body["redirect_uris"], | |
| }, | |
| ) | |
| async def oauth_register(self, request: starlette.requests.Request) -> starlette.responses.Response: | |
| """ | |
| This method implements the OAuth dynamic client registration endpoint. It responds to all requests | |
| with a fixed client identifier. | |
| """ | |
| try: | |
| body = await request.json() | |
| if "redirect_uris" not in body: | |
| return starlette.responses.JSONResponse( | |
| content={"error": "redirect_uris is required"}, | |
| status_code=400, | |
| ) | |
| except Exception: | |
| return starlette.responses.JSONResponse( | |
| content={"error": "Invalid JSON in request body"}, | |
| status_code=400, | |
| ) | |
| return starlette.responses.JSONResponse( | |
| content={ | |
| "client_id": self._oauth_client, | |
| "redirect_uris": body["redirect_uris"], | |
| }, | |
| ) |
🤖 Prompt for AI Agents
In server.py around lines 390 to 401, the oauth_register method assumes the
request body contains "redirect_uris" without validation, risking a KeyError.
Add input validation to check if "redirect_uris" exists in the request JSON
before accessing it. If missing, return an appropriate error response indicating
the required field is absent. This will prevent runtime errors and improve
robustness.
ea51a8b to
b9281b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
server.py (1)
395-398: Add timeout to external HTTP request.The external request to the OAuth authorization server lacks a timeout, which could cause the middleware to hang indefinitely if the server is unresponsive.
response = requests.get( url=f"{self._oauth_url}/.well-known/oauth-authorization-server", + timeout=10, )
🧹 Nitpick comments (4)
server.py (4)
14-14: Remove unused import.The
urllib.parseimport is not used anywhere in the code.-import urllib.parse
312-312: Fix formatting: Add required blank lines.Class definitions should be preceded by two blank lines according to PEP 8.
+ + class OAuthMiddleware(starlette.middleware.base.BaseHTTPMiddleware):
424-431: Consider adding error handling for malformed JSON.While the code handles missing
redirect_urisproperly, it doesn't handle malformed JSON in the request body.async def oauth_register(self, request: starlette.requests.Request) -> starlette.responses.Response: """ This method implements the OAuth dynamic client registration endpoint. It responds to all requests with a fixed client identifier. """ - body = await request.json() + try: + body = await request.json() + except Exception: + return starlette.responses.JSONResponse( + content={"error": "Invalid JSON in request body"}, + status_code=400, + ) redirect_uris = body.get("redirect_uris", []) return starlette.responses.JSONResponse( content={ "client_id": self._oauth_client, "redirect_uris": redirect_uris, }, )
433-433: Fix formatting: Add blank line after class definition.There should be two blank lines after class or function definitions according to PEP 8.
) + if __name__ == "__main__":
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(2 hunks)server.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jhernand
PR: openshift-assisted/assisted-service-mcp#5
File: server.py:0-0
Timestamp: 2025-06-26T18:46:49.998Z
Learning: When constructing URLs for OAuth endpoints in middleware, use environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place, as the URL in the request may not be the URL that clients need to use.
server.py (1)
Learnt from: jhernand
PR: openshift-assisted/assisted-service-mcp#5
File: server.py:0-0
Timestamp: 2025-06-26T18:46:49.998Z
Learning: When constructing URLs for OAuth endpoints in middleware, use environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place, as the URL in the request may not be the URL that clients need to use.
🪛 Ruff (0.11.9)
server.py
14-14: urllib.parse imported but unused
Remove unused import: urllib.parse
(F401)
🪛 Flake8 (7.2.0)
server.py
[error] 14-14: 'urllib.parse' imported but unused
(F401)
[error] 312-312: expected 2 blank lines, found 1
(E302)
[error] 433-433: expected 2 blank lines after class or function definition, found 1
(E305)
🔇 Additional comments (3)
server.py (3)
39-40: Good API migration.The update to use
fastmcp.server.dependencies.get_http_headers()correctly follows the new API pattern.
60-65: Improved authorization header parsing.The enhanced logic for parsing the Authorization header with proper splitting and validation is well implemented.
434-468: Well-implemented OAuth integration with proper configuration.The main execution block correctly:
- Creates a Starlette application from the MCP instance
- Conditionally enables OAuth middleware based on environment variables
- Provides sensible defaults for OAuth configuration
- Adds CORS middleware for cross-origin support
- Uses appropriate uvicorn configuration
The environment variable approach aligns with the learning about using environment variables instead of request URLs for OAuth endpoints, which is important for reverse proxy scenarios.
jhernand
force-pushed
the
implement_oauth_endpoints
branch
from
b9281b9 to
3029d8c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server.py (1)
434-469: Good implementation of configurable OAuth with proper middleware setup.The main execution block correctly implements the requirements:
- Conditional OAuth enablement via
OAUTH_ENABLEDenvironment variable addresses the PR objective- Sensible defaults for OAuth configuration parameters
- Proper Starlette application creation for middleware support
- CORS middleware addition for client compatibility
Consider tightening CORS configuration for production deployments, as the current settings (
allow_origins="*"withallow_credentials=True) are very permissive and could pose security risks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(2 hunks)server.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jhernand
PR: openshift-assisted/assisted-service-mcp#5
File: server.py:0-0
Timestamp: 2025-06-26T18:46:49.998Z
Learning: When constructing URLs for OAuth endpoints in middleware, use environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place, as the URL in the request may not be the URL that clients need to use.
server.py (1)
Learnt from: jhernand
PR: openshift-assisted/assisted-service-mcp#5
File: server.py:0-0
Timestamp: 2025-06-26T18:46:49.998Z
Learning: When constructing URLs for OAuth endpoints in middleware, use environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place, as the URL in the request may not be the URL that clients need to use.
🔇 Additional comments (4)
server.py (4)
1-13: Import statements look good for OAuth implementation.The migration to
fastmcppackage and addition of Starlette/Uvicorn imports properly support the new OAuth middleware functionality.
17-17: Good simplification of MCP instance creation.Removing the host parameter is appropriate since host configuration is now handled by
uvicorn.run()in the main execution block.
38-64: Excellent migration to dependency injection for header access.The update to use
fastmcp.server.dependencies.get_http_headers()improves testability and follows better architectural patterns while maintaining the same authentication logic.
312-431: Well-implemented OAuth middleware with proper architecture.The
OAuthMiddlewareclass correctly implements the required OAuth endpoints with good separation of concerns:
- Proper routing of OAuth endpoints without authentication requirements
- Correct 401 response with
WWW-Authenticateheader for protected endpoints- Environment variable usage for URLs follows the established learning about reverse proxy scenarios
- Good error handling for external OAuth metadata requests
- Safe handling of JSON request body in registration endpoint using
.get()methodThe implementation successfully enables MCP clients to complete OAuth flows as intended.
|
@eranco74 I think this is ready to merge now. |
| return InventoryClient(get_access_token()).update_host(host_id, infraenv_id, host_role=role).to_str() | ||
|
|
||
|
|
||
| class OAuthMiddleware(starlette.middleware.base.BaseHTTPMiddleware): |
There was a problem hiding this comment.
Do you think this might be better in a separate file? server.py is getting a bit busy.
There was a problem hiding this comment.
Yes, it can be in a separate file, will do that.
| async def oauth_register(self, request: starlette.requests.Request) -> starlette.responses.Response: | ||
| """ | ||
| This method implements the OAuth dynamic client registration endpoint. It responds to all requests | ||
| with a fixed client identifier. |
There was a problem hiding this comment.
What does this mean? Is there any problem with reusing a client identifier? (disclaimer, I'm not very familiar with oauth)
There was a problem hiding this comment.
The "client" is the application that is requesting access to the protected resources. If we were strict this should be something like "VScode", "Cursor", etc. But that would mean requesting the creation of such clients to the sso.redhat.com team. Instead we use "cloud-services" that is already used by plenty of applications in OCM. There is no problem in reusing it, as it isn't an confidential client.
| # Add the CORS middleware: | ||
| app.add_middleware( | ||
| starlette.middleware.cors.CORSMiddleware, | ||
| allow_origins="*", | ||
| allow_methods="*", | ||
| allow_credentials=True, | ||
| ) |
There was a problem hiding this comment.
Should this only be added if oauth_enabled is true?
There was a problem hiding this comment.
I am not sure. CORS would be useful even if there was no OAuth authorization. For example, if access was completely anonymous CORS would still be needed for clients like the MCP inspector, which runs in a browser. Actually that is the reason I added this: to be able to use the MCP inspector. But the MCP inspector combined with OAuth doesn't work anyhow, because sso.redhat.com would also need to support it. So I think I will CORS support completely, we can add it later.
jhernand
force-pushed
the
implement_oauth_endpoints
branch
from
3029d8c to
a343a0f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
oauth.py (1)
124-136: Add input validation and error handling for JSON parsing.The method lacks proper error handling for JSON parsing and input validation, which could cause runtime errors if the request body is malformed or missing expected fields.
Apply this diff to add proper error handling:
async def _register(self, request: starlette.requests.Request) -> starlette.responses.Response: """ This method implements the OAuth dynamic client registration endpoint. It responds to all requests with a fixed client identifier. """ - body = await request.json() - redirect_uris = body.get("redirect_uris", []) + try: + body = await request.json() + except Exception: + return starlette.responses.JSONResponse( + content={"error": "Invalid JSON in request body"}, + status_code=400, + ) + + redirect_uris = body.get("redirect_uris", []) return starlette.responses.JSONResponse( content={ "client_id": self._oauth_client, "redirect_uris": redirect_uris, }, )
🧹 Nitpick comments (1)
oauth.py (1)
2-2: Fix typo in module docstring.There's a typo in the module docstring: "implemnts" should be "implements".
-This module contains the Starlette middleware that implemnts OAuth authorization. +This module contains the Starlette middleware that implements OAuth authorization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(2 hunks)oauth.py(1 hunks)server.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: jhernand
PR: openshift-assisted/assisted-service-mcp#5
File: server.py:0-0
Timestamp: 2025-06-26T18:46:49.998Z
Learning: When constructing URLs for OAuth endpoints in middleware, use environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place, as the URL in the request may not be the URL that clients need to use.
server.py (1)
Learnt from: jhernand
PR: openshift-assisted/assisted-service-mcp#5
File: server.py:0-0
Timestamp: 2025-06-26T18:46:49.998Z
Learning: When constructing URLs for OAuth endpoints in middleware, use environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place, as the URL in the request may not be the URL that clients need to use.
oauth.py (1)
Learnt from: jhernand
PR: openshift-assisted/assisted-service-mcp#5
File: server.py:0-0
Timestamp: 2025-06-26T18:46:49.998Z
Learning: When constructing URLs for OAuth endpoints in middleware, use environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place, as the URL in the request may not be the URL that clients need to use.
🧬 Code Graph Analysis (1)
server.py (2)
service_client/assisted_service_api.py (1)
InventoryClient(11-129)oauth.py (1)
Middleware(13-136)
🪛 Pylint (3.3.7)
oauth.py
[refactor] 13-13: Too few public methods (1/2)
(R0903)
🔇 Additional comments (10)
oauth.py (5)
5-10: LGTM!The imports are well-organized and include all necessary Starlette components for middleware implementation.
19-38: LGTM!The constructor properly initializes the middleware with required OAuth configuration parameters and follows proper inheritance patterns.
40-71: LGTM!The dispatch method correctly routes OAuth endpoints without authentication and enforces the presence of an Authorization header for all other requests. The implementation properly follows the learning about using environment variables (self._self_url) instead of request URLs for constructing OAuth endpoints.
73-91: LGTM!The protected resource metadata endpoint correctly returns the required OAuth 2.0 resource metadata format with appropriate scopes and authorization server information.
93-122: LGTM!The metadata endpoint implementation correctly fetches from the real authorization server, modifies the registration endpoint to point to the local endpoint, and restricts scopes to prevent authorization failures. The timeout parameter and error handling are appropriate.
server.py (5)
3-9: LGTM!The import changes properly transition from direct MCP imports to using fastmcp and add necessary dependencies for OAuth integration and uvicorn server.
13-13: LGTM!The simplified FastMCP initialization removes unnecessary explicit host parameter, improving configurability.
34-36: LGTM!The change to use dependency injection via
fastmcp.server.dependencies.get_http_headers()instead of direct request context access improves the architecture and follows proper dependency injection patterns.
55-60: LGTM!Consistent application of the new dependency injection pattern for HTTP header access, maintaining consistency with the previous change.
308-334: LGTM!The OAuth middleware integration is well-implemented with proper conditional enabling based on environment variables. The configuration uses sensible defaults and follows the established learning about using environment variables for URL construction. The transition to using uvicorn directly provides better control over the server configuration.
eranco74
left a comment
eranco74
left a comment
There was a problem hiding this comment.
Looks good to me.
Just a nit
Can you update the readme with the auth options priority/exclusion.
What happens if I pass the offline token when OAUTH_ENABLED is enabled?
Will passing the access token (as we currently do from LSC) still work?
| from service_client import InventoryClient | ||
|
|
||
| mcp = FastMCP("AssistedService", host="0.0.0.0") | ||
| mcp = fastmcp.FastMCP("AssistedService") |
There was a problem hiding this comment.
Why? I think the default is 127.0.0.1
There was a problem hiding this comment.
The default may be 127.0.0.1, but it isn't relevant anymore because we don't run the MCP directly. Instead we create a Starlette application and start it with this:
uvicorn.run(app, host="0.0.0.0", port=8000)See the __main__ code for details.
Yes, I will.
It depends on your client. If your client knows how to use OAuth (VScode or Cursor, for example) then it will initiate the OAuth process and when that finishes it will get an access token and send it with every request. As a result the If your client doesn't know how to use OAuth then access will be rejected, and you need to either set the
Yes, if the access token is present in the |
jhernand
force-pushed
the
implement_oauth_endpoints
branch
from
a343a0f to
2e50a7d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
oauth.py (2)
2-2: Fix typo in module docstring.There's a spelling error in the docstring.
-This module contains the Starlette middleware that implemnts OAuth authorization. +This module contains the Starlette middleware that implements OAuth authorization.
124-136: Consider adding basic validation for dynamic client registration.The current implementation accepts any request body without validation. Consider adding basic checks for required OAuth dynamic registration parameters.
async def _register(self, request: starlette.requests.Request) -> starlette.responses.Response: """ This method implements the OAuth dynamic client registration endpoint. It responds to all requests with a fixed client identifier. """ try: body = await request.json() except Exception: return starlette.responses.JSONResponse( status_code=400, content={"error": "invalid_request", "error_description": "Invalid JSON in request body"}, ) + + # Basic validation for redirect_uris + redirect_uris = body.get("redirect_uris", []) + if redirect_uris and not isinstance(redirect_uris, list): + return starlette.responses.JSONResponse( + status_code=400, + content={"error": "invalid_request", "error_description": "redirect_uris must be an array"}, + ) + - redirect_uris = body.get("redirect_uris", []) return starlette.responses.JSONResponse( content={ "client_id": self._oauth_client, "redirect_uris": redirect_uris, }, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(2 hunks)oauth.py(1 hunks)server.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- server.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jhernand
PR: openshift-assisted/assisted-service-mcp#5
File: server.py:0-0
Timestamp: 2025-06-26T18:46:49.998Z
Learning: When constructing URLs for OAuth endpoints in middleware, use environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place, as the URL in the request may not be the URL that clients need to use.
oauth.py (1)
Learnt from: jhernand
PR: openshift-assisted/assisted-service-mcp#5
File: server.py:0-0
Timestamp: 2025-06-26T18:46:49.998Z
Learning: When constructing URLs for OAuth endpoints in middleware, use environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place, as the URL in the request may not be the URL that clients need to use.
🪛 Pylint (3.3.7)
oauth.py
[refactor] 13-13: Too few public methods (1/2)
(R0903)
🔇 Additional comments (2)
oauth.py (2)
63-63: Excellent use of environment variable for URL construction.Good implementation following the retrieved learning about using environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place.
48-71: Authentication logic correctly implements OAuth flow initiation.The middleware correctly handles the OAuth endpoints without authentication and returns proper 401 responses with WWW-Authenticate headers for protected resources, which will trigger OAuth flow in compliant clients.
| This method implements the OAuth dynamic client registration endpoint. It responds to all requests | ||
| with a fixed client identifier. | ||
| """ | ||
| body = await request.json() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for JSON parsing.
The await request.json() call can raise exceptions if the request body is malformed JSON or missing. This should be handled gracefully.
- body = await request.json()
+ try:
+ body = await request.json()
+ except Exception:
+ return starlette.responses.JSONResponse(
+ status_code=400,
+ content={"error": "invalid_request", "error_description": "Invalid JSON in request body"},
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| body = await request.json() | |
| try: | |
| body = await request.json() | |
| except Exception: | |
| return starlette.responses.JSONResponse( | |
| status_code=400, | |
| content={"error": "invalid_request", "error_description": "Invalid JSON in request body"}, | |
| ) |
🤖 Prompt for AI Agents
In oauth.py at line 129, the call to await request.json() can raise exceptions
if the JSON is malformed or missing. Wrap this call in a try-except block to
catch JSON parsing errors, and handle them gracefully by returning an
appropriate error response or message instead of letting the exception
propagate.
2e50a7d to
fa1e4fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
oauth.py (3)
99-104: Replace synchronous HTTP call with asynchronous version.Using
requests.get()in an async method blocks the event loop and degrades performance. This issue was previously identified and should be addressed.
98-104: Add error handling for external OAuth server failures.The code only uses
raise_for_status()for error handling but lacks handling for network issues. This was previously identified and should be addressed with proper exception handling and fallback behavior.
129-129: Add error handling for JSON parsing.The
await request.json()call can raise exceptions if the request body is malformed JSON. This was previously identified and should be handled gracefully.
🧹 Nitpick comments (2)
oauth.py (1)
2-2: Fix typo in module docstring."implemnts" should be "implements".
-This module contains the Starlette middleware that implemnts OAuth authorization. +This module contains the Starlette middleware that implements OAuth authorization.README.md (1)
167-168: Fix markdown list formatting for consistency.The static analysis tool flags inconsistent list formatting. Consider using asterisks instead of dashes for unordered lists to maintain consistency with the rest of the document.
-- `OAUTH_URL`: `https://sso.redhat.com/auth/realms/redhat-external` -- `OAUTH_CLIENT`: `cloud-services` +* `OAUTH_URL`: `https://sso.redhat.com/auth/realms/redhat-external` +* `OAUTH_CLIENT`: `cloud-services`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(2 hunks)oauth.py(1 hunks)server.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jhernand
PR: openshift-assisted/assisted-service-mcp#5
File: server.py:0-0
Timestamp: 2025-06-26T18:46:49.998Z
Learning: When constructing URLs for OAuth endpoints in middleware, use environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place, as the URL in the request may not be the URL that clients need to use.
oauth.py (1)
Learnt from: jhernand
PR: openshift-assisted/assisted-service-mcp#5
File: server.py:0-0
Timestamp: 2025-06-26T18:46:49.998Z
Learning: When constructing URLs for OAuth endpoints in middleware, use environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place, as the URL in the request may not be the URL that clients need to use.
🪛 Pylint (3.3.7)
oauth.py
[refactor] 13-13: Too few public methods (1/2)
(R0903)
🪛 LanguageTool
README.md
[uncategorized] ~167-~167: Loose punctuation mark.
Context: .... The default values are: - OAUTH_URL: `https://sso.redhat.com/auth/realms/red...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
README.md
167-167: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
168-168: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
🔇 Additional comments (5)
oauth.py (3)
63-63: Consistent use of environment variable for self URL construction.Based on the retrieved learning, when constructing URLs for OAuth endpoints in middleware, use environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place. The code correctly uses
self._self_urlhere, which aligns with the best practice.
13-38: Well-structured middleware initialization.The middleware constructor properly accepts all necessary configuration parameters and follows Starlette middleware patterns. The parameter documentation is clear and comprehensive.
40-71: Effective request routing and authentication enforcement.The dispatch method properly routes OAuth endpoints and enforces authentication for protected endpoints. The approach of checking for the authorization header presence (without validation) to trigger OAuth flow is appropriate for this use case.
README.md (2)
57-58: Good restructuring of content flow.Moving the "Ask about your clusters" section earlier improves the document's logical flow by placing usage examples closer to the setup instructions.
139-185: Comprehensive and accurate OAuth documentation.The new "Authorization Options" section provides clear, well-structured documentation of the authentication methods. The priority order, configuration parameters, and detailed explanations align well with the implementation in oauth.py.
This patch implements enough OAuth endpoints to make the server usable from MCP clients like VScode without having to manually provide a token. When the MCP client receives a 401 code from the server it will assume that it uses OAuth, and it will try to fetch the OAuth metadata from `/.well-known/oauth-authorization-server`. As we don't want to filly implement that, we implement it calling the equivalent endpoint of _sso.redhat.com_. We then modify a couple of things in the content, and return it to the client. One of the things that we modify is the dynamic client registration endpoint. The MCP client will try to use the one in the metadata, but _sso.redhat.com_ doesn't allow that. So we replace it with our own, where we accept all requests and respond always with the same client identifier. With this the MCP client is able to complete the OAuth process, and obtain an access token that will then use to call the MCP. The MCP will then in turn use it to call the assisted installer REST API. Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
jhernand
force-pushed
the
implement_oauth_endpoints
branch
from
fa1e4fc to
b38232d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
oauth.py (1)
133-133: Add error handling for JSON parsing.The
await request.json()call can raise exceptions if the request body contains malformed JSON or is missing. This should be handled gracefully to prevent the middleware from crashing.- body = await request.json() + try: + body = await request.json() + except Exception: + return starlette.responses.JSONResponse( + status_code=400, + content={"error": "invalid_request", "error_description": "Invalid JSON in request body"}, + )
🧹 Nitpick comments (3)
oauth.py (2)
2-2: Fix typo in module docstring.There's a typo in "implemnts" - it should be "implements".
-This module contains the Starlette middleware that implemnts OAuth authorization. +This module contains the Starlette middleware that implements OAuth authorization.
113-121: Consider adding validation for scope restrictions.While the scope filtering is a good security practice, consider adding validation to ensure only expected scopes are included and log any unexpected scopes from the upstream server for monitoring purposes.
# The MCP clients also try to request all the scopes listed in the metadata, but our authorization # server returns a lot of scopes, and most of them will be rejected for our client. So we replace # that large list with a much smaller list containing only the scopes that we need. - body["scopes_supported"] = [ - "openid", - "api.ocm", - ] + expected_scopes = ["openid", "api.ocm"] + original_scopes = body.get("scopes_supported", []) + unexpected_scopes = [s for s in original_scopes if s not in expected_scopes] + if unexpected_scopes: + # Consider logging unexpected scopes for monitoring + pass + body["scopes_supported"] = expected_scopesREADME.md (1)
167-168: Fix markdown list formatting inconsistency.The static analysis correctly identified inconsistent list formatting. The document uses asterisks (
*) for other lists but dashes (-) here. Maintain consistency throughout the document.- - `OAUTH_URL`: `https://sso.redhat.com/auth/realms/redhat-external` - - `OAUTH_CLIENT`: `cloud-services` + * `OAUTH_URL`: `https://sso.redhat.com/auth/realms/redhat-external` + * `OAUTH_CLIENT`: `cloud-services`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(2 hunks)oauth.py(1 hunks)server.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jhernand
PR: openshift-assisted/assisted-service-mcp#5
File: server.py:0-0
Timestamp: 2025-06-26T18:46:49.998Z
Learning: When constructing URLs for OAuth endpoints in middleware, use environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place, as the URL in the request may not be the URL that clients need to use.
oauth.py (1)
Learnt from: jhernand
PR: openshift-assisted/assisted-service-mcp#5
File: server.py:0-0
Timestamp: 2025-06-26T18:46:49.998Z
Learning: When constructing URLs for OAuth endpoints in middleware, use environment variables (like SELF_URL) instead of request URLs when there may be reverse proxies in place, as the URL in the request may not be the URL that clients need to use.
🪛 LanguageTool
README.md
[uncategorized] ~167-~167: Loose punctuation mark.
Context: .... The default values are: - OAUTH_URL: `https://sso.redhat.com/auth/realms/red...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
README.md
167-167: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
168-168: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
🪛 Pylint (3.3.7)
oauth.py
[refactor] 13-13: Too few public methods (1/2)
(R0903)
🔇 Additional comments (4)
oauth.py (2)
99-108: Excellent async HTTP implementation with proper error handling.The implementation correctly uses
httpx.AsyncClientfor non-blocking HTTP calls and includes proper exception handling for network and HTTP errors. This addresses the concerns from previous reviews about blocking the event loop and handling external server failures.
63-63: URL construction follows best practices from retrieved learnings.The implementation correctly uses the
self._self_urlenvironment variable instead of extracting URLs from the request, which aligns with the retrieved learning about handling reverse proxies properly.README.md (2)
139-185: Comprehensive and accurate OAuth documentation.The authorization options section provides clear, well-structured documentation that accurately reflects the implementation. The priority ordering, configuration details, and examples are helpful for users. The explanation of URL construction for reverse proxy scenarios aligns well with the implementation approach.
154-162: Verify OAuth flow behavior with MCP clients.
The server-side code configures the OAuth endpoints, but the browser-launch behavior is implemented in the MCP client library. Please confirm that all supported MCP clients (e.g., the VS Code extension, the Cursor CLI, etc.) actually open a browser window when you initiate OAuth authentication. If any client differs, update the README to accurately describe each client’s user experience.
|
|
||
| token = mcp.get_context().request_context.request.headers.get("OCM-Offline-Token") | ||
| headers = fastmcp.server.dependencies.get_http_headers() | ||
| token = headers.get("ocm-offline-token") |
There was a problem hiding this comment.
Why change this? Also I assume this header name is not case sensitive?
There was a problem hiding this comment.
As part of the change we are importing fastmcp instead of mcp.server.fastmcp. One of the differences is that there is no get_context() method. Instead of that there is the fastmcp.server.dependencies package that provides methods (like get_http_headers) to get the dependencies.
Changing the imported package was necessary because the mcp.server.fastmcp doesn't expose the functionality to create a Starlette applicaton from the FastMCP instance, and I needed that to be able to add the middleware.
The get_http_headers method returns a plain Python dictionary witht the headers converted to lower case:
For more information, this is what Gemini has to say about the differences between fastmcp and mcp.server.fastmcp:
The relationship between "fastmcp" and "mcp.server.fastmcp" in Python is a matter of packaging and evolution within the Model Context Protocol (MCP) ecosystem.
Here's the breakdown:
fastmcp(the standalone library):
- This refers to the
fastmcpPython package that you would install directly (e.g.,pip install fastmcp).- It's a high-level, Pythonic framework designed to dramatically simplify the process of building MCP servers.
- It abstracts away many of the complexities of the raw MCP protocol, offering a decorator-based API (
@mcp.tool(),@mcp.resource(),@mcp.prompt()) that makes it easy to define functions that expose capabilities to Large Language Models (LLMs).- It aims to provide a fast and simple way for Python developers to create MCP servers.
mcp.server.fastmcp(part of the official MCP Python SDK):
- This refers to
fastmcpas it is integrated within the officialmodelcontextprotocol/python-sdk.- Crucially, FastMCP 1.0 was so successful that it was directly incorporated into the official MCP Python SDK. This means that for basic use cases, you can often use
mcp.server.fastmcpdirectly if you have the SDK installed.- It represents the "upstream" version of FastMCP for those who are using the broader MCP Python SDK.
In essence:
fastmcpis the project/framework that originated as a simpler way to build MCP servers.mcp.server.fastmcpis the module within the officialmodelcontextprotocolSDK that contains the FastMCP implementation.Why the two ways of importing/using?
- Historical Context: FastMCP was initially a separate project that proved to be very effective. The developers of the official MCP Python SDK recognized its value and integrated it.
- Version Evolution: While
mcp.server.fastmcpin the SDK might represent a stable version (often FastMCP 1.0), the standalonefastmcppackage continues to evolve with newer versions (e.g., FastMCP 2.0 and beyond) that introduce advanced features like proxying, composing servers, and generating servers from OpenAPI specs.- Choice for Developers:
- If you're starting a new project and want the latest features and a more direct experience with the FastMCP framework, you'd likely install
fastmcpas a standalone package.- If you're already using the broader MCP Python SDK and only need basic MCP server functionalities, the
mcp.server.fastmcpmodule within the SDK might suffice.General Recommendation:
For new projects, it's generally recommended to install and use the standalone
fastmcppackage, as it is the actively maintained version and provides the most up-to-date features for building MCP servers.
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
I am moving this to draft again because in my latest tests I wasn't able to connect with VS code: the authorization server is rejecting the redirect URI. This worked fine a few days ago, I wonder if something has changed in the configuration of the authorization server. I will check. Don't merge it yet. |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
4 participants
This patch implements enough OAuth endpoints to make the server usable from MCP clients like VScode without having to manually provide a token.
When the MCP client receives a 401 code from the server it will assume that it uses OAuth, and it will try to fetch the OAuth metadata from
/.well-known/oauth-authorization-server. As we don't want to filly implement that, we implement it calling the equivalent endpoint of sso.redhat.com. We then modify a couple of things in the content, and return it to the client.One of the things that we modify is the dynamic client registration endpoint. The MCP client will try to use the one in the metadata, but sso.redhat.com doesn't allow that. So we replace it with our own, where we accept all requests and respond always with the same client identifier.
With this the MCP client is able to complete the OAuth process, and obtain an access token that will then use to call the MCP. The MCP will then in turn use it to call the assisted installer REST API.
Summary by CodeRabbit
New Features
Documentation