From 37db82613d7ff2e9c5fc70f789901aa4a08c4027 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 27 May 2026 23:48:34 +0200 Subject: [PATCH] feat(search): ACL-aware vector filter via Nextcloud Shares lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The vector index has always been strictly per-user: every Qdrant payload carries a `user_id` and the search filter is `user_id == querying_user`. A file Alice indexed cannot be discovered by Bob even if she has shared it with him — Bob would have to re-index it under his own user_id to make it searchable, which means duplicate index entries for every share recipient. Switch to ownership-with-ACL-expansion: - New `nextcloud_mcp_server.search.access_filter` module: - `list_accessible_owners(sharing_client, user_id)` calls the OCS Sharing API (`shared_with_me=true`) and returns `{user_id} ∪ {uid_owner of each share}`. Fails open to `[user_id]` so a misbehaving Sharing API doesn't black-hole search. - `build_ownership_filter(user_id, accessible_owners)` returns a Qdrant `Filter` whose `should` branch matches either the new `owner_id IN accessible_owners` field or the legacy `user_id` field. The legacy branch keeps points indexed before this change reachable without a migration backfill. - Indexer payload (`vector/processor.py`) now writes `owner_id` alongside `user_id`. `DocumentTask` gains an optional `owner_id` field; today the scanner always runs as the owner so the processor falls back to `user_id`, but the field is plumbed so a future shared-with-me crawler can set the true owner without reshaping the payload contract. - `SemanticSearchAlgorithm.search` and `BM25HybridSearchAlgorithm.search` accept `accessible_owners` via kwargs and use the new ownership filter. Default behaviour with no kwarg is unchanged (self-only). - Both user-facing callers — the MCP tool path (`server/semantic.py`) and the visualization Starlette route (`auth/viz_routes.py`) — compute `accessible_owners` from the authenticated Nextcloud client before invoking the search algorithm. Eviction, scanner deletion, placeholder, and chunk-context paths intentionally keep the legacy `user_id` semantics (those are "operations on a specific user's records", not cross-user reads). - 10 new unit tests in `tests/unit/search/test_access_filter.py` cover self-only default, owner expansion, dedup, fallback fields, OCS failure, and the legacy `should`-branch shape. Pairs with cbcoutinho/astrolabe#89 — together they let an Astrolabe user find content owners have shared with them without going through any re-authorization flow or re-indexing. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/auth/viz_routes.py | 12 +- nextcloud_mcp_server/search/access_filter.py | 98 +++++++++++++++ nextcloud_mcp_server/search/bm25_hybrid.py | 7 +- nextcloud_mcp_server/search/semantic.py | 13 +- nextcloud_mcp_server/server/semantic.py | 9 ++ nextcloud_mcp_server/vector/processor.py | 10 ++ nextcloud_mcp_server/vector/scanner.py | 6 + tests/unit/search/test_access_filter.py | 118 +++++++++++++++++++ 8 files changed, 263 insertions(+), 10 deletions(-) create mode 100644 nextcloud_mcp_server/search/access_filter.py create mode 100644 tests/unit/search/test_access_filter.py diff --git a/nextcloud_mcp_server/auth/viz_routes.py b/nextcloud_mcp_server/auth/viz_routes.py index c1741ebd..d3cc92d8 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -33,6 +33,7 @@ from nextcloud_mcp_server.search import ( BM25HybridSearchAlgorithm, SemanticSearchAlgorithm, ) +from nextcloud_mcp_server.search.access_filter import list_accessible_owners from nextcloud_mcp_server.search.context import ( get_chunk_bbox_and_page_from_qdrant, get_chunk_with_context, @@ -158,7 +159,7 @@ async def vector_visualization_search(request: Request) -> JSONResponse: with trace_operation("vector_viz.get_auth_client"): auth_client_ctx = await _get_authenticated_client_for_userinfo(request) - async with auth_client_ctx as nc_client: # noqa: F841 + async with auth_client_ctx as nc_client: # Create search algorithm (no client needed - verification removed) if algorithm == "semantic": search_algo = SemanticSearchAlgorithm(score_threshold=score_threshold) @@ -172,6 +173,13 @@ async def vector_visualization_search(request: Request) -> JSONResponse: status_code=400, ) + # Expand the caller to every owner whose content they have + # read access to — same logic as the MCP tool path. See + # nextcloud_mcp_server.search.access_filter. + accessible_owners = await list_accessible_owners( + nc_client.sharing, username + ) + # Execute search (supports cross-app when doc_types=None) # Get unverified results with buffer for filtering search_start = time.perf_counter() @@ -192,6 +200,7 @@ async def vector_visualization_search(request: Request) -> JSONResponse: limit=limit * 2, # Buffer for verification filtering doc_type=None, # Search all types score_threshold=score_threshold, + accessible_owners=accessible_owners, ) all_results.extend(unverified_results) else: @@ -211,6 +220,7 @@ async def vector_visualization_search(request: Request) -> JSONResponse: limit=limit * 2, # Buffer for verification filtering doc_type=doc_type, score_threshold=score_threshold, + accessible_owners=accessible_owners, ) all_results.extend(unverified_results) # Sort by score before verification diff --git a/nextcloud_mcp_server/search/access_filter.py b/nextcloud_mcp_server/search/access_filter.py new file mode 100644 index 00000000..71778b11 --- /dev/null +++ b/nextcloud_mcp_server/search/access_filter.py @@ -0,0 +1,98 @@ +"""ACL-aware ownership filter for semantic / BM25 search. + +The vector store payload carries an ``owner_id`` field — the UID of the user +who owns the underlying Nextcloud document. At query time, a user should +be able to find every document whose owner has shared it (directly or via +group / link) with them, without re-indexing. + +This module turns "who can user X read?" into a Qdrant filter: +``owner_id IN accessible_owners`` where ``accessible_owners`` is +``{X} ∪ {owners of files / objects shared with X}``. + +A second OR-branch matches the legacy ``user_id`` field so points indexed +before this change (which carry only ``user_id``) continue to be findable +by their original indexer. New points carry both fields. +""" + +from __future__ import annotations + +import logging +from typing import Any, Protocol + +from qdrant_client.models import FieldCondition, Filter, MatchAny, MatchValue + +logger = logging.getLogger(__name__) + + +class _SharingClientProtocol(Protocol): + """Subset of SharingClient that this module actually uses.""" + + async def list_shares( + self, path: str | None = None, shared_with_me: bool = False + ) -> list[dict[str, Any]]: ... + + +async def list_accessible_owners( + sharing_client: _SharingClientProtocol, + user_id: str, +) -> list[str]: + """Return every owner UID whose content `user_id` should be able to search. + + The set is ``{user_id} ∪ {uid_owner of each share with shared_with_me=True}``. + Duplicates are removed; ordering is not significant (Qdrant ``MatchAny`` + treats the list as a set). + + 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. + """ + owners: set[str] = {user_id} + try: + shares = await sharing_client.list_shares(shared_with_me=True) + except Exception as exc: # noqa: BLE001 — degrade gracefully + logger.warning( + "Sharing API unavailable; falling back to self-only owner filter " + "for user %s (%s)", + user_id, + exc, + ) + return [user_id] + + for share in shares: + # OCS returns the share owner under `uid_owner` (the file owner, + # not the share recipient). Some Nextcloud versions also surface + # `owner` as a fallback display field — we tolerate both. + owner = share.get("uid_owner") or share.get("owner") + 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) + + +def build_ownership_filter( + user_id: str, accessible_owners: list[str] | None = None +) -> Filter: + """Build the Qdrant ``Filter`` constraining a search to readable points. + + Matches points whose ``owner_id`` is in ``accessible_owners`` OR whose + legacy ``user_id`` equals ``user_id``. The legacy branch keeps points + indexed before this change reachable until they're re-indexed. + + Args: + user_id: Querying user (used for the legacy ``user_id`` fallback + and as the only-self default when ``accessible_owners`` is None). + accessible_owners: Pre-computed list of owner UIDs the user has + access to. When None, defaults to ``[user_id]`` (no shares + expansion — used by callers that genuinely want self-only + scope such as eviction sweeps). + + Returns: + A Qdrant ``Filter`` ready to be nested under a parent ``must`` clause. + """ + owners = accessible_owners if accessible_owners is not None else [user_id] + return Filter( + should=[ + FieldCondition(key="owner_id", match=MatchAny(any=owners)), + FieldCondition(key="user_id", match=MatchValue(value=user_id)), + ] + ) diff --git a/nextcloud_mcp_server/search/bm25_hybrid.py b/nextcloud_mcp_server/search/bm25_hybrid.py index 8f60c580..30103017 100644 --- a/nextcloud_mcp_server/search/bm25_hybrid.py +++ b/nextcloud_mcp_server/search/bm25_hybrid.py @@ -10,6 +10,7 @@ from nextcloud_mcp_server.config import get_settings from nextcloud_mcp_server.embedding import get_bm25_service, get_embedding_service from nextcloud_mcp_server.observability.metrics import record_qdrant_operation from nextcloud_mcp_server.observability.tracing import trace_operation +from nextcloud_mcp_server.search.access_filter import build_ownership_filter from nextcloud_mcp_server.search.algorithms import ( SearchAlgorithm, SearchResult, @@ -98,6 +99,7 @@ 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", @@ -131,10 +133,7 @@ class BM25HybridSearchAlgorithm(SearchAlgorithm): # Build Qdrant filter filter_conditions = [ get_placeholder_filter(), # Always exclude placeholders from user-facing queries - FieldCondition( - key="user_id", - match=MatchValue(value=user_id), - ), + build_ownership_filter(user_id, accessible_owners), ] # Add doc_type filter if specified diff --git a/nextcloud_mcp_server/search/semantic.py b/nextcloud_mcp_server/search/semantic.py index 54465c03..fbd5d199 100644 --- a/nextcloud_mcp_server/search/semantic.py +++ b/nextcloud_mcp_server/search/semantic.py @@ -8,6 +8,7 @@ from qdrant_client.models import FieldCondition, Filter, MatchValue from nextcloud_mcp_server.config import get_settings from nextcloud_mcp_server.embedding import get_embedding_service from nextcloud_mcp_server.observability.metrics import record_qdrant_operation +from nextcloud_mcp_server.search.access_filter import build_ownership_filter from nextcloud_mcp_server.search.algorithms import ( SearchAlgorithm, SearchResult, @@ -65,7 +66,11 @@ class SemanticSearchAlgorithm(SearchAlgorithm): user_id: User ID for filtering limit: Maximum results to return doc_type: Optional document type filter - **kwargs: Additional parameters (score_threshold override) + **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 @@ -75,6 +80,7 @@ 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", @@ -97,10 +103,7 @@ class SemanticSearchAlgorithm(SearchAlgorithm): # Build Qdrant filter filter_conditions = [ get_placeholder_filter(), # Always exclude placeholders from user-facing queries - FieldCondition( - key="user_id", - match=MatchValue(value=user_id), - ), + build_ownership_filter(user_id, accessible_owners), ] # Add doc_type filter if specified diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index e8d1a396..c99cb274 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -30,6 +30,7 @@ from nextcloud_mcp_server.models.semantic import ( from nextcloud_mcp_server.observability.metrics import ( instrument_tool, ) +from nextcloud_mcp_server.search.access_filter import list_accessible_owners from nextcloud_mcp_server.search.bm25_hybrid import BM25HybridSearchAlgorithm from nextcloud_mcp_server.search.context import get_chunk_with_context from nextcloud_mcp_server.search.verification import verify_search_results @@ -121,6 +122,12 @@ def configure_semantic_tools(mcp: FastMCP): ) ) + # Expand the caller's identity to every owner whose content they + # have read access to via Nextcloud shares. Lets a user find files + # owners have shared with them without having to re-index those + # files under their own user_id. + accessible_owners = await list_accessible_owners(client.sharing, username) + try: # Create BM25 hybrid search algorithm with specified fusion search_algo = BM25HybridSearchAlgorithm( @@ -153,6 +160,7 @@ def configure_semantic_tools(mcp: FastMCP): limit=limit * 2, doc_type=None, # Signal to search all types score_threshold=score_threshold, + accessible_owners=accessible_owners, ) all_results.extend(unverified_results) else: @@ -177,6 +185,7 @@ def configure_semantic_tools(mcp: FastMCP): limit=limit * 2, doc_type=dtype, score_threshold=score_threshold, + accessible_owners=accessible_owners, ) all_results.extend(unverified_results) diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index d77f1868..09c960ce 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -706,6 +706,16 @@ async def _index_document( }, payload={ "user_id": doc_task.user_id, + # owner_id is the UID of the file's owner — what + # search-time ACL expansion filters on. Today the scanner + # always runs as the file's owner (per-user crawl, only + # surfaces files the user owns or that fall under their + # WebDAV root), so owner_id == user_id is correct for + # every doc type indexed here. The fields are kept + # separate so a future indexer change that lets a user + # crawl shared-with-them content can set owner_id to the + # true owner without losing the "who indexed this" trail. + "owner_id": doc_task.owner_id or doc_task.user_id, "doc_id": doc_task.doc_id, "doc_type": doc_task.doc_type, "is_placeholder": False, # Real indexed document (not placeholder) diff --git a/nextcloud_mcp_server/vector/scanner.py b/nextcloud_mcp_server/vector/scanner.py index d14bbcec..ba3f5998 100644 --- a/nextcloud_mcp_server/vector/scanner.py +++ b/nextcloud_mcp_server/vector/scanner.py @@ -108,6 +108,12 @@ class DocumentTask: metadata: dict[str, int | str] | None = ( None # Additional metadata (e.g., board_id/stack_id for deck_card) ) + # UID of the true owner of the indexed object, used by the search-time + # ACL filter. None today (scanner always runs as the owner, so the + # processor falls back to user_id), but settable so a future + # shared-with-me crawl can pass through the actual owner without + # reshaping the payload contract. + owner_id: str | None = None # Track documents potentially deleted (grace period before actual deletion) diff --git a/tests/unit/search/test_access_filter.py b/tests/unit/search/test_access_filter.py new file mode 100644 index 00000000..f3ee3bb0 --- /dev/null +++ b/tests/unit/search/test_access_filter.py @@ -0,0 +1,118 @@ +"""Tests for nextcloud_mcp_server.search.access_filter.""" + +from __future__ import annotations + +from unittest.mock import AsyncMock + +import pytest + +from nextcloud_mcp_server.search.access_filter import ( + build_ownership_filter, + list_accessible_owners, +) + + +class TestListAccessibleOwners: + @pytest.mark.unit + async def test_includes_self_even_with_no_shares(self) -> None: + sharing = AsyncMock() + sharing.list_shares.return_value = [] + + owners = await list_accessible_owners(sharing, "alice") + assert owners == ["alice"] + + @pytest.mark.unit + async def test_collects_uid_owner_from_shares(self) -> None: + sharing = AsyncMock() + sharing.list_shares.return_value = [ + {"uid_owner": "bob", "share_with": "alice"}, + {"uid_owner": "carol", "share_with": "alice"}, + ] + + owners = await list_accessible_owners(sharing, "alice") + assert set(owners) == {"alice", "bob", "carol"} + + @pytest.mark.unit + async def test_deduplicates_repeated_owners(self) -> None: + sharing = AsyncMock() + sharing.list_shares.return_value = [ + {"uid_owner": "bob"}, + {"uid_owner": "bob"}, # same owner shares many files + {"uid_owner": "bob"}, + ] + + owners = await list_accessible_owners(sharing, "alice") + assert sorted(owners) == ["alice", "bob"] + + @pytest.mark.unit + async def test_falls_back_to_owner_field_when_uid_owner_missing(self) -> None: + # Some Nextcloud versions surface `owner` instead of `uid_owner` + # on the shared-with-me response. + sharing = AsyncMock() + sharing.list_shares.return_value = [{"owner": "bob"}] + + owners = await list_accessible_owners(sharing, "alice") + assert sorted(owners) == ["alice", "bob"] + + @pytest.mark.unit + async def test_ignores_share_with_no_owner_field(self) -> None: + sharing = AsyncMock() + sharing.list_shares.return_value = [ + {"id": 42}, # malformed share entry + {"uid_owner": "bob"}, + {"uid_owner": 12345}, # non-string owner — skip + ] + + owners = await list_accessible_owners(sharing, "alice") + assert sorted(owners) == ["alice", "bob"] + + @pytest.mark.unit + async def test_degrades_to_self_on_sharing_api_failure(self) -> None: + sharing = AsyncMock() + sharing.list_shares.side_effect = RuntimeError("OCS down") + + owners = await list_accessible_owners(sharing, "alice") + # Fail-open to "self only" rather than blowing up search. + assert owners == ["alice"] + + @pytest.mark.unit + async def test_calls_shared_with_me(self) -> None: + sharing = AsyncMock() + sharing.list_shares.return_value = [] + + await list_accessible_owners(sharing, "alice") + sharing.list_shares.assert_awaited_once_with(shared_with_me=True) + + +class TestBuildOwnershipFilter: + def test_defaults_to_self_only_when_owners_omitted(self) -> None: + flt = build_ownership_filter("alice") + + assert flt.should is not None + assert len(flt.should) == 2 # owner_id branch + legacy user_id branch + owner_branch, user_branch = flt.should + assert owner_branch.key == "owner_id" + assert owner_branch.match.any == ["alice"] + assert user_branch.key == "user_id" + assert user_branch.match.value == "alice" + + def test_expands_owner_branch_with_accessible_owners(self) -> None: + flt = build_ownership_filter("alice", ["alice", "bob", "carol"]) + + owner_branch, user_branch = flt.should + # Owner branch reflects the expanded set. + assert set(owner_branch.match.any) == {"alice", "bob", "carol"} + # Legacy user_id branch keeps the original user — that's the only + # legacy match path, so it must NOT widen to other owners. + assert user_branch.match.value == "alice" + + def test_explicit_empty_list_still_keeps_legacy_branch(self) -> None: + # Edge case: caller passed an explicit empty list. We shouldn't + # silently re-default to [user_id] in the owner branch, but the + # legacy branch is still the safety net so the user can find their + # own content from before the migration. + flt = build_ownership_filter("alice", []) + + owner_branch, user_branch = flt.should + assert owner_branch.match.any == [] + assert user_branch.match.value == "alice"