From cafbfd15a94f6c945211aaaf8b402ef7923b811c Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 29 May 2026 13:28:45 +0200 Subject: [PATCH] fix: address PR #813 review (owner_id index, cache bound, explicit param) - vector/qdrant_client.py: add owner_id to _PAYLOAD_INDEX_FIELDS (BLOCKING). Every search applies MatchAny(key="owner_id", ...); without a keyword index Qdrant full-scans the collection and may 400 on Qdrant Cloud strict mode. _ensure_payload_indexes is idempotent so existing collections migrate at startup. - search/access_filter.py: bound the process-global _owners_cache with an LRU cap (was one unbounded entry per active user, never evicted); document the owner-level over-fetch limitation (a prolific sharer floods the recall buffer with ghost candidates that verify-on-read drops, with no second Qdrant pass) as a TODO toward per-file filtering. - search/algorithms.py + semantic.py + bm25_hybrid.py: promote accessible_owners from **kwargs to an explicit keyword-only parameter on the SearchAlgorithm ABC and both implementations, so a misspelled keyword is a type error rather than a silent fall back to self-only scope. - search/verification.py: document that _verify_files now verifies by global file id (WebDAV SEARCH), not by path. - tests/unit/search/test_access_filter.py: add cache-hit, TTL-expiry, failure-not-cached, and LRU-bound tests. Bumps the astrolabe submodule with the matching #89 review fixes. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/search/access_filter.py | 24 +++++++- nextcloud_mcp_server/search/algorithms.py | 8 +++ nextcloud_mcp_server/search/bm25_hybrid.py | 6 +- nextcloud_mcp_server/search/semantic.py | 9 +-- nextcloud_mcp_server/search/verification.py | 10 ++++ nextcloud_mcp_server/vector/qdrant_client.py | 9 +++ tests/unit/search/test_access_filter.py | 58 ++++++++++++++++++++ third_party/astrolabe | 2 +- 8 files changed, 119 insertions(+), 7 deletions(-) diff --git a/nextcloud_mcp_server/search/access_filter.py b/nextcloud_mcp_server/search/access_filter.py index 08b375a1..67506872 100644 --- a/nextcloud_mcp_server/search/access_filter.py +++ b/nextcloud_mcp_server/search/access_filter.py @@ -18,6 +18,7 @@ from __future__ import annotations import logging import time +from collections import OrderedDict from typing import Any, Protocol from qdrant_client.models import FieldCondition, Filter, MatchAny, MatchValue @@ -30,7 +31,13 @@ logger = logging.getLogger(__name__) # an OCS round-trip per query. Safe: verify-on-read still gates each result # against Nextcloud, so a revoked share is caught there regardless of this cache. _OWNERS_CACHE_TTL_SECONDS = 30.0 -_owners_cache: dict[str, tuple[float, list[str]]] = {} +# Cap the number of cached users so the process-global cache can't grow +# unboundedly in a long-running multi-user deployment (one entry per active +# user, never evicted otherwise). LRU eviction by insertion/access order via +# OrderedDict; the cap is generous relative to any realistic concurrent-user +# count, so steady state is effectively all-hit. +_OWNERS_CACHE_MAXSIZE = 1024 +_owners_cache: OrderedDict[str, tuple[float, list[str]]] = OrderedDict() def clear_accessible_owners_cache() -> None: @@ -64,12 +71,24 @@ async def list_accessible_owners( more incoming shares than the OCS page size could have some owners omitted; if that becomes real, add pagination to SharingClient. + Granularity / over-fetch limitation (TODO, finer-grained filtering): this + expansion is *owner-level*, not *file-level*. If a prolific content creator + shares a single item with the querying user, that owner's whole indexed + corpus becomes a Qdrant candidate set for the querier even though only the + shared item is accessible. Verify-on-read correctly drops the inaccessible + "ghost" candidates, but because there is no second Qdrant pass to replenish, + a ``limit=N`` search can return fewer than N results when the over-fetch + buffer (2× in nc_semantic_search / viz_routes) is dominated by ghosts. A + per-file ownership index would remove this tension and is the natural + starting point for future work (intentionally out of scope here). + Sharing API failures are non-fatal — we degrade to ``[user_id]`` and log so a hiccup in OCS doesn't black-hole the user's own search. """ now = time.monotonic() cached = _owners_cache.get(user_id) if cached is not None and now - cached[0] < _OWNERS_CACHE_TTL_SECONDS: + _owners_cache.move_to_end(user_id) # mark as recently used (LRU) return list(cached[1]) # copy so callers can't mutate the cached value owners: set[str] = {user_id} @@ -94,6 +113,9 @@ async def list_accessible_owners( result = list(owners) _owners_cache[user_id] = (now, result) + _owners_cache.move_to_end(user_id) # newest = most-recently-used + while len(_owners_cache) > _OWNERS_CACHE_MAXSIZE: + _owners_cache.popitem(last=False) # evict least-recently-used logger.debug("Accessible owners for user %s: %d entries", user_id, len(result)) return list(result) diff --git a/nextcloud_mcp_server/search/algorithms.py b/nextcloud_mcp_server/search/algorithms.py index a0015e33..2057a679 100644 --- a/nextcloud_mcp_server/search/algorithms.py +++ b/nextcloud_mcp_server/search/algorithms.py @@ -274,6 +274,8 @@ class SearchAlgorithm(ABC): user_id: str, limit: int = 10, doc_type: str | None = None, + *, + accessible_owners: list[str] | None = None, **kwargs: Any, ) -> list[SearchResult]: """Execute search with the given parameters. @@ -283,6 +285,12 @@ class SearchAlgorithm(ABC): user_id: User ID for multi-tenant filtering limit: Maximum number of results to return doc_type: Optional document type filter (note, file, calendar, etc.) + accessible_owners: Owner UIDs the user is allowed to read (self plus + the owners of content shared with them), pre-computed from the + OCS Sharing API by the caller. Declared explicitly — rather than + buried in ``**kwargs`` — so a misspelled keyword is a type error + instead of a silent fall back to self-only scope. ``None`` means + self-only (``[user_id]``). **kwargs: Algorithm-specific parameters Returns: diff --git a/nextcloud_mcp_server/search/bm25_hybrid.py b/nextcloud_mcp_server/search/bm25_hybrid.py index 30103017..78e3ef86 100644 --- a/nextcloud_mcp_server/search/bm25_hybrid.py +++ b/nextcloud_mcp_server/search/bm25_hybrid.py @@ -71,6 +71,8 @@ class BM25HybridSearchAlgorithm(SearchAlgorithm): user_id: str, limit: int = 10, doc_type: str | None = None, + *, + accessible_owners: list[str] | None = None, **kwargs: Any, ) -> list[SearchResult]: """ @@ -89,6 +91,9 @@ class BM25HybridSearchAlgorithm(SearchAlgorithm): user_id: User ID for filtering limit: Maximum results to return doc_type: Optional document type filter + accessible_owners: Owner UIDs the user can read (self + share + senders), pre-computed by the caller from the OCS Sharing API. + Defaults to ``[user_id]`` (self-only) when ``None``. **kwargs: Additional parameters (score_threshold override) Returns: @@ -99,7 +104,6 @@ class BM25HybridSearchAlgorithm(SearchAlgorithm): """ settings = get_settings() score_threshold = kwargs.get("score_threshold", self.score_threshold) - accessible_owners: list[str] | None = kwargs.get("accessible_owners") logger.info( "BM25 hybrid search: query='%s', user=%s, limit=%s, score_threshold=%s, doc_type=%s, fusion=%s", diff --git a/nextcloud_mcp_server/search/semantic.py b/nextcloud_mcp_server/search/semantic.py index fbd5d199..c5bfca9c 100644 --- a/nextcloud_mcp_server/search/semantic.py +++ b/nextcloud_mcp_server/search/semantic.py @@ -49,6 +49,8 @@ class SemanticSearchAlgorithm(SearchAlgorithm): user_id: str, limit: int = 10, doc_type: str | None = None, + *, + accessible_owners: list[str] | None = None, **kwargs: Any, ) -> list[SearchResult]: """Execute semantic search using vector similarity. @@ -66,11 +68,11 @@ class SemanticSearchAlgorithm(SearchAlgorithm): user_id: User ID for filtering limit: Maximum results to return doc_type: Optional document type filter + accessible_owners: Owner UIDs the user can read (self + share + senders), pre-computed by the caller from the OCS Sharing API. + Defaults to ``[user_id]`` (self-only) when ``None``. **kwargs: - score_threshold (float): override the instance default - - accessible_owners (list[str]): owner UIDs the user can read - (self + share senders). Pre-computed by the caller from the - OCS Sharing API. Defaults to ``[user_id]`` when omitted. Returns: List of unverified SearchResult objects ranked by similarity score @@ -80,7 +82,6 @@ class SemanticSearchAlgorithm(SearchAlgorithm): """ settings = get_settings() score_threshold = kwargs.get("score_threshold", self.score_threshold) - accessible_owners: list[str] | None = kwargs.get("accessible_owners") logger.info( "Semantic search: query='%s', user=%s, limit=%s, score_threshold=%s, doc_type=%s", diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index a7e64377..ee10d7d9 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -132,6 +132,16 @@ async def _verify_files( results: list[SearchResult], semaphore: anyio.Semaphore, ) -> set[str]: + """Return the doc_ids of file results this user may actually access. + + Verifies each file by its *global* Nextcloud file id via an ACL-aware + WebDAV SEARCH (``webdav.file_accessible_by_id``), NOT by path. This is the + ACL-aware-search fix: a file an owner shared with the querying user mounts + at a different path under each tree, so the previous path-based check + (``get_file_info``) produced false 404s and dropped legitimate shared-file + hits. Definitive 403/404 → inaccessible (dropped + scheduled for eviction + by the caller); transient/ambiguous errors → kept (fail-open). + """ # safe: cooperative concurrency, no lock needed (see verify_search_results) accessible: set[str] = set() diff --git a/nextcloud_mcp_server/vector/qdrant_client.py b/nextcloud_mcp_server/vector/qdrant_client.py index 9f54d960..9f110e43 100644 --- a/nextcloud_mcp_server/vector/qdrant_client.py +++ b/nextcloud_mcp_server/vector/qdrant_client.py @@ -38,6 +38,15 @@ logger = logging.getLogger(__name__) _PAYLOAD_INDEX_FIELDS: dict[str, PayloadSchemaType] = { "doc_id": PayloadSchemaType.KEYWORD, "user_id": PayloadSchemaType.KEYWORD, + # owner_id is the ACL-aware filter field: every search applies + # MatchAny(key="owner_id", any=accessible_owners) (see + # search/access_filter.py). Without a keyword index Qdrant full-scans the + # collection to evaluate it — invisible at small scale, but a latency + # regression at tens of thousands of points and an HTTP 400 on Qdrant + # Cloud strict payload-validation mode. Mirrors the user_id treatment; + # _ensure_payload_indexes is idempotent so existing collections migrate + # at startup without operator intervention. + "owner_id": PayloadSchemaType.KEYWORD, "doc_type": PayloadSchemaType.KEYWORD, "is_placeholder": PayloadSchemaType.BOOL, "chunk_index": PayloadSchemaType.INTEGER, diff --git a/tests/unit/search/test_access_filter.py b/tests/unit/search/test_access_filter.py index 9c7bdf89..4965018f 100644 --- a/tests/unit/search/test_access_filter.py +++ b/tests/unit/search/test_access_filter.py @@ -6,6 +6,7 @@ from unittest.mock import AsyncMock import pytest +from nextcloud_mcp_server.search import access_filter from nextcloud_mcp_server.search.access_filter import ( build_ownership_filter, clear_accessible_owners_cache, @@ -94,6 +95,63 @@ class TestListAccessibleOwners: sharing.list_shares.assert_awaited_once_with(shared_with_me=True) +class TestOwnersCacheBehavior: + @pytest.mark.unit + async def test_second_call_within_ttl_uses_cache(self) -> None: + sharing = AsyncMock() + sharing.list_shares.return_value = [{"uid_owner": "bob"}] + + first = await list_accessible_owners(sharing, "alice") + second = await list_accessible_owners(sharing, "alice") + + assert sorted(first) == ["alice", "bob"] + assert second == first + # Only one OCS round-trip — the second call was served from cache. + sharing.list_shares.assert_awaited_once() + + @pytest.mark.unit + async def test_expired_entry_triggers_fresh_ocs_call(self) -> None: + sharing = AsyncMock() + sharing.list_shares.return_value = [{"uid_owner": "bob"}] + + await list_accessible_owners(sharing, "alice") + # Age the cached entry past the TTL without sleeping/patching the clock. + ts, value = access_filter._owners_cache["alice"] + access_filter._owners_cache["alice"] = ( + ts - access_filter._OWNERS_CACHE_TTL_SECONDS - 1.0, + value, + ) + await list_accessible_owners(sharing, "alice") + + assert sharing.list_shares.await_count == 2 + + @pytest.mark.unit + async def test_failure_is_not_cached(self) -> None: + sharing = AsyncMock() + sharing.list_shares.side_effect = RuntimeError("OCS down") + + await list_accessible_owners(sharing, "alice") # degrades to self-only + # A later success must not be masked by a cached failure. + sharing.list_shares.side_effect = None + sharing.list_shares.return_value = [{"uid_owner": "bob"}] + + owners = await list_accessible_owners(sharing, "alice") + assert sorted(owners) == ["alice", "bob"] + + @pytest.mark.unit + async def test_cache_is_bounded_lru(self, monkeypatch) -> None: + monkeypatch.setattr(access_filter, "_OWNERS_CACHE_MAXSIZE", 2) + sharing = AsyncMock() + sharing.list_shares.return_value = [] + + await list_accessible_owners(sharing, "u1") + await list_accessible_owners(sharing, "u2") + await list_accessible_owners(sharing, "u3") # evicts u1 (least recent) + + assert set(access_filter._owners_cache.keys()) == {"u2", "u3"} + assert len(access_filter._owners_cache) == 2 + + class TestBuildOwnershipFilter: def test_defaults_to_self_only_when_owners_omitted(self) -> None: flt = build_ownership_filter("alice") diff --git a/third_party/astrolabe b/third_party/astrolabe index 86616d92..70684e0b 160000 --- a/third_party/astrolabe +++ b/third_party/astrolabe @@ -1 +1 @@ -Subproject commit 86616d921e811eafeb85b72a6380c6b73bc3168e +Subproject commit 70684e0b516bd36d31716f3c40cfbc68382b5c12