-
-
Notifications
You must be signed in to change notification settings - Fork 926
fix(cli): complete OAuth auth flow for CLI mode #505
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
ecf3690
686f661
7c5082a
0763bdf
637f561
4f30ee4
bfd1bee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,6 +43,10 @@ def __init__(self, port: int = 8000, base_uri: str = "http://localhost"): | |
| self.server_thread = None | ||
| self.is_running = False | ||
|
|
||
| # CLI auth completion signaling | ||
| self.auth_completed = threading.Event() | ||
| self.auth_result: Optional[dict] = None # {"success": bool, "user_id": str|None, "error": str|None} | ||
|
|
||
| # Setup the callback route | ||
| self._setup_callback_route() | ||
| # Setup attachment serving route | ||
|
|
@@ -62,19 +66,25 @@ async def oauth_callback(request: Request): | |
| f"Authentication failed: Google returned an error: {error}." | ||
| ) | ||
| logger.error(error_message) | ||
| self.auth_result = {"success": False, "user_id": None, "error": error_message} | ||
| self.auth_completed.set() | ||
| return create_error_response(error_message) | ||
|
|
||
| if not code: | ||
| error_message = ( | ||
| "Authentication failed: No authorization code received from Google." | ||
| ) | ||
| logger.error(error_message) | ||
| self.auth_result = {"success": False, "user_id": None, "error": error_message} | ||
| self.auth_completed.set() | ||
| return create_error_response(error_message) | ||
|
|
||
| try: | ||
| # Check if we have credentials available (environment variables or file) | ||
| error_message = check_client_secrets() | ||
| if error_message: | ||
| self.auth_result = {"success": False, "user_id": None, "error": error_message} | ||
| self.auth_completed.set() | ||
| return create_server_error_response(error_message) | ||
|
|
||
| logger.info( | ||
|
|
@@ -96,12 +106,18 @@ async def oauth_callback(request: Request): | |
| f"OAuth callback: Successfully authenticated user: {verified_user_id}." | ||
| ) | ||
|
|
||
| # Signal completion for CLI auth flow | ||
| self.auth_result = {"success": True, "user_id": verified_user_id, "error": None} | ||
| self.auth_completed.set() | ||
|
|
||
| # Return success page using shared template | ||
| return create_success_response(verified_user_id) | ||
|
|
||
| except Exception as e: | ||
| error_message_detail = f"Error processing OAuth callback: {str(e)}" | ||
| logger.error(error_message_detail, exc_info=True) | ||
| self.auth_result = {"success": False, "user_id": None, "error": str(e)} | ||
| self.auth_completed.set() | ||
| return create_server_error_response(str(e)) | ||
|
|
||
| def _setup_attachment_route(self): | ||
|
|
@@ -200,6 +216,22 @@ def run_server(): | |
| logger.error(error_msg) | ||
| return False, error_msg | ||
|
|
||
| def wait_for_auth(self, timeout: float = 300) -> Optional[dict]: | ||
| """ | ||
| Block until OAuth callback is received or timeout. | ||
|
|
||
| Args: | ||
| timeout: Maximum seconds to wait (default 5 minutes) | ||
|
|
||
| Returns: | ||
| Auth result dict {"success": bool, "user_id": str|None, "error": str|None} | ||
| or None if timed out | ||
| """ | ||
| completed = self.auth_completed.wait(timeout=timeout) | ||
| if completed: | ||
| return self.auth_result | ||
| return None | ||
|
|
||
|
Comment on lines
+240
to
+255
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reset auth state between flows to avoid stale success.
Consider adding an explicit reset path (called before opening the auth URL) to clear the event and result. 🔧 Suggested change- def wait_for_auth(self, timeout: float = 300) -> Optional[dict]:
+ def wait_for_auth(self, timeout: float = 300, reset: bool = False) -> Optional[dict]:
"""
Block until OAuth callback is received or timeout.
@@
- completed = self.auth_completed.wait(timeout=timeout)
+ if reset:
+ self.auth_result = None
+ self.auth_completed.clear()
+ completed = self.auth_completed.wait(timeout=timeout)
if completed:
return self.auth_result
return None🤖 Prompt for AI Agents |
||
| def stop(self): | ||
| """Stop the minimal OAuth server.""" | ||
| if not self.is_running: | ||
|
|
@@ -279,6 +311,34 @@ def ensure_oauth_callback_available( | |
| return False, error_msg | ||
|
|
||
|
|
||
| def set_cli_oauth_server(server: MinimalOAuthServer) -> None: | ||
| """Register a MinimalOAuthServer as the global instance (used by CLI mode).""" | ||
| global _minimal_oauth_server | ||
| _minimal_oauth_server = server | ||
|
|
||
|
|
||
| def get_cli_oauth_port() -> int: | ||
| """ | ||
| Find an available port for the CLI OAuth callback server. | ||
| Tries ports 8000-8009, falls back to OS-assigned port. | ||
|
|
||
| Returns: | ||
| Available port number | ||
| """ | ||
| for port in range(8000, 8010): | ||
| try: | ||
| with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: | ||
| s.bind(("localhost", port)) | ||
| return port | ||
| except OSError: | ||
| continue | ||
|
|
||
| # Fallback: let OS assign a port | ||
| with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: | ||
| s.bind(("localhost", 0)) | ||
| return s.getsockname()[1] | ||
|
Comment on lines
+341
to
+360
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not pick the callback port with a bind-and-release probe.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably more minor than major, the likelihood that a port gets claimed in the moments between check and initialization seems low
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The slightly more interesting edge case remains: if some other process already happened to be listening on the chosen port before Happy to leave this as-is if you'd prefer — it's a pre-existing pattern and the practical risk is low. 🧠 Learnings used |
||
|
|
||
|
|
||
| def cleanup_oauth_callback_server(): | ||
| """Clean up the minimal OAuth server if it was started.""" | ||
| global _minimal_oauth_server | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -16,10 +16,16 @@ | |||||
| import asyncio | ||||||
| import json | ||||||
| import logging | ||||||
| import os | ||||||
| import sys | ||||||
| from typing import Any, Dict, List, Optional | ||||||
|
|
||||||
| from auth.oauth_config import set_transport_mode | ||||||
| from auth.oauth_config import set_transport_mode, reload_oauth_config | ||||||
| from auth.oauth_callback_server import ( | ||||||
| MinimalOAuthServer, | ||||||
| get_cli_oauth_port, | ||||||
| set_cli_oauth_server, | ||||||
| ) | ||||||
|
Comment on lines
+24
to
+28
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CLI run path starts OAuth server but never tears it down. Line 474 starts a callback server, but the 🔧 Suggested fix from auth.oauth_callback_server import (
MinimalOAuthServer,
+ cleanup_oauth_callback_server,
get_cli_oauth_port,
set_cli_oauth_server,
)
@@
if parsed["command"] == "run":
# Pre-start OAuth server before running tools that may need auth
oauth_server = _setup_cli_oauth_server()
# Merge stdin args with inline args (inline takes precedence)
args = read_stdin_args()
args.update(parsed["tool_args"])
- try:
- result = await run_tool(server, parsed["tool_name"], args)
- print(result)
- return 0
- except Exception as e:
- from auth.google_auth import GoogleAuthenticationError
- if isinstance(e, GoogleAuthenticationError) and e.auth_url:
- result = await _handle_cli_auth_flow(
- server, parsed["tool_name"], args, e, oauth_server
- )
- print(result)
- return 0
- raise
+ try:
+ try:
+ result = await run_tool(server, parsed["tool_name"], args)
+ print(result)
+ return 0
+ except Exception as e:
+ from auth.google_auth import GoogleAuthenticationError
+ if isinstance(e, GoogleAuthenticationError) and e.auth_url:
+ result = await _handle_cli_auth_flow(
+ server, parsed["tool_name"], args, e, oauth_server
+ )
+ print(result)
+ return 0
+ raise
+ finally:
+ cleanup_oauth_callback_server()Also applies to: 473-492 🤖 Prompt for AI Agents |
||||||
|
|
||||||
| logger = logging.getLogger(__name__) | ||||||
|
|
||||||
|
|
@@ -264,6 +270,10 @@ async def run_tool(server, tool_name: str, args: Dict[str, Any]) -> str: | |||||
| f"Provided parameters: {list(call_args.keys())}" | ||||||
| ) | ||||||
| except Exception as e: | ||||||
| # Let GoogleAuthenticationError propagate for CLI auth handling | ||||||
| from auth.google_auth import GoogleAuthenticationError | ||||||
| if isinstance(e, GoogleAuthenticationError): | ||||||
| raise | ||||||
| logger.error(f"[CLI] Error executing {tool_name}: {e}", exc_info=True) | ||||||
| return f"Error: {type(e).__name__}: {e}" | ||||||
|
|
||||||
|
|
@@ -361,6 +371,74 @@ def read_stdin_args() -> Dict[str, Any]: | |||||
| return {} | ||||||
|
|
||||||
|
|
||||||
| def _setup_cli_oauth_server() -> MinimalOAuthServer: | ||||||
| """ | ||||||
| Pre-start a MinimalOAuthServer for CLI auth flow. | ||||||
|
|
||||||
| Finds an available port, configures the redirect URI, starts the server, | ||||||
| and registers it as the global OAuth server. | ||||||
|
|
||||||
| Returns: | ||||||
| The started MinimalOAuthServer instance | ||||||
| """ | ||||||
| port = get_cli_oauth_port() | ||||||
| redirect_uri = f"http://localhost:{port}/oauth2callback" | ||||||
|
|
||||||
| # Set redirect URI in env so OAuthConfig picks it up | ||||||
| os.environ["GOOGLE_OAUTH_REDIRECT_URI"] = redirect_uri | ||||||
| reload_oauth_config() | ||||||
|
|
||||||
| oauth_server = MinimalOAuthServer(port=port, base_uri="http://localhost") | ||||||
| success, error_msg = oauth_server.start() | ||||||
| if not success: | ||||||
| raise RuntimeError(f"Failed to start OAuth server: {error_msg}") | ||||||
|
|
||||||
| set_cli_oauth_server(oauth_server) | ||||||
| logger.info(f"[CLI] OAuth callback server ready on port {port}") | ||||||
| return oauth_server | ||||||
|
|
||||||
|
|
||||||
| async def _handle_cli_auth_flow( | ||||||
| server, tool_name: str, args: Dict[str, Any], auth_error, oauth_server: MinimalOAuthServer | ||||||
| ) -> str: | ||||||
| """ | ||||||
| Handle OAuth authentication flow in CLI mode. | ||||||
|
|
||||||
| Prints the auth URL, waits for the browser callback, then retries the tool. | ||||||
|
|
||||||
| Args: | ||||||
| server: The FastMCP server instance | ||||||
| tool_name: Name of the tool to retry after auth | ||||||
| args: Tool arguments | ||||||
| auth_error: The GoogleAuthenticationError with auth_url | ||||||
| oauth_server: The running MinimalOAuthServer | ||||||
|
|
||||||
| Returns: | ||||||
| Tool result as string | ||||||
| """ | ||||||
| print("\n" + "=" * 60, file=sys.stderr) | ||||||
| print("Authentication Required", file=sys.stderr) | ||||||
| print("=" * 60, file=sys.stderr) | ||||||
| print(f"\nOpen this URL in your browser:\n", file=sys.stderr) | ||||||
| print(f" {auth_error.auth_url}\n", file=sys.stderr) | ||||||
| print("Waiting for authentication (timeout: 5 minutes)...", file=sys.stderr) | ||||||
|
|
||||||
| # Wait for the OAuth callback in a thread (blocking call) | ||||||
| result = await asyncio.to_thread(oauth_server.wait_for_auth, 300) | ||||||
|
|
||||||
| if result is None: | ||||||
| raise RuntimeError("Authentication timed out after 5 minutes.") | ||||||
|
|
||||||
| if not result["success"]: | ||||||
| raise RuntimeError(f"Authentication failed: {result['error']}") | ||||||
|
|
||||||
| print(f"\nAuthenticated as: {result['user_id']}", file=sys.stderr) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't print the authenticated account ID to stderr. This sends a raw Google account identifier into CLI transcripts and agent event streams. A generic success message, or at most a masked address, keeps the flow usable without leaking PII. 🔧 Suggested change- print(f"\nAuthenticated as: {result['user_id']}", file=sys.stderr)
+ print("\nAuthentication completed successfully.", file=sys.stderr)As per coding guidelines "Never log or leak secrets, refresh tokens, or PII in exceptions or event streams." 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CLI mode is explicitly local to a user system so I would not consider this "major" - just a nit
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
🧠 Learnings used |
||||||
| print("Retrying tool execution...\n", file=sys.stderr) | ||||||
|
|
||||||
| # Retry the tool | ||||||
| return await run_tool(server, tool_name, args) | ||||||
|
coderabbitai[bot] marked this conversation as resolved.
|
||||||
|
|
||||||
|
|
||||||
| async def handle_cli_mode(server, cli_args: List[str]) -> int: | ||||||
| """ | ||||||
| Main entry point for CLI mode. | ||||||
|
|
@@ -390,13 +468,26 @@ async def handle_cli_mode(server, cli_args: List[str]) -> int: | |||||
| return 0 | ||||||
|
|
||||||
| if parsed["command"] == "run": | ||||||
| # Pre-start OAuth server before running tools that may need auth | ||||||
| oauth_server = _setup_cli_oauth_server() | ||||||
|
|
||||||
|
Comment on lines
472
to
+475
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't make every CLI tool run depend on starting the callback server. This now fails the 🤖 Prompt for AI Agents
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Valid
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain
🏁 Script executed: Length of output: 378 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 1495
🧠 Learnings used💡 Use |
||||||
| # Merge stdin args with inline args (inline takes precedence) | ||||||
| args = read_stdin_args() | ||||||
| args.update(parsed["tool_args"]) | ||||||
|
|
||||||
| result = await run_tool(server, parsed["tool_name"], args) | ||||||
| print(result) | ||||||
| return 0 | ||||||
| try: | ||||||
| result = await run_tool(server, parsed["tool_name"], args) | ||||||
| print(result) | ||||||
| return 0 | ||||||
| except Exception as e: | ||||||
| from auth.google_auth import GoogleAuthenticationError | ||||||
| if isinstance(e, GoogleAuthenticationError) and e.auth_url: | ||||||
| result = await _handle_cli_auth_flow( | ||||||
| server, parsed["tool_name"], args, e, oauth_server | ||||||
| ) | ||||||
| print(result) | ||||||
| return 0 | ||||||
| raise | ||||||
|
|
||||||
| # Unknown command | ||||||
| print(f"Unknown command: {parsed['command']}") | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.