diff --git a/nextcloud_mcp_server/client/__init__.py b/nextcloud_mcp_server/client/__init__.py index 178327ea..6891e47d 100644 --- a/nextcloud_mcp_server/client/__init__.py +++ b/nextcloud_mcp_server/client/__init__.py @@ -199,6 +199,46 @@ class NextcloudClient: return response.json() + async def get_enabled_apps(self) -> set[str]: + """Return the set of app ids enabled for the authenticated user. + + Uses the per-user core navigation endpoint, which lists only apps the + current user can access (respecting group restrictions). The vector + scanner uses this to skip polling apps the user lacks, which would 404 + and flood tenant logs. Preferred over ``/cloud/capabilities`` because + the News app advertises no capability and so never appears there. + """ + response = await self._client.get( + "/ocs/v2.php/core/navigation/apps", + headers={"OCS-APIRequest": "true", "Accept": "application/json"}, + ) + response.raise_for_status() + data = response.json() + # ``X or {}``/``or []`` (not ``.get(k, default)``) so a present-but-null + # ``ocs``/``data`` (``{"ocs": null}``) coerces to empty instead of + # raising AttributeError on ``None.get``. + ocs = data.get("ocs") or {} + # A 200 carrying ``meta.status != "ok"`` is an OCS-level failure (auth / + # permission edge cases) that ``raise_for_status`` can't see. Raise so + # the scanner's ``_get_enabled_apps_or_none`` catches it and falls back + # to scanning every app, rather than silently gating all apps off for a + # cycle on an empty ``data``. Tolerate a missing/empty meta (our own + # mocks, and any envelope that omits it). + status = (ocs.get("meta") or {}).get("status") + if status and status != "ok": # falsy (missing/None/"") tolerated + raise ValueError(f"OCS navigation returned status={status!r}") + entries = ocs.get("data") or [] + enabled: set[str] = set() + for entry in entries: + # ``app`` is the canonical app id; ``id`` matches it for the apps we + # gate. Union both so an unexpected nav-entry shape never hides an + # enabled app — a false "disabled" would skip real indexing. + for key in ("app", "id"): + value = entry.get(key) + if value: + enabled.add(value) + return enabled + async def notes_search_notes(self, *, query: str): """Search notes using token-based matching with relevance ranking.""" all_notes = self.notes.get_all_notes() diff --git a/nextcloud_mcp_server/vector/scanner.py b/nextcloud_mcp_server/vector/scanner.py index 2fbe2109..b1085aa0 100644 --- a/nextcloud_mcp_server/vector/scanner.py +++ b/nextcloud_mcp_server/vector/scanner.py @@ -247,6 +247,38 @@ async def scanner_task( logger.info("Scanner task stopped - stream closed") +async def _get_enabled_apps_or_none( + nc_client: NextcloudClient, user_id: str, scan_id: int +) -> set[str] | None: + """Enabled-app id set for gating, or ``None`` when detection fails. + + ``None`` signals "couldn't determine" — callers must then scan every app + (the prior behaviour), so a transient navigation-endpoint failure never + silently halts indexing. The per-app 404 guards in ``scan_user_documents`` + remain the safety net for that fallback path. + """ + try: + return await nc_client.get_enabled_apps() + except Exception as e: + logger.warning( + "[SCAN-%s] Could not determine enabled apps for %s (%s); scanning all apps", + scan_id, + user_id, + e, + ) + return None + + +def _app_enabled(app_id: str, enabled_apps: set[str] | None) -> bool: + """Whether ``app_id`` should be scanned for the current user. + + ``enabled_apps is None`` means detection failed — every app is treated as + enabled (the scan-all fallback) so a transient navigation-endpoint failure + never silently halts indexing. + """ + return enabled_apps is None or app_id in enabled_apps + + async def scan_user_documents( user_id: str, send_stream: TaskProducer, @@ -328,6 +360,11 @@ async def scan_user_documents( logger.debug("Found %s indexed documents in Qdrant", len(indexed_doc_ids)) + # Determine which apps are enabled for this user so we skip polling + # apps they lack — those polls 404 and flood tenant logs. ``None`` means + # detection failed: fall back to scanning every app (prior behaviour). + enabled_apps = await _get_enabled_apps_or_none(nc_client, user_id, scan_id) + # Notes (isolated so an uninstalled or disabled Notes app — whose API # returns 404 — cannot abort scanning of the other apps; this mirrors the # per-app try/except guards already wrapping files/news/deck below). @@ -336,29 +373,36 @@ async def scan_user_documents( current_time = time.time() queued = 0 - try: - queued += await scan_notes( - user_id=user_id, - send_stream=send_stream, - nc_client=nc_client, - initial_sync=initial_sync, - scan_id=scan_id, - prune_before=prune_before, - indexed_doc_ids=indexed_doc_ids, - grace_period=grace_period, - current_time=current_time, - ) - except HTTPStatusError as e: - if e.response.status_code == 404: - logger.info( - "[SCAN-%s] Notes app unavailable for %s (HTTP 404); skipping notes", - scan_id, - user_id, + if _app_enabled("notes", enabled_apps): + try: + queued += await scan_notes( + user_id=user_id, + send_stream=send_stream, + nc_client=nc_client, + initial_sync=initial_sync, + scan_id=scan_id, + prune_before=prune_before, + indexed_doc_ids=indexed_doc_ids, + grace_period=grace_period, + current_time=current_time, ) - else: + except HTTPStatusError as e: + if e.response.status_code == 404: + logger.info( + "[SCAN-%s] Notes app unavailable for %s (HTTP 404); skipping notes", + scan_id, + user_id, + ) + else: + logger.warning("Failed to scan notes for %s: %s", user_id, e) + except Exception as e: logger.warning("Failed to scan notes for %s: %s", user_id, e) - except Exception as e: - logger.warning("Failed to scan notes for %s: %s", user_id, e) + else: + logger.debug( + "[SCAN-%s] Notes app not enabled for %s; skipping notes", + scan_id, + user_id, + ) if initial_sync: logger.info("Sent %s documents for initial sync: %s", queued, user_id) @@ -666,31 +710,45 @@ async def scan_user_documents( # Scan News items (starred + unread) news_queued = 0 - try: - news_queued = await scan_news_items( - user_id=user_id, - send_stream=send_stream, - nc_client=nc_client, - initial_sync=initial_sync, - scan_id=scan_id, + if _app_enabled("news", enabled_apps): + try: + news_queued = await scan_news_items( + user_id=user_id, + send_stream=send_stream, + nc_client=nc_client, + initial_sync=initial_sync, + scan_id=scan_id, + ) + queued += news_queued + except Exception as e: + logger.warning("Failed to scan news items for %s: %s", user_id, e) + else: + logger.debug( + "[SCAN-%s] News app not enabled for %s; skipping news items", + scan_id, + user_id, ) - queued += news_queued - except Exception as e: - logger.warning("Failed to scan news items for %s: %s", user_id, e) # Scan Deck cards deck_queued = 0 - try: - deck_queued = await scan_deck_cards( - user_id=user_id, - send_stream=send_stream, - nc_client=nc_client, - initial_sync=initial_sync, - scan_id=scan_id, + if _app_enabled("deck", enabled_apps): + try: + deck_queued = await scan_deck_cards( + user_id=user_id, + send_stream=send_stream, + nc_client=nc_client, + initial_sync=initial_sync, + scan_id=scan_id, + ) + queued += deck_queued + except Exception as e: + logger.warning("Failed to scan deck cards for %s: %s", user_id, e) + else: + logger.debug( + "[SCAN-%s] Deck app not enabled for %s; skipping deck cards", + scan_id, + user_id, ) - queued += deck_queued - except Exception as e: - logger.warning("Failed to scan deck cards for %s: %s", user_id, e) if queued > 0: logger.info( diff --git a/tests/unit/client/test_nextcloud_client.py b/tests/unit/client/test_nextcloud_client.py index 46f6846b..e437a43b 100644 --- a/tests/unit/client/test_nextcloud_client.py +++ b/tests/unit/client/test_nextcloud_client.py @@ -7,7 +7,7 @@ folders) into a flat list of files. """ from typing import Any -from unittest.mock import AsyncMock +from unittest.mock import AsyncMock, MagicMock import pytest @@ -31,6 +31,103 @@ def _make_client() -> Any: pytestmark = pytest.mark.unit +def _navigation_response(entries: list[dict]) -> MagicMock: + """Build a mocked OCS v2 ``core/navigation/apps`` response.""" + response = MagicMock() + response.status_code = 200 + response.raise_for_status = MagicMock() + response.json.return_value = {"ocs": {"meta": {}, "data": entries}} + return response + + +class TestGetEnabledApps: + async def test_returns_app_ids_from_navigation(self): + client = _make_client() + client._client = AsyncMock() + client._client.get = AsyncMock( + return_value=_navigation_response( + [ + {"id": "files", "app": "files"}, + {"id": "notes", "app": "notes"}, + {"id": "deck", "app": "deck"}, + {"id": "news", "app": "news"}, + ] + ) + ) + + apps = await client.get_enabled_apps() + + assert apps == {"files", "notes", "deck", "news"} + # Hits the per-user navigation endpoint, not capabilities. + assert ( + client._client.get.await_args.args[0] == "/ocs/v2.php/core/navigation/apps" + ) + + async def test_unions_id_and_app_keys(self): + """When ``id`` and ``app`` differ, both are collected so an enabled + app is never hidden by an unexpected nav-entry id.""" + client = _make_client() + client._client = AsyncMock() + client._client.get = AsyncMock( + return_value=_navigation_response([{"id": "files_sharing", "app": "files"}]) + ) + + apps = await client.get_enabled_apps() + + assert apps == {"files", "files_sharing"} + + async def test_empty_navigation_returns_empty_set(self): + client = _make_client() + client._client = AsyncMock() + client._client.get = AsyncMock(return_value=_navigation_response([])) + + assert await client.get_enabled_apps() == set() + + async def test_skips_entries_missing_both_keys(self): + client = _make_client() + client._client = AsyncMock() + client._client.get = AsyncMock( + return_value=_navigation_response( + [{"name": "Logout", "href": "/logout"}, {"app": "notes"}] + ) + ) + + assert await client.get_enabled_apps() == {"notes"} + + @pytest.mark.parametrize("body", [{}, {"ocs": None}, {"ocs": {"data": None}}]) + async def test_malformed_envelope_returns_empty_set(self, body): + """A missing/null ``ocs``/``data`` envelope yields an empty set rather + than raising. NOTE: an empty set does NOT trigger the + ``_get_enabled_apps_or_none`` scan-all fallback (that fires only on + exceptions) — all optional apps are gated off for this scan cycle, with + Files unaffected (unconditional) and the next cycle retrying normally.""" + client = _make_client() + response = MagicMock() + response.raise_for_status = MagicMock() + response.json.return_value = body + client._client = AsyncMock() + client._client.get = AsyncMock(return_value=response) + + assert await client.get_enabled_apps() == set() + + async def test_ocs_failure_status_raises(self): + """A 200 with ``ocs.meta.status == "failure"`` raises so the scanner's + ``_get_enabled_apps_or_none`` falls back to scanning all apps, instead + of silently gating every app off on the empty ``data`` of a failure + envelope.""" + client = _make_client() + response = MagicMock() + response.raise_for_status = MagicMock() + response.json.return_value = { + "ocs": {"meta": {"status": "failure", "statuscode": 997}, "data": None} + } + client._client = AsyncMock() + client._client.get = AsyncMock(return_value=response) + + with pytest.raises(ValueError, match="failure"): + await client.get_enabled_apps() + + class TestNormaliseSearchResult: def test_adds_leading_slash_to_path(self): result = _normalise_search_result( diff --git a/tests/unit/vector/test_scanner_app_gating.py b/tests/unit/vector/test_scanner_app_gating.py new file mode 100644 index 00000000..6d2da6ad --- /dev/null +++ b/tests/unit/vector/test_scanner_app_gating.py @@ -0,0 +1,75 @@ +"""Unit tests for the vector scanner's enabled-app gating helpers. + +``scan_user_documents`` skips polling apps the user doesn't have enabled (those +polls 404 and flood tenant logs). ``_get_enabled_apps_or_none`` resolves the +enabled-app set, returning ``None`` on any failure so the caller falls back to +scanning every app (the prior behaviour) rather than silently halting indexing; +``_app_enabled`` is the gate predicate applied per app. +""" + +import logging +from unittest.mock import AsyncMock + +import pytest +from httpx import HTTPStatusError, Request, Response + +from nextcloud_mcp_server.vector.scanner import ( + _app_enabled, + _get_enabled_apps_or_none, +) + +pytestmark = pytest.mark.unit + + +async def test_returns_enabled_set_on_success(): + nc_client = AsyncMock() + nc_client.get_enabled_apps = AsyncMock(return_value={"files", "notes"}) + + result = await _get_enabled_apps_or_none(nc_client, "alice", scan_id=1234) + + assert result == {"files", "notes"} + + +async def test_returns_none_when_detection_raises(caplog): + nc_client = AsyncMock() + request = Request("GET", "https://nc.test/ocs/v2.php/core/navigation/apps") + nc_client.get_enabled_apps = AsyncMock( + side_effect=HTTPStatusError( + "boom", request=request, response=Response(503, request=request) + ) + ) + + caplog.set_level(logging.WARNING, logger="nextcloud_mcp_server.vector.scanner") + result = await _get_enabled_apps_or_none(nc_client, "alice", scan_id=1234) + + # None signals scan-all fallback; _app_enabled treats `None` as + # "every app enabled" so indexing never silently stops. + assert result is None + assert "scanning all apps" in caplog.text + + +async def test_value_error_from_ocs_failure_returns_none(): + """A ValueError (e.g. OCS meta.status=='failure' from get_enabled_apps) + routes through the scan-all fallback like any other exception.""" + nc_client = AsyncMock() + nc_client.get_enabled_apps = AsyncMock( + side_effect=ValueError("OCS navigation returned status='failure'") + ) + + result = await _get_enabled_apps_or_none(nc_client, "alice", scan_id=1234) + + assert result is None + + +def test_none_set_enables_every_app(): + """A None set means detection failed, so every app must be scanned.""" + assert _app_enabled("news", None) is True + assert _app_enabled("deck", None) is True + + +def test_concrete_set_gates_precisely(): + """A resolved set scans only the apps it contains.""" + enabled = {"notes", "files"} + assert _app_enabled("notes", enabled) is True + assert _app_enabled("news", enabled) is False + assert _app_enabled("deck", enabled) is False