refactor: consolidate ALLOWED_MCP_CLIENTS and add redirect URI validation
Merge ALLOWED_MCP_CLOUD_CLIENTS into a single ALLOWED_MCP_CLIENTS env var that supports both simple client IDs and pipe-separated client_id|redirect_uri entries. Enforce HTTPS for non-localhost redirect URIs, warn on malformed entries, and use wildcard scopes for all static clients (upstream IdP enforces actual scopes). Add deprecation warning for the old env var. Also fixes DCR proxy error messages to reference only ALLOWED_MCP_CLIENTS and use "Upstream" instead of "Nextcloud" for IdP-agnostic language. Enables Login Flow v2 + DCR on the mcp-keycloak docker-compose service. Adds 17 unit tests for ClientRegistry parsing/validation and 7 keycloak integration tests for DCR lifecycle, AS metadata, and client authorization. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
co-authored by
Claude Opus 4.6
Claude Opus 4.6
parent
2a34015443
commit
91e7665f41
@@ -1,69 +0,0 @@
|
||||
From deab2dac3d73d25f20a95c18103f327ab48f837a Mon Sep 17 00:00:00 2001
|
||||
From: Chris Coutinho <chris@coutinho.io>
|
||||
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 <chris@coutinho.io>
|
||||
---
|
||||
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
|
||||
|
||||
@@ -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;
|
||||
@@ -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 ""
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -50,35 +50,57 @@ 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
|
||||
"""
|
||||
# Deprecation warning for old env var
|
||||
if os.getenv("ALLOWED_MCP_CLOUD_CLIENTS"):
|
||||
logger.warning(
|
||||
"ALLOWED_MCP_CLOUD_CLIENTS is deprecated. "
|
||||
"Merge entries into ALLOWED_MCP_CLIENTS using the format: "
|
||||
"client_id|https://redirect-uri"
|
||||
)
|
||||
|
||||
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"],
|
||||
is_public=True,
|
||||
)
|
||||
logger.info(f"Registered static client: {client_id}")
|
||||
|
||||
# Load cloud clients (web-based, HTTPS redirect URIs)
|
||||
# Format: "client_id|redirect_uri,client_id2|redirect_uri2"
|
||||
cloud_clients = os.getenv("ALLOWED_MCP_CLOUD_CLIENTS", "").strip()
|
||||
if cloud_clients:
|
||||
for entry in cloud_clients.split(","):
|
||||
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)
|
||||
is_loopback = parsed.hostname in ("localhost", "127.0.0.1")
|
||||
|
||||
if parsed.scheme == "https":
|
||||
pass # HTTPS always allowed
|
||||
elif parsed.scheme == "http" and is_loopback:
|
||||
pass # HTTP localhost allowed
|
||||
else:
|
||||
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),
|
||||
@@ -86,7 +108,16 @@ class ClientRegistry:
|
||||
allowed_scopes=["*"],
|
||||
is_public=True,
|
||||
)
|
||||
logger.info(f"Registered cloud client: {cid}")
|
||||
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:
|
||||
@@ -111,7 +142,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"},
|
||||
),
|
||||
@@ -119,7 +150,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"},
|
||||
),
|
||||
|
||||
@@ -1267,7 +1267,7 @@ async def oauth_register_proxy(request: Request) -> 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 or ALLOWED_MCP_CLOUD_CLIENTS.",
|
||||
"ALLOWED_MCP_CLIENTS.",
|
||||
},
|
||||
status_code=400,
|
||||
)
|
||||
@@ -1283,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()
|
||||
|
||||
@@ -0,0 +1,200 @@
|
||||
"""
|
||||
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"]
|
||||
|
||||
# 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
|
||||
|
||||
|
||||
# --- 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
|
||||
@@ -0,0 +1,177 @@
|
||||
"""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)
|
||||
monkeypatch.delenv("ALLOWED_MCP_CLOUD_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_deprecated_cloud_clients_warning(monkeypatch, caplog):
|
||||
monkeypatch.setenv("ALLOWED_MCP_CLOUD_CLIENTS", "old|https://old.com/cb")
|
||||
monkeypatch.setenv("ALLOWED_MCP_CLIENTS", "new-client")
|
||||
with caplog.at_level(logging.WARNING):
|
||||
registry_mod.get_client_registry()
|
||||
|
||||
assert "ALLOWED_MCP_CLOUD_CLIENTS is deprecated" in caplog.text
|
||||
|
||||
|
||||
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"
|
||||
Reference in New Issue
Block a user