diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index 7bd7ffb7..2698e3aa 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -33,7 +33,7 @@ jobs: - name: Run Claude Code Review id: claude-review - uses: anthropics/claude-code-action@58dbe8ed6879f0d3b02ac295b20d5fdfe7733e0c # v1.0.85 + uses: anthropics/claude-code-action@6e2bd52842c65e914eba5c8badd17560bd26b5de # v1.0.89 with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} allowed_bots: "renovate-bot-cbcoutinho" diff --git a/.github/workflows/claude.yml b/.github/workflows/claude.yml index 3cdb444a..bbec39d9 100644 --- a/.github/workflows/claude.yml +++ b/.github/workflows/claude.yml @@ -32,7 +32,7 @@ jobs: - name: Run Claude Code id: claude - uses: anthropics/claude-code-action@58dbe8ed6879f0d3b02ac295b20d5fdfe7733e0c # v1.0.85 + uses: anthropics/claude-code-action@6e2bd52842c65e914eba5c8badd17560bd26b5de # v1.0.89 with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 98f03490..a1183929 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -43,6 +43,7 @@ jobs: - "single-user" - "multi-user-basic" - "login-flow" + - "keycloak" include: # Version-specific image pins — Renovate updates these via customManagers in renovate.json # Each entry is pinned to its major version (e.g., NC 31 only gets 31.x updates) @@ -81,6 +82,14 @@ jobs: needs-playwright: true extra-args: "" + - mode: keycloak + profile: keycloak + markers: "keycloak" + wait-port: 8002 + mcp-internal-url: "http://mcp-keycloak:8002" + needs-playwright: true + extra-args: "" + name: integration (${{ matrix.mode }} / nc${{ matrix.nextcloud_version }}) steps: @@ -174,14 +183,40 @@ jobs: done echo "MCP service is ready on port ${{ matrix.wait-port }}." + - name: Wait for Keycloak + if: matrix.mode == 'keycloak' + run: | + echo "Waiting for Keycloak realm at http://localhost:8888..." + max_attempts=30 + attempt=0 + until curl -sf http://localhost:8888/realms/nextcloud-mcp > /dev/null 2>&1; do + attempt=$((attempt + 1)) + if [ $attempt -ge $max_attempts ]; then + echo "Keycloak did not become ready in time." + docker compose --profile keycloak logs keycloak + exit 1 + fi + echo "Attempt $attempt/$max_attempts: Not ready, sleeping 5s..." + sleep 5 + done + echo "Keycloak is ready." + - name: Verify OIDC configuration - if: matrix.mode == 'login-flow' + if: matrix.mode == 'login-flow' || matrix.mode == 'keycloak' run: | echo "=== OIDC Discovery ===" curl -s http://localhost:8080/.well-known/openid-configuration | jq . echo "=== OIDC App Status ===" docker compose exec -T app php occ app:list --output=json 2>/dev/null | jq '.enabled.oidc // "NOT INSTALLED"' + - name: Verify Keycloak realm + if: matrix.mode == 'keycloak' + run: | + echo "=== Keycloak Realm Discovery ===" + curl -s http://localhost:8888/realms/nextcloud-mcp/.well-known/openid-configuration | jq . + echo "=== Keycloak Provider in Nextcloud ===" + docker compose exec -T app php occ user_oidc:provider keycloak 2>/dev/null || echo "Provider not yet configured" + - name: Run tests (${{ matrix.mode }}) env: NEXTCLOUD_HOST: "http://localhost:8080" diff --git a/CHANGELOG.md b/CHANGELOG.md index c288fba2..d84d49b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,18 @@ All notable changes to the Nextcloud MCP Server will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [PEP 440](https://peps.python.org/pep-0440/). +## v0.68.3 (2026-04-05) + +### Fix + +- address PR review feedback for client registry and DCR proxy +- support cloud OAuth clients and graceful DCR fallback + +### Refactor + +- remove ALLOWED_MCP_CLOUD_CLIENTS and add keycloak CI profile +- consolidate ALLOWED_MCP_CLIENTS and add redirect URI validation + ## v0.68.2 (2026-04-04) ### Fix diff --git a/app-hooks/patches/0001-Fix-Bearer-token-authentication-causing-session-logo.patch b/app-hooks/patches/0001-Fix-Bearer-token-authentication-causing-session-logo.patch deleted file mode 100644 index c578441c..00000000 --- a/app-hooks/patches/0001-Fix-Bearer-token-authentication-causing-session-logo.patch +++ /dev/null @@ -1,69 +0,0 @@ -From deab2dac3d73d25f20a95c18103f327ab48f837a Mon Sep 17 00:00:00 2001 -From: Chris Coutinho -Date: Sun, 12 Oct 2025 21:09:29 +0200 -Subject: [PATCH 1/1] Fix Bearer token authentication causing session logout - -When using Bearer token authentication with OIDC, API requests to -endpoints with @CORS annotations (like Notes API) were failing with -401 Unauthorized errors. This occurred because: - -1. Bearer token validation successfully authenticated the user -2. A session was created for the authenticated user -3. Nextcloud's CORSMiddleware detected the logged-in session but no - CSRF token, causing it to call session->logout() -4. The logout invalidated the session, breaking the API request - -This fix sets the 'app_api' session flag during Bearer token -authentication, which instructs CORSMiddleware to skip the CSRF check -and logout logic. This is the same mechanism used by Nextcloud's -AppAPI framework for external application authentication. - -The flag is set at all successful Bearer token authentication points: -- Line 243: After OIDC Identity Provider validation -- Line 310: After auto-provisioning with bearer provisioning -- Line 315: After existing user authentication -- Line 337: After LDAP user sync - -Fixes: Bearer token authentication for all Nextcloud APIs -Tested-with: nextcloud-mcp-server integration tests -Signed-off-by: Chris Coutinho ---- - lib/User/Backend.php | 4 ++++ - 1 file changed, 4 insertions(+) - -diff --git a/lib/User/Backend.php b/lib/User/Backend.php -index 23cfb18..65665cc 100644 ---- a/lib/User/Backend.php -+++ b/lib/User/Backend.php -@@ -240,6 +240,7 @@ class Backend extends ABackend implements IPasswordConfirmationBackend, IGetDisp - $this->eventDispatcher->dispatchTyped($validationEvent); - $oidcProviderUserId = $validationEvent->getUserId(); - if ($oidcProviderUserId !== null) { -+ $this->session->set('app_api', true); - return $oidcProviderUserId; - } else { - $this->logger->debug('[NextcloudOidcProviderValidator] The bearer token validation has failed'); -@@ -306,10 +307,12 @@ class Backend extends ABackend implements IPasswordConfirmationBackend, IGetDisp - } - - $this->session->set('last-password-confirm', strtotime('+4 year', time())); -+ $this->session->set('app_api', true); - return $userId; - } elseif ($this->userExists($tokenUserId)) { - $this->checkFirstLogin($tokenUserId); - $this->session->set('last-password-confirm', strtotime('+4 year', time())); -+ $this->session->set('app_api', true); - return $tokenUserId; - } else { - // check if the user exists locally -@@ -331,6 +334,7 @@ class Backend extends ABackend implements IPasswordConfirmationBackend, IGetDisp - } - $this->checkFirstLogin($tokenUserId); - $this->session->set('last-password-confirm', strtotime('+4 year', time())); -+ $this->session->set('app_api', true); - return $tokenUserId; - } - } --- -2.51.0 - diff --git a/app-hooks/patches/cors-bearer-token.patch b/app-hooks/patches/cors-bearer-token.patch deleted file mode 100644 index 2186db9f..00000000 --- a/app-hooks/patches/cors-bearer-token.patch +++ /dev/null @@ -1,18 +0,0 @@ -diff --git a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php -index 4453f5a7d4b..f1ca9b48d21 100644 ---- a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php -+++ b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php -@@ -73,6 +73,13 @@ class CORSMiddleware extends Middleware { - $user = array_key_exists('PHP_AUTH_USER', $this->request->server) ? $this->request->server['PHP_AUTH_USER'] : null; - $pass = array_key_exists('PHP_AUTH_PW', $this->request->server) ? $this->request->server['PHP_AUTH_PW'] : null; - -+ // Allow Bearer token authentication for CORS requests -+ // Bearer tokens are stateless and don't require CSRF protection -+ $authorizationHeader = $this->request->getHeader('Authorization'); -+ if (!empty($authorizationHeader) && str_starts_with($authorizationHeader, 'Bearer ')) { -+ return; -+ } -+ - // Allow to use the current session if a CSRF token is provided - if ($this->request->passesCSRFCheck()) { - return; diff --git a/app-hooks/post-installation/20-apply-cors-bearer-token-patch.sh b/app-hooks/post-installation/20-apply-cors-bearer-token-patch.sh deleted file mode 100755 index f05fd9ee..00000000 --- a/app-hooks/post-installation/20-apply-cors-bearer-token-patch.sh +++ /dev/null @@ -1,64 +0,0 @@ -#!/bin/bash -# -# Apply upstream CORSMiddleware Bearer token authentication patch -# -# This patch allows Bearer tokens to bypass CORS/CSRF checks, fixing -# authentication issues with app-specific APIs (Notes, Calendar, etc.) -# when using OAuth/OIDC Bearer tokens. -# -# Upstream PR: https://github.com/nextcloud/server/pull/55878 -# Commit: 8fb5e77db82 (fix(cors): Allow Bearer token authentication) -# - -set -e - -PATCH_FILE="/docker-entrypoint-hooks.d/patches/cors-bearer-token.patch" -TARGET_FILE="/var/www/html/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php" - -echo "====================================================================" -echo "Applying CORSMiddleware Bearer token authentication patch..." -echo "====================================================================" - -# Check if patch file exists -if [ ! -f "$PATCH_FILE" ]; then - echo "⚠ Warning: Patch file not found: $PATCH_FILE" - echo " Skipping CORS Bearer token patch" - exit 0 -fi - -# Check if target file exists -if [ ! -f "$TARGET_FILE" ]; then - echo "⚠ Warning: Target file not found: $TARGET_FILE" - echo " Skipping CORS Bearer token patch" - exit 0 -fi - -# Check if already patched -if grep -q "Allow Bearer token authentication for CORS requests" "$TARGET_FILE"; then - echo "✓ CORSMiddleware already patched for Bearer token support" - exit 0 -fi - -echo "Applying patch to CORSMiddleware.php..." - -# Apply the patch -cd /var/www/html -if patch -p1 --dry-run < "$PATCH_FILE" > /dev/null 2>&1; then - patch -p1 < "$PATCH_FILE" - echo "✓ Patch applied successfully" -else - echo "⚠ Warning: Patch failed to apply (may already be applied or file changed)" - echo " This is expected if using a Nextcloud version that already includes the fix" - exit 0 -fi - -echo "" -echo "====================================================================" -echo "✓ CORSMiddleware Bearer token patch applied" -echo "====================================================================" -echo "" -echo "Benefits:" -echo " • Bearer tokens now work with app-specific APIs (Notes, Calendar, etc.)" -echo " • OAuth/OIDC authentication works without CORS errors" -echo " • Stateless API authentication is properly supported" -echo "" diff --git a/charts/nextcloud-mcp-server/.cz.toml b/charts/nextcloud-mcp-server/.cz.toml index 5f361ec5..a582b622 100644 --- a/charts/nextcloud-mcp-server/.cz.toml +++ b/charts/nextcloud-mcp-server/.cz.toml @@ -1,6 +1,6 @@ [tool.commitizen] name = "cz_conventional_commits" -version = "0.58.26" +version = "0.58.28" tag_format = "nextcloud-mcp-server-$version" version_scheme = "semver" update_changelog_on_bump = true diff --git a/charts/nextcloud-mcp-server/CHANGELOG.md b/charts/nextcloud-mcp-server/CHANGELOG.md index faa02a02..f0f149ae 100644 --- a/charts/nextcloud-mcp-server/CHANGELOG.md +++ b/charts/nextcloud-mcp-server/CHANGELOG.md @@ -14,6 +14,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Configurable resource limits - Grafana dashboard annotations +## nextcloud-mcp-server-0.58.28 (2026-04-05) + +### Fix + +- address PR review feedback for client registry and DCR proxy +- support cloud OAuth clients and graceful DCR fallback + +### Refactor + +- remove ALLOWED_MCP_CLOUD_CLIENTS and add keycloak CI profile +- consolidate ALLOWED_MCP_CLIENTS and add redirect URI validation + +## nextcloud-mcp-server-0.58.27 (2026-04-05) + ## nextcloud-mcp-server-0.58.26 (2026-04-04) ### Fix diff --git a/charts/nextcloud-mcp-server/Chart.yaml b/charts/nextcloud-mcp-server/Chart.yaml index 1815e1b8..1fc7425a 100644 --- a/charts/nextcloud-mcp-server/Chart.yaml +++ b/charts/nextcloud-mcp-server/Chart.yaml @@ -2,8 +2,8 @@ apiVersion: v2 name: nextcloud-mcp-server description: A Helm chart for Nextcloud MCP Server - enables AI assistants to interact with Nextcloud type: application -version: 0.58.26 -appVersion: "0.68.2" +version: 0.58.28 +appVersion: "0.68.3" keywords: - nextcloud - mcp diff --git a/docker-compose.yml b/docker-compose.yml index 3108404b..18154298 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -230,6 +230,10 @@ services: - ENABLE_TOKEN_EXCHANGE=true - TOKEN_EXCHANGE_CACHE_TTL=300 # Cache exchanged tokens for 5 minutes (default) + # Login Flow v2 (ADR-022) with external IdP + - ENABLE_LOGIN_FLOW=true + - ENABLE_DCR=true + # OAuth scopes (optional - uses defaults if not specified) - NEXTCLOUD_OIDC_SCOPES=openid profile email offline_access notes:read notes:write calendar:read calendar:write contacts:read contacts:write cookbook:read cookbook:write deck:read deck:write tables:read tables:write files:read files:write sharing:read sharing:write todo:read todo:write diff --git a/nextcloud_mcp_server/auth/client_registry.py b/nextcloud_mcp_server/auth/client_registry.py index 85541cab..af46828b 100644 --- a/nextcloud_mcp_server/auth/client_registry.py +++ b/nextcloud_mcp_server/auth/client_registry.py @@ -36,6 +36,12 @@ class ClientRegistry: 2. Integrate with IdP client registry 3. Store client metadata in database 4. Support client updates and revocation + + Scope Policy: + All clients are registered with allowed_scopes=["*"] (wildcard). + The MCP server acts as an OAuth AS proxy — it validates client + identity and redirect URIs locally, but delegates scope enforcement + to the upstream IdP (Nextcloud or Keycloak). """ def __init__(self, allow_dynamic_registration: bool = False): @@ -50,25 +56,72 @@ class ClientRegistry: self._load_static_clients() def _load_static_clients(self): - """Load statically configured clients from environment.""" - # Load from ALLOWED_MCP_CLIENTS environment variable + """Load statically configured clients from environment. + + Format: comma-separated entries, each either: + - Simple client ID: gets localhost redirect URIs + - client_id|redirect_uri: gets the specified redirect URI + + Redirect URI rules: + - http://localhost:* and http://127.0.0.1:* are allowed (native clients) + - https:// redirect URIs are allowed (cloud clients) + - http:// non-localhost redirect URIs are rejected with a warning + """ allowed_clients = os.getenv("ALLOWED_MCP_CLIENTS", "").strip() if allowed_clients: - # Parse comma-separated list - for client_id in allowed_clients.split(","): - client_id = client_id.strip() - if client_id: - # Create basic client info - # In production, would load full metadata from database - self._clients[client_id] = MCPClientInfo( - client_id=client_id, - name=self._get_client_name(client_id), - redirect_uris=["http://localhost:*", "http://127.0.0.1:*"], - allowed_scopes=["openid", "profile", "email", "mcp-server:api"], + for entry in allowed_clients.split(","): + entry = entry.strip() + if not entry: + continue + + if "|" in entry: + cid, redirect = entry.split("|", 1) + cid, redirect = cid.strip(), redirect.strip() + + if not cid or not redirect: + logger.warning( + f"Skipping malformed ALLOWED_MCP_CLIENTS entry: {entry!r}" + ) + continue + + parsed = urlparse(redirect) + hostname = parsed.hostname + if hostname is None: + logger.warning( + f"Skipping client {cid!r}: cannot parse hostname " + f"from {redirect!r}" + ) + continue + is_loopback = hostname in ("localhost", "127.0.0.1", "::1") + + if not ( + parsed.scheme == "https" + or (parsed.scheme == "http" and is_loopback) + ): + logger.warning( + f"Rejecting client {cid!r}: HTTP redirect URIs are only " + f"allowed for localhost, got {redirect!r}" + ) + continue + + self._clients[cid] = MCPClientInfo( + client_id=cid, + name=self._get_client_name(cid), + redirect_uris=[redirect], + allowed_scopes=["*"], is_public=True, ) - logger.info(f"Registered static client: {client_id}") + logger.info(f"Registered static client: {cid}") + else: + self._clients[entry] = MCPClientInfo( + client_id=entry, + name=self._get_client_name(entry), + redirect_uris=["http://localhost:*", "http://127.0.0.1:*"], + allowed_scopes=["*"], + is_public=True, + ) + logger.info(f"Registered static client: {entry}") # Add well-known clients if not explicitly configured if not self._clients: @@ -78,6 +131,7 @@ class ClientRegistry: """Get human-readable name for client_id.""" known_names = { "claude-desktop": "Claude Desktop", + "claude-ai": "Claude AI", "continue-dev": "Continue IDE Extension", "zed-editor": "Zed Editor", "vscode-mcp": "VS Code MCP Extension", @@ -92,7 +146,7 @@ class ClientRegistry: client_id="claude-desktop", name="Claude Desktop", redirect_uris=["http://localhost:*", "http://127.0.0.1:*"], - allowed_scopes=["openid", "profile", "email", "mcp-server:api"], + allowed_scopes=["*"], is_public=True, metadata={"vendor": "Anthropic"}, ), @@ -100,7 +154,7 @@ class ClientRegistry: client_id="test-mcp-client", name="Test MCP Client", redirect_uris=["http://localhost:*", "http://127.0.0.1:*"], - allowed_scopes=["openid", "profile", "email", "mcp-server:api"], + allowed_scopes=["*"], is_public=True, metadata={"purpose": "testing"}, ), @@ -163,6 +217,8 @@ class ClientRegistry: """ # Parse the redirect URI parsed = urlparse(redirect_uri) + if not parsed.hostname: + return False # Check against registered patterns for pattern in client.redirect_uris: @@ -171,7 +227,7 @@ class ClientRegistry: pattern_base = pattern.replace(":*", "") if redirect_uri.startswith(pattern_base + ":"): # Validate it's localhost with a port - if parsed.hostname in ["localhost", "127.0.0.1"]: + if parsed.hostname in ("localhost", "127.0.0.1", "::1"): return True elif redirect_uri == pattern: return True diff --git a/nextcloud_mcp_server/auth/oauth_routes.py b/nextcloud_mcp_server/auth/oauth_routes.py index 63beff2b..f8839789 100644 --- a/nextcloud_mcp_server/auth/oauth_routes.py +++ b/nextcloud_mcp_server/auth/oauth_routes.py @@ -1228,7 +1228,6 @@ async def oauth_register_proxy(request: Request) -> JSONResponse: ) oauth_config = oauth_ctx["config"] - nextcloud_host = oauth_config["nextcloud_host"] # Rate limit DCR requests per client IP client_ip = request.client.host if request.client else "unknown" @@ -1249,21 +1248,29 @@ async def oauth_register_proxy(request: Request) -> JSONResponse: timestamps.append(now) _dcr_rate_limit[client_ip] = timestamps - # Discover registration endpoint from OIDC discovery (prefer over hardcoded path) + # Discover registration endpoint from OIDC discovery discovery_url = oauth_config.get("discovery_url") + registration_endpoint = None if discovery_url: try: discovery = await _get_cached_discovery(discovery_url) - registration_endpoint = discovery.get( - "registration_endpoint", f"{nextcloud_host}/apps/oidc/register" - ) + registration_endpoint = discovery.get("registration_endpoint") except Exception: - logger.warning( - "Failed to fetch OIDC discovery for DCR endpoint, using fallback" - ) - registration_endpoint = f"{nextcloud_host}/apps/oidc/register" - else: - registration_endpoint = f"{nextcloud_host}/apps/oidc/register" + logger.warning("Failed to fetch OIDC discovery for DCR endpoint") + + if not registration_endpoint: + logger.warning( + "DCR proxy: Upstream IdP does not support dynamic client registration" + ) + return JSONResponse( + { + "error": "registration_not_supported", + "error_description": "The upstream identity provider does not support " + "dynamic client registration. Configure the client statically using " + "ALLOWED_MCP_CLIENTS.", + }, + status_code=400, + ) logger.info(f"DCR proxy: Forwarding registration to {registration_endpoint}") @@ -1276,7 +1283,7 @@ async def oauth_register_proxy(request: Request) -> JSONResponse: if response.status_code not in (200, 201): logger.error( - f"DCR proxy: Nextcloud registration failed: {response.status_code} {response.text}" + f"DCR proxy: Upstream registration failed: {response.status_code} {response.text}" ) return JSONResponse( response.json() diff --git a/pyproject.toml b/pyproject.toml index 048ad480..40282eda 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "nextcloud-mcp-server" -version = "0.68.2" +version = "0.68.3" description = "Model Context Protocol (MCP) server for Nextcloud integration - enables AI assistants to interact with Nextcloud data" authors = [ {name = "Chris Coutinho", email = "chris@coutinho.io"} diff --git a/tests/server/keycloak/__init__.py b/tests/server/keycloak/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/server/keycloak/test_keycloak_clients.py b/tests/server/keycloak/test_keycloak_clients.py new file mode 100644 index 00000000..d0467aaa --- /dev/null +++ b/tests/server/keycloak/test_keycloak_clients.py @@ -0,0 +1,218 @@ +""" +Integration tests for DCR and static OIDC clients against the mcp-keycloak service. + +The mcp-keycloak service uses Keycloak as an external IdP. Keycloak supports +Dynamic Client Registration (RFC 7591/7592), so the DCR proxy forwards +registrations to Keycloak and registers the resulting client locally. + +Static clients configured via ALLOWED_MCP_CLIENTS should work for the +authorization flow without DCR. + +Requires: docker compose --profile keycloak up --build -d +""" + +import base64 +import hashlib +import logging +import secrets + +import httpx +import pytest + +logger = logging.getLogger(__name__) + +pytestmark = [pytest.mark.integration, pytest.mark.keycloak] + +MCP_KEYCLOAK_BASE_URL = "http://localhost:8002" +KEYCLOAK_BASE_URL = "http://localhost:8888" +KEYCLOAK_REALM = "nextcloud-mcp" + + +@pytest.fixture(scope="module") +async def keycloak_mcp_available(): + """Check that the mcp-keycloak service is reachable and return AS metadata.""" + async with httpx.AsyncClient(timeout=10.0) as client: + try: + resp = await client.get( + f"{MCP_KEYCLOAK_BASE_URL}/.well-known/oauth-authorization-server" + ) + resp.raise_for_status() + return resp.json() + except (httpx.ConnectError, httpx.HTTPStatusError) as e: + pytest.skip(f"mcp-keycloak service not available: {e}") + + +@pytest.fixture() +async def dcr_client(keycloak_mcp_available): + """Register a client via DCR proxy and clean up after test.""" + async with httpx.AsyncClient(timeout=30.0) as http: + # Register via MCP proxy + resp = await http.post( + f"{MCP_KEYCLOAK_BASE_URL}/oauth/register", + json={ + "client_name": "test-dcr-keycloak", + "redirect_uris": ["http://localhost:9999/callback"], + "grant_types": ["authorization_code"], + "response_types": ["code"], + "token_endpoint_auth_method": "client_secret_basic", + "scope": "openid profile email", + }, + ) + resp.raise_for_status() + data = resp.json() + + yield data + + # Cleanup: delete via Keycloak directly (proxy URI uses internal hostname) + client_id = data.get("client_id") + rat = data.get("registration_access_token") + if client_id and rat: + async with httpx.AsyncClient(timeout=10.0) as http: + await http.delete( + f"{KEYCLOAK_BASE_URL}/realms/{KEYCLOAK_REALM}" + f"/clients-registrations/openid-connect/{client_id}", + headers={"Authorization": f"Bearer {rat}"}, + ) + + +# --- DCR tests --- + + +async def test_dcr_proxy_registers_client(dcr_client): + """DCR proxy should forward registration to Keycloak and return + RFC 7591 response with client credentials.""" + assert "client_id" in dcr_client + assert "client_secret" in dcr_client + assert dcr_client["client_name"] == "test-dcr-keycloak" + + +async def test_dcr_proxy_returns_rfc7592_fields(dcr_client): + """DCR response should include RFC 7592 management fields for + client lifecycle management.""" + assert "registration_access_token" in dcr_client + assert dcr_client["registration_access_token"] + assert "registration_client_uri" in dcr_client + assert dcr_client["registration_client_uri"] + + +async def test_dcr_client_accepted_by_authorize(keycloak_mcp_available, dcr_client): + """A DCR-registered client should be accepted by the authorization endpoint + (redirects to IdP login rather than returning an error).""" + # Generate PKCE challenge (required by the server) + verifier = secrets.token_urlsafe(64) + challenge = hashlib.sha256(verifier.encode()).digest() + code_challenge = base64.urlsafe_b64encode(challenge).rstrip(b"=").decode() + + async with httpx.AsyncClient(timeout=30.0, follow_redirects=False) as http: + resp = await http.get( + f"{MCP_KEYCLOAK_BASE_URL}/oauth/authorize", + params={ + "response_type": "code", + "client_id": dcr_client["client_id"], + "redirect_uri": "http://localhost:9999/callback", + "state": "test-state", + "scope": "openid", + "code_challenge": code_challenge, + "code_challenge_method": "S256", + }, + ) + + # Should redirect to IdP (302) not error (400) + assert resp.status_code == 302, ( + f"Expected redirect to IdP, got {resp.status_code}: {resp.text}" + ) + + +async def test_dcr_client_deletion_via_keycloak(keycloak_mcp_available): + """Full DCR lifecycle: register via proxy, verify, delete via RFC 7592.""" + async with httpx.AsyncClient(timeout=30.0) as http: + # Register + resp = await http.post( + f"{MCP_KEYCLOAK_BASE_URL}/oauth/register", + json={ + "client_name": "test-dcr-lifecycle", + "redirect_uris": ["http://localhost:9999/callback"], + "grant_types": ["authorization_code"], + "response_types": ["code"], + }, + ) + assert resp.status_code in (200, 201) + data = resp.json() + client_id = data["client_id"] + rat = data["registration_access_token"] + + try: + # Delete via Keycloak (external URL) + del_resp = await http.delete( + f"{KEYCLOAK_BASE_URL}/realms/{KEYCLOAK_REALM}" + f"/clients-registrations/openid-connect/{client_id}", + headers={"Authorization": f"Bearer {rat}"}, + ) + assert del_resp.status_code == 204 + except Exception: + # Best-effort cleanup if assertion failed + try: + await http.delete( + f"{KEYCLOAK_BASE_URL}/realms/{KEYCLOAK_REALM}" + f"/clients-registrations/openid-connect/{client_id}", + headers={"Authorization": f"Bearer {rat}"}, + ) + except Exception: + pass + raise + + +# --- AS metadata tests --- + + +async def test_as_metadata_advertises_registration_endpoint(keycloak_mcp_available): + """AS metadata should advertise /oauth/register for DCR discovery.""" + metadata = keycloak_mcp_available + assert "registration_endpoint" in metadata + assert metadata["registration_endpoint"].endswith("/oauth/register") + + +async def test_as_metadata_has_required_fields(keycloak_mcp_available): + """Verify the AS metadata contains all RFC 8414 required fields.""" + metadata = keycloak_mcp_available + required_fields = [ + "issuer", + "authorization_endpoint", + "token_endpoint", + "response_types_supported", + "grant_types_supported", + "code_challenge_methods_supported", + ] + for field in required_fields: + assert field in metadata, f"Missing required field: {field}" + + +# --- Static client / unknown client tests --- + + +async def test_authorize_rejects_unknown_client(keycloak_mcp_available): + """Authorization endpoint should reject client_ids that are neither + statically configured nor dynamically registered.""" + async with httpx.AsyncClient(timeout=30.0, follow_redirects=False) as http: + resp = await http.get( + f"{MCP_KEYCLOAK_BASE_URL}/oauth/authorize", + params={ + "response_type": "code", + "client_id": "nonexistent-client-id", + "redirect_uri": "http://localhost:9999/callback", + "state": "test-state", + "scope": "openid", + }, + ) + + # Should return an error (400 or redirect with error) + assert resp.status_code in (400, 302) + if resp.status_code == 400: + data = resp.json() + assert "error" in data + else: + # 302 redirect must carry an error parameter + location = resp.headers.get("location", "") + assert "error=" in location, ( + f"302 redirect should contain error= in Location, got: {location}" + ) diff --git a/tests/unit/test_client_registry.py b/tests/unit/test_client_registry.py new file mode 100644 index 00000000..30c5709b --- /dev/null +++ b/tests/unit/test_client_registry.py @@ -0,0 +1,200 @@ +"""Unit tests for ClientRegistry ALLOWED_MCP_CLIENTS parsing and validation.""" + +import logging + +import pytest + +import nextcloud_mcp_server.auth.client_registry as registry_mod + +pytestmark = pytest.mark.unit + + +@pytest.fixture(autouse=True) +def _reset_registry(): + """Reset the singleton registry before each test.""" + registry_mod._registry = None + yield + registry_mod._registry = None + + +def _get_registry(monkeypatch, value: str | None = None): + """Helper to create a registry with the given ALLOWED_MCP_CLIENTS value.""" + if value is not None: + monkeypatch.setenv("ALLOWED_MCP_CLIENTS", value) + else: + monkeypatch.delenv("ALLOWED_MCP_CLIENTS", raising=False) + return registry_mod.get_client_registry() + + +def test_simple_client_ids(monkeypatch): + registry = _get_registry(monkeypatch, "claude-desktop, zed-editor") + clients = registry.list_clients() + assert len(clients) == 2 + + claude = registry.get_client("claude-desktop") + assert claude is not None + assert claude.redirect_uris == ["http://localhost:*", "http://127.0.0.1:*"] + assert claude.allowed_scopes == ["*"] + + zed = registry.get_client("zed-editor") + assert zed is not None + assert zed.redirect_uris == ["http://localhost:*", "http://127.0.0.1:*"] + + +def test_pipe_separated_https(monkeypatch): + registry = _get_registry(monkeypatch, "myapp|https://app.example.com/callback") + client = registry.get_client("myapp") + assert client is not None + assert client.redirect_uris == ["https://app.example.com/callback"] + assert client.allowed_scopes == ["*"] + + +def test_pipe_separated_localhost(monkeypatch): + registry = _get_registry(monkeypatch, "dev-tool|http://localhost:3000/cb") + client = registry.get_client("dev-tool") + assert client is not None + assert client.redirect_uris == ["http://localhost:3000/cb"] + + +def test_pipe_separated_loopback_ip(monkeypatch): + registry = _get_registry(monkeypatch, "dev|http://127.0.0.1:9090/cb") + client = registry.get_client("dev") + assert client is not None + assert client.redirect_uris == ["http://127.0.0.1:9090/cb"] + + +def test_mixed_entries(monkeypatch): + registry = _get_registry( + monkeypatch, "claude-desktop, cloud-app|https://cloud.example.com/cb" + ) + clients = registry.list_clients() + assert len(clients) == 2 + + claude = registry.get_client("claude-desktop") + assert claude is not None + assert claude.redirect_uris == ["http://localhost:*", "http://127.0.0.1:*"] + + cloud = registry.get_client("cloud-app") + assert cloud is not None + assert cloud.redirect_uris == ["https://cloud.example.com/cb"] + + +def test_http_non_localhost_rejected(monkeypatch, caplog): + with caplog.at_level(logging.WARNING): + registry = _get_registry(monkeypatch, "bad-client|http://evil.com/cb") + + assert registry.get_client("bad-client") is None + assert "Rejecting client" in caplog.text + assert "evil.com" in caplog.text + + +def test_empty_string_uses_well_known(monkeypatch): + registry = _get_registry(monkeypatch, "") + clients = registry.list_clients() + client_ids = {c.client_id for c in clients} + assert "claude-desktop" in client_ids + assert "test-mcp-client" in client_ids + + +def test_unset_env_uses_well_known(monkeypatch): + registry = _get_registry(monkeypatch, None) + clients = registry.list_clients() + client_ids = {c.client_id for c in clients} + assert "claude-desktop" in client_ids + assert "test-mcp-client" in client_ids + + +def test_malformed_entries_skipped_with_warning(monkeypatch, caplog): + with caplog.at_level(logging.WARNING): + registry = _get_registry(monkeypatch, "good, |, , bad|") + + # Only "good" should be registered + assert registry.get_client("good") is not None + assert len(registry.list_clients()) == 1 + assert "malformed" in caplog.text.lower() + + +def test_all_scopes_wildcard(monkeypatch): + registry = _get_registry(monkeypatch, "test-client") + client = registry.get_client("test-client") + assert client is not None + assert client.allowed_scopes == ["*"] + + +def test_validate_client_wildcard_scopes(monkeypatch): + registry = _get_registry(monkeypatch, "test-client") + valid, err = registry.validate_client( + "test-client", scopes=["anything", "goes", "here"] + ) + assert valid is True + assert err is None + + +def test_validate_redirect_uri_https_match(monkeypatch): + registry = _get_registry(monkeypatch, "cloud|https://x.com/cb") + valid, err = registry.validate_client("cloud", redirect_uri="https://x.com/cb") + assert valid is True + assert err is None + + +def test_validate_redirect_uri_https_mismatch(monkeypatch): + registry = _get_registry(monkeypatch, "cloud|https://x.com/cb") + valid, err = registry.validate_client("cloud", redirect_uri="https://other.com/cb") + assert valid is False + assert "redirect_uri" in err.lower() + + +def test_validate_redirect_uri_localhost_wildcard(monkeypatch): + registry = _get_registry(monkeypatch, "native-client") + valid, err = registry.validate_client( + "native-client", redirect_uri="http://localhost:12345/callback" + ) + assert valid is True + assert err is None + + +def test_well_known_clients_wildcard_scopes(monkeypatch): + registry = _get_registry(monkeypatch, None) + for client in registry.list_clients(): + assert client.allowed_scopes == ["*"], ( + f"Well-known client {client.client_id} should have wildcard scopes" + ) + + +def test_client_name_resolution(monkeypatch): + registry = _get_registry(monkeypatch, "claude-desktop, custom-tool") + assert registry.get_client("claude-desktop").name == "Claude Desktop" + assert registry.get_client("custom-tool").name == "Custom Tool" + + +def test_ipv6_loopback_allowed(monkeypatch): + registry = _get_registry(monkeypatch, "ipv6-app|http://[::1]:3000/cb") + client = registry.get_client("ipv6-app") + assert client is not None + assert client.redirect_uris == ["http://[::1]:3000/cb"] + + +def test_malformed_uri_no_hostname_skipped(monkeypatch, caplog): + with caplog.at_level(logging.WARNING): + registry = _get_registry(monkeypatch, "bad|http:///no-host") + + assert registry.get_client("bad") is None + assert "cannot parse hostname" in caplog.text + + +def test_validate_redirect_uri_ipv6_loopback(monkeypatch): + """IPv6 loopback redirect URIs should match wildcard localhost patterns.""" + registry = _get_registry(monkeypatch, "ipv6-app|http://[::1]:3000/cb") + valid, err = registry.validate_client( + "ipv6-app", redirect_uri="http://[::1]:3000/cb" + ) + assert valid is True + assert err is None + + +def test_validate_redirect_uri_no_hostname(monkeypatch): + """Redirect URIs with no parseable hostname should be rejected.""" + registry = _get_registry(monkeypatch, "test-client") + valid, err = registry.validate_client("test-client", redirect_uri="not-a-uri") + assert valid is False + assert "redirect_uri" in err.lower() diff --git a/tests/unit/test_dcr_proxy.py b/tests/unit/test_dcr_proxy.py new file mode 100644 index 00000000..07eed461 --- /dev/null +++ b/tests/unit/test_dcr_proxy.py @@ -0,0 +1,65 @@ +"""Unit tests for DCR proxy registration_not_supported path.""" + +import json +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from nextcloud_mcp_server.auth.oauth_routes import oauth_register_proxy + +pytestmark = pytest.mark.unit + + +def _make_request(body: dict, oauth_config: dict) -> MagicMock: + """Create a mock Starlette Request.""" + request = AsyncMock() + request.json = AsyncMock(return_value=body) + request.client = MagicMock() + request.client.host = "127.0.0.1" + request.app = MagicMock() + request.app.state.oauth_context = {"config": oauth_config} + return request + + +_DCR_BODY = { + "client_name": "test", + "redirect_uris": ["http://localhost:9999/cb"], +} + + +async def test_registration_not_supported_when_no_endpoint(): + """When discovery doc lacks registration_endpoint, return 400.""" + request = _make_request( + body=_DCR_BODY, + oauth_config={ + "discovery_url": "https://idp.example.com/.well-known/openid-configuration" + }, + ) + + discovery_doc = { + "issuer": "https://idp.example.com", + "authorization_endpoint": "https://idp.example.com/auth", + } + + with patch( + "nextcloud_mcp_server.auth.oauth_routes._get_cached_discovery", + new_callable=AsyncMock, + return_value=discovery_doc, + ): + response = await oauth_register_proxy(request) + + assert response.status_code == 400 + body = json.loads(response.body) + assert body["error"] == "registration_not_supported" + assert "ALLOWED_MCP_CLIENTS" in body["error_description"] + + +async def test_registration_not_supported_when_no_discovery_url(): + """When no discovery_url is configured, return 400.""" + request = _make_request(body=_DCR_BODY, oauth_config={}) + + response = await oauth_register_proxy(request) + + assert response.status_code == 400 + body = json.loads(response.body) + assert body["error"] == "registration_not_supported" diff --git a/uv.lock b/uv.lock index 47266afd..a1b1bfcc 100644 --- a/uv.lock +++ b/uv.lock @@ -2095,7 +2095,7 @@ wheels = [ [[package]] name = "nextcloud-mcp-server" -version = "0.68.2" +version = "0.68.3" source = { editable = "." } dependencies = [ { name = "aiosqlite" },