Merge pull request #745 from cbcoutinho/feat/strict-auth-allowlists

feat(auth): drop test-client defaults, add ALLOWED_MGMT_CLIENT allowlist

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

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)
 # ============================================

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,

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]
 
+        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
+        if access_token is None:
-        return await self._verify_without_audience_check(token, cache_key)
+            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:
         """

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):
+def test_empty_string_yields_empty_registry(monkeypatch, caplog):
+    with caplog.at_level(logging.WARNING):
         registry = _get_registry(monkeypatch, "")
-    clients = registry.list_clients()
+    assert registry.list_clients() == []
-    client_ids = {c.client_id for c in clients}
+    valid, err = registry.validate_client("claude-desktop")
-    assert "claude-desktop" in client_ids
+    assert valid is False
-    assert "test-mcp-client" in client_ids
+    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):
+def test_unset_env_yields_empty_registry(monkeypatch, caplog):
+    with caplog.at_level(logging.WARNING):
         registry = _get_registry(monkeypatch, None)
-    clients = registry.list_clients()
+    assert registry.list_clients() == []
-    client_ids = {c.client_id for c in clients}
+    valid, err = registry.validate_client("test-mcp-client")
-    assert "claude-desktop" in client_ids
+    assert valid is False
-    assert "test-mcp-client" in client_ids
+    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"

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