From 148ab8c117ca49521a45ebed9ba41087381806c7 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 3 May 2026 22:26:35 +0200 Subject: [PATCH] fix(webhooks): use OCS v2 capabilities for /api/v1/apps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Astrolabe webhooks UI hits /api/v1/apps on the MCP server, which forwarded the OAuth bearer token to /ocs/v1.php/cloud/apps?filter=enabled. That OCS endpoint is admin-only AND @PasswordConfirmationRequired — neither requirement is satisfiable via an OAuth bearer token, so even an admin user's token returns a silent 401 (no entry in nextcloud.log). Switch to /ocs/v2.php/cloud/capabilities, which has no admin or password- confirmation gate, accepts the existing bearer token, and returns a capabilities map keyed by app id (notes, files, tables, forms, etc.). This is sufficient for the webhook presets UI to gate available presets against the running Nextcloud instance's enabled apps. Bearer is preserved on the outbound call because anonymous capabilities omits notes/tables/forms — only authenticated capabilities exposes them. Tests: - New unit test covers the regression (asserts /ocs/v2.php/cloud/capabilities is hit, NOT /cloud/apps), response parsing, sanitized error messages, and missing-config paths. - New integration test under tests/server/login_flow/ drives a real OAuth flow against mcp-login-flow with a static OIDC client (nextcloudMcpServerUIPublicClient) and asserts /api/v1/apps returns 200 with core/files in the response. docker-compose.yml: aligns mcp-login-flow's ALLOWED_MGMT_CLIENT with mcp-multi-user-basic so the same static-client test fixture works for both. Follow-up to homelab-argocd #1608, which set ALLOWED_MGMT_CLIENT in production but didn't unblock the webhooks flow. Co-Authored-By: Claude Opus 4.7 (1M context) --- docker-compose.yml | 6 + nextcloud_mcp_server/api/webhooks.py | 24 ++- tests/server/login_flow/conftest.py | 197 ++++++++++++++++++ .../server/login_flow/test_management_api.py | 52 +++++ tests/unit/test_management_apps_endpoint.py | 165 +++++++++++++++ 5 files changed, 433 insertions(+), 11 deletions(-) create mode 100644 tests/server/login_flow/test_management_api.py create mode 100644 tests/unit/test_management_apps_endpoint.py diff --git a/docker-compose.yml b/docker-compose.yml index 3a1c6c0e..6e8a1f8c 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -288,6 +288,12 @@ services: - ENABLE_SEMANTIC_SEARCH=true - VECTOR_SYNC_SCAN_INTERVAL=60 - VECTOR_SYNC_PROCESSOR_WORKERS=1 + + # Management API allowlist (ADR-018) — matches mcp-multi-user-basic so + # the same configure_astrolabe_for_mcp_server fixture (which creates the + # static `nextcloudMcpServerUIPublicClient` OIDC client) works for tests + # against this profile too. + - ALLOWED_MGMT_CLIENT=nextcloudMcpServerUIPublicClient volumes: - login-flow-data:/app/data - login-flow-oauth-storage:/app/.oauth diff --git a/nextcloud_mcp_server/api/webhooks.py b/nextcloud_mcp_server/api/webhooks.py index 3f9a3a28..c281d3d1 100644 --- a/nextcloud_mcp_server/api/webhooks.py +++ b/nextcloud_mcp_server/api/webhooks.py @@ -47,7 +47,9 @@ async def get_installed_apps(request: Request) -> JSONResponse: ) try: - # Get Bearer token from request + # Get Bearer token from request — forwarded to Nextcloud so the + # capabilities response includes per-user / per-app entries (anonymous + # capabilities omits notes, tables, forms etc.). token = extract_bearer_token(request) if not token: raise ValueError("Missing Authorization header") @@ -59,21 +61,20 @@ async def get_installed_apps(request: Request) -> JSONResponse: if not nextcloud_host: raise ValueError("Nextcloud host not configured") - # Create authenticated HTTP client + # Use OCS v2 capabilities. The legacy /ocs/v1.php/cloud/apps endpoint is + # admin-only AND @PasswordConfirmationRequired — neither is satisfiable + # via an OAuth bearer token, so it always 401s. Capabilities has no such + # gates and returns a map keyed by app id for every enabled app that + # implements OCSCapabilities, which is sufficient to populate the + # webhook presets UI. async with nextcloud_httpx_client( base_url=nextcloud_host, headers={"Authorization": f"Bearer {token}"}, timeout=30.0, ) as client: - # Get installed apps using OCS API - # Notes, Calendar, Deck, Tables, etc. are apps that support webhooks - # We check which ones are installed and enabled - ocs_url = "/ocs/v1.php/cloud/apps" - params = {"filter": "enabled"} - response = await client.get( - ocs_url, - params=params, + "/ocs/v2.php/cloud/capabilities", + params={"format": "json"}, headers={"OCS-APIRequest": "true", "Accept": "application/json"}, ) @@ -81,7 +82,8 @@ async def get_installed_apps(request: Request) -> JSONResponse: raise ValueError(f"OCS API returned status {response.status_code}") data = response.json() - apps = data.get("ocs", {}).get("data", {}).get("apps", []) + capabilities = data.get("ocs", {}).get("data", {}).get("capabilities", {}) + apps = sorted(capabilities.keys()) return JSONResponse({"apps": apps}) diff --git a/tests/server/login_flow/conftest.py b/tests/server/login_flow/conftest.py index ac4a81d2..f56d6aa7 100644 --- a/tests/server/login_flow/conftest.py +++ b/tests/server/login_flow/conftest.py @@ -933,3 +933,200 @@ async def diana_login_flow_mcp_client( password=test_users_setup["diana"]["password"], ): yield session + + +# Static OIDC client used by the management API integration tests. +# Matches the value `mcp-login-flow` and `mcp-multi-user-basic` allowlist +# (`ALLOWED_MGMT_CLIENT=nextcloudMcpServerUIPublicClient`) so tokens issued +# to it pass the management API allowlist check. +STATIC_MGMT_CLIENT_ID = "nextcloudMcpServerUIPublicClient" + + +@pytest.fixture(scope="session") +async def login_flow_static_client_credentials(anyio_backend, oauth_callback_server): + """Pre-create the static OIDC client `nextcloudMcpServerUIPublicClient` + via `occ oidc:create` with the test's OAuth callback URL. + + The static client_id is allowlisted on `mcp-login-flow` (and + `mcp-multi-user-basic`) via `ALLOWED_MGMT_CLIENT`, so tokens it issues + pass the management API allowlist check. Uses a confidential JWT-token + client to match production Astrolabe configuration. + + Yields: (client_id, client_secret, callback_url, token_endpoint, authorization_endpoint) + """ + import json + import subprocess + + nextcloud_host = os.getenv("NEXTCLOUD_HOST") + if not nextcloud_host: + pytest.skip("Static client tests require NEXTCLOUD_HOST") + + auth_states, callback_url = oauth_callback_server + client_id = STATIC_MGMT_CLIENT_ID + + # Idempotent: remove if a previous session left one behind + subprocess.run( + [ + "docker", + "compose", + "exec", + "-T", + "app", + "php", + "/var/www/html/occ", + "oidc:remove", + client_id, + ], + check=False, + capture_output=True, + ) + + logger.info(f"Creating static OIDC client {client_id} with callback {callback_url}") + result = subprocess.run( + [ + "docker", + "compose", + "exec", + "-T", + "app", + "php", + "/var/www/html/occ", + "oidc:create", + "Login Flow Static Client (test)", + callback_url, + "--client_id", + client_id, + "--type", + "confidential", + "--flow", + "code", + "--token_type", + "jwt", + "--resource_url", + LOGIN_FLOW_MCP_BASE_URL, + "--allowed_scopes", + DEFAULT_FULL_SCOPES, + ], + check=True, + capture_output=True, + text=True, + ) + try: + client_output = json.loads(result.stdout.strip()) + except json.JSONDecodeError as e: + raise RuntimeError( + f"occ oidc:create returned non-JSON output: {result.stdout[:200]!r}" + ) from e + client_secret = client_output.get("client_secret") + if not client_secret: + raise ValueError("occ oidc:create did not return client_secret in JSON output") + + async with httpx.AsyncClient(timeout=30.0) as http_client: + discovery_response = await http_client.get( + f"{nextcloud_host}/.well-known/openid-configuration" + ) + discovery_response.raise_for_status() + oidc_config = discovery_response.json() + + yield ( + client_id, + client_secret, + callback_url, + oidc_config["token_endpoint"], + oidc_config["authorization_endpoint"], + ) + + subprocess.run( + [ + "docker", + "compose", + "exec", + "-T", + "app", + "php", + "/var/www/html/occ", + "oidc:remove", + client_id, + ], + check=False, + capture_output=True, + ) + + +@pytest.fixture(scope="session") +async def login_flow_static_client_token( + anyio_backend, + browser, + login_flow_static_client_credentials, + oauth_callback_server, +) -> str: + """Drive the OAuth auth-code flow using the static OIDC client and + return the raw access_token string. + + Mirrors `login_flow_oauth_token` but feeds it static credentials instead + of a DCR-generated client. Required for hitting management API + endpoints which gate on `ALLOWED_MGMT_CLIENT`. + """ + nextcloud_host = os.getenv("NEXTCLOUD_HOST") + username = os.getenv("NEXTCLOUD_USERNAME") + password = os.getenv("NEXTCLOUD_PASSWORD") + if not all([nextcloud_host, username, password]): + pytest.skip( + "Static client OAuth requires NEXTCLOUD_HOST, NEXTCLOUD_USERNAME, NEXTCLOUD_PASSWORD" + ) + + auth_states, _ = oauth_callback_server + client_id, client_secret, callback_url, token_endpoint, authorization_endpoint = ( + login_flow_static_client_credentials + ) + + state = secrets.token_urlsafe(32) + scopes_encoded = quote(DEFAULT_FULL_SCOPES, safe="") + auth_url = ( + f"{authorization_endpoint}?" + f"response_type=code&" + f"client_id={client_id}&" + f"redirect_uri={quote(callback_url, safe='')}&" + f"state={state}&" + f"scope={scopes_encoded}" + ) + + context = await browser.new_context(ignore_https_errors=True) + page = await context.new_page() + try: + await page.goto(auth_url, wait_until="networkidle", timeout=60000) + if "/login" in page.url or "/index.php/login" in page.url: + await page.wait_for_selector('input[name="user"]', timeout=10000) + await page.fill('input[name="user"]', username) + await page.fill('input[name="password"]', password) + await page.click('button[type="submit"]') + await page.wait_for_load_state("networkidle", timeout=60000) + try: + await _handle_oauth_consent_screen(page, username) + except Exception: + pass + + start = time.time() + while state not in auth_states: + if time.time() - start > 30: + raise TimeoutError("Timeout waiting for OAuth callback") + await anyio.sleep(0.5) + auth_code = auth_states[state] + finally: + await context.close() + + async with httpx.AsyncClient(timeout=30.0) as http_client: + token_response = await http_client.post( + token_endpoint, + data={ + "grant_type": "authorization_code", + "code": auth_code, + "redirect_uri": callback_url, + "client_id": client_id, + "client_secret": client_secret, + }, + ) + token_response.raise_for_status() + token_data = token_response.json() + + return token_data["access_token"] diff --git a/tests/server/login_flow/test_management_api.py b/tests/server/login_flow/test_management_api.py new file mode 100644 index 00000000..366e4990 --- /dev/null +++ b/tests/server/login_flow/test_management_api.py @@ -0,0 +1,52 @@ +"""Integration tests for the management API on the login-flow MCP server. + +These tests drive a real OAuth flow against Nextcloud's `oidc` app using the +static `nextcloudMcpServerUIPublicClient` client (which is allowlisted on the +`mcp-login-flow` container via `ALLOWED_MGMT_CLIENT`), then hit the +management API endpoints with the resulting bearer token. + +Regression coverage for the bug where /api/v1/apps proxied to OCS v1 +/cloud/apps and always 401'd. The handler now uses /ocs/v2.php/cloud/capabilities, +which is reachable for OAuth bearer tokens. +""" + +import httpx +import pytest + +LOGIN_FLOW_API_BASE_URL = "http://localhost:8004" + +pytestmark = [pytest.mark.integration, pytest.mark.oauth] + + +async def test_get_installed_apps_returns_capability_keys( + login_flow_static_client_token: str, +): + """GET /api/v1/apps returns 200 with a list of enabled-app capability keys.""" + async with httpx.AsyncClient(timeout=30.0) as client: + response = await client.get( + f"{LOGIN_FLOW_API_BASE_URL}/api/v1/apps", + headers={"Authorization": f"Bearer {login_flow_static_client_token}"}, + ) + + assert response.status_code == 200, ( + f"/api/v1/apps returned {response.status_code}: {response.text}" + ) + + data = response.json() + assert "apps" in data + assert isinstance(data["apps"], list) + + # Anonymous capabilities always exposes core; authenticated also exposes + # files. Both should be present whether or not the oidc app's + # BearerAuthMiddleware ran for this OCS route. + apps = data["apps"] + assert "core" in apps, f"expected 'core' in apps, got {apps}" + assert "files" in apps, f"expected 'files' in apps, got {apps}" + + +async def test_get_installed_apps_requires_bearer_token(): + """No Authorization header → 401 (handler's token validator rejects it).""" + async with httpx.AsyncClient(timeout=30.0) as client: + response = await client.get(f"{LOGIN_FLOW_API_BASE_URL}/api/v1/apps") + + assert response.status_code == 401 diff --git a/tests/unit/test_management_apps_endpoint.py b/tests/unit/test_management_apps_endpoint.py new file mode 100644 index 00000000..e2e82098 --- /dev/null +++ b/tests/unit/test_management_apps_endpoint.py @@ -0,0 +1,165 @@ +""" +Unit tests for the Management API /api/v1/apps endpoint. + +These tests cover the regression where /api/v1/apps proxied to +/ocs/v1.php/cloud/apps — an admin-only and @PasswordConfirmationRequired +endpoint that always 401s for OAuth bearer tokens. The handler now uses +/ocs/v2.php/cloud/capabilities, which accepts the bearer and returns an +authenticated capability map keyed by app id. +""" + +from unittest.mock import AsyncMock, MagicMock + +import pytest +from starlette.applications import Starlette +from starlette.routing import Route +from starlette.testclient import TestClient + +from nextcloud_mcp_server.api.webhooks import get_installed_apps + +pytestmark = pytest.mark.unit + + +def _build_test_app() -> Starlette: + app = Starlette(routes=[Route("/api/v1/apps", get_installed_apps, methods=["GET"])]) + app.state.oauth_context = {"config": {"nextcloud_host": "http://nc.test"}} + return app + + +def _patch_token_validation(mocker, user_id: str = "admin") -> None: + mocker.patch( + "nextcloud_mcp_server.api.webhooks.validate_token_and_get_user", + new=AsyncMock(return_value=(user_id, {"sub": user_id})), + ) + + +def _patch_outbound_client(mocker, response: MagicMock) -> AsyncMock: + """Patch the outbound httpx client; return the mocked .get() AsyncMock.""" + mock_get = AsyncMock(return_value=response) + mock_client = AsyncMock() + mock_client.get = mock_get + mock_client.__aenter__ = AsyncMock(return_value=mock_client) + # Must return False (or None) so async-with does NOT suppress exceptions + # raised inside the block — default AsyncMock() resolves to a truthy + # MagicMock and would silently swallow ValueError. + mock_client.__aexit__ = AsyncMock(return_value=False) + + mock_factory = MagicMock(return_value=mock_client) + mocker.patch( + "nextcloud_mcp_server.api.webhooks.nextcloud_httpx_client", mock_factory + ) + # Return both so tests can assert on factory kwargs and call args + mock_factory.attach_mock(mock_get, "get") + return mock_factory + + +def _capabilities_response(capabilities: dict[str, dict]) -> MagicMock: + response = MagicMock() + response.status_code = 200 + response.json.return_value = {"ocs": {"data": {"capabilities": capabilities}}} + return response + + +async def test_returns_sorted_capability_keys(mocker): + """Happy path: handler hits OCS v2 capabilities and returns sorted app keys.""" + _patch_token_validation(mocker) + response = _capabilities_response( + { + "notes": {"api_version": "1.4"}, + "files": {"bigfilechunking": True}, + "core": {"webdav-root": "remote.php/webdav"}, + "tables": {"foo": "bar"}, + } + ) + factory = _patch_outbound_client(mocker, response) + + app = _build_test_app() + client = TestClient(app) + http_response = client.get( + "/api/v1/apps", headers={"Authorization": "Bearer test-token"} + ) + + assert http_response.status_code == 200 + assert http_response.json() == {"apps": ["core", "files", "notes", "tables"]} + + # Regression check: outbound URL is OCS v2 capabilities, NOT v1 cloud/apps. + # /ocs/v1.php/cloud/apps was admin-only + @PasswordConfirmationRequired + # which always 401s for OAuth bearer tokens — that was the bug. + factory.get.assert_awaited_once() + called_path = factory.get.call_args.args[0] + assert called_path == "/ocs/v2.php/cloud/capabilities" + assert "/cloud/apps" not in called_path + + # Bearer token forwarded so authenticated capabilities are returned + factory_call_kwargs = factory.call_args.kwargs + assert factory_call_kwargs["headers"] == {"Authorization": "Bearer test-token"} + + +async def test_empty_capabilities_returns_empty_list(mocker): + """Empty capabilities map → empty apps list, not an error.""" + _patch_token_validation(mocker) + response = _capabilities_response({}) + _patch_outbound_client(mocker, response) + + app = _build_test_app() + client = TestClient(app) + http_response = client.get( + "/api/v1/apps", headers={"Authorization": "Bearer test-token"} + ) + + assert http_response.status_code == 200 + assert http_response.json() == {"apps": []} + + +async def test_ocs_error_returns_500_with_sanitized_message(mocker): + """Non-200 from Nextcloud OCS surfaces as a generic 500 to the caller.""" + _patch_token_validation(mocker) + error_response = MagicMock() + error_response.status_code = 503 + _patch_outbound_client(mocker, error_response) + + app = _build_test_app() + client = TestClient(app) + http_response = client.get( + "/api/v1/apps", headers={"Authorization": "Bearer test-token"} + ) + + assert http_response.status_code == 500 + body = http_response.json() + assert body["error"] == "Internal error" + # _sanitize_error_for_client returns a generic message; specific status + # code must NOT leak to the client + assert "503" not in body["message"] + + +async def test_missing_nextcloud_host_returns_500(mocker): + """Misconfigured oauth_context (no nextcloud_host) surfaces as 500.""" + _patch_token_validation(mocker) + response = _capabilities_response({}) + _patch_outbound_client(mocker, response) + + app = Starlette(routes=[Route("/api/v1/apps", get_installed_apps, methods=["GET"])]) + app.state.oauth_context = {"config": {}} # no nextcloud_host + + client = TestClient(app) + http_response = client.get( + "/api/v1/apps", headers={"Authorization": "Bearer test-token"} + ) + + assert http_response.status_code == 500 + + +async def test_missing_authorization_returns_500(mocker): + """When token validation passes but Authorization header is absent, the + handler can't construct the outbound bearer header — returns 500 with a + sanitized message.""" + _patch_token_validation(mocker) + response = _capabilities_response({}) + _patch_outbound_client(mocker, response) + + app = _build_test_app() + client = TestClient(app) + # No Authorization header + http_response = client.get("/api/v1/apps") + + assert http_response.status_code == 500