diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index cd877b8..3de462f 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -177,7 +177,7 @@ jobs: echo "MCP service is ready on port ${{ matrix.wait-port }}." - name: Verify OIDC configuration - if: matrix.needs-playwright + if: matrix.mode == 'oauth' || matrix.mode == 'login-flow' run: | echo "=== OIDC Discovery ===" curl -s http://localhost:8080/.well-known/openid-configuration | jq . diff --git a/nextcloud_mcp_server/api/access.py b/nextcloud_mcp_server/api/access.py index e3cd077..3dfba83 100644 --- a/nextcloud_mcp_server/api/access.py +++ b/nextcloud_mcp_server/api/access.py @@ -14,6 +14,7 @@ from nextcloud_mcp_server.api.passwords import ( _extract_basic_auth, _get_app_password_storage, ) +from nextcloud_mcp_server.auth.scope_authorization import invalidate_scope_cache from nextcloud_mcp_server.models.auth import ALL_SUPPORTED_SCOPES logger = logging.getLogger(__name__) @@ -79,6 +80,11 @@ async def update_user_scopes(request: Request) -> JSONResponse: This only updates the stored scopes, not the app password itself. The app password remains valid; scope enforcement is application-level. + + Security note: This endpoint allows direct scope modification without + re-authenticating via Login Flow. The caller must authenticate with + valid BasicAuth credentials (user_id + app_password), which serves + as the authorization check. """ path_user_id = request.path_params.get("user_id") if not path_user_id: @@ -113,7 +119,7 @@ async def update_user_scopes(request: Request) -> JSONResponse: { "success": False, "error": f"Invalid scopes: {', '.join(invalid)}", - "valid_scopes": ALL_SUPPORTED_SCOPES, + "valid_scopes": sorted(ALL_SUPPORTED_SCOPES), }, status_code=400, ) @@ -137,6 +143,9 @@ async def update_user_scopes(request: Request) -> JSONResponse: scopes=scopes, ) + # Invalidate scope cache so subsequent tool calls see updated scopes + invalidate_scope_cache(username) + return JSONResponse( { "success": True, @@ -159,6 +168,6 @@ async def list_supported_scopes(_: Request) -> JSONResponse: return JSONResponse( { "success": True, - "scopes": ALL_SUPPORTED_SCOPES, + "scopes": sorted(ALL_SUPPORTED_SCOPES), } ) diff --git a/nextcloud_mcp_server/app.py b/nextcloud_mcp_server/app.py index 190dc3a..8a8523c 100644 --- a/nextcloud_mcp_server/app.py +++ b/nextcloud_mcp_server/app.py @@ -1532,13 +1532,19 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = mcp_app = mcp.streamable_http_app() async def _login_flow_cleanup_loop() -> None: - """Periodically clean up expired Login Flow v2 sessions.""" + """Periodically clean up expired Login Flow v2 sessions and proxy codes.""" + from nextcloud_mcp_server.auth.oauth_routes import ( # noqa: PLC0415 + _cleanup_expired_proxy_codes, + ) + while True: try: storage = await get_shared_storage() count = await storage.delete_expired_login_flow_sessions() if count: logger.info(f"Cleaned up {count} expired login flow sessions") + # Also clean up expired AS proxy codes/sessions + _cleanup_expired_proxy_codes() except Exception as e: logger.warning(f"Login flow cleanup error: {e}") await anyio.sleep(3600) # Every hour diff --git a/nextcloud_mcp_server/auth/client_registration.py b/nextcloud_mcp_server/auth/client_registration.py index 9831144..7f1a9fc 100644 --- a/nextcloud_mcp_server/auth/client_registration.py +++ b/nextcloud_mcp_server/auth/client_registration.py @@ -83,6 +83,7 @@ async def register_client( scopes: str = "openid profile email", token_type: str | None = "Bearer", resource_url: str | None = None, + max_retries: int = 3, ) -> ClientInfo: """ Register a new OAuth client using RFC 7591 Dynamic Client Registration. @@ -98,6 +99,7 @@ async def register_client( token_type: Type of access tokens (default: "Bearer", supports "JWT" for Nextcloud). Set to None to omit this field (required for Keycloak and other standard providers). resource_url: OAuth 2.0 Protected Resource URL (RFC 9728) - used for token introspection authorization + max_retries: Maximum number of retries for 429 responses (default: 3) Returns: ClientInfo with registration details @@ -135,57 +137,91 @@ async def register_client( logger.debug(f"Registration endpoint: {registration_endpoint}") async with nextcloud_httpx_client(timeout=30.0) as client: - try: - response = await client.post( - registration_endpoint, - json=client_metadata, - headers={"Content-Type": "application/json"}, - ) - response.raise_for_status() - - client_info = response.json() - logger.info( - f"Successfully registered client: {client_info.get('client_id')}" - ) - expires_at = dt.datetime.fromtimestamp( - client_info.get("client_secret_expires_at") - ) - logger.info( - f"Client expires at: {expires_at} " - f"(in {client_info.get('client_secret_expires_at', 0) - int(time.time())} seconds)" - ) - - # Log if RFC 7592 fields are present - has_reg_token = "registration_access_token" in client_info - has_reg_uri = "registration_client_uri" in client_info - if has_reg_token and has_reg_uri: - logger.info( - "RFC 7592 management fields received - client deletion will be supported" + for attempt in range(max_retries): + try: + response = await client.post( + registration_endpoint, + json=client_metadata, + headers={"Content-Type": "application/json"}, ) - else: - logger.warning("RFC 7592 fields missing - client deletion may not work") - return ClientInfo( - client_id=client_info["client_id"], - client_secret=client_info["client_secret"], - client_id_issued_at=client_info.get( - "client_id_issued_at", int(time.time()) - ), - client_secret_expires_at=client_info.get( - "client_secret_expires_at", int(time.time()) + 3600 - ), - redirect_uris=client_info.get("redirect_uris", redirect_uris), - registration_access_token=client_info.get("registration_access_token"), - registration_client_uri=client_info.get("registration_client_uri"), - ) + if response.status_code == 429: + # Rate limited - retry with exponential backoff + if attempt < max_retries - 1: + retry_after = int(response.headers.get("Retry-After", 2)) + wait_time = min(retry_after, 2**attempt) + logger.warning( + f"Rate limited (429) registering client, " + f"retrying in {wait_time}s (attempt {attempt + 1}/{max_retries})" + ) + await anyio.sleep(wait_time) + continue + else: + logger.error( + f"Failed to register client after {max_retries} attempts: Rate limited (429)" + ) + response.raise_for_status() - except httpx.HTTPStatusError as e: - logger.error(f"Failed to register client: HTTP {e.response.status_code}") - logger.error(f"Response: {e.response.text}") - raise - except KeyError as e: - logger.error(f"Invalid response from registration endpoint: missing {e}") - raise ValueError(f"Invalid registration response: missing {e}") + response.raise_for_status() + + client_info = response.json() + logger.info( + f"Successfully registered client: {client_info.get('client_id')}" + ) + expires_at = dt.datetime.fromtimestamp( + client_info.get("client_secret_expires_at") + ) + logger.info( + f"Client expires at: {expires_at} " + f"(in {client_info.get('client_secret_expires_at', 0) - int(time.time())} seconds)" + ) + + # Log if RFC 7592 fields are present + has_reg_token = "registration_access_token" in client_info + has_reg_uri = "registration_client_uri" in client_info + if has_reg_token and has_reg_uri: + logger.info( + "RFC 7592 management fields received - client deletion will be supported" + ) + else: + logger.warning( + "RFC 7592 fields missing - client deletion may not work" + ) + + return ClientInfo( + client_id=client_info["client_id"], + client_secret=client_info["client_secret"], + client_id_issued_at=client_info.get( + "client_id_issued_at", int(time.time()) + ), + client_secret_expires_at=client_info.get( + "client_secret_expires_at", int(time.time()) + 3600 + ), + redirect_uris=client_info.get("redirect_uris", redirect_uris), + registration_access_token=client_info.get( + "registration_access_token" + ), + registration_client_uri=client_info.get("registration_client_uri"), + ) + + except httpx.HTTPStatusError as e: + logger.error( + f"Failed to register client: HTTP {e.response.status_code}" + ) + logger.error(f"Response: {e.response.text}") + raise + except KeyError as e: + logger.error( + f"Invalid response from registration endpoint: missing {e}" + ) + raise ValueError(f"Invalid registration response: missing {e}") + + # Should not reach here, but raise if we do + raise httpx.HTTPStatusError( + "Registration failed after retries", + request=httpx.Request("POST", registration_endpoint), + response=httpx.Response(429), + ) async def delete_client( diff --git a/nextcloud_mcp_server/auth/elicitation.py b/nextcloud_mcp_server/auth/elicitation.py index ef7120e..871e827 100644 --- a/nextcloud_mcp_server/auth/elicitation.py +++ b/nextcloud_mcp_server/auth/elicitation.py @@ -76,9 +76,13 @@ async def present_login_url( logger.info("User cancelled login flow") return "cancelled" - except NotImplementedError as e: + except NotImplementedError: # Elicitation not supported by this client/SDK - fall back to message - logger.debug( - f"Elicitation not available ({type(e).__name__}: {e}), returning URL in message" + logger.debug("Elicitation not available, returning URL in message") + return "message_only" + except Exception as e: + logger.warning( + f"Elicitation failed unexpectedly ({type(e).__name__}: {e}), " + "falling back to message" ) return "message_only" diff --git a/nextcloud_mcp_server/auth/oauth_routes.py b/nextcloud_mcp_server/auth/oauth_routes.py index 22174c5..97415ed 100644 --- a/nextcloud_mcp_server/auth/oauth_routes.py +++ b/nextcloud_mcp_server/auth/oauth_routes.py @@ -26,6 +26,7 @@ import secrets import time from base64 import urlsafe_b64encode from dataclasses import dataclass, field +from typing import Any from urllib.parse import urlencode from urllib.parse import urlparse as parse_url @@ -50,20 +51,21 @@ logger = logging.getLogger(__name__) @dataclass class ProxyCodeEntry: - """Stores state for a proxy authorization code issued by the AS proxy.""" + """Stores state for a proxy authorization code issued by the AS proxy. + + Proxy codes have a 60-second TTL as a security mitigation: they are + single-use, ephemeral codes that bridge the AS proxy callback and the + client's token exchange. The short window limits replay risk. + """ client_id: str client_redirect_uri: str client_state: str code_challenge: str code_challenge_method: str - nc_token_response: dict # Full JSON token response from Nextcloud + nc_token_response: dict[str, Any] # Full JSON token response from Nextcloud created_at: float = field(default_factory=time.time) - expires_at: float = 0.0 - - def __post_init__(self): - if self.expires_at == 0.0: - self.expires_at = self.created_at + 60 # 60 second TTL + expires_at: float = field(default_factory=lambda: time.time() + 60) @property def is_expired(self) -> bool: @@ -73,7 +75,11 @@ class ProxyCodeEntry: # Server-side state for AS proxy authorize → callback mapping @dataclass class ASProxySession: - """Stores state between /oauth/authorize and the Nextcloud callback.""" + """Stores state between /oauth/authorize and the Nextcloud callback. + + Sessions have a 600-second (10 minute) TTL to allow time for the user + to complete the browser-based authorization flow. + """ client_id: str client_redirect_uri: str @@ -82,11 +88,7 @@ class ASProxySession: code_challenge_method: str requested_scopes: str created_at: float = field(default_factory=time.time) - expires_at: float = 0.0 - - def __post_init__(self): - if self.expires_at == 0.0: - self.expires_at = self.created_at + 600 # 10 minute TTL + expires_at: float = field(default_factory=lambda: time.time() + 600) @property def is_expired(self) -> bool: @@ -97,6 +99,30 @@ class ASProxySession: _proxy_codes: dict[str, ProxyCodeEntry] = {} _as_proxy_sessions: dict[str, ASProxySession] = {} +# OIDC discovery document cache (URL → (expires_at, data)) +_discovery_cache: dict[str, tuple[float, dict[str, Any]]] = {} +_DISCOVERY_CACHE_TTL = 300 # 5 minutes + +# DCR rate limiting (IP → [timestamps]) +_dcr_rate_limit: dict[str, list[float]] = {} +_DCR_RATE_LIMIT_MAX = 10 # max requests +_DCR_RATE_LIMIT_WINDOW = 60 # per 60 seconds + + +async def _get_cached_discovery(url: str) -> dict[str, Any]: + """Fetch OIDC discovery document with caching (5-minute TTL).""" + now = time.time() + if url in _discovery_cache: + expires_at, data = _discovery_cache[url] + if now < expires_at: + return data + async with nextcloud_httpx_client() as http_client: + response = await http_client.get(url) + response.raise_for_status() + data = response.json() + _discovery_cache[url] = (now + _DISCOVERY_CACHE_TTL, data) + return data + def _cleanup_expired_proxy_codes() -> None: """Remove expired proxy codes and sessions.""" @@ -295,11 +321,8 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse: status_code=500, ) - async with nextcloud_httpx_client() as http_client: - response = await http_client.get(discovery_url) - response.raise_for_status() - discovery = response.json() - authorization_endpoint = discovery["authorization_endpoint"] + discovery = await _get_cached_discovery(discovery_url) + authorization_endpoint = discovery["authorization_endpoint"] # Replace internal Docker hostname with public URL for browser access public_issuer = os.getenv("NEXTCLOUD_PUBLIC_ISSUER_URL") @@ -424,11 +447,8 @@ async def oauth_authorize_nextcloud( status_code=500, ) - async with nextcloud_httpx_client() as http_client: - response = await http_client.get(discovery_url) - response.raise_for_status() - discovery = response.json() - authorization_endpoint = discovery["authorization_endpoint"] + discovery = await _get_cached_discovery(discovery_url) + authorization_endpoint = discovery["authorization_endpoint"] # Fix internal hostname for browser access public_issuer = os.getenv("NEXTCLOUD_PUBLIC_ISSUER_URL") @@ -530,11 +550,17 @@ async def oauth_callback_nextcloud(request: Request): callback_uri = f"{mcp_server_url}/oauth/callback" discovery_url = oauth_config.get("discovery_url") - async with nextcloud_httpx_client() as http_client: - response = await http_client.get(discovery_url) - response.raise_for_status() - discovery = response.json() - token_endpoint = discovery["token_endpoint"] + if not discovery_url: + return JSONResponse( + { + "error": "server_error", + "error_description": "OIDC discovery URL not configured", + }, + status_code=500, + ) + + discovery = await _get_cached_discovery(discovery_url) + token_endpoint = discovery["token_endpoint"] # Build token exchange params token_params = { @@ -797,16 +823,32 @@ async def _oauth_callback_as_proxy( mcp_server_client_secret = os.getenv( "MCP_SERVER_CLIENT_SECRET", oauth_config.get("client_secret") ) + + if not mcp_server_client_id or not mcp_server_client_secret: + return JSONResponse( + { + "error": "server_error", + "error_description": "MCP server OAuth credentials not configured", + }, + status_code=500, + ) + mcp_server_url = oauth_config["mcp_server_url"] callback_uri = f"{mcp_server_url}/oauth/callback" # Discover token endpoint discovery_url = oauth_config.get("discovery_url") - async with nextcloud_httpx_client() as http_client: - response = await http_client.get(discovery_url) - response.raise_for_status() - discovery = response.json() - token_endpoint = discovery["token_endpoint"] + if not discovery_url: + return JSONResponse( + { + "error": "server_error", + "error_description": "OIDC discovery URL not configured", + }, + status_code=500, + ) + + discovery = await _get_cached_discovery(discovery_url) + token_endpoint = discovery["token_endpoint"] # Exchange auth code with Nextcloud (server-side, confidential client, no PKCE) token_params = { @@ -942,8 +984,17 @@ async def _token_authorization_code(request: Request, form) -> JSONResponse: status_code=400, ) - # Validate client_id matches - if client_id and client_id != entry.client_id: + # Validate client_id (required per RFC 6749 Section 4.1.3) + if not client_id: + return JSONResponse( + { + "error": "invalid_request", + "error_description": "client_id is required", + }, + status_code=400, + ) + + if client_id != entry.client_id: return JSONResponse( { "error": "invalid_grant", @@ -952,8 +1003,17 @@ async def _token_authorization_code(request: Request, form) -> JSONResponse: status_code=400, ) - # Validate redirect_uri matches - if redirect_uri and redirect_uri != entry.client_redirect_uri: + # Validate redirect_uri (required per RFC 6749 Section 4.1.3) + if not redirect_uri: + return JSONResponse( + { + "error": "invalid_request", + "error_description": "redirect_uri is required", + }, + status_code=400, + ) + + if redirect_uri != entry.client_redirect_uri: return JSONResponse( { "error": "invalid_grant", @@ -962,26 +1022,29 @@ async def _token_authorization_code(request: Request, form) -> JSONResponse: status_code=400, ) - # Verify PKCE - if entry.code_challenge: - if not code_verifier: - return JSONResponse( - { - "error": "invalid_grant", - "error_description": "code_verifier is required (PKCE)", - }, - status_code=400, - ) + # Verify PKCE (always required — oauth_authorize mandates code_challenge) + assert entry.code_challenge, ( + "code_challenge must be set (enforced by oauth_authorize)" + ) # noqa: S101 - if not _verify_pkce_s256(code_verifier, entry.code_challenge): - logger.warning(f"PKCE verification failed for client {entry.client_id}") - return JSONResponse( - { - "error": "invalid_grant", - "error_description": "PKCE verification failed", - }, - status_code=400, - ) + if not code_verifier: + return JSONResponse( + { + "error": "invalid_grant", + "error_description": "code_verifier is required (PKCE)", + }, + status_code=400, + ) + + if not _verify_pkce_s256(code_verifier, entry.code_challenge): + logger.warning(f"PKCE verification failed for client {entry.client_id}") + return JSONResponse( + { + "error": "invalid_grant", + "error_description": "PKCE verification failed", + }, + status_code=400, + ) logger.info( f"AS proxy token: Returning Nextcloud token for client {entry.client_id}" @@ -1022,15 +1085,31 @@ async def _token_refresh(request: Request, form) -> JSONResponse: mcp_server_client_secret = os.getenv( "MCP_SERVER_CLIENT_SECRET", oauth_config.get("client_secret") ) + + if not mcp_server_client_id or not mcp_server_client_secret: + return JSONResponse( + { + "error": "server_error", + "error_description": "MCP server OAuth credentials not configured", + }, + status_code=500, + ) + mcp_server_url = oauth_config["mcp_server_url"] # Discover token endpoint discovery_url = oauth_config.get("discovery_url") - async with nextcloud_httpx_client() as http_client: - response = await http_client.get(discovery_url) - response.raise_for_status() - discovery = response.json() - token_endpoint = discovery["token_endpoint"] + if not discovery_url: + return JSONResponse( + { + "error": "server_error", + "error_description": "OIDC discovery URL not configured", + }, + status_code=500, + ) + + discovery = await _get_cached_discovery(discovery_url) + token_endpoint = discovery["token_endpoint"] # Proxy refresh request to Nextcloud token_params = { @@ -1095,8 +1174,40 @@ async def oauth_register_proxy(request: Request) -> JSONResponse: oauth_config = oauth_ctx["config"] nextcloud_host = oauth_config["nextcloud_host"] - # Proxy DCR to Nextcloud - registration_endpoint = f"{nextcloud_host}/apps/oidc/register" + # Rate limit DCR requests per client IP + client_ip = request.client.host if request.client else "unknown" + now = time.time() + timestamps = _dcr_rate_limit.get(client_ip, []) + # Remove timestamps outside the window + timestamps = [t for t in timestamps if now - t < _DCR_RATE_LIMIT_WINDOW] + if len(timestamps) >= _DCR_RATE_LIMIT_MAX: + logger.warning(f"DCR rate limit exceeded for {client_ip}") + return JSONResponse( + { + "error": "too_many_requests", + "error_description": "Rate limit exceeded for client registration", + }, + status_code=429, + headers={"Retry-After": str(_DCR_RATE_LIMIT_WINDOW)}, + ) + timestamps.append(now) + _dcr_rate_limit[client_ip] = timestamps + + # Discover registration endpoint from OIDC discovery (prefer over hardcoded path) + discovery_url = oauth_config.get("discovery_url") + if discovery_url: + try: + discovery = await _get_cached_discovery(discovery_url) + registration_endpoint = discovery.get( + "registration_endpoint", f"{nextcloud_host}/apps/oidc/register" + ) + 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.info(f"DCR proxy: Forwarding registration to {registration_endpoint}") diff --git a/nextcloud_mcp_server/auth/scope_authorization.py b/nextcloud_mcp_server/auth/scope_authorization.py index 76f665c..b959a06 100644 --- a/nextcloud_mcp_server/auth/scope_authorization.py +++ b/nextcloud_mcp_server/auth/scope_authorization.py @@ -1,6 +1,7 @@ """Scope-based authorization for MCP tools.""" import logging +import time from functools import wraps from typing import Any, Callable @@ -141,7 +142,7 @@ def require_scopes(*required_scopes: str): if get_settings().enable_login_flow and not set(required_scopes).issubset( IDENTITY_ONLY_SCOPES ): - from nextcloud_mcp_server.server.oauth_tools import ( # noqa: PLC0415 + from nextcloud_mcp_server.auth.token_utils import ( # noqa: PLC0415 extract_user_id_from_token, ) @@ -476,9 +477,18 @@ def discover_all_scopes(mcp) -> list[str]: # ── Login Flow v2 helpers ──────────────────────────────────────────────── +# Scope cache: user_id → (expires_at, scopes) +_scope_cache: dict[str, tuple[float, list[str] | str | None]] = {} +_SCOPE_CACHE_TTL = 300 # 5 minutes + + +def invalidate_scope_cache(user_id: str) -> None: + """Remove cached scopes for a user (call when scopes are updated).""" + _scope_cache.pop(user_id, None) + async def _get_stored_scopes(user_id: str) -> list[str] | str | None: - """Look up stored app password scopes for a user. + """Look up stored app password scopes for a user (with TTL cache). Returns: - list[str]: Specific scopes granted @@ -489,11 +499,21 @@ async def _get_stored_scopes(user_id: str) -> list[str] | str | None: Storage/infrastructure exceptions propagate to the caller (require_scopes decorator) for proper MCP error responses. """ + now = time.time() + if user_id in _scope_cache: + expires_at, cached = _scope_cache[user_id] + if now < expires_at: + return cached + storage = await get_shared_storage() data = await storage.get_app_password_with_scopes(user_id) if data is None: - return None - if data["scopes"] is None: - return "all" - return data["scopes"] + result = None + elif data["scopes"] is None: + result = "all" + else: + result = data["scopes"] + + _scope_cache[user_id] = (now + _SCOPE_CACHE_TTL, result) + return result diff --git a/nextcloud_mcp_server/auth/storage.py b/nextcloud_mcp_server/auth/storage.py index ffe1d69..ec2f2bc 100644 --- a/nextcloud_mcp_server/auth/storage.py +++ b/nextcloud_mcp_server/auth/storage.py @@ -1493,6 +1493,9 @@ class RefreshTokenStorage: app_password: Nextcloud app password to encrypt and store scopes: List of granted scopes (None = all scopes allowed) username: Nextcloud loginName from Login Flow v2 response + + Raises: + ValueError: If any scope is not in ALL_SUPPORTED_SCOPES """ if not self._initialized: await self.initialize() @@ -1503,6 +1506,16 @@ class RefreshTokenStorage: "Set TOKEN_ENCRYPTION_KEY for app password storage." ) + # Defense-in-depth: validate scopes at storage layer + if scopes is not None: + from nextcloud_mcp_server.models.auth import ( # noqa: PLC0415 + ALL_SUPPORTED_SCOPES, + ) + + invalid = [s for s in scopes if s not in ALL_SUPPORTED_SCOPES] + if invalid: + raise ValueError(f"Invalid scopes: {invalid}") + encrypted_password = self.cipher.encrypt(app_password.encode()) scopes_json = json.dumps(scopes) if scopes is not None else None now = int(time.time()) diff --git a/nextcloud_mcp_server/auth/token_utils.py b/nextcloud_mcp_server/auth/token_utils.py new file mode 100644 index 0000000..d9a4cb4 --- /dev/null +++ b/nextcloud_mcp_server/auth/token_utils.py @@ -0,0 +1,85 @@ +"""Token utility functions for extracting user identity from MCP access tokens. + +Extracted from server/oauth_tools.py to break circular import dependencies +between server/ and auth/ layers. +""" + +import logging +import os + +import jwt +from mcp.server.auth.middleware.auth_context import get_access_token +from mcp.server.auth.provider import AccessToken +from mcp.server.fastmcp import Context + +from nextcloud_mcp_server.auth.userinfo_routes import _query_idp_userinfo + +from ..http import nextcloud_httpx_client + +logger = logging.getLogger(__name__) + + +async def extract_user_id_from_token(ctx: Context) -> str: + """Extract user_id from the MCP access token (Flow 1). + + Handles both JWT and opaque tokens: + - JWT: Decode and extract 'sub' claim + - Opaque: Call userinfo endpoint to get 'sub' + + Args: + ctx: MCP context with access token + + Returns: + user_id extracted from token, or "default_user" as fallback + """ + # Use MCP SDK's get_access_token() which uses contextvars + access_token: AccessToken | None = get_access_token() + + if not access_token or not access_token.token: + logger.warning(" ✗ No access token found via get_access_token()") + return "default_user" + + token = access_token.token + is_jwt = "." in token and token.count(".") >= 2 + logger.info(f" Token type: {'JWT' if is_jwt else 'Opaque'}") + + # Try JWT decode first + if is_jwt: + try: + payload = jwt.decode(token, options={"verify_signature": False}) + user_id = payload.get("sub", "unknown") + logger.info(f" ✓ JWT decode successful: user_id={user_id}") + return user_id + except Exception as e: + logger.error(f" ✗ JWT decode failed: {type(e).__name__}: {e}") + + # Opaque token - call userinfo endpoint + logger.info(" Opaque token detected, calling userinfo endpoint...") + try: + # Get userinfo endpoint from OIDC discovery + oidc_discovery_uri = os.getenv( + "OIDC_DISCOVERY_URI", + "http://localhost:8080/.well-known/openid-configuration", + ) + async with nextcloud_httpx_client() as http_client: + discovery_response = await http_client.get(oidc_discovery_uri) + discovery_response.raise_for_status() + discovery = discovery_response.json() + userinfo_endpoint = discovery.get("userinfo_endpoint") + + if userinfo_endpoint: + userinfo = await _query_idp_userinfo(token, userinfo_endpoint) + if userinfo: + user_id = userinfo.get("sub", "unknown") + logger.info(f" ✓ Userinfo query successful: user_id={user_id}") + return user_id + else: + logger.error(" ✗ Userinfo query failed") + else: + logger.error(" ✗ No userinfo_endpoint available") + except Exception as e: + logger.error(f" ✗ Userinfo query failed: {type(e).__name__}: {e}") + + # Fallback + logger.warning(" Using fallback user_id: default_user") + return "default_user" diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index 818c655..2c5c068 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -5,7 +5,7 @@ import socket import ssl from dataclasses import dataclass from enum import Enum -from typing import Any, Optional +from typing import Any class DeploymentMode(Enum): @@ -169,32 +169,32 @@ class Settings: # Optional: If not set, mode is auto-detected from other settings # Valid values: single_user_basic, multi_user_basic, oauth_single_audience, # oauth_token_exchange, smithery - deployment_mode: Optional[str] = None + deployment_mode: str | None = None # OAuth/OIDC settings - oidc_discovery_url: Optional[str] = None - oidc_client_id: Optional[str] = None - oidc_client_secret: Optional[str] = None - oidc_issuer: Optional[str] = None + oidc_discovery_url: str | None = None + oidc_client_id: str | None = None + oidc_client_secret: str | None = None + oidc_issuer: str | None = None # Nextcloud settings - nextcloud_host: Optional[str] = None - nextcloud_username: Optional[str] = None - nextcloud_password: Optional[str] = None - nextcloud_app_password: Optional[str] = None # Preferred over nextcloud_password + nextcloud_host: str | None = None + nextcloud_username: str | None = None + nextcloud_password: str | None = None + nextcloud_app_password: str | None = None # Preferred over nextcloud_password # Nextcloud SSL/TLS settings nextcloud_verify_ssl: bool = True - nextcloud_ca_bundle: Optional[str] = None + nextcloud_ca_bundle: str | None = None # ADR-005: Token Audience Validation (required for OAuth mode) - nextcloud_mcp_server_url: Optional[str] = None # MCP server URL (used as audience) - nextcloud_resource_uri: Optional[str] = None # Nextcloud resource identifier + nextcloud_mcp_server_url: str | None = None # MCP server URL (used as audience) + nextcloud_resource_uri: str | None = None # Nextcloud resource identifier # Token verification endpoints - jwks_uri: Optional[str] = None - introspection_uri: Optional[str] = None - userinfo_uri: Optional[str] = None + jwks_uri: str | None = None + introspection_uri: str | None = None + userinfo_uri: str | None = None # Progressive Consent settings (always enabled - no flag needed) enable_token_exchange: bool = False @@ -218,8 +218,8 @@ class Settings: # TOKEN_STORAGE_DB: Path to SQLite database for persistent storage. # Used for webhook tracking (all modes) and OAuth token storage. # Defaults to /tmp/tokens.db - token_encryption_key: Optional[str] = None - token_storage_db: Optional[str] = None + token_encryption_key: str | None = None + token_storage_db: str | None = None # Vector sync settings (ADR-007) vector_sync_enabled: bool = False @@ -229,19 +229,19 @@ class Settings: vector_sync_user_poll_interval: int = 60 # seconds - OAuth mode user discovery # Qdrant settings (mutually exclusive modes) - qdrant_url: Optional[str] = None # Network mode: http://qdrant:6333 - qdrant_location: Optional[str] = None # Local mode: :memory: or /path/to/data - qdrant_api_key: Optional[str] = None + qdrant_url: str | None = None # Network mode: http://qdrant:6333 + qdrant_location: str | None = None # Local mode: :memory: or /path/to/data + qdrant_api_key: str | None = None qdrant_collection: str = "nextcloud_content" # Ollama settings (for embeddings) - ollama_base_url: Optional[str] = None + ollama_base_url: str | None = None ollama_embedding_model: str = "nomic-embed-text" ollama_verify_ssl: bool = True # OpenAI settings (for embeddings) - openai_api_key: Optional[str] = None - openai_base_url: Optional[str] = None + openai_api_key: str | None = None + openai_base_url: str | None = None openai_embedding_model: str = "text-embedding-3-small" # Document chunking settings (for vector embeddings) @@ -251,7 +251,7 @@ class Settings: # Observability settings metrics_enabled: bool = True metrics_port: int = 9090 - otel_exporter_otlp_endpoint: Optional[str] = None + otel_exporter_otlp_endpoint: str | None = None otel_exporter_verify_ssl: bool = False otel_service_name: str = "nextcloud-mcp-server" otel_traces_sampler: str = "always_on" diff --git a/nextcloud_mcp_server/context.py b/nextcloud_mcp_server/context.py index 3b47144..29045aa 100644 --- a/nextcloud_mcp_server/context.py +++ b/nextcloud_mcp_server/context.py @@ -272,7 +272,7 @@ async def _get_client_from_login_flow( Raises: ProvisioningRequiredError: If no stored app password exists """ - from nextcloud_mcp_server.server.oauth_tools import ( # noqa: PLC0415 + from nextcloud_mcp_server.auth.token_utils import ( # noqa: PLC0415 extract_user_id_from_token, ) diff --git a/nextcloud_mcp_server/models/auth.py b/nextcloud_mcp_server/models/auth.py index dca8e49..f249229 100644 --- a/nextcloud_mcp_server/models/auth.py +++ b/nextcloud_mcp_server/models/auth.py @@ -51,26 +51,28 @@ class UpdateScopesResponse(BaseResponse): new_scopes: list[str] | None = Field(None, description="Updated scope set") -# All supported application-level scopes -ALL_SUPPORTED_SCOPES = ( - "notes:read", - "notes:write", - "calendar:read", - "calendar:write", - "todo:read", - "todo:write", - "contacts:read", - "contacts:write", - "files:read", - "files:write", - "tables:read", - "tables:write", - "deck:read", - "deck:write", - "cookbook:read", - "cookbook:write", - "sharing:read", - "sharing:write", - "news:read", - "news:write", +# All supported application-level scopes (frozenset for O(1) membership tests) +ALL_SUPPORTED_SCOPES: frozenset[str] = frozenset( + { + "notes:read", + "notes:write", + "calendar:read", + "calendar:write", + "todo:read", + "todo:write", + "contacts:read", + "contacts:write", + "files:read", + "files:write", + "tables:read", + "tables:write", + "deck:read", + "deck:write", + "cookbook:read", + "cookbook:write", + "sharing:read", + "sharing:write", + "news:read", + "news:write", + } ) diff --git a/nextcloud_mcp_server/server/auth_tools.py b/nextcloud_mcp_server/server/auth_tools.py index 795d256..4c7530f 100644 --- a/nextcloud_mcp_server/server/auth_tools.py +++ b/nextcloud_mcp_server/server/auth_tools.py @@ -16,6 +16,7 @@ from nextcloud_mcp_server.auth.elicitation import present_login_url from nextcloud_mcp_server.auth.login_flow import LoginFlowV2Client from nextcloud_mcp_server.auth.scope_authorization import require_scopes from nextcloud_mcp_server.auth.storage import get_shared_storage +from nextcloud_mcp_server.auth.token_utils import extract_user_id_from_token from nextcloud_mcp_server.config import get_nextcloud_ssl_verify, get_settings from nextcloud_mcp_server.models.auth import ( ALL_SUPPORTED_SCOPES, @@ -23,7 +24,6 @@ from nextcloud_mcp_server.models.auth import ( ProvisionStatusResponse, UpdateScopesResponse, ) -from nextcloud_mcp_server.server.oauth_tools import extract_user_id_from_token logger = logging.getLogger(__name__) @@ -92,7 +92,7 @@ def register_auth_tools(mcp: FastMCP) -> None: return ProvisionAccessResponse( status="error", message=f"Invalid scopes: {', '.join(invalid_scopes)}. " - f"Valid scopes: {', '.join(ALL_SUPPORTED_SCOPES)}", + f"Valid scopes: {', '.join(sorted(ALL_SUPPORTED_SCOPES))}", success=False, ) @@ -160,6 +160,13 @@ def register_auth_tools(mcp: FastMCP) -> None: "Login acknowledged. Call nc_auth_check_status to verify " "and complete provisioning." ) + return ProvisionAccessResponse( + status="pending", + login_url=init_response.login_url, + message=message, + user_id=user_id, + requested_scopes=requested_scopes, + ) return ProvisionAccessResponse( status="login_required", @@ -174,10 +181,12 @@ def register_auth_tools(mcp: FastMCP) -> None: title="Check Nextcloud Access Status", description=( "Check if Nextcloud access has been provisioned. " - "If a Login Flow is pending, this will poll for completion." + "If a Login Flow is pending, this will poll for completion. " + "Recommended polling interval: 5 seconds." ), annotations=ToolAnnotations( readOnlyHint=True, + idempotentHint=True, openWorldHint=True, ), ) diff --git a/nextcloud_mcp_server/server/oauth_tools.py b/nextcloud_mcp_server/server/oauth_tools.py index 55abf56..75e261e 100644 --- a/nextcloud_mcp_server/server/oauth_tools.py +++ b/nextcloud_mcp_server/server/oauth_tools.py @@ -12,9 +12,6 @@ from datetime import datetime, timezone from typing import Optional from urllib.parse import urlencode -import jwt -from mcp.server.auth.middleware.auth_context import get_access_token -from mcp.server.auth.provider import AccessToken from mcp.server.fastmcp import Context from mcp.types import ToolAnnotations from pydantic import BaseModel, Field @@ -23,80 +20,16 @@ from nextcloud_mcp_server.auth import require_scopes from nextcloud_mcp_server.auth.astrolabe_client import AstrolabeClient from nextcloud_mcp_server.auth.storage import RefreshTokenStorage from nextcloud_mcp_server.auth.token_broker import TokenBrokerService -from nextcloud_mcp_server.auth.userinfo_routes import _query_idp_userinfo + +# Re-export for backward compatibility — canonical location is auth.token_utils +from nextcloud_mcp_server.auth.token_utils import ( + extract_user_id_from_token as extract_user_id_from_token, # noqa: PLC0414 +) from nextcloud_mcp_server.config import get_settings -from ..http import nextcloud_httpx_client - logger = logging.getLogger(__name__) -async def extract_user_id_from_token(ctx: Context) -> str: - """Extract user_id from the MCP access token (Flow 1). - - Handles both JWT and opaque tokens: - - JWT: Decode and extract 'sub' claim - - Opaque: Call userinfo endpoint to get 'sub' - - Args: - ctx: MCP context with access token - - Returns: - user_id extracted from token, or "default_user" as fallback - """ - # Use MCP SDK's get_access_token() which uses contextvars - access_token: AccessToken | None = get_access_token() - - if not access_token or not access_token.token: - logger.warning(" ✗ No access token found via get_access_token()") - return "default_user" - - token = access_token.token - is_jwt = "." in token and token.count(".") >= 2 - logger.info(f" Token type: {'JWT' if is_jwt else 'Opaque'}") - - # Try JWT decode first - if is_jwt: - try: - payload = jwt.decode(token, options={"verify_signature": False}) - user_id = payload.get("sub", "unknown") - logger.info(f" ✓ JWT decode successful: user_id={user_id}") - return user_id - except Exception as e: - logger.error(f" ✗ JWT decode failed: {type(e).__name__}: {e}") - - # Opaque token - call userinfo endpoint - logger.info(" Opaque token detected, calling userinfo endpoint...") - try: - # Get userinfo endpoint from OIDC discovery - oidc_discovery_uri = os.getenv( - "OIDC_DISCOVERY_URI", - "http://localhost:8080/.well-known/openid-configuration", - ) - async with nextcloud_httpx_client() as http_client: - discovery_response = await http_client.get(oidc_discovery_uri) - discovery_response.raise_for_status() - discovery = discovery_response.json() - userinfo_endpoint = discovery.get("userinfo_endpoint") - - if userinfo_endpoint: - userinfo = await _query_idp_userinfo(token, userinfo_endpoint) - if userinfo: - user_id = userinfo.get("sub", "unknown") - logger.info(f" ✓ Userinfo query successful: user_id={user_id}") - return user_id - else: - logger.error(" ✗ Userinfo query failed") - else: - logger.error(" ✗ No userinfo_endpoint available") - except Exception as e: - logger.error(f" ✗ Userinfo query failed: {type(e).__name__}: {e}") - - # Fallback - logger.warning(" Using fallback user_id: default_user") - return "default_user" - - class ProvisioningStatus(BaseModel): """Status of Nextcloud provisioning for a user.""" diff --git a/tests/unit/api/__init__.py b/tests/unit/api/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/unit/api/test_access.py b/tests/unit/api/test_access.py new file mode 100644 index 0000000..380168a --- /dev/null +++ b/tests/unit/api/test_access.py @@ -0,0 +1,243 @@ +"""Unit tests for access.py REST API endpoints. + +Tests the REST API endpoints for user access and scope management: +- GET /api/v1/users/{user_id}/access - Get user's provisioned access and scopes +- PATCH /api/v1/users/{user_id}/scopes - Update user's application-level scopes +- GET /api/v1/scopes - List all supported scopes +""" + +import base64 +import tempfile +from pathlib import Path + +import pytest +from cryptography.fernet import Fernet +from starlette.applications import Starlette +from starlette.routing import Route +from starlette.testclient import TestClient + +from nextcloud_mcp_server.api.access import ( + get_user_access, + list_supported_scopes, + update_user_scopes, +) +from nextcloud_mcp_server.auth.storage import RefreshTokenStorage +from nextcloud_mcp_server.models.auth import ALL_SUPPORTED_SCOPES + +pytestmark = pytest.mark.unit + + +@pytest.fixture +def encryption_key(): + """Generate a test encryption key.""" + return Fernet.generate_key().decode() + + +@pytest.fixture +async def temp_storage(encryption_key): + """Create temporary storage instance with encryption for testing.""" + with tempfile.TemporaryDirectory() as tmpdir: + db_path = Path(tmpdir) / "test_access.db" + storage = RefreshTokenStorage( + db_path=str(db_path), encryption_key=encryption_key + ) + await storage.initialize() + yield storage + + +def create_basic_auth_header(username: str, password: str) -> str: + """Create BasicAuth header value.""" + credentials = f"{username}:{password}" + encoded = base64.b64encode(credentials.encode()).decode() + return f"Basic {encoded}" + + +def create_test_app(storage): + """Create a test Starlette app with the access endpoints.""" + app = Starlette( + routes=[ + Route( + "/api/v1/users/{user_id}/access", + get_user_access, + methods=["GET"], + ), + Route( + "/api/v1/users/{user_id}/scopes", + update_user_scopes, + methods=["PATCH"], + ), + Route( + "/api/v1/scopes", + list_supported_scopes, + methods=["GET"], + ), + ], + ) + app.state.storage = storage + return app + + +class TestGetUserAccess: + """Tests for GET /api/v1/users/{user_id}/access.""" + + async def test_not_provisioned(self, temp_storage): + """Returns provisioned=False when no app password stored.""" + app = create_test_app(temp_storage) + client = TestClient(app) + + resp = client.get( + "/api/v1/users/alice/access", + headers={"Authorization": create_basic_auth_header("alice", "pw")}, + ) + assert resp.status_code == 200 + data = resp.json() + assert data["success"] is True + assert data["provisioned"] is False + assert data["scopes"] is None + + async def test_provisioned_with_scopes(self, temp_storage): + """Returns provisioned=True with scopes when app password exists.""" + await temp_storage.store_app_password_with_scopes( + user_id="alice", + app_password="test-app-pw", + scopes=["notes:read", "calendar:write"], + username="alice_nc", + ) + + app = create_test_app(temp_storage) + client = TestClient(app) + + resp = client.get( + "/api/v1/users/alice/access", + headers={"Authorization": create_basic_auth_header("alice", "pw")}, + ) + assert resp.status_code == 200 + data = resp.json() + assert data["success"] is True + assert data["provisioned"] is True + assert set(data["scopes"]) == {"notes:read", "calendar:write"} + assert data["username"] == "alice_nc" + + async def test_missing_auth_header(self, temp_storage): + """Returns 401 when no Authorization header.""" + app = create_test_app(temp_storage) + client = TestClient(app) + + resp = client.get("/api/v1/users/alice/access") + assert resp.status_code == 401 + + async def test_user_id_mismatch(self, temp_storage): + """Returns 403 when path user_id doesn't match auth credentials.""" + app = create_test_app(temp_storage) + client = TestClient(app) + + resp = client.get( + "/api/v1/users/alice/access", + headers={"Authorization": create_basic_auth_header("bob", "pw")}, + ) + assert resp.status_code == 403 + + +class TestUpdateUserScopes: + """Tests for PATCH /api/v1/users/{user_id}/scopes.""" + + async def test_update_valid_scopes(self, temp_storage): + """Successfully updates scopes for a provisioned user.""" + await temp_storage.store_app_password_with_scopes( + user_id="alice", + app_password="test-app-pw", + scopes=["notes:read"], + username="alice_nc", + ) + + app = create_test_app(temp_storage) + client = TestClient(app) + + resp = client.patch( + "/api/v1/users/alice/scopes", + headers={"Authorization": create_basic_auth_header("alice", "pw")}, + json={"scopes": ["notes:read", "notes:write", "calendar:read"]}, + ) + assert resp.status_code == 200 + data = resp.json() + assert data["success"] is True + assert set(data["scopes"]) == {"notes:read", "notes:write", "calendar:read"} + + async def test_invalid_scopes(self, temp_storage): + """Returns 400 for invalid scope names.""" + await temp_storage.store_app_password_with_scopes( + user_id="alice", + app_password="test-app-pw", + scopes=["notes:read"], + ) + + app = create_test_app(temp_storage) + client = TestClient(app) + + resp = client.patch( + "/api/v1/users/alice/scopes", + headers={"Authorization": create_basic_auth_header("alice", "pw")}, + json={"scopes": ["notes:read", "invalid:scope"]}, + ) + assert resp.status_code == 400 + data = resp.json() + assert data["success"] is False + assert "invalid:scope" in data["error"] + + async def test_user_not_provisioned(self, temp_storage): + """Returns 404 when user has no app password.""" + app = create_test_app(temp_storage) + client = TestClient(app) + + resp = client.patch( + "/api/v1/users/alice/scopes", + headers={"Authorization": create_basic_auth_header("alice", "pw")}, + json={"scopes": ["notes:read"]}, + ) + assert resp.status_code == 404 + data = resp.json() + assert data["success"] is False + + async def test_missing_scopes_field(self, temp_storage): + """Returns 400 when scopes field is missing from body.""" + app = create_test_app(temp_storage) + client = TestClient(app) + + resp = client.patch( + "/api/v1/users/alice/scopes", + headers={"Authorization": create_basic_auth_header("alice", "pw")}, + json={"something_else": True}, + ) + assert resp.status_code == 400 + + async def test_invalid_json_body(self, temp_storage): + """Returns 400 for invalid JSON body.""" + app = create_test_app(temp_storage) + client = TestClient(app) + + resp = client.patch( + "/api/v1/users/alice/scopes", + headers={ + "Authorization": create_basic_auth_header("alice", "pw"), + "Content-Type": "application/json", + }, + content=b"not json", + ) + assert resp.status_code == 400 + + +class TestListSupportedScopes: + """Tests for GET /api/v1/scopes.""" + + async def test_returns_all_scopes(self, temp_storage): + """Returns all supported scopes sorted.""" + app = create_test_app(temp_storage) + client = TestClient(app) + + resp = client.get("/api/v1/scopes") + assert resp.status_code == 200 + data = resp.json() + assert data["success"] is True + assert set(data["scopes"]) == ALL_SUPPORTED_SCOPES + # Verify it's sorted + assert data["scopes"] == sorted(data["scopes"]) diff --git a/tests/unit/test_scope_authorization_stored.py b/tests/unit/test_scope_authorization_stored.py index 5fc531b..0b8ceab 100644 --- a/tests/unit/test_scope_authorization_stored.py +++ b/tests/unit/test_scope_authorization_stored.py @@ -10,11 +10,20 @@ import pytest from nextcloud_mcp_server.auth.scope_authorization import ( _get_stored_scopes, + _scope_cache, ) pytestmark = pytest.mark.unit +@pytest.fixture(autouse=True) +def clear_scope_cache(): + """Clear scope cache before each test.""" + _scope_cache.clear() + yield + _scope_cache.clear() + + async def test_get_stored_scopes_with_scopes(): """Test getting specific scopes from storage.""" mock_storage = AsyncMock()