From 8a0d06107e2f08bde8503e7d6708cb277e375886 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 22 Apr 2026 19:50:13 +0200 Subject: [PATCH 1/5] fix(api): use stored app password for chunk-context and pdf-preview MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The /api/v1/chunk-context and /api/v1/pdf-preview handlers in api/visualization.py forwarded the incoming OAuth bearer directly to Nextcloud via NextcloudClient.from_token. In multi-user BasicAuth mode Nextcloud has no validator for those bearers on Notes/WebDAV, so it treats the request as anonymous and returns 401 — surfaced to the user as a 500 from /apps/astrolabe/api/chunk-context. Search worked because it only hits Qdrant. Architecturally, OAuth is only for Astrolabe→MCP server; MCP server→ Nextcloud always uses the per-user app password stored during provision (background sync already does this via vector.oauth_sync). - Resolve the Nextcloud client through get_user_client_basic_auth in both get_chunk_context and get_pdf_preview, surfacing NotProvisionedError as a clean 401 instead of opaque 500. - Apply the same fix to the session-cookie variant in auth/viz_routes.chunk_context_endpoint for the internal viz UI. Tests: - New unit file test_management_chunk_context_endpoint.py, including a regression guard that asserts get_user_client_basic_auth is awaited (so reverting to from_token fails without needing a live Nextcloud). - Updated test_management_pdf_preview_endpoint.py to mock the new auth path (drops extract_bearer_token / NextcloudClient.from_token patches). - New integration test test_astrolabe_chunk_context.py drives the full chain (browser → Astrolabe → MCP → Nextcloud) in multi-user BasicAuth mode, plus bare-bones 401 checks on the MCP endpoint. Full unit suite: 546 passed. Companion PR on astrolabe (cbcoutinho/astrolabe#66) sends the Nextcloud UID as loginName in the app-password POST body so the stored record is complete. Submodule bump to that branch will follow once CI reproduces the failure on the old submodule. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/api/visualization.py | 50 ++-- nextcloud_mcp_server/auth/viz_routes.py | 28 +- .../test_astrolabe_chunk_context.py | 241 +++++++++++++++ .../test_management_chunk_context_endpoint.py | 282 ++++++++++++++++++ .../test_management_pdf_preview_endpoint.py | 94 ++---- 5 files changed, 597 insertions(+), 98 deletions(-) create mode 100644 tests/integration/test_astrolabe_chunk_context.py create mode 100644 tests/unit/test_management_chunk_context_endpoint.py diff --git a/nextcloud_mcp_server/api/visualization.py b/nextcloud_mcp_server/api/visualization.py index 2b0650c1..242c7c13 100644 --- a/nextcloud_mcp_server/api/visualization.py +++ b/nextcloud_mcp_server/api/visualization.py @@ -23,10 +23,8 @@ from nextcloud_mcp_server.api.management import ( _parse_int_param, _sanitize_error_for_client, _validate_query_string, - extract_bearer_token, validate_token_and_get_user, ) -from nextcloud_mcp_server.client import NextcloudClient from nextcloud_mcp_server.config import get_settings from nextcloud_mcp_server.embedding.service import get_embedding_service from nextcloud_mcp_server.search import ( @@ -34,6 +32,10 @@ from nextcloud_mcp_server.search import ( SemanticSearchAlgorithm, ) from nextcloud_mcp_server.search.context import get_chunk_with_context +from nextcloud_mcp_server.vector.oauth_sync import ( + NotProvisionedError, + get_user_client_basic_auth, +) from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client from nextcloud_mcp_server.vector.visualization import compute_pca_coordinates @@ -512,11 +514,6 @@ async def get_chunk_context(request: Request) -> JSONResponse: # Convert doc_id to int if possible (most IDs are int) doc_id_val: str | int = int(doc_id) if doc_id.isdigit() else doc_id - # Get bearer token for client initialization - token = extract_bearer_token(request) - if not token: - raise ValueError("Missing token") - # Get Nextcloud host from OAuth context oauth_ctx = request.app.state.oauth_context nextcloud_host = oauth_ctx.get("config", {}).get("nextcloud_host", "") @@ -524,10 +521,19 @@ async def get_chunk_context(request: Request) -> JSONResponse: if not nextcloud_host: raise ValueError("Nextcloud host not configured") - # Initialize authenticated Nextcloud client - async with NextcloudClient.from_token( - base_url=nextcloud_host, token=token, username=user_id - ) as nc_client: + # Use the user's stored app password for Nextcloud calls. + # The OAuth bearer is only used to authenticate Astrolabe → MCP Server; + # MCP Server → Nextcloud always uses the app password provisioned + # during the authorization step. + try: + nc_client = await get_user_client_basic_auth(user_id, nextcloud_host) + except NotProvisionedError as e: + return JSONResponse( + {"success": False, "error": str(e)}, + status_code=401, + ) + + async with nc_client: chunk_context = await get_chunk_with_context( nc_client=nc_client, user_id=user_id, @@ -687,11 +693,6 @@ async def get_pdf_preview(request: Request) -> JSONResponse: except ValueError as e: return JSONResponse({"success": False, "error": str(e)}, status_code=400) - # Get bearer token for WebDAV authentication - token = extract_bearer_token(request) - if not token: - raise ValueError("Missing token") - # Get Nextcloud host from OAuth context oauth_ctx = request.app.state.oauth_context nextcloud_host = oauth_ctx.get("config", {}).get("nextcloud_host", "") @@ -699,10 +700,19 @@ async def get_pdf_preview(request: Request) -> JSONResponse: if not nextcloud_host: raise ValueError("Nextcloud host not configured") - # Download PDF via WebDAV using user's token - async with NextcloudClient.from_token( - base_url=nextcloud_host, token=token, username=user_id - ) as nc_client: + # Use the user's stored app password for Nextcloud calls. + # The OAuth bearer is only used to authenticate Astrolabe → MCP Server; + # MCP Server → Nextcloud always uses the app password provisioned + # during the authorization step. + try: + nc_client = await get_user_client_basic_auth(user_id, nextcloud_host) + except NotProvisionedError as e: + return JSONResponse( + {"success": False, "error": str(e)}, + status_code=401, + ) + + async with nc_client: pdf_bytes, _ = await nc_client.webdav.read_file(file_path) # Check file size limit (50 MB) diff --git a/nextcloud_mcp_server/auth/viz_routes.py b/nextcloud_mcp_server/auth/viz_routes.py index fe45327d..d072c373 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -34,6 +34,10 @@ from nextcloud_mcp_server.search import ( SemanticSearchAlgorithm, ) from nextcloud_mcp_server.search.context import get_chunk_with_context +from nextcloud_mcp_server.vector.oauth_sync import ( + NotProvisionedError, + get_user_client_basic_auth, +) from nextcloud_mcp_server.vector.pca import PCA from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client @@ -554,12 +558,28 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: # Convert doc_id to int (all document types use int IDs) doc_id_int = int(doc_id) - # Get authenticated Nextcloud client - # Use context expansion module to fetch chunk with surrounding context - async with await _get_authenticated_client_for_userinfo(request) as nc_client: + user_id = request.user.display_name + settings = get_settings() + nextcloud_host = settings.nextcloud_host + if not nextcloud_host: + raise RuntimeError("Nextcloud host not configured") + + # Use the user's stored app password for Nextcloud calls. + # The session cookie only authenticates the browser → MCP Server hop; + # MCP Server → Nextcloud always uses the app password provisioned + # during the authorization step. + try: + nc_client = await get_user_client_basic_auth(user_id, nextcloud_host) + except NotProvisionedError as e: + return JSONResponse( + {"success": False, "error": str(e)}, + status_code=401, + ) + + async with nc_client: chunk_context = await get_chunk_with_context( nc_client=nc_client, - user_id=request.user.display_name, # User ID from auth + user_id=user_id, doc_id=doc_id_int, doc_type=doc_type, chunk_start=start, diff --git a/tests/integration/test_astrolabe_chunk_context.py b/tests/integration/test_astrolabe_chunk_context.py new file mode 100644 index 00000000..ddcff90a --- /dev/null +++ b/tests/integration/test_astrolabe_chunk_context.py @@ -0,0 +1,241 @@ +"""Integration test for the chunk-context HTTP path in multi-user BasicAuth mode. + +Cross-system interface test: Tests the MCP server's /api/v1/chunk-context +handler via the Astrolabe PHP app (/apps/astrolabe/api/chunk-context). Astrolabe +source lives in ./third_party/astrolabe (submodule) and is installed during +test setup by app-hooks/post-installation/20-install-astrolabe-app.sh. + +Regression test: Prior to this test, `get_chunk_context` in +nextcloud_mcp_server/api/visualization.py forwarded the OAuth bearer token +directly to Nextcloud via NextcloudClient.from_token(...). In multi-user +BasicAuth deployments, Nextcloud doesn't validate that bearer on the Notes +API, so the handler returned 404 (wrapped by Astrolabe as 500). This test +exercises the full chain: + + browser session → astrolabe → MCP server (OAuth bearer) → + get_user_client_basic_auth → Nextcloud (app password BasicAuth) → note + +so a regression to from_token-style auth would surface as a 500/404 from +Astrolabe instead of a 200 with chunk_text. +""" + +import json +import logging +import re +import uuid + +import pytest + +from tests.conftest import create_mcp_client_session +from tests.integration.test_astrolabe_multi_user_background_sync import ( + complete_astrolabe_authorization, + login_to_nextcloud, +) +from tests.integration.test_astrolabe_plotly_visualization import wait_for_vector_sync + +logger = logging.getLogger(__name__) + +pytestmark = [pytest.mark.integration, pytest.mark.multi_user_basic] + + +def _build_basic_auth_header(username: str, password: str) -> str: + import base64 + + credentials = base64.b64encode(f"{username}:{password}".encode()).decode("utf-8") + return f"Basic {credentials}" + + +@pytest.mark.timeout(300) +async def test_chunk_context_endpoint_uses_app_password( + browser, + test_users_setup, + configure_astrolabe_for_mcp_server, +): + """Astrolabe /api/chunk-context must return 200 with populated chunk_text. + + Covers the regression where the MCP handler used BearerAuth against + Nextcloud instead of BasicAuth with the stored app password. + """ + await configure_astrolabe_for_mcp_server( + mcp_server_internal_url="http://mcp-multi-user-basic:8000", + mcp_server_public_url="http://localhost:8003", + ) + + username = "alice" + password = test_users_setup[username]["password"] + note_id = None + unique_term = f"chunk_ctx_test_{uuid.uuid4().hex[:8]}" + + context = await browser.new_context(ignore_https_errors=True) + page = await context.new_page() + + try: + await login_to_nextcloud(page, username, password) + auth_result = await complete_astrolabe_authorization(page, username, password) + assert auth_result["step1"], "OAuth authorization did not complete" + assert auth_result["step2"], "App password provisioning did not complete" + + auth_header = _build_basic_auth_header(username, password) + async with create_mcp_client_session( + url="http://localhost:8003/mcp", + headers={"Authorization": auth_header}, + client_name="Alice Chunk Context Test", + ) as mcp_client: + initial_sync = await mcp_client.call_tool("nc_get_vector_sync_status", {}) + if initial_sync.isError: + pytest.skip("Vector sync not enabled on mcp-multi-user-basic") + initial_count = json.loads(initial_sync.content[0].text).get( + "indexed_count", 0 + ) + + note_body = ( + f"# Chunk Context Regression Test\n\n" + f"This document exists to verify that the chunk-context HTTP " + f"endpoint can re-fetch it after indexing. " + f"Unique marker: {unique_term}.\n\n" + f"Paragraph two exists so there is a plausible surrounding " + f"context to slice. Lorem ipsum dolor sit amet, consectetur " + f"adipiscing elit." + ) + note_response = await mcp_client.call_tool( + "nc_notes_create_note", + { + "title": f"Chunk Context Test {unique_term}", + "content": note_body, + "category": "Test", + }, + ) + assert not note_response.isError, f"Create note failed: {note_response}" + note_id = json.loads(note_response.content[0].text).get("id") + assert note_id is not None + + sync_complete, status = await wait_for_vector_sync( + mcp_client, initial_count, timeout_seconds=90 + ) + assert sync_complete, f"Vector sync did not complete: {status}" + + # Use the browser's session to drive Astrolabe end-to-end, the way a + # real user would: this exercises astrolabe's OAuth token retrieval + # and the MCP server's handler under a real bearer. + search_resp = await page.request.get( + f"http://localhost:8080/apps/astrolabe/api/search" + f"?query={unique_term}&algorithm=hybrid&limit=5&include_pca=false" + ) + assert search_resp.ok, ( + f"Astrolabe search failed: {search_resp.status} {await search_resp.text()}" + ) + search_data = await search_resp.json() + assert search_data.get("success"), f"Search returned error: {search_data}" + results = search_data.get("results") or [] + note_result = next( + ( + r + for r in results + if r.get("doc_type") == "note" and str(r.get("id")) == str(note_id) + ), + None, + ) + assert note_result is not None, ( + f"Note {note_id} with term '{unique_term}' not found in search results: " + f"{results}" + ) + start = note_result.get("chunk_start_offset") + end = note_result.get("chunk_end_offset") + assert start is not None and end is not None, ( + f"Search result missing chunk offsets: {note_result}" + ) + + chunk_resp = await page.request.get( + "http://localhost:8080/apps/astrolabe/api/chunk-context", + params={ + "doc_type": "note", + "doc_id": str(note_id), + "start": str(start), + "end": str(end), + }, + ) + assert chunk_resp.status == 200, ( + f"chunk-context returned {chunk_resp.status}, body: " + f"{await chunk_resp.text()}" + ) + chunk_data = await chunk_resp.json() + assert chunk_data.get("success") is True, f"Response: {chunk_data}" + chunk_text = chunk_data.get("chunk_text") or "" + assert chunk_text, f"Empty chunk_text in response: {chunk_data}" + # The unique marker is in the indexed body, so it must appear in the + # chunk text or in the surrounding context. + combined = ( + chunk_text + + chunk_data.get("before_context", "") + + chunk_data.get("after_context", "") + ) + assert re.search(re.escape(unique_term), combined), ( + f"Unique term {unique_term} missing from chunk+context: " + f"chunk_text={chunk_text!r}, before={chunk_data.get('before_context')!r}, " + f"after={chunk_data.get('after_context')!r}" + ) + finally: + try: + if note_id is not None: + auth_header = _build_basic_auth_header(username, password) + async with create_mcp_client_session( + url="http://localhost:8003/mcp", + headers={"Authorization": auth_header}, + client_name="Alice Chunk Context Cleanup", + ) as mcp_client: + await mcp_client.call_tool( + "nc_notes_delete_note", {"note_id": note_id} + ) + except Exception as cleanup_err: + logger.warning(f"Cleanup failed for note {note_id}: {cleanup_err}") + await context.close() + + +@pytest.mark.timeout(60) +async def test_chunk_context_endpoint_requires_authentication(): + """Direct HTTP hit at /api/v1/chunk-context without a bearer must 401.""" + import httpx + + async with httpx.AsyncClient() as client: + response = await client.get( + "http://localhost:8003/api/v1/chunk-context", + params={ + "doc_type": "note", + "doc_id": "1", + "start": "0", + "end": "10", + }, + ) + assert response.status_code == 401, ( + f"Expected 401 without auth, got {response.status_code}: {response.text}" + ) + + +@pytest.mark.timeout(60) +async def test_chunk_context_endpoint_handles_missing_app_password(): + """Bearer token for a user with no provisioned app password must produce a + clean 401 (NotProvisionedError path), not a 500 or opaque upstream error. + + We simulate "user has not provisioned" by sending a syntactically valid + bearer that the token verifier will reject. The exact error class doesn't + matter for this check — what matters is the handler never 500s on the + missing-credential path. + """ + import httpx + + async with httpx.AsyncClient() as client: + response = await client.get( + "http://localhost:8003/api/v1/chunk-context", + params={ + "doc_type": "note", + "doc_id": "1", + "start": "0", + "end": "10", + }, + headers={"Authorization": "Bearer invalid.token.value"}, + ) + # Must be 401 (unauthorized) or 404 (route guard), not 500. + assert response.status_code in (401, 404), ( + f"Expected 401/404 for invalid bearer, got {response.status_code}: " + f"{response.text}" + ) diff --git a/tests/unit/test_management_chunk_context_endpoint.py b/tests/unit/test_management_chunk_context_endpoint.py new file mode 100644 index 00000000..04d3b0fc --- /dev/null +++ b/tests/unit/test_management_chunk_context_endpoint.py @@ -0,0 +1,282 @@ +""" +Unit tests for Management API chunk-context endpoint. + +Tests the /api/v1/chunk-context endpoint focusing on: +- Parameter validation (doc_type, doc_id, start, end, context) +- OAuth token validation +- Nextcloud credential path (must use get_user_client_basic_auth, not + NextcloudClient.from_token — see regression in api/visualization.py) +- Error handling (missing params, invalid ranges, missing credentials) +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from starlette.applications import Starlette +from starlette.routing import Route +from starlette.testclient import TestClient + +from nextcloud_mcp_server.api.visualization import get_chunk_context +from nextcloud_mcp_server.vector.oauth_sync import NotProvisionedError + +pytestmark = pytest.mark.unit + + +def create_test_app(): + """Create a test Starlette app with the chunk-context endpoint.""" + app = Starlette( + routes=[ + Route("/api/v1/chunk-context", get_chunk_context, methods=["GET"]), + ] + ) + app.state.oauth_context = {"config": {"nextcloud_host": "http://localhost:8080"}} + return app + + +def _make_mock_chunk_context(chunk_text="chunk", before="before", after="after"): + """Mock the ChunkContext dataclass with enough fields for the handler.""" + ctx = MagicMock() + ctx.chunk_text = chunk_text + ctx.before_context = before + ctx.after_context = after + ctx.has_before_truncation = False + ctx.has_after_truncation = False + ctx.page_number = None + ctx.chunk_index = 0 + ctx.total_chunks = 1 + return ctx + + +def _make_mock_nc_client(): + """Mock NextcloudClient that supports `async with`.""" + mock_client = MagicMock() + mock_client.__aenter__ = AsyncMock(return_value=mock_client) + mock_client.__aexit__ = AsyncMock(return_value=None) + return mock_client + + +class TestChunkContextParameterValidation: + """Tests for parameter validation in the chunk-context endpoint.""" + + def test_missing_params_returns_400(self): + with patch( + "nextcloud_mcp_server.api.visualization.validate_token_and_get_user", + new_callable=AsyncMock, + return_value=("testuser", True), + ): + app = create_test_app() + client = TestClient(app) + # Missing end + response = client.get( + "/api/v1/chunk-context?doc_type=note&doc_id=1&start=0", + headers={"Authorization": "Bearer test-token"}, + ) + assert response.status_code == 400 + data = response.json() + assert data["success"] is False + assert "required parameters" in data["error"].lower() + + def test_end_less_than_or_equal_start_returns_400(self): + with patch( + "nextcloud_mcp_server.api.visualization.validate_token_and_get_user", + new_callable=AsyncMock, + return_value=("testuser", True), + ): + app = create_test_app() + client = TestClient(app) + response = client.get( + "/api/v1/chunk-context?doc_type=note&doc_id=1&start=100&end=100", + headers={"Authorization": "Bearer test-token"}, + ) + assert response.status_code == 400 + data = response.json() + assert data["success"] is False + assert "end must be greater than start" in data["error"].lower() + + def test_non_numeric_start_returns_400(self): + with patch( + "nextcloud_mcp_server.api.visualization.validate_token_and_get_user", + new_callable=AsyncMock, + return_value=("testuser", True), + ): + app = create_test_app() + client = TestClient(app) + response = client.get( + "/api/v1/chunk-context?doc_type=note&doc_id=1&start=abc&end=10", + headers={"Authorization": "Bearer test-token"}, + ) + assert response.status_code == 400 + + +class TestChunkContextTokenValidation: + """Tests for OAuth token validation.""" + + def test_missing_token_returns_401(self): + with patch( + "nextcloud_mcp_server.api.visualization.validate_token_and_get_user", + new_callable=AsyncMock, + side_effect=ValueError("Missing Authorization header"), + ): + app = create_test_app() + client = TestClient(app) + response = client.get( + "/api/v1/chunk-context?doc_type=note&doc_id=1&start=0&end=10" + ) + assert response.status_code == 401 + data = response.json() + assert data["error"] == "Unauthorized" + + def test_invalid_token_returns_401(self): + with patch( + "nextcloud_mcp_server.api.visualization.validate_token_and_get_user", + new_callable=AsyncMock, + side_effect=Exception("Token expired"), + ): + app = create_test_app() + client = TestClient(app) + response = client.get( + "/api/v1/chunk-context?doc_type=note&doc_id=1&start=0&end=10", + headers={"Authorization": "Bearer invalid-token"}, + ) + assert response.status_code == 401 + + +class TestChunkContextCredentialPath: + """Regression tests: the handler MUST use get_user_client_basic_auth. + + The earlier bug (reproduced in homelab 2026-04-22) forwarded the OAuth + bearer directly to Nextcloud via NextcloudClient.from_token. That works in + single-user env-BasicAuth mode but fails with 401 in multi-user BasicAuth + mode because Nextcloud won't validate that bearer on the Notes API. + """ + + def test_successful_fetch_uses_app_password_client(self): + """Handler must build its NC client via get_user_client_basic_auth and + return the chunk text from get_chunk_with_context.""" + mock_nc_client = _make_mock_nc_client() + mock_ctx = _make_mock_chunk_context( + chunk_text="hello world", + before="before the chunk", + after="after the chunk", + ) + + with ( + patch( + "nextcloud_mcp_server.api.visualization.validate_token_and_get_user", + new_callable=AsyncMock, + return_value=("testuser", True), + ), + patch( + "nextcloud_mcp_server.api.visualization.get_user_client_basic_auth", + new_callable=AsyncMock, + return_value=mock_nc_client, + ) as mock_basic_auth, + patch( + "nextcloud_mcp_server.api.visualization.get_chunk_with_context", + new_callable=AsyncMock, + return_value=mock_ctx, + ), + ): + app = create_test_app() + client = TestClient(app) + response = client.get( + "/api/v1/chunk-context?doc_type=note&doc_id=42&start=0&end=11", + headers={"Authorization": "Bearer test-token"}, + ) + + assert response.status_code == 200 + data = response.json() + assert data["success"] is True + assert data["chunk_text"] == "hello world" + assert data["before_context"] == "before the chunk" + assert data["after_context"] == "after the chunk" + + # Regression guard: credential resolution must go through the + # app-password helper, not NextcloudClient.from_token. + mock_basic_auth.assert_awaited_once() + args, kwargs = mock_basic_auth.call_args + positional = list(args) + if "user_id" in kwargs: + positional.insert(0, kwargs["user_id"]) + assert positional[0] == "testuser" + + def test_not_provisioned_returns_401(self): + """If the user has no stored app password, the handler must surface + NotProvisionedError as a clean 401 (not 500).""" + with ( + patch( + "nextcloud_mcp_server.api.visualization.validate_token_and_get_user", + new_callable=AsyncMock, + return_value=("testuser", True), + ), + patch( + "nextcloud_mcp_server.api.visualization.get_user_client_basic_auth", + new_callable=AsyncMock, + side_effect=NotProvisionedError( + "User testuser has not provisioned an app password." + ), + ), + ): + app = create_test_app() + client = TestClient(app) + response = client.get( + "/api/v1/chunk-context?doc_type=note&doc_id=1&start=0&end=10", + headers={"Authorization": "Bearer test-token"}, + ) + assert response.status_code == 401 + data = response.json() + assert data["success"] is False + assert "app password" in data["error"].lower() + + def test_chunk_fetch_returns_none_yields_404(self): + """When get_chunk_with_context returns None (doc missing / offsets + out of range), the handler reports 404 with a structured error body.""" + mock_nc_client = _make_mock_nc_client() + + with ( + patch( + "nextcloud_mcp_server.api.visualization.validate_token_and_get_user", + new_callable=AsyncMock, + return_value=("testuser", True), + ), + patch( + "nextcloud_mcp_server.api.visualization.get_user_client_basic_auth", + new_callable=AsyncMock, + return_value=mock_nc_client, + ), + patch( + "nextcloud_mcp_server.api.visualization.get_chunk_with_context", + new_callable=AsyncMock, + return_value=None, + ), + ): + app = create_test_app() + client = TestClient(app) + response = client.get( + "/api/v1/chunk-context?doc_type=note&doc_id=999&start=0&end=10", + headers={"Authorization": "Bearer test-token"}, + ) + assert response.status_code == 404 + data = response.json() + assert data["success"] is False + assert "failed to fetch chunk context" in data["error"].lower() + + +class TestChunkContextConfigErrors: + """Tests for configuration failure paths.""" + + def test_missing_nextcloud_host_config(self): + with patch( + "nextcloud_mcp_server.api.visualization.validate_token_and_get_user", + new_callable=AsyncMock, + return_value=("testuser", True), + ): + app = create_test_app() + app.state.oauth_context = {"config": {"nextcloud_host": ""}} + client = TestClient(app) + response = client.get( + "/api/v1/chunk-context?doc_type=note&doc_id=1&start=0&end=10", + headers={"Authorization": "Bearer test-token"}, + ) + # Handler wraps ValueError via _sanitize_error_for_client → 500 + assert response.status_code == 500 diff --git a/tests/unit/test_management_pdf_preview_endpoint.py b/tests/unit/test_management_pdf_preview_endpoint.py index b0e80079..02e1deb3 100644 --- a/tests/unit/test_management_pdf_preview_endpoint.py +++ b/tests/unit/test_management_pdf_preview_endpoint.py @@ -72,10 +72,6 @@ class TestPdfPreviewParameterValidation: new_callable=AsyncMock, return_value=("testuser", True), ), - patch( - "nextcloud_mcp_server.api.visualization.extract_bearer_token", - return_value="test-token", - ), ): app = create_test_app() client = TestClient(app) @@ -97,10 +93,6 @@ class TestPdfPreviewParameterValidation: new_callable=AsyncMock, return_value=("testuser", True), ), - patch( - "nextcloud_mcp_server.api.visualization.extract_bearer_token", - return_value="test-token", - ), ): app = create_test_app() client = TestClient(app) @@ -130,10 +122,6 @@ class TestPdfPreviewParameterValidation: new_callable=AsyncMock, return_value=("testuser", True), ), - patch( - "nextcloud_mcp_server.api.visualization.extract_bearer_token", - return_value="test-token", - ), ): app = create_test_app() client = TestClient(app) @@ -163,10 +151,6 @@ class TestPdfPreviewParameterValidation: new_callable=AsyncMock, return_value=("testuser", True), ), - patch( - "nextcloud_mcp_server.api.visualization.extract_bearer_token", - return_value="test-token", - ), ): app = create_test_app() client = TestClient(app) @@ -240,11 +224,8 @@ class TestPdfPreviewRendering: return_value=("testuser", True), ), patch( - "nextcloud_mcp_server.api.visualization.extract_bearer_token", - return_value="test-token", - ), - patch( - "nextcloud_mcp_server.client.NextcloudClient.from_token", + "nextcloud_mcp_server.api.visualization.get_user_client_basic_auth", + new_callable=AsyncMock, return_value=mock_nc_client, ), ): @@ -289,11 +270,8 @@ class TestPdfPreviewRendering: return_value=("testuser", True), ), patch( - "nextcloud_mcp_server.api.visualization.extract_bearer_token", - return_value="test-token", - ), - patch( - "nextcloud_mcp_server.client.NextcloudClient.from_token", + "nextcloud_mcp_server.api.visualization.get_user_client_basic_auth", + new_callable=AsyncMock, return_value=mock_nc_client, ), ): @@ -329,11 +307,8 @@ class TestPdfPreviewRendering: return_value=("testuser", True), ), patch( - "nextcloud_mcp_server.api.visualization.extract_bearer_token", - return_value="test-token", - ), - patch( - "nextcloud_mcp_server.client.NextcloudClient.from_token", + "nextcloud_mcp_server.api.visualization.get_user_client_basic_auth", + new_callable=AsyncMock, return_value=mock_nc_client, ), ): @@ -368,11 +343,8 @@ class TestPdfPreviewRendering: return_value=("testuser", True), ), patch( - "nextcloud_mcp_server.api.visualization.extract_bearer_token", - return_value="test-token", - ), - patch( - "nextcloud_mcp_server.client.NextcloudClient.from_token", + "nextcloud_mcp_server.api.visualization.get_user_client_basic_auth", + new_callable=AsyncMock, return_value=mock_nc_client, ), ): @@ -412,11 +384,8 @@ class TestPdfPreviewEdgeCases: return_value=("testuser", True), ), patch( - "nextcloud_mcp_server.api.visualization.extract_bearer_token", - return_value="test-token", - ), - patch( - "nextcloud_mcp_server.client.NextcloudClient.from_token", + "nextcloud_mcp_server.api.visualization.get_user_client_basic_auth", + new_callable=AsyncMock, return_value=mock_nc_client, ), ): @@ -442,10 +411,6 @@ class TestPdfPreviewEdgeCases: new_callable=AsyncMock, return_value=("testuser", True), ), - patch( - "nextcloud_mcp_server.api.visualization.extract_bearer_token", - return_value="test-token", - ), ): app = create_test_app() # Override with empty config @@ -481,11 +446,8 @@ class TestPdfPreviewEdgeCases: return_value=("testuser", True), ), patch( - "nextcloud_mcp_server.api.visualization.extract_bearer_token", - return_value="test-token", - ), - patch( - "nextcloud_mcp_server.client.NextcloudClient.from_token", + "nextcloud_mcp_server.api.visualization.get_user_client_basic_auth", + new_callable=AsyncMock, return_value=mock_nc_client, ), ): @@ -523,11 +485,8 @@ class TestPdfPreviewEdgeCases: return_value=("testuser", True), ), patch( - "nextcloud_mcp_server.api.visualization.extract_bearer_token", - return_value="test-token", - ), - patch( - "nextcloud_mcp_server.client.NextcloudClient.from_token", + "nextcloud_mcp_server.api.visualization.get_user_client_basic_auth", + new_callable=AsyncMock, return_value=mock_nc_client, ), ): @@ -560,10 +519,6 @@ class TestPdfPreviewSecurityValidation: new_callable=AsyncMock, return_value=("testuser", True), ), - patch( - "nextcloud_mcp_server.api.visualization.extract_bearer_token", - return_value="test-token", - ), ): app = create_test_app() client = TestClient(app) @@ -610,11 +565,8 @@ class TestPdfPreviewSecurityValidation: return_value=("testuser", True), ), patch( - "nextcloud_mcp_server.api.visualization.extract_bearer_token", - return_value="test-token", - ), - patch( - "nextcloud_mcp_server.client.NextcloudClient.from_token", + "nextcloud_mcp_server.api.visualization.get_user_client_basic_auth", + new_callable=AsyncMock, return_value=mock_nc_client, ), ): @@ -652,11 +604,8 @@ class TestPdfPreviewSecurityValidation: return_value=("testuser", True), ), patch( - "nextcloud_mcp_server.api.visualization.extract_bearer_token", - return_value="test-token", - ), - patch( - "nextcloud_mcp_server.client.NextcloudClient.from_token", + "nextcloud_mcp_server.api.visualization.get_user_client_basic_auth", + new_callable=AsyncMock, return_value=mock_nc_client, ), ): @@ -696,11 +645,8 @@ class TestPdfPreviewSecurityValidation: return_value=("testuser", True), ), patch( - "nextcloud_mcp_server.api.visualization.extract_bearer_token", - return_value="test-token", - ), - patch( - "nextcloud_mcp_server.client.NextcloudClient.from_token", + "nextcloud_mcp_server.api.visualization.get_user_client_basic_auth", + new_callable=AsyncMock, return_value=mock_nc_client, ), ): From bea5c3f5eec352f28ed64c0494709ec3f09f0ae1 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 22 Apr 2026 20:15:05 +0200 Subject: [PATCH 2/5] test: include Nextcloud CSRF token in chunk-context integration test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Astrolabe's ApiController endpoints (search, chunk-context) require a CSRF `requesttoken` header — axios picks it up from OC.requestToken automatically in the SPA, but page.request.get() does not. The first CI run failed on the search step with 412 CSRF check failed before reaching the chunk-context assertion that was supposed to surface the handler bug. Load the Astrolabe page, read OC.requestToken, and pass it on both calls. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../test_astrolabe_chunk_context.py | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_astrolabe_chunk_context.py b/tests/integration/test_astrolabe_chunk_context.py index ddcff90a..42523c6f 100644 --- a/tests/integration/test_astrolabe_chunk_context.py +++ b/tests/integration/test_astrolabe_chunk_context.py @@ -116,10 +116,24 @@ async def test_chunk_context_endpoint_uses_app_password( # Use the browser's session to drive Astrolabe end-to-end, the way a # real user would: this exercises astrolabe's OAuth token retrieval - # and the MCP server's handler under a real bearer. + # and the MCP server's handler under a real bearer. Nextcloud's + # controllers require a CSRF token (`requesttoken` header) that the + # rendered SPA picks up from `OC.requestToken`; direct page.request + # calls don't get it for free, so we load an Astrolabe page and + # forward the token on each subsequent API call. + await page.goto( + "http://localhost:8080/apps/astrolabe/", wait_until="networkidle" + ) + request_token = await page.evaluate("window.OC && OC.requestToken") + assert request_token, ( + "Could not read OC.requestToken from Astrolabe page — is the user logged in?" + ) + csrf_headers = {"requesttoken": request_token} + search_resp = await page.request.get( f"http://localhost:8080/apps/astrolabe/api/search" - f"?query={unique_term}&algorithm=hybrid&limit=5&include_pca=false" + f"?query={unique_term}&algorithm=hybrid&limit=5&include_pca=false", + headers=csrf_headers, ) assert search_resp.ok, ( f"Astrolabe search failed: {search_resp.status} {await search_resp.text()}" @@ -153,6 +167,7 @@ async def test_chunk_context_endpoint_uses_app_password( "start": str(start), "end": str(end), }, + headers=csrf_headers, ) assert chunk_resp.status == 200, ( f"chunk-context returned {chunk_resp.status}, body: " From 06d871ce2225ad0c4adcc41a6c5ba9708cce2fd3 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 22 Apr 2026 20:17:34 +0200 Subject: [PATCH 3/5] test: address chunk-context review comments - Rename test_chunk_context_endpoint_handles_missing_app_password to test_chunk_context_endpoint_rejects_invalid_bearer so it reflects what is actually exercised: an invalid bearer is rejected upfront at validate_token_and_get_user, not at the NotProvisionedError branch. The NotProvisionedError path is covered by the corresponding unit test in test_management_chunk_context_endpoint.py. - Hoist `import base64` to module level per PEP 8. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../test_astrolabe_chunk_context.py | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/tests/integration/test_astrolabe_chunk_context.py b/tests/integration/test_astrolabe_chunk_context.py index 42523c6f..847df157 100644 --- a/tests/integration/test_astrolabe_chunk_context.py +++ b/tests/integration/test_astrolabe_chunk_context.py @@ -19,6 +19,7 @@ so a regression to from_token-style auth would surface as a 500/404 from Astrolabe instead of a 200 with chunk_text. """ +import base64 import json import logging import re @@ -39,8 +40,6 @@ pytestmark = [pytest.mark.integration, pytest.mark.multi_user_basic] def _build_basic_auth_header(username: str, password: str) -> str: - import base64 - credentials = base64.b64encode(f"{username}:{password}".encode()).decode("utf-8") return f"Basic {credentials}" @@ -227,14 +226,14 @@ async def test_chunk_context_endpoint_requires_authentication(): @pytest.mark.timeout(60) -async def test_chunk_context_endpoint_handles_missing_app_password(): - """Bearer token for a user with no provisioned app password must produce a - clean 401 (NotProvisionedError path), not a 500 or opaque upstream error. +async def test_chunk_context_endpoint_rejects_invalid_bearer(): + """A syntactically-valid-but-unverifiable bearer must not 500. - We simulate "user has not provisioned" by sending a syntactically valid - bearer that the token verifier will reject. The exact error class doesn't - matter for this check — what matters is the handler never 500s on the - missing-credential path. + The NotProvisionedError path (handler reached but no stored app password) + is covered by the corresponding unit test. This check guards the other + rejection path: token validation fails upfront at + `validate_token_and_get_user`, which must turn into a clean 401/404 from + the HTTP layer, not an opaque 500. """ import httpx @@ -249,7 +248,6 @@ async def test_chunk_context_endpoint_handles_missing_app_password(): }, headers={"Authorization": "Bearer invalid.token.value"}, ) - # Must be 401 (unauthorized) or 404 (route guard), not 500. assert response.status_code in (401, 404), ( f"Expected 401/404 for invalid bearer, got {response.status_code}: " f"{response.text}" From b3bd14f183b40ba6ab8594a3e3e8c43118fbb1ac Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 22 Apr 2026 20:26:41 +0200 Subject: [PATCH 4/5] test: tighten regression guard and hoist httpx import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per review: - Hoist `import httpx` out of the two test function bodies and into the module imports at the top of test_astrolabe_chunk_context.py. - Simplify the regression guard in test_management_chunk_context_endpoint.py to use `mock.assert_awaited_once_with(...)` instead of manually unpacking call_args. This is stricter — it fails loudly on signature change — and matches the canonical pattern for asserting mock calls. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/integration/test_astrolabe_chunk_context.py | 5 +---- tests/unit/test_management_chunk_context_endpoint.py | 9 +++------ 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/tests/integration/test_astrolabe_chunk_context.py b/tests/integration/test_astrolabe_chunk_context.py index 847df157..c8ef1eee 100644 --- a/tests/integration/test_astrolabe_chunk_context.py +++ b/tests/integration/test_astrolabe_chunk_context.py @@ -25,6 +25,7 @@ import logging import re import uuid +import httpx import pytest from tests.conftest import create_mcp_client_session @@ -208,8 +209,6 @@ async def test_chunk_context_endpoint_uses_app_password( @pytest.mark.timeout(60) async def test_chunk_context_endpoint_requires_authentication(): """Direct HTTP hit at /api/v1/chunk-context without a bearer must 401.""" - import httpx - async with httpx.AsyncClient() as client: response = await client.get( "http://localhost:8003/api/v1/chunk-context", @@ -235,8 +234,6 @@ async def test_chunk_context_endpoint_rejects_invalid_bearer(): `validate_token_and_get_user`, which must turn into a clean 401/404 from the HTTP layer, not an opaque 500. """ - import httpx - async with httpx.AsyncClient() as client: response = await client.get( "http://localhost:8003/api/v1/chunk-context", diff --git a/tests/unit/test_management_chunk_context_endpoint.py b/tests/unit/test_management_chunk_context_endpoint.py index 04d3b0fc..e387a91a 100644 --- a/tests/unit/test_management_chunk_context_endpoint.py +++ b/tests/unit/test_management_chunk_context_endpoint.py @@ -193,12 +193,9 @@ class TestChunkContextCredentialPath: # Regression guard: credential resolution must go through the # app-password helper, not NextcloudClient.from_token. - mock_basic_auth.assert_awaited_once() - args, kwargs = mock_basic_auth.call_args - positional = list(args) - if "user_id" in kwargs: - positional.insert(0, kwargs["user_id"]) - assert positional[0] == "testuser" + mock_basic_auth.assert_awaited_once_with( + "testuser", "http://localhost:8080" + ) def test_not_provisioned_returns_401(self): """If the user has no stored app password, the handler must surface From 9cf4e16672c285d7858336238a36b7bdcb512579 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 22 Apr 2026 21:55:16 +0200 Subject: [PATCH 5/5] test: poll Astrolabe search until the target note is indexed The previous run on nc32 failed at the search-result assertion because `wait_for_vector_sync` returned on the first indexed-count bump (deck seed cards) before this specific note hit Qdrant. Replace the single search call with a poll that retries every 2s until the unique term returns our note, or times out after 60s with a loud diagnostic. The previously-observed flake would now wait past the deck-card indexing window rather than racing it. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../test_astrolabe_chunk_context.py | 87 ++++++++++++++----- 1 file changed, 65 insertions(+), 22 deletions(-) diff --git a/tests/integration/test_astrolabe_chunk_context.py b/tests/integration/test_astrolabe_chunk_context.py index c8ef1eee..201ca49d 100644 --- a/tests/integration/test_astrolabe_chunk_context.py +++ b/tests/integration/test_astrolabe_chunk_context.py @@ -23,8 +23,10 @@ import base64 import json import logging import re +import time import uuid +import anyio import httpx import pytest @@ -45,6 +47,63 @@ def _build_basic_auth_header(username: str, password: str) -> str: return f"Basic {credentials}" +async def _poll_astrolabe_search_for_note( + page, + unique_term: str, + note_id, + csrf_headers: dict, + timeout_seconds: int = 60, +) -> dict: + """Poll Astrolabe's search endpoint until `note_id` shows up in results. + + `wait_for_vector_sync` only waits for the total indexed count to grow — + it does not guarantee that *this specific* document is visible yet + (observed on nc32 where deck-card seed data indexes first and the new + note arrives in Qdrant a few seconds later). Poll until the unique term + returns our note, or fail loudly with the last response we saw. + """ + deadline = time.monotonic() + timeout_seconds + last_results: list | None = None + attempts = 0 + while time.monotonic() < deadline: + attempts += 1 + search_resp = await page.request.get( + f"http://localhost:8080/apps/astrolabe/api/search" + f"?query={unique_term}&algorithm=hybrid&limit=5&include_pca=false", + headers=csrf_headers, + ) + assert search_resp.ok, ( + f"Astrolabe search failed on attempt {attempts}: " + f"{search_resp.status} {await search_resp.text()}" + ) + search_data = await search_resp.json() + assert search_data.get("success"), ( + f"Astrolabe search returned error on attempt {attempts}: {search_data}" + ) + last_results = search_data.get("results") or [] + note_result = next( + ( + r + for r in last_results + if r.get("doc_type") == "note" and str(r.get("id")) == str(note_id) + ), + None, + ) + if note_result is not None: + logger.info( + f"Note {note_id} surfaced in Astrolabe search after {attempts} " + f"attempts (~{attempts * 2}s)" + ) + return note_result + await anyio.sleep(2) + + raise AssertionError( + f"Note {note_id} with unique term '{unique_term}' did not surface in " + f"Astrolabe search within {timeout_seconds}s ({attempts} attempts). " + f"Last {len(last_results or [])} results: {last_results}" + ) + + @pytest.mark.timeout(300) async def test_chunk_context_endpoint_uses_app_password( browser, @@ -130,28 +189,12 @@ async def test_chunk_context_endpoint_uses_app_password( ) csrf_headers = {"requesttoken": request_token} - search_resp = await page.request.get( - f"http://localhost:8080/apps/astrolabe/api/search" - f"?query={unique_term}&algorithm=hybrid&limit=5&include_pca=false", - headers=csrf_headers, - ) - assert search_resp.ok, ( - f"Astrolabe search failed: {search_resp.status} {await search_resp.text()}" - ) - search_data = await search_resp.json() - assert search_data.get("success"), f"Search returned error: {search_data}" - results = search_data.get("results") or [] - note_result = next( - ( - r - for r in results - if r.get("doc_type") == "note" and str(r.get("id")) == str(note_id) - ), - None, - ) - assert note_result is not None, ( - f"Note {note_id} with term '{unique_term}' not found in search results: " - f"{results}" + note_result = await _poll_astrolabe_search_for_note( + page=page, + unique_term=unique_term, + note_id=note_id, + csrf_headers=csrf_headers, + timeout_seconds=60, ) start = note_result.get("chunk_start_offset") end = note_result.get("chunk_end_offset")