Merge remote-tracking branch 'origin/master' into feat/decomp-hook-points

# Conflicts:
#	nextcloud_mcp_server/vector/scanner.py
This commit is contained in:
Chris Coutinho
2026-05-29 18:31:05 +02:00
40 changed files with 2070 additions and 1804 deletions
+188
View File
@@ -0,0 +1,188 @@
"""Tests for nextcloud_mcp_server.search.access_filter."""
from __future__ import annotations
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,
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:
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 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")
# Self-only: just the user_id branch. Self is NOT duplicated into an
# owner_id branch (the user_id branch already covers self-owned content).
assert flt.should is not None
assert len(flt.should) == 1
(user_branch,) = flt.should
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 holds only the OTHER owners — self ("alice") is excluded
# because the user_id branch already matches self-owned content.
assert set(owner_branch.match.any) == {"bob", "carol"}
assert user_branch.key == "user_id"
assert user_branch.match.value == "alice"
def test_explicit_empty_list_omits_owner_branch_keeps_legacy(self) -> None:
# Edge case: caller passed an explicit empty list. The owner_id branch
# is omitted entirely (rather than relying on MatchAny(any=[]) matching
# nothing); the legacy user_id branch remains as the safety net so the
# user still finds their own content from before the migration.
flt = build_ownership_filter("alice", [])
assert flt.should is not None
assert len(flt.should) == 1
(user_branch,) = flt.should
assert user_branch.key == "user_id"
assert user_branch.match.value == "alice"
+74 -55
View File
@@ -438,10 +438,16 @@ async def test_verify_news_items_malformed_api_response_keeps_all(mocker):
@pytest.mark.unit
async def test_verify_files_uses_path_from_metadata(mocker):
"""File verifier reads path from SearchResult.metadata, no Qdrant round-trip."""
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.
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(
get_file_info=mocker.AsyncMock(return_value={"id": 100})
file_accessible_by_id=mocker.AsyncMock(return_value=True)
)
client = SimpleNamespace(webdav=webdav_client, username="alice")
@@ -452,14 +458,15 @@ async def test_verify_files_uses_path_from_metadata(mocker):
)
assert result == {"100"}
webdav_client.get_file_info.assert_awaited_once_with("Documents/foo.txt")
webdav_client.file_accessible_by_id.assert_awaited_once_with(100)
@pytest.mark.unit
async def test_verify_files_404_drops(mocker):
"""get_file_info raising HTTPStatusError(404) is a definitive drop."""
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(
get_file_info=mocker.AsyncMock(side_effect=_http_error(404))
file_accessible_by_id=mocker.AsyncMock(return_value=False)
)
client = SimpleNamespace(webdav=webdav_client, username="alice")
@@ -473,68 +480,49 @@ async def test_verify_files_404_drops(mocker):
@pytest.mark.unit
async def test_verify_files_malformed_propfind_keeps_result(mocker):
"""get_file_info returning None means malformed PROPFIND — keep the result.
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")
Per the contract change in webdav.py: ``None`` is now reserved for the
ambiguous "malformed XML" case. Real 404s raise HTTPStatusError. The
file verifier must NOT evict on the ambiguous case (we cannot tell
whether the file exists), only log a warning and keep the result.
"""
webdav_client = SimpleNamespace(get_file_info=mocker.AsyncMock(return_value=None))
client = SimpleNamespace(webdav=webdav_client, username="alice")
result = await _verify_files(
client,
[_make_result(124, doc_type="file", metadata={"path": "x.txt"})],
_sem(),
)
result = await _verify_files(
client,
[_make_result(123, doc_type="file", metadata={"path": "brittle.txt"})],
_sem(),
)
assert result == {"123"}, "ambiguous None must keep result, not evict"
assert result == set(), f"{status} on the SEARCH call must drop"
@pytest.mark.unit
async def test_verify_files_403_drops(mocker):
"""get_file_info raising HTTPStatusError(403) is a definitive drop."""
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(
get_file_info=mocker.AsyncMock(side_effect=_http_error(403))
file_accessible_by_id=mocker.AsyncMock(
side_effect=AssertionError("must not be called")
)
)
client = SimpleNamespace(webdav=webdav_client, username="alice")
result = await _verify_files(
client,
[_make_result(124, doc_type="file", metadata={"path": "forbidden.txt"})],
[_make_result("not-a-file-id", doc_type="file", metadata={"path": "x.txt"})],
_sem(),
)
assert result == set()
@pytest.mark.unit
async def test_verify_files_missing_path_metadata_keeps_unverified(mocker):
"""Without a path in metadata we cannot verify — fail open, don't drop."""
webdav_client = SimpleNamespace(
get_file_info=mocker.AsyncMock(side_effect=AssertionError("must not be called"))
)
client = SimpleNamespace(webdav=webdav_client, username="alice")
# No metadata at all
result = await _verify_files(client, [_make_result(555, doc_type="file")], _sem())
assert result == {"555"}
webdav_client.get_file_info.assert_not_awaited()
# Metadata present but no "path" key
result = await _verify_files(
client, [_make_result(556, doc_type="file", metadata={})], _sem()
)
assert result == {"556"}
webdav_client.get_file_info.assert_not_awaited()
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(
get_file_info=mocker.AsyncMock(side_effect=_http_error(503))
file_accessible_by_id=mocker.AsyncMock(side_effect=_http_error(503))
)
client = SimpleNamespace(webdav=webdav_client, username="alice")
@@ -549,9 +537,9 @@ async def test_verify_files_transient_5xx_keeps(mocker):
@pytest.mark.unit
async def test_verify_files_429_keeps_as_transient(mocker):
"""HTTP 429 from get_file_info must NOT silently drop file results."""
"""HTTP 429 from the SEARCH call must NOT silently drop file results."""
webdav_client = SimpleNamespace(
get_file_info=mocker.AsyncMock(side_effect=_http_error(429))
file_accessible_by_id=mocker.AsyncMock(side_effect=_http_error(429))
)
client = SimpleNamespace(webdav=webdav_client, username="alice")
@@ -566,14 +554,14 @@ async def test_verify_files_429_keeps_as_transient(mocker):
@pytest.mark.unit
async def test_verify_files_unexpected_exception_keeps(mocker):
"""A non-HTTP exception from get_file_info must not drop the result.
"""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(
get_file_info=mocker.AsyncMock(side_effect=RuntimeError("dav blew up"))
file_accessible_by_id=mocker.AsyncMock(side_effect=RuntimeError("dav blew up"))
)
client = SimpleNamespace(webdav=webdav_client, username="alice")
@@ -886,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.
+10 -2
View File
@@ -68,7 +68,11 @@ class TestIndexedPath:
# One scroll call, and the filter must include chunk_index (not offsets)
qdrant_client.scroll.assert_awaited_once()
scroll_kwargs = qdrant_client.scroll.await_args.kwargs
filter_keys = [c.key for c in scroll_kwargs["scroll_filter"].must]
# Skip nested Filters (the ACL ownership sub-filter) — only field
# conditions carry a `.key`.
filter_keys = [
c.key for c in scroll_kwargs["scroll_filter"].must if hasattr(c, "key")
]
assert "chunk_index" in filter_keys
assert "chunk_start_offset" not in filter_keys
assert "chunk_end_offset" not in filter_keys
@@ -92,7 +96,11 @@ class TestOffsetFallbackPath:
assert result == (bbox, 2)
scroll_kwargs = qdrant_client.scroll.await_args.kwargs
filter_keys = [c.key for c in scroll_kwargs["scroll_filter"].must]
# Skip nested Filters (the ACL ownership sub-filter) — only field
# conditions carry a `.key`.
filter_keys = [
c.key for c in scroll_kwargs["scroll_filter"].must if hasattr(c, "key")
]
assert "chunk_start_offset" in filter_keys
assert "chunk_end_offset" in filter_keys
assert "chunk_index" not in filter_keys
+39
View File
@@ -151,6 +151,45 @@ async def test_poll_expired(flow_client):
assert result.app_password is None
async def test_initiate_rewrites_login_url_to_public_host():
"""When server↔Nextcloud uses an internal host (e.g. the ``app`` Docker
service), the browser-facing login URL must be rewritten to the configured
public host; the poll endpoint stays on the internal host for server-side
polling. Mock URLs use https to match this file's convention (the rewrite
is scheme-agnostic, so this exercises the same origin-replacement logic)."""
client = LoginFlowV2Client(
nextcloud_host="https://nc-internal.test", # server↔Nextcloud origin
verify_ssl=False,
public_host="https://cloud.example.com", # browser-reachable origin
)
mock_response = _mock_response(
200,
{
# Nextcloud builds these from the request (internal) host.
"login": "https://nc-internal.test/login/v2/flow/tok123",
"poll": {
"endpoint": "https://nc-internal.test/login/v2/poll",
"token": "tok", # value irrelevant here; this test asserts the URLs
},
},
)
mock_client = AsyncMock()
mock_client.post.return_value = mock_response
mock_client.__aenter__ = AsyncMock(return_value=mock_client)
mock_client.__aexit__ = AsyncMock(return_value=False)
with patch(
"nextcloud_mcp_server.auth.login_flow.nextcloud_httpx_client",
return_value=mock_client,
):
result = await client.initiate()
# Browser-facing URL uses the public host...
assert result.login_url == "https://cloud.example.com/login/v2/flow/tok123"
# ...while the poll endpoint stays on the internal host (server polls it).
assert result.poll_endpoint == "https://nc-internal.test/login/v2/poll"
async def test_initiate_with_custom_user_agent(flow_client):
"""Test that custom user agent is passed in the request."""
mock_response = _mock_response(
@@ -180,6 +180,25 @@ async def test_provision_app_password_invalid_format():
assert "Invalid app password format" in response.json()["error"]
def test_app_password_pattern_accepts_dashed_and_raw_tokens():
"""The format guard accepts both the dashed Security-settings format and
the raw token from the one-click ``core/getapppassword`` flow, and still
rejects short / illegal-character input."""
from nextcloud_mcp_server.api.passwords import APP_PASSWORD_PATTERN
# Dashed format a user copies from Security settings.
assert APP_PASSWORD_PATTERN.match("abcde-ABCDE-12345-fghij-67890")
# Raw 72-char token returned by core/getapppassword (one-click opt-in).
assert APP_PASSWORD_PATTERN.match(
"kZmgLDQnqQHUAxhRq4d2VssBfjsI0PaHbL4JySWtwJkzVgAf34c0sZshEjZjuj1PLbwrf83q"
)
# Still rejects obviously-bad input.
assert not APP_PASSWORD_PATTERN.match("short")
assert not APP_PASSWORD_PATTERN.match("invalid-password") # < 20 chars
assert not APP_PASSWORD_PATTERN.match("has spaces not allowed in this token")
assert not APP_PASSWORD_PATTERN.match("contains/slash/" + "a" * 20)
async def test_provision_app_password_success(temp_storage, mocker):
"""Test successful app password provisioning."""
# Mock settings (imported locally in the function)
@@ -0,0 +1,93 @@
"""Unit tests for app-password-store awareness in the provisioning tools.
Login Flow v2 (nc_auth_provision_access) and the management app-password API
write the credential to this server's ``app_passwords`` store — the same store
``require_provisioning``/``get_client`` use to grant tool access. The OAuth
provisioning tools (check_provisioning_status / revoke_nextcloud_access) must
read and clear that store too, otherwise they report "not provisioned" while
tools still work, and "nothing to revoke" while the credential persists.
"""
from types import SimpleNamespace
from unittest.mock import AsyncMock, MagicMock
import pytest
from nextcloud_mcp_server.server import oauth_tools
from nextcloud_mcp_server.server.oauth_tools import (
_get_provisioning_status,
_revoke_nextcloud_access,
)
pytestmark = pytest.mark.unit
@pytest.fixture
def _no_astrolabe_settings(mocker):
"""Disable the astrolabe-status branch so the app_passwords store is hit."""
mocker.patch.object(
oauth_tools,
"get_settings",
return_value=SimpleNamespace(oidc_client_id=None, oidc_client_secret=None),
)
async def test_status_reports_provisioned_for_app_password_store(
mocker, _no_astrolabe_settings
):
"""A Login Flow v2 app password in storage => is_provisioned with the
app_password credential type (was previously reported as not provisioned)."""
storage = MagicMock()
# Only truthiness + "scopes" are read by _get_provisioning_status; omit the
# app_password value entirely (avoids a false-positive hard-coded-credential
# finding and keeps the mock to what the code under test actually uses).
storage.get_app_password_with_scopes = AsyncMock(
return_value={"scopes": ["notes.read"]}
)
storage.get_refresh_token = AsyncMock(return_value=None)
mocker.patch.object(
oauth_tools, "get_shared_storage", AsyncMock(return_value=storage)
)
status = await _get_provisioning_status(MagicMock(), "tester")
assert status.is_provisioned is True
assert status.credential_type == "app_password"
assert status.flow_type == "login_flow_v2"
assert status.scopes == ["notes.read"]
storage.get_refresh_token.assert_not_awaited() # app password short-circuits
async def test_revoke_deletes_app_password(mocker, _no_astrolabe_settings):
"""Revoke must delete the app password from storage (not just refresh tokens)."""
storage = MagicMock()
storage.get_app_password_with_scopes = AsyncMock(return_value={"scopes": None})
storage.get_refresh_token = AsyncMock(return_value=None)
storage.delete_app_password = AsyncMock(return_value=True)
mocker.patch.object(
oauth_tools, "get_shared_storage", AsyncMock(return_value=storage)
)
mocker.patch.object(oauth_tools, "invalidate_scope_cache")
result = await _revoke_nextcloud_access(MagicMock(), "tester")
assert result.success is True
storage.delete_app_password.assert_awaited_once_with("tester")
oauth_tools.invalidate_scope_cache.assert_called_once_with("tester")
async def test_revoke_noop_when_nothing_provisioned(mocker, _no_astrolabe_settings):
"""No credential of any kind => graceful no-op, no deletion attempted."""
storage = MagicMock()
storage.get_app_password_with_scopes = AsyncMock(return_value=None)
storage.get_refresh_token = AsyncMock(return_value=None)
storage.delete_app_password = AsyncMock()
mocker.patch.object(
oauth_tools, "get_shared_storage", AsyncMock(return_value=storage)
)
result = await _revoke_nextcloud_access(MagicMock(), "tester")
assert result.success is True
assert "No Nextcloud access to revoke" in result.message
storage.delete_app_password.assert_not_awaited()