From b1fac2d7a89a5065467ea718126e281475d62dba Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 29 May 2026 02:15:44 +0200 Subject: [PATCH] fix(search): address PR #813 review (viz verify-on-read, owners cache, docs) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - viz_routes: run verify_search_results before returning results. After the accessible_owners expansion the viz can surface OTHER users' shared docs, so it must drop ones the caller can no longer access (revoked share) — same as the nc_semantic_search tool path. (Blocking review item.) - access_filter: cache list_accessible_owners per user for 30s to keep the OCS shares round-trip off the search hot path (failures aren't cached); document the single-page OCS limitation; add a clear_accessible_owners_cache() test helper. Comment the empty-accessible_owners MatchAny([]) edge case. - verification: comment why cross-user eviction is a deliberate no-op (eviction is scoped to the querying user's id, so a recipient's revoked access never deletes the owner's points; the recipient self-heals via accessible_owners). - algorithms: declare SearchResult.original_score (set by the viz route) so the now-precisely-typed result list type-checks. - tests: cross-user eviction-no-op safety test; autouse owners-cache reset in the access_filter + shared-search tests; replace async-no-await qdrant fakes with AsyncMock (clears SonarCloud S7503). Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/auth/viz_routes.py | 17 +++++++-- nextcloud_mcp_server/search/access_filter.py | 40 ++++++++++++++++++-- nextcloud_mcp_server/search/algorithms.py | 3 ++ nextcloud_mcp_server/search/verification.py | 9 +++++ tests/integration/test_acl_owner_filter.py | 7 ++-- tests/integration/test_acl_shared_search.py | 21 +++++++--- tests/unit/search/test_access_filter.py | 10 +++++ tests/unit/search/test_verification.py | 31 +++++++++++++++ 8 files changed, 122 insertions(+), 16 deletions(-) diff --git a/nextcloud_mcp_server/auth/viz_routes.py b/nextcloud_mcp_server/auth/viz_routes.py index d3cc92d8..84110ab2 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -38,6 +38,7 @@ from nextcloud_mcp_server.search.context import ( get_chunk_bbox_and_page_from_qdrant, get_chunk_with_context, ) +from nextcloud_mcp_server.search.verification import verify_search_results from nextcloud_mcp_server.utils.validation import is_valid_nextcloud_doc_id from nextcloud_mcp_server.vector.oauth_sync import ( NotProvisionedError, @@ -226,10 +227,18 @@ async def vector_visualization_search(request: Request) -> JSONResponse: # Sort by score before verification all_results.sort(key=lambda r: r.score, reverse=True) - # No verification needed for visualization - we only need Qdrant metadata - # (title, excerpt, doc_type) which is already in search results. - # Verification is only needed for sampling (LLM needs full content). - search_results = all_results[:limit] + # Verify-on-read (ADR-019). Now that accessible_owners is expanded + # via OCS shares, the result set can include OTHER users' shared + # documents — so we must drop any the caller can no longer access + # (e.g. a revoked share whose index entry hasn't reconciled yet), + # exactly as the nc_semantic_search tool path does. Skipping this + # would let the viz surface stale titles/excerpts from another + # user's index after a share is revoked. + with trace_operation("vector_viz.verify_on_read"): + verified_results, _dropped = await verify_search_results( + nc_client, all_results + ) + search_results = verified_results[:limit] search_duration = time.perf_counter() - search_start # Store original scores and normalize for visualization diff --git a/nextcloud_mcp_server/search/access_filter.py b/nextcloud_mcp_server/search/access_filter.py index 71778b11..08b375a1 100644 --- a/nextcloud_mcp_server/search/access_filter.py +++ b/nextcloud_mcp_server/search/access_filter.py @@ -17,12 +17,26 @@ by their original indexer. New points carry both fields. from __future__ import annotations import logging +import time from typing import Any, Protocol from qdrant_client.models import FieldCondition, Filter, MatchAny, MatchValue logger = logging.getLogger(__name__) +# Short-lived per-user cache for the OCS shares lookup, which otherwise runs on +# every search/viz request. Trades up to this many seconds of share-visibility +# staleness (a freshly-granted share is searchable a little late) for avoiding +# 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]]] = {} + + +def clear_accessible_owners_cache() -> None: + """Drop all cached accessible-owners entries (used by tests).""" + _owners_cache.clear() + class _SharingClientProtocol(Protocol): """Subset of SharingClient that this module actually uses.""" @@ -42,9 +56,22 @@ async def list_accessible_owners( Duplicates are removed; ordering is not significant (Qdrant ``MatchAny`` treats the list as a set). + Results are cached per user for ``_OWNERS_CACHE_TTL_SECONDS`` to keep the + OCS round-trip off the search hot path. Failures are not cached. + + Note: ``list_shares(shared_with_me=True)`` returns whatever the OCS endpoint + yields in a single page (SharingClient does not paginate today). A user with + more incoming shares than the OCS page size could have some owners omitted; + if that becomes real, add pagination to SharingClient. + 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: + return list(cached[1]) # copy so callers can't mutate the cached value + owners: set[str] = {user_id} try: shares = await sharing_client.list_shares(shared_with_me=True) @@ -55,7 +82,7 @@ async def list_accessible_owners( user_id, exc, ) - return [user_id] + return [user_id] # don't cache failures — retry on the next search for share in shares: # OCS returns the share owner under `uid_owner` (the file owner, @@ -65,8 +92,10 @@ async def list_accessible_owners( if isinstance(owner, str) and owner: owners.add(owner) - logger.debug("Accessible owners for user %s: %d entries", user_id, len(owners)) - return list(owners) + result = list(owners) + _owners_cache[user_id] = (now, result) + logger.debug("Accessible owners for user %s: %d entries", user_id, len(result)) + return list(result) def build_ownership_filter( @@ -90,6 +119,11 @@ def build_ownership_filter( A Qdrant ``Filter`` ready to be nested under a parent ``must`` clause. """ owners = accessible_owners if accessible_owners is not None else [user_id] + # Edge case: an explicit empty ``accessible_owners`` yields + # ``MatchAny(any=[])``, which Qdrant treats as matching nothing — so the + # owner_id branch contributes no results and access comes solely from the + # legacy ``user_id`` branch below (self-owned content). Callers that want + # share expansion must pass a non-empty list. return Filter( should=[ FieldCondition(key="owner_id", match=MatchAny(any=owners)), diff --git a/nextcloud_mcp_server/search/algorithms.py b/nextcloud_mcp_server/search/algorithms.py index ed1b1ddc..a0015e33 100644 --- a/nextcloud_mcp_server/search/algorithms.py +++ b/nextcloud_mcp_server/search/algorithms.py @@ -168,6 +168,9 @@ class SearchResult: chunk_index: int = 0 total_chunks: int = 1 point_id: str | None = None + # Pre-normalization score, set by the visualization route before it rescales + # ``score`` to [0, 1] for visual encoding (see auth/viz_routes.py). + original_score: float | None = None def __post_init__(self): """Validate score is non-negative. diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index ee4588df..a7e64377 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -586,6 +586,15 @@ async def verify_search_results( if evict_on_missing and inaccessible: async def evict(doc_id: str, doc_type: str) -> None: + # Eviction is scoped to the QUERYING user's own points + # (user_id == the searcher). For a cross-user shared document + # (owner_id=alice surfaced to bob via accessible_owners), bob + # failing verification evicts with user_id=bob — a deliberate + # no-op, because alice's points carry user_id=alice and must NOT + # be deleted just because bob's share was revoked. Bob's view + # self-heals via list_accessible_owners (alice drops out of his + # accessible owners once OCS no longer reports the share). See the + # legacy-user_id semantics note in build_ownership_filter. try: await delete_document_points(doc_id, doc_type, user_id) except Exception as e: diff --git a/tests/integration/test_acl_owner_filter.py b/tests/integration/test_acl_owner_filter.py index 068362c3..c47e6b88 100644 --- a/tests/integration/test_acl_owner_filter.py +++ b/tests/integration/test_acl_owner_filter.py @@ -15,6 +15,8 @@ real-Nextcloud flow (share + verify-on-read) lives in ``test_acl_shared_search.py``. """ +from unittest.mock import AsyncMock + import pytest from qdrant_client import AsyncQdrantClient from qdrant_client.models import Distance, PointStruct, VectorParams @@ -82,12 +84,9 @@ async def seeded_collection(monkeypatch): await client.upsert(collection_name=collection, points=points, wait=True) # Point the algorithm at the in-memory client + deterministic embeddings. - async def _fake_get_qdrant_client(): - return client - monkeypatch.setattr( "nextcloud_mcp_server.search.semantic.get_qdrant_client", - _fake_get_qdrant_client, + AsyncMock(return_value=client), ) monkeypatch.setattr( "nextcloud_mcp_server.search.semantic.get_embedding_service", diff --git a/tests/integration/test_acl_shared_search.py b/tests/integration/test_acl_shared_search.py index 4302f2a3..47148084 100644 --- a/tests/integration/test_acl_shared_search.py +++ b/tests/integration/test_acl_shared_search.py @@ -23,6 +23,7 @@ proves the real share → accessible_owners → filter → verify chain. import os import uuid +from unittest.mock import AsyncMock import pytest from httpx import BasicAuth @@ -32,12 +33,25 @@ from qdrant_client.models import Distance, PointStruct, VectorParams from nextcloud_mcp_server.client import NextcloudClient from nextcloud_mcp_server.config import get_settings from nextcloud_mcp_server.embedding import SimpleEmbeddingProvider -from nextcloud_mcp_server.search.access_filter import list_accessible_owners +from nextcloud_mcp_server.search.access_filter import ( + clear_accessible_owners_cache, + list_accessible_owners, +) from nextcloud_mcp_server.search.semantic import SemanticSearchAlgorithm from nextcloud_mcp_server.search.verification import verify_search_results pytestmark = pytest.mark.integration + +@pytest.fixture(autouse=True) +def _reset_owners_cache(): + """Reset the process-global accessible-owners cache around each test so a + real OCS share created in a fixture isn't masked by a stale cached entry.""" + clear_accessible_owners_cache() + yield + clear_accessible_owners_cache() + + _DOC_TEXT = "Confidential quarterly infrastructure budget and capacity plan" @@ -127,12 +141,9 @@ async def seeded_semantic(monkeypatch, shared_file): wait=True, ) - async def _fake_get_qdrant_client(): - return client - monkeypatch.setattr( "nextcloud_mcp_server.search.semantic.get_qdrant_client", - _fake_get_qdrant_client, + AsyncMock(return_value=client), ) monkeypatch.setattr( "nextcloud_mcp_server.search.semantic.get_embedding_service", diff --git a/tests/unit/search/test_access_filter.py b/tests/unit/search/test_access_filter.py index f3ee3bb0..9c7bdf89 100644 --- a/tests/unit/search/test_access_filter.py +++ b/tests/unit/search/test_access_filter.py @@ -8,10 +8,20 @@ import pytest from nextcloud_mcp_server.search.access_filter import ( build_ownership_filter, + clear_accessible_owners_cache, list_accessible_owners, ) +@pytest.fixture(autouse=True) +def _reset_owners_cache(): + """The accessible-owners cache is process-global; reset it around each test + so the shared "alice" user_id can't leak cached results between tests.""" + clear_accessible_owners_cache() + yield + clear_accessible_owners_cache() + + class TestListAccessibleOwners: @pytest.mark.unit async def test_includes_self_even_with_no_shares(self) -> None: diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py index 8bd3c102..ca480c3e 100644 --- a/tests/unit/search/test_verification.py +++ b/tests/unit/search/test_verification.py @@ -874,6 +874,37 @@ async def test_verify_search_results_drops_inaccessible_and_evicts(mocker): spy_evict.assert_awaited_once_with("99", "note", "alice") +@pytest.mark.unit +async def test_verify_evicts_cross_user_file_under_querying_user_id(mocker): + """A shared file the recipient can no longer access is evicted under the + QUERYING user's id, never the owner's. + + This guards the cross-user eviction no-op: a point owned by alice + (user_id=alice) surfaced to bob via accessible_owners and then found + inaccessible must be evicted with user_id=bob — which deletes nothing of + alice's (her points carry user_id=alice). So a recipient's revoked access + can never delete the owner's index entries; bob's view self-heals via + list_accessible_owners instead. A future change that evicted under the + owner's id would corrupt the owner's index, and this test would catch it. + """ + spy_evict = mocker.AsyncMock() + mocker.patch.object(verification, "delete_document_points", spy_evict) + + webdav_client = SimpleNamespace( + file_accessible_by_id=mocker.AsyncMock(return_value=False) + ) + client = SimpleNamespace(webdav=webdav_client, username="bob") + + kept, dropped_count = await verify_search_results( + client, + [_make_result(777, doc_type="file", metadata={"path": "shared.txt"})], + ) + + assert kept == [] + assert dropped_count == 1 + spy_evict.assert_awaited_once_with("777", "file", "bob") + + @pytest.mark.unit async def test_verify_search_results_fire_and_forget_eviction(mocker): """When eviction_task_group is provided, eviction does not block the response.