oauth for tools #537
oauth for tools #537
Conversation
…nt into feat/mcp_agent_oauth
WalkthroughAdds end-to-end OAuth support: new config models, token storage/management, HTTP auth integration, server-side authorization/introspection and callback handling, app/context wiring, tools for workflow pre-authorization, extensive examples and docs, and tests for audience validation and OAuth utilities. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client (httpx)
participant OAuthAuth as OAuthHttpxAuth
participant TokenMgr as TokenManager
participant Store as TokenStore
participant Server as Downstream MCP Server
Client->>OAuthAuth: Request (no Authorization)
OAuthAuth->>TokenMgr: ensure_access_token(context, server_name, config, scopes?)
alt Preconfigured token exists
TokenMgr->>Store: get(preconfigured user key)
Store-->>TokenMgr: TokenRecord
else Token for current user
TokenMgr->>Store: get(user key)
Store-->>TokenMgr: TokenRecord or None
alt Expired and refreshable
TokenMgr->>TokenMgr: _refresh_token(...)
TokenMgr-->>TokenMgr: New TokenRecord
else No valid token
TokenMgr->>TokenMgr: AuthorizationFlowCoordinator.authorize(...)
TokenMgr-->>TokenMgr: TokenRecord
end
end
TokenMgr-->>OAuthAuth: Access token
OAuthAuth->>Client: Add Authorization: Bearer ...
Client->>Server: Request with token
Server-->>Client: 401 Unauthorized
OAuthAuth->>TokenMgr: invalidate(user, resource, scopes)
TokenMgr-->>OAuthAuth: ok
OAuthAuth->>TokenMgr: ensure_access_token(...) (refresh/reauthorize)
TokenMgr-->>OAuthAuth: New token
OAuthAuth->>Client: Retry with new Authorization
Client->>Server: Retried request
Server-->>Client: 200 OK
sequenceDiagram
autonumber
participant Agent as MCP Agent Server
participant Upstream as MCP Client (user)
participant Flow as AuthorizationFlowCoordinator
participant AS as Authorization Server
participant CB as Internal Callback Endpoint
participant Store as TokenStore
Agent->>Flow: authorize(user, server_name, oauth_config, scopes)
Flow->>Upstream: auth/request (auth URL or prompt)
alt User completes via internal redirect
Upstream->>AS: Authorize (code, state)
AS->>CB: Redirect /internal/oauth/callback/{flow_id}?code=...&state=...
CB->>Flow: deliver(flow_id, params)
else Elicit returns code directly
Upstream-->>Flow: code, state
end
Flow->>AS: Token exchange (code, verifier, redirect_uri)
AS-->>Flow: access_token [+ refresh_token]
Flow-->>Agent: TokenRecord
Agent->>Store: set(TokenStoreKey, TokenRecord)
Store-->>Agent: ok
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![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
🧹 Nitpick comments (12)
examples/oauth/README.md (2)
12-13: Clarify the synthetic user mechanism.The statement "we create a synthetic user and use that" is vague. Consider documenting what this synthetic user identifier looks like (e.g., a hash of the workflow name, a fixed constant) and any implications for token isolation or security.
17-19: Document the incomplete end-to-end flow.Since the dynamic_auth flow is noted as incomplete, consider adding a TODO or issue reference, and briefly describe what remains to be implemented (e.g., client handling of elicitation responses, token persistence after auth).
examples/oauth/dynamic_auth/worker.py (1)
15-15: Consider explicit relative import.The import
from main import appassumesmain.pyis in the same directory. For clarity and to avoid potential import issues, consider using an explicit relative import:from .main import app.examples/oauth/preconfigured/mcp_agent.secrets.yaml.example (1)
38-41: Consider adding token expiry guidance.The optional
access_tokenfield doesn't mention token expiration. Consider adding a comment about token refresh requirements or validity periods to help users understand when manual token updates are needed.examples/oauth/preconfigured/README.md (2)
84-93: Specify language for fenced code block.As per static analysis hint, add a language identifier to the fenced code block for better rendering and syntax highlighting.
Apply this diff:
-``` +```text examples/oauth/ ├── README.md # This file
330-330: Sanitize the example token in curl command.The static analysis tool flagged a potential security issue with the authorization header example. While this is a placeholder (
YOUR_TOKEN), consider making it more obviously fake to prevent accidental copy-paste with real tokens.Apply this diff:
-curl -H "Authorization: Bearer YOUR_TOKEN" https://api.github.com/user +curl -H "Authorization: Bearer ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" https://api.github.com/userexamples/oauth/workflow_pre_auth/client.py (2)
30-44: Improve token validation guidance.The token validation logic checks prefixes but doesn't verify token format comprehensively. Consider adding:
- Length validation (GitHub tokens have specific lengths)
- Character set validation (base62 encoding)
- Clearer error messages distinguishing between missing tokens and malformed tokens
Consider enhancing the validation:
access_token = os.getenv('GITHUB_ACCESS_TOKEN') if not access_token: print("\nGitHub access token not found in environment variable GITHUB_ACCESS_TOKEN") print("\nTo get a GitHub access token:") print("1. Run the oauth_demo.py script from examples/oauth/ to get a fresh token") print("2. Or go to GitHub Settings > Developer settings > Personal access tokens") print("3. Create a token with 'read:org' and 'public_repo' scopes") print("\nThen set the token:") print("export GITHUB_ACCESS_TOKEN='your_token_here'") + sys.exit(1) -# Verify token format -if not access_token.startswith(('gho_', 'ghp_', 'github_pat_')): +# Verify token format (GitHub tokens are base62, 40+ chars after prefix) +if access_token and not access_token.startswith(('gho_', 'ghp_', 'github_pat_')): print(f"Warning: Token doesn't look like a GitHub token (got: {access_token[:10]}...)") print("GitHub tokens usually start with 'gho_', 'ghp_', or 'github_pat_'") + print("Proceeding anyway, but authentication may fail.")
179-190: Remove unused helper function_tool_result_to_json.
This function is defined in examples/oauth/workflow_pre_auth/client.py but never called in this module; remove it to avoid dead code.src/mcp_agent/oauth/callbacks.py (2)
26-35: Remove or explain the debugging comment.Line 31 contains a commented-out debug statement (
# print all entries in _pending for debugging). Either remove this comment or replace it with proper logging if debugging is needed.Apply this diff:
async def deliver(self, flow_id: str, payload: Dict[str, Any]) -> bool: """Set the result for a pending flow, returning False when no listener exists.""" async with self._lock: future = self._pending.get(flow_id) if future is None: - # print all entries in _pending for debugging return False
46-50: Consider memory leak prevention.The
discardmethod removes futures from_pending, but there's no automatic cleanup for completed futures. Over time, this could lead to memory growth if flows complete successfully but aren't explicitly discarded.Consider adding periodic cleanup or automatic removal of completed futures in the
deliverandfailmethods:async def deliver(self, flow_id: str, payload: Dict[str, Any]) -> bool: """Set the result for a pending flow, returning False when no listener exists.""" async with self._lock: future = self._pending.get(flow_id) if future is None: return False if not future.done(): future.set_result(payload) + # Clean up completed future to prevent memory leak + self._pending.pop(flow_id, None) return TrueSimilar change for the
failmethod.src/mcp_agent/oauth/store/in_memory.py (1)
17-22: Simplify the get method.Lines 19-21 can be simplified by returning the result directly:
async def get(self, key: TokenStoreKey) -> TokenRecord | None: async with self._lock: - record = self._records.get(key) - if record is None: - return None - return record + return self._records.get(key)docs/oauth_support_design.md (1)
45-58: Add language specifier to fenced code block.The code block at Line 45 should specify a language (e.g.,
plaintextortext) to comply with Markdown linting rules.-``` +```text src/mcp_agent/oauth/ __init__.py identity.py # OAuthUserIdentity, helpers to extract from auth context
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
auth_spec.pngis excluded by!**/*.png
📒 Files selected for processing (40)
docs/configuration.mdx(1 hunks)docs/oauth_support_design.md(1 hunks)examples/oauth/README.md(1 hunks)examples/oauth/dynamic_auth/client.py(1 hunks)examples/oauth/dynamic_auth/main.py(1 hunks)examples/oauth/dynamic_auth/worker.py(1 hunks)examples/oauth/preconfigured/README.md(1 hunks)examples/oauth/preconfigured/main.py(1 hunks)examples/oauth/preconfigured/mcp_agent.config.yaml(1 hunks)examples/oauth/preconfigured/mcp_agent.secrets.yaml.example(1 hunks)examples/oauth/preconfigured/oauth_demo.py(1 hunks)examples/oauth/preconfigured/requirements.txt(1 hunks)examples/oauth/preconfigured/workflow_example.py(1 hunks)examples/oauth/workflow_pre_auth/client.py(1 hunks)examples/oauth/workflow_pre_auth/main.py(1 hunks)examples/oauth/workflow_pre_auth/worker.py(1 hunks)src/mcp_agent/app.py(5 hunks)src/mcp_agent/config.py(3 hunks)src/mcp_agent/core/context.py(5 hunks)src/mcp_agent/executor/workflow.py(0 hunks)src/mcp_agent/mcp/mcp_server_registry.py(2 hunks)src/mcp_agent/oauth/__init__.py(1 hunks)src/mcp_agent/oauth/access_token.py(1 hunks)src/mcp_agent/oauth/callbacks.py(1 hunks)src/mcp_agent/oauth/errors.py(1 hunks)src/mcp_agent/oauth/flow.py(1 hunks)src/mcp_agent/oauth/http/__init__.py(1 hunks)src/mcp_agent/oauth/http/auth.py(1 hunks)src/mcp_agent/oauth/identity.py(1 hunks)src/mcp_agent/oauth/manager.py(1 hunks)src/mcp_agent/oauth/metadata.py(1 hunks)src/mcp_agent/oauth/pkce.py(1 hunks)src/mcp_agent/oauth/records.py(1 hunks)src/mcp_agent/oauth/store/__init__.py(1 hunks)src/mcp_agent/oauth/store/base.py(1 hunks)src/mcp_agent/oauth/store/in_memory.py(1 hunks)src/mcp_agent/server/app_server.py(9 hunks)src/mcp_agent/server/token_verifier.py(1 hunks)tests/test_audience_validation.py(1 hunks)tests/test_oauth_utils.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/mcp_agent/executor/workflow.py
🧰 Additional context used
🧬 Code graph analysis (27)
src/mcp_agent/oauth/http/__init__.py (1)
src/mcp_agent/oauth/http/auth.py (1)
OAuthHttpxAuth(14-78)
src/mcp_agent/oauth/store/__init__.py (2)
src/mcp_agent/oauth/store/base.py (3)
TokenStore(26-33)TokenStoreKey(12-18)scope_fingerprint(21-23)src/mcp_agent/oauth/store/in_memory.py (1)
InMemoryTokenStore(12-30)
src/mcp_agent/core/context.py (3)
src/mcp_agent/oauth/identity.py (1)
OAuthUserIdentity(12-40)src/mcp_agent/oauth/manager.py (2)
TokenManager(52-337)aclose(335-337)src/mcp_agent/oauth/store/base.py (1)
TokenStore(26-33)
examples/oauth/workflow_pre_auth/worker.py (2)
src/mcp_agent/executor/temporal/__init__.py (1)
create_temporal_worker_for_app(498-554)examples/oauth/dynamic_auth/worker.py (1)
main(22-27)
examples/oauth/dynamic_auth/worker.py (3)
src/mcp_agent/executor/temporal/__init__.py (1)
create_temporal_worker_for_app(498-554)examples/oauth/dynamic_auth/main.py (1)
main(146-161)examples/oauth/workflow_pre_auth/worker.py (1)
main(22-27)
tests/test_audience_validation.py (3)
src/mcp_agent/config.py (1)
MCPAuthorizationServerSettings(38-77)src/mcp_agent/server/token_verifier.py (2)
MCPAgentTokenVerifier(21-200)_introspect(53-141)src/mcp_agent/oauth/access_token.py (4)
MCPAccessToken(11-80)_extract_all_audiences(83-103)from_introspection(22-64)validate_audience(73-80)
src/mcp_agent/oauth/records.py (1)
src/mcp_agent/oauth/access_token.py (1)
is_expired(66-71)
src/mcp_agent/oauth/flow.py (7)
src/mcp_agent/core/context.py (2)
mcp(114-115)Context(62-115)src/mcp_agent/config.py (2)
MCPOAuthClientSettings(80-100)OAuthSettings(114-123)src/mcp_agent/oauth/errors.py (4)
AuthorizationDeclined(8-9)CallbackTimeoutError(12-13)MissingUserIdentityError(20-21)OAuthFlowError(4-5)src/mcp_agent/oauth/identity.py (1)
OAuthUserIdentity(12-40)src/mcp_agent/oauth/pkce.py (3)
generate_code_challenge(21-23)generate_code_verifier(10-18)generate_state(26-27)src/mcp_agent/oauth/records.py (1)
TokenRecord(11-46)src/mcp_agent/oauth/callbacks.py (2)
create_handle(16-24)discard(46-50)
src/mcp_agent/oauth/identity.py (1)
src/mcp_agent/oauth/access_token.py (1)
MCPAccessToken(11-80)
examples/oauth/preconfigured/main.py (2)
src/mcp_agent/app.py (5)
MCPApp(56-1085)run(472-495)context(163-168)logger(207-224)server_registry(175-176)src/mcp_agent/mcp/gen_client.py (1)
gen_client(17-49)
src/mcp_agent/server/app_server.py (8)
src/mcp_agent/core/context.py (2)
mcp(114-115)Context(62-115)src/mcp_agent/config.py (1)
MCPOAuthClientSettings(80-100)src/mcp_agent/oauth/identity.py (3)
OAuthUserIdentity(12-40)from_access_token(26-40)cache_key(21-23)src/mcp_agent/oauth/manager.py (2)
create_default_user_for_preconfigured_tokens(41-49)aclose(335-337)src/mcp_agent/server/token_verifier.py (2)
MCPAgentTokenVerifier(21-200)aclose(193-194)src/mcp_agent/oauth/store/base.py (3)
get(29-29)TokenStoreKey(12-18)scope_fingerprint(21-23)src/mcp_agent/oauth/callbacks.py (2)
deliver(26-35)create_handle(16-24)src/mcp_agent/oauth/records.py (1)
TokenRecord(11-46)
examples/oauth/workflow_pre_auth/main.py (5)
src/mcp_agent/core/context.py (2)
mcp(114-115)Context(62-115)src/mcp_agent/app.py (10)
context(163-168)MCPApp(56-1085)config(171-172)logger(207-224)tool(813-813)tool(816-822)tool(824-876)server_registry(175-176)run(472-495)workflows(195-196)src/mcp_agent/server/app_server.py (4)
app(148-150)create_mcp_server_for_app(366-1698)run(2076-2081)workflows(153-155)src/mcp_agent/mcp/gen_client.py (1)
gen_client(17-49)src/mcp_agent/config.py (6)
MCPServerSettings(149-210)Settings(687-800)LoggerSettings(639-684)MCPSettings(213-221)MCPServerAuthSettings(29-35)MCPOAuthClientSettings(80-100)
examples/oauth/dynamic_auth/main.py (4)
src/mcp_agent/server/app_server.py (4)
app(148-150)create_mcp_server_for_app(366-1698)run(2076-2081)workflows(153-155)src/mcp_agent/app.py (2)
config(171-172)logger(207-224)src/mcp_agent/config.py (8)
MCPServerSettings(149-210)LoggerSettings(639-684)MCPSettings(213-221)MCPServerAuthSettings(29-35)MCPOAuthClientSettings(80-100)OAuthSettings(114-123)OAuthTokenStoreSettings(103-111)TemporalSettings(497-517)src/mcp_agent/mcp/gen_client.py (1)
gen_client(17-49)
src/mcp_agent/mcp/mcp_server_registry.py (2)
src/mcp_agent/oauth/http/auth.py (1)
OAuthHttpxAuth(14-78)src/mcp_agent/logging/logger.py (1)
warning(281-289)
src/mcp_agent/oauth/metadata.py (1)
src/mcp_agent/logging/logger.py (2)
get_logger(513-541)warning(281-289)
examples/oauth/preconfigured/oauth_demo.py (2)
examples/oauth/workflow_pre_auth/client.py (1)
main(47-176)examples/oauth/preconfigured/main.py (1)
main(156-176)
examples/oauth/preconfigured/workflow_example.py (5)
src/mcp_agent/app.py (5)
MCPApp(56-1085)context(163-168)logger(207-224)server_registry(175-176)run(472-495)src/mcp_agent/agents/agent.py (1)
Agent(62-1162)src/mcp_agent/core/context.py (2)
Context(62-115)mcp(114-115)src/mcp_agent/mcp/gen_client.py (1)
gen_client(17-49)examples/oauth/workflow_pre_auth/client.py (1)
main(47-176)
examples/oauth/workflow_pre_auth/client.py (7)
src/mcp_agent/config.py (1)
MCPServerSettings(149-210)src/mcp_agent/mcp/gen_client.py (1)
gen_client(17-49)src/mcp_agent/mcp/mcp_agent_client_session.py (1)
MCPAgentClientSession(73-428)src/mcp_agent/human_input/console_handler.py (1)
console_input_callback(68-78)src/mcp_agent/elicitation/handler.py (1)
console_elicitation_callback(138-167)examples/oauth/preconfigured/oauth_demo.py (1)
main(299-380)examples/oauth/dynamic_auth/client.py (5)
main(27-141)on_server_log(50-53)ConsolePrintingClientSession(57-72)_received_notification(58-72)make_session(74-86)
src/mcp_agent/app.py (3)
src/mcp_agent/oauth/manager.py (3)
TokenManager(52-337)create_default_user_for_preconfigured_tokens(41-49)store_preconfigured_token(73-109)src/mcp_agent/oauth/store/in_memory.py (1)
InMemoryTokenStore(12-30)src/mcp_agent/oauth/identity.py (1)
cache_key(21-23)
src/mcp_agent/oauth/manager.py (10)
src/mcp_agent/config.py (2)
MCPOAuthClientSettings(80-100)OAuthSettings(114-123)src/mcp_agent/logging/logger.py (2)
get_logger(513-541)warning(281-289)src/mcp_agent/oauth/errors.py (2)
OAuthFlowError(4-5)TokenRefreshError(16-17)src/mcp_agent/oauth/flow.py (2)
AuthorizationFlowCoordinator(37-212)authorize(44-212)src/mcp_agent/oauth/identity.py (2)
OAuthUserIdentity(12-40)cache_key(21-23)src/mcp_agent/oauth/metadata.py (4)
fetch_authorization_server_metadata(25-31)fetch_resource_metadata(15-22)normalize_resource(55-60)select_authorization_server(34-52)src/mcp_agent/oauth/records.py (2)
TokenRecord(11-46)is_expired(26-30)src/mcp_agent/oauth/store/in_memory.py (3)
InMemoryTokenStore(12-30)get(17-22)delete(28-30)src/mcp_agent/oauth/store/base.py (5)
TokenStore(26-33)TokenStoreKey(12-18)scope_fingerprint(21-23)get(29-29)delete(33-33)src/mcp_agent/core/context.py (1)
Context(62-115)
examples/oauth/dynamic_auth/client.py (6)
src/mcp_agent/config.py (1)
MCPServerSettings(149-210)src/mcp_agent/mcp/gen_client.py (1)
gen_client(17-49)src/mcp_agent/mcp/mcp_agent_client_session.py (1)
MCPAgentClientSession(73-428)src/mcp_agent/human_input/console_handler.py (1)
console_input_callback(68-78)src/mcp_agent/elicitation/handler.py (1)
console_elicitation_callback(138-167)examples/oauth/workflow_pre_auth/client.py (5)
main(47-176)on_server_log(72-75)ConsolePrintingClientSession(79-94)_received_notification(80-94)make_session(96-108)
src/mcp_agent/oauth/http/auth.py (2)
src/mcp_agent/oauth/manager.py (3)
TokenManager(52-337)ensure_access_token(111-231)invalidate(233-247)src/mcp_agent/core/context.py (1)
Context(62-115)
src/mcp_agent/oauth/store/in_memory.py (2)
src/mcp_agent/oauth/store/base.py (4)
TokenStore(26-33)TokenStoreKey(12-18)get(29-29)delete(33-33)src/mcp_agent/oauth/records.py (1)
TokenRecord(11-46)
tests/test_oauth_utils.py (4)
src/mcp_agent/oauth/metadata.py (2)
normalize_resource(55-60)select_authorization_server(34-52)src/mcp_agent/oauth/records.py (2)
TokenRecord(11-46)is_expired(26-30)src/mcp_agent/oauth/store/in_memory.py (3)
InMemoryTokenStore(12-30)get(17-22)delete(28-30)src/mcp_agent/oauth/store/base.py (4)
TokenStoreKey(12-18)scope_fingerprint(21-23)get(29-29)delete(33-33)
src/mcp_agent/server/token_verifier.py (3)
src/mcp_agent/config.py (1)
MCPAuthorizationServerSettings(38-77)src/mcp_agent/logging/logger.py (2)
get_logger(513-541)warning(281-289)src/mcp_agent/oauth/access_token.py (2)
MCPAccessToken(11-80)from_introspection(22-64)
src/mcp_agent/oauth/access_token.py (1)
src/mcp_agent/oauth/records.py (1)
is_expired(26-30)
src/mcp_agent/oauth/store/base.py (2)
src/mcp_agent/oauth/records.py (1)
TokenRecord(11-46)src/mcp_agent/oauth/store/in_memory.py (2)
get(17-22)delete(28-30)
🪛 Gitleaks (8.28.0)
examples/oauth/preconfigured/README.md
[high] 330-330: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🪛 markdownlint-cli2 (0.18.1)
docs/oauth_support_design.md
45-45: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
examples/oauth/preconfigured/README.md
84-84: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
292-292: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
298-298: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
304-304: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
310-310: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 OSV Scanner (2.2.2)
examples/oauth/preconfigured/requirements.txt
[HIGH] 1-1: ujson 5.0.0: Stack-buffer-overflow in Buffer_AppendIndentUnchecked
(OSV-2021-955)
[HIGH] 1-1: ujson 5.0.0: undefined
(PYSEC-2022-25)
[HIGH] 1-1: ujson 5.0.0: UltraJSON vulnerable to Out-of-bounds Write
[HIGH] 1-1: ujson 5.0.0: Potential double free of buffer during string decoding
[HIGH] 1-1: ujson 5.0.0: Incorrect handling of invalid surrogate pair characters
🔇 Additional comments (47)
examples/oauth/dynamic_auth/worker.py (1)
22-27: LGTM: Worker setup is correct.The worker setup properly uses the async context manager from
create_temporal_worker_for_appand awaitsworker.run(), following the pattern established in other example workers.src/mcp_agent/oauth/pkce.py (3)
21-23: LGTM: Code challenge generation is correct.The SHA-256 hashing and base64url encoding (with padding removed) correctly implements RFC 7636 requirements for PKCE code challenge with the S256 method.
26-27: LGTM: State generation is secure.Using
secrets.token_urlsafeprovides cryptographically strong random state values suitable for CSRF protection in OAuth flows.
10-18: No changes needed for generate_code_verifier. Verified it consistently returns the requested length (43–128) with only URL-safe characters; the existing heuristic plus fallback is safe.Likely an incorrect or invalid review comment.
src/mcp_agent/oauth/store/base.py (3)
11-18: LGTM: TokenStoreKey design is sound.The frozen dataclass with user_key, resource, authorization_server, and scope_fingerprint provides a good composite key for token isolation. The optional authorization_server field allows flexibility for different OAuth configurations.
21-23: LGTM: Scope fingerprint implementation is correct.The deterministic fingerprint (sorted, deduplicated, space-joined) ensures consistent cache keys regardless of scope ordering or duplicates in the input.
26-33: LGTM: TokenStore protocol is complete.The async protocol with get/set/delete operations provides a clean abstraction for different storage backends (in-memory, Redis, etc.). The Protocol typing ensures compile-time interface checking.
examples/oauth/workflow_pre_auth/client.py (1)
110-164: LGTM: Error handling is comprehensive.The exception handling correctly identifies and tolerates benign shutdown races (BrokenResourceError) while re-raising unexpected exceptions. The fallback to string matching handles cases where the backport isn't available.
src/mcp_agent/oauth/callbacks.py (1)
9-24: LGTM: Future creation and reuse logic is correct.The implementation properly:
- Uses asyncio.Lock for thread-safe access to the pending dictionary
- Reuses existing futures if they're not yet completed
- Creates new futures on the running event loop
- Handles the race condition where a future might complete between checks
src/mcp_agent/config.py (7)
14-22: LGTM!The import additions support the new OAuth configuration models appropriately.
AnyHttpUrlenables URL validation, andmodel_validatorallows custom validation logic for OAuth settings.
29-36: LGTM!The OAuth client settings integration into
MCPServerAuthSettingsis well-structured. The forward reference andarbitrary_types_allowedconfiguration are appropriate for supporting nested OAuth models.
38-77: LGTM with note on planned improvement.The authorization server configuration model correctly enforces required fields (
issuer_url,resource_server_url,expected_audiences) when enabled, aligning with RFC 9068 compliance. The TODO comment at line 53 indicates a planned refinement to tieexpected_audiencestoapp_id/config_idfor uniqueness, which is a reasonable future enhancement that doesn't block the current implementation.
80-101: LGTM with good security defaults.The OAuth client settings model is comprehensive and includes sensible security defaults: PKCE is required by default (
require_pkce=True), and internal callback handling is enabled (use_internal_callback=True). The support for pre-configured tokens provides flexibility for scenarios where tokens are obtained externally.
103-111: LGTM!The token store settings provide appropriate configuration for token persistence with support for both in-memory (development) and Redis (production) backends. The
refresh_leeway_secondsdefault of 60 seconds with a non-negative constraint is sensible.
114-123: LGTM!Global OAuth settings are well-structured with sensible defaults: 5-minute flow timeout (minimum 30 seconds) provides adequate time for user authorization while preventing indefinite waits.
739-743: LGTM!The OAuth configuration fields integrate cleanly into the Settings class. The
authorizationfield is appropriately optional (None default) for opt-in protected resource mode, whileoauthhas a default factory ensuring consistent initialization.src/mcp_agent/oauth/http/__init__.py (1)
1-6: LGTM!The module cleanly exposes the OAuth HTTP authentication helper through a well-defined public API. The docstring and
__all__declaration make the intended usage clear.src/mcp_agent/oauth/store/__init__.py (1)
1-12: LGTM!The token store package cleanly exposes its public API: the base protocol (
TokenStore), key structure (TokenStoreKey), utility function (scope_fingerprint), and in-memory implementation (InMemoryTokenStore). The structure promotes clear separation between interface and implementation.examples/oauth/workflow_pre_auth/worker.py (1)
1-32: LGTM!The worker script follows the standard Temporal worker setup pattern using
create_temporal_worker_for_app. The structure is appropriate for an example demonstrating pre-auth workflow support.src/mcp_agent/oauth/__init__.py (1)
1-20: LGTM!The OAuth package initializer appropriately avoids eager imports to prevent circular dependencies while clearly defining the public API surface through
__all__. This is a sensible pattern for complex package initialization.src/mcp_agent/mcp/mcp_server_registry.py (2)
34-34: LGTM!The import of
OAuthHttpxAuthis appropriately placed for integrating OAuth authentication with Streamable HTTP transports.
219-236: LGTM with good defensive checks.The OAuth authentication integration for Streamable HTTP transports correctly validates both configuration (
oauth_cfg.enabled) and runtime prerequisites (token_manageravailability) before enabling auth. The warning message at line 223 provides clear diagnostic information when OAuth is misconfigured, and graceful degradation (no auth) is appropriate for this scenario.docs/configuration.mdx (1)
126-168: LGTM with clear distinction between authorization modes.The OAuth configuration section effectively distinguishes between inbound token validation (
authorization) and delegated authorization for downstream servers (oauth). The example at lines 133-163 demonstrates proper configuration of both scenarios, and the guidance at lines 165-167 clearly explains:
- Authorization server advertising and token enforcement
- Token store backend recommendations (memory vs Redis)
- Downstream server OAuth configuration and planned dynamic client registration
examples/oauth/preconfigured/mcp_agent.config.yaml (1)
1-116: LGTM with helpful guidance for users.tests/test_oauth_utils.py (5)
25-28: LGTM!The test correctly verifies deterministic scope ordering and deduplication.
31-37: LGTM!The test correctly validates expiry logic with and without leeway.
40-55: LGTM!The async round-trip test correctly validates store operations (set, get, delete).
77-86: LGTM!The test correctly validates resource normalization with fallback and error handling.
58-74: ProtectedResourceMetadata normalizes URLs with trailing slashes; tests are valid
No changes required.src/mcp_agent/oauth/errors.py (1)
1-22: LGTM!Clean exception hierarchy with descriptive docstrings for OAuth-related failures.
tests/test_audience_validation.py (9)
12-32: LGTM!Test correctly validates successful audience matching.
36-55: LGTM!Test correctly validates audience validation failure with non-matching audiences.
59-79: LGTM!Test correctly validates OAuth 2.0 resource indicators as audience sources.
83-92: LGTM!Test correctly validates extraction of multiple audiences from both aud and resource claims.
96-105: LGTM!Test correctly handles string-valued audience claims.
109-120: LGTM!Test correctly validates that missing audiences cause validation failure.
123-141: LGTM!Test correctly validates configuration requirements for RFC 9068 compliance.
147-196: LGTM!Integration test correctly validates audience handling in the token verifier with mocked HTTP responses.
201-224: LGTM!Test correctly handles edge cases in audience extraction (empty payloads, None values, duplicates).
src/mcp_agent/oauth/identity.py (2)
11-40: LGTM!Clean implementation of OAuthUserIdentity with proper fallback logic for extracting identity from access tokens.
43-46: LGTM!Helper function correctly extracts claims from tokens with proper None handling.
src/mcp_agent/core/context.py (3)
36-57: LGTM!Proper use of TYPE_CHECKING to avoid circular imports while maintaining type safety.
101-106: LGTM!OAuth fields are properly integrated into the Context with clear documentation.
271-277: LGTM!Token manager cleanup is properly integrated with exception handling.
src/mcp_agent/app.py (3)
6-6: LGTM!Proper imports for OAuth token management and FunctionType for wrapper handling.
Also applies to: 43-44
254-275: LGTM!OAuth initialization is properly guarded with try-except and integrates cleanly with the context.
623-640: LGTM!Using
FunctionTypeto rebuild the wrapper with the original function's globals ensures that string annotations resolve correctly in the workflow module's namespace.
examples/oauth/dynamic_auth/main.py
Outdated
| oauth=OAuthSettings( | ||
| callback_base_url=AnyHttpUrl("http://localhost:8000"), | ||
| flow_timeout_seconds=300, | ||
| token_store=OAuthTokenStoreSettings(refresh_leeway_seconds=60), | ||
| ), | ||
| mcp=MCPSettings( | ||
| servers={ | ||
| "github": MCPServerSettings( | ||
| name="github", | ||
| transport="streamable_http", | ||
| url="https://api.githubcopilot.com/mcp/", | ||
| auth=MCPServerAuthSettings( | ||
| oauth=MCPOAuthClientSettings( | ||
| client_id=client_id, | ||
| client_secret=client_secret, | ||
| use_internal_callback=True, | ||
| enabled=True, | ||
| scopes= [ | ||
| "read:org", # Required for search_orgs tool | ||
| "public_repo", # Access to public repositories | ||
| "user:email" # User information access | ||
| ], | ||
| authorization_server=AnyHttpUrl("https://github.com/login/oauth"), | ||
| resource=AnyHttpUrl("https://api.githubcopilot.com/mcp") | ||
| ) |
There was a problem hiding this comment.
Fix AnyHttpUrl instantiation
Line 53 (and the other AnyHttpUrl(...) calls in this block) will raise TypeError: 'UrlConstraints' object is not callable on import because in Pydantic v2 AnyHttpUrl is just a typing alias, not a constructor. That means the example crashes before it even runs. Pass plain strings (Pydantic will coerce them) or validate via TypeAdapter instead.
- callback_base_url=AnyHttpUrl("http://localhost:8000"),
+ callback_base_url="http://localhost:8000",
@@
- authorization_server=AnyHttpUrl("https://github.com/login/oauth"),
- resource=AnyHttpUrl("https://api.githubcopilot.com/mcp")
+ authorization_server="https://github.com/login/oauth",
+ resource="https://api.githubcopilot.com/mcp"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| oauth=OAuthSettings( | |
| callback_base_url=AnyHttpUrl("http://localhost:8000"), | |
| flow_timeout_seconds=300, | |
| token_store=OAuthTokenStoreSettings(refresh_leeway_seconds=60), | |
| ), | |
| mcp=MCPSettings( | |
| servers={ | |
| "github": MCPServerSettings( | |
| name="github", | |
| transport="streamable_http", | |
| url="https://api.githubcopilot.com/mcp/", | |
| auth=MCPServerAuthSettings( | |
| oauth=MCPOAuthClientSettings( | |
| client_id=client_id, | |
| client_secret=client_secret, | |
| use_internal_callback=True, | |
| enabled=True, | |
| scopes= [ | |
| "read:org", # Required for search_orgs tool | |
| "public_repo", # Access to public repositories | |
| "user:email" # User information access | |
| ], | |
| authorization_server=AnyHttpUrl("https://github.com/login/oauth"), | |
| resource=AnyHttpUrl("https://api.githubcopilot.com/mcp") | |
| ) | |
| oauth=OAuthSettings( | |
| callback_base_url="http://localhost:8000", | |
| flow_timeout_seconds=300, | |
| token_store=OAuthTokenStoreSettings(refresh_leeway_seconds=60), | |
| ), | |
| mcp=MCPSettings( | |
| servers={ | |
| "github": MCPServerSettings( | |
| name="github", | |
| transport="streamable_http", | |
| url="https://api.githubcopilot.com/mcp/", | |
| auth=MCPServerAuthSettings( | |
| oauth=MCPOAuthClientSettings( | |
| client_id=client_id, | |
| client_secret=client_secret, | |
| use_internal_callback=True, | |
| enabled=True, | |
| scopes=[ | |
| "read:org", # Required for search_orgs tool | |
| "public_repo", # Access to public repositories | |
| "user:email" # User information access | |
| ], | |
| authorization_server="https://github.com/login/oauth", | |
| resource="https://api.githubcopilot.com/mcp" | |
| ) | |
| ) | |
| } | |
| } | |
| ) |
🤖 Prompt for AI Agents
In examples/oauth/dynamic_auth/main.py around lines 52 to 76, the code currently
calls AnyHttpUrl(...) which in Pydantic v2 is a typing alias (not callable) and
will raise TypeError on import; replace these AnyHttpUrl(...) instantiations
with plain string literals (e.g. "http://localhost:8000",
"https://github.com/login/oauth", "https://api.githubcopilot.com/mcp") so
Pydantic can coerce/validate them, or alternatively validate using a TypeAdapter
if you need explicit runtime validation; apply this change to callback_base_url,
authorization_server, resource and any other AnyHttpUrl calls in this block.
| ujson>=5.0.0 | ||
|
|
There was a problem hiding this comment.
Avoid shipping known-vulnerable ujson floor
The lower bound ujson>=5.0.0 allows installation of versions with multiple high-severity CVEs (OSV-2021-955, GHSA-fh56-85cw-5pq6, GHSA-fm67-cv37-96ff, GHSA-wpqr-jcpx-745r). Please raise the minimum to a patched release or drop the dependency from this default requirements file until you can confirm a safe version.
Apply this diff if you prefer to remove it for now:
-# Optional: Enhanced JSON handling
-ujson>=5.0.0
+# Optional: Enhanced JSON handling (install manually after selecting a patched version)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In examples/oauth/preconfigured/requirements.txt around lines 19-20, the current
lower bound "ujson>=5.0.0" may allow known vulnerable ujson releases; remove
this dependency from the default requirements file (delete the ujson line) or,
if you must keep it, replace the range with a verified safe minimum (pin to a
patched release after confirming which version fixes the listed CVEs) and update
any tests/requirements-lock accordingly so installations no longer pull
vulnerable versions.
| pytest-asyncio>=0.21.1 | ||
|
|
||
| # For the interactive OAuth demo | ||
| webbrowser # Standard library - no explicit install needed |
There was a problem hiding this comment.
Drop webbrowser from requirements
webbrowser is part of the Python standard library; keeping it here forces pip to look for a third-party package that doesn’t exist, so pip install -r requirements.txt will fail. Please remove this line.
-# For the interactive OAuth demo
-webbrowser # Standard library - no explicit install needed
+# For the interactive OAuth demo
+# Uses Python's built-in webbrowser module (no install required)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| webbrowser # Standard library - no explicit install needed | |
| # For the interactive OAuth demo | |
| # Uses Python's built-in webbrowser module (no install required) |
🤖 Prompt for AI Agents
In examples/oauth/preconfigured/requirements.txt around line 32, the file
incorrectly lists "webbrowser" which is part of the Python standard library and
causes pip to fail looking for a nonexistent package; remove the "webbrowser"
line from requirements.txt, save the file, run pip install -r requirements.txt
to verify there are no lookup errors, and commit the change.
| query_mapping = { | ||
| "AI/ML": ["machine-learning", "artificial-intelligence", "deep-learning", "tensorflow", "pytorch"], | ||
| "cloud": ["cloud-computing", "aws", "azure", "kubernetes", "docker"], | ||
| "security": ["cybersecurity", "security", "encryption", "vulnerability"], | ||
| "web": ["web-development", "javascript", "react", "vue", "angular"], | ||
| "mobile": ["mobile-development", "android", "ios", "react-native", "flutter"], | ||
| "data": ["data-science", "analytics", "big-data", "database", "sql"], | ||
| "devtools": ["developer-tools", "ci-cd", "testing", "monitoring", "automation"] | ||
| } | ||
|
|
||
| all_queries = [] | ||
| for area in focus_areas: | ||
| queries = query_mapping.get(area.lower(), [area.lower()]) | ||
| all_queries.extend(queries) | ||
|
|
||
| # Remove duplicates while preserving order | ||
| unique_queries = list(dict.fromkeys(all_queries)) | ||
|
|
There was a problem hiding this comment.
Preserve curated queries when mapping focus areas.
query_mapping defines entries like "AI/ML", but the lookup calls area.lower(), so "AI/ML" turns into "ai/ml" and always hits the fallback. That drops all of the richer query lists you intended for those focus areas.
Normalize the dictionary to lowercase (and reuse the same lowercase key during lookup) so predefined queries are actually used.
- query_mapping = {
- "AI/ML": ["machine-learning", "artificial-intelligence", "deep-learning", "tensorflow", "pytorch"],
+ query_mapping = {
+ "ai/ml": ["machine-learning", "artificial-intelligence", "deep-learning", "tensorflow", "pytorch"],
"cloud": ["cloud-computing", "aws", "azure", "kubernetes", "docker"],
"security": ["cybersecurity", "security", "encryption", "vulnerability"],
"web": ["web-development", "javascript", "react", "vue", "angular"],
"mobile": ["mobile-development", "android", "ios", "react-native", "flutter"],
"data": ["data-science", "analytics", "big-data", "database", "sql"],
"devtools": ["developer-tools", "ci-cd", "testing", "monitoring", "automation"]
}
all_queries = []
for area in focus_areas:
- queries = query_mapping.get(area.lower(), [area.lower()])
+ key = area.lower()
+ queries = query_mapping.get(key, [key])
all_queries.extend(queries)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| query_mapping = { | |
| "AI/ML": ["machine-learning", "artificial-intelligence", "deep-learning", "tensorflow", "pytorch"], | |
| "cloud": ["cloud-computing", "aws", "azure", "kubernetes", "docker"], | |
| "security": ["cybersecurity", "security", "encryption", "vulnerability"], | |
| "web": ["web-development", "javascript", "react", "vue", "angular"], | |
| "mobile": ["mobile-development", "android", "ios", "react-native", "flutter"], | |
| "data": ["data-science", "analytics", "big-data", "database", "sql"], | |
| "devtools": ["developer-tools", "ci-cd", "testing", "monitoring", "automation"] | |
| } | |
| all_queries = [] | |
| for area in focus_areas: | |
| queries = query_mapping.get(area.lower(), [area.lower()]) | |
| all_queries.extend(queries) | |
| # Remove duplicates while preserving order | |
| unique_queries = list(dict.fromkeys(all_queries)) | |
| query_mapping = { | |
| "ai/ml": ["machine-learning", "artificial-intelligence", "deep-learning", "tensorflow", "pytorch"], | |
| "cloud": ["cloud-computing", "aws", "azure", "kubernetes", "docker"], | |
| "security": ["cybersecurity", "security", "encryption", "vulnerability"], | |
| "web": ["web-development", "javascript", "react", "vue", "angular"], | |
| "mobile": ["mobile-development", "android", "ios", "react-native", "flutter"], | |
| "data": ["data-science", "analytics", "big-data", "database", "sql"], | |
| "devtools": ["developer-tools", "ci-cd", "testing", "monitoring", "automation"] | |
| } | |
| all_queries = [] | |
| for area in focus_areas: | |
| key = area.lower() | |
| queries = query_mapping.get(key, [key]) | |
| all_queries.extend(queries) | |
| # Remove duplicates while preserving order | |
| unique_queries = list(dict.fromkeys(all_queries)) |
🤖 Prompt for AI Agents
In examples/oauth/preconfigured/workflow_example.py around lines 230-247, the
query_mapping keys use mixed case like "AI/ML" but the lookup uses area.lower(),
so mappings never match and fallback single-term queries are used; fix by
normalizing the mapping keys to lowercase (e.g., convert all keys to .lower()
when creating query_mapping or build a new dict with k.lower():v) and then
continue to use queries = query_mapping.get(area.lower(), [area.lower()]) so the
curated lists are preserved and duplicates are still removed afterward.
| settings = Settings( | ||
| execution_engine="asyncio", | ||
| logger=LoggerSettings(level="info"), | ||
| mcp=MCPSettings( | ||
| servers={ | ||
| "github": MCPServerSettings( | ||
| name="github", | ||
| transport="streamable_http", | ||
| url="https://api.githubcopilot.com/mcp/", | ||
| auth=MCPServerAuthSettings( | ||
| oauth=MCPOAuthClientSettings( | ||
| enabled=True, | ||
| scopes= [ | ||
| "read:org", # Required for search_orgs tool | ||
| "public_repo", # Access to public repositories | ||
| "user:email" # User information access | ||
| ], | ||
| authorization_server=AnyHttpUrl("https://github.com/login/oauth"), | ||
| resource=AnyHttpUrl("https://api.githubcopilot.com/mcp") | ||
| ) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Stop calling AnyHttpUrl as a constructor
Lines 50-52 call AnyHttpUrl(...), which explodes under Pydantic v2 with TypeError: 'UrlConstraints' object is not callable. As in the other example, this prevents the module from loading. Feed raw strings (letting the Settings model validate them) or go through a TypeAdapter.
- authorization_server=AnyHttpUrl("https://github.com/login/oauth"),
- resource=AnyHttpUrl("https://api.githubcopilot.com/mcp")
+ authorization_server="https://github.com/login/oauth",
+ resource="https://api.githubcopilot.com/mcp"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| settings = Settings( | |
| execution_engine="asyncio", | |
| logger=LoggerSettings(level="info"), | |
| mcp=MCPSettings( | |
| servers={ | |
| "github": MCPServerSettings( | |
| name="github", | |
| transport="streamable_http", | |
| url="https://api.githubcopilot.com/mcp/", | |
| auth=MCPServerAuthSettings( | |
| oauth=MCPOAuthClientSettings( | |
| enabled=True, | |
| scopes= [ | |
| "read:org", # Required for search_orgs tool | |
| "public_repo", # Access to public repositories | |
| "user:email" # User information access | |
| ], | |
| authorization_server=AnyHttpUrl("https://github.com/login/oauth"), | |
| resource=AnyHttpUrl("https://api.githubcopilot.com/mcp") | |
| ) | |
| ) | |
| ) | |
| settings = Settings( | |
| execution_engine="asyncio", | |
| logger=LoggerSettings(level="info"), | |
| mcp=MCPSettings( | |
| servers={ | |
| "github": MCPServerSettings( | |
| name="github", | |
| transport="streamable_http", | |
| url="https://api.githubcopilot.com/mcp/", | |
| auth=MCPServerAuthSettings( | |
| oauth=MCPOAuthClientSettings( | |
| enabled=True, | |
| scopes=[ | |
| "read:org", # Required for search_orgs tool | |
| "public_repo", # Access to public repositories | |
| "user:email" # User information access | |
| ], | |
| authorization_server="https://github.com/login/oauth", | |
| resource="https://api.githubcopilot.com/mcp" | |
| ) | |
| ) | |
| ) | |
| } | |
| ) | |
| ) |
🤖 Prompt for AI Agents
In examples/oauth/workflow_pre_auth/main.py around lines 33 to 54, the code
incorrectly calls AnyHttpUrl(...) which raises TypeError under Pydantic v2;
update the MCP OAuth URL fields to be plain string literals (e.g.
"https://github.com/login/oauth" and "https://api.githubcopilot.com/mcp") so the
Settings model validates them, or if you need explicit upfront validation use a
Pydantic TypeAdapter for AnyHttpUrl to validate the strings before assignment.
| # check for a globally configure token | ||
| key = TokenStoreKey( | ||
| user_key=create_default_user_for_preconfigured_tokens().cache_key, | ||
| resource=resource_str, | ||
| authorization_server=auth_server_str, | ||
| scope_fingerprint=scope_fingerprint(scope_list) | ||
| ) | ||
|
|
||
| lock = self._locks[key] | ||
|
|
||
| async with lock: | ||
| record = await self._token_store.get(key) | ||
| if record: | ||
| return record | ||
|
|
||
| # there is no global token, look for a user specific one | ||
| key = TokenStoreKey( | ||
| user_key=user.cache_key, | ||
| resource=resource_str, | ||
| authorization_server=auth_server_str, | ||
| scope_fingerprint=scope_fingerprint(scope_list) | ||
| ) | ||
|
|
||
| lock = self._locks[key] | ||
| async with lock: | ||
| record = await self._token_store.get(key) | ||
| leeway = ( | ||
| self._settings.token_store.refresh_leeway_seconds | ||
| if self._settings and self._settings.token_store | ||
| else 60 | ||
| ) | ||
| if record and not record.is_expired(leeway_seconds=leeway): | ||
| return record | ||
|
|
There was a problem hiding this comment.
Fix token-store key mismatch and null-user crash.
store_preconfigured_token writes records using a synthetic user (which may include the session_id) and normalizes resource to "" when it’s absent. Here we rebuild the lookup key differently—resource stays None and we ignore the session—so preconfigured tokens are never found (e.g., stdio servers without url fall straight into the OAuth flow). Immediately afterward we call user.cache_key without verifying that context.current_user exists, which raises an AttributeError instead of a clear OAuth error. Result: pre-auth tokens are skipped and requests crash when no user is set.
Align the key construction with the stored form and guard the user lookup so we fail gracefully.
-from mcp_agent.oauth.errors import (
- OAuthFlowError,
- TokenRefreshError,
-)
+from mcp_agent.oauth.errors import (
+ MissingUserIdentityError,
+ OAuthFlowError,
+ TokenRefreshError,
+)
@@
- # check for a globally configure token
- key = TokenStoreKey(
- user_key=create_default_user_for_preconfigured_tokens().cache_key,
- resource=resource_str,
- authorization_server=auth_server_str,
- scope_fingerprint=scope_fingerprint(scope_list)
- )
+ resource_key = resource_str or ""
+ global_user_key = create_default_user_for_preconfigured_tokens(
+ context.session_id
+ ).cache_key
+ # check for a globally configured token
+ key = TokenStoreKey(
+ user_key=global_user_key,
+ resource=resource_key,
+ authorization_server=auth_server_str,
+ scope_fingerprint=scope_fingerprint(scope_list),
+ )
@@
- # there is no global token, look for a user specific one
- key = TokenStoreKey(
- user_key=user.cache_key,
- resource=resource_str,
- authorization_server=auth_server_str,
- scope_fingerprint=scope_fingerprint(scope_list)
- )
+ # there is no global token, look for a user specific one
+ if user is None:
+ raise MissingUserIdentityError(
+ f"Cannot acquire OAuth token for '{server_name}' without an authenticated MCP user"
+ )
+ key = TokenStoreKey(
+ user_key=user.cache_key,
+ resource=resource_key,
+ authorization_server=auth_server_str,
+ scope_fingerprint=scope_fingerprint(scope_list),
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # check for a globally configure token | |
| key = TokenStoreKey( | |
| user_key=create_default_user_for_preconfigured_tokens().cache_key, | |
| resource=resource_str, | |
| authorization_server=auth_server_str, | |
| scope_fingerprint=scope_fingerprint(scope_list) | |
| ) | |
| lock = self._locks[key] | |
| async with lock: | |
| record = await self._token_store.get(key) | |
| if record: | |
| return record | |
| # there is no global token, look for a user specific one | |
| key = TokenStoreKey( | |
| user_key=user.cache_key, | |
| resource=resource_str, | |
| authorization_server=auth_server_str, | |
| scope_fingerprint=scope_fingerprint(scope_list) | |
| ) | |
| lock = self._locks[key] | |
| async with lock: | |
| record = await self._token_store.get(key) | |
| leeway = ( | |
| self._settings.token_store.refresh_leeway_seconds | |
| if self._settings and self._settings.token_store | |
| else 60 | |
| ) | |
| if record and not record.is_expired(leeway_seconds=leeway): | |
| return record | |
| from mcp_agent.oauth.errors import ( | |
| MissingUserIdentityError, | |
| OAuthFlowError, | |
| TokenRefreshError, | |
| ) | |
| resource_key = resource_str or "" | |
| global_user_key = create_default_user_for_preconfigured_tokens( | |
| context.session_id | |
| ).cache_key | |
| # check for a globally configured token | |
| key = TokenStoreKey( | |
| user_key=global_user_key, | |
| resource=resource_key, | |
| authorization_server=auth_server_str, | |
| scope_fingerprint=scope_fingerprint(scope_list), | |
| ) | |
| lock = self._locks[key] | |
| async with lock: | |
| record = await self._token_store.get(key) | |
| if record: | |
| return record | |
| # there is no global token, look for a user specific one | |
| if user is None: | |
| raise MissingUserIdentityError( | |
| f"Cannot acquire OAuth token for '{server_name}' without an authenticated MCP user" | |
| ) | |
| key = TokenStoreKey( | |
| user_key=user.cache_key, | |
| resource=resource_key, | |
| authorization_server=auth_server_str, | |
| scope_fingerprint=scope_fingerprint(scope_list), | |
| ) | |
| lock = self._locks[key] | |
| async with lock: | |
| record = await self._token_store.get(key) | |
| leeway = ( | |
| self._settings.token_store.refresh_leeway_seconds | |
| if self._settings and self._settings.token_store | |
| else 60 | |
| ) | |
| if record and not record.is_expired(leeway_seconds=leeway): | |
| return record |
| store_key = TokenStoreKey( | ||
| user_key=app_context.current_user.cache_key, | ||
| resource=resource_str, | ||
| authorization_server=auth_server_str, | ||
| scope_fingerprint=scope_fingerprint(scope_list), | ||
| ) |
There was a problem hiding this comment.
Guard against missing user identity before storing tokens
app_context.current_user can be None whenever _set_upstream_from_request_ctx_if_available fails to derive an identity (e.g., headless/internal requests without session_id). In that case this path blows up with an AttributeError on cache_key, aborting the pre-auth flow. Please ensure a user identity is present (fallback to create_default_user_for_preconfigured_tokens, or raise a clear ToolError) before constructing the TokenStoreKey.
- # Create storage key using current user
- store_key = TokenStoreKey(
- user_key=app_context.current_user.cache_key,
+ identity = app_context.current_user
+ if identity is None:
+ try:
+ session_id = ctx.request_context.request.query_params.get("session_id")
+ except Exception:
+ session_id = None
+ identity = create_default_user_for_preconfigured_tokens(session_id)
+ app_context.current_user = identity
+ if identity is None:
+ raise ToolError("Unable to resolve user identity for token storage.")
+
+ # Create storage key using resolved user identity
+ store_key = TokenStoreKey(
+ user_key=identity.cache_key,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| store_key = TokenStoreKey( | |
| user_key=app_context.current_user.cache_key, | |
| resource=resource_str, | |
| authorization_server=auth_server_str, | |
| scope_fingerprint=scope_fingerprint(scope_list), | |
| ) | |
| identity = app_context.current_user | |
| if identity is None: | |
| try: | |
| session_id = ctx.request_context.request.query_params.get("session_id") | |
| except Exception: | |
| session_id = None | |
| identity = create_default_user_for_preconfigured_tokens(session_id) | |
| app_context.current_user = identity | |
| if identity is None: | |
| raise ToolError("Unable to resolve user identity for token storage.") | |
| # Create storage key using resolved user identity | |
| store_key = TokenStoreKey( | |
| user_key=identity.cache_key, | |
| resource=resource_str, | |
| authorization_server=auth_server_str, | |
| scope_fingerprint=scope_fingerprint(scope_list), | |
| ) |
🤖 Prompt for AI Agents
In src/mcp_agent/server/app_server.py around lines 1651 to 1656, guard the
TokenStoreKey construction against app_context.current_user being None: check if
app_context.current_user is present and if not, attempt to obtain a fallback via
create_default_user_for_preconfigured_tokens (assigning it to
app_context.current_user) or raise a clear ToolError with a descriptive message;
only then build the TokenStoreKey using the guaranteed user.cache_key so the
pre-auth flow does not raise AttributeError.
tests/test_audience_validation.py
Outdated
| async def test_partial_audience_match(): | ||
| """Test that partial audience matches are sufficient for validation.""" | ||
| settings = MCPAuthorizationServerSettings( | ||
| enabled=True, | ||
| issuer_url="https://auth.example.com", | ||
| resource_server_url="https://api.example.com", | ||
| introspection_endpoint="https://auth.example.com/introspect", | ||
| expected_audiences=["https://api.example.com", "https://other-api.com"], | ||
| strict_audience_validation=True, | ||
| ) | ||
|
|
||
| # Token has one matching and one non-matching audience | ||
| payload = { | ||
| "active": True, | ||
| "aud": ["https://api.example.com", "https://unrelated.com"], | ||
| "sub": "user123", | ||
| "exp": 1234567890, | ||
| "iss": "https://auth.example.com/", | ||
| } | ||
|
|
||
| token = MCPAccessToken.from_introspection("test_token", payload) | ||
| # Should succeed because at least one audience matches | ||
| assert token.validate_audience(settings.expected_audiences) is True No newline at end of file |
There was a problem hiding this comment.
Remove non-existent configuration field.
Line 236 references strict_audience_validation=True, but this field does not exist in MCPAuthorizationServerSettings (src/mcp_agent/config.py:37-76). This will cause a Pydantic validation error when the test runs.
settings = MCPAuthorizationServerSettings(
enabled=True,
issuer_url="https://auth.example.com",
resource_server_url="https://api.example.com",
introspection_endpoint="https://auth.example.com/introspect",
expected_audiences=["https://api.example.com", "https://other-api.com"],
- strict_audience_validation=True,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def test_partial_audience_match(): | |
| """Test that partial audience matches are sufficient for validation.""" | |
| settings = MCPAuthorizationServerSettings( | |
| enabled=True, | |
| issuer_url="https://auth.example.com", | |
| resource_server_url="https://api.example.com", | |
| introspection_endpoint="https://auth.example.com/introspect", | |
| expected_audiences=["https://api.example.com", "https://other-api.com"], | |
| strict_audience_validation=True, | |
| ) | |
| # Token has one matching and one non-matching audience | |
| payload = { | |
| "active": True, | |
| "aud": ["https://api.example.com", "https://unrelated.com"], | |
| "sub": "user123", | |
| "exp": 1234567890, | |
| "iss": "https://auth.example.com/", | |
| } | |
| token = MCPAccessToken.from_introspection("test_token", payload) | |
| # Should succeed because at least one audience matches | |
| assert token.validate_audience(settings.expected_audiences) is True | |
| async def test_partial_audience_match(): | |
| """Test that partial audience matches are sufficient for validation.""" | |
| settings = MCPAuthorizationServerSettings( | |
| enabled=True, | |
| issuer_url="https://auth.example.com", | |
| resource_server_url="https://api.example.com", | |
| introspection_endpoint="https://auth.example.com/introspect", | |
| expected_audiences=["https://api.example.com", "https://other-api.com"], | |
| ) | |
| # Token has one matching and one non-matching audience | |
| payload = { | |
| "active": True, | |
| "aud": ["https://api.example.com", "https://unrelated.com"], | |
| "sub": "user123", | |
| "exp": 1234567890, | |
| "iss": "https://auth.example.com/", | |
| } | |
| token = MCPAccessToken.from_introspection("test_token", payload) | |
| # Should succeed because at least one audience matches | |
| assert token.validate_audience(settings.expected_audiences) is True |
🤖 Prompt for AI Agents
In tests/test_audience_validation.py around lines 228 to 250, the test
constructs MCPAuthorizationServerSettings with a non-existent parameter
strict_audience_validation=True which will trigger Pydantic validation error;
remove that parameter from the settings instantiation (or replace it with the
correct existing config field if intended) so the test uses only valid
MCPAuthorizationServerSettings fields.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
examples/oauth/preconfigured/workflow_example.py (1)
221-241: Fix focus area mapping by normalizing keys to lowercase (lookup already lowercases).Current keys like "AI/ML" never match
query_mapping.get(area.lower(), ...), so curated queries are dropped.Apply key normalization and use the same lowercase key at lookup.
- query_mapping = { - "AI/ML": [ + query_mapping = { + "ai/ml": [ "machine-learning", "artificial-intelligence", "deep-learning", "tensorflow", "pytorch", ], "cloud": ["cloud-computing", "aws", "azure", "kubernetes", "docker"], "security": ["cybersecurity", "security", "encryption", "vulnerability"], "web": ["web-development", "javascript", "react", "vue", "angular"], "mobile": ["mobile-development", "android", "ios", "react-native", "flutter"], "data": ["data-science", "analytics", "big-data", "database", "sql"], "devtools": ["developer-tools", "ci-cd", "testing", "monitoring", "automation"], } - all_queries = [] - for area in focus_areas: - queries = query_mapping.get(area.lower(), [area.lower()]) - all_queries.extend(queries) + all_queries = [] + for area in focus_areas: + key = area.lower() + queries = query_mapping.get(key, [key]) + all_queries.extend(queries)src/mcp_agent/server/app_server.py (1)
1663-1669: Guard against missing user identity when building TokenStoreKey.
app_context.current_usercan be None (e.g., headless/internal calls), causing an AttributeError oncache_key. Resolve/fallback identity before constructing the key.- # Create storage key using current user - store_key = TokenStoreKey( - user_key=app_context.current_user.cache_key, + # Resolve user identity (fallback to synthetic identity if absent) + identity = app_context.current_user + if identity is None: + try: + session_id = ctx.request_context.request.query_params.get("session_id") + except Exception: + session_id = None + identity = create_default_user_for_preconfigured_tokens(session_id) + app_context.current_user = identity + if identity is None: + raise ToolError("Unable to resolve user identity for token storage.") + + # Create storage key using resolved user identity + store_key = TokenStoreKey( + user_key=identity.cache_key, resource=resource_str, authorization_server=auth_server_str, scope_fingerprint=scope_fingerprint(scope_list), )examples/oauth/workflow_pre_auth/main.py (1)
56-60: Don’t callAnyHttpUrlas a constructor (Pydantic v2); pass plain strings.Using
AnyHttpUrl(...)raisesTypeError: 'UrlConstraints' object is not callable. Provide raw strings and let the model validate them.- authorization_server=AnyHttpUrl( - "https://github.com/login/oauth" - ), - resource=AnyHttpUrl("https://api.githubcopilot.com/mcp"), + authorization_server="https://github.com/login/oauth", + resource="https://api.githubcopilot.com/mcp",examples/oauth/dynamic_auth/main.py (1)
64-88: Fix AnyHttpUrl instantiation to prevent import crash.Lines 64, 85-86, and 88 call
AnyHttpUrl(...), which fails in Pydantic v2 becauseAnyHttpUrlis a type alias, not a constructor. This causes aTypeError: 'UrlConstraints' object is not callableat import time.Pass plain strings instead—Pydantic will coerce and validate them:
- callback_base_url=AnyHttpUrl("http://localhost:8000"), + callback_base_url="http://localhost:8000", @@ - authorization_server=AnyHttpUrl( - "https://github.com/login/oauth" - ), - resource=AnyHttpUrl("https://api.githubcopilot.com/mcp"), + authorization_server="https://github.com/login/oauth", + resource="https://api.githubcopilot.com/mcp",src/mcp_agent/oauth/manager.py (1)
159-180: Fix token-store key mismatch and add missing user validation.Lines 159-165 construct a key for globally preconfigured tokens, but the logic has three critical issues:
Resource normalization mismatch:
store_preconfigured_token(line 115) stores tokens withresource_str or "", but line 162 usesresource_strdirectly (which may beNone). This prevents lookup of preconfigured tokens whenresourceis absent.Missing session_id: Line 161 calls
create_default_user_for_preconfigured_tokens()without passingcontext.session_id, butstore_preconfigured_token(line 81) passessession_idwhen creating the synthetic user. This causes key mismatches for session-scoped preconfigured tokens.User None-check missing: Line 176 accesses
user.cache_keywithout verifyingcontext.current_userexists, causingAttributeErrorwhen no user is authenticated (e.g., stdio servers with no upstream session).Apply this diff to align key construction and guard user access:
+from mcp_agent.oauth.errors import ( + MissingUserIdentityError, + OAuthFlowError, + TokenRefreshError, +) -from mcp_agent.oauth.errors import ( - OAuthFlowError, - TokenRefreshError, -) @@ # check for a globally configure token + resource_key = resource_str or "" + global_user_key = create_default_user_for_preconfigured_tokens( + context.session_id + ).cache_key key = TokenStoreKey( - user_key=create_default_user_for_preconfigured_tokens().cache_key, - resource=resource_str, + user_key=global_user_key, + resource=resource_key, authorization_server=auth_server_str, scope_fingerprint=scope_fingerprint(scope_list), ) @@ # there is no global token, look for a user specific one + if user is None: + raise MissingUserIdentityError( + f"Cannot acquire OAuth token for '{server_name}' without an authenticated MCP user" + ) key = TokenStoreKey( user_key=user.cache_key, - resource=resource_str, + resource=resource_key, authorization_server=auth_server_str, scope_fingerprint=scope_fingerprint(scope_list), )
🧹 Nitpick comments (5)
src/mcp_agent/config.py (1)
941-958: Preferdefault_factoryover instantiated defaults for nested models.Using instances as defaults can create subtle shared-state issues. Switch to
Field(default_factory=...)forotel,logger,usage_telemetry, andagents.- otel: OpenTelemetrySettings | None = OpenTelemetrySettings() + otel: OpenTelemetrySettings | None = Field(default_factory=OpenTelemetrySettings) """OpenTelemetry logging settings for the MCP Agent application""" - logger: LoggerSettings | None = LoggerSettings() + logger: LoggerSettings | None = Field(default_factory=LoggerSettings) """Logger settings for the MCP Agent application""" - usage_telemetry: UsageTelemetrySettings | None = UsageTelemetrySettings() + usage_telemetry: UsageTelemetrySettings | None = Field(default_factory=UsageTelemetrySettings) """Usage tracking settings for the MCP Agent application""" - agents: SubagentSettings | None = SubagentSettings() + agents: SubagentSettings | None = Field(default_factory=SubagentSettings) """Settings for defining and loading subagents for the MCP Agent application"""src/mcp_agent/oauth/flow.py (1)
177-185: Optional: Support Basic auth for token exchange when servers require it.Some providers require client auth via HTTP Basic instead of (or in addition to) form fields. Consider using
auth=httpx.BasicAuth(client_id, client_secret)whenclient_secretis provided and a setting indicates Basic is required.examples/oauth/dynamic_auth/client.py (1)
147-157: Remove unused helper function.
_tool_result_to_jsonis defined but never called in this file. Unlikeworkflow_pre_auth/client.pywhich uses it to process tool results, this example directly prints the result at line 109.Apply this diff:
- -def _tool_result_to_json(tool_result: CallToolResult): - if tool_result.content and len(tool_result.content) > 0: - text = tool_result.content[0].text - try: - # Try to parse the response as JSON if it's a string - import json - - return json.loads(text) - except (json.JSONDecodeError, TypeError): - # If it's not valid JSON, just use the text - return None -examples/oauth/dynamic_auth/main.py (2)
110-110: Clean up debug print statement.The print statement has a malformed closing parenthesis:
"running activity)"should be"running activity".Apply this diff:
- print("running activity)") + print("running activity")
145-146: Avoid direct manipulation of private logger attributes.Lines 145-146 directly set
app._logger._bound_context, relying on internal implementation details. This is fragile and may break if the logger implementation changes.The logger's bound context should already be set during
app.run()(seesrc/mcp_agent/app.pylines 210-227). If you need to ensure it's current, consider adding a public method onMCPAppto refresh the logger binding, or simply remove these lines and rely on the app's lifecycle to manage the binding.If this binding is essential at tool execution time, propose adding
app.refresh_logger_context()as a public API:# In src/mcp_agent/app.py def refresh_logger_context(self) -> None: """Update logger's bound context (e.g., after context changes).""" if self._logger and hasattr(self._logger, "_bound_context"): self._logger._bound_context = self._contextThen call
app.refresh_logger_context()instead of direct attribute access.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docs/configuration.mdx(1 hunks)examples/oauth/dynamic_auth/client.py(1 hunks)examples/oauth/dynamic_auth/main.py(1 hunks)examples/oauth/preconfigured/main.py(1 hunks)examples/oauth/preconfigured/oauth_demo.py(1 hunks)examples/oauth/preconfigured/workflow_example.py(1 hunks)examples/oauth/workflow_pre_auth/client.py(1 hunks)examples/oauth/workflow_pre_auth/main.py(1 hunks)src/mcp_agent/app.py(5 hunks)src/mcp_agent/config.py(3 hunks)src/mcp_agent/oauth/flow.py(1 hunks)src/mcp_agent/oauth/manager.py(1 hunks)src/mcp_agent/server/app_server.py(9 hunks)tests/test_audience_validation.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/mcp_agent/app.py
- examples/oauth/preconfigured/main.py
- tests/test_audience_validation.py
🧰 Additional context used
🧬 Code graph analysis (8)
examples/oauth/workflow_pre_auth/main.py (4)
src/mcp_agent/core/context.py (2)
mcp(114-115)Context(62-115)src/mcp_agent/server/app_server.py (3)
app(149-151)run(2089-2094)workflows(154-156)src/mcp_agent/mcp/gen_client.py (1)
gen_client(17-49)src/mcp_agent/config.py (4)
MCPServerSettings(149-213)Settings(895-1014)MCPServerAuthSettings(29-35)MCPOAuthClientSettings(80-100)
examples/oauth/dynamic_auth/main.py (4)
src/mcp_agent/server/app_server.py (4)
app(149-151)create_mcp_server_for_app(367-1711)run(2089-2094)workflows(154-156)src/mcp_agent/app.py (2)
config(175-176)logger(211-228)src/mcp_agent/config.py (7)
MCPServerSettings(149-213)MCPSettings(216-224)MCPServerAuthSettings(29-35)MCPOAuthClientSettings(80-100)OAuthSettings(114-123)OAuthTokenStoreSettings(103-111)TemporalSettings(550-570)src/mcp_agent/mcp/gen_client.py (1)
gen_client(17-49)
src/mcp_agent/oauth/manager.py (9)
src/mcp_agent/config.py (2)
MCPOAuthClientSettings(80-100)OAuthSettings(114-123)src/mcp_agent/oauth/errors.py (2)
OAuthFlowError(4-5)TokenRefreshError(16-17)src/mcp_agent/oauth/flow.py (2)
AuthorizationFlowCoordinator(37-215)authorize(44-215)src/mcp_agent/oauth/identity.py (2)
OAuthUserIdentity(12-40)cache_key(21-23)src/mcp_agent/oauth/metadata.py (4)
fetch_authorization_server_metadata(25-31)fetch_resource_metadata(15-22)normalize_resource(55-60)select_authorization_server(34-52)src/mcp_agent/oauth/records.py (2)
TokenRecord(11-46)is_expired(26-30)src/mcp_agent/oauth/store/in_memory.py (3)
InMemoryTokenStore(12-30)get(17-22)delete(28-30)src/mcp_agent/oauth/store/base.py (5)
TokenStore(26-33)TokenStoreKey(12-18)scope_fingerprint(21-23)get(29-29)delete(33-33)src/mcp_agent/core/context.py (1)
Context(62-115)
examples/oauth/preconfigured/workflow_example.py (4)
src/mcp_agent/app.py (8)
MCPApp(56-1105)context(167-172)logger(211-228)server_registry(179-180)async_tool(899-899)async_tool(902-907)async_tool(909-957)run(492-515)src/mcp_agent/agents/agent.py (1)
Agent(62-1162)src/mcp_agent/core/context.py (2)
Context(62-115)mcp(114-115)src/mcp_agent/mcp/gen_client.py (1)
gen_client(17-49)
src/mcp_agent/server/app_server.py (10)
src/mcp_agent/core/context.py (2)
mcp(114-115)Context(62-115)src/mcp_agent/app.py (5)
config(175-176)session_id(207-208)context(167-172)upstream_session(191-192)upstream_session(195-196)src/mcp_agent/config.py (1)
MCPOAuthClientSettings(80-100)src/mcp_agent/oauth/identity.py (3)
OAuthUserIdentity(12-40)from_access_token(26-40)cache_key(21-23)src/mcp_agent/oauth/errors.py (1)
CallbackTimeoutError(12-13)src/mcp_agent/oauth/manager.py (2)
create_default_user_for_preconfigured_tokens(41-56)aclose(383-385)src/mcp_agent/server/token_verifier.py (2)
MCPAgentTokenVerifier(21-200)aclose(193-194)src/mcp_agent/oauth/store/base.py (3)
get(29-29)TokenStoreKey(12-18)scope_fingerprint(21-23)src/mcp_agent/oauth/callbacks.py (2)
deliver(26-35)create_handle(16-24)src/mcp_agent/oauth/records.py (1)
TokenRecord(11-46)
examples/oauth/workflow_pre_auth/client.py (6)
src/mcp_agent/config.py (1)
MCPServerSettings(149-213)src/mcp_agent/mcp/gen_client.py (1)
gen_client(17-49)src/mcp_agent/mcp/mcp_agent_client_session.py (1)
MCPAgentClientSession(73-428)src/mcp_agent/human_input/console_handler.py (1)
console_input_callback(68-78)src/mcp_agent/elicitation/handler.py (1)
console_elicitation_callback(138-167)examples/oauth/dynamic_auth/client.py (5)
on_server_log(51-54)ConsolePrintingClientSession(58-73)_received_notification(59-73)make_session(75-87)_tool_result_to_json(147-157)
examples/oauth/dynamic_auth/client.py (7)
src/mcp_agent/core/context.py (2)
mcp(114-115)Context(62-115)src/mcp_agent/config.py (1)
MCPServerSettings(149-213)src/mcp_agent/mcp/gen_client.py (1)
gen_client(17-49)src/mcp_agent/mcp/mcp_agent_client_session.py (1)
MCPAgentClientSession(73-428)src/mcp_agent/human_input/console_handler.py (1)
console_input_callback(68-78)src/mcp_agent/elicitation/handler.py (1)
console_elicitation_callback(138-167)examples/oauth/workflow_pre_auth/client.py (6)
main(49-182)on_server_log(74-77)ConsolePrintingClientSession(81-96)_received_notification(82-96)make_session(98-110)_tool_result_to_json(185-195)
src/mcp_agent/oauth/flow.py (6)
src/mcp_agent/core/context.py (2)
mcp(114-115)Context(62-115)src/mcp_agent/config.py (2)
MCPOAuthClientSettings(80-100)OAuthSettings(114-123)src/mcp_agent/oauth/errors.py (4)
AuthorizationDeclined(8-9)CallbackTimeoutError(12-13)MissingUserIdentityError(20-21)OAuthFlowError(4-5)src/mcp_agent/oauth/pkce.py (3)
generate_code_challenge(21-23)generate_code_verifier(10-18)generate_state(26-27)src/mcp_agent/oauth/records.py (1)
TokenRecord(11-46)src/mcp_agent/oauth/callbacks.py (2)
create_handle(16-24)discard(46-50)
⏰ 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: update_release_draft
🔇 Additional comments (1)
examples/oauth/preconfigured/oauth_demo.py (1)
1-399: Demo looks good.End-to-end flow is clear and self-contained. No blocking issues.
| ```yaml | ||
| authorization: | ||
| enabled: true | ||
| issuer_url: https://auth.example.com | ||
| resource_server_url: https://agent.example.com/mcp | ||
| required_scopes: ["mcp.read", "mcp.write"] | ||
| introspection_endpoint: https://auth.example.com/oauth/introspect | ||
| introspection_client_id: ${INTROSPECTION_CLIENT_ID} | ||
| introspection_client_secret: ${INTROSPECTION_CLIENT_SECRET} | ||
| oauth: | ||
| callback_base_url: https://agent.example.com | ||
| flow_timeout_seconds: 180 | ||
| token_store: | ||
| backend: memory # use "redis" for multi-instance deployments | ||
| refresh_leeway_seconds: 90 | ||
| mcp: | ||
| servers: | ||
| github: | ||
| transport: streamable_http | ||
| url: https://github.mcp.example.com/mcp | ||
| auth: | ||
| oauth: | ||
| enabled: true | ||
| scopes: ["repo", "user:email"] | ||
| client_id: ${GITHUB_MCP_CLIENT_ID} | ||
| client_secret: ${GITHUB_MCP_CLIENT_SECRET} | ||
| redirect_uri_options: | ||
| - https://agent.example.com/internal/oauth/callback | ||
| ``` | ||
|
|
||
| - When `authorization.enabled` is true the MCP server advertises `/.well-known/oauth-protected-resource` and enforces bearer tokens using the provided introspection or JWKS configuration. | ||
| - `oauth` enables delegated authorization flows; the default in-memory token store is ideal for local development while Redis is recommended for production clusters. | ||
| - Downstream servers opt into OAuth via `mcp.servers.<name>.auth.oauth`. Supplying a `client_id`/`client_secret` allows immediate usage; support for dynamic client registration is planned as a follow-up. | ||
|
|
There was a problem hiding this comment.
Add required expected_audiences to the Authorization example (validation fails without it).
MCPAuthorizationServerSettings enforces expected_audiences when authorization.enabled: true. The example omits it and will fail validation.
Add it to the YAML and call it out in the text.
authorization:
enabled: true
issuer_url: https://auth.example.com
resource_server_url: https://agent.example.com/mcp
+ expected_audiences: ["mcp-agent"] # required for RFC 9068 audience validation
required_scopes: ["mcp.read", "mcp.write"]
introspection_endpoint: https://auth.example.com/oauth/introspect
introspection_client_id: ${INTROSPECTION_CLIENT_ID}
introspection_client_secret: ${INTROSPECTION_CLIENT_SECRET}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```yaml | |
| authorization: | |
| enabled: true | |
| issuer_url: https://auth.example.com | |
| resource_server_url: https://agent.example.com/mcp | |
| required_scopes: ["mcp.read", "mcp.write"] | |
| introspection_endpoint: https://auth.example.com/oauth/introspect | |
| introspection_client_id: ${INTROSPECTION_CLIENT_ID} | |
| introspection_client_secret: ${INTROSPECTION_CLIENT_SECRET} | |
| oauth: | |
| callback_base_url: https://agent.example.com | |
| flow_timeout_seconds: 180 | |
| token_store: | |
| backend: memory # use "redis" for multi-instance deployments | |
| refresh_leeway_seconds: 90 | |
| mcp: | |
| servers: | |
| github: | |
| transport: streamable_http | |
| url: https://github.mcp.example.com/mcp | |
| auth: | |
| oauth: | |
| enabled: true | |
| scopes: ["repo", "user:email"] | |
| client_id: ${GITHUB_MCP_CLIENT_ID} | |
| client_secret: ${GITHUB_MCP_CLIENT_SECRET} | |
| redirect_uri_options: | |
| - https://agent.example.com/internal/oauth/callback | |
| ``` | |
| - When `authorization.enabled` is true the MCP server advertises `/.well-known/oauth-protected-resource` and enforces bearer tokens using the provided introspection or JWKS configuration. | |
| - `oauth` enables delegated authorization flows; the default in-memory token store is ideal for local development while Redis is recommended for production clusters. | |
| - Downstream servers opt into OAuth via `mcp.servers.<name>.auth.oauth`. Supplying a `client_id`/`client_secret` allows immediate usage; support for dynamic client registration is planned as a follow-up. |
🤖 Prompt for AI Agents
In docs/configuration.mdx around lines 133 to 168, the authorization example
omits the required expected_audiences field which causes
MCPAuthorizationServerSettings validation to fail when authorization.enabled:
true; add an expected_audiences: [... ] entry under the authorization block
(e.g. a list containing the resource server audience(s) such as the mcp resource
server URL or its audience claim) and update the explanatory text below the YAML
to call out that expected_audiences must be set when authorization is enabled
and should match the token "aud" claim used by the authorization server.
| import asyncio | ||
| import time | ||
|
|
||
| from datetime import timedelta | ||
| from anyio.streams.memory import MemoryObjectReceiveStream, MemoryObjectSendStream | ||
| from mcp import ClientSession | ||
| from mcp.types import CallToolResult, LoggingMessageNotificationParams | ||
| from mcp_agent.app import MCPApp | ||
| from mcp_agent.config import MCPServerSettings | ||
| from mcp_agent.core.context import Context | ||
| from mcp_agent.mcp.gen_client import gen_client | ||
| from mcp_agent.mcp.mcp_agent_client_session import MCPAgentClientSession | ||
| from mcp_agent.human_input.console_handler import console_input_callback | ||
| from mcp_agent.elicitation.handler import console_elicitation_callback | ||
|
|
||
| from rich import print | ||
|
|
||
| try: | ||
| from exceptiongroup import ExceptionGroup as _ExceptionGroup # Python 3.10 backport | ||
| except Exception: # pragma: no cover | ||
| _ExceptionGroup = None # type: ignore | ||
| try: | ||
| from anyio import BrokenResourceError as _BrokenResourceError | ||
| except Exception: # pragma: no cover | ||
| _BrokenResourceError = None # type: ignore | ||
|
|
||
|
|
||
| async def main(): | ||
| # Create MCPApp to get the server registry | ||
| app = MCPApp( | ||
| name="workflow_mcp_client", | ||
| human_input_callback=console_input_callback, | ||
| elicitation_callback=console_elicitation_callback, | ||
| ) | ||
| async with app.run() as client_app: | ||
| logger = client_app.logger | ||
| context = client_app.context | ||
|
|
||
| # Connect to the workflow server | ||
| logger.info("Connecting to workflow server...") | ||
|
|
||
| # Override the server configuration to point to our local script | ||
| context.server_registry.registry["basic_agent_server"] = MCPServerSettings( | ||
| name="basic_agent_server", | ||
| description="Local workflow server running the basic agent example", | ||
| transport="sse", | ||
| url="http://127.0.0.1:8000/sse", | ||
| ) | ||
|
|
||
| # Define a logging callback to receive server-side log notifications | ||
| async def on_server_log(params: LoggingMessageNotificationParams) -> None: | ||
| level = params.level.upper() | ||
| name = params.logger or "server" | ||
| print(f"[SERVER LOG] [{level}] [{name}] {params.data}") | ||
|
|
||
| # Provide a client session factory that installs our logging callback | ||
| # and prints non-logging notifications to the console | ||
| class ConsolePrintingClientSession(MCPAgentClientSession): | ||
| async def _received_notification(self, notification): # type: ignore[override] | ||
| try: | ||
| method = getattr(notification.root, "method", None) | ||
| except Exception: | ||
| method = None | ||
|
|
||
| # Avoid duplicating server log prints (handled by logging_callback) | ||
| if method and method != "notifications/message": | ||
| try: | ||
| data = notification.model_dump() | ||
| except Exception: | ||
| data = str(notification) | ||
| print(f"[SERVER NOTIFY] {method}: {data}") | ||
|
|
||
| return await super()._received_notification(notification) | ||
|
|
||
| def make_session( | ||
| read_stream: MemoryObjectReceiveStream, | ||
| write_stream: MemoryObjectSendStream, | ||
| read_timeout_seconds: timedelta | None, | ||
| context: Context | None = None, | ||
| ) -> ClientSession: | ||
| return ConsolePrintingClientSession( | ||
| read_stream=read_stream, | ||
| write_stream=write_stream, | ||
| read_timeout_seconds=read_timeout_seconds, | ||
| logging_callback=on_server_log, | ||
| context=context, | ||
| ) | ||
|
|
||
| try: | ||
| async with gen_client( | ||
| "basic_agent_server", | ||
| context.server_registry, | ||
| client_session_factory=make_session, | ||
| ) as server: | ||
| try: | ||
| await server.set_logging_level("info") | ||
| except Exception: | ||
| # Older servers may not support logging capability | ||
| print("[client] Server does not support logging/setLevel") | ||
|
|
||
| # List available tools | ||
| tools_result = await server.list_tools() | ||
| logger.info( | ||
| "Available tools:", | ||
| data={"tools": [tool.name for tool in tools_result.tools]}, | ||
| ) | ||
|
|
||
| print( | ||
| await server.call_tool("github_org_search", {"query": "lastmileai"}) | ||
| ) | ||
| except Exception as e: | ||
| # Tolerate benign shutdown races from stdio client (BrokenResourceError within ExceptionGroup) | ||
| if _ExceptionGroup is not None and isinstance(e, _ExceptionGroup): | ||
| subs = getattr(e, "exceptions", []) or [] | ||
| if ( | ||
| _BrokenResourceError is not None | ||
| and subs | ||
| and all(isinstance(se, _BrokenResourceError) for se in subs) | ||
| ): | ||
| logger.debug("Ignored BrokenResourceError from stdio shutdown") | ||
| else: | ||
| raise | ||
| elif _BrokenResourceError is not None and isinstance( | ||
| e, _BrokenResourceError | ||
| ): | ||
| logger.debug("Ignored BrokenResourceError from stdio shutdown") | ||
| elif "BrokenResourceError" in str(e): | ||
| logger.debug( | ||
| "Ignored BrokenResourceError from stdio shutdown (string match)" | ||
| ) | ||
| else: | ||
| raise | ||
| # Nudge cleanup of subprocess transports before the loop closes to avoid | ||
| # 'Event loop is closed' from BaseSubprocessTransport.__del__ on GC. | ||
| try: | ||
| await asyncio.sleep(0) | ||
| except Exception: | ||
| pass | ||
| try: | ||
| import gc | ||
|
|
||
| gc.collect() | ||
| except Exception: | ||
| pass | ||
|
|
||
|
|
||
| def _tool_result_to_json(tool_result: CallToolResult): | ||
| if tool_result.content and len(tool_result.content) > 0: | ||
| text = tool_result.content[0].text | ||
| try: | ||
| # Try to parse the response as JSON if it's a string | ||
| import json | ||
|
|
||
| return json.loads(text) | ||
| except (json.JSONDecodeError, TypeError): | ||
| # If it's not valid JSON, just use the text | ||
| return None | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| start = time.time() | ||
| asyncio.run(main()) | ||
| end = time.time() | ||
| t = end - start | ||
|
|
||
| print(f"Total run time: {t:.2f}s") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract shared client logic to eliminate duplication.
This file duplicates nearly all of examples/oauth/workflow_pre_auth/client.py (only the tool invocation at line 109 differs). Both define identical ConsolePrintingClientSession, make_session, exception handling, and cleanup logic.
Extract the common client scaffolding into a shared module (e.g., examples/oauth/common/client_utils.py) and reuse it in both examples. This keeps the examples focused on their unique OAuth patterns (dynamic_auth vs. workflow_pre_auth) rather than repeating boilerplate.
Example refactor:
Create examples/oauth/common/client_utils.py:
from mcp_agent.mcp.mcp_agent_client_session import MCPAgentClientSession
class ConsolePrintingClientSession(MCPAgentClientSession):
async def _received_notification(self, notification):
# ... (current implementation)
pass
def make_session_factory(on_server_log):
def make_session(read_stream, write_stream, read_timeout_seconds, context=None):
return ConsolePrintingClientSession(
read_stream=read_stream,
write_stream=write_stream,
read_timeout_seconds=read_timeout_seconds,
logging_callback=on_server_log,
context=context,
)
return make_sessionThen simplify both example clients to import and use these utilities.
| print( | ||
| "\nGitHub client id and/or secret not found in GITHUB_CLIENT_Id and GITHUB_CLIENT_SECRET " |
There was a problem hiding this comment.
Fix typo in environment variable name.
Line 43 references GITHUB_CLIENT_Id (lowercase 'd') but should be GITHUB_CLIENT_ID (uppercase 'D') to match the actual variable name at line 38.
Apply this diff:
- "\nGitHub client id and/or secret not found in GITHUB_CLIENT_Id and GITHUB_CLIENT_SECRET "
+ "\nGitHub client id and/or secret not found in GITHUB_CLIENT_ID and GITHUB_CLIENT_SECRET "📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print( | |
| "\nGitHub client id and/or secret not found in GITHUB_CLIENT_Id and GITHUB_CLIENT_SECRET " | |
| print( | |
| "\nGitHub client id and/or secret not found in GITHUB_CLIENT_ID and GITHUB_CLIENT_SECRET " | |
| ) |
🤖 Prompt for AI Agents
In examples/oauth/dynamic_auth/main.py around lines 42-43, the printed
environment variable name contains a typo: it uses GITHUB_CLIENT_Id (lowercase
'd') but should be GITHUB_CLIENT_ID; update the string to reference
GITHUB_CLIENT_ID so it matches the actual environment variable used earlier and
any checks that depend on that exact name.
| # Get GitHub access token from environment or ask user | ||
| access_token = os.getenv("GITHUB_ACCESS_TOKEN") | ||
|
|
||
| if not access_token: | ||
| print("\nGitHub access token not found in environment variable GITHUB_ACCESS_TOKEN") | ||
| print("\nTo get a GitHub access token:") | ||
| print("1. Run the oauth_demo.py script from examples/oauth/ to get a fresh token") | ||
| print("2. Or go to GitHub Settings > Developer settings > Personal access tokens") | ||
| print("3. Create a token with 'read:org' and 'public_repo' scopes") | ||
| print("\nThen set the token:") | ||
| print("export GITHUB_ACCESS_TOKEN='your_token_here'") | ||
|
|
||
| # Verify token format | ||
| if not access_token.startswith(("gho_", "ghp_", "github_pat_")): | ||
| print( | ||
| f"Warning: Token doesn't look like a GitHub token (got: {access_token[:10]}...)" | ||
| ) | ||
| print("GitHub tokens usually start with 'gho_', 'ghp_', or 'github_pat_'") | ||
|
|
There was a problem hiding this comment.
Guard against missing GITHUB_ACCESS_TOKEN; avoid None.startswith crash.
When the env var is absent, the script continues and calls access_token.startswith(...), raising an exception.
Add an early return after guidance and only validate format when non-empty.
access_token = os.getenv("GITHUB_ACCESS_TOKEN")
if not access_token:
print("\nGitHub access token not found in environment variable GITHUB_ACCESS_TOKEN")
print("\nTo get a GitHub access token:")
print("1. Run the oauth_demo.py script from examples/oauth/ to get a fresh token")
print("2. Or go to GitHub Settings > Developer settings > Personal access tokens")
print("3. Create a token with 'read:org' and 'public_repo' scopes")
print("\nThen set the token:")
print("export GITHUB_ACCESS_TOKEN='your_token_here'")
+ return # Exit early; nothing to do without a token
-# Verify token format
-if not access_token.startswith(("gho_", "ghp_", "github_pat_")):
+# Verify token format (best-effort)
+if access_token and not access_token.startswith(("gho_", "ghp_", "github_pat_")):
- print(
- f"Warning: Token doesn't look like a GitHub token (got: {access_token[:10]}...)"
- )
+ print("Warning: Token doesn't look like a GitHub token")
print("GitHub tokens usually start with 'gho_', 'ghp_', or 'github_pat_'")Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
examples/oauth/workflow_pre_auth/client.py lines 29-47: the code prints guidance
when GITHUB_ACCESS_TOKEN is missing but then continues and calls
access_token.startswith(...) which raises an AttributeError if access_token is
None; change the flow to exit early after printing guidance (e.g., return from
function or call sys.exit(1)) when access_token is falsy, and only perform the
startswith format check inside the branch where access_token is present so
validation runs only on a non-empty string.
| # Need to run full authorization flow - only if no token found or refresh failed | ||
| if not record: | ||
| resource_hint = ( | ||
| str(oauth_config.resource) | ||
| if oauth_config.resource | ||
| else getattr(server_config, "url", None) | ||
| ) | ||
| server_url = getattr(server_config, "url", None) | ||
| resource = normalize_resource(resource_hint, server_url) | ||
|
|
||
| # Get OAuth metadata for full authorization flow | ||
| parsed_resource = URL(resource) | ||
| metadata_url = str( | ||
| parsed_resource.copy_with( | ||
| path="/.well-known/oauth-protected-resource" | ||
| + parsed_resource.path | ||
| ) | ||
| ) | ||
| resource_metadata = await self._get_resource_metadata(metadata_url) | ||
| auth_server_url = select_authorization_server( | ||
| resource_metadata, str(oauth_config.authorization_server) | ||
| ) | ||
| auth_metadata = await self._get_authorization_metadata(auth_server_url) | ||
|
|
||
| record = await self._flow.authorize( | ||
| context=context, | ||
| user=user, | ||
| server_name=server_name, | ||
| oauth_config=oauth_config, | ||
| resource=resource, | ||
| authorization_server_url=auth_server_url, | ||
| resource_metadata=resource_metadata, | ||
| auth_metadata=auth_metadata, | ||
| scopes=scope_list, | ||
| ) | ||
| await self._token_store.set(key, record) | ||
| return record |
There was a problem hiding this comment.
Authorization flow skipped when refresh fails.
Line 235's condition if not record: only triggers the full OAuth flow when no token exists in the store. However, if token refresh fails at lines 226-232 (catching TokenRefreshError), the code deletes the token (line 227) but record still refers to the original expired token object. Execution skips the OAuth flow block (lines 236-270) and returns the expired token at line 274.
Result: After a failed refresh, the caller receives an expired token instead of obtaining a fresh one via interactive authorization.
Restructure the logic to re-run authorization after refresh failure. One approach:
try:
refreshed = await self._refresh_token(
record,
oauth_config=oauth_config,
auth_metadata=auth_metadata,
resource=resource,
scopes=scope_list,
)
except TokenRefreshError:
await self._token_store.delete(key)
+ record = None # Mark record as invalid to trigger full flow below
else:
if refreshed:
await self._token_store.set(key, refreshed)
return refreshed
await self._token_store.delete(key)
+ record = None # Mark record as invalid to trigger full flow below
# Need to run full authorization flow - only if no token found or refresh failed
if not record:This ensures that after any refresh failure or unsuccessful refresh, the code proceeds to the interactive authorization flow instead of returning a stale token.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Need to run full authorization flow - only if no token found or refresh failed | |
| if not record: | |
| resource_hint = ( | |
| str(oauth_config.resource) | |
| if oauth_config.resource | |
| else getattr(server_config, "url", None) | |
| ) | |
| server_url = getattr(server_config, "url", None) | |
| resource = normalize_resource(resource_hint, server_url) | |
| # Get OAuth metadata for full authorization flow | |
| parsed_resource = URL(resource) | |
| metadata_url = str( | |
| parsed_resource.copy_with( | |
| path="/.well-known/oauth-protected-resource" | |
| + parsed_resource.path | |
| ) | |
| ) | |
| resource_metadata = await self._get_resource_metadata(metadata_url) | |
| auth_server_url = select_authorization_server( | |
| resource_metadata, str(oauth_config.authorization_server) | |
| ) | |
| auth_metadata = await self._get_authorization_metadata(auth_server_url) | |
| record = await self._flow.authorize( | |
| context=context, | |
| user=user, | |
| server_name=server_name, | |
| oauth_config=oauth_config, | |
| resource=resource, | |
| authorization_server_url=auth_server_url, | |
| resource_metadata=resource_metadata, | |
| auth_metadata=auth_metadata, | |
| scopes=scope_list, | |
| ) | |
| await self._token_store.set(key, record) | |
| return record | |
| try: | |
| refreshed = await self._refresh_token( | |
| record, | |
| oauth_config=oauth_config, | |
| auth_metadata=auth_metadata, | |
| resource=resource, | |
| scopes=scope_list, | |
| ) | |
| except TokenRefreshError: | |
| await self._token_store.delete(key) | |
| record = None # Mark record as invalid to trigger full flow below | |
| else: | |
| if refreshed: | |
| await self._token_store.set(key, refreshed) | |
| return refreshed | |
| await self._token_store.delete(key) | |
| record = None # Mark record as invalid to trigger full flow below | |
| # Need to run full authorization flow - only if no token found or refresh failed | |
| if not record: | |
| resource_hint = ( | |
| str(oauth_config.resource) | |
| if oauth_config.resource | |
| else getattr(server_config, "url", None) | |
| ) | |
| server_url = getattr(server_config, "url", None) | |
| resource = normalize_resource(resource_hint, server_url) | |
| # Get OAuth metadata for full authorization flow | |
| parsed_resource = URL(resource) | |
| metadata_url = str( | |
| parsed_resource.copy_with( | |
| path="/.well-known/oauth-protected-resource" | |
| parsed_resource.path | |
| ) | |
| ) | |
| resource_metadata = await self._get_resource_metadata(metadata_url) | |
| auth_server_url = select_authorization_server( | |
| resource_metadata, str(oauth_config.authorization_server) | |
| ) | |
| auth_metadata = await self._get_authorization_metadata(auth_server_url) | |
| record = await self._flow.authorize( | |
| context=context, | |
| user=user, | |
| server_name=server_name, | |
| oauth_config=oauth_config, | |
| resource=resource, | |
| authorization_server_url=auth_server_url, | |
| resource_metadata=resource_metadata, | |
| auth_metadata=auth_metadata, | |
| scopes=scope_list, | |
| ) | |
| await self._token_store.set(key, record) | |
| return record |
🤖 Prompt for AI Agents
In src/mcp_agent/oauth/manager.py around lines 234 to 270, after a
TokenRefreshError the code deletes the stored token but leaves the local
variable record populated so the subsequent "if not record" block is skipped and
an expired token is returned; modify the error handling so that after deleting
the token you also clear the local record (e.g., set record = None or a falsy
flag) or set a force_authorize flag, then let execution continue so the full
authorization flow runs and a fresh record is obtained and stored before
returning.
| token_record = TokenRecord( | ||
| access_token=access_token, | ||
| refresh_token=token_data.get("refresh_token"), | ||
| scopes=tuple(scope_list), | ||
| expires_at=token_data.get("expires_at"), | ||
| token_type=token_data.get("token_type", "Bearer"), | ||
| resource=server_name, | ||
| authorization_server=auth_server_str, | ||
| metadata={"workflow_name": workflow_name}, | ||
| ) | ||
|
|
||
| str(oauth_config.resource) if oauth_config.resource else getattr( | ||
| server_config, "url", None | ||
| ) | ||
| # Create storage key using current user |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Align stored TokenRecord.resource with the computed resource; remove dead code.
TokenRecord.resource is set to server_name while the storage key uses resource_str (URL/URN). Also, stray code at Lines 1660-1663 is dead.
Use resource_str for consistency and delete the leftover lines.
- token_record = TokenRecord(
+ token_record = TokenRecord(
access_token=access_token,
refresh_token=token_data.get("refresh_token"),
scopes=tuple(scope_list),
expires_at=token_data.get("expires_at"),
token_type=token_data.get("token_type", "Bearer"),
- resource=server_name,
+ resource=resource_str,
authorization_server=auth_server_str,
metadata={"workflow_name": workflow_name},
)
-
- str(oauth_config.resource) if oauth_config.resource else getattr(
- server_config, "url", None
- )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| token_record = TokenRecord( | |
| access_token=access_token, | |
| refresh_token=token_data.get("refresh_token"), | |
| scopes=tuple(scope_list), | |
| expires_at=token_data.get("expires_at"), | |
| token_type=token_data.get("token_type", "Bearer"), | |
| resource=server_name, | |
| authorization_server=auth_server_str, | |
| metadata={"workflow_name": workflow_name}, | |
| ) | |
| str(oauth_config.resource) if oauth_config.resource else getattr( | |
| server_config, "url", None | |
| ) | |
| # Create storage key using current user | |
| token_record = TokenRecord( | |
| access_token=access_token, | |
| refresh_token=token_data.get("refresh_token"), | |
| scopes=tuple(scope_list), | |
| expires_at=token_data.get("expires_at"), | |
| token_type=token_data.get("token_type", "Bearer"), | |
| resource=resource_str, | |
| authorization_server=auth_server_str, | |
| metadata={"workflow_name": workflow_name}, | |
| ) | |
| # Create storage key using current user |
🤖 Prompt for AI Agents
In src/mcp_agent/server/app_server.py around lines 1649 to 1663,
TokenRecord.resource is being set to server_name while the storage key is built
from resource_str; replace server_name with the computed resource_str so the
stored TokenRecord.resource matches the storage key, and remove the dead stray
lines that call str(oauth_config.resource)... getattr(server_config, "url",
None) (lines 1660-1663) since they have no effect.
2 participants
Summary by CodeRabbit
New Features
Documentation
Examples
Tests