admin/mcp-nextcloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #834 from cbcoutinho/fix/verify-on-read-tag-gate

Browse Source 
fix(search): gate verify-on-read file results on vector-index tag membership
This commit is contained in:
Chris Coutinho
2026-06-03 02:00:34 +02:00
committed by GitHub
parent f8afde0751 b736bf199b
commit 9adbf9e8a5
10 changed files with 431 additions and 221 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/integration/conftest.py
+13
View File
@@ -13,6 +13,19 @@ logger = logging.getLogger(__name__)
# Valid provider names
VALID_PROVIDERS = ["openai", "ollama", "anthropic", "bedrock"]
# Canonical minimal valid PDF for integration tests. verify-on-read gates file
# results on the vector-index tag via
# find_files_by_tag(..., mime_type_filter="application/pdf"), so file fixtures
# must be PDFs (matching what the scanner indexes), not .txt. Shared here so the
# constant is defined once rather than drifting across test modules.
PDF_BYTES = (
b"%PDF-1.4\n"
b"1 0 obj<</Type/Catalog/Pages 2 0 R>>endobj\n"
b"2 0 obj<</Type/Pages/Kids[3 0 R]/Count 1>>endobj\n"
b"3 0 obj<</Type/Page/Parent 2 0 R/MediaBox[0 0 200 200]>>endobj\n"
b"trailer<</Root 1 0 R>>\n%%EOF\n"
)
def pytest_addoption(parser):
"""Add --provider command line option for RAG tests."""
tests/integration/test_acl_shared_search.py
+28 -5
View File
@@ -40,6 +40,7 @@ from nextcloud_mcp_server.search.access_filter import (
from nextcloud_mcp_server.search.context import get_chunk_with_context
from nextcloud_mcp_server.search.semantic import SemanticSearchAlgorithm
from nextcloud_mcp_server.search.verification import verify_search_results
from tests.integration.conftest import PDF_BYTES
pytestmark = pytest.mark.integration
@@ -81,7 +82,13 @@ async def acl_users(test_users_setup):
@pytest.fixture
async def shared_file(acl_users):
"""alice creates a nested file and shares it with bob (not diana).
"""alice creates a nested PDF, tags it ``vector-index``, and shares it with
bob (not diana).
The vector-index tag is required because verify-on-read now gates file
results on current tag membership (in addition to ACL access). The tag is
created userVisible so the owner's assignment surfaces in the recipient's
systemtag REPORT — this fixture is the live check of that assumption.
Yields (file_id, owner_relative_path); cleans up the directory after.
"""
@@ -89,18 +96,28 @@ async def shared_file(acl_users):
suffix = uuid.uuid4().hex[:8]
test_dir = f"acl_e2e_{suffix}"
nested = f"{test_dir}/reports"
path = f"{nested}/budget.txt"
path = f"{nested}/budget.pdf"
await alice.webdav.create_directory(test_dir)
await alice.webdav.create_directory(nested)
await alice.webdav.write_file(path, _DOC_TEXT.encode(), "text/plain")
await alice.webdav.write_file(path, PDF_BYTES, "application/pdf")
file_id = (await alice.webdav.get_file_info(path))["id"]
tag = await alice.webdav.get_or_create_tag(
name=get_settings().vector_sync_pdf_tag,
user_visible=True,
user_assignable=True,
)
await alice.webdav.assign_tag_to_file(file_id, tag["id"])
await alice.sharing.create_share(
path=f"/{path}", share_with="bob", share_type=0, permissions=1
)
try:
yield file_id, path
finally:
try:
await alice.webdav.remove_tag_from_file(file_id, tag["id"])
except Exception:
pass
await alice.webdav.delete_resource(test_dir)
@@ -132,7 +149,7 @@ async def seeded_semantic(monkeypatch, shared_file):
"user_id": "alice",
"is_placeholder": False,
"file_path": path,
"title": "budget.txt",
"title": "budget.pdf",
"excerpt": _DOC_TEXT,
"chunk_index": 0,
"total_chunks": 1,
@@ -180,7 +197,13 @@ async def _search_as(user_client, file_id_unused) -> list:
async def test_recipient_finds_shared_file_without_indexing(acl_users, seeded_semantic):
"""Bob finds alice's shared file end-to-end: real share lookup expands his
accessible owners to include alice, the filter surfaces her point, and
real verification confirms his ACL access — all without bob indexing."""
real verification confirms both his ACL access AND that the file is still
in the vector-index tag set — all without bob indexing.
This also exercises the strict tag-gate's key assumption: a vector-index
tag alice assigned (userVisible) surfaces in bob's systemtag REPORT for a
file shared into his tree. If a future Nextcloud version stops surfacing an
owner's tag to a recipient, this assertion is where it fails first."""
file_id = seeded_semantic
# Sanity: the live OCS lookup really does expand bob to include alice.
owners = await list_accessible_owners(acl_users["bob"].sharing, "bob")
tests/integration/test_verify_on_read.py
+47 -31
View File
@@ -5,16 +5,16 @@ instance — the verification path's whole purpose is to consult Nextcloud as
the source of truth, so unit-level mocks don't catch protocol or status-code
mismatches between our verifier and the real API.
**Coverage**: only the ``note`` verifier is exercised against real Nextcloud
here. The ``file`` (WebDAV PROPFIND), ``deck_card`` (Deck app), and
``news_item`` (News app) verifiers are unit-tested with mocked HTTP
responses in ``tests/unit/search/test_verification.py``. Adding integration
coverage for those types is tracked as a follow-up — it requires fixture
data (tagged PDFs in user files, a Deck board with cards, a News feed) that
is non-trivial to seed from CI. The mocked unit tests are accurate for
status-code semantics but won't catch payload-shape regressions in those
Nextcloud apps; the trade-off is documented here so future readers know
which suite owns which verifier.
**Coverage**: the ``note`` verifier and the ``file`` verifier (tag-membership
gate, see the shared-recipient tests below) are exercised against real
Nextcloud here. The ``deck_card`` (Deck app) and ``news_item`` (News app)
verifiers are unit-tested with mocked HTTP responses in
``tests/unit/search/test_verification.py``. Adding integration coverage for
those types is tracked as a follow-up — it requires fixture data (a Deck board
with cards, a News feed) that is non-trivial to seed from CI. The mocked unit
tests are accurate for status-code semantics but won't catch payload-shape
regressions in those Nextcloud apps; the trade-off is documented here so
future readers know which suite owns which verifier.
Qdrant is mocked out (``delete_document_points`` and the payload-resolution
helpers) so these tests don't require a running vector database. The unit
@@ -30,9 +30,11 @@ import pytest
from httpx import BasicAuth, HTTPStatusError
from nextcloud_mcp_server.client import NextcloudClient
from nextcloud_mcp_server.config import get_settings
from nextcloud_mcp_server.search import verification
from nextcloud_mcp_server.search.algorithms import SearchResult
from nextcloud_mcp_server.search.verification import verify_search_results
from tests.integration.conftest import PDF_BYTES
logger = logging.getLogger(__name__)
@@ -41,7 +43,7 @@ pytestmark = pytest.mark.integration
def _result_for_note(note_id: int) -> SearchResult:
return SearchResult(
id=note_id,
id=str(note_id),
doc_type="note",
title=f"note_{note_id}",
excerpt="...",
@@ -51,9 +53,10 @@ def _result_for_note(note_id: int) -> SearchResult:
def _result_for_file(file_id: int, path: str) -> SearchResult:
# Mirrors what the algorithm layer propagates: doc_id IS the global file id,
# ``path`` is carried in metadata (owner-relative) for log context only.
# stringified (SearchResult.id is always str), ``path`` is carried in
# metadata (owner-relative) for log context only.
return SearchResult(
id=file_id,
id=str(file_id),
doc_type="file",
title=path.split("/")[-1],
excerpt="...",
@@ -83,7 +86,7 @@ async def test_verify_keeps_accessible_note(
kept, dropped_count = await verify_search_results(nc_client, results)
assert [r.id for r in kept] == [note_id]
assert [r.id for r in kept] == [str(note_id)]
assert dropped_count == 0
spy_evict.assert_not_awaited()
@@ -126,7 +129,7 @@ async def test_verify_drops_deleted_note_and_schedules_eviction(
assert kept == [], "deleted note must not pass verification"
assert dropped_count == 1
spy_evict.assert_awaited_once_with(note_id, "note", nc_client.username)
spy_evict.assert_awaited_once_with(str(note_id), "note", nc_client.username)
async def test_verify_mixed_accessible_and_deleted(
@@ -155,9 +158,9 @@ async def test_verify_mixed_accessible_and_deleted(
]
kept, dropped_count = await verify_search_results(nc_client, results)
assert [r.id for r in kept] == [accessible_id]
assert [r.id for r in kept] == [str(accessible_id)]
assert dropped_count == 1
spy_evict.assert_awaited_once_with(ghost_id, "note", nc_client.username)
spy_evict.assert_awaited_once_with(str(ghost_id), "note", nc_client.username)
async def test_verify_dedupes_chunks_of_same_document(
@@ -176,7 +179,7 @@ async def test_verify_dedupes_chunks_of_same_document(
# Three chunks of the same note (chunk_index varies)
results = [
SearchResult(
id=note_id,
id=str(note_id),
doc_type="note",
title="note",
excerpt=f"chunk {i}",
@@ -222,11 +225,13 @@ async def alice_bob_clients(test_users_setup):
async def test_verify_keeps_nested_file_shared_with_recipient(
alice_bob_clients, mocker
):
"""The PR #813 acceptance check at the verifier layer.
"""The PR #813 acceptance check at the verifier layer, under tag-gating.
Alice owns a file in a *subfolder* and shares it with Bob. Verifying the
result as Bob must KEEP it — proving the id-based check sees the share.
A path-based check (the old behaviour) would 404 here and wrongly drop it.
Alice owns a PDF in a *subfolder*, tags it ``vector-index`` (userVisible),
and shares it with Bob. Verifying the result as Bob must KEEP it — proving
the tag REPORT surfaces an owner-assigned tag on a file shared into Bob's
tree. If a future Nextcloud version stops surfacing the owner's tag to a
recipient, this is where strict tag-gating regresses shared search.
"""
spy_evict = mocker.AsyncMock()
mocker.patch.object(verification, "delete_document_points", spy_evict)
@@ -235,12 +240,18 @@ async def test_verify_keeps_nested_file_shared_with_recipient(
suffix = uuid.uuid4().hex[:8]
test_dir = f"acl_verify_{suffix}"
nested_dir = f"{test_dir}/reports"
shared_path = f"{nested_dir}/shared.txt"
shared_path = f"{nested_dir}/shared.pdf"
await alice.webdav.create_directory(test_dir)
await alice.webdav.create_directory(nested_dir)
await alice.webdav.write_file(shared_path, b"alice's shared report", "text/plain")
await alice.webdav.write_file(shared_path, PDF_BYTES, "application/pdf")
file_id = (await alice.webdav.get_file_info(shared_path))["id"]
tag = await alice.webdav.get_or_create_tag(
name=get_settings().vector_sync_pdf_tag,
user_visible=True,
user_assignable=True,
)
await alice.webdav.assign_tag_to_file(file_id, tag["id"])
await alice.sharing.create_share(
path=f"/{shared_path}", share_with="bob", share_type=0, permissions=1
@@ -251,28 +262,33 @@ async def test_verify_keeps_nested_file_shared_with_recipient(
bob, [_result_for_file(file_id, shared_path)]
)
assert [r.id for r in kept] == [file_id], (
"a nested file shared with bob must pass verification for bob"
assert [r.id for r in kept] == [str(file_id)], (
"a nested tagged PDF shared with bob must pass verification for bob"
)
assert dropped_count == 0
spy_evict.assert_not_awaited()
finally:
try:
await alice.webdav.remove_tag_from_file(file_id, tag["id"])
except Exception:
pass
await alice.webdav.delete_resource(test_dir)
async def test_verify_drops_unshared_file_for_other_user(alice_bob_clients, mocker):
"""Negative control: a file Alice did NOT share is inaccessible to Bob and
must be dropped + scheduled for eviction under his identity."""
"""Negative control: a file Alice did NOT share is absent from Bob's
vector-index tag set (and his tree), so it must be dropped + scheduled for
eviction under his identity."""
spy_evict = mocker.AsyncMock()
mocker.patch.object(verification, "delete_document_points", spy_evict)
alice, bob = alice_bob_clients
suffix = uuid.uuid4().hex[:8]
test_dir = f"acl_verify_priv_{suffix}"
private_path = f"{test_dir}/private.txt"
private_path = f"{test_dir}/private.pdf"
await alice.webdav.create_directory(test_dir)
await alice.webdav.write_file(private_path, b"alice's private note", "text/plain")
await alice.webdav.write_file(private_path, PDF_BYTES, "application/pdf")
file_id = (await alice.webdav.get_file_info(private_path))["id"]
try:
@@ -282,6 +298,6 @@ async def test_verify_drops_unshared_file_for_other_user(alice_bob_clients, mock
assert kept == [], "an unshared file must not pass verification for bob"
assert dropped_count == 1
spy_evict.assert_awaited_once_with(file_id, "file", bob.username)
spy_evict.assert_awaited_once_with(str(file_id), "file", bob.username)
finally:
await alice.webdav.delete_resource(test_dir)
tests/unit/search/test_verification.py
+142 -110
View File
@@ -437,141 +437,173 @@ async def test_verify_news_items_malformed_api_response_keeps_all(mocker):
# ---------------------------------------------------------------------------
def _patch_excluded(mocker, paths: set[str] | None = None, *, side_effect=None):
"""Patch the lazily-imported EXCLUDED_TAGS lookup used by _verify_files."""
if side_effect is not None:
mock = mocker.AsyncMock(side_effect=side_effect)
else:
mock = mocker.AsyncMock(return_value=paths if paths is not None else set())
return mocker.patch(
"nextcloud_mcp_server.server.tag_exclusion.get_excluded_file_paths", mock
)
def _file_client(mocker, *, tagged=None, find_side_effect=None, username="alice"):
"""Build a client whose find_files_by_tag returns the given tagged files."""
if find_side_effect is not None:
find = mocker.AsyncMock(side_effect=find_side_effect)
else:
find = mocker.AsyncMock(return_value=tagged if tagged is not None else [])
return SimpleNamespace(
find_files_by_tag=find,
webdav=SimpleNamespace(),
username=username,
)
@pytest.mark.unit
async def test_verify_files_accessible_by_global_id_is_kept(mocker):
"""File verifier resolves the file by its global ID (the doc_id), ACL-aware.
async def test_verify_files_tagged_is_kept(mocker):
"""A file currently carrying the vector-index tag is kept, and the tagged
set is fetched with a single batch call (not one per result)."""
_patch_excluded(mocker)
client = _file_client(mocker, tagged=[{"id": 100, "path": "/Documents/foo.pdf"}])
This is what lets a recipient verify a file an owner shared with them:
file_accessible_by_id searches the user's whole tree (incl. mounted
shares) by global file id, not a path under the caller's own root (which
would 404 on shared files mounted at a different path).
"""
webdav_client = SimpleNamespace(
file_accessible_by_id=mocker.AsyncMock(return_value=True)
)
client = SimpleNamespace(webdav=webdav_client, username="alice")
result = await _verify_files(
client,
[_make_result(100, doc_type="file", metadata={"path": "Documents/foo.txt"})],
_sem(),
)
result = await _verify_files(client, [_make_result(100, doc_type="file")], _sem())
assert result == {"100"}
webdav_client.file_accessible_by_id.assert_awaited_once_with(100)
client.find_files_by_tag.assert_awaited_once_with(
"vector-index", mime_type_filter="application/pdf"
)
@pytest.mark.unit
async def test_verify_files_inaccessible_id_drops(mocker):
"""file_accessible_by_id returning False (file not in the user's tree) is a
definitive drop — the file is neither owned by nor shared with the user."""
webdav_client = SimpleNamespace(
file_accessible_by_id=mocker.AsyncMock(return_value=False)
)
client = SimpleNamespace(webdav=webdav_client, username="alice")
async def test_verify_files_untagged_drops(mocker):
"""A file removed from the vector-index tag (absent from the tagged set) is
dropped even though it may still exist and be readable by the user."""
_patch_excluded(mocker)
client = _file_client(mocker, tagged=[{"id": 100, "path": "/Documents/foo.pdf"}])
result = await _verify_files(
client,
[_make_result(123, doc_type="file", metadata={"path": "gone.txt"})],
[_make_result(100, doc_type="file"), _make_result(200, doc_type="file")],
_sem(),
)
# 100 is still tagged → kept; 200 was untagged → dropped.
assert result == {"100"}
@pytest.mark.unit
async def test_verify_files_deleted_drops(mocker):
"""A deleted file is absent from the tagged set → dropped (caller evicts)."""
_patch_excluded(mocker)
client = _file_client(mocker, tagged=[])
result = await _verify_files(client, [_make_result(123, doc_type="file")], _sem())
assert result == set()
@pytest.mark.unit
async def test_verify_files_403_404_drops(mocker):
"""A 403/404 raised by the SEARCH call is treated as a definitive drop,
consistent with the shared _is_definitive_404_or_403 policy used by every
verifier. (Normal inaccessibility surfaces as an empty result set, not a
status code, and is covered by test_verify_files_inaccessible_id_drops.)"""
for status in (403, 404):
webdav_client = SimpleNamespace(
file_accessible_by_id=mocker.AsyncMock(side_effect=_http_error(status))
)
client = SimpleNamespace(webdav=webdav_client, username="alice")
async def test_verify_files_empty_tag_set_skips_exclusion_lookup(mocker):
"""When the tag REPORT returns no files, the EXCLUDED_TAGS lookup is skipped
entirely: an empty tagged set drops every valid-id result regardless of
exclusions, so the lookup's 2xN WebDAV fan-out is wasted work. Malformed
doc_ids are still kept (fail-open), exactly as on the non-empty path."""
excluded = _patch_excluded(mocker, {"Secret"})
client = _file_client(mocker, tagged=[])
result = await _verify_files(
client,
[
_make_result(123, doc_type="file"),
_make_result("not-a-file-id", doc_type="file"),
],
_sem(),
)
# Valid id absent from the (empty) tagged set → dropped; malformed id kept.
assert result == {"not-a-file-id"}
# The optimization: no exclusion fan-out when there is nothing to filter.
excluded.assert_not_awaited()
@pytest.mark.unit
async def test_verify_files_excluded_path_drops(mocker):
"""A tagged file under an EXCLUDED_TAGS folder must not surface — exclusion
wins, parity with the scanner's defense-in-depth filter."""
# get_excluded_file_paths returns slash-stripped (normalised) paths.
_patch_excluded(mocker, {"Secret"})
client = _file_client(
mocker,
tagged=[
{"id": 100, "path": "/Documents/foo.pdf"},
{"id": 200, "path": "/Secret/bar.pdf"},
],
)
result = await _verify_files(
client,
[_make_result(100, doc_type="file"), _make_result(200, doc_type="file")],
_sem(),
)
assert result == {"100"}
@pytest.mark.unit
async def test_verify_files_tag_fetch_failure_keeps_all(mocker):
"""If the tag REPORT itself fails, keep every file result (fail-open) —
never silently shrink results on a backend blip.
Unlike the per-access verifiers (notes/deck/news), where a definitive
403/404 is the DROP signal, the file verifier fails open on *every* HTTP
error — including 403/404. The whole result set hinges on one batch REPORT,
so a disabled systemtags endpoint (commonly 403) must not nuke all file
results; the next query re-verifies. 403 and 404 are pinned here alongside
the transient 503/429 to lock that contract against regression.
"""
_patch_excluded(mocker)
for exc in (
_http_error(403),
_http_error(404),
_http_error(503),
_http_error(429),
RuntimeError("dav blew up"),
):
client = _file_client(mocker, find_side_effect=exc)
result = await _verify_files(
client,
[_make_result(124, doc_type="file", metadata={"path": "x.txt"})],
[_make_result(7, doc_type="file"), _make_result(8, doc_type="file")],
_sem(),
)
assert result == set(), f"{status} on the SEARCH call must drop"
assert result == {"7", "8"}, f"{exc!r} on the tag fetch must keep all results"
@pytest.mark.unit
async def test_verify_files_non_numeric_id_keeps_unverified(mocker):
"""Without a numeric file id we cannot verify — fail open, don't drop."""
webdav_client = SimpleNamespace(
file_accessible_by_id=mocker.AsyncMock(
side_effect=AssertionError("must not be called")
)
)
client = SimpleNamespace(webdav=webdav_client, username="alice")
async def test_verify_files_exclusion_lookup_failure_proceeds(mocker):
"""If the EXCLUDED_TAGS lookup fails, proceed without the exclusion filter
rather than dropping legitimate tagged hits."""
_patch_excluded(mocker, side_effect=RuntimeError("ocs down"))
client = _file_client(mocker, tagged=[{"id": 100, "path": "/Documents/foo.pdf"}])
result = await _verify_files(client, [_make_result(100, doc_type="file")], _sem())
assert result == {"100"}
@pytest.mark.unit
async def test_verify_files_non_numeric_id_keeps(mocker):
"""A malformed (non-numeric) doc_id cannot be matched against the numeric
tag REPORT, so it is kept (defense-in-depth, false-positive preferred)."""
_patch_excluded(mocker)
client = _file_client(mocker, tagged=[])
result = await _verify_files(
client,
[_make_result("not-a-file-id", doc_type="file", metadata={"path": "x.txt"})],
_sem(),
client, [_make_result("not-a-file-id", doc_type="file")], _sem()
)
assert result == {"not-a-file-id"}
webdav_client.file_accessible_by_id.assert_not_awaited()
@pytest.mark.unit
async def test_verify_files_transient_5xx_keeps(mocker):
webdav_client = SimpleNamespace(
file_accessible_by_id=mocker.AsyncMock(side_effect=_http_error(503))
)
client = SimpleNamespace(webdav=webdav_client, username="alice")
result = await _verify_files(
client,
[_make_result(7, doc_type="file", metadata={"path": "x.txt"})],
_sem(),
)
assert result == {"7"}
@pytest.mark.unit
async def test_verify_files_429_keeps_as_transient(mocker):
"""HTTP 429 from the SEARCH call must NOT silently drop file results."""
webdav_client = SimpleNamespace(
file_accessible_by_id=mocker.AsyncMock(side_effect=_http_error(429))
)
client = SimpleNamespace(webdav=webdav_client, username="alice")
result = await _verify_files(
client,
[_make_result(7, doc_type="file", metadata={"path": "x.txt"})],
_sem(),
)
assert result == {"7"}
@pytest.mark.unit
async def test_verify_files_unexpected_exception_keeps(mocker):
"""A non-HTTP exception from file_accessible_by_id must not drop the result.
The catch-all ``except Exception`` branch in the file verifier exists
so a bug in the WebDAV client (or an httpx ConnectError on a flaky
network) cannot silently shrink result pages.
"""
webdav_client = SimpleNamespace(
file_accessible_by_id=mocker.AsyncMock(side_effect=RuntimeError("dav blew up"))
)
client = SimpleNamespace(webdav=webdav_client, username="alice")
result = await _verify_files(
client,
[_make_result(8, doc_type="file", metadata={"path": "y.txt"})],
_sem(),
)
assert result == {"8"}
# ---------------------------------------------------------------------------
@@ -889,11 +921,11 @@ async def test_verify_evicts_cross_user_file_under_querying_user_id(mocker):
"""
spy_evict = mocker.AsyncMock()
mocker.patch.object(verification, "delete_document_points", spy_evict)
_patch_excluded(mocker)
webdav_client = SimpleNamespace(
file_accessible_by_id=mocker.AsyncMock(return_value=False)
)
client = SimpleNamespace(webdav=webdav_client, username="bob")
# The shared file is no longer in bob's tagged set (share revoked → absent
# from his vector-index tag REPORT), so the file verifier drops it.
client = _file_client(mocker, tagged=[], username="bob")
kept, dropped_count = await verify_search_results(
client,