fix(search): address PR #813 review (viz verify-on-read, owners cache, docs)
- 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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
6206f4a634
commit
b1fac2d7a8
@@ -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
|
||||
|
||||
@@ -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)),
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user