From 3ad9198f36f05f05b7b691598521b06795dec1c0 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 15 Oct 2025 16:27:22 +0200 Subject: [PATCH] fix(oauth): Remove the option to force_register new clients --- CLAUDE.md | 17 +++++++---------- docs/oauth-architecture.md | 7 ++++--- docs/oauth-setup.md | 14 +++++++------- docs/quickstart-oauth.md | 8 ++++---- .../auth/client_registration.py | 13 +++++-------- pyproject.toml | 4 ++-- tests/conftest.py | 2 -- 7 files changed, 29 insertions(+), 36 deletions(-) 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..852f85d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -762,7 +762,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 @@ -852,7 +851,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]}...")