PR Review: OAuth Implementation for MCP (roboflow-mcp #51)

roboflow-mcp-51-20260525-144953.md

PR Review: OAuth Implementation for MCP

Repository: roboflow/roboflow-mcp PR: #51 — OAuth Implementation for MCP Branch: mike/mcp-oauth → main Author: Mike-Medvedev (Michael Medvedev) Review Date: 2026-05-25 Generation Timestamp: 2026-05-25 14:49:53 America/Sao_Paulo Base Commit: e79d8aeb05cb7c2703dd62d572e48d53df058d1f Head Commit: ded89e9a0c784231df5392b911bbbfc5873bb06c Changed Files: 36 Additions / Deletions: +1464 / -323 PR URL: https://github.com/roboflow/roboflow-mcp/pull/51

DISCLAIMER: Generated by Codex.

📋 Summary

This revision is a substantial improvement over the May 22 review. All six previous findings are resolved:

#	Previous finding	Status
1	Uncached duplicate workspace lookups for OAuth	Resolved — custom CredentialWorkspaceCache with TTL + singleflight
2	Missing call_tool enforcement for API-key-only tools	Resolved — _raise_if_oauth_call_requires_api_key guard in middleware
3	Over-broad model:infer scope in OAuth consent	Resolved — removed from scopes; replaced with model:manage
4	Blank Bearer header bypass at HTTP gate	Resolved — _has_mcp_credential now delegates to _bearer_token()
5	Private async_lru internal test reset hack	Resolved — async_lru dependency removed entirely
6	Unused _access_token parameter	Resolved — parameter removed

The PR now implements a complete MCP OAuth 2.1 flow with proper credential caching, execution-time tool enforcement, and clean separation between auth transport, credential policy, and tool business logic. The async_lru dependency is gone, replaced by a purpose-built CredentialWorkspaceCache that handles both API key and OAuth credentials with singleflight deduplication.

🏗️ Architectural Review

The PR splits auth concerns into three well-bounded layers: the HTTP challenge middleware (MCPAuthChallengeMiddleware) gates unauthenticated requests before FastMCP sees them, a credential extraction layer (auth/auth.py) normalizes Bearer tokens and API keys into a single get_credential() call, and a tool-level policy layer (oauth_compatibility.py) classifies tools as OAuth-compatible or API-key-only. The middleware enforces the policy at both on_list_tools (catalog filtering) and on_call_tool (execution blocking).

The new CredentialWorkspaceCache is a well-designed replacement for async_lru. It hashes credentials before storing (so raw tokens never sit in memory as dict keys), applies a 5-minute TTL only to OAuth entries (API keys remain unbounded), and coalesces concurrent lookups for the same credential via singleflight. This is the right shape for a server that mixes short-lived OAuth tokens with long-lived API keys.

The logging layer is now lighter: _request_log_fields() no longer resolves workspace on every tool call, logging only credential_type instead. Workspace is still resolved during on_initialize, which provides enough context for correlation. This eliminates the redundant workspace lookup that the previous review identified.

flowchart LR
    Client["MCP client"] --> Challenge["MCPAuthChallengeMiddleware"]
    Challenge -->|"no credential → 401 + RFC 9728 metadata"| Client
    Challenge -->|"credential present"| FastMCP["FastMCP"]
    FastMCP --> ListTools["on_list_tools"]
    ListTools --> CatalogPolicy["OAuth compatibility policy"]
    CatalogPolicy -->|"filter API-key-only tools"| Client
    FastMCP --> CallTool["on_call_tool"]
    CallTool --> Guard["_raise_if_oauth_call_requires_api_key"]
    Guard -->|"403 if blocked"| Client
    Guard -->|"allowed"| ToolHandler["Tool handler"]
    ToolHandler --> WSCache["CredentialWorkspaceCache"]
    WSCache -->|"cache hit"| ToolHandler
    WSCache -->|"cache miss + singleflight"| Platform["Roboflow Platform API"]
    ToolHandler --> Platform

Loading

The credential_params / api_headers helpers in client.py correctly route OAuth tokens into Authorization: Bearer headers while keeping API keys as ?api_key= query params. This dual-format adapter is applied uniformly across api_get, api_post, api_patch, api_post_multipart, and the rf_api.py functions that build their own requests.

🐛 Code Quality Issues

1. 🤨 Missing blank line between top-level functions in middleware.py

src/roboflow_mcp/middleware.py:160-161 has _initialize_log_fields ending immediately followed by _request_log_fields with no blank line separator:

    return fields
async def _request_log_fields() -> dict[str, object]:

PEP 8 requires two blank lines between top-level function definitions. This is purely cosmetic but will flag in linters.

2. 🤨 Tool call logs lost workspace context

src/roboflow_mcp/middleware.py:161-182 — _request_log_fields() now logs credential_type but no longer resolves workspace. This is a deliberate trade-off to eliminate the duplicate uncached lookup from the previous version. Workspace is still logged during on_initialize, so it's available for session-level correlation.

If per-tool-call workspace context matters for production debugging, a lighter approach would be to read the workspace from the cache without triggering a fetch — CredentialWorkspaceCache could expose a peek() that returns the cached value or None, avoiding any network call while still enriching tool call logs when the data is already warm.

3. 🤨 Stale singleflight task references after caller cancellation

src/roboflow_mcp/workspace_cache.py:72-76 — If all callers awaiting a singleflight task are cancelled before the task completes, the task finishes and stores its result in cache, but the completed Task object stays in _inflight forever. The next caller finds the cached result and returns early without cleaning up _inflight.

This is a minor memory leak bounded by the number of distinct credential hashes. In practice it's inconsequential — each stale entry is just a reference to a completed asyncio.Task — but it could be cleaned up by having _fetch_and_store remove itself from _inflight on completion.

💡 Recommendations

• Fix the missing blank line in middleware.py between _initialize_log_fields and _request_log_fields.
• Consider adding a cache-only peek() to CredentialWorkspaceCache so tool-call logging can include workspace when it's already cached without triggering a network call.
• Have _fetch_and_store clean up its own _inflight entry in a finally block, so stale tasks don't linger when all callers are cancelled.

All three are low-priority follow-ups, not merge blockers.

🧪 Tests

Test coverage is solid for this revision:

• tests/test_oauth_auth.py (286 lines) covers protected-resource metadata, Bearer challenge, blank Bearer rejection, credential precedence, OAuth workspace lookup using Bearer header, API-key workspace lookup using query param, and api_get credential formatting for both auth types.
• tests/test_tool_catalog.py adds tests for OAuth catalog filtering, API-key full catalog, execution-time blocking of API-key-only tools via OAuth, tool classification completeness, and scope set validation.
• tests/test_client_cache.py adds 6 new tests covering OAuth token caching with TTL, hashed cache keys, LRU eviction, TTL expiration, singleflight deduplication, and end-to-end singleflight through the client module.
• tests/conftest.py removes the brittle async_lru internals hack and replaces it with a clean cache_clear() call.

The previous review's three test gaps are all addressed:

• OAuth call to API-key-only tool → test_oauth_call_to_api_key_only_tool_is_blocked
• Blank Bearer header → test_auth_challenge_middleware_rejects_blank_bearer_token
• Workspace lookup count → singleflight tests verify deduplication

⚖️ Verdict

✅ Approve

All six issues from the previous review are resolved. The OAuth flow is well-structured, the credential cache is robust, the tool surface enforcement is both advertised and executed, and the scope set matches the reachable OAuth functionality. The remaining findings are cosmetic or low-priority follow-ups.