Add OAuth authentication support #157
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eranco74 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughAdds full OAuth support to the Assisted Service MCP Server: new oauth package (manager, middleware, models, store, utils), integrates OAuth into the API and MCP token flow, updates settings and auth priority (Authorization header → OAuth → offline), adds docs, scripts, dependencies, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MCP Client
participant Server as MCP Server
participant OAuth as OAuth Provider
participant Browser as Browser
rect rgb(230,240,255)
Client->>Server: MCP Request (may include Authorization header)
end
alt Authorization header present
Server->>Server: Use header token (priority 1)
Server->>Client: Proceed
else No header & OAuth enabled
Server->>Server: Check cached token
alt Cached valid token
Server->>Client: Proceed with cached token
else No valid token
Server->>Server: Generate PKCE & auth URL
Server->>Browser: Open auth URL
Browser->>OAuth: User authenticates
OAuth->>Browser: Redirect with code
Browser->>Server: /oauth/callback (code,state)
Server->>OAuth: Exchange code for token (code_verifier)
OAuth->>Server: Return access_token (+refresh_token)
Server->>TokenStore: Store token
Server->>Client: Resume with token
end
else No header & OAuth disabled
Server->>Server: Use offline token (env or header) and exchange via SSO
Server->>Client: Proceed or return auth error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
bot
added
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
assisted_service_mcp/src/api.py (1)
7-184: Proposed type annotations introduce NameError; fix import structure for conditional OAuthResponse import is correctly flagged as unused, but the proposed diff breaks type checking:
NameError in dispatch method: The diff removes
Responsefrom imports (line 9) but uses-> Responsein the return type hint. Either:
- Keep
Responseimport and use it in the type hint, OR- Use
RequestResponseEndpointfromstarlette.types(more idiomatic for Starlette middleware)JSONResponse must stay conditional: Moving
JSONResponseto top-level imports breaks the OAuth-only structure. It should remain imported on line 46 inside theif settings.OAUTH_ENABLED:block. Remove the duplicate import from the proposed top-level imports.Correct approach:
from starlette.middleware.base import BaseHTTPMiddleware from starlette.requests import Request from starlette.types import RequestResponseEndpoint # ← idiomatic alternativeThen annotate:
async def dispatch(self, request: Request, call_next: RequestResponseEndpoint) -> Response:Alternatively, keep
Responseimport and useCallable[[Request], Awaitable[Response]]as originally proposed, but ensure the import is not removed.Add return type annotations to the handlers (e.g.,
async def oauth_well_known_handler(request: Request) -> JSONResponse:) as proposed.
🧹 Nitpick comments (6)
start-oauth-server.sh (2)
8-13: Use a safer pattern for loadingoauth-config.env
export $(grep ... | xargs)is brittle (word-splitting, quoting edge cases) and is flagged by ShellCheck (SC2046). A more robust approach for local dev is to treat the file as a shell env file and auto-export:-if [ -f oauth-config.env ]; then - export $(grep -v '^#' oauth-config.env | xargs) -else +if [ -f oauth-config.env ]; then + set -o allexport + # shellcheck disable=SC1091 + . oauth-config.env + set +o allexport +else echo "Error: oauth-config.env not found!" exit 1 fiThis avoids word-splitting issues and keeps the script aligned with ShellCheck guidance.
45-49: Consider usingexecwhen starting the MCP serverUsing
execlets the Python process take over the shell PID and improves signal/exit-code propagation:-echo "Starting MCP server..." +echo "Starting MCP server..." ... -python -m assisted_service_mcp.src.main +exec python -m assisted_service_mcp.src.mainNot mandatory for a dev script, but a small robustness win.
tests/test_oauth.py (1)
159-216: OAuth integration tests look good; minor note on patching settingsThe integration tests correctly exercise both header-based token ID resolution and Bearer-token fallback, as well as the disabled and no-context cases. One small consistency nit:
- Most tests patch
settings.OAUTH_ENABLEDvia'assisted_service_mcp.src.oauth.settings.OAUTH_ENABLED', whiletest_get_oauth_access_token_from_mcp_disabledpatches'assisted_service_mcp.src.settings.settings.OAUTH_ENABLED'. Both work because they hit the same underlyingsettingsobject, but using a single patch target everywhere would make the tests more uniform.No functional changes required; consider normalizing the patch path if you touch this file again.
assisted_service_mcp/utils/auth.py (1)
48-150: OAuth priority logic matches spec, but simplify branching & fix linter issues
- The priority order (Authorization header → OAuth when enabled → offline only when OAuth is disabled) matches the PR objectives where offline-token fallbacks are disabled if OAuth is on. Good alignment.
- You can silence the
R1705(elseafterreturn) in the OAuth block by de-indenting theelsebody:- if oauth_token_func: - oauth_token = oauth_token_func(mcp) - if oauth_token: - log.debug("Found OAuth access token (priority 2)") - return oauth_token - else: - log.debug("OAuth token function returned None - OAuth flow may be in progress") + if oauth_token_func: + oauth_token = oauth_token_func(mcp) + if oauth_token: + log.debug("Found OAuth access token (priority 2)") + return oauth_token + log.debug("OAuth token function returned None - OAuth flow may be in progress")
- To address
R0912(too many branches), consider extracting the offline-token exchange into a helper (e.g.,_get_access_token_from_offline(mcp, offline_token_func)), leavingget_access_tokenfocused on priority selection; this should drop branch count without changing behavior.- Black is also complaining about formatting in this file; running
black assisted_service_mcp/utils/auth.pyafter the refactor will resolve both style and formatting checks.assisted_service_mcp/src/settings.py (1)
109-153: OAuth settings block looks good; consider optional validation of URLs.The new OAuth settings (OAUTH_ENABLED, OAUTH_URL, OAUTH_CLIENT, SELF_URL, OAUTH_REDIRECT_URI) are well-placed and consistent with the docs and oauth.py usage.
If you want to harden configuration a bit further, you could optionally extend
validate_configto sanity-check things like:
SELF_URLandOAUTH_URLstart withhttp://orhttps://.OAUTH_CLIENTis non-empty whenOAUTH_ENABLEDis true.Not required for this PR, but it would catch misconfigurations earlier.
assisted_service_mcp/src/mcp.py (1)
47-53: OAuth token flow wiring looks solid; consider small refactors for readability.The new OAuth integration in the MCP server is well structured:
_create_oauth_token_funcavoids circular imports cleanly and usesget_oauth_access_token_from_mcp,mcp_oauth_middleware.completed_tokens, andpending_auth_sessionsin a way that matches the state/ID handling inoauth_callback_handler(state prefixmcp_auth_…).- Injecting
oauth_token_func=self._get_oauth_tokenintoget_access_tokenpreserves the documented priority: Authorization header → OAuth flow (with error if no token) → offline token only whenOAUTH_ENABLEDis false.- The “flow in progress” check prevents repeatedly opening browsers for the same client.
A couple of minor refactors you could consider:
- Lines 131–132: replace
__import__("time").time()with a normalimport timeat the top of the file andtime.time()here; it’s simpler and clearer._get_mcp_client_identifier: user-agent + client IP is a reasonable heuristic; if the MCP context later exposes a more explicit client identifier, it would be a good place to plug it in.Functionally this looks good to ship; the above are just readability/maintainability touches.
Also applies to: 60-151, 153-176
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.gitignore(1 hunks)OAUTH_SETUP.md(1 hunks)README.md(2 hunks)assisted_service_mcp/src/api.py(2 hunks)assisted_service_mcp/src/mcp.py(2 hunks)assisted_service_mcp/src/mcp_oauth_middleware.py(1 hunks)assisted_service_mcp/src/oauth.py(1 hunks)assisted_service_mcp/src/settings.py(1 hunks)assisted_service_mcp/utils/auth.py(4 hunks)doc/oauth_authentication.md(1 hunks)oauth-config.env(1 hunks)pyproject.toml(1 hunks)start-oauth-server.sh(1 hunks)tests/test_auth_priority.py(1 hunks)tests/test_oauth.py(1 hunks)tests/test_oauth_integration.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T19:01:36.933Z
Learnt from: carbonin
Repo: openshift-assisted/assisted-service-mcp PR: 111
File: pyproject.toml:9-9
Timestamp: 2025-09-25T19:01:36.933Z
Learning: The `mcp` Python package (mcp>=1.15.0) includes FastMCP functionality and provides the same import path `from mcp.server.fastmcp import FastMCP` for backward compatibility with the standalone `fastmcp` package. This allows drop-in replacement when migrating from `fastmcp>=2.8.0` to `mcp>=1.15.0` without requiring code changes.
Applied to files:
assisted_service_mcp/src/mcp.py
🧬 Code graph analysis (8)
tests/test_auth_priority.py (2)
assisted_service_mcp/utils/auth.py (1)
get_access_token(48-150)tests/test_auth.py (1)
get_context(24-25)
assisted_service_mcp/utils/auth.py (1)
assisted_service_mcp/src/settings.py (1)
get_setting(245-254)
assisted_service_mcp/src/mcp.py (2)
assisted_service_mcp/utils/auth.py (1)
get_access_token(48-150)assisted_service_mcp/src/oauth.py (3)
get_oauth_access_token_from_mcp(495-531)get_stored_access_token(161-190)get_authorization_url(68-96)
assisted_service_mcp/src/api.py (2)
assisted_service_mcp/src/oauth.py (3)
oauth_register_handler(268-299)oauth_callback_handler(302-441)oauth_token_handler(444-492)assisted_service_mcp/src/mcp_oauth_middleware.py (1)
handle_mcp_request(25-222)
assisted_service_mcp/src/mcp_oauth_middleware.py (1)
assisted_service_mcp/src/oauth.py (3)
get_stored_access_token(161-190)get_authorization_url(68-96)exchange_code_for_token(98-159)
tests/test_oauth.py (2)
assisted_service_mcp/src/oauth.py (7)
OAuthManager(20-261)oauth_callback_handler(302-441)oauth_register_handler(268-299)oauth_token_handler(444-492)get_oauth_access_token_from_mcp(495-531)get_authorization_url(68-96)exchange_code_for_token(98-159)tests/test_auth_priority.py (1)
setup_method(13-24)
tests/test_oauth_integration.py (1)
assisted_service_mcp/src/oauth.py (3)
oauth_register_handler(268-299)oauth_callback_handler(302-441)oauth_token_handler(444-492)
assisted_service_mcp/src/oauth.py (1)
tests/test_auth.py (1)
get_context(24-25)
🪛 GitHub Actions: Black
assisted_service_mcp/utils/auth.py
[error] 1-1: Black formatting check failed. 3 files would be reformatted. Run 'black --write' to fix code style issues in this file.
tests/test_oauth.py
[error] 1-1: Black formatting check failed. 3 files would be reformatted. Run 'black --write' to fix code style issues in this file.
tests/test_oauth_integration.py
[error] 1-1: Black formatting check failed. 3 files would be reformatted. Run 'black --write' to fix code style issues in this file.
🪛 GitHub Actions: Pyright
assisted_service_mcp/src/mcp_oauth_middleware.py
[error] 1-1: pyright: Cannot access attribute 'copy' for class 'Scope'. Attribute 'copy' is unknown.
[error] 1-1: pyright: Cannot access attribute 'copy' for class 'Scope'. Attribute 'copy' is unknown.
[error] 1-1: pyright: Cannot access attribute 'copy' for class 'Scope'. Attribute 'copy' is unknown.
[error] 1-1: pyright: Argument of type 'object' cannot be assigned to parameter 'value' of type 'str' in function 'setitem'.
tests/test_oauth.py
[error] 1-1: pyright: Cannot access attribute 'decode' for class 'memoryview[int]'. Attribute 'decode' is unknown (reportAttributeAccessIssue).
[error] 1-1: pyright: Cannot access attribute 'decode' for class 'memoryview[int]'. Attribute 'decode' is unknown (reportAttributeAccessIssue).
assisted_service_mcp/src/oauth.py
[error] 1-1: pyright: Argument of type 'object' cannot be assigned to parameter 'value' of type 'str' in function 'setitem'.
[error] 1-1: pyright: Argument of type 'UploadFile | str | Any' cannot be assigned to parameter 'code' of type 'str' in function 'exchange_code_for_token'.
[error] 1-1: pyright: Argument of type 'UploadFile | str | Any' cannot be assigned to parameter 'state' of type 'str' in function 'exchange_code_for_token'.
🪛 GitHub Actions: Python linter
assisted_service_mcp/utils/auth.py
[error] 98-98: R1705: Unnecessary "else" after "return". Remove the "else" and de-indent the code inside it.
[error] 48-48: R0912: Too many branches (14/12).
assisted_service_mcp/src/mcp_oauth_middleware.py
[error] 25-25: R0914: Too many local variables (18/15).
[error] 151-151: R1705: Unnecessary "else" after "return". Remove the "else" and de-indent the code inside it.
[warning] 4-8: Unused imports in module: json, urllib.parse, parse_qs, urlparse.
tests/test_oauth.py
[warning] 37-223: Trailing whitespace / formatting issues detected across the test file (C0303).
assisted_service_mcp/src/oauth.py
[error] 36-36: R0902: Too many instance attributes (8/7).
🪛 GitHub Actions: Ruff
tests/test_auth_priority.py
[error] 5-5: Remove unused import: pytest
assisted_service_mcp/src/api.py
[error] 9-9: Remove unused import: starlette.responses.Response
assisted_service_mcp/src/mcp_oauth_middleware.py
[error] 4-4: Remove unused import: json
[error] 5-5: Remove unused import: urllib.parse
[error] 8-8: Remove unused import: parse_qs
[error] 8-8: Remove unused import: urlparse
tests/test_oauth.py
[error] 3-3: Remove unused import: json
[error] 4-4: Remove unused import: secrets
[error] 6-6: Remove unused import: parse_qs
[error] 6-6: Remove unused import: urlparse
[error] 11-11: Remove unused import: fastapi.testclient.TestClient
tests/test_oauth_integration.py
[error] 14-14: Remove unused import: Route
assisted_service_mcp/src/oauth.py
[error] 5-5: Remove unused import: json
[error] 14-14: Remove unused import: fastapi.responses.RedirectResponse
[error] 354-354: Local variable 'access_token' is assigned to but never used
🪛 GitHub Actions: Type checks
tests/test_auth_priority.py
[error] 42-42: Function is missing a type annotation [no-untyped-def]
[error] 55-55: Function is missing a type annotation [no-untyped-def]
[error] 62-62: Function is missing a type annotation [no-untyped-def]
[error] 80-80: Function is missing a type annotation [no-untyped-def]
assisted_service_mcp/src/api.py
[error] 40-40: Function is missing a return type annotation [no-untyped-def]
[error] 40-40: Function is missing a type annotation for one or more arguments [no-untyped-def]
[error] 48-48: Function is missing a return type annotation [no-untyped-def]
[error] 64-64: Function is missing a return type annotation [no-untyped-def]
[error] 74-74: Function is missing a return type annotation [no-untyped-def]
[error] 92-92: Function is missing a return type annotation [no-untyped-def]
[error] 96-96: Function is missing a return type annotation [no-untyped-def]
[error] 138-138: Function is missing a return type annotation [no-untyped-def]
assisted_service_mcp/src/mcp_oauth_middleware.py
[error] 21-21: Function is missing a return type annotation [no-untyped-def]
[error] 25-25: Function is missing a return type annotation [no-untyped-def]
[error] 383-383: Function is missing a return type annotation [no-untyped-def]
[error] 398-398: Call to untyped function "MCPOAuthMiddleware" in typed context [no-untyped-call]
tests/test_oauth_integration.py
[error] 10-10: Function is missing a return type annotation [no-untyped-def]
[error] 28-28: Function is missing a type annotation [no-untyped-def]
[error] 34-34: Function is missing a type annotation [no-untyped-def]
[error] 40-40: Function is missing a type annotation [no-untyped-def]
[error] 55-55: Function is missing a return type annotation [no-untyped-def]
[error] 68-68: Function is missing a type annotation [no-untyped-def]
[error] 81-81: Function is missing a type annotation [no-untyped-def]
[error] 92-92: Function is missing a type annotation [no-untyped-def]
🪛 Gitleaks (8.29.0)
doc/oauth_authentication.md
[high] 108-108: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
README.md
[grammar] ~60-~60: Ensure spelling is correct
Context: ...-Token Header #### Note: this option is avilable when Oauth is disabled 1. **Get your ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~85-~85: Ensure spelling is correct
Context: ... ``` ##Advanced Transport Options The recomended transport is streamable-http as showen ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~85-~85: Ensure spelling is correct
Context: ...omended transport is streamable-http as showen in the examples above Other transport m...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
doc/oauth_authentication.md
16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Shellcheck (0.11.0)
start-oauth-server.sh
[warning] 9-9: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / assisted-service-mcp-saas-main-on-pull-request
🔇 Additional comments (4)
.gitignore (1)
8-10: LGTM!Adding
.coverage(coverage.py artifact) andassisted-service-mcp.log(runtime log) to .gitignore is appropriate and follows standard Python project conventions. Both entries align with the test and OAuth features introduced in this PR.pyproject.toml (1)
22-24: OAuth-related dependencies align with the new functionality
authlib,cryptography, anduvicornare appropriate additions for the OAuth2 + FastAPI stack in this project. Just ensure these minimum versions are compatible with your deployment environment and any existing lock/constraints files.tests/test_oauth.py (1)
31-79: OAuthManager tests cover success and failure paths wellThe tests for
OAuthManager.exchange_code_for_tokencorrectly:
- Seed state via
get_authorization_urlbefore exchange.- Cover the happy path and the
requests.RequestExceptionfailure path (raisingHTTPException(400)).No changes needed here.
oauth-config.env (1)
1-18: Local OAuth configuration template looks appropriateThe variables and defaults here line up with the documented setup and the
start-oauth-server.shscript (OAuth settings, MCP host/port, logging, assisted-service URLs). This is a good, self-contained local dev config.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (7)
README.md (1)
60-60: Fix typos and formatting issues in documentation.Several typos and formatting issues remain:
- Line 60: "avilable" → "available", "Oauth" → "OAuth"
- Line 83: Add space after
##for proper Markdown heading- Line 85: "recomended" → "recommended", "showen" → "shown"
- Line 138: Change duplicate "3." to "4." for correct numbering
Also applies to: 83-85, 138-138
tests/test_oauth.py (2)
96-118: Fix async mocking forexchange_code_for_token.The patch at lines 109-111 needs
new_callable=AsyncMockbecauseoauth_callback_handlerawaitsexchange_code_for_token. Without this, the test will raiseTypeError: object MagicMock can't be used in 'await' expression.Apply this diff:
with ( patch("assisted_service_mcp.src.oauth.settings.OAUTH_ENABLED", True), patch( - "assisted_service_mcp.src.oauth.oauth_manager.exchange_code_for_token" + "assisted_service_mcp.src.oauth.oauth_manager.exchange_code_for_token", + new_callable=AsyncMock, ) as mock_exchange, ):
136-163: Fix async mocking forexchange_code_for_token.Same issue at lines 150-151 - needs
new_callable=AsyncMock.Apply this diff:
with ( patch("assisted_service_mcp.src.oauth.settings.OAUTH_ENABLED", True), patch( - "assisted_service_mcp.src.oauth.oauth_manager.exchange_code_for_token" + "assisted_service_mcp.src.oauth.oauth_manager.exchange_code_for_token", + new_callable=AsyncMock, ) as mock_exchange, ):doc/oauth_authentication.md (2)
16-18: Add language tag to fenced code block.The code block should specify a language for proper syntax highlighting.
Apply this diff:
**Example:** -``` +```http Authorization: Bearer eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9...--- `108-108`: **Replace JWT-like token with placeholder to avoid false positives.** The sample token triggers security scanners. Use an obvious placeholder instead. Apply this diff: ```diff - "access_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9...", + "access_token": "<ACCESS_TOKEN_PLACEHOLDER>",assisted_service_mcp/src/mcp_oauth_middleware.py (1)
23-31: Add missing type annotations and global instance typing to unblock mypymypy is currently failing on this module due to untyped methods and the untyped global instance. You can resolve the reported issues by:
- Importing
Callable/Awaitableand tighteningcall_nexttyping (including inOAuthWaitContext).- Adding return types to the class methods and
_create_timeout_response.- Typing the global
mcp_oauth_middlewareinstance.A concrete patch could look like:
-from typing import Any, Dict, Optional +from typing import Any, Awaitable, Callable, Dict, Optional @@ @dataclass class OAuthWaitContext: @@ - request: Request - call_next: Any + request: Request + call_next: Callable[[Request], Awaitable[Response]] client_id: str @@ -class MCPOAuthMiddleware: +class MCPOAuthMiddleware: @@ - def __init__(self): + def __init__(self) -> None: @@ - async def handle_mcp_request(self, request: Request, call_next): + async def handle_mcp_request( + self, request: Request, call_next: Callable[[Request], Awaitable[Response]] + ) -> Response: @@ - async def _try_existing_token(self, request: Request, call_next, client_id: str): + async def _try_existing_token( + self, + request: Request, + call_next: Callable[[Request], Awaitable[Response]], + client_id: str, + ) -> Optional[Response]: @@ - async def _handle_pending_auth(self, request: Request, call_next, client_id: str): + async def _handle_pending_auth( + self, + request: Request, + call_next: Callable[[Request], Awaitable[Response]], + client_id: str, + ) -> Optional[Response]: @@ - async def _start_new_oauth_flow(self, request: Request, call_next, client_id: str): + async def _start_new_oauth_flow( + self, + request: Request, + call_next: Callable[[Request], Awaitable[Response]], + client_id: str, + ) -> Response: @@ - async def _wait_for_oauth_completion(self, context: OAuthWaitContext): + async def _wait_for_oauth_completion( + self, context: OAuthWaitContext + ) -> Response: @@ - async def _create_authenticated_request( - self, request: Request, call_next, token: str - ): + async def _create_authenticated_request( + self, + request: Request, + call_next: Callable[[Request], Awaitable[Response]], + token: str, + ) -> Response: @@ - def _create_timeout_response(self, auth_info=None): + def _create_timeout_response( + self, auth_info: Optional[Dict[str, Any]] = None + ) -> JSONResponse: @@ - def cleanup_expired_sessions(self, max_age_seconds: int = 600): + def cleanup_expired_sessions(self, max_age_seconds: int = 600) -> None: @@ - def _handle_oauth_completion_callback(self, state: str, token_id: str): + def _handle_oauth_completion_callback(self, state: str, token_id: str) -> None: @@ -# Global middleware instance -mcp_oauth_middleware = MCPOAuthMiddleware() +# Global middleware instance +mcp_oauth_middleware: MCPOAuthMiddleware = MCPOAuthMiddleware()This should clear the
no-untyped-defandno-untyped-callerrors in the pipeline while keeping behavior unchanged.If you want to double‑check the
call_nexttyping, verify FastAPI’s middlewarecall_nextsignature in the docs.Also applies to: 36-38, 43-64, 65-84, 85-103, 104-124, 125-155, 156-177, 178-205, 332-344, 345-382
assisted_service_mcp/src/oauth.py (1)
295-304: Type the wrapperregister_mcp_oauth_completion_callbackto satisfy mypyThe module-level wrapper is the remaining untyped function and is causing the mypy error at Line 295. It should mirror the signature of
OAuthManager.register_mcp_oauth_completion_callback.Suggested patch:
-def register_mcp_oauth_completion_callback(callback_func): +def register_mcp_oauth_completion_callback( + callback_func: Callable[[str, str], None] +) -> None: @@ - """ - oauth_manager.register_mcp_oauth_completion_callback(callback_func) + """ + oauth_manager.register_mcp_oauth_completion_callback(callback_func)This keeps the public API explicit and should clear the reported type-check failure.
You can rerun your mypy workflow after this change to confirm that the type error on this function disappears.
🧹 Nitpick comments (3)
assisted_service_mcp/src/mcp.py (1)
57-146: LGTM with minor style suggestion.The OAuth token creation logic is well-structured with appropriate caching, session management, and error handling. The lazy import pattern correctly avoids circular dependencies.
One small suggestion: Line 130 uses
__import__("time").time()which is unusual. Consider importingtimeat the module level for better readability.+import time + """Assisted Service MCP server implementation.""" import asyncioThen replace line 130:
- "timestamp": __import__("time").time(), + "timestamp": time.time(),assisted_service_mcp/src/mcp_oauth_middleware.py (2)
104-112: Use a more robust session ID thanlen(self.pending_auth_sessions)Using
session_id = f"mcp_auth_{len(self.pending_auth_sessions)}"risks collisions if multiple auth flows start concurrently before their entries are inserted, since both can compute the same length and reuse the same ID.Consider switching to a random or time-based ID:
- session_id = f"mcp_auth_{len(self.pending_auth_sessions)}" + session_id = f"mcp_auth_{secrets.token_hex(8)}"(You can import
secretshere or reuse it from the oauth module as appropriate.)
207-223: MCP request detection viacontent-type == "application/json"may be too broadThe heuristic:
is_mcp_request = ( request.url.path.startswith("/mcp") or "mcp" in request.headers.get("user-agent", "").lower() or request.headers.get("content-type") == "application/json" )treats any unauthenticated JSON request as an MCP request, which may unintentionally trigger OAuth for non-MCP clients sending JSON without auth.
If that’s a concern, consider tightening the condition (e.g., using only the path and/or user-agent, or
startswith("application/json")plus additional checks).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
OAUTH_SETUP.md(1 hunks)README.md(2 hunks)assisted_service_mcp/src/api.py(2 hunks)assisted_service_mcp/src/mcp.py(2 hunks)assisted_service_mcp/src/mcp_oauth_middleware.py(1 hunks)assisted_service_mcp/src/oauth.py(1 hunks)assisted_service_mcp/src/oauth_utils.py(1 hunks)assisted_service_mcp/src/settings.py(1 hunks)assisted_service_mcp/utils/auth.py(4 hunks)doc/oauth_authentication.md(1 hunks)oauth-config.env(1 hunks)pyproject.toml(1 hunks)start-oauth-server.sh(1 hunks)tests/test_auth.py(2 hunks)tests/test_auth_priority.py(1 hunks)tests/test_mcp.py(0 hunks)tests/test_oauth.py(1 hunks)tests/test_oauth_integration.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/test_mcp.py
✅ Files skipped from review due to trivial changes (2)
- tests/test_auth.py
- OAUTH_SETUP.md
🚧 Files skipped from review as they are similar to previous changes (5)
- assisted_service_mcp/src/settings.py
- tests/test_oauth_integration.py
- pyproject.toml
- oauth-config.env
- tests/test_auth_priority.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T19:01:36.933Z
Learnt from: carbonin
Repo: openshift-assisted/assisted-service-mcp PR: 111
File: pyproject.toml:9-9
Timestamp: 2025-09-25T19:01:36.933Z
Learning: The `mcp` Python package (mcp>=1.15.0) includes FastMCP functionality and provides the same import path `from mcp.server.fastmcp import FastMCP` for backward compatibility with the standalone `fastmcp` package. This allows drop-in replacement when migrating from `fastmcp>=2.8.0` to `mcp>=1.15.0` without requiring code changes.
Applied to files:
assisted_service_mcp/src/mcp.py
🧬 Code graph analysis (6)
assisted_service_mcp/src/api.py (3)
assisted_service_mcp/src/oauth.py (3)
oauth_register_handler(307-338)oauth_callback_handler(341-423)oauth_token_handler(426-474)assisted_service_mcp/src/mcp_oauth_middleware.py (1)
handle_mcp_request(43-63)tests/test_log_analyzer.py (1)
get(10-15)
assisted_service_mcp/src/mcp.py (3)
assisted_service_mcp/utils/auth.py (1)
get_access_token(48-146)assisted_service_mcp/src/oauth.py (3)
get_oauth_access_token_from_mcp(477-513)get_stored_access_token(161-191)get_authorization_url(72-100)assisted_service_mcp/src/oauth_utils.py (1)
open_browser_for_oauth(9-19)
assisted_service_mcp/src/oauth.py (2)
assisted_service_mcp/src/oauth_utils.py (2)
extract_oauth_callback_params(87-100)get_oauth_success_html(22-84)tests/test_auth.py (1)
get_context(24-25)
tests/test_oauth.py (3)
assisted_service_mcp/src/oauth.py (7)
OAuthManager(23-288)oauth_callback_handler(341-423)oauth_register_handler(307-338)oauth_token_handler(426-474)get_oauth_access_token_from_mcp(477-513)get_authorization_url(72-100)exchange_code_for_token(102-159)tests/test_auth_priority.py (1)
setup_method(17-28)tests/test_auth.py (1)
get_context(24-25)
assisted_service_mcp/utils/auth.py (1)
assisted_service_mcp/src/settings.py (1)
get_setting(245-254)
assisted_service_mcp/src/mcp_oauth_middleware.py (2)
assisted_service_mcp/src/oauth.py (5)
register_mcp_oauth_completion_callback(270-281)register_mcp_oauth_completion_callback(295-304)get_stored_access_token(161-191)get_authorization_url(72-100)exchange_code_for_token(102-159)assisted_service_mcp/src/oauth_utils.py (2)
get_oauth_success_html(22-84)open_browser_for_oauth(9-19)
🪛 GitHub Actions: Type checks
assisted_service_mcp/src/api.py
[error] 40-40: mypy: Function is missing a return type annotation [no-untyped-def]
[error] 40-40: mypy: Function is missing a type annotation for one or more arguments [no-untyped-def]
[error] 47-47: mypy: Function is missing a return type annotation [no-untyped-def]
[error] 63-63: mypy: Function is missing a return type annotation [no-untyped-def]
[error] 73-73: mypy: Function is missing a return type annotation [no-untyped-def]
[error] 91-91: mypy: Function is missing a return type annotation [no-untyped-def]
[error] 95-95: mypy: Function is missing a return type annotation [no-untyped-def]
[error] 137-137: mypy: Function is missing a return type annotation [no-untyped-def]
assisted_service_mcp/src/oauth.py
[error] 295-295: mypy: Function is missing a type annotation [no-untyped-def]
assisted_service_mcp/src/oauth_utils.py
[error] 87-87: mypy: Function is missing a type annotation for one or more arguments [no-untyped-def]
assisted_service_mcp/src/mcp_oauth_middleware.py
[error] 36-36: mypy: Function is missing a return type annotation [no-untyped-def]
[error] 36-36: mypy: Function is missing a type annotation for one or more arguments [no-untyped-def]
[error] 37-37: mypy: Function is missing a return type annotation [no-untyped-def]
[error] 43-43: mypy: Function is missing a return type annotation [no-untyped-def]
[error] 65-65: mypy: Function is missing a return type annotation [no-untyped-def]
[error] 85-85: mypy: Function is missing a return type annotation [no-untyped-def]
[error] 104-104: mypy: Function is missing a return type annotation [no-untyped-def]
[error] 125-125: mypy: Function is missing a return type annotation [no-untyped-def]
[error] 154-154: mypy: Call to untyped function "_create_timeout_response" in typed context [no-untyped-call]
[error] 156-156: mypy: Function is missing a return type annotation [no-untyped-def]
[error] 156-156: mypy: Function is missing a type annotation for one or more arguments [no-untyped-def]
[error] 178-178: mypy: Function is missing a type annotation [no-untyped-def]
[error] 332-332: mypy: Function is missing a return type annotation [no-untyped-def]
[error] 345-345: mypy: Function is missing a return type annotation [no-untyped-def]
[error] 382-382: mypy: Call to untyped function "MCPOAuthMiddleware" in typed context [no-untyped-call]
🪛 Gitleaks (8.29.0)
doc/oauth_authentication.md
[high] 108-108: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
README.md
[grammar] ~60-~60: Ensure spelling is correct
Context: ...-Token Header #### Note: this option is avilable when Oauth is disabled 1. **Get your ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~85-~85: Ensure spelling is correct
Context: ... ``` ##Advanced Transport Options The recomended transport is streamable-http as showen ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~85-~85: Ensure spelling is correct
Context: ...omended transport is streamable-http as showen in the examples above Other transport m...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
doc/oauth_authentication.md
11-11: Bare URL used
(MD034, no-bare-urls)
62-62: Bare URL used
(MD034, no-bare-urls)
83-83: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🪛 Shellcheck (0.11.0)
start-oauth-server.sh
[warning] 9-9: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / assisted-service-mcp-saas-main-on-pull-request
🔇 Additional comments (6)
assisted_service_mcp/src/mcp.py (1)
148-171: LGTM!The client identifier extraction has appropriate fallback logic and error handling. The combination of user-agent and client IP provides a reasonable unique identifier for MCP clients.
assisted_service_mcp/utils/auth.py (1)
48-146: LGTM!The authentication priority implementation is well-structured and clearly follows the documented order:
- Authorization header
- OAuth flow (with no fallback when enabled)
- Offline token methods
The explicit disabling of offline token fallback when OAuth is enabled ensures consistent authentication behavior. Logging and error messages are clear and helpful.
assisted_service_mcp/src/oauth_utils.py (1)
9-20: LGTM!The OAuth utility functions are well-implemented with appropriate error handling and helpful user feedback. The HTML generation provides clear instructions for both MCP and non-MCP flows.
Also applies to: 22-84
assisted_service_mcp/src/api.py (1)
29-187: LGTM with type annotation fixes!The OAuth integration is well-structured with appropriate middleware, discovery endpoints, and status polling. The conditional registration based on
OAUTH_ENABLEDensures clean separation. Once the type annotations are added, this will be production-ready.assisted_service_mcp/src/mcp_oauth_middleware.py (1)
125-155: Nice extraction of the OAuth wait loopThe shared
_wait_for_oauth_completionhelper consolidates the polling logic and keepshandle_mcp_requestreadable while enforcing a clear timeout path via_create_timeout_response. This matches the intended design from earlier review feedback.assisted_service_mcp/src/oauth.py (1)
52-101: PKCE generation and authorization URL construction look solidThe PKCE verifier/challenge generation and query parameter assembly for the authorization URL are correct and consistent with common OAuth2/OIDC practices (including
code_challenge_method="S256"and redirect URI from settings). No issues here.If desired, cross‑check against your OAuth provider’s PKCE documentation to confirm all required parameters are present.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (10)
start-oauth-server.sh (1)
7-13: Quote the command substitution to prevent word splitting.Line 9 uses unquoted command substitution which can cause word splitting and globbing issues when environment variable values contain spaces or special characters.
Apply this diff to use a safer approach:
# Load OAuth configuration if [ -f oauth-config.env ]; then - export $(grep -v '^#' oauth-config.env | xargs) + set -a + source oauth-config.env + set +a else echo "Error: oauth-config.env not found!" exit 1 fiassisted_service_mcp/src/oauth_utils.py (1)
87-100: Add type annotation for the request parameter.The
requestparameter is missing a type annotation, causing mypy to report ano-untyped-deferror.Apply this diff:
+from starlette.requests import Request + -def extract_oauth_callback_params(request) -> Dict[str, Any]: +def extract_oauth_callback_params(request: Request) -> Dict[str, Any]: """Extract and validate OAuth callback parameters.doc/oauth_authentication.md (2)
16-18: Add language identifier to fenced code block.The fenced code block is missing a language identifier, which triggers markdownlint MD040.
Apply this diff:
-``` +```http Authorization: Bearer eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9...--- `107-113`: **Replace example token with an obvious placeholder.** Gitleaks is flagging the JWT-looking `access_token` value as a potential secret. Replace it with an obvious placeholder to avoid false positives in security scans. Apply this diff: ```diff { - "access_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9...", + "access_token": "ACCESS_TOKEN_PLACEHOLDER", "token_type": "Bearer", "expires_in": 3600, "refresh_token": "refresh_token_if_available", "scope": "openid profile email" }assisted_service_mcp/src/api.py (4)
39-41: Add type annotations to the dispatch method.The
dispatchmethod is missing type annotations for thecall_nextparameter and return type, causing mypy errors.Apply this diff:
+from starlette.middleware.base import RequestResponseEndpoint +from starlette.responses import Response + class OAuthMiddleware(BaseHTTPMiddleware): - async def dispatch(self, request: Request, call_next): + async def dispatch(self, request: Request, call_next: RequestResponseEndpoint) -> Response: return await mcp_oauth_middleware.handle_mcp_request(request, call_next)
47-88: Add return type annotations to discovery handler functions.Multiple handler functions are missing return type annotations, causing mypy errors on lines 47, 63, and 73.
Apply this diff:
- async def oauth_well_known_handler(_request: Request): + async def oauth_well_known_handler(_request: Request) -> JSONResponse: """OAuth discovery endpoint.""" - async def oauth_protected_resource_handler(_request: Request): + async def oauth_protected_resource_handler(_request: Request) -> JSONResponse: """OAuth protected resource discovery.""" - async def mcp_register_handler(_request: Request): + async def mcp_register_handler(_request: Request) -> JSONResponse: """MCP registration endpoint."""
91-97: Add return type annotations to wrapper functions.The wrapper functions are missing return type annotations, causing mypy errors on lines 91 and 95.
Apply this diff:
- async def wrapped_oauth_register_handler(request: Request): + async def wrapped_oauth_register_handler(request: Request) -> JSONResponse: result = await oauth_register_handler(request) return JSONResponse(result) - async def wrapped_oauth_token_handler(request: Request): + async def wrapped_oauth_token_handler(request: Request) -> JSONResponse: result = await oauth_token_handler(request) return JSONResponse(result)
137-177: Add return type annotation to status handler function.The
oauth_status_handlerfunction is missing a return type annotation, causing a mypy error on line 137.Apply this diff:
- async def oauth_status_handler(request: Request): + async def oauth_status_handler(request: Request) -> JSONResponse: """Check OAuth authentication status for a client."""assisted_service_mcp/src/mcp_oauth_middleware.py (2)
36-36: Add missing return type annotations throughout the class.Multiple methods are missing return type annotations, causing type check failures. Add appropriate return types:
- def __init__(self): + def __init__(self) -> None: - async def handle_mcp_request(self, request: Request, call_next): + async def handle_mcp_request(self, request: Request, call_next: Any) -> Response: - async def _try_existing_token(self, request: Request, call_next, client_id: str): + async def _try_existing_token(self, request: Request, call_next: Any, client_id: str) -> Optional[Response]: - async def _handle_pending_auth(self, request: Request, call_next, client_id: str): + async def _handle_pending_auth(self, request: Request, call_next: Any, client_id: str) -> Optional[Response]: - async def _start_new_oauth_flow(self, request: Request, call_next, client_id: str): + async def _start_new_oauth_flow(self, request: Request, call_next: Any, client_id: str) -> Response: - async def _wait_for_oauth_completion(self, context: OAuthWaitContext): + async def _wait_for_oauth_completion(self, context: OAuthWaitContext) -> Response: - async def _create_authenticated_request( - self, request: Request, call_next, token: str - ): + async def _create_authenticated_request( + self, request: Request, call_next: Any, token: str + ) -> Response: - def _create_timeout_response(self, auth_info=None): + def _create_timeout_response(self, auth_info: Optional[Dict[str, Any]] = None) -> JSONResponse: - def cleanup_expired_sessions(self, max_age_seconds: int = 600): + def cleanup_expired_sessions(self, max_age_seconds: int = 600) -> None: - def _handle_oauth_completion_callback(self, state: str, token_id: str): + def _handle_oauth_completion_callback(self, state: str, token_id: str) -> None: -mcp_oauth_middleware = MCPOAuthMiddleware() +mcp_oauth_middleware: MCPOAuthMiddleware = MCPOAuthMiddleware()Also applies to: 43-43, 65-65, 85-85, 104-104, 125-125, 156-158, 178-178, 332-332, 345-345, 382-382
287-287: Fix critical session_id parsing bug in OAuth callback.The current parsing
state.split("_")[0]extracts only"mcp"from states like"mcp_auth_0_<client_id>", but the session is stored with key"mcp_auth_0"(line 250). This mismatch meanshandle_oauth_callbackwill never find valid sessions for MCP flows.Apply this fix to match the completion callback logic:
- # Find the session - session_id = state.split("_")[0] if "_" in state else state + # Find the session; for MCP flows state is "mcp_auth_<n>_<client_id>" + parts = state.split("_") + if len(parts) >= 3 and parts[0] == "mcp" and parts[1] == "auth": + session_id = "_".join(parts[:3]) # "mcp_auth_0" + else: + session_id = state session_info = self.pending_auth_sessions.get(session_id)
🧹 Nitpick comments (6)
assisted_service_mcp/src/mcp.py (1)
57-146: Consider importing time module at function scope for clarity.The OAuth token creation logic is well-structured with proper state management. However, line 130 uses
__import__("time").time()which is unconventional.For better readability, consider importing time at the function scope:
def get_oauth_token(mcp: Any) -> Optional[str]: if not settings.OAUTH_ENABLED: return None try: + import time # Import only when OAuth is enabled and function is called from assisted_service_mcp.src.oauth import ( get_oauth_access_token_from_mcp, ) # ... rest of imports ... # ... later in the code ... mcp_oauth_middleware.pending_auth_sessions[session_id] = { "client_id": client_id, "state": state, "auth_url": auth_url, - "timestamp": __import__("time").time(), + "timestamp": time.time(), }OAUTH_REFACTORING_COMPLETE.md (1)
1-185: Consider whether this internal refactoring document should be committed.This file appears to be an internal tracking/completion document for the OAuth refactoring work. Such documents typically don't belong in the repository codebase because:
- It contains transient information about the refactoring process rather than permanent documentation
- It references temporary artifacts like
.oauth_backup/directories- The "Next Steps" section includes cleanup tasks that suggest this is work-in-progress tracking
- User-facing documentation should live in
doc/or similar directoriesConsider:
- Moving OAuth setup/usage documentation to
doc/oauth_setup.mdor similar- Removing refactoring process details (the "Before/After" comparisons, quality metrics, etc.)
- Keeping this only in the PR description if needed for review context
- Or deleting it after the PR is merged since the information is captured in git history
assisted_service_mcp/src/mcp_oauth_middleware.py (1)
332-343: Consider automatic cleanup of expired sessions.The
cleanup_expired_sessionsmethod is defined but never called automatically. This could lead to memory leaks as expired sessions accumulate inpending_auth_sessions. Consider:
- Call cleanup automatically at the start of
handle_mcp_request- Use a background task to periodically clean up expired sessions
- Or clean up inline when checking for pending sessions
Quick fix - add cleanup at the start of request handling:
async def handle_mcp_request(self, request: Request, call_next): """Handle MCP requests and initiate OAuth if needed.""" + # Clean up expired sessions periodically + self.cleanup_expired_sessions() + # Check if this is an MCP request without authentication if not self._is_mcp_request_without_auth(request):assisted_service_mcp/src/oauth.py (3)
115-119: Consider adding state expiry validation.The OAuth state is validated for presence but not for age. A stale state parameter could be reused if an attacker captures it. While the state is popped after use (line 119), adding a timestamp check would provide defense-in-depth against timing attacks.
Consider adding expiry validation:
if state not in self._oauth_states: log.error("Invalid OAuth state: %s", state) raise HTTPException(status_code=400, detail="Invalid OAuth state") oauth_state = self._oauth_states.pop(state) + + # Validate state age (e.g., 10 minute max) + state_timestamp = oauth_state.get("timestamp") + if state_timestamp: + # Note: timestamp is currently a hex string, not a float + # Consider storing time.time() instead for validation + passNote: Currently
timestampis set tosecrets.token_hex(16)(line 86) rather than an actual timestamp, so this would require refactoring.
356-357: Verify None handling for extracted OAuth parameters.The
extract_oauth_callback_paramshelper returns a dict with potentiallyNonevalues (as query params can be absent), but the subsequent code at line 357 directly unpacks without explicit None guards before the check at line 374.While the check at line 374
if not code or not state:does validate, it's after unpacking. This is technically safe sinceNoneis falsy, but the flow could be clearer. Consider adding type hints to make the None-possibility explicit:# Extract parameters from callback params = extract_oauth_callback_params(request) - code, state, error = params["code"], params["state"], params["error"] + code: Optional[str] = params["code"] + state: Optional[str] = params["state"] + error: Optional[str] = params["error"]
36-43: Localhost replacement could be more robust.The string contains check
if "localhost" in self_url_str:might match unintended cases (e.g., "mylocalhost.com") and only handles one local development scenario.For more precise handling:
- if "localhost" in self_url_str: + # Parse URL to check hostname specifically + from urllib.parse import urlparse, urlunparse + parsed = urlparse(self_url_str) + if parsed.hostname == "localhost": # Replace localhost with 127.0.0.1 for better Red Hat SSO compatibility - base_url = self_url_str.replace("localhost", "127.0.0.1") + parsed = parsed._replace(netloc=parsed.netloc.replace("localhost", "127.0.0.1")) + base_url = urlunparse(parsed) self.redirect_uri = f"{base_url}/oauth/callback"However, the current implementation is probably sufficient for the Red Hat SSO use case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
OAUTH_REFACTORING_COMPLETE.md(1 hunks)OAUTH_SETUP.md(1 hunks)README.md(2 hunks)assisted_service_mcp/src/api.py(2 hunks)assisted_service_mcp/src/mcp.py(2 hunks)assisted_service_mcp/src/mcp_oauth_middleware.py(1 hunks)assisted_service_mcp/src/oauth.py(1 hunks)assisted_service_mcp/src/oauth_utils.py(1 hunks)assisted_service_mcp/src/settings.py(1 hunks)assisted_service_mcp/utils/auth.py(4 hunks)doc/oauth_authentication.md(1 hunks)oauth-config.env(1 hunks)pyproject.toml(1 hunks)start-oauth-server.sh(1 hunks)tests/test_auth.py(2 hunks)tests/test_auth_priority.py(1 hunks)tests/test_mcp.py(0 hunks)tests/test_oauth.py(1 hunks)tests/test_oauth_integration.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/test_mcp.py
🚧 Files skipped from review as they are similar to previous changes (7)
- OAUTH_SETUP.md
- pyproject.toml
- tests/test_auth_priority.py
- tests/test_oauth.py
- tests/test_oauth_integration.py
- oauth-config.env
- assisted_service_mcp/src/settings.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T19:01:36.933Z
Learnt from: carbonin
Repo: openshift-assisted/assisted-service-mcp PR: 111
File: pyproject.toml:9-9
Timestamp: 2025-09-25T19:01:36.933Z
Learning: The `mcp` Python package (mcp>=1.15.0) includes FastMCP functionality and provides the same import path `from mcp.server.fastmcp import FastMCP` for backward compatibility with the standalone `fastmcp` package. This allows drop-in replacement when migrating from `fastmcp>=2.8.0` to `mcp>=1.15.0` without requiring code changes.
Applied to files:
assisted_service_mcp/src/mcp.py
🧬 Code graph analysis (6)
tests/test_auth.py (1)
assisted_service_mcp/utils/auth.py (1)
get_access_token(48-146)
assisted_service_mcp/utils/auth.py (1)
assisted_service_mcp/src/settings.py (1)
get_setting(245-254)
assisted_service_mcp/src/oauth.py (2)
assisted_service_mcp/src/oauth_utils.py (2)
extract_oauth_callback_params(87-100)get_oauth_success_html(22-84)tests/test_auth.py (1)
get_context(24-25)
assisted_service_mcp/src/mcp.py (3)
assisted_service_mcp/utils/auth.py (1)
get_access_token(48-146)assisted_service_mcp/src/oauth.py (3)
get_oauth_access_token_from_mcp(477-513)get_stored_access_token(161-191)get_authorization_url(72-100)assisted_service_mcp/src/oauth_utils.py (1)
open_browser_for_oauth(9-19)
assisted_service_mcp/src/mcp_oauth_middleware.py (2)
assisted_service_mcp/src/oauth.py (5)
register_mcp_oauth_completion_callback(270-281)register_mcp_oauth_completion_callback(295-304)get_stored_access_token(161-191)get_authorization_url(72-100)exchange_code_for_token(102-159)assisted_service_mcp/src/oauth_utils.py (2)
get_oauth_success_html(22-84)open_browser_for_oauth(9-19)
assisted_service_mcp/src/api.py (2)
assisted_service_mcp/src/oauth.py (3)
oauth_register_handler(307-338)oauth_callback_handler(341-423)oauth_token_handler(426-474)assisted_service_mcp/src/mcp_oauth_middleware.py (1)
handle_mcp_request(43-63)
🪛 GitHub Actions: Type checks
assisted_service_mcp/src/oauth.py
[error] 295-295: Function is missing a type annotation [no-untyped-def]
assisted_service_mcp/src/oauth_utils.py
[error] 87-87: Function is missing a type annotation for one or more arguments [no-untyped-def]
assisted_service_mcp/src/mcp_oauth_middleware.py
[error] 36-36: Function is missing a return type annotation [no-untyped-def]
[warning] 36-36: Use "-> None" if function does not return a value
[warning] 37-37: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs [annotation-unchecked]
[error] 43-43: Function is missing a return type annotation [no-untyped-def]
[error] 65-65: Function is missing a return type annotation [no-untyped-def]
[error] 85-85: Function is missing a return type annotation [no-untyped-def]
[error] 104-104: Function is missing a return type annotation [no-untyped-def]
[error] 125-125: Function is missing a return type annotation [no-untyped-def]
[error] 154-154: Call to untyped function "_create_timeout_response" in typed context [no-untyped-call]
[error] 156-156: Function is missing a return type annotation [no-untyped-def]
[error] 178-178: Function is missing a type annotation [no-untyped-def]
[error] 332-332: Function is missing a return type annotation [no-untyped-def]
[error] 345-345: Function is missing a return type annotation [no-untyped-def]
[error] 382-382: Call to untyped function "MCPOAuthMiddleware" in typed context [no-untyped-call]
assisted_service_mcp/src/api.py
[error] 40-40: Function is missing a return type annotation [no-untyped-def]
[error] 40-40: Function is missing a type annotation for one or more arguments [no-untyped-def]
[error] 47-47: Function is missing a return type annotation [no-untyped-def]
[error] 63-63: Function is missing a return type annotation [no-untyped-def]
[error] 73-73: Function is missing a return type annotation [no-untyped-def]
[error] 91-91: Function is missing a return type annotation [no-untyped-def]
[error] 95-95: Function is missing a return type annotation [no-untyped-def]
[error] 137-137: Function is missing a return type annotation [no-untyped-def]
🪛 Gitleaks (8.29.0)
doc/oauth_authentication.md
[high] 108-108: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 markdownlint-cli2 (0.18.1)
README.md
16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Shellcheck (0.11.0)
start-oauth-server.sh
[warning] 9-9: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / assisted-service-mcp-saas-main-on-pull-request
🔇 Additional comments (16)
start-oauth-server.sh (2)
15-26: LGTM!The configuration display section clearly shows loaded OAuth settings and endpoint URLs for easy verification.
28-49: LGTM!The client configuration snippet and OAuth flow explanation provide clear guidance for users. The server startup command is correct.
assisted_service_mcp/src/oauth_utils.py (2)
9-20: LGTM!The function properly handles browser opening with appropriate error handling and logging.
22-84: LGTM!The HTML generation appropriately customizes instructions and auto-close behavior based on the flow type.
tests/test_auth.py (2)
71-91: LGTM!The test correctly adapts to the updated
get_access_tokensignature, now relying onOFFLINE_TOKENfrom settings rather than an injected function parameter.
94-115: LGTM!The test correctly adapts to the new authentication priority flow, using
OFFLINE_TOKENfrom settings instead of the deprecatedoffline_token_funcparameter.assisted_service_mcp/src/mcp.py (2)
46-50: LGTM!The initialization correctly wires the OAuth token function into the authentication flow, enabling priority-based token resolution.
148-171: LGTM!The client identifier logic provides robust identification with appropriate fallbacks for edge cases.
README.md (4)
7-28: LGTM!The simple token setup instructions provide a clear, quick-start path for users.
30-57: LGTM!The OAuth authentication section provides clear guidance with appropriate references to detailed setup documentation.
59-81: LGTM!The OCM-Offline-Token header option is clearly documented with the appropriate OAuth-disabled caveat.
83-139: LGTM!The advanced transport options and authentication methods are clearly documented with proper formatting and sequencing.
assisted_service_mcp/utils/auth.py (3)
48-75: LGTM!The updated signature and comprehensive docstring clearly explain the new authentication priority flow and OAuth integration.
90-113: LGTM!The OAuth priority logic correctly implements the documented authentication flow, properly disabling offline token fallback when OAuth is enabled to ensure consistent authentication.
115-146: LGTM!The offline token fallback is correctly implemented and only executed when OAuth is disabled, maintaining backward compatibility while supporting the new OAuth priority flow.
assisted_service_mcp/src/oauth.py (1)
150-153: Review comment is accurate; no action required unless type errors occur.The verification confirms your analysis: Authlib does not expose a typed OAuth2Token class—tokens are plain dicts, so
__setitem__works functionally. However, static type checkers may complain about adding arbitrary fields to a dict-like object without explicit type definitions, making your suggested refactor a valid optional improvement for type safety.The current code is correct as written. The suggested refactor is appropriate if type checker warnings appear.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/test_auth.py (1)
71-91: Tests patchget_setting("OFFLINE_TOKEN"), butget_access_tokennow usesget_offline_tokenIn both
test_get_access_token_sso_request_exceptionandtest_get_access_token_invalid_json_response, theget_settingpatch adds an"OFFLINE_TOKEN"branch (lines 80–84 and 107–111), but the currentget_access_tokenimplementation retrieves the offline token viaget_offline_token(mcp)and only callsget_setting("SSO_URL")for the SSO endpoint.As a result, these tests depend on whatever
OFFLINE_TOKENhappens to be insettings/environment and may fail or behave differently ifget_offline_tokenraises when no offline token is configured.Consider tightening the tests so they don’t rely on ambient configuration, for example:
def test_get_access_token_sso_request_exception() -> None: mod = importlib.import_module("assisted_service_mcp.utils.auth") mcp = MagicMock() mcp.get_context.return_value = MagicMock(request_context=None) with ( - patch("assisted_service_mcp.utils.auth.requests.post") as mock_post, - patch( - "assisted_service_mcp.utils.auth.get_setting", - side_effect=lambda k: ( - "https://sso.example.com" - if k == "SSO_URL" - else "offline-token" if k == "OFFLINE_TOKEN" else "" - ), - ), + patch("assisted_service_mcp.utils.auth.requests.post") as mock_post, + patch( + "assisted_service_mcp.src.settings.settings.OFFLINE_TOKEN", + "offline-token", + ), + patch( + "assisted_service_mcp.utils.auth.get_setting", + side_effect=lambda k: "https://sso.example.com" if k == "SSO_URL" else "", + ), ): mock_post.side_effect = requests.exceptions.RequestException("network error") with pytest.raises( RuntimeError, match="Failed to obtain access token from SSO" ): mod.get_access_token(mcp)Apply the same pattern to
test_get_access_token_invalid_json_responseso both tests explicitly provide an offline token through the same pathget_access_tokenactually uses.Also applies to: 99-115
assisted_service_mcp/src/mcp.py (1)
34-50: UseOAuthManager.get_access_token_by_clientand avoid cross‑thread access to middleware state.The overall wiring (lazy import, per‑client ID, clear UX errors) looks good, but two things are worth fixing:
- Token refresh is bypassed
You currently call:
token = oauth_manager.token_store.get_access_token_by_client(client_id)This only returns a stored access token (or
None) and never uses the refresh token path you implemented inOAuthManager.get_access_token_by_client, which is where_refresh_tokenis invoked. As a result, once an access token expires you will force users through a full OAuth flow again instead of transparently refreshing.Consider switching to:
token = oauth_manager.get_access_token_by_client(client_id)(and similarly in
MCPOAuthMiddleware.handle_mcp_request), so the refresh logic is consistently applied.
- Reading
pending_auth_sessionsfrom worker threads
_wrap_toolrunsself._get_access_tokenviaasyncio.to_thread, so_create_oauth_tokenexecutes on a worker thread. That closure iterates overmcp_oauth_middleware.pending_auth_sessions.items(), while the middleware mutates the same dict on the event‑loop thread.A plain dict is not safe for concurrent mutation + iteration and can raise
RuntimeError: dictionary changed size during iterationunder load. It also exposes subtle ordering/timing issues.To make this robust, you should either:
- Centralize the “auth session in progress” check inside the middleware (e.g., a helper method that encapsulates locking/single‑thread access), and call that instead of iterating the dict directly, or
- Add explicit synchronization (e.g., a
threading.Lockshared by all accesses topending_auth_sessions) and always hold it around both reads and writes.Without this, background token lookups can intermittently fail at runtime.
Also applies to: 57-138
README.md (1)
92-112: Fix SSE/STDIO examples so they are executable JSON/commands.Two small doc issues that could trip users up:
In the STDIO example, the JSON snippet shows:
"env": { "OFFLINE_TOKEN": <your token> }Strict JSON requires the value to be a string; consider using
"OFFLINE_TOKEN": "<your token>"(as you already do for<offline token>elsewhere).In the SSE example, the command is:
OFFLINE_TOKEN=<your token> TRANSPORT=sse uv run assisted_service_mcp.src.mainThis likely won’t run as intended—everywhere else you use
uv run python -m assisted_service_mcp.src.main. For consistency and to match the other examples, it should probably be:OFFLINE_TOKEN=<your token> TRANSPORT=sse uv run python -m assisted_service_mcp.src.mainThese tweaks keep the examples copy‑pasteable and consistent.
Also applies to: 118-129
♻️ Duplicate comments (3)
doc/oauth_authentication.md (1)
15-18: Fix markdown language tag and placeholder token to satisfy lint/gitleaksThe doc still has the two issues flagged previously:
- Lines 16–18: fenced block under the Authorization header example lacks a language tag.
- Lines 108–113: the sample
access_tokenvalue still looks like a real JWT and triggers gitleaks.You can address both with something like:
-**Example:** -``` -Authorization: Bearer eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9... -``` +**Example:** +```http +Authorization: Bearer ACCESS_TOKEN_HERE +``` @@ - "access_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9...", + "access_token": "ACCESS_TOKEN_HERE",Apply the same obvious-placeholder pattern to any other JWT-like example tokens in this file if present.
Also applies to: 105-113
start-oauth-server.sh (1)
7-13: Use a safer pattern to loadoauth-config.env(ShellCheck SC2046)Unquoted command substitution with
export $(...)is fragile (word splitting, globbing, silent failures in the pipeline). Prefer sourcing withset -aso complex values are handled correctly.-# Load OAuth configuration -if [ -f oauth-config.env ]; then - export $(grep -v '^#' oauth-config.env | xargs) -else - echo "Error: oauth-config.env not found!" - exit 1 -fi +# Load OAuth configuration +if [ -f oauth-config.env ]; then + set -a + # shellcheck disable=SC1091 + . oauth-config.env + set +a +else + echo "Error: oauth-config.env not found!" + exit 1 +fitests/test_oauth_integration.py (1)
9-121: Add minimal type hints to satisfy mypy in this test file.The fixtures (
oauth_enabled_app,oauth_disabled_app) and async wrappers/tests still lack type annotations, which was already flagged by CI and prior review. Even loose annotations (e.g.,-> Starlettefor fixtures,request: Request/-> JSONResponse | Anyfor wrappers,-> Noneandoauth_enabled_app: Starlettefor tests) are enough to keep mypy happy.Please update imports (
from typing import Any,from starlette.applications import Starlette,from starlette.requests import Request,from starlette.responses import JSONResponse) at module level and re-run mypy/ruff to confirm theno-untyped-deferrors are gone.
🧹 Nitpick comments (9)
assisted_service_mcp/src/oauth/utils.py (1)
11-102: OAuth utility helpers look correct and side‑effect safeBrowser-opening is guarded with logging; success HTML is static apart from trusted values; callback param extraction is straightforward. No changes needed here.
assisted_service_mcp/src/oauth/middleware.py (1)
59-83: MCP request detection bycontent-typeis brittle and a bit over‑broad
_is_mcp_request_without_authcurrently treats any request withcontent-type == "application/json"as an MCP request (in addition to/mcppath andmcpin user‑agent). Two small concerns:
- Equality check will miss common values like
application/json; charset=utf-8.- Treating all JSON requests as MCP may unintentionally trigger OAuth for non‑MCP JSON endpoints.
A more robust heuristic might be:
- is_mcp_request = ( - request.url.path.startswith("/mcp") - or "mcp" in request.headers.get("user-agent", "").lower() - or request.headers.get("content-type") == "application/json" - ) + content_type = request.headers.get("content-type", "").lower() + is_mcp_request = ( + request.url.path.startswith("/mcp") + or "mcp" in request.headers.get("user-agent", "").lower() + or content_type.startswith("application/json") + )And, if you expect other JSON endpoints, you could further narrow the condition (e.g., only
/mcppath or specific user‑agent markers).assisted_service_mcp/src/mcp.py (1)
140-163: Client identifier helper is consistent but duplicated with middleware.
_get_mcp_client_identifiermirrorsMCPOAuthMiddleware._get_client_identifier(user‑agent + client IP), which is good for consistency between middleware and server‑side token lookup. To reduce drift, consider extracting this logic into a shared helper (or reusing the middleware helper) so future changes don’t accidentally diverge.The defensive fallbacks (
mcp_client_unknown,mcp_client_fallback) are reasonable.assisted_service_mcp/src/oauth/__init__.py (1)
1-49: OAuth package exports are well-structured; minor note onsettingsre-export.The consolidated exports (manager, middleware, models, store, utils) make imports from
assisted_service_mcp.src.oauthstraightforward and support the lazy import pattern used inmcp.py.Re‑exporting
settingshere is convenient for tests, but it does slightly blur the boundary between config and the OAuth package. If you find more modules importingsettingsviaoauth, consider keeping config imports consistently fromassisted_service_mcp.src.settingsand using thesettingsre‑export only in tests.Otherwise, the module surface looks clean and intentional.
assisted_service_mcp/src/oauth/models.py (1)
9-55: OAuthToken model looks solid; consider minor typing polish only.Structure, serialization, and backward‑compatible dict-style access all look correct and consistent with TokenStore usage. If you want to tighten typing a bit later, you could return
Dict[str, Any]instead of baredictfromto_dict/from_dict, but that’s purely cosmetic.assisted_service_mcp/src/api.py (2)
51-92: Discovery and MCP registration endpoints are consistent and safe.Both
oauth_well_known_handlerandmcp_register_handlerreturn static JSON derived fromsettingsand do not depend on runtime state; the shapes look consistent with typical OAuth/OIDC discovery and MCP expectations. Hard‑coded version/description inmcp_register_handleris fine for now, but you may eventually want to derive version from a single source of truth.
140-185: OAuth status endpoint works but does a linear scan of pending sessions.Logic for
oauth_status_handler(authenticated/pending/not_authenticated) is sound and matchesTokenStore.get_token_by_clientplusmcp_oauth_middleware.pending_auth_sessions. For large numbers of concurrent sessions, the linear scan overpending_auth_sessionscould get expensive; if this ever becomes hot, consider indexing pending sessions by client_id as well.If you expect high concurrency, you may want to add basic metrics/logging around
/oauth/statuslatency to see if this becomes a bottleneck.assisted_service_mcp/src/oauth/manager.py (2)
33-52: Redirect URI construction is reasonable; localhost handling is pragmatic.Building
redirect_urifromSELF_URL, with the special‑case replacement oflocalhost→127.0.0.1, is sensible for RH SSO quirks. Just be aware that ifSELF_URLever includes a path segment, naive string concatenation could lead to double slashes; not a blocker, but something to watch.
241-304: Token refresh logic may never run given TokenStore’s expiry behavior.
get_access_token_by_id/get_access_token_by_clientrely ontoken.is_expired()to decide when to call_refresh_token, butTokenStore.get_token_by_id/get_token_by_clientalready drop expired tokens before returning them. That means, in practice, you almost never see an expired token here, so refresh is unlikely to be triggered.You may want to revisit responsibility for expiry vs refresh (e.g., let
TokenStorereturn expired tokens and haveOAuthManagerdecide whether to refresh/remove) so that the “automatic refresh” behavior actually occurs. Double‑check call sites and adjust whichever layer makes more sense to own this policy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
OAUTH_SETUP.md(1 hunks)README.md(2 hunks)assisted_service_mcp/src/api.py(2 hunks)assisted_service_mcp/src/mcp.py(2 hunks)assisted_service_mcp/src/oauth/__init__.py(1 hunks)assisted_service_mcp/src/oauth/manager.py(1 hunks)assisted_service_mcp/src/oauth/middleware.py(1 hunks)assisted_service_mcp/src/oauth/models.py(1 hunks)assisted_service_mcp/src/oauth/store.py(1 hunks)assisted_service_mcp/src/oauth/utils.py(1 hunks)assisted_service_mcp/src/settings.py(1 hunks)assisted_service_mcp/utils/auth.py(4 hunks)doc/oauth_authentication.md(1 hunks)oauth-config.env(1 hunks)pyproject.toml(2 hunks)start-oauth-server.sh(1 hunks)tests/test_auth.py(2 hunks)tests/test_auth_priority.py(1 hunks)tests/test_mcp.py(0 hunks)tests/test_oauth.py(1 hunks)tests/test_oauth_integration.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/test_mcp.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/test_auth_priority.py
- OAUTH_SETUP.md
- assisted_service_mcp/src/settings.py
- pyproject.toml
- tests/test_oauth.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T19:01:36.933Z
Learnt from: carbonin
Repo: openshift-assisted/assisted-service-mcp PR: 111
File: pyproject.toml:9-9
Timestamp: 2025-09-25T19:01:36.933Z
Learning: The `mcp` Python package (mcp>=1.15.0) includes FastMCP functionality and provides the same import path `from mcp.server.fastmcp import FastMCP` for backward compatibility with the standalone `fastmcp` package. This allows drop-in replacement when migrating from `fastmcp>=2.8.0` to `mcp>=1.15.0` without requiring code changes.
Applied to files:
assisted_service_mcp/src/mcp.py
🧬 Code graph analysis (9)
tests/test_auth.py (1)
assisted_service_mcp/utils/auth.py (1)
get_access_token(48-146)
assisted_service_mcp/src/oauth/middleware.py (4)
assisted_service_mcp/src/oauth/models.py (3)
OAuthState(100-159)get(91-96)from_json(127-148)assisted_service_mcp/src/oauth/utils.py (1)
open_browser_for_oauth(11-21)assisted_service_mcp/src/oauth/manager.py (3)
get_access_token_by_client(280-303)create_authorization_url(77-115)oauth_callback_handler(409-489)assisted_service_mcp/src/oauth/store.py (1)
get_access_token_by_client(91-101)
tests/test_oauth_integration.py (2)
assisted_service_mcp/src/oauth/manager.py (3)
oauth_register_handler(375-406)oauth_callback_handler(409-489)oauth_token_handler(492-552)assisted_service_mcp/src/oauth/models.py (1)
get(91-96)
assisted_service_mcp/src/oauth/store.py (2)
assisted_service_mcp/src/oauth/models.py (4)
OAuthToken(10-96)get(91-96)is_expired(26-32)is_expired(150-159)assisted_service_mcp/src/oauth/manager.py (3)
get_access_token_by_id(241-264)get_access_token_by_client(280-303)cleanup_expired_tokens(353-368)
assisted_service_mcp/src/api.py (3)
assisted_service_mcp/src/oauth/manager.py (3)
oauth_register_handler(375-406)oauth_callback_handler(409-489)oauth_token_handler(492-552)assisted_service_mcp/src/oauth/middleware.py (1)
handle_mcp_request(29-57)assisted_service_mcp/src/oauth/store.py (1)
get_token_by_client(64-77)
assisted_service_mcp/utils/auth.py (1)
assisted_service_mcp/src/settings.py (1)
get_setting(245-254)
assisted_service_mcp/src/mcp.py (5)
assisted_service_mcp/utils/auth.py (1)
get_access_token(48-146)assisted_service_mcp/src/oauth/manager.py (2)
get_oauth_access_token_from_mcp(555-589)get_access_token_by_client(280-303)assisted_service_mcp/src/oauth/utils.py (1)
open_browser_for_oauth(11-21)assisted_service_mcp/src/oauth/store.py (1)
get_access_token_by_client(91-101)assisted_service_mcp/src/oauth/models.py (1)
get(91-96)
assisted_service_mcp/src/oauth/manager.py (4)
assisted_service_mcp/src/oauth/models.py (7)
OAuthState(100-159)OAuthToken(10-96)to_json(111-124)from_json(127-148)is_expired(26-32)is_expired(150-159)get(91-96)assisted_service_mcp/src/oauth/store.py (9)
TokenStore(14-189)store_token(29-42)get_access_token_by_id(79-89)get_token_by_id(44-62)get_access_token_by_client(91-101)get_token_by_client(64-77)update_token(103-131)remove_token(133-142)cleanup_expired_tokens(154-173)assisted_service_mcp/src/oauth/utils.py (2)
extract_oauth_callback_params(89-102)get_oauth_success_html(24-86)tests/test_auth.py (1)
get_context(24-25)
assisted_service_mcp/src/oauth/__init__.py (5)
assisted_service_mcp/src/oauth/manager.py (5)
OAuthManager(27-368)get_oauth_access_token_from_mcp(555-589)oauth_callback_handler(409-489)oauth_register_handler(375-406)oauth_token_handler(492-552)assisted_service_mcp/src/oauth/middleware.py (1)
MCPOAuthMiddleware(18-313)assisted_service_mcp/src/oauth/models.py (2)
OAuthState(100-159)OAuthToken(10-96)assisted_service_mcp/src/oauth/store.py (1)
TokenStore(14-189)assisted_service_mcp/src/oauth/utils.py (3)
extract_oauth_callback_params(89-102)get_oauth_success_html(24-86)open_browser_for_oauth(11-21)
🪛 Gitleaks (8.29.0)
doc/oauth_authentication.md
[high] 108-108: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Shellcheck (0.11.0)
start-oauth-server.sh
[warning] 9-9: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / assisted-service-mcp-saas-main-on-pull-request
🔇 Additional comments (10)
oauth-config.env (1)
1-18: Env defaults for local OAuth dev look saneThe configuration is coherent for local development (consistent SSO/Inventory URLs, sensible defaults, and no secrets). No functional issues stand out.
README.md (1)
7-81: Quick Start and authentication priority docs align well with implementation.The three Quick Start options and the “Authentication Methods” section clearly mirror the runtime behavior in
utils/auth.get_access_token:
- Authorization header first
- OAuth flow when
OAUTH_ENABLED=true- Offline token env/header only when OAuth is disabled (and Option 3 explicitly calls this out)
This should make the auth priority and OAuth‑only behavior very clear to users.
Also applies to: 83-88, 131-146
assisted_service_mcp/utils/auth.py (2)
115-145: Offline token flow remains correct and well-logged.The offline-token branch (env var first, then
OCM-Offline-Tokenheader, SSO exchange, and robust error handling) is preserved and clearly logged. The final debug line (“Successfully generated new access token from offline token”) helps differentiate OAuth vs offline paths when debugging.No changes needed here.
48-115: No breaking changes found; all call sites are properly configured.Verification of all
get_access_tokencall sites confirms the OAuth priority logic is safe:
- Default behavior (
OAUTH_ENABLED=False): All test calls withoutoauth_token_func(test_auth.py lines 66, 91, 115) rely onOAUTH_ENABLEDdefaulting toFalse, which allows the offline token fallback flow to execute normally.- Explicit OAuth scenarios: test_auth_priority.py calls that require OAuth (lines 50, 70) explicitly pass
oauth_token_func.- Production code: assisted_service_mcp/src/mcp.py:47 properly constructs and passes
oauth_token_functoget_access_token.- Tests with
OAUTH_ENABLED=True: test_auth_priority.py lines 36, 98 use explicit patches toFalsewhen testing offline flow, or passoauth_token_funcwhen testing OAuth flow.The implementation is correct and the codebase handles all priority scenarios without breaking changes.
assisted_service_mcp/src/oauth/models.py (1)
99-159: OAuthState JSON encoding/expiry handling is straightforward and correct.State JSON round‑tripping and expiry logic look good and match how
OAuthManageruses_pending_states. No issues from a correctness or CSRF‑protection perspective.assisted_service_mcp/src/api.py (3)
31-48: OAuth middleware wiring and typing look correct.
OAuthMiddleware.dispatchhas explicit types and delegates cleanly tomcp_oauth_middleware.handle_mcp_request, which matches the expected(request, call_next) -> Responsecontract forBaseHTTPMiddleware.Please re-run your type checks (mypy) to confirm there are no remaining
no-untyped-defcomplaints around this override.
94-109: Wrapper handlers correctly adapt manager dict responses to Starlette.Wrapping
oauth_register_handlerandoauth_token_handlerto returnJSONResponseis the right way to expose FastAPI‑style dict handlers via a plain Starlette app.Ensure tests cover both success and error paths for these wrappers (they appear to, via
tests/test_oauth_integration.py).
110-139: Well‑known OAuth discovery routes are wired comprehensively.The various
/.well-known/*and/mcp/.well-known/*routes all point to the appropriate discovery handlers and HTTP methods; this should give MCP clients and tools plenty of ways to discover your OAuth configuration.assisted_service_mcp/src/oauth/manager.py (2)
77-115: PKCE + JSON-based state handling are correct and CSRF-safe.
create_authorization_urlcorrectly generates a PKCE verifier/challenge, stores a structuredOAuthStatekeyed by its JSON, and includes state+PKCE params in the auth URL. Combined with the later_pending_stateslookup, this gives you both CSRF protection and a place to stash the verifier.
555-589:get_oauth_access_token_from_mcphelper is reasonable and conservative.The helper respects
OAUTH_ENABLED, safely walks the context chain, prefers an internalX-OAuth-Token-IDindirection when present, and otherwise falls back to a Bearer token in the Authorization header. That strikes a good balance between new OAuth flows and backward compatibility.Ensure your MCP server/context implementation always populates
request_context.request; otherwise you may want to guard againstAttributeErroron missing attributes.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
assisted_service_mcp/src/oauth/store.py (1)
24-189: Concurrent access safety remains unaddressed.The previous review correctly identified that
TokenStorelacks synchronization for concurrent access from both the event loop and worker threads. This issue persists in the current code:
- Multiple async contexts (middleware, manager) call TokenStore methods
- Worker threads spawned via
asyncio.to_threadinassisted_service_mcp/src/mcp.py(line 244) also access the store- Plain dict operations without locks can lead to race conditions under concurrent access
Consider adding a
threading.RLock()to synchronize all TokenStore methods, or restructure to ensure all access happens on a single thread.start-oauth-server.sh (1)
7-13: Word splitting issue remains unaddressed.The unquoted command substitution on line 9 can cause word splitting and globbing issues, as previously noted. The current code still uses the unsafe pattern.
Consider applying the safer approach suggested in the previous review:
- export $(grep -v '^#' oauth-config.env | xargs) + set -a + source oauth-config.env + set +aThis approach sources the file directly with automatic export enabled, avoiding the word-splitting and globbing risks.
tests/test_auth_priority.py (1)
93-96: Fix patch target forget_setting.The patch targets
assisted_service_mcp.src.settings.get_setting, but you should patch where the symbol is used, not where it's defined. Sinceget_access_tokeninauth.pyimportsget_settingfrom settings, patch the imported reference in the auth module.Apply this fix:
with patch( - "assisted_service_mcp.src.settings.get_setting" + "assisted_service_mcp.utils.auth.get_setting" ) as mock_get_setting:
🧹 Nitpick comments (2)
README.md (2)
11-11: Minor: Consider wrapping URLs in markdown link syntax.The bare URLs on lines 11 and 62 trigger markdown linters. While functional, consider wrapping them in markdown link syntax for better rendering:
-1. **Get your OpenShift API token** from https://cloud.redhat.com/openshift/token +1. **Get your OpenShift API token** from <https://cloud.redhat.com/openshift/token>The angle brackets tell markdown parsers to treat it as a link.
Also applies to: 62-62
90-90: Minor: Heading level should increment by one.Line 90 jumps from
##(level 2) directly to####(level 4), skipping level 3. Consider using###instead for proper heading hierarchy:-#### STDIO Transport +### STDIO Transport
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
README.md(2 hunks)assisted_service_mcp/src/api.py(2 hunks)assisted_service_mcp/src/mcp.py(2 hunks)assisted_service_mcp/src/oauth/__init__.py(1 hunks)assisted_service_mcp/src/oauth/manager.py(1 hunks)assisted_service_mcp/src/oauth/middleware.py(1 hunks)assisted_service_mcp/src/oauth/models.py(1 hunks)assisted_service_mcp/src/oauth/store.py(1 hunks)assisted_service_mcp/src/oauth/utils.py(1 hunks)assisted_service_mcp/src/settings.py(1 hunks)assisted_service_mcp/utils/auth.py(4 hunks)doc/OAUTH_SETUP.md(1 hunks)doc/oauth_authentication.md(1 hunks)oauth-config.env(1 hunks)pyproject.toml(2 hunks)start-oauth-server.sh(1 hunks)tests/test_auth.py(3 hunks)tests/test_auth_priority.py(1 hunks)tests/test_mcp.py(0 hunks)tests/test_oauth.py(1 hunks)tests/test_oauth_integration.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/test_mcp.py
✅ Files skipped from review due to trivial changes (2)
- doc/OAUTH_SETUP.md
- doc/oauth_authentication.md
🚧 Files skipped from review as they are similar to previous changes (7)
- assisted_service_mcp/src/oauth/models.py
- pyproject.toml
- assisted_service_mcp/src/api.py
- assisted_service_mcp/src/oauth/init.py
- assisted_service_mcp/src/oauth/utils.py
- oauth-config.env
- tests/test_oauth.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T19:01:36.933Z
Learnt from: carbonin
Repo: openshift-assisted/assisted-service-mcp PR: 111
File: pyproject.toml:9-9
Timestamp: 2025-09-25T19:01:36.933Z
Learning: The `mcp` Python package (mcp>=1.15.0) includes FastMCP functionality and provides the same import path `from mcp.server.fastmcp import FastMCP` for backward compatibility with the standalone `fastmcp` package. This allows drop-in replacement when migrating from `fastmcp>=2.8.0` to `mcp>=1.15.0` without requiring code changes.
Applied to files:
assisted_service_mcp/src/mcp.py
🧬 Code graph analysis (8)
assisted_service_mcp/src/mcp.py (4)
assisted_service_mcp/utils/auth.py (1)
get_access_token(48-146)assisted_service_mcp/src/oauth/manager.py (2)
get_oauth_access_token_from_mcp(558-598)get_access_token_by_client(281-304)assisted_service_mcp/src/oauth/utils.py (1)
open_browser_for_oauth(11-21)assisted_service_mcp/src/oauth/store.py (1)
get_access_token_by_client(91-101)
assisted_service_mcp/utils/auth.py (1)
assisted_service_mcp/src/settings.py (1)
get_setting(245-254)
assisted_service_mcp/src/oauth/middleware.py (3)
assisted_service_mcp/src/oauth/models.py (3)
OAuthState(100-159)get(91-96)from_json(127-148)assisted_service_mcp/src/oauth/manager.py (3)
get_access_token_by_client(281-304)create_authorization_url(77-115)oauth_callback_handler(411-491)assisted_service_mcp/src/oauth/store.py (1)
get_access_token_by_client(91-101)
assisted_service_mcp/src/oauth/manager.py (3)
assisted_service_mcp/src/oauth/models.py (7)
OAuthState(100-159)OAuthToken(10-96)to_json(111-124)from_json(127-148)is_expired(26-32)is_expired(150-159)get(91-96)assisted_service_mcp/src/oauth/store.py (9)
TokenStore(14-189)store_token(29-42)get_access_token_by_id(79-89)get_token_by_id(44-62)get_access_token_by_client(91-101)get_token_by_client(64-77)update_token(103-131)remove_token(133-142)cleanup_expired_tokens(154-173)assisted_service_mcp/src/oauth/utils.py (2)
extract_oauth_callback_params(89-102)get_oauth_success_html(24-86)
assisted_service_mcp/src/oauth/store.py (2)
assisted_service_mcp/src/oauth/models.py (4)
OAuthToken(10-96)get(91-96)is_expired(26-32)is_expired(150-159)assisted_service_mcp/src/oauth/manager.py (3)
get_access_token_by_id(242-265)get_access_token_by_client(281-304)cleanup_expired_tokens(355-370)
tests/test_auth.py (1)
assisted_service_mcp/utils/auth.py (1)
get_access_token(48-146)
tests/test_oauth_integration.py (2)
assisted_service_mcp/src/oauth/manager.py (3)
oauth_register_handler(377-408)oauth_callback_handler(411-491)oauth_token_handler(494-555)assisted_service_mcp/src/oauth/models.py (1)
get(91-96)
tests/test_auth_priority.py (2)
assisted_service_mcp/utils/auth.py (1)
get_access_token(48-146)assisted_service_mcp/src/oauth/models.py (1)
get(91-96)
🪛 markdownlint-cli2 (0.18.1)
README.md
11-11: Bare URL used
(MD034, no-bare-urls)
62-62: Bare URL used
(MD034, no-bare-urls)
90-90: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
🪛 Shellcheck (0.11.0)
start-oauth-server.sh
[warning] 9-9: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / assisted-service-mcp-saas-main-on-pull-request
🔇 Additional comments (8)
assisted_service_mcp/src/oauth/middleware.py (1)
193-201: Past review comment addressed - auth URL capture fixed.The previous issue where
auth_urlwould always beNonehas been correctly addressed. The code now captures the auth URL before cleaning up sessions, ensuring the timeout response can include the auth URL when available.tests/test_auth.py (2)
78-91: LGTM! Test correctly updated for new authentication flow.The test now patches
get_offline_tokendirectly and callsget_access_token(mcp)without theoffline_token_funcparameter, correctly reflecting the new OAuth-first authentication priority flow.
105-115: LGTM! Test correctly updated for new authentication flow.The test properly patches
get_offline_tokenand validates the error handling path when the SSO response is invalid, consistent with the updated authentication flow.assisted_service_mcp/src/settings.py (1)
109-153: LGTM! OAuth settings well-structured and documented.The OAuth configuration fields are properly defined with Pydantic Fields, clear descriptions, and appropriate defaults. The relationship between
SELF_URLandOAUTH_REDIRECT_URIis well-documented, and the Optional typing forOAUTH_REDIRECT_URIcorrectly reflects its automatic construction behavior.assisted_service_mcp/src/mcp.py (1)
140-163: LGTM! Client identifier extraction is well-structured.The method correctly extracts user-agent and IP from the request context with appropriate fallbacks for missing data and exceptions. The implementation is consistent with the middleware's approach.
assisted_service_mcp/utils/auth.py (2)
48-113: LGTM! Authentication priority flow correctly implemented.The updated authentication flow properly prioritizes OAuth when enabled and prevents fallback to offline tokens, ensuring a consistent OAuth-only authentication path. The logic is clear:
- Authorization header (Bearer token) - highest priority
- OAuth flow (when enabled) - raises error if token unavailable, no fallback
- Offline token - only when OAuth is disabled
Error messages are actionable and the docstring accurately describes the behavior.
115-145: LGTM! Offline token flow correctly isolated.The offline token authentication path is correctly isolated to only execute when OAuth is disabled. The token exchange logic with Red Hat SSO remains unchanged and includes proper error handling for network issues and invalid responses.
assisted_service_mcp/src/oauth/manager.py (1)
206-213: Past review issues resolved - async HTTP and content-type handling look good.The previous review flagged two issues that have been addressed:
- Async HTTP (lines 206-213): Now correctly uses
httpx.AsyncClient()withawait client.post()instead of blockingrequests.post().- Content-type handling (lines 510-511): Now uses
content_type.startswith("application/json")instead of exact equality, properly handling charset parameters.Both fixes are correct and follow best practices for async FastAPI handlers.
Also applies to: 510-511
|
/retest |
1 similar comment
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
doc/OAUTH_SETUP.md (1)
28-33: Resolve contradictions around token refresh and cleanup in the docsThis doc still has conflicting statements:
- Line 31 says tokens are “automatically refreshed … (5 minutes before expiry)”.
- Line 116 says “Tokens are cached until server restart (no automatic refresh yet)”.
- Line 75–76 advertises “Automatic Cleanup: Expired sessions are cleaned up automatically”.
Please align this with the actual implementation:
- Describe whether refresh is proactive vs on-demand (e.g., refreshed when accessed and close to expiry), and remove the “no automatic refresh yet” wording if it’s no longer true.
- Only claim “Automatic Cleanup” if cleanup of expired sessions is actually implemented; otherwise, mark it as future work or clarify that tokens/sessions live until restart.
You can confirm current behavior by double-checking
OAuthManager.get_access_token_by_client, anycleanup_expired_tokensimplementation, and where those are called from the middleware or background tasks.Also applies to: 70-76, 112-116
start-oauth-server.sh (1)
7-13: Load env file without unquoted command substitutionUsing
export $(grep -v '^#' oauth-config.env | xargs)is brittle (word splitting, globbing, and broken values with spaces) and flagged by Shellcheck.Prefer a safer pattern:
if [ -f oauth-config.env ]; then - export $(grep -v '^#' oauth-config.env | xargs) + set -a + # shellcheck source=/dev/null + source oauth-config.env + set +a else echo "Error: oauth-config.env not found!" exit 1 fiThis keeps values intact and clears the SC2046 warning.
tests/test_oauth.py (1)
109-132: Make patchedexchange_code_for_tokenawaitable in handler testsBoth handler tests patch
oauth_manager.exchange_code_for_tokenwith a plainMagicMock, but the production code doesawait oauth_manager.exchange_code_for_token(...). Awaiting a non-async mock will raiseTypeError.Switch these patches to
AsyncMock:async def test_oauth_callback_handler_success(self) -> None: @@ - with ( - patch("assisted_service_mcp.src.oauth.settings.OAUTH_ENABLED", True), - patch( - "assisted_service_mcp.src.oauth.oauth_manager.exchange_code_for_token" - ) as mock_exchange, - ): - mock_exchange.return_value = {"access_token": "test_token"} + with ( + patch("assisted_service_mcp.src.oauth.settings.OAUTH_ENABLED", True), + patch( + "assisted_service_mcp.src.oauth.oauth_manager.exchange_code_for_token", + new_callable=AsyncMock, + ) as mock_exchange, + ): + mock_exchange.return_value = {"access_token": "test_token"} @@ async def test_oauth_token_handler_success(self) -> None: @@ - with ( - patch("assisted_service_mcp.src.oauth.settings.OAUTH_ENABLED", True), - patch( - "assisted_service_mcp.src.oauth.oauth_manager.exchange_code_for_token" - ) as mock_exchange, - ): + with ( + patch("assisted_service_mcp.src.oauth.settings.OAUTH_ENABLED", True), + patch( + "assisted_service_mcp.src.oauth.oauth_manager.exchange_code_for_token", + new_callable=AsyncMock, + ) as mock_exchange, + ): mock_exchange.return_value = { "access_token": "test_token", "token_type": "Bearer", "expires_in": 3600, }This keeps the behavior but makes the mocks awaitable so the async handlers can be tested without runtime errors.
Also applies to: 149-176
tests/test_auth_priority.py (4)
46-48: Add type annotations to helper function.The
mock_oauth_funchelper function lacks type annotations, causingno-untyped-deffailures in type checks.Apply this change:
- def mock_oauth_func(_mcp): + def mock_oauth_func(_mcp: object) -> str: # This should not be called return "oauth_token"
59-62: Add type annotations to helper function.The
mock_header_gethelper function lacks type annotations, causingno-untyped-deffailures in type checks.Apply this change:
- def mock_header_get(key, default=None): + def mock_header_get(key: str, default: object | None = None) -> object | None: if key == "Authorization": return None return default
66-67: Add type annotations to helper function.The
mock_oauth_funchelper function lacks type annotations, causingno-untyped-deffailures in type checks.Apply this change:
- def mock_oauth_func(_mcp): + def mock_oauth_func(_mcp: object) -> str: return "oauth_access_token"
93-96: Fix patch target to match whereget_settingis imported.The patch targets
assisted_service_mcp.src.settings.get_setting, butget_access_tokeninassisted_service_mcp/utils/auth.pyimportsget_settinginto its own module namespace. The patch should target the symbol where it's actually used.Apply this change:
with patch( - "assisted_service_mcp.src.settings.get_setting" + "assisted_service_mcp.utils.auth.get_setting" ) as mock_get_setting:
🧹 Nitpick comments (3)
assisted_service_mcp/src/oauth/utils.py (1)
11-21: Tighten logging of auth URL and align docstring with behaviorTwo small polish points here:
open_browser_for_oauth()logs the fullauth_urlat info level, which will include state and PKCE parameters. For safety and quieter logs, consider either logging at debug level or redacting query params before logging.extract_oauth_callback_params()only extracts parameters; it doesn’t validate them despite the docstring saying “Extract and validate”. Either add basic validation or update the docstring to just say “Extract OAuth callback parameters” to avoid confusion.Also applies to: 24-33, 89-97
assisted_service_mcp/utils/auth.py (1)
48-64: Auth priority logic looks correct; clarify offline-token header in docstringThe
get_access_tokenflow (Authorization header → OAuth when enabled → offline token only when OAuth disabled) matches the documented priority and the new tests.One small inconsistency: the docstring’s step 3 mentions “Offline token via environment variable” but
get_offline_token()also supports theOCM-Offline-Tokenheader as a fallback. Consider updating the docstring comment to mention both env and header so it aligns with the actual behavior.Also applies to: 90-114, 115-145
README.md (1)
11-12: Address markdownlint nits: bare URLs and heading levelA few minor doc tweaks will keep markdownlint happy and improve readability:
- Lines 11 and 62: wrap bare URLs, e.g.
from <https://cloud.redhat.com/openshift/token>
or[OpenShift API token](https://cloud.redhat.com/openshift/token).- Line 90:
#### STDIO Transportskips directly from an##heading. Change it to### STDIO Transport(or introduce an intermediate###heading) so heading levels increment by one.Also applies to: 62-64, 83-91
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
README.md(2 hunks)assisted_service_mcp/src/api.py(2 hunks)assisted_service_mcp/src/mcp.py(2 hunks)assisted_service_mcp/src/oauth/__init__.py(1 hunks)assisted_service_mcp/src/oauth/manager.py(1 hunks)assisted_service_mcp/src/oauth/middleware.py(1 hunks)assisted_service_mcp/src/oauth/models.py(1 hunks)assisted_service_mcp/src/oauth/store.py(1 hunks)assisted_service_mcp/src/oauth/utils.py(1 hunks)assisted_service_mcp/src/settings.py(1 hunks)assisted_service_mcp/utils/auth.py(4 hunks)doc/OAUTH_SETUP.md(1 hunks)doc/oauth_authentication.md(1 hunks)oauth-config.env(1 hunks)pyproject.toml(2 hunks)start-oauth-server.sh(1 hunks)tests/test_auth.py(3 hunks)tests/test_auth_priority.py(1 hunks)tests/test_mcp.py(0 hunks)tests/test_oauth.py(1 hunks)tests/test_oauth_integration.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/test_mcp.py
🚧 Files skipped from review as they are similar to previous changes (6)
- oauth-config.env
- doc/oauth_authentication.md
- pyproject.toml
- assisted_service_mcp/src/oauth/init.py
- assisted_service_mcp/src/settings.py
- assisted_service_mcp/src/oauth/models.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T19:01:36.933Z
Learnt from: carbonin
Repo: openshift-assisted/assisted-service-mcp PR: 111
File: pyproject.toml:9-9
Timestamp: 2025-09-25T19:01:36.933Z
Learning: The `mcp` Python package (mcp>=1.15.0) includes FastMCP functionality and provides the same import path `from mcp.server.fastmcp import FastMCP` for backward compatibility with the standalone `fastmcp` package. This allows drop-in replacement when migrating from `fastmcp>=2.8.0` to `mcp>=1.15.0` without requiring code changes.
Applied to files:
assisted_service_mcp/src/mcp.py
🧬 Code graph analysis (10)
tests/test_oauth.py (3)
assisted_service_mcp/src/oauth/manager.py (6)
oauth_callback_handler(411-491)oauth_register_handler(377-408)oauth_token_handler(494-555)get_oauth_access_token_from_mcp(558-598)get_authorization_url(117-154)exchange_code_for_token(156-240)tests/test_auth_priority.py (1)
setup_method(17-28)assisted_service_mcp/src/oauth/models.py (2)
get(91-96)OAuthToken(10-96)
assisted_service_mcp/src/oauth/middleware.py (4)
assisted_service_mcp/src/oauth/models.py (3)
OAuthState(100-159)get(91-96)from_json(127-148)assisted_service_mcp/src/oauth/utils.py (1)
open_browser_for_oauth(11-21)assisted_service_mcp/src/oauth/store.py (1)
get_access_token_by_client(100-110)assisted_service_mcp/src/oauth/manager.py (3)
get_access_token_by_client(281-304)create_authorization_url(77-115)oauth_callback_handler(411-491)
assisted_service_mcp/src/oauth/store.py (2)
assisted_service_mcp/src/oauth/models.py (4)
OAuthToken(10-96)get(91-96)is_expired(26-32)is_expired(150-159)assisted_service_mcp/src/oauth/manager.py (3)
get_access_token_by_id(242-265)get_access_token_by_client(281-304)cleanup_expired_tokens(355-370)
assisted_service_mcp/src/api.py (3)
assisted_service_mcp/src/oauth/manager.py (3)
oauth_register_handler(377-408)oauth_callback_handler(411-491)oauth_token_handler(494-555)assisted_service_mcp/src/oauth/middleware.py (1)
handle_mcp_request(29-57)assisted_service_mcp/src/oauth/store.py (1)
get_token_by_client(71-86)
assisted_service_mcp/src/mcp.py (5)
assisted_service_mcp/utils/auth.py (1)
get_access_token(48-146)assisted_service_mcp/src/oauth/manager.py (3)
get_oauth_access_token_from_mcp(558-598)get_access_token_by_client(281-304)create_authorization_url(77-115)assisted_service_mcp/src/oauth/utils.py (1)
open_browser_for_oauth(11-21)assisted_service_mcp/src/oauth/store.py (1)
get_access_token_by_client(100-110)assisted_service_mcp/src/oauth/models.py (3)
get(91-96)OAuthState(100-159)from_json(127-148)
tests/test_auth.py (1)
assisted_service_mcp/utils/auth.py (1)
get_access_token(48-146)
assisted_service_mcp/src/oauth/manager.py (3)
assisted_service_mcp/src/oauth/models.py (7)
OAuthState(100-159)OAuthToken(10-96)to_json(111-124)from_json(127-148)is_expired(26-32)is_expired(150-159)get(91-96)assisted_service_mcp/src/oauth/store.py (9)
TokenStore(15-215)store_token(34-48)get_access_token_by_id(88-98)get_token_by_id(50-69)get_access_token_by_client(100-110)get_token_by_client(71-86)update_token(112-141)remove_token(143-150)cleanup_expired_tokens(177-197)assisted_service_mcp/src/oauth/utils.py (2)
extract_oauth_callback_params(89-102)get_oauth_success_html(24-86)
assisted_service_mcp/utils/auth.py (1)
assisted_service_mcp/src/settings.py (1)
get_setting(245-254)
tests/test_auth_priority.py (2)
assisted_service_mcp/utils/auth.py (1)
get_access_token(48-146)tests/test_auth.py (1)
get_context(24-25)
tests/test_oauth_integration.py (2)
assisted_service_mcp/src/oauth/manager.py (3)
oauth_register_handler(377-408)oauth_callback_handler(411-491)oauth_token_handler(494-555)assisted_service_mcp/src/oauth/models.py (1)
get(91-96)
🪛 markdownlint-cli2 (0.18.1)
README.md
11-11: Bare URL used
(MD034, no-bare-urls)
62-62: Bare URL used
(MD034, no-bare-urls)
90-90: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
🪛 Shellcheck (0.11.0)
start-oauth-server.sh
[warning] 9-9: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / assisted-service-mcp-saas-main-on-pull-request
🔇 Additional comments (7)
tests/test_auth.py (1)
71-91: Offline-token error-path tests correctly adapted to new APIPatching
get_offline_tokenandget_settingdirectly is a good way to exercise the SSO failure and invalid JSON paths now thatoffline_token_funcis gone. The expectations onRuntimeErrormessages and use ofrequests.postlook solid.Also applies to: 94-115
assisted_service_mcp/src/oauth/middleware.py (1)
1-320: LGTM! OAuth middleware implementation is solid.The middleware correctly handles automatic OAuth flows for MCP clients with proper session tracking, timeout handling, and token caching. The past review comment about capturing
auth_urlbefore cleanup (lines 193-201) has been properly addressed.assisted_service_mcp/src/api.py (1)
32-189: LGTM! OAuth endpoints properly wired with correct type annotations.All handler functions have proper type annotations, the middleware is correctly integrated, and OAuth discovery endpoints follow standard patterns. Past review comments regarding type annotations have been fully addressed.
tests/test_oauth_integration.py (1)
1-120: LGTM! Integration tests properly type-annotated.All fixtures and test methods have proper type annotations. Past review comments regarding missing type hints have been fully addressed.
assisted_service_mcp/src/oauth/store.py (1)
15-215: LGTM! Thread-safe token storage properly implemented.The
TokenStoreclass correctly usesthreading.RLockto protect all dictionary operations, addressing the past review comment about concurrency. The re-entrant lock allows methods likeget_token_by_clientto safely callget_token_by_idwithout deadlock. Expiry handling and cleanup logic are sound.assisted_service_mcp/src/oauth/manager.py (2)
156-240: LGTM! Async HTTP client properly used for token exchange.The
exchange_code_for_tokenmethod correctly useshttpx.AsyncClient(lines 206-213) instead of blockingrequests.post, addressing the past review comment. Error handling and token storage flow are solid.
494-555: LGTM! Content-type detection properly handles charset parameters.The
oauth_token_handlercorrectly usescontent_type.startswith("application/json")(line 511) to handle content-type headers with charset parameters (e.g.,application/json; charset=utf-8), addressing the past review comment.
|
/test eval-test |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
assisted_service_mcp/src/oauth/manager.py (1)
504-539: Content-type handling inoauth_token_handleris now robustSwitching to
content_type.startswith("application/json")ensures JSON bodies with charset parameters (e.g.application/json; charset=utf-8) are correctly parsed viarequest.json()instead of falling back torequest.form(). This aligns with common client behavior and fixes the earlier strict-equality pitfall.tests/test_oauth.py (1)
254-277: Patchexchange_code_for_tokenasAsyncMockin handler testsBoth
test_oauth_callback_handler_successandtest_oauth_token_handler_successpatchoauth_manager.exchange_code_for_tokenwithout specifyingnew_callable=AsyncMock:patch("assisted_service_mcp.src.oauth.oauth_manager.exchange_code_for_token") as mock_exchangeBut
oauth_callback_handlerandoauth_token_handlerdoawait oauth_manager.exchange_code_for_token(...). Awaiting a plainMagicMockwill raiseTypeError: object MagicMock can't be used in 'await' expression, so these tests will fail at runtime.Update both patches to use
AsyncMockso the patched function is awaitable, for example:- with ( - patch("assisted_service_mcp.src.oauth.settings.OAUTH_ENABLED", True), - patch( - "assisted_service_mcp.src.oauth.oauth_manager.exchange_code_for_token" - ) as mock_exchange, - ): - mock_exchange.return_value = {"access_token": "test_token"} + with ( + patch("assisted_service_mcp.src.oauth.settings.OAUTH_ENABLED", True), + patch( + "assisted_service_mcp.src.oauth.oauth_manager.exchange_code_for_token", + new_callable=AsyncMock, + ) as mock_exchange, + ): + mock_exchange.return_value = {"access_token": "test_token"}Apply the same pattern in
test_oauth_token_handler_success.Also applies to: 294-321
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
assisted_service_mcp/src/mcp.py(2 hunks)assisted_service_mcp/src/oauth/manager.py(1 hunks)assisted_service_mcp/src/oauth/middleware.py(1 hunks)assisted_service_mcp/src/oauth/store.py(1 hunks)tests/test_oauth.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T19:01:36.933Z
Learnt from: carbonin
Repo: openshift-assisted/assisted-service-mcp PR: 111
File: pyproject.toml:9-9
Timestamp: 2025-09-25T19:01:36.933Z
Learning: The `mcp` Python package (mcp>=1.15.0) includes FastMCP functionality and provides the same import path `from mcp.server.fastmcp import FastMCP` for backward compatibility with the standalone `fastmcp` package. This allows drop-in replacement when migrating from `fastmcp>=2.8.0` to `mcp>=1.15.0` without requiring code changes.
Applied to files:
assisted_service_mcp/src/mcp.py
🧬 Code graph analysis (5)
tests/test_oauth.py (5)
assisted_service_mcp/src/oauth/manager.py (8)
OAuthManager(31-380)oauth_callback_handler(421-501)oauth_register_handler(387-418)oauth_token_handler(504-565)get_oauth_access_token_from_mcp(568-608)get_authorization_url(121-158)exchange_code_for_token(160-244)get_access_token_by_client(288-314)tests/test_auth_priority.py (1)
setup_method(17-28)assisted_service_mcp/src/oauth/models.py (4)
OAuthToken(10-96)is_expired(26-32)is_expired(150-159)get(91-96)assisted_service_mcp/src/oauth/store.py (3)
store_token(34-48)get_access_token_by_client(106-116)get_token_by_id(50-72)assisted_service_mcp/src/mcp.py (1)
AssistedServiceMCPServer(28-298)
assisted_service_mcp/src/oauth/manager.py (3)
assisted_service_mcp/src/oauth/models.py (7)
OAuthState(100-159)OAuthToken(10-96)to_json(111-124)from_json(127-148)is_expired(26-32)is_expired(150-159)get(91-96)assisted_service_mcp/src/oauth/store.py (10)
TokenStore(15-221)store_token(34-48)get_access_token_by_id(94-104)get_token_by_id(50-72)remove_token(149-156)get_access_token_by_client(106-116)get_token_by_client(74-92)remove_client_token(172-181)update_token(118-147)cleanup_expired_tokens(183-203)assisted_service_mcp/src/oauth/utils.py (2)
extract_oauth_callback_params(89-102)get_oauth_success_html(24-86)
assisted_service_mcp/src/oauth/store.py (2)
assisted_service_mcp/src/oauth/models.py (4)
OAuthToken(10-96)get(91-96)is_expired(26-32)is_expired(150-159)assisted_service_mcp/src/oauth/manager.py (3)
get_access_token_by_id(246-272)get_access_token_by_client(288-314)cleanup_expired_tokens(365-380)
assisted_service_mcp/src/oauth/middleware.py (3)
assisted_service_mcp/src/oauth/models.py (3)
OAuthState(100-159)get(91-96)from_json(127-148)assisted_service_mcp/src/oauth/utils.py (1)
open_browser_for_oauth(11-21)assisted_service_mcp/src/oauth/manager.py (3)
get_access_token_by_client(288-314)create_authorization_url(81-119)oauth_callback_handler(421-501)
assisted_service_mcp/src/mcp.py (4)
assisted_service_mcp/utils/auth.py (1)
get_access_token(48-146)assisted_service_mcp/src/oauth/manager.py (3)
get_oauth_access_token_from_mcp(568-608)get_access_token_by_client(288-314)create_authorization_url(81-119)assisted_service_mcp/src/oauth/utils.py (1)
open_browser_for_oauth(11-21)assisted_service_mcp/src/oauth/models.py (3)
get(91-96)OAuthState(100-159)from_json(127-148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / assisted-service-mcp-saas-main-on-pull-request
🔇 Additional comments (3)
assisted_service_mcp/src/oauth/store.py (1)
28-33: TokenStore concurrency and expiry handling look solidThe centralized
TokenStorecorrectly wraps all reads/writes in a re‑entrant lock, reusesget_token_by_idfromget_token_by_client, and cleans up expired tokens viacleanup_expired_tokenswithout exposing inconsistent state. This aligns well with the manager’sget_access_token_by_*and_refresh_tokenflows.Also applies to: 34-48, 50-72, 183-203
assisted_service_mcp/src/oauth/manager.py (1)
61-80: PKCE-based OAuth flow and token lifecycle are implemented correctlyThe manager’s PKCE generation, structured
OAuthStatehandling (including legacy state fallback), async token exchange viahttpx.AsyncClient, and refresh logic tied to the newTokenStoreall look consistent and robust.get_access_token_by_id/get_access_token_by_clientcorrectly useinclude_expired=Truefor refresh, and stale tokens are cleaned up on refresh failure.Also applies to: 81-120, 160-245, 288-315, 316-364
assisted_service_mcp/src/mcp.py (1)
58-107: Based on my verification, the review comment identifies a legitimate concern about code maintainability and future refactoring risks. The current architecture is actually safe because:
- Production usage is protected:
_wrap_toolusesasyncio.to_thread(self._get_access_token)which offloads to a worker thread, makingasyncio.run()safe when_get_oauth_tokenis called there.- Tests are synchronous: All test calls to
_get_oauth_tokenoccur in synchronous test functions, whereasyncio.run()is safe.- No problematic direct calls found: There are no production code paths that call
_get_oauth_tokendirectly from within an event loop.However, the review comment raises a valid forward-looking concern: since
_get_oauth_tokenis a private method with an implicit constraint (must not be called from event loop context), future refactors could inadvertently violate this. The review's suggestion to either document the constraint or refactor for clarity is sound practice.resolve_review_comment
| self.pending_auth_sessions: Dict[str, Dict[str, Any]] = {} | ||
|
|
||
| async def handle_mcp_request(self, request: Request, call_next: Any) -> Response: | ||
| """Handle MCP requests and initiate OAuth if needed. | ||
| Args: | ||
| request: FastAPI request |
There was a problem hiding this comment.
Protect pending_auth_sessions against cross‑thread access
pending_auth_sessions is a plain dict mutated in multiple places in this middleware and also accessed from AssistedServiceMCPServer._create_oauth_token via the shared mcp_oauth_middleware instance. The middleware code runs on the FastAPI event loop, while _create_oauth_token is executed in worker threads via asyncio.to_thread.
Because there is no lock around reads/writes, concurrent access can hit RuntimeError: dictionary changed size during iteration (e.g., while _cleanup_client_sessions iterates and deletes) or cause subtle races.
Consider aligning this with TokenStore by adding a threading lock around all accesses to pending_auth_sessions (or moving these session records into a centralized, locked structure) so both the middleware and MCP helper use a synchronized API rather than touching the dict directly.
Also applies to: 100-112, 141-147, 174-201, 301-315
🤖 Prompt for AI Agents
In assisted_service_mcp/src/oauth/middleware.py around lines 27 to 33 (and also
apply to ranges 100-112, 141-147, 174-201, 301-315), pending_auth_sessions is an
unprotected dict that is mutated from the FastAPI event loop and from worker
threads causing race conditions; protect it by adding a threading.Lock (e.g.,
self._pending_auth_sessions_lock = threading.Lock()) and wrap all reads and
writes to pending_auth_sessions in lock acquisitions (use with
self._pending_auth_sessions_lock: before iterating, adding, removing, or copying
entries), or alternatively move session management into a centralized,
thread-safe helper class that exposes locked get/set/delete/iterate methods and
update all code paths (including AssistedServiceMCPServer._create_oauth_token
and _cleanup_client_sessions) to use that locked API.
|
/retest |
Signed-off-by: Eran Cohen <[email protected]>
assisted_service_mcp/src/api.py
Outdated
|
|
||
| # OAuth discovery endpoints for better MCP client compatibility | ||
|
|
||
| async def oauth_well_known_handler(_request: Request) -> JSONResponse: |
There was a problem hiding this comment.
| async def oauth_well_known_handler(_request: Request) -> JSONResponse: | |
| async def oauth_well_known_openid_config(_request: Request) -> JSONResponse: |
assisted_service_mcp/src/api.py
Outdated
| app.add_route("/oauth/token", wrapped_oauth_token_handler, methods=["POST"]) | ||
|
|
||
| # OAuth discovery endpoints | ||
| app.add_route( |
There was a problem hiding this comment.
We have too many routes leading to the openid configuration, we should only really have one, "/.well-known/openid-configuration/mcp" and maybe also "/.well-known/openid-configuration"
The standard forces clients to go there anyway: https://modelcontextprotocol.io/specification/draft/basic/authorization#authorization-server-metadata-discovery
|
So our solution is basically to pretend the MCP client (e.g. cursor) is ocm-cli? |
|
Should we implement this section?
|
| "response_types_supported": ["code"], | ||
| "grant_types_supported": ["authorization_code"], | ||
| "code_challenge_methods_supported": ["S256"], | ||
| "scopes_supported": ["openid", "profile", "email"], |
There was a problem hiding this comment.
Should we have registration_endpoint here pointing at our /register endpoint?
There was a problem hiding this comment.
We do implement metadata discovery, so we should let the MCP client know where our registration endpoint is
This also means we don't have to keep it at /register, we can have it anywhere
There was a problem hiding this comment.
- Implement OAuth2 authentication flow - Update API and MCP server to support OAuth authentication - Add OAuth tests and auth priority tests - Include OAuth documentation and setup guides - Add startup script for OAuth server
- Add threading.RLock() to TokenStore for concurrent access protection - Protect all dict operations (store, get, update, remove, cleanup) with lock - Add _remove_token_unsafe() internal method for re-entrant calls - Prevents race conditions between FastAPI handlers and MCP tool threads - RLock allows nested locking (e.g., get_token_by_client -> get_token_by_id)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
start-oauth-server.sh (1)
7-13: Safer env loading: avoid unquoted command substitution
export $(grep -v '^#' oauth-config.env | xargs)is still vulnerable to word splitting/globbing and remains flagged by ShellCheck. Prefer sourcing the file with automatic export.-# Load OAuth configuration -if [ -f oauth-config.env ]; then - export $(grep -v '^#' oauth-config.env | xargs) + # Load OAuth configuration + if [ -f oauth-config.env ]; then + set -a + # shellcheck source=/dev/null + source oauth-config.env + set +aassisted_service_mcp/src/oauth/middleware.py (1)
24-28:pending_auth_sessionsis still not thread‑safe; protect it with a lock or central store
pending_auth_sessionsremains a plain dict that is read and mutated here and also fromAssistedServiceMCPServer._create_oauth_tokenvia the sharedmcp_oauth_middlewareinstance. With FastAPI handlers on the event loop and MCP tools using worker threads, this is vulnerable to races andRuntimeError: dictionary changed size during iteration.A minimal direction is to add a re‑entrant lock and wrap all accesses:
-import asyncio -from typing import Any, Dict, Optional +import asyncio +import threading +from typing import Any, Dict, Optional @@ def __init__(self) -> None: """Initialize middleware.""" - # Track pending authentication sessions - self.pending_auth_sessions: Dict[str, Dict[str, Any]] = {} + # Track pending authentication sessions + self.pending_auth_sessions: Dict[str, Dict[str, Any]] = {} + self._sessions_lock = threading.RLock() @@ def _has_pending_auth(self, client_id: str) -> bool: @@ - for session_info in self.pending_auth_sessions.values(): - if session_info.get("client_id") == client_id: - return True - return False + with self._sessions_lock: + for session_info in self.pending_auth_sessions.values(): + if session_info.get("client_id") == client_id: + return True + return False @@ - # Store session info - self.pending_auth_sessions[session_id] = { - "client_id": client_id, - "state": state_json, - "auth_url": auth_url, - "timestamp": asyncio.get_event_loop().time(), - } + # Store session info + with self._sessions_lock: + self.pending_auth_sessions[session_id] = { + "client_id": client_id, + "state": state_json, + "auth_url": auth_url, + "timestamp": asyncio.get_event_loop().time(), + } @@ - auth_url = None - for session_info in self.pending_auth_sessions.values(): - if session_info.get("client_id") == client_id: - auth_url = session_info.get("auth_url") - break + auth_url = None + with self._sessions_lock: + for session_info in self.pending_auth_sessions.values(): + if session_info.get("client_id") == client_id: + auth_url = session_info.get("auth_url") + break @@ - sessions_to_remove = [ - session_id - for session_id, session_info in self.pending_auth_sessions.items() - if session_info.get("client_id") == client_id - ] - for session_id in sessions_to_remove: - del self.pending_auth_sessions[session_id] + with self._sessions_lock: + sessions_to_remove = [ + session_id + for session_id, session_info in self.pending_auth_sessions.items() + if session_info.get("client_id") == client_id + ] + for session_id in sessions_to_remove: + del self.pending_auth_sessions[session_id] @@ - expired_sessions = [ - session_id - for session_id, info in self.pending_auth_sessions.items() - if current_time - info["timestamp"] > max_age_seconds - ] - - for session_id in expired_sessions: - del self.pending_auth_sessions[session_id] - log.info("Cleaned up expired OAuth session: %s", session_id) + with self._sessions_lock: + expired_sessions = [ + session_id + for session_id, info in self.pending_auth_sessions.items() + if current_time - info["timestamp"] > max_age_seconds + ] + + for session_id in expired_sessions: + del self.pending_auth_sessions[session_id] + log.info("Cleaned up expired OAuth session: %s", session_id)You’ll also want to route external access (e.g., from
AssistedServiceMCPServer._create_oauth_token) through helper methods onMCPOAuthMiddlewarerather than manipulatingpending_auth_sessionsdirectly so those callers also benefit from the lock.Also applies to: 100-112, 141-147, 174-201, 205-218, 301-315
tests/test_oauth.py (1)
254-277: Patchexchange_code_for_tokenwithAsyncMock, notMagicMock, in async handler testsBoth handler tests still patch
oauth_manager.exchange_code_for_tokenas a plainMagicMock, but the handlersawaitthis function, which will result inTypeError: object MagicMock can't be used in 'await' expression.Update the patches to use
AsyncMockso the await works correctly:@@ async def test_oauth_callback_handler_success(self) -> None: - with ( - patch("assisted_service_mcp.src.oauth.settings.OAUTH_ENABLED", True), - patch( - "assisted_service_mcp.src.oauth.oauth_manager.exchange_code_for_token" - ) as mock_exchange, - ): - mock_exchange.return_value = {"access_token": "test_token"} + with ( + patch("assisted_service_mcp.src.oauth.settings.OAUTH_ENABLED", True), + patch( + "assisted_service_mcp.src.oauth.oauth_manager.exchange_code_for_token", + new_callable=AsyncMock, + ) as mock_exchange, + ): + mock_exchange.return_value = {"access_token": "test_token"} @@ async def test_oauth_token_handler_success(self) -> None: - with ( - patch("assisted_service_mcp.src.oauth.settings.OAUTH_ENABLED", True), - patch( - "assisted_service_mcp.src.oauth.oauth_manager.exchange_code_for_token" - ) as mock_exchange, - ): - mock_exchange.return_value = { + with ( + patch("assisted_service_mcp.src.oauth.settings.OAUTH_ENABLED", True), + patch( + "assisted_service_mcp.src.oauth.oauth_manager.exchange_code_for_token", + new_callable=AsyncMock, + ) as mock_exchange, + ): + mock_exchange.return_value = { "access_token": "test_token", "token_type": "Bearer", "expires_in": 3600, }Also applies to: 294-321
tests/test_auth_priority.py (1)
76-100: Patchget_settingin the module under test, notsrc.settings.
get_access_tokencalls theget_settingsymbol imported intoassisted_service_mcp.utils.auth, so patchingassisted_service_mcp.src.settings.get_settingwon’t intercept those calls. The offline-token test can end up using the real configuration instead of the mocked SSO URL.Patch the correct target:
- with patch( - "assisted_service_mcp.src.settings.get_setting" - ) as mock_get_setting: + with patch( + "assisted_service_mcp.utils.auth.get_setting" + ) as mock_get_setting:This keeps the test fully isolated from environment/config.
🧹 Nitpick comments (3)
README.md (1)
131-138: Clarify offline token methods are only active when OAuth is disabledTo mirror the actual behavior in
get_access_token(offline token only used whenOAUTH_ENABLEDis false), consider making that explicit in the summary list:-3. **Environment Variable** - `OFFLINE_TOKEN` environment variable -4. **OCM-Offline-Token Header** - `OCM-Offline-Token: <token>` in request headers +3. **Environment Variable** - `OFFLINE_TOKEN` environment variable (only when OAuth is disabled) +4. **OCM-Offline-Token Header** - `OCM-Offline-Token: <token>` in request headers (only when OAuth is disabled)assisted_service_mcp/src/mcp.py (1)
58-159: Be explicit about thread-safety ofpending_auth_sessions.The
_create_oauth_tokenclosure is run from a worker thread (viaasyncio.to_thread(self._get_access_token)), and it iteratesmcp_oauth_middleware.pending_auth_sessions.items(). Ifpending_auth_sessionsis a plain dict also mutated from FastAPI handlers, this can race and raiseRuntimeError: dictionary changed size during iteration.If it isn’t already, consider guarding that mapping with a lock or exposing a thread-safe helper on the middleware (e.g., a method that checks “in-progress for client_id” under its own lock) so this code never iterates a concurrently-mutated dict. Otherwise, no functional issues with the overall OAuth flow here.
assisted_service_mcp/src/oauth/manager.py (1)
81-119: Consider keeping the PKCE verifier server-side rather than embedding it in state.
create_authorization_urlserializesOAuthState(includingcode_verifier) into thestateparameter, which then round-trips through the browser. Functionally this works, but it slightly weakens PKCE’s threat model because anyone who can read the front‑channel redirect sees both the code and the verifier.A more conservative pattern is to generate a compact random state string, store the full
OAuthState(with verifier) in_pending_stateskeyed by that state, and send only the opaque state token to the client. That keeps the verifier strictly server-side while preserving your existing lookup and expiry logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
README.md(2 hunks)assisted_service_mcp/src/api.py(2 hunks)assisted_service_mcp/src/mcp.py(2 hunks)assisted_service_mcp/src/oauth/__init__.py(1 hunks)assisted_service_mcp/src/oauth/manager.py(1 hunks)assisted_service_mcp/src/oauth/middleware.py(1 hunks)assisted_service_mcp/src/oauth/models.py(1 hunks)assisted_service_mcp/src/oauth/store.py(1 hunks)assisted_service_mcp/src/oauth/utils.py(1 hunks)assisted_service_mcp/src/settings.py(1 hunks)assisted_service_mcp/utils/auth.py(4 hunks)doc/OAUTH_SETUP.md(1 hunks)doc/oauth_authentication.md(1 hunks)oauth-config.env(1 hunks)pyproject.toml(2 hunks)start-oauth-server.sh(1 hunks)tests/src/test_mcp.py(0 hunks)tests/test_auth_priority.py(1 hunks)tests/test_oauth.py(1 hunks)tests/test_oauth_integration.py(1 hunks)tests/utils/test_auth.py(2 hunks)
💤 Files with no reviewable changes (1)
- tests/src/test_mcp.py
🚧 Files skipped from review as they are similar to previous changes (9)
- doc/OAUTH_SETUP.md
- oauth-config.env
- tests/utils/test_auth.py
- assisted_service_mcp/src/settings.py
- assisted_service_mcp/src/oauth/utils.py
- tests/test_oauth_integration.py
- pyproject.toml
- assisted_service_mcp/src/api.py
- doc/oauth_authentication.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T19:01:36.933Z
Learnt from: carbonin
Repo: openshift-assisted/assisted-service-mcp PR: 111
File: pyproject.toml:9-9
Timestamp: 2025-09-25T19:01:36.933Z
Learning: The `mcp` Python package (mcp>=1.15.0) includes FastMCP functionality and provides the same import path `from mcp.server.fastmcp import FastMCP` for backward compatibility with the standalone `fastmcp` package. This allows drop-in replacement when migrating from `fastmcp>=2.8.0` to `mcp>=1.15.0` without requiring code changes.
Applied to files:
assisted_service_mcp/src/mcp.py
🧬 Code graph analysis (8)
assisted_service_mcp/src/mcp.py (4)
assisted_service_mcp/utils/auth.py (1)
get_access_token(48-146)assisted_service_mcp/src/oauth/manager.py (3)
get_oauth_access_token_from_mcp(568-608)get_access_token_by_client(288-314)create_authorization_url(81-119)assisted_service_mcp/src/oauth/utils.py (1)
open_browser_for_oauth(11-21)assisted_service_mcp/src/oauth/models.py (3)
get(91-96)OAuthState(100-159)from_json(127-148)
assisted_service_mcp/src/oauth/manager.py (3)
assisted_service_mcp/src/oauth/models.py (7)
OAuthState(100-159)OAuthToken(10-96)to_json(111-124)from_json(127-148)is_expired(26-32)is_expired(150-159)get(91-96)assisted_service_mcp/src/oauth/store.py (10)
TokenStore(15-221)store_token(34-48)get_access_token_by_id(94-104)get_token_by_id(50-72)remove_token(149-156)get_access_token_by_client(106-116)get_token_by_client(74-92)remove_client_token(172-181)update_token(118-147)cleanup_expired_tokens(183-203)assisted_service_mcp/src/oauth/utils.py (2)
extract_oauth_callback_params(89-102)get_oauth_success_html(24-86)
assisted_service_mcp/src/oauth/__init__.py (5)
assisted_service_mcp/src/oauth/manager.py (5)
OAuthManager(31-380)get_oauth_access_token_from_mcp(568-608)oauth_callback_handler(421-501)oauth_register_handler(387-418)oauth_token_handler(504-565)assisted_service_mcp/src/oauth/middleware.py (1)
MCPOAuthMiddleware(18-316)assisted_service_mcp/src/oauth/models.py (2)
OAuthState(100-159)OAuthToken(10-96)assisted_service_mcp/src/oauth/store.py (1)
TokenStore(15-221)assisted_service_mcp/src/oauth/utils.py (3)
extract_oauth_callback_params(89-102)get_oauth_success_html(24-86)open_browser_for_oauth(11-21)
tests/test_oauth.py (4)
assisted_service_mcp/src/oauth/manager.py (7)
OAuthManager(31-380)oauth_callback_handler(421-501)oauth_register_handler(387-418)oauth_token_handler(504-565)get_oauth_access_token_from_mcp(568-608)exchange_code_for_token(160-244)get_access_token_by_client(288-314)assisted_service_mcp/src/oauth/models.py (4)
OAuthToken(10-96)is_expired(26-32)is_expired(150-159)get(91-96)assisted_service_mcp/src/oauth/store.py (3)
store_token(34-48)get_access_token_by_client(106-116)get_token_by_id(50-72)assisted_service_mcp/src/mcp.py (1)
AssistedServiceMCPServer(28-298)
assisted_service_mcp/utils/auth.py (1)
assisted_service_mcp/src/settings.py (1)
get_setting(245-254)
tests/test_auth_priority.py (1)
assisted_service_mcp/utils/auth.py (1)
get_access_token(48-146)
assisted_service_mcp/src/oauth/store.py (2)
assisted_service_mcp/src/oauth/models.py (4)
OAuthToken(10-96)get(91-96)is_expired(26-32)is_expired(150-159)assisted_service_mcp/src/oauth/manager.py (3)
get_access_token_by_id(246-272)get_access_token_by_client(288-314)cleanup_expired_tokens(365-380)
assisted_service_mcp/src/oauth/middleware.py (3)
assisted_service_mcp/src/oauth/models.py (3)
OAuthState(100-159)get(91-96)from_json(127-148)assisted_service_mcp/src/oauth/utils.py (1)
open_browser_for_oauth(11-21)assisted_service_mcp/src/oauth/manager.py (3)
get_access_token_by_client(288-314)create_authorization_url(81-119)oauth_callback_handler(421-501)
🪛 Shellcheck (0.11.0)
start-oauth-server.sh
[warning] 9-9: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (14)
assisted_service_mcp/utils/auth.py (1)
48-145: Access‑token priority and OAuth gating look consistent with designThe header → OAuth (when
OAUTH_ENABLED) → offline‑token (only when OAuth is disabled) flow is implemented as documented, and failing hard when OAuth is enabled but no token is available matches the “no offline fallback under OAuth” contract. No changes needed here.assisted_service_mcp/src/oauth/__init__.py (1)
6-49: OAuth package surface is clean and consistentRe‑exports (manager, middleware, models, store, utils, settings) line up with usage in
mcp.pyand tests;__all__accurately captures the intended public API.assisted_service_mcp/src/oauth/models.py (1)
9-158: OAuthToken/OAuthState modeling looks solid.Field choices, expiry checks, and (de)serialization all align with the way manager.py and store.py consume these types; the dict-like API for OAuthToken should keep legacy callers happy without complicating the model. I don’t see any correctness issues here.
assisted_service_mcp/src/mcp.py (3)
35-52: FastMCP/OAuth wiring in__init__looks correct.Injecting
_get_oauth_tokenviaget_access_token(self.mcp, oauth_token_func=...)correctly respects the documented auth priority and keeps the MCP server construction clean.
161-184: Client identifier helper is reasonable.Deriving a per-client key from user-agent and client IP (with safe fallbacks) is a pragmatic way to partition tokens without overcomplicating state; the defensive try/except ensures auth doesn’t explode on odd contexts.
250-280: Tool wrapper correctly isolates auth from tool signatures.Using
asyncio.to_thread(self._get_access_token)to fetch the token off the event loop and then injecting a zero-arglambda: tokenkeeps tool functions simple and avoids blocking the loop on network/SSO calls. The signature rewrite to hide the auth parameter is also a nice touch for MCP clients.assisted_service_mcp/src/oauth/store.py (1)
15-221: Thread-safe token store design looks correct.Locking via
RLock,include_expiredsemantics, and the shared_remove_token_unsafehelper keep_tokensand_client_tokensconsistent and safe under concurrent access from async handlers and worker threads. Behavior aligns with how OAuthManager refreshes and cleans up tokens.assisted_service_mcp/src/oauth/manager.py (7)
37-59: Initialization and redirect URI logic are sensible.Using SELF_URL plus an override-able OAUTH_REDIRECT_URI, and normalizing localhost → 127.0.0.1 for RH SSO quirks, makes the manager usable both locally and in deployed environments. Centralizing the TokenStore here is also a clean way to replace the previous scattered storage.
160-245: Token exchange flow is robust and async-friendly.The JSON/legacy state handling, expiry checks via
OAuthState.is_expired(), and asynchttpxPOST to the token endpoint are all wired correctly. Storing tokens viaTokenStorewith a small expiry buffer is a reasonable trade-off to avoid returning near-expiry tokens to callers.
246-315: Access-token retrieval and refresh integrate cleanly with TokenStore.Both
get_access_token_by_idandget_access_token_by_clientcorrectly useinclude_expired=Trueto decide whether to refresh, update the store on success, and clean up tokens if refresh fails. That matches how callers (MCP and handlers) expect “best effort refresh, else drop” semantics.
387-419: OAuth registration handler matches the MCP/OAuth integration story.Exposing a registration endpoint that returns a ready-to-use authorization URL plus token endpoint, redirect URI, and scopes is consistent with the MCP OAuth narrative and keeps clients simple. Internal
client_idfor tracking vs. the configuredOAUTH_CLIENTin the response is a reasonable separation.
421-501: Callback handler error handling and success UX are well covered.You validate error/code/state, delegate to
exchange_code_for_token, and return clear HTML feedback (including MCP-specific success hints) which should make first-time OAuth flows much smoother. The optional MCP-flow detection viaOAuthState.from_jsondegrades gracefully for legacy states.
504-565: Token handler JSON/form parsing and response shaping look good.Using
content_type.startswith("application/json")correctly handles charset-qualified JSON, and the dict-vs-OAuthTokenbranch keeps tests/mocks working while returning well-structured OAuth responses (access_token,token_type,expires_in,refresh_token,scope).
568-608: MCP helper correctly reuses stored tokens without refresh.
get_oauth_access_token_from_mcpcleanly extracts either a token-id header (validated via TokenStore and expiry) or a Bearer token from Authorization, which is exactly what callers like_create_oauth_tokenneed for cheap cached-token reuse in the sync MCP context.
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
2 participants
Overview
This PR introduces comprehensive OAuth 2.1 authentication support for the Assisted Service MCP server, enabling seamless integration with MCP clients like Cursor without requiring manual token management.
Automatic OAuth Flow
Zero-configuration authentication: When Cursor connects, OAuth flow starts automatically
Browser-based authentication: Opens Red Hat SSO in browser for secure authentication
Token caching: Automatically caches and refreshes access tokens
Seamless reconnection: Subsequent connections use cached tokens transparently
Authentication Priority
The server now follows this authentication hierarchy:
Authorization Header: Bearer (highest priority)
OAuth Flow: Automatic OAuth when enabled and no token found
Offline Token (Environment): OFFLINE_TOKEN environment variable (only when OAuth is disabled)
Offline Token (Header): OCM-Offline-Token header (deprecated but supported for backward compatibility)
Important: When OAuth is enabled, offline token fallback (priorities 3 & 4) is disabled to ensure consistent OAuth-only authentication.
User Experience
User configures Cursor with the OAuth endpoint
When Cursor connects, browser opens automatically for Red Hat SSO
User authenticates once in browser
All subsequent connections work seamlessly with cached tokens
Tokens refresh automatically in the background
Technical Implementation
New Components
OAuthManager: Core OAuth2.1 flow implementation with PKCE
MCPOAuthMiddleware: Automatic OAuth initiation for MCP clients
OAuth Endpoints: Registration, callback, token exchange, and discovery
Summary by CodeRabbit
New Features
Documentation
Tests
Chores