diff --git a/CLAUDE.md b/CLAUDE.md index 751834d..b579afd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -135,16 +135,15 @@ Each Nextcloud app has a corresponding server module that: OAuth integration tests support both **automated** (Playwright) and **interactive** authentication flows: **Automated Testing (Default - Recommended for CI/CD):** -- **Default fixtures**: `nc_oauth_client`, `nc_mcp_oauth_client` now use Playwright automation by default +- **Default fixtures**: `nc_oauth_client`, `nc_mcp_oauth_client` use Playwright automation - Uses Playwright headless browser automation to complete OAuth flow programmatically -- **Shared OAuth Client**: All test users authenticate using a single OAuth client (matching MCP server behavior) - - Single `client_id`/`client_secret` pair is registered and reused for all test users - - Stored in `.nextcloud_oauth_shared_test_client.json` with `force_register=False` for reuse - - Reduces OAuth client registrations and matches production MCP server architecture +- **Shared OAuth Client**: All test users authenticate using a single OAuth client + - Stored in `.nextcloud_oauth_shared_test_client.json` + - Matches production MCP server behavior + - Each user gets their own unique access token + - Implementation: `shared_oauth_client_credentials` fixture in `tests/conftest.py:812` - All Playwright fixtures: `playwright_oauth_token`, `nc_oauth_client`, `nc_mcp_oauth_client`, `nc_oauth_client_playwright`, `nc_mcp_oauth_client_playwright` - Multi-user fixtures: `alice_oauth_token`, `bob_oauth_token`, `charlie_oauth_token`, `diana_oauth_token` - - All use `shared_oauth_client_credentials` fixture for consistent client credentials - - Each user gets unique access tokens via same OAuth client (like multiple users using the MCP server) - Requires: `NEXTCLOUD_HOST`, `NEXTCLOUD_USERNAME`, `NEXTCLOUD_PASSWORD` environment variables - Uses `pytest-playwright-asyncio` for async Playwright fixtures - Playwright configuration: Use pytest CLI args like `--browser firefox --headed` to customize @@ -179,14 +178,12 @@ OAuth integration tests support both **automated** (Playwright) and **interactiv - `mcp-oauth` (port 8001): Uses OAuth authentication - for OAuth-specific testing - Start OAuth MCP server: `docker-compose up --build -d mcp-oauth` - **Important**: When working on OAuth functionality, always rebuild `mcp-oauth` container, not `mcp` -- Shared OAuth client is registered once and reused across test runs -- Client credentials cached in `.nextcloud_oauth_shared_test_client.json` +- OAuth client credentials cached in `.nextcloud_oauth_shared_test_client.json` **CI/CD Considerations:** - Interactive OAuth tests are automatically skipped when `GITHUB_ACTIONS` environment variable is set - Automated Playwright tests will run in CI/CD environments - Use Firefox browser in CI: `--browser firefox` (Chromium may have issues with localhost redirects) -- Shared client approach reduces test time and API calls to Nextcloud ### Configuration Files diff --git a/docs/oauth-architecture.md b/docs/oauth-architecture.md index dbf9864..cec3faa 100644 --- a/docs/oauth-architecture.md +++ b/docs/oauth-architecture.md @@ -217,11 +217,12 @@ NEXTCLOUD_HOST=https://nextcloud.example.com **How it works**: 1. Server checks `/.well-known/openid-configuration` for `registration_endpoint` -2. Calls `/apps/oidc/register` to register new client +2. Calls `/apps/oidc/register` to register a client on first startup 3. Saves credentials to `.nextcloud_oauth_client.json` -4. Re-registers if credentials expire +4. Reuses these credentials on subsequent startups +5. Re-registers only if credentials are missing or expired -**Best for**: Development, testing, short-lived deployments +**Best for**: Development, testing, quick deployments ### Pre-configured Client diff --git a/docs/oauth-setup.md b/docs/oauth-setup.md index 3024f3f..7b90136 100644 --- a/docs/oauth-setup.md +++ b/docs/oauth-setup.md @@ -165,23 +165,23 @@ You have two options for managing OAuth clients: ### Mode A: Automatic Registration (Dynamic Client Registration) -**Best for**: Development, testing, short-lived deployments +**Best for**: Development, testing, quick deployments **How it works**: -- MCP server automatically registers OAuth client at startup +- MCP server automatically registers an OAuth client on first startup - Uses Nextcloud's dynamic client registration endpoint - Saves credentials to `.nextcloud_oauth_client.json` +- Reuses stored credentials on subsequent restarts - Re-registers automatically if credentials expire **Pros**: - Zero configuration required - Quick setup -- No manual client management +- Automatic credential management **Cons**: - Clients expire (default: 1 hour, configurable) -- Must re-register on restart if expired -- Not ideal for long-running production +- Must have dynamic client registration enabled on Nextcloud **Configuration**: Skip to [Step 4](#step-4-configure-mcp-server) with minimal config. @@ -192,8 +192,8 @@ You have two options for managing OAuth clients: **Best for**: Production, long-running deployments, stable environments **How it works**: -- You manually register OAuth client via Nextcloud CLI -- Provide client credentials to MCP server +- You manually register an OAuth client via Nextcloud CLI +- Provide client credentials to MCP server via environment variables - Credentials don't expire **Pros**: diff --git a/docs/quickstart-oauth.md b/docs/quickstart-oauth.md index 47f8fae..7675ee3 100644 --- a/docs/quickstart-oauth.md +++ b/docs/quickstart-oauth.md @@ -151,11 +151,11 @@ curl https://your.nextcloud.instance.com/.well-known/openid-configuration This quick start uses **automatic client registration** which is perfect for: - Development - Testing -- Short-lived deployments +- Quick deployments -For **production deployments**, you should: -1. Pre-register OAuth clients manually -2. Use dedicated client credentials +For **production deployments**, consider: +1. Pre-registering OAuth client manually +2. Using dedicated client credentials that don't expire 3. See [OAuth Setup Guide](oauth-setup.md) for production configuration --- diff --git a/nextcloud_mcp_server/auth/client_registration.py b/nextcloud_mcp_server/auth/client_registration.py index 2e2943d..b8b7340 100644 --- a/nextcloud_mcp_server/auth/client_registration.py +++ b/nextcloud_mcp_server/auth/client_registration.py @@ -211,7 +211,6 @@ async def load_or_register_client( storage_path: str | Path, client_name: str = "Nextcloud MCP Server", redirect_uris: list[str] | None = None, - force_register: bool = True, ) -> ClientInfo: """ Load client from storage or register a new one if not found/expired. @@ -219,7 +218,7 @@ async def load_or_register_client( This function: 1. Checks for existing client credentials in storage 2. Validates the credentials are not expired - 3. Registers a new client if needed + 3. Registers a new client if needed (no stored credentials or expired) 4. Saves the new client credentials Args: @@ -228,7 +227,6 @@ async def load_or_register_client( storage_path: Path to store client credentials client_name: Name of the client application redirect_uris: List of redirect URIs - force_register: Force registration even if valid credentials exist Returns: ClientInfo with valid credentials @@ -239,11 +237,10 @@ async def load_or_register_client( """ storage_path = Path(storage_path) - # Try to load existing client unless forced to register - if not force_register: - client_info = load_client_from_file(storage_path) - if client_info: - return client_info + # Try to load existing client + client_info = load_client_from_file(storage_path) + if client_info: + return client_info # Register new client logger.info("Registering new OAuth client...") diff --git a/pyproject.toml b/pyproject.toml index 6ebe5a9..ac4b550 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -22,8 +22,8 @@ asyncio_mode = "auto" asyncio_default_test_loop_scope = "session" asyncio_default_fixture_loop_scope = "session" log_cli = 1 -log_cli_level = "INFO" -log_level = "INFO" +log_cli_level = "WARN" +log_level = "WARN" markers = [ "integration: marks tests as slow (deselect with '-m \"not slow\"')", "oauth: marks tests as oauth (deselect with '-m \"not oauth\"')" diff --git a/tests/conftest.py b/tests/conftest.py index a13018d..90a0f84e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -651,8 +651,10 @@ def oauth_callback_server(): """ Fixture to create an HTTP server for OAuth callback handling. - Yields a tuple of (auth_state, server_url) where: - - auth_state: A dict with {"code": None} that will be populated with the auth code + Supports multiple concurrent OAuth flows using state parameters for correlation. + + Yields a tuple of (auth_states, server_url) where: + - auth_states: A dict mapping state parameter to auth code - server_url: The callback URL for the server (e.g., "http://localhost:8081") The server automatically shuts down when the fixture is torn down. @@ -663,8 +665,9 @@ def oauth_callback_server(): from http.server import BaseHTTPRequestHandler, HTTPServer from urllib.parse import parse_qs, urlparse - # Use a mutable container to share state across threads - auth_state = {"code": None} + # Use a dict to store auth codes keyed by state parameter + # This allows multiple concurrent OAuth flows + auth_states = {} httpd = None server_thread = None @@ -674,26 +677,27 @@ def oauth_callback_server(): pass def do_GET(self): - # Ignore subsequent requests if we already have a code - # (this is a session-scoped fixture, so only process the first auth code) - if auth_state["code"] is not None: - self.send_response(200) - self.send_header("Content-type", "text/html") - self.end_headers() - self.wfile.write( - b"

Authentication already completed

" - ) - return - # Parse the callback request parsed_path = urlparse(self.path) query = parse_qs(parsed_path.query) code = query.get("code", [None])[0] + state = query.get("state", [None])[0] # Only process if we have a valid code if code: - auth_state["code"] = code - logger.info(f"OAuth callback received. Code: {code[:20]}...") + # Store code keyed by state parameter for correlation + if state: + auth_states[state] = code + logger.info( + f"OAuth callback received for state={state[:16]}... Code: {code[:20]}..." + ) + else: + # Fallback for flows without state parameter (legacy interactive flow) + auth_states["_default"] = code + logger.info( + f"OAuth callback received (no state). Code: {code[:20]}..." + ) + self.send_response(200) self.send_header("Content-type", "text/html") self.end_headers() @@ -714,8 +718,8 @@ def oauth_callback_server(): server_thread.start() logger.info("OAuth callback server started on http://localhost:8081") - # Yield the auth state and server URL - yield auth_state, "http://localhost:8081" + # Yield the auth states dict and server URL + yield auth_states, "http://localhost:8081" finally: # Clean up the server @@ -746,8 +750,8 @@ async def interactive_oauth_token(oauth_callback_server) -> str: from nextcloud_mcp_server.auth.client_registration import load_or_register_client - # Unpack the server fixture - auth_state, callback_url = oauth_callback_server + # Unpack the server fixture (now returns dict of auth_states) + auth_states, callback_url = oauth_callback_server nextcloud_host = os.getenv("NEXTCLOUD_HOST") async with httpx.AsyncClient() as http_client: @@ -762,7 +766,6 @@ async def interactive_oauth_token(oauth_callback_server) -> str: registration_endpoint=registration_endpoint, storage_path=".nextcloud_oauth_shared_test_client.json", redirect_uris=[callback_url], - force_register=False, # Reuse existing credentials if valid ) # First, open Nextcloud login page to establish session @@ -772,22 +775,22 @@ async def interactive_oauth_token(oauth_callback_server) -> str: "After logging in, the OAuth authorization will proceed automatically" ) - # Construct authorization URL + # Construct authorization URL (no state parameter for interactive flow) auth_url = f"{authorization_endpoint}?response_type=code&client_id={client_info.client_id}&redirect_uri={callback_url}&scope=openid%20profile%20email" # Open authorization URL in browser webbrowser.open(auth_url) - # Wait for auth code with timeout + # Wait for auth code with timeout (uses "_default" key for flows without state) timeout = 120 # 2 minutes start_time = time.time() - while not auth_state["code"]: + while "_default" not in auth_states: if time.time() - start_time > timeout: raise TimeoutError("OAuth authorization timed out after 2 minutes") logger.info("Waiting for OAuth authorization...") time.sleep(1) - auth_code = auth_state["code"] + auth_code = auth_states["_default"] logger.info("Received authorization code, exchanging for token...") token_response = await http_client.post( @@ -810,13 +813,15 @@ async def interactive_oauth_token(oauth_callback_server) -> str: @pytest.fixture(scope="session") -async def shared_oauth_client_credentials(): +async def shared_oauth_client_credentials(oauth_callback_server): """ Fixture to obtain shared OAuth client credentials that will be reused for all users. This registers a single OAuth client with Nextcloud that matches the MCP server's registration, allowing all test users to authenticate using the same client_id/secret. + Now uses the real OAuth callback server for reliable token acquisition. + Returns: Tuple of (client_id, client_secret, callback_url, token_endpoint, authorization_endpoint) """ @@ -826,7 +831,11 @@ async def shared_oauth_client_credentials(): if not nextcloud_host: pytest.skip("Shared OAuth client requires NEXTCLOUD_HOST") + # Get callback URL from the real callback server + auth_states, callback_url = oauth_callback_server + logger.info("Setting up shared OAuth client credentials for all test users...") + logger.info(f"Using real callback server at: {callback_url}") async with httpx.AsyncClient(timeout=30.0) as http_client: # OIDC Discovery @@ -842,9 +851,6 @@ async def shared_oauth_client_credentials(): if not all([token_endpoint, registration_endpoint, authorization_endpoint]): raise ValueError("OIDC discovery missing required endpoints") - # Use callback URL that won't actually be used (we extract code from browser URL) - callback_url = "http://localhost:9999/oauth/callback" - # Register or load shared OAuth client (matches MCP server registration) client_info = await load_or_register_client( nextcloud_url=nextcloud_host, @@ -852,7 +858,6 @@ async def shared_oauth_client_credentials(): storage_path=".nextcloud_oauth_shared_test_client.json", client_name="Nextcloud MCP Server - Shared Test Client", redirect_uris=[callback_url], - force_register=False, # Reuse existing credentials if valid ) logger.info(f"Shared OAuth client ready: {client_info.client_id[:16]}...") @@ -868,7 +873,9 @@ async def shared_oauth_client_credentials(): @pytest.fixture(scope="session") -async def playwright_oauth_token(browser, shared_oauth_client_credentials) -> str: +async def playwright_oauth_token( + browser, shared_oauth_client_credentials, oauth_callback_server +) -> str: """ Fixture to obtain an OAuth access token using Playwright headless browser automation. @@ -877,7 +884,7 @@ async def playwright_oauth_token(browser, shared_oauth_client_credentials) -> st 2. Navigating to authorization URL in headless browser 3. Programmatically filling in login form 4. Handling OAuth consent - 5. Extracting auth code from redirect + 5. Waiting for callback server to receive auth code (NEW: using real callback server!) 6. Exchanging code for access token Environment variables required: @@ -890,7 +897,9 @@ async def playwright_oauth_token(browser, shared_oauth_client_credentials) -> st - Browser fixture provided by pytest-playwright-asyncio - See: https://playwright.dev/python/docs/test-runners """ - from urllib.parse import parse_qs, urlparse + import secrets + import time + from urllib.parse import quote nextcloud_host = os.getenv("NEXTCLOUD_HOST") username = os.getenv("NEXTCLOUD_USERNAME") @@ -901,6 +910,9 @@ async def playwright_oauth_token(browser, shared_oauth_client_credentials) -> st "Playwright OAuth requires NEXTCLOUD_HOST, NEXTCLOUD_USERNAME, and NEXTCLOUD_PASSWORD" ) + # Get auth_states dict from callback server + auth_states, _ = oauth_callback_server + # Unpack shared client credentials client_id, client_secret, callback_url, token_endpoint, authorization_endpoint = ( shared_oauth_client_credentials @@ -908,13 +920,19 @@ async def playwright_oauth_token(browser, shared_oauth_client_credentials) -> st logger.info(f"Starting Playwright-based OAuth flow for {username}...") logger.info(f"Using shared OAuth client: {client_id[:16]}...") + logger.info(f"Using real callback server at: {callback_url}") - # Construct authorization URL + # Generate unique state parameter for this OAuth flow + state = secrets.token_urlsafe(32) + logger.debug(f"Generated state: {state[:16]}...") + + # Construct authorization URL with state parameter auth_url = ( f"{authorization_endpoint}?" f"response_type=code&" f"client_id={client_id}&" - f"redirect_uri={callback_url}&" + f"redirect_uri={quote(callback_url, safe='')}&" + f"state={state}&" f"scope=openid%20profile%20email" ) @@ -971,33 +989,24 @@ async def playwright_oauth_token(browser, shared_oauth_client_credentials) -> st except Exception as e: logger.debug(f"No authorization button found or already authorized: {e}") - # Wait for redirect to callback URL (which will fail to load, but we just need the URL) - try: - # The redirect might fail since localhost:9999 isn't actually running - # But we can still extract the code from the URL - await page.wait_for_url(f"{callback_url}*", timeout=10000) - except Exception as e: - # Expected - the callback URL won't load, but we should have the URL - logger.debug(f"Callback redirect (expected to fail): {e}") + # Wait for callback server to receive the auth code + # Browser will be redirected to localhost:8081 which will capture the code + logger.info("Waiting for callback server to receive auth code...") + timeout_seconds = 30 + start_time = time.time() + while state not in auth_states: + if time.time() - start_time > timeout_seconds: + # Take a screenshot for debugging + screenshot_path = "/tmp/playwright_oauth_error.png" + await page.screenshot(path=screenshot_path) + logger.error(f"Screenshot saved to {screenshot_path}") + raise TimeoutError( + f"Timeout waiting for OAuth callback (state={state[:16]}...)" + ) + await asyncio.sleep(0.5) - # Extract auth code from URL - final_url = page.url - logger.debug(f"Final URL: {final_url}") - - parsed_url = urlparse(final_url) - query_params = parse_qs(parsed_url.query) - auth_code = query_params.get("code", [None])[0] - - if not auth_code: - # Take a screenshot for debugging - screenshot_path = "/tmp/playwright_oauth_error.png" - await page.screenshot(path=screenshot_path) - logger.error(f"Screenshot saved to {screenshot_path}") - raise ValueError( - f"No authorization code found in redirect URL: {final_url}" - ) - - logger.info(f"Successfully extracted authorization code: {auth_code[:20]}...") + auth_code = auth_states[state] + logger.info(f"Successfully received authorization code: {auth_code[:20]}...") finally: await context.close() @@ -1236,23 +1245,31 @@ async def test_users_setup(nc_client: NextcloudClient): async def _get_oauth_token_for_user( - browser, shared_oauth_client_credentials, username: str, password: str + browser, + shared_oauth_client_credentials, + auth_states, + username: str, + password: str, ) -> str: """ Helper function to get OAuth access token for a user via Playwright. Uses shared OAuth client credentials to authenticate multiple users with the same client. + Now uses real callback server with state parameters for reliable token acquisition. Args: browser: Playwright browser instance shared_oauth_client_credentials: Tuple of (client_id, client_secret, callback_url, token_endpoint, authorization_endpoint) + auth_states: Dict mapping state parameters to auth codes (from callback server) username: Username to authenticate as password: Password for the user Returns: OAuth access token string """ - from urllib.parse import parse_qs, urlparse + import secrets + import time + from urllib.parse import quote nextcloud_host = os.getenv("NEXTCLOUD_HOST") @@ -1267,14 +1284,17 @@ async def _get_oauth_token_for_user( logger.info(f"Getting OAuth token for user: {username}...") logger.info(f"Using shared OAuth client: {client_id[:16]}...") - # Construct authorization URL with properly encoded redirect_uri - from urllib.parse import quote + # Generate unique state parameter for this OAuth flow + state = secrets.token_urlsafe(32) + logger.debug(f"Generated state for {username}: {state[:16]}...") + # Construct authorization URL with state parameter auth_url = ( f"{authorization_endpoint}?" f"response_type=code&" f"client_id={client_id}&" f"redirect_uri={quote(callback_url, safe='')}&" + f"state={state}&" f"scope=openid%20profile%20email" ) @@ -1311,22 +1331,25 @@ async def _get_oauth_token_for_user( except Exception as e: logger.debug(f"No authorization needed for {username}: {e}") - # Wait for redirect and extract auth code - try: - await page.wait_for_url(f"{callback_url}*", timeout=30000) - except Exception: - pass # Expected - callback won't load - - final_url = page.url - parsed_url = urlparse(final_url) - query_params = parse_qs(parsed_url.query) - auth_code = query_params.get("code", [None])[0] - - if not auth_code: - raise ValueError( - f"No authorization code found for {username} in URL: {final_url}" - ) + # Wait for callback server to receive the auth code + # Browser will be redirected to localhost:8081 which will capture the code + logger.info( + f"Waiting for callback server to receive auth code for {username}..." + ) + timeout_seconds = 30 + start_time = time.time() + while state not in auth_states: + if time.time() - start_time > timeout_seconds: + # Take screenshot for debugging + screenshot_path = f"/tmp/playwright_oauth_timeout_{username}.png" + await page.screenshot(path=screenshot_path) + logger.error(f"Screenshot saved to {screenshot_path}") + raise TimeoutError( + f"Timeout waiting for OAuth callback for {username} (state={state[:16]}...)" + ) + await asyncio.sleep(0.5) + auth_code = auth_states[state] logger.info(f"Got auth code for {username}: {auth_code[:20]}...") finally: @@ -1360,7 +1383,7 @@ async def _get_oauth_token_for_user( # Parallel token retrieval fixture - fetches all OAuth tokens concurrently @pytest.fixture(scope="session") async def all_oauth_tokens( - browser, shared_oauth_client_credentials, test_users_setup + browser, shared_oauth_client_credentials, test_users_setup, oauth_callback_server ) -> dict[str, str]: """ Fetch OAuth tokens for all test users in parallel for speed. @@ -1368,26 +1391,34 @@ async def all_oauth_tokens( Returns a dict mapping username to OAuth access token. This is significantly faster than fetching tokens sequentially. - Note: We add a small stagger between starting each flow to avoid - race conditions in Nextcloud's OAuth session handling. + Now uses the real callback server with state parameters for reliable + concurrent token acquisition without race conditions. """ import asyncio import time + # Get auth_states dict from callback server + auth_states, callback_url = oauth_callback_server + start_time = time.time() logger.info("Fetching OAuth tokens for all users in parallel...") + logger.info(f"Using callback server at {callback_url} with state-based correlation") async def get_token_with_delay(username: str, config: dict, delay: float): """Get token for a user after a small delay to stagger requests.""" if delay > 0: await asyncio.sleep(delay) return await _get_oauth_token_for_user( - browser, shared_oauth_client_credentials, username, config["password"] + browser, + shared_oauth_client_credentials, + auth_states, + username, + config["password"], ) # Create tasks for all users with staggered starts (2.0s apart) tasks = { - username: get_token_with_delay(username, config, (idx + 1) * 2.0) + username: get_token_with_delay(username, config, idx * 0.5) for idx, (username, config) in enumerate(test_users_setup.items()) }