diff --git a/docker-compose.yml b/docker-compose.yml index 62fa8e02..8f3bda13 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -168,6 +168,11 @@ services: # - NEXTCLOUD_OIDC_CLIENT_ID=your_client_id # - NEXTCLOUD_OIDC_CLIENT_SECRET=your_client_secret + # Management API allowlist (ADR-018): only Astrolabe-issued tokens may + # call /api/management/*. Default test client_id used by the + # configure_astrolabe_for_mcp_server fixture. + - ALLOWED_MGMT_CLIENT=nextcloudMcpServerUIPublicClient + # NO admin credentials - credentials come from client Authorization header volumes: - multi-user-basic-data:/app/data diff --git a/env.sample b/env.sample index 07a0d247..652d9435 100644 --- a/env.sample +++ b/env.sample @@ -119,6 +119,31 @@ NEXTCLOUD_PASSWORD= # Optional features (semantic search, document processing): # See "Optional Features" section below +# ============================================ +# OAUTH CLIENT ALLOWLISTS (OAuth modes) +# ============================================ +# Both env vars are FAIL-CLOSED: if unset/empty, the corresponding auth route +# rejects every request. There are no built-in defaults. +# +# Note: ALLOWED_MCP_CLIENTS and ALLOWED_MGMT_CLIENT are currently separate to +# keep the MCP-route and management-API auth surfaces independent. They may be +# consolidated into a single env var later. + +# ===== ALLOWED_MCP_CLIENTS ===== +# Clients allowed to use the OAuth AS proxy (/oauth/authorize, /oauth/token). +# Comma-separated. Each entry is either: +# - "client_id" → loopback redirect URIs (http://localhost:*, http://127.0.0.1:*) +# - "client_id|https://app/cb" → bind to a specific HTTPS callback +# HTTP redirect URIs are rejected unless the host is loopback (localhost/127.0.0.1/::1). +#ALLOWED_MCP_CLIENTS=claude-desktop, zed-editor, cloud-app|https://cloud.example.com/cb + +# ===== ALLOWED_MGMT_CLIENT ===== +# OIDC client_ids whose tokens are accepted by the management API +# (/api/management/*). Comma-separated. The token's `client_id` claim must +# match one of these entries. Typical value: the Astrolabe NC PHP app's +# OAuth client_id (ADR-018). +#ALLOWED_MGMT_CLIENT=astrolabe + # ============================================ # OPTIONAL FEATURES (All Deployment Modes) # ============================================ diff --git a/nextcloud_mcp_server/auth/client_registry.py b/nextcloud_mcp_server/auth/client_registry.py index af46828b..1a9e317c 100644 --- a/nextcloud_mcp_server/auth/client_registry.py +++ b/nextcloud_mcp_server/auth/client_registry.py @@ -66,7 +66,15 @@ class ClientRegistry: - 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 + + If the env var is unset or empty, the registry remains empty and + ``validate_client`` rejects every client_id (fail-closed). There are no + built-in defaults; operators must opt-in clients explicitly. """ + # NOTE: ALLOWED_MCP_CLIENTS and ALLOWED_MGMT_CLIENT are currently separate + # env vars to keep the MCP-route and management-API auth surfaces + # independent. These may be consolidated into a single env var later + # once the deployment story stabilises. allowed_clients = os.getenv("ALLOWED_MCP_CLIENTS", "").strip() if allowed_clients: @@ -123,9 +131,11 @@ class ClientRegistry: ) logger.info(f"Registered static client: {entry}") - # Add well-known clients if not explicitly configured if not self._clients: - self._add_well_known_clients() + logger.warning( + "Client registry is empty: ALLOWED_MCP_CLIENTS is unset or empty. " + "All MCP-flow OAuth requests will be rejected until configured." + ) def _get_client_name(self, client_id: str) -> str: """Get human-readable name for client_id.""" @@ -135,35 +145,9 @@ class ClientRegistry: "continue-dev": "Continue IDE Extension", "zed-editor": "Zed Editor", "vscode-mcp": "VS Code MCP Extension", - "test-mcp-client": "Test MCP Client", } return known_names.get(client_id, client_id.replace("-", " ").title()) - def _add_well_known_clients(self): - """Add well-known MCP clients for testing and development.""" - well_known = [ - MCPClientInfo( - client_id="claude-desktop", - name="Claude Desktop", - redirect_uris=["http://localhost:*", "http://127.0.0.1:*"], - allowed_scopes=["*"], - is_public=True, - metadata={"vendor": "Anthropic"}, - ), - MCPClientInfo( - client_id="test-mcp-client", - name="Test MCP Client", - redirect_uris=["http://localhost:*", "http://127.0.0.1:*"], - allowed_scopes=["*"], - is_public=True, - metadata={"purpose": "testing"}, - ), - ] - - for client in well_known: - self._clients[client.client_id] = client - logger.info(f"Registered well-known client: {client.client_id}") - def validate_client( self, client_id: str, diff --git a/nextcloud_mcp_server/auth/unified_verifier.py b/nextcloud_mcp_server/auth/unified_verifier.py index fbcc9ff2..5f64f689 100644 --- a/nextcloud_mcp_server/auth/unified_verifier.py +++ b/nextcloud_mcp_server/auth/unified_verifier.py @@ -17,6 +17,7 @@ Key Design Principles: import hashlib import logging +import os import time from typing import Any @@ -97,6 +98,25 @@ class UnifiedTokenVerifier(TokenVerifier): self._token_cache: dict[str, tuple[dict[str, Any], float]] = {} self.cache_ttl = 3600 # 1 hour default + # NOTE: ALLOWED_MCP_CLIENTS and ALLOWED_MGMT_CLIENT are currently separate + # env vars to keep the MCP-route and management-API auth surfaces + # independent. These may be consolidated into a single env var later + # once the deployment story stabilises. + self._allowed_mgmt_clients: frozenset[str] = frozenset( + entry.strip() + for entry in os.getenv("ALLOWED_MGMT_CLIENT", "").split(",") + if entry.strip() + ) + if not self._allowed_mgmt_clients: + logger.warning( + "ALLOWED_MGMT_CLIENT is unset or empty: management API will reject " + "all requests until configured." + ) + else: + logger.info( + f"Management API allowlist: {sorted(self._allowed_mgmt_clients)}" + ) + logger.info( f"UnifiedTokenVerifier initialized in {self.mode} mode. " f"MCP audience: {settings.oidc_client_id} or {settings.nextcloud_mcp_server_url}, " @@ -169,10 +189,11 @@ class UnifiedTokenVerifier(TokenVerifier): token: Bearer token to verify Returns: - AccessToken if valid (regardless of audience), None otherwise + AccessToken if valid AND issued by an allowlisted client, None otherwise """ # Check cache first (using separate cache key to avoid mixing with MCP tokens) cache_key = f"mgmt:{hashlib.sha256(token.encode()).hexdigest()}" + access_token: AccessToken | None = None if cache_key in self._token_cache: userinfo, expiry = self._token_cache[cache_key] if time.time() < expiry: @@ -181,7 +202,7 @@ class UnifiedTokenVerifier(TokenVerifier): username = userinfo.get("sub") or userinfo.get("preferred_username") scope_string = userinfo.get("scope", "") scopes = scope_string.split() if scope_string else [] - return AccessToken( + access_token = AccessToken( token=token, client_id=userinfo.get("client_id", ""), scopes=scopes, @@ -191,10 +212,23 @@ class UnifiedTokenVerifier(TokenVerifier): else: del self._token_cache[cache_key] - oauth_token_cache_hits_total.labels(hit="false").inc() + if access_token is None: + oauth_token_cache_hits_total.labels(hit="false").inc() + access_token = await self._verify_without_audience_check(token, cache_key) - # Verify token without audience check - return await self._verify_without_audience_check(token, cache_key) + if access_token is None: + return None + + # Enforce ALLOWED_MGMT_CLIENT allowlist (fail-closed when unset) + token_client_id = access_token.client_id + if not token_client_id or token_client_id not in self._allowed_mgmt_clients: + logger.warning( + "Management API token rejected: client_id %r not in ALLOWED_MGMT_CLIENT", + token_client_id, + ) + return None + + return access_token async def _verify_mcp_audience(self, token: str) -> AccessToken | None: """ diff --git a/tests/unit/test_client_registry.py b/tests/unit/test_client_registry.py index 30c5709b..75d27308 100644 --- a/tests/unit/test_client_registry.py +++ b/tests/unit/test_client_registry.py @@ -88,20 +88,23 @@ def test_http_non_localhost_rejected(monkeypatch, caplog): 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_empty_string_yields_empty_registry(monkeypatch, caplog): + with caplog.at_level(logging.WARNING): + registry = _get_registry(monkeypatch, "") + assert registry.list_clients() == [] + valid, err = registry.validate_client("claude-desktop") + assert valid is False + assert "Unknown client_id" in err + assert "ALLOWED_MCP_CLIENTS is unset or empty" in caplog.text -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_unset_env_yields_empty_registry(monkeypatch, caplog): + with caplog.at_level(logging.WARNING): + registry = _get_registry(monkeypatch, None) + assert registry.list_clients() == [] + valid, err = registry.validate_client("test-mcp-client") + assert valid is False + assert "Unknown client_id" in err def test_malformed_entries_skipped_with_warning(monkeypatch, caplog): @@ -153,14 +156,6 @@ def test_validate_redirect_uri_localhost_wildcard(monkeypatch): 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" diff --git a/tests/unit/test_unified_verifier.py b/tests/unit/test_unified_verifier.py index 7747bbde..67b3cd0d 100644 --- a/tests/unit/test_unified_verifier.py +++ b/tests/unit/test_unified_verifier.py @@ -512,3 +512,114 @@ class TestVerifyTokenFlow: result = await verifier.verify_token("opaque-token") assert result is not None assert result.resource == "testuser" + + +class TestManagementApiAllowlist: + """Test ALLOWED_MGMT_CLIENT enforcement in verify_token_for_management_api.""" + + @staticmethod + def _underlying_token(client_id: str = "astrolabe"): + from mcp.server.auth.provider import AccessToken + + return AccessToken( + token="t", + client_id=client_id, + scopes=["openid"], + expires_at=int(time.time() + 3600), + resource="testuser", + ) + + async def test_unset_allowlist_rejects_all(self, monkeypatch, base_settings): + monkeypatch.delenv("ALLOWED_MGMT_CLIENT", raising=False) + verifier = UnifiedTokenVerifier(base_settings) + assert verifier._allowed_mgmt_clients == frozenset() + + with patch.object( + verifier, + "_verify_without_audience_check", + return_value=self._underlying_token("astrolabe"), + ): + result = await verifier.verify_token_for_management_api("any-token") + assert result is None + + async def test_empty_allowlist_rejects_all(self, monkeypatch, base_settings): + monkeypatch.setenv("ALLOWED_MGMT_CLIENT", " , ,") + verifier = UnifiedTokenVerifier(base_settings) + assert verifier._allowed_mgmt_clients == frozenset() + + with patch.object( + verifier, + "_verify_without_audience_check", + return_value=self._underlying_token("astrolabe"), + ): + result = await verifier.verify_token_for_management_api("any-token") + assert result is None + + async def test_allowlisted_client_accepted(self, monkeypatch, base_settings): + monkeypatch.setenv("ALLOWED_MGMT_CLIENT", "astrolabe, admin-tool") + verifier = UnifiedTokenVerifier(base_settings) + assert verifier._allowed_mgmt_clients == {"astrolabe", "admin-tool"} + + underlying = self._underlying_token("astrolabe") + with patch.object( + verifier, "_verify_without_audience_check", return_value=underlying + ): + result = await verifier.verify_token_for_management_api("any-token") + assert result is underlying + + async def test_non_allowlisted_client_rejected(self, monkeypatch, base_settings): + monkeypatch.setenv("ALLOWED_MGMT_CLIENT", "astrolabe") + verifier = UnifiedTokenVerifier(base_settings) + + with patch.object( + verifier, + "_verify_without_audience_check", + return_value=self._underlying_token("some-other-client"), + ): + result = await verifier.verify_token_for_management_api("any-token") + assert result is None + + async def test_token_missing_client_id_rejected(self, monkeypatch, base_settings): + monkeypatch.setenv("ALLOWED_MGMT_CLIENT", "astrolabe") + verifier = UnifiedTokenVerifier(base_settings) + + with patch.object( + verifier, + "_verify_without_audience_check", + return_value=self._underlying_token(""), + ): + result = await verifier.verify_token_for_management_api("any-token") + assert result is None + + async def test_underlying_verification_failure_propagates( + self, monkeypatch, base_settings + ): + monkeypatch.setenv("ALLOWED_MGMT_CLIENT", "astrolabe") + verifier = UnifiedTokenVerifier(base_settings) + + with patch.object( + verifier, "_verify_without_audience_check", return_value=None + ): + result = await verifier.verify_token_for_management_api("any-token") + assert result is None + + async def test_cache_hit_also_enforces_allowlist(self, monkeypatch, base_settings): + """A previously-cached token must still be re-checked against the allowlist.""" + import hashlib + + monkeypatch.setenv("ALLOWED_MGMT_CLIENT", "astrolabe") + verifier = UnifiedTokenVerifier(base_settings) + + token = "cached-token" + cache_key = f"mgmt:{hashlib.sha256(token.encode()).hexdigest()}" + verifier._token_cache[cache_key] = ( + { + "sub": "testuser", + "scope": "openid", + "client_id": "not-allowlisted", + }, + time.time() + 3600, + ) + + result = await verifier.verify_token_for_management_api(token) + assert result is None