Merge pull request #760 from cbcoutinho/fix/webhook-apps-use-capabilities
fix(webhooks): use OCS v2 capabilities for /api/v1/apps
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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})
|
||||
|
||||
|
||||
@@ -933,3 +933,210 @@ 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)
|
||||
|
||||
# After login the oidc app issues a JS-driven re-authorize chain
|
||||
# (/apps/oidc/redirect → /apps/oidc/authorize → /apps/oidc/consent).
|
||||
# networkidle can fire during the gap before consent renders, so
|
||||
# poll for either the consent page or the callback hit before
|
||||
# bailing.
|
||||
start = time.time()
|
||||
consent_handled = False
|
||||
while state not in auth_states:
|
||||
if time.time() - start > 60:
|
||||
raise TimeoutError("Timeout waiting for OAuth callback")
|
||||
if not consent_handled:
|
||||
try:
|
||||
handled = await _handle_oauth_consent_screen(page, username)
|
||||
if handled:
|
||||
consent_handled = True
|
||||
except Exception as e:
|
||||
logger.warning("Consent screen handling raised: %s", e)
|
||||
consent_handled = True # don't retry indefinitely
|
||||
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"]
|
||||
|
||||
@@ -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.login_flow]
|
||||
|
||||
|
||||
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
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user