diff --git a/docker-compose.yml b/docker-compose.yml index c7bf0e25..82fc3088 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -36,7 +36,7 @@ services: # Mount OIDC development directory outside /var/www/html to avoid rsync conflicts # The post-installation hook will register /opt/apps as an additional app directory #- ./third_party:/opt/apps:ro - #- ./third_party/astrolabe:/opt/apps/astrolabe:ro + - ./third_party/astrolabe:/opt/apps/astrolabe:ro #- ./third_party/oidc:/opt/apps/oidc:ro environment: - NEXTCLOUD_TRUSTED_DOMAINS=app diff --git a/nextcloud_mcp_server/api/passwords.py b/nextcloud_mcp_server/api/passwords.py index bf2651e6..212355b9 100644 --- a/nextcloud_mcp_server/api/passwords.py +++ b/nextcloud_mcp_server/api/passwords.py @@ -30,9 +30,11 @@ from ..http import nextcloud_httpx_client logger = logging.getLogger(__name__) # App password format regex (Nextcloud format: xxxxx-xxxxx-xxxxx-xxxxx-xxxxx) -APP_PASSWORD_PATTERN = re.compile( - r"^[a-zA-Z0-9]{5}-[a-zA-Z0-9]{5}-[a-zA-Z0-9]{5}-[a-zA-Z0-9]{5}-[a-zA-Z0-9]{5}$" -) +# Shape guard only — the authoritative check is the BasicAuth validation +# against Nextcloud below. Accepts both the dashed format a user copies from +# Security settings (xxxxx-xxxxx-xxxxx-xxxxx-xxxxx) and the raw token returned +# by the one-click ``core/getapppassword`` flow (a long alphanumeric string). +APP_PASSWORD_PATTERN = re.compile(r"^[a-zA-Z0-9-]{20,256}$") # Timeout for Nextcloud API validation requests (seconds) NEXTCLOUD_VALIDATION_TIMEOUT = 10.0 diff --git a/nextcloud_mcp_server/api/visualization.py b/nextcloud_mcp_server/api/visualization.py index c13d2f7d..736d9858 100644 --- a/nextcloud_mcp_server/api/visualization.py +++ b/nextcloud_mcp_server/api/visualization.py @@ -11,6 +11,7 @@ All endpoints require OAuth bearer token authentication via UnifiedTokenVerifier import base64 import logging +from collections.abc import Awaitable, Callable from typing import Any import pymupdf @@ -30,10 +31,12 @@ 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, ) +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, @@ -43,6 +46,70 @@ from nextcloud_mcp_server.vector.visualization import compute_pca_coordinates logger = logging.getLogger(__name__) +_NEXTCLOUD_HOST_NOT_CONFIGURED = "Nextcloud host not configured" + + +async def _search_with_acl( + request: Request, + user_id: str, + execute: Callable[[list[str] | None], Awaitable[list]], +) -> list: + """Resolve the caller's Nextcloud client, run ``execute(accessible_owners)``, + and verify-on-read — shared by the /api/v1 search endpoints. + + The OAuth bearer only authenticates Astrolabe → MCP Server; MCP Server → + Nextcloud uses the provisioned app password. When the caller never + provisioned background sync there is no client to expand shares or verify + with, so we fall back to self-only, unverified search (the pre-ACL + behaviour) rather than 401 — keeping search working for users who haven't + opted into background indexing. + + Args: + request: The Starlette request (carries ``app.state.oauth_context``). + user_id: The authenticated caller. + execute: Coroutine that runs the search for a given owner scope + (``None`` ⇒ self-only). + + Returns: + The result list (verified for provisioned callers). + + Raises: + ValueError: If the Nextcloud host is not configured. + """ + oauth_ctx = request.app.state.oauth_context + nextcloud_host = oauth_ctx.get("config", {}).get("nextcloud_host", "") + if not nextcloud_host: + raise ValueError(_NEXTCLOUD_HOST_NOT_CONFIGURED) + + try: + nc_client = await get_user_client_basic_auth(user_id, nextcloud_host) + except NotProvisionedError: + logger.debug("User %s not provisioned; self-only unverified search", user_id) + results = await execute(None) + else: + async with nc_client: + # Expand to owners who shared content with the caller (same as the + # MCP tool path) so shared documents are searchable. + accessible_owners = await list_accessible_owners(nc_client.sharing, user_id) + results = await execute(accessible_owners) + # Verify-on-read (ADR-019): drop documents the caller can no longer + # access (e.g. a revoked share). Eviction runs inline — this + # Starlette route has no FastMCP lifespan task group. + results, _dropped = await verify_search_results(nc_client, results) + + # Safe to log titles now: provisioned callers passed verify-on-read; + # non-provisioned ran self-only (unverified titles are never logged — see + # the search algorithms). + if results: + logger.debug( + "Top verified results: %s", + ", ".join( + f"{r.doc_type}_{r.id} (score={r.score:.3f}, title='{r.title}')" + for r in results[:5] + ), + ) + return results + async def unified_search(request: Request) -> JSONResponse: """POST /api/v1/search - Search endpoint for Nextcloud Unified Search. @@ -164,25 +231,41 @@ async def unified_search(request: Request) -> JSONResponse: # Request extra results to handle offset search_limit = limit + offset - # Execute search - all_results = [] - if doc_types and isinstance(doc_types, list): - for doc_type in doc_types: - if doc_type: - results = await search_algo.search( - query=query, - user_id=user_id, - limit=search_limit, - doc_type=doc_type, - ) - all_results.extend(results) - all_results.sort(key=lambda r: r.score, reverse=True) - else: - all_results = await search_algo.search( - query=query, - user_id=user_id, - limit=search_limit, - ) + async def _execute(owners: list[str] | None) -> list: + """Run the search across requested doc_types with the given owner + scope (None ⇒ self-only).""" + results: list = [] + if doc_types and isinstance(doc_types, list): + for doc_type in doc_types: + if doc_type: + results.extend( + await search_algo.search( + query=query, + user_id=user_id, + limit=search_limit, + doc_type=doc_type, + accessible_owners=owners, + ) + ) + # Sort, then cap to a fixed over-fetch budget before the result + # reaches verify-on-read. Without this, N doc_types each fetched + # at search_limit would send N*search_limit candidates into + # verification — one Nextcloud round-trip each — scaling the cost + # with len(doc_types). 2x leaves headroom for verify-on-read + # drops before pagination, matching the nc_semantic_search and + # viz_routes pattern. + results.sort(key=lambda r: r.score, reverse=True) + results = results[: search_limit * 2] + else: + results = await search_algo.search( + query=query, + user_id=user_id, + limit=search_limit, + accessible_owners=owners, + ) + return results + + all_results = await _search_with_acl(request, user_id, _execute) # Sort results by score (no deduplication - show all chunks) sorted_results = sorted(all_results, key=lambda r: r.score, reverse=True) @@ -357,29 +440,37 @@ async def vector_search(request: Request) -> JSONResponse: score_threshold=score_threshold, fusion=fusion ) - # Execute search for each doc_type if specified, otherwise search all - all_results = [] - if doc_types and isinstance(doc_types, list): - # Search each doc_type separately and merge results - for doc_type in doc_types: - if doc_type: # Skip empty strings - results = await search_algo.search( - query=query, - user_id=user_id, - limit=limit, - doc_type=doc_type, - ) - all_results.extend(results) - # Sort merged results by score and limit - all_results.sort(key=lambda r: r.score, reverse=True) - all_results = all_results[:limit] - else: - # Search all document types - all_results = await search_algo.search( - query=query, - user_id=user_id, - limit=limit, - ) + async def _execute(owners: list[str] | None) -> list: + """Run the search across requested doc_types with the given owner + scope (None ⇒ self-only).""" + results: list = [] + if doc_types and isinstance(doc_types, list): + # Search each doc_type separately and merge results + for doc_type in doc_types: + if doc_type: # Skip empty strings + results.extend( + await search_algo.search( + query=query, + user_id=user_id, + limit=limit, + doc_type=doc_type, + accessible_owners=owners, + ) + ) + # Sort merged results by score and limit + results.sort(key=lambda r: r.score, reverse=True) + results = results[:limit] + else: + # Search all document types + results = await search_algo.search( + query=query, + user_id=user_id, + limit=limit, + accessible_owners=owners, + ) + return results + + all_results = await _search_with_acl(request, user_id, _execute) # Format results for PHP client formatted_results = [] @@ -554,7 +645,7 @@ async def get_chunk_context(request: Request) -> JSONResponse: nextcloud_host = oauth_ctx.get("config", {}).get("nextcloud_host", "") if not nextcloud_host: - raise ValueError("Nextcloud host not configured") + raise ValueError(_NEXTCLOUD_HOST_NOT_CONFIGURED) # Use the user's stored app password for Nextcloud calls. # The OAuth bearer is only used to authenticate Astrolabe → MCP Server; @@ -569,6 +660,10 @@ async def get_chunk_context(request: Request) -> JSONResponse: ) async with nc_client: + # Expand to owners who shared content with the caller so the cached + # chunk lookup can resolve cross-user SHARED FILES (gated per-file + # inside get_chunk_with_context). Same expansion as the search path. + accessible_owners = await list_accessible_owners(nc_client.sharing, user_id) chunk_context = await get_chunk_with_context( nc_client=nc_client, user_id=user_id, @@ -579,6 +674,7 @@ async def get_chunk_context(request: Request) -> JSONResponse: chunk_index=chunk_index, total_chunks=total_chunks, context_chars=context_chars, + accessible_owners=accessible_owners, ) if chunk_context is None: @@ -598,12 +694,16 @@ async def get_chunk_context(request: Request) -> JSONResponse: page_number = chunk_context.page_number if doc_type == "file": + # Reaching here means the file chunk context resolved, so access was + # already confirmed (get_chunk_with_context gates files by id); + # the bbox/page lookup uses the same owner scope for cross-user files. qdrant_bbox, qdrant_page = await get_chunk_bbox_and_page_from_qdrant( user_id=user_id, doc_id=doc_id, chunk_index=chunk_index, chunk_start=start, chunk_end=end, + accessible_owners=accessible_owners, ) if qdrant_bbox is not None: chunk_bbox = qdrant_bbox @@ -711,7 +811,7 @@ async def get_pdf_preview(request: Request) -> JSONResponse: nextcloud_host = oauth_ctx.get("config", {}).get("nextcloud_host", "") if not nextcloud_host: - raise ValueError("Nextcloud host not configured") + raise ValueError(_NEXTCLOUD_HOST_NOT_CONFIGURED) # Use the user's stored app password for Nextcloud calls. # The OAuth bearer is only used to authenticate Astrolabe → MCP Server; diff --git a/nextcloud_mcp_server/auth/login_flow.py b/nextcloud_mcp_server/auth/login_flow.py index 95dfd3e1..fbe422f0 100644 --- a/nextcloud_mcp_server/auth/login_flow.py +++ b/nextcloud_mcp_server/auth/login_flow.py @@ -68,17 +68,27 @@ class LoginFlowV2Client: 2. Poll for completion to receive the app password Args: - nextcloud_host: Base URL of the Nextcloud instance + nextcloud_host: Base URL of the Nextcloud instance, reachable by this + server (may be an internal/Docker hostname, e.g. http://app:80). verify_ssl: SSL verification setting (True, False, or SSLContext) + public_host: Externally-reachable Nextcloud base URL for the + browser-facing login URL (e.g. https://cloud.example.com). When the + server talks to Nextcloud over an internal hostname, Nextcloud + builds the login URL with that internal host — unusable in the + user's browser. If set, the login URL's origin is rewritten to this + public host. When None, the login URL is returned unchanged + (correct when nextcloud_host is already the public URL). """ def __init__( self, nextcloud_host: str, verify_ssl: bool | ssl.SSLContext = True, + public_host: str | None = None, ): self.nextcloud_host = nextcloud_host.rstrip("/") self.verify_ssl = verify_ssl + self.public_host = public_host.rstrip("/") if public_host else None async def initiate( self, user_agent: str = "nextcloud-mcp-server" @@ -119,8 +129,23 @@ class LoginFlowV2Client: # so server-side polling works across Docker networks. poll_endpoint = self._rewrite_to_nextcloud_host(raw_poll_endpoint) + # The login URL is opened in the *user's browser*, so it must use + # the externally-reachable host. Nextcloud builds it from the + # request host (our internal nextcloud_host), so rewrite it to the + # public host when one is configured (internal != external). + login_url = data["login"] + if self.public_host: + rewritten = rewrite_url_origin(login_url, self.public_host) + if rewritten != login_url: + logger.debug( + "Rewrote Login Flow v2 login_url to public host: %s → %s", + login_url, + rewritten, + ) + login_url = rewritten + result = LoginFlowInitResponse( - login_url=data["login"], + login_url=login_url, poll_endpoint=poll_endpoint, poll_token=poll_data["token"], ) diff --git a/nextcloud_mcp_server/auth/provision_routes.py b/nextcloud_mcp_server/auth/provision_routes.py index 651c196a..b83525f1 100644 --- a/nextcloud_mcp_server/auth/provision_routes.py +++ b/nextcloud_mcp_server/auth/provision_routes.py @@ -74,6 +74,7 @@ async def _poll_and_store(provision_id: str) -> None: flow_client = LoginFlowV2Client( nextcloud_host=nextcloud_host, verify_ssl=get_nextcloud_ssl_verify(), + public_host=settings.nextcloud_public_issuer_url, ) poll_endpoint = session["poll_endpoint"] @@ -205,6 +206,7 @@ async def provision_page( flow_client = LoginFlowV2Client( nextcloud_host=nextcloud_host, verify_ssl=get_nextcloud_ssl_verify(), + public_host=settings.nextcloud_public_issuer_url, ) init_response = await flow_client.initiate() except Exception as e: diff --git a/nextcloud_mcp_server/auth/viz_routes.py b/nextcloud_mcp_server/auth/viz_routes.py index c1741ebd..849a348b 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -33,10 +33,12 @@ 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, ) +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, @@ -158,7 +160,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 +174,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 +201,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,15 +221,45 @@ 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 + # Sort by score, then cap to the same limit*2 over-fetch budget + # as the cross-app branch and the nc_semantic_search tool path + # (server/semantic.py). Without this, N doc_types each fetched + # at limit*2 would send N*limit*2 candidates into verify-on-read, + # multiplying the Nextcloud round-trip cost (and latency) by N. all_results.sort(key=lambda r: r.score, reverse=True) + all_results = all_results[: limit * 2] - # 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. + # Eviction of dropped (e.g. revoked-share) points runs INLINE here + # by design: this is a Starlette route with no access to the + # FastMCP lifespan-owned ``eviction_task_group`` that the + # nc_semantic_search tool path passes for fire-and-forget eviction. + # The visualization is an interactive, low-QPS endpoint, so blocking + # briefly on the Qdrant delete is acceptable. + with trace_operation("vector_viz.verify_on_read"): + verified_results, _dropped = await verify_search_results( + nc_client, all_results + ) + # Safe to log titles now: these passed verify-on-read (unverified + # titles are never logged — see the search algorithms). + if verified_results: + logger.debug( + "Top verified results: %s", + ", ".join( + f"{r.doc_type}_{r.id} (score={r.score:.3f}, title='{r.title}')" + for r in verified_results[:5] + ), + ) + search_results = verified_results[:limit] search_duration = time.perf_counter() - search_start # Store original scores and normalize for visualization @@ -636,6 +676,10 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: ) async with nc_client: + # Expand to owners who shared content with the caller so the cached + # chunk lookup can resolve cross-user SHARED FILES (gated per-file + # inside get_chunk_with_context). Same expansion as the search path. + accessible_owners = await list_accessible_owners(nc_client.sharing, user_id) chunk_context = await get_chunk_with_context( nc_client=nc_client, user_id=user_id, @@ -646,6 +690,7 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: chunk_index=chunk_index, total_chunks=total_chunks, context_chars=context_chars, + accessible_owners=accessible_owners, ) # Check if context expansion succeeded @@ -674,12 +719,16 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: chunk_bbox = None page_number = chunk_context.page_number if doc_type == "file": + # Reaching here means the file chunk context resolved, so access was + # already confirmed (get_chunk_with_context gates files by id); + # the bbox/page lookup uses the same owner scope for cross-user files. qdrant_bbox, qdrant_page = await get_chunk_bbox_and_page_from_qdrant( user_id=user_id, doc_id=doc_id, chunk_index=chunk_index, chunk_start=start, chunk_end=end, + accessible_owners=accessible_owners, ) if qdrant_bbox is not None: chunk_bbox = qdrant_bbox diff --git a/nextcloud_mcp_server/client/webdav.py b/nextcloud_mcp_server/client/webdav.py index f2068187..5fc63f09 100644 --- a/nextcloud_mcp_server/client/webdav.py +++ b/nextcloud_mcp_server/client/webdav.py @@ -1073,6 +1073,49 @@ class WebDAVClient(BaseNextcloudClient): limit=limit, ) + async def file_accessible_by_id(self, file_id: int) -> bool: + """ACL-aware access check for a file by its global Nextcloud file ID. + + Used by verify-on-read (ADR-019). Searches the authenticated user's + whole files tree — which *includes mounted shares* — via WebDAV SEARCH + (RFC 5323) filtered on ``oc:fileid``, returning True iff the user can + currently access the file. + + This is the only check that resolves shared files correctly: + + - :meth:`get_file_info` resolves a path under the caller's *own* root, + so it 404s on a file shared into the caller's account (Nextcloud + mounts received shares at the recipient's root by basename, a + different path than the owner indexed). + - The ``/remote.php/dav/meta/{id}/`` endpoint resolves only the user's + *own* storage, so it 404s on shared files too. + + SEARCH-by-fileid handles all cases: owned files, directly-shared files, + and files reachable via a shared parent folder (verified empirically). + + Args: + file_id: Nextcloud internal (global) file ID. + + Returns: + True if the user can access the file, False if it is not present + in their tree (not owned and not shared with them). + + Raises: + HTTPStatusError: On transport/server errors — callers treat these + as transient (keep the result), not as a definitive denial. + """ + where = ( + "" + f"{int(file_id)}" + ) + results = await self.search_files( + scope="", # user's whole files tree, incl. mounted shares + where_conditions=where, + properties=["fileid"], + limit=1, + ) + return len(results) > 0 + async def _get_file_info_by_id(self, file_id: int) -> Dict[str, Any]: """Get file information by Nextcloud file ID using WebDAV. diff --git a/nextcloud_mcp_server/search/access_filter.py b/nextcloud_mcp_server/search/access_filter.py new file mode 100644 index 00000000..70607afc --- /dev/null +++ b/nextcloud_mcp_server/search/access_filter.py @@ -0,0 +1,179 @@ +"""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. + +Operator note (existing data): a Qdrant ``owner_id`` field condition matches +nothing on points that lack the field, so documents indexed *before* this +change never surface to share recipients — only to their original indexer via +the legacy ``user_id`` branch. ACL-aware search is therefore effectively a +no-op for pre-existing data until each owner's scanner re-indexes it. Trigger a +re-index after deploying this feature if it should apply to already-indexed +content immediately. +""" + +from __future__ import annotations + +import logging +import time +from collections import OrderedDict +from typing import Any, Protocol + +from qdrant_client.models import Condition, 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 +# Cap the number of cached users so the process-global cache can't grow +# unboundedly in a long-running multi-user deployment (one entry per active +# user, never evicted otherwise). LRU eviction by insertion/access order via +# OrderedDict; the cap is generous relative to any realistic concurrent-user +# count, so steady state is effectively all-hit. +_OWNERS_CACHE_MAXSIZE = 1024 +_owners_cache: OrderedDict[str, tuple[float, list[str]]] = OrderedDict() + + +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.""" + + 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). + + 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. + + Granularity / over-fetch limitation (TODO, finer-grained filtering): this + expansion is *owner-level*, not *file-level*. If a prolific content creator + shares a single item with the querying user, that owner's whole indexed + corpus becomes a Qdrant candidate set for the querier even though only the + shared item is accessible. Verify-on-read correctly drops the inaccessible + "ghost" candidates, but because there is no second Qdrant pass to replenish, + a ``limit=N`` search can return fewer than N results when the over-fetch + buffer (2× in nc_semantic_search / viz_routes) is dominated by ghosts. A + per-file ownership index would remove this tension and is the natural + starting point for future work (intentionally out of scope here). + + 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: + _owners_cache.move_to_end(user_id) # mark as recently used (LRU) + 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) + 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] # don't cache failures — retry on the next search + + 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. The intent is + # "absent, not empty": a missing/blank `uid_owner` falls through to + # `owner`, and a non-string or empty result skips the (malformed) share. + owner = share.get("uid_owner") or share.get("owner") or None + if not isinstance(owner, str) or not owner: + continue + owners.add(owner) + + result = list(owners) + _owners_cache[user_id] = (now, result) + # Promote to the most-recently-used end. This is a no-op for a brand-new + # key (dict insertion already appends) but is needed when re-inserting an + # existing key after its TTL expired. + _owners_cache.move_to_end(user_id) + while len(_owners_cache) > _OWNERS_CACHE_MAXSIZE: + _owners_cache.popitem(last=False) # evict the least-recently-used entry + logger.debug( + "Accessible owners for user %s: %d entries (%d other owner(s))", + user_id, + len(result), + len(result) - 1, + ) + return list(result) + + +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`` (excluding + self) OR whose ``user_id`` equals ``user_id``. The ``user_id`` branch covers + *all* of the caller's own content — both new points (where + ``owner_id == user_id``) and legacy points indexed before ``owner_id`` + existed — so self is intentionally NOT repeated in the ``owner_id`` branch. + + Args: + user_id: Querying user (matched by the ``user_id`` branch, which is the + self-only 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] + # The ``user_id`` branch is always present and already covers self-owned + # content (new + legacy). The ``owner_id`` branch is added only for OTHER + # owners (share senders) — listing self there too would overlap the + # ``user_id`` branch for no benefit. When there are no other owners the + # ``owner_id`` branch is omitted entirely, so we never depend on + # ``MatchAny(any=[])`` matching nothing (not a documented Qdrant guarantee). + other_owners = [owner for owner in owners if owner != user_id] + conditions: list[Condition] = [ + FieldCondition(key="user_id", match=MatchValue(value=user_id)), + ] + if other_owners: + conditions.insert( + 0, FieldCondition(key="owner_id", match=MatchAny(any=other_owners)) + ) + return Filter(should=conditions) diff --git a/nextcloud_mcp_server/search/algorithms.py b/nextcloud_mcp_server/search/algorithms.py index ed1b1ddc..bbd22341 100644 --- a/nextcloud_mcp_server/search/algorithms.py +++ b/nextcloud_mcp_server/search/algorithms.py @@ -5,9 +5,10 @@ from abc import ABC, abstractmethod from dataclasses import dataclass from typing import Any, Protocol, runtime_checkable -from qdrant_client.models import FieldCondition, Filter, MatchValue, ScoredPoint +from qdrant_client.models import Filter, ScoredPoint from nextcloud_mcp_server.config import get_settings +from nextcloud_mcp_server.search.access_filter import build_ownership_filter from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client @@ -75,14 +76,24 @@ class NextcloudClientProtocol(Protocol): ... -async def get_indexed_doc_types(user_id: str) -> set[str]: +async def get_indexed_doc_types( + user_id: str, accessible_owners: list[str] | None = None +) -> set[str]: """Query Qdrant to get actually-indexed document types for a user. This enables search algorithms to check which document types are available before attempting to search/verify them, allowing graceful cross-app search. Args: - user_id: User ID to filter by + user_id: User ID to filter by. + accessible_owners: Owner UIDs the user may read (self + share senders), + as computed by ``access_filter.list_accessible_owners``. When + provided, doc-type discovery is ACL-aware and matches the same + ownership scope as the actual search (so a share recipient discovers + cross-user doc_types). When ``None`` (the default), discovery is + **self-only** — a recipient won't see doc_types that exist only in + another owner's shared content. Pass the expanded set for cross-user + discovery. Returns: Set of document type strings (e.g., {"note", "file", "calendar"}) @@ -106,7 +117,9 @@ async def get_indexed_doc_types(user_id: str) -> set[str]: scroll_filter=Filter( must=[ get_placeholder_filter(), # Exclude placeholders from doc_type discovery - FieldCondition(key="user_id", match=MatchValue(value=user_id)), + # ACL-aware ownership scope (owner_id IN owners OR legacy + # user_id == user_id), matching the real search filter. + build_ownership_filter(user_id, accessible_owners), ] ), limit=1000, # Sample size to discover types @@ -168,6 +181,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. @@ -271,6 +287,8 @@ class SearchAlgorithm(ABC): user_id: str, limit: int = 10, doc_type: str | None = None, + *, + accessible_owners: list[str] | None = None, **kwargs: Any, ) -> list[SearchResult]: """Execute search with the given parameters. @@ -280,6 +298,12 @@ class SearchAlgorithm(ABC): user_id: User ID for multi-tenant filtering limit: Maximum number of results to return doc_type: Optional document type filter (note, file, calendar, etc.) + accessible_owners: Owner UIDs the user is allowed to read (self plus + the owners of content shared with them), pre-computed from the + OCS Sharing API by the caller. Declared explicitly — rather than + buried in ``**kwargs`` — so a misspelled keyword is a type error + instead of a silent fall back to self-only scope. ``None`` means + self-only (``[user_id]``). **kwargs: Algorithm-specific parameters Returns: diff --git a/nextcloud_mcp_server/search/bm25_hybrid.py b/nextcloud_mcp_server/search/bm25_hybrid.py index 8f60c580..ba83d1ac 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, @@ -70,6 +71,8 @@ class BM25HybridSearchAlgorithm(SearchAlgorithm): user_id: str, limit: int = 10, doc_type: str | None = None, + *, + accessible_owners: list[str] | None = None, **kwargs: Any, ) -> list[SearchResult]: """ @@ -88,6 +91,9 @@ class BM25HybridSearchAlgorithm(SearchAlgorithm): user_id: User ID for filtering limit: Maximum results to return doc_type: Optional document type filter + accessible_owners: Owner UIDs the user can read (self + share + senders), pre-computed by the caller from the OCS Sharing API. + Defaults to ``[user_id]`` (self-only) when ``None``. **kwargs: Additional parameters (score_threshold override) Returns: @@ -131,10 +137,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 @@ -238,12 +241,10 @@ class BM25HybridSearchAlgorithm(SearchAlgorithm): if len(results) >= limit: break + # Log the count only — NOT titles. These results are unverified: with + # owner-level share expansion the candidate set can include other users' + # documents that verify-on-read will drop, so titles must not be logged + # until after verification (the verifying callers log verified titles). logger.info("Returning %s unverified results after deduplication", len(results)) - if results: - result_details = [ - f"{r.doc_type}_{r.id} (score={r.score:.3f}, title='{r.title}')" - for r in results[:5] # Show top 5 - ] - logger.debug("Top results: %s", ", ".join(result_details)) return results diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index aae29405..42eb8813 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -7,10 +7,12 @@ position markers for better visualization and understanding of search results. import logging from dataclasses import dataclass +from httpx import HTTPStatusError from qdrant_client.models import FieldCondition, Filter, MatchValue from nextcloud_mcp_server.client import NextcloudClient from nextcloud_mcp_server.config import get_settings +from nextcloud_mcp_server.search.access_filter import build_ownership_filter from nextcloud_mcp_server.utils.validation import is_valid_nextcloud_doc_id from nextcloud_mcp_server.vector.html_processor import html_to_markdown from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter @@ -20,7 +22,12 @@ logger = logging.getLogger(__name__) async def _get_chunk_from_qdrant( - user_id: str, doc_id: str, doc_type: str, chunk_start: int, chunk_end: int + user_id: str, + doc_id: str, + doc_type: str, + chunk_start: int, + chunk_end: int, + accessible_owners: list[str] | None = None, ) -> str | None: """Retrieve full chunk text from Qdrant payload. @@ -28,11 +35,15 @@ async def _get_chunk_from_qdrant( chunk content already stored in Qdrant. Args: - user_id: User ID who owns the document + user_id: Querying user. doc_id: Document ID doc_type: Document type (e.g., "note", "file") chunk_start: Character offset where chunk starts chunk_end: Character offset where chunk ends + accessible_owners: Owner UIDs the caller may read (self + share senders). + When None, the lookup is self-only. Callers must only pass an + expanded set after confirming the caller can access the document + (see ``get_chunk_with_context``) — the filter is owner-level. Returns: Full chunk text from Qdrant excerpt field, or None if not found @@ -46,7 +57,7 @@ async def _get_chunk_from_qdrant( collection_name=settings.get_collection_name(), scroll_filter=Filter( must=[ - FieldCondition(key="user_id", match=MatchValue(value=user_id)), + build_ownership_filter(user_id, accessible_owners), FieldCondition(key="doc_id", match=MatchValue(value=doc_id)), FieldCondition(key="doc_type", match=MatchValue(value=doc_type)), FieldCondition( @@ -93,17 +104,24 @@ async def _get_chunk_from_qdrant( async def _get_chunk_by_index_from_qdrant( - user_id: str, doc_id: str, doc_type: str, chunk_index: int + user_id: str, + doc_id: str, + doc_type: str, + chunk_index: int, + accessible_owners: list[str] | None = None, ) -> str | None: """Retrieve chunk text by chunk_index from Qdrant payload. Used to fetch adjacent chunks for context expansion. Args: - user_id: User ID who owns the document + user_id: Querying user. doc_id: Document ID doc_type: Document type (e.g., "note", "file") chunk_index: Zero-based chunk index in document + accessible_owners: Owner UIDs the caller may read; None ⇒ self-only. + Only pass an expanded set after a per-document access check (see + ``get_chunk_with_context``). Returns: Full chunk text from Qdrant excerpt field, or None if not found @@ -117,7 +135,7 @@ async def _get_chunk_by_index_from_qdrant( collection_name=settings.get_collection_name(), scroll_filter=Filter( must=[ - FieldCondition(key="user_id", match=MatchValue(value=user_id)), + build_ownership_filter(user_id, accessible_owners), FieldCondition(key="doc_id", match=MatchValue(value=doc_id)), FieldCondition(key="doc_type", match=MatchValue(value=doc_type)), FieldCondition( @@ -172,7 +190,13 @@ async def _get_deck_metadata_from_qdrant( qdrant_client = await get_qdrant_client() settings = get_settings() - # Query for any chunk of this card (we just need metadata) + # Query for any chunk of this card (we just need metadata). + # Intentionally self-only (raw user_id, not build_ownership_filter): + # deck cards are a documented cross-user gap — the Deck API is per-user, + # so cross-user deck context can't be fetched with the caller's + # credentials anyway (see the doc_type=="file"-only gate in + # get_chunk_with_context). Every other internal Qdrant lookup here is + # ACL-aware; this one is the deliberate exception. scroll_result = await qdrant_client.scroll( collection_name=settings.get_collection_name(), scroll_filter=Filter( @@ -217,6 +241,7 @@ async def get_chunk_bbox_and_page_from_qdrant( chunk_index: int | None, chunk_start: int, chunk_end: int, + accessible_owners: list[str] | None = None, ) -> tuple[list | None, int | None]: """Fetch chunk_bbox and page_number for a chunk from Qdrant payload. @@ -256,7 +281,7 @@ async def get_chunk_bbox_and_page_from_qdrant( must=[ get_placeholder_filter(), FieldCondition(key="doc_id", match=MatchValue(value=doc_id)), - FieldCondition(key="user_id", match=MatchValue(value=user_id)), + build_ownership_filter(user_id, accessible_owners), FieldCondition( key="chunk_index", match=MatchValue(value=chunk_index) ), @@ -273,7 +298,7 @@ async def get_chunk_bbox_and_page_from_qdrant( must=[ get_placeholder_filter(), FieldCondition(key="doc_id", match=MatchValue(value=doc_id)), - FieldCondition(key="user_id", match=MatchValue(value=user_id)), + build_ownership_filter(user_id, accessible_owners), FieldCondition( key="chunk_start_offset", match=MatchValue(value=chunk_start), @@ -352,6 +377,7 @@ async def get_chunk_with_context( chunk_index: int | None = None, total_chunks: int = 1, context_chars: int = 300, + accessible_owners: list[str] | None = None, ) -> ChunkContext | None: """Fetch chunk with surrounding context. @@ -361,7 +387,7 @@ async def get_chunk_with_context( Args: nc_client: Authenticated Nextcloud client - user_id: User ID who owns the document + user_id: Querying user. doc_id: Document ID (str — keyword-indexed in Qdrant payload) doc_type: Type of document ("note", "file", etc.) chunk_start: Character offset where chunk starts @@ -372,6 +398,10 @@ async def get_chunk_with_context( field). When None, falls back to the (chunk_start, chunk_end) lookup. total_chunks: Total number of chunks in document context_chars: Number of characters to include before/after chunk + accessible_owners: Owner UIDs the caller may read (self + share senders). + Used to support cross-user context for SHARED FILES only, and only + after a per-file access check (see ``lookup_owners`` below). For + non-file types the lookup stays self-only. Returns: ChunkContext with expanded context and markers, or None if document @@ -380,13 +410,53 @@ async def get_chunk_with_context( # doc_id is keyword-indexed in Qdrant as str — pass through verbatim # (no int coercion; producers always stringify on write). + # Determine the ownership scope for the Qdrant cached-chunk lookups. + # + # ``accessible_owners`` is OWNER-level (every owner who shared anything with + # the caller), so widening the lookup to it unconditionally would let a + # recipient of a single shared file read ANY of that owner's cached chunks + # by guessing doc_ids. We therefore honour it only for FILES, and only after + # confirming the caller can access THIS file by id (``file_accessible_by_id`` + # is cross-user-safe: a WebDAV SEARCH over the caller's whole tree incl. + # mounted shares). For per-user types (note/deck/news) there is no + # share-mounted by-id access via the caller's credentials, so the lookup + # stays self-only — cross-user context for those types is a known gap. + lookup_owners: list[str] | None = None # None ⇒ self-only + if doc_type == "file" and accessible_owners: + try: + if await nc_client.webdav.file_accessible_by_id(int(doc_id)): + lookup_owners = accessible_owners + else: + # Not owned and not shared with the caller → no access. Return + # early rather than falling back to a self-only lookup that + # would also miss (and so the result is the same None, but this + # is explicit and skips a pointless Qdrant round-trip). + logger.debug( + "File %s not accessible to %s; no cross-user chunk context", + doc_id, + user_id, + ) + return None + except (ValueError, TypeError): + # Non-numeric doc_id: shouldn't happen (endpoints validate), but + # degrade to self-only rather than raising. + logger.warning("Non-numeric file doc_id %r; using self-only scope", doc_id) + except HTTPStatusError as exc: + # Transient transport/server error — treat as inconclusive and fall + # back to self-only so the caller's own files still resolve. + logger.warning( + "file_accessible_by_id(%s) failed (%s); using self-only scope", + doc_id, + exc, + ) + # Try to get chunk from Qdrant (fast path). # Prefer chunk_index lookup (always-indexed field) when caller supplied it; # fall back to (chunk_start, chunk_end) lookup otherwise. chunk_text: str | None = None if chunk_index is not None: chunk_text = await _get_chunk_by_index_from_qdrant( - user_id, doc_id, doc_type, chunk_index + user_id, doc_id, doc_type, chunk_index, accessible_owners=lookup_owners ) # When chunk_index is supplied, the indexed lookup is canonical: both the # index path and the offset path query the same Qdrant collection, so an @@ -398,7 +468,12 @@ async def get_chunk_with_context( skip_offset_lookup = chunk_index is not None if chunk_text is None and not skip_offset_lookup: chunk_text = await _get_chunk_from_qdrant( - user_id, doc_id, doc_type, chunk_start, chunk_end + user_id, + doc_id, + doc_type, + chunk_start, + chunk_end, + accessible_owners=lookup_owners, ) if chunk_text: @@ -422,7 +497,11 @@ async def get_chunk_with_context( # Fetch previous chunk if not first chunk if chunk_index > 0: before_chunk = await _get_chunk_by_index_from_qdrant( - user_id, doc_id, doc_type, chunk_index - 1 + user_id, + doc_id, + doc_type, + chunk_index - 1, + accessible_owners=lookup_owners, ) if before_chunk: # Remove overlap: the last chunk_overlap chars of previous chunk @@ -443,7 +522,11 @@ async def get_chunk_with_context( # Fetch next chunk if not last chunk if chunk_index < total_chunks - 1: after_chunk = await _get_chunk_by_index_from_qdrant( - user_id, doc_id, doc_type, chunk_index + 1 + user_id, + doc_id, + doc_type, + chunk_index + 1, + accessible_owners=lookup_owners, ) if after_chunk: # Remove overlap: the first chunk_overlap chars of next chunk diff --git a/nextcloud_mcp_server/search/semantic.py b/nextcloud_mcp_server/search/semantic.py index 54465c03..bc09a7ae 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, @@ -48,6 +49,8 @@ class SemanticSearchAlgorithm(SearchAlgorithm): user_id: str, limit: int = 10, doc_type: str | None = None, + *, + accessible_owners: list[str] | None = None, **kwargs: Any, ) -> list[SearchResult]: """Execute semantic search using vector similarity. @@ -65,7 +68,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) + accessible_owners: Owner UIDs the user can read (self + share + senders), pre-computed by the caller from the OCS Sharing API. + Defaults to ``[user_id]`` (self-only) when ``None``. + **kwargs: + - score_threshold (float): override the instance default Returns: List of unverified SearchResult objects ranked by similarity score @@ -97,10 +104,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 @@ -159,12 +163,10 @@ class SemanticSearchAlgorithm(SearchAlgorithm): if len(results) >= limit: break + # Log the count only — NOT titles. These results are unverified: with + # owner-level share expansion the candidate set can include other users' + # documents that verify-on-read will drop, so titles must not be logged + # until after verification (the verifying callers log verified titles). logger.info("Returning %s unverified results after deduplication", len(results)) - if results: - result_details = [ - f"{r.doc_type}_{r.id} (score={r.score:.3f}, title='{r.title}')" - for r in results[:5] # Show top 5 - ] - logger.debug("Top results: %s", ", ".join(result_details)) return results diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index 480f3515..ee10d7d9 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -132,45 +132,55 @@ async def _verify_files( results: list[SearchResult], semaphore: anyio.Semaphore, ) -> set[str]: + """Return the doc_ids of file results this user may actually access. + + Verifies each file by its *global* Nextcloud file id via an ACL-aware + WebDAV SEARCH (``webdav.file_accessible_by_id``), NOT by path. This is the + ACL-aware-search fix: a file an owner shared with the querying user mounts + at a different path under each tree, so the previous path-based check + (``get_file_info``) produced false 404s and dropped legitimate shared-file + hits. Definitive 403/404 → inaccessible (dropped + scheduled for eviction + by the caller); transient/ambiguous errors → kept (fail-open). + """ # safe: cooperative concurrency, no lock needed (see verify_search_results) accessible: set[str] = set() async def check(result: SearchResult) -> None: doc_id = result.id # file_path is propagated from the Qdrant payload by the algorithm - # layer (bm25_hybrid.py / semantic.py). No extra Qdrant round-trip. + # layer (bm25_hybrid.py / semantic.py); kept here only for log context. file_path = (result.metadata or {}).get("path") - if not file_path: - # Cannot verify without a path; treat as accessible to avoid - # silently dropping legitimate results when payload is missing - # (legacy data, or a future doc_type that doesn't propagate path). + + # Verify by *global* file ID via an ACL-aware WebDAV SEARCH, NOT by + # path. For files the vector ``doc_id`` IS the Nextcloud file ID, and + # file_accessible_by_id searches the user's whole tree (incl. mounted + # shares), so a file an owner shared with this user verifies as + # accessible even though it lives at a different path under the owner's + # root. A path-based check (the old behaviour) would 404 on shared + # files mounted at the recipient's root by basename and silently drop + # legitimate ACL-aware-search results. + # + # Hoisted cast mirrors _verify_notes: a malformed id keeps the result + # (fail open) with a specific log line rather than a generic + # "unexpected error" from the catch-all below. + try: + file_id_int = int(doc_id) + except (TypeError, ValueError) as e: logger.warning( - "No file path in metadata for file_id %s; keeping result " - "(verification skipped)", + "Non-numeric file id %r (%s): %s; keeping result", doc_id, + file_path, + e, ) accessible.add(doc_id) return async with semaphore: try: - info = await client.webdav.get_file_info(file_path) - if info is None: - # Contract (see WebDAVClient.get_file_info docstring): - # `None` means a malformed PROPFIND response — an - # ambiguous state, not a definitive 404. Treat as - # transient and KEEP the result rather than evicting. - # Real 404s raise HTTPStatusError and land in the - # _is_definitive_404_or_403 branch below. - logger.warning( - "Malformed PROPFIND response verifying file %s (%s); " - "keeping result (ambiguous state, not a definitive 404)", - doc_id, - file_path, - ) + if await client.webdav.file_accessible_by_id(file_id_int): accessible.add(doc_id) - return - accessible.add(doc_id) + # else: definitively inaccessible (not owned, not shared) — + # drop and let the caller schedule eviction. except HTTPStatusError as e: if _is_definitive_404_or_403(e): return @@ -183,6 +193,8 @@ async def _verify_files( ) accessible.add(doc_id) except Exception as e: + # Network blip / unexpected WebDAV error — ambiguous, not a + # definitive denial. Keep the result; the next query re-verifies. logger.warning( "Unexpected error verifying file %s (%s): %s; keeping result", doc_id, @@ -584,6 +596,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/nextcloud_mcp_server/server/auth_tools.py b/nextcloud_mcp_server/server/auth_tools.py index 262e254f..8b5b22aa 100644 --- a/nextcloud_mcp_server/server/auth_tools.py +++ b/nextcloud_mcp_server/server/auth_tools.py @@ -113,6 +113,7 @@ def register_auth_tools(mcp: FastMCP) -> None: flow_client = LoginFlowV2Client( nextcloud_host=nextcloud_host, verify_ssl=get_nextcloud_ssl_verify(), + public_host=settings.nextcloud_public_issuer_url, ) init_response = await flow_client.initiate() except Exception as e: @@ -258,6 +259,7 @@ def register_auth_tools(mcp: FastMCP) -> None: flow_client = LoginFlowV2Client( nextcloud_host=nextcloud_host, verify_ssl=get_nextcloud_ssl_verify(), + public_host=settings.nextcloud_public_issuer_url, ) poll_result = await flow_client.poll( poll_endpoint=session["poll_endpoint"], @@ -431,6 +433,7 @@ def register_auth_tools(mcp: FastMCP) -> None: flow_client = LoginFlowV2Client( nextcloud_host=nextcloud_host, verify_ssl=get_nextcloud_ssl_verify(), + public_host=settings.nextcloud_public_issuer_url, ) init_response = await flow_client.initiate() except Exception as e: diff --git a/nextcloud_mcp_server/server/oauth_tools.py b/nextcloud_mcp_server/server/oauth_tools.py index 3d18e5e3..7e0ee70f 100644 --- a/nextcloud_mcp_server/server/oauth_tools.py +++ b/nextcloud_mcp_server/server/oauth_tools.py @@ -17,6 +17,7 @@ from pydantic import BaseModel, Field from nextcloud_mcp_server.auth import require_scopes from nextcloud_mcp_server.auth.astrolabe_client import AstrolabeClient +from nextcloud_mcp_server.auth.scope_authorization import invalidate_scope_cache from nextcloud_mcp_server.auth.storage import get_shared_storage from nextcloud_mcp_server.auth.token_broker import TokenBrokerService @@ -132,6 +133,26 @@ async def _get_provisioning_status(ctx: Context, user_id: str) -> ProvisioningSt ) storage = await get_shared_storage() + # Login Flow v2 app password stored directly in this server's storage — + # written by nc_auth_provision_access and the management app-password API, + # and the credential that require_provisioning / get_client actually use. + # Checked here so check_provisioning_status and revoke_nextcloud_access stay + # consistent with what actually grants tool access (the dual-store drift in + # the original code reported "not provisioned" while tools still worked). + app_pw = await storage.get_app_password_with_scopes(user_id) + if app_pw: + logger.debug( + " get_provisioning_status: app password (login-flow store) FOUND " + "for user_id=%s", + user_id, + ) + return ProvisioningStatus( + is_provisioned=True, + credential_type="app_password", + scopes=app_pw.get("scopes"), + flow_type="login_flow_v2", + ) + token_data = await storage.get_refresh_token(user_id) if not token_data: @@ -297,9 +318,30 @@ async def _revoke_nextcloud_access(ctx: Context, user_id: str) -> RevocationResu message="No Nextcloud access to revoke.", ) - # Initialize Token Broker to handle revocation storage = await get_shared_storage() + # App-password credential (Login Flow v2 / management API): there is no + # IdP token to revoke — removing it from this server's storage drops the + # server's access. Without this, revoke previously only handled refresh + # tokens and left the app password in place (tools kept working). + if status.credential_type == "app_password": + deleted = await storage.delete_app_password(user_id) + invalidate_scope_cache(user_id) + if deleted: + return RevocationResult( + success=True, + message=( + "Successfully revoked Nextcloud access (app password " + "removed). You can run provisioning again if needed." + ), + ) + return RevocationResult( + success=True, + message="No Nextcloud access to revoke.", + ) + + # Refresh-token credential: revoke via the Token Broker (IdP revocation). + # Get OAuth client credentials from storage client_creds = await storage.get_oauth_client() if not client_creds: diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index e8d1a396..ed651a29 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,8 +122,19 @@ 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 + # The nc_semantic_search tool deliberately uses BM25-hybrid (dense + + # sparse with RRF/DBSF fusion) as the single tool-layer algorithm. + # SemanticSearchAlgorithm is not dead code — it backs the dense-only + # option that the visualization/API surfaces expose explicitly + # (auth/viz_routes.py and api/visualization.py). Both algorithms take + # accessible_owners, so ACL-aware search works on every surface. search_algo = BM25HybridSearchAlgorithm( score_threshold=score_threshold, fusion=fusion ) @@ -153,6 +165,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 +190,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) @@ -221,6 +235,17 @@ def configure_semantic_tools(mcp: FastMCP): verified_chunk_count, dropped_count, ) + # Safe to log titles now: these results passed verify-on-read, so the + # caller is confirmed to have access (unverified titles were never + # logged — see the search algorithms). + if verified_results: + logger.debug( + "Top verified results: %s", + ", ".join( + f"{r.doc_type}_{r.id} (score={r.score:.3f}, title='{r.title}')" + for r in verified_results[:5] + ), + ) search_results = verified_results[:limit] # Convert SearchResult objects to SemanticSearchResult for response. @@ -314,6 +339,12 @@ def configure_semantic_tools(mcp: FastMCP): chunk_index=result.chunk_index, total_chunks=result.total_chunks, context_chars=context_chars, + # Forward the share-expanded owner set so context + # expansion works for shared files (the per-file + # file_accessible_by_id gate inside still enforces + # access). Without this the lookup stays self-only + # and silently falls back to the plain excerpt. + accessible_owners=accessible_owners, ) if chunk_context: diff --git a/nextcloud_mcp_server/vector/oauth_sync.py b/nextcloud_mcp_server/vector/oauth_sync.py index 6a44eebf..ad37556d 100644 --- a/nextcloud_mcp_server/vector/oauth_sync.py +++ b/nextcloud_mcp_server/vector/oauth_sync.py @@ -45,6 +45,38 @@ class NotProvisionedError(Exception): pass +# Process-wide app-password storage for the BasicAuth client path. +# +# get_user_client_basic_auth is on the search hot path (Unified Search and the +# /api/v1 viz endpoints call it per request). Creating a fresh +# RefreshTokenStorage and running ``initialize()`` — a full Alembic upgrade in +# a worker thread — on every call is both wasteful and unsafe: concurrent +# upgrades race on Alembic's non-thread-safe module-global EnvironmentContext +# proxy, surfacing as ``KeyError: 'script'``. Cache one initialized instance, +# guarded by a lock so the one-time migration runs exactly once. The lock is +# created lazily inside an async context (anyio primitives must not be built at +# import time — trio compatibility), mirroring vector/qdrant_client.py. +_basic_auth_storage: "RefreshTokenStorage | None" = None +_basic_auth_storage_lock: anyio.Lock | None = None + + +async def _get_initialized_basic_auth_storage() -> "RefreshTokenStorage": + """Return the process-wide, already-initialized app-password storage.""" + global _basic_auth_storage, _basic_auth_storage_lock + if _basic_auth_storage is not None: + return _basic_auth_storage + # Safe under cooperative scheduling: no await between the None-check and the + # assignment, so two coroutines cannot both create a lock. + if _basic_auth_storage_lock is None: + _basic_auth_storage_lock = anyio.Lock() + async with _basic_auth_storage_lock: + if _basic_auth_storage is None: + storage = RefreshTokenStorage.from_env() + await storage.initialize() + _basic_auth_storage = storage + return _basic_auth_storage + + @dataclass class UserSyncState: """State for a single user's scanner task.""" @@ -76,10 +108,11 @@ async def get_user_client_basic_auth( Raises: NotProvisionedError: If user has not provisioned an app password """ - # Get or create storage instance + # Get or create storage instance. Reuse a process-wide initialized instance + # rather than building one (and running an Alembic upgrade) per call — see + # _get_initialized_basic_auth_storage for why (hot path + Alembic race). if storage is None: - storage = RefreshTokenStorage.from_env() - await storage.initialize() + storage = await _get_initialized_basic_auth_storage() # Retrieve app password from local storage app_password = await storage.get_app_password(user_id) 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/qdrant_client.py b/nextcloud_mcp_server/vector/qdrant_client.py index 9f54d960..9f110e43 100644 --- a/nextcloud_mcp_server/vector/qdrant_client.py +++ b/nextcloud_mcp_server/vector/qdrant_client.py @@ -38,6 +38,15 @@ logger = logging.getLogger(__name__) _PAYLOAD_INDEX_FIELDS: dict[str, PayloadSchemaType] = { "doc_id": PayloadSchemaType.KEYWORD, "user_id": PayloadSchemaType.KEYWORD, + # owner_id is the ACL-aware filter field: every search applies + # MatchAny(key="owner_id", any=accessible_owners) (see + # search/access_filter.py). Without a keyword index Qdrant full-scans the + # collection to evaluate it — invisible at small scale, but a latency + # regression at tens of thousands of points and an HTTP 400 on Qdrant + # Cloud strict payload-validation mode. Mirrors the user_id treatment; + # _ensure_payload_indexes is idempotent so existing collections migrate + # at startup without operator intervention. + "owner_id": PayloadSchemaType.KEYWORD, "doc_type": PayloadSchemaType.KEYWORD, "is_placeholder": PayloadSchemaType.BOOL, "chunk_index": PayloadSchemaType.INTEGER, 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/integration/test_acl_owner_filter.py b/tests/integration/test_acl_owner_filter.py new file mode 100644 index 00000000..99a7cdab --- /dev/null +++ b/tests/integration/test_acl_owner_filter.py @@ -0,0 +1,212 @@ +"""ACL-aware ownership filter — deterministic, in-memory Qdrant. + +Proves the query-time ownership expansion added for ACL-aware search +(``search/access_filter.build_ownership_filter`` → +``SemanticSearchAlgorithm``): a user finds documents whose owner shared them +(``owner_id`` ∈ accessible_owners), does not find documents owned by users who +have not shared with them, and legacy points carrying only ``user_id`` stay +findable by their original indexer. + +This complements ``tests/unit/search/test_access_filter.py`` (filter +construction in isolation) by exercising the filter against a real Qdrant +engine through the actual search algorithm — no Nextcloud, no verification +layer, no background sync, so it is fast and deterministic. The full +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 + +from nextcloud_mcp_server.config import get_settings +from nextcloud_mcp_server.embedding import SimpleEmbeddingProvider +from nextcloud_mcp_server.search.algorithms import get_indexed_doc_types +from nextcloud_mcp_server.search.context import _get_chunk_by_index_from_qdrant +from nextcloud_mcp_server.search.semantic import SemanticSearchAlgorithm + +pytestmark = pytest.mark.integration + +# Same text for every point so cosine similarity to the query is ~identical: +# the *filter*, not the score, must decide what each user sees. +_DOC_TEXT = "Quarterly infrastructure budget planning and resource allocation" + +# (point_id, doc_id, owner_id, user_id) — owner_id=None mimics a legacy point +# indexed before the owner_id payload field existed. +_ALICE_FILE = (101, "101", "alice", "alice") +_CHARLIE_FILE = (102, "102", "charlie", "charlie") +_LEGACY_DAVE_FILE = (103, "103", None, "dave") + + +@pytest.fixture +async def seeded_collection(monkeypatch): + """In-memory Qdrant seeded with three file points, wired into the algorithm. + + Yields the ``SimpleEmbeddingProvider`` so the test can build a query vector + identical to the one the algorithm will generate. + """ + provider = SimpleEmbeddingProvider(dimension=384) + client = AsyncQdrantClient(":memory:") + collection = get_settings().get_collection_name() + + # The production collection uses a named "dense" vector (see + # vector/qdrant_client.py); the semantic algorithm queries using="dense". + await client.create_collection( + collection_name=collection, + vectors_config={"dense": VectorParams(size=384, distance=Distance.COSINE)}, + ) + + embedding = await provider.embed(_DOC_TEXT) + points = [] + for point_id, doc_id, owner_id, user_id in ( + _ALICE_FILE, + _CHARLIE_FILE, + _LEGACY_DAVE_FILE, + ): + payload = { + "doc_id": doc_id, + "doc_type": "file", + "user_id": user_id, + "is_placeholder": False, + "file_path": f"docs/{doc_id}.txt", + "title": f"file {doc_id}", + "excerpt": _DOC_TEXT, + "chunk_index": 0, + "total_chunks": 1, + } + # Legacy points carry no owner_id at all. + if owner_id is not None: + payload["owner_id"] = owner_id + points.append( + PointStruct(id=point_id, vector={"dense": embedding}, payload=payload) + ) + + await client.upsert(collection_name=collection, points=points, wait=True) + + # Point the algorithm at the in-memory client + deterministic embeddings. + monkeypatch.setattr( + "nextcloud_mcp_server.search.semantic.get_qdrant_client", + AsyncMock(return_value=client), + ) + monkeypatch.setattr( + "nextcloud_mcp_server.search.semantic.get_embedding_service", + lambda: provider, + ) + # get_indexed_doc_types reads the client from the algorithms module. + monkeypatch.setattr( + "nextcloud_mcp_server.search.algorithms.get_qdrant_client", + AsyncMock(return_value=client), + ) + # The cached-chunk lookups read the client from the context module. + monkeypatch.setattr( + "nextcloud_mcp_server.search.context.get_qdrant_client", + AsyncMock(return_value=client), + ) + + yield provider + + await client.close() + + +def _ids(results): + return {r.id for r in results} + + +async def test_shared_owner_is_visible_unshared_is_not(seeded_collection): + """Bob sees Alice's file (shared → owner in accessible_owners), not Charlie's.""" + algo = SemanticSearchAlgorithm(score_threshold=0.0) + + results = await algo.search( + query=_DOC_TEXT, + user_id="bob", + limit=10, + doc_type="file", + accessible_owners=["bob", "alice"], + ) + + found = _ids(results) + assert "101" in found, "Alice's shared file must be discoverable by Bob" + assert "102" not in found, "Charlie's unshared file must NOT be visible to Bob" + assert "103" not in found, "Legacy file owned by dave must NOT be visible to Bob" + + +async def test_no_shares_sees_only_own(seeded_collection): + """With no shares, Bob (who owns nothing here) gets nothing.""" + algo = SemanticSearchAlgorithm(score_threshold=0.0) + + results = await algo.search( + query=_DOC_TEXT, + user_id="bob", + limit=10, + doc_type="file", + accessible_owners=["bob"], + ) + + assert _ids(results) == set() + + +async def test_legacy_user_id_point_still_found_by_indexer(seeded_collection): + """A pre-owner_id point stays findable by its original indexer via the + legacy ``user_id`` OR-branch in build_ownership_filter.""" + algo = SemanticSearchAlgorithm(score_threshold=0.0) + + results = await algo.search( + query=_DOC_TEXT, + user_id="dave", + limit=10, + doc_type="file", + accessible_owners=["dave"], + ) + + found = _ids(results) + assert "103" in found, "dave must still find his own legacy (user_id-only) file" + assert "101" not in found + assert "102" not in found + + +async def test_owner_sees_own_new_style_point(seeded_collection): + """Alice finds her own file via the owner_id branch.""" + algo = SemanticSearchAlgorithm(score_threshold=0.0) + + results = await algo.search( + query=_DOC_TEXT, + user_id="alice", + limit=10, + doc_type="file", + accessible_owners=["alice"], + ) + + found = _ids(results) + assert "101" in found + assert "102" not in found + assert "103" not in found + + +async def test_get_indexed_doc_types_is_acl_aware(seeded_collection): + """get_indexed_doc_types respects the ownership scope: with the expanded + accessible_owners Bob discovers the shared "file" type, but self-only Bob + (who owns nothing here) discovers nothing — proving it is no longer + ACL-blind.""" + # ACL-aware: Bob can read Alice's shared file → discovers "file". + assert await get_indexed_doc_types("bob", accessible_owners=["bob", "alice"]) == { + "file" + } + # Self-only (default): Bob owns nothing here → discovers nothing. + assert await get_indexed_doc_types("bob") == set() + + +async def test_cached_chunk_lookup_is_acl_aware(seeded_collection): + """The cached-chunk Qdrant lookup honours accessible_owners: Bob retrieves + the excerpt of Alice's file point (owner_id=alice, chunk_index=0) when alice + is in his accessible owners, but not when scoped self-only. This is the + Qdrant-layer half of cross-user file chunk context (the per-file access + gate lives in get_chunk_with_context / file_accessible_by_id).""" + # Alice's seeded file point (_ALICE_FILE) carries excerpt=_DOC_TEXT at chunk 0. + text = await _get_chunk_by_index_from_qdrant( + "bob", "101", "file", 0, accessible_owners=["bob", "alice"] + ) + assert text == _DOC_TEXT + # Self-only Bob cannot reach Alice's cached chunk. + assert await _get_chunk_by_index_from_qdrant("bob", "101", "file", 0) is None diff --git a/tests/integration/test_acl_shared_search.py b/tests/integration/test_acl_shared_search.py new file mode 100644 index 00000000..e689c29d --- /dev/null +++ b/tests/integration/test_acl_shared_search.py @@ -0,0 +1,271 @@ +"""End-to-end ACL-aware semantic search against a real Nextcloud (PR #813). + +This is the card-120 acceptance criterion exercised across the *new* code +paths together: + +1. ``list_accessible_owners`` resolves the querying user's real OCS shares into + the set of owner UIDs they may search. +2. ``SemanticSearchAlgorithm`` applies the expanded ownership filter in Qdrant. +3. ``verify_search_results`` re-checks each hit against real Nextcloud + (ACL-aware, by global file id). + +Qdrant is in-memory and seeded directly with one point owned by *alice* — this +deliberately stands in for the background scanner (whose only relevant change +is writing ``owner_id`` into the payload, covered separately). Nextcloud itself +is real, so the share lookup (step 1) and the verification (step 3) exercise +the live OCS Sharing + WebDAV APIs. The result: bob, with whom alice shared the +file, finds it without having indexed anything; diana, with no share, does not. + +The pure-filter matrix lives in ``test_acl_owner_filter.py`` and the +verification layer in ``test_verify_on_read.py``; this test is the glue that +proves the real share → accessible_owners → filter → verify chain. +""" + +import os +import uuid +from unittest.mock import AsyncMock + +import pytest +from httpx import BasicAuth +from qdrant_client import AsyncQdrantClient +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 ( + clear_accessible_owners_cache, + list_accessible_owners, +) +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 + +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" + + +def _user_client(username: str, password: str) -> NextcloudClient: + return NextcloudClient( + base_url=os.environ["NEXTCLOUD_HOST"], + username=username, + auth=BasicAuth(username, password), + password=password, + ) + + +@pytest.fixture +async def acl_users(test_users_setup): + """alice (owner), bob (recipient), diana (no access) direct clients.""" + clients = { + name: _user_client(name, test_users_setup[name]["password"]) + for name in ("alice", "bob", "diana") + } + try: + yield clients + finally: + for c in clients.values(): + await c._client.aclose() + + +@pytest.fixture +async def shared_file(acl_users): + """alice creates a nested file and shares it with bob (not diana). + + Yields (file_id, owner_relative_path); cleans up the directory after. + """ + alice = acl_users["alice"] + suffix = uuid.uuid4().hex[:8] + test_dir = f"acl_e2e_{suffix}" + nested = f"{test_dir}/reports" + path = f"{nested}/budget.txt" + + await alice.webdav.create_directory(test_dir) + await alice.webdav.create_directory(nested) + await alice.webdav.write_file(path, _DOC_TEXT.encode(), "text/plain") + file_id = (await alice.webdav.get_file_info(path))["id"] + await alice.sharing.create_share( + path=f"/{path}", share_with="bob", share_type=0, permissions=1 + ) + try: + yield file_id, path + finally: + await alice.webdav.delete_resource(test_dir) + + +@pytest.fixture +async def seeded_semantic(monkeypatch, shared_file): + """In-memory Qdrant carrying alice's file point, wired into the algorithm. + + Stands in for the background scanner: the point carries ``owner_id=alice`` + exactly as the scanner now writes it. + """ + file_id, path = shared_file + provider = SimpleEmbeddingProvider(dimension=384) + client = AsyncQdrantClient(":memory:") + collection = get_settings().get_collection_name() + await client.create_collection( + collection_name=collection, + vectors_config={"dense": VectorParams(size=384, distance=Distance.COSINE)}, + ) + await client.upsert( + collection_name=collection, + points=[ + PointStruct( + id=int(file_id), + vector={"dense": await provider.embed(_DOC_TEXT)}, + payload={ + "doc_id": str(file_id), + "doc_type": "file", + "owner_id": "alice", + "user_id": "alice", + "is_placeholder": False, + "file_path": path, + "title": "budget.txt", + "excerpt": _DOC_TEXT, + "chunk_index": 0, + "total_chunks": 1, + }, + ) + ], + wait=True, + ) + + monkeypatch.setattr( + "nextcloud_mcp_server.search.semantic.get_qdrant_client", + AsyncMock(return_value=client), + ) + monkeypatch.setattr( + "nextcloud_mcp_server.search.semantic.get_embedding_service", + lambda: provider, + ) + # The cached-chunk lookups (get_chunk_with_context) read the client from the + # context module — point it at the same in-memory Qdrant. + monkeypatch.setattr( + "nextcloud_mcp_server.search.context.get_qdrant_client", + AsyncMock(return_value=client), + ) + yield file_id + await client.close() + + +async def _search_as(user_client, file_id_unused) -> list: + """Run the full new chain (share lookup → filter → verify) as a user.""" + accessible_owners = await list_accessible_owners( + user_client.sharing, user_client.username + ) + algo = SemanticSearchAlgorithm(score_threshold=0.0) + unverified = await algo.search( + query=_DOC_TEXT, + user_id=user_client.username, + limit=10, + doc_type="file", + accessible_owners=accessible_owners, + ) + kept, _dropped = await verify_search_results(user_client, unverified) + return kept + + +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.""" + 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") + assert "alice" in owners, "OCS shared-with-me must surface alice as an owner" + + kept = await _search_as(acl_users["bob"], file_id) + + assert [r.id for r in kept] == [str(file_id)], ( + "bob must find alice's shared file via semantic search" + ) + + +async def test_non_recipient_does_not_find_file(acl_users, seeded_semantic): + """Diana, with no share, never sees the file: her accessible-owners set + excludes alice, so the ownership filter drops the point before verification.""" + owners = await list_accessible_owners(acl_users["diana"].sharing, "diana") + assert "alice" not in owners + + kept = await _search_as(acl_users["diana"], seeded_semantic) + + assert kept == [], "diana (no share) must not find alice's file" + + +async def test_file_accessible_by_id_resolves_shares(acl_users, shared_file): + """Lock the verify-on-read contract directly on ``file_accessible_by_id``. + + The WebDAV SEARCH-by-fileid with ``scope=""`` must resolve a file that the + caller does NOT own but which is shared with them. This is the exact check + verify-on-read depends on for shared, nested files; a Nextcloud change to + how ``scope=""`` is interpreted would otherwise silently break ACL-aware + verification. The file lives in a subfolder, so a path-based check would + 404 for the recipient — only the by-id SEARCH gets it right. + """ + file_id, _path = shared_file + fid = int(file_id) + + # Owner and share recipient can both reach it... + assert await acl_users["alice"].webdav.file_accessible_by_id(fid) is True + assert await acl_users["bob"].webdav.file_accessible_by_id(fid) is True + # ...the non-recipient cannot. + assert await acl_users["diana"].webdav.file_accessible_by_id(fid) is False + + +async def test_cross_user_file_chunk_context(acl_users, seeded_semantic): + """End-to-end cross-user FILE chunk context: Bob (a share recipient) gets + Alice's cached chunk text, Diana (no share) gets None. + + Exercises the full secure path: the ACL-aware Qdrant cached-chunk lookup + (owner_id=alice surfaces for Bob) gated by a real per-file + ``file_accessible_by_id`` check against live Nextcloud. Diana fails the gate + and is denied even though the chunk is cached. Per-user types are covered by + the self-only behaviour elsewhere — this is the file path the feature adds. + """ + file_id = seeded_semantic + bob = acl_users["bob"] + diana = acl_users["diana"] + + bob_owners = await list_accessible_owners(bob.sharing, "bob") + assert "alice" in bob_owners + + ctx = await get_chunk_with_context( + nc_client=bob, + user_id="bob", + doc_id=str(file_id), + doc_type="file", + chunk_start=0, + chunk_end=len(_DOC_TEXT), + chunk_index=0, + total_chunks=1, + accessible_owners=bob_owners, + ) + assert ctx is not None, "Bob (share recipient) must get Alice's cached chunk" + assert ctx.chunk_text == _DOC_TEXT + + # Diana has no share → per-file gate denies even though the chunk is cached. + diana_owners = await list_accessible_owners(diana.sharing, "diana") + denied = await get_chunk_with_context( + nc_client=diana, + user_id="diana", + doc_id=str(file_id), + doc_type="file", + chunk_start=0, + chunk_end=len(_DOC_TEXT), + chunk_index=0, + total_chunks=1, + accessible_owners=diana_owners, + ) + assert denied is None, "Diana (no share) must not get cross-user chunk context" diff --git a/tests/integration/test_astrolabe_chunk_context.py b/tests/integration/test_astrolabe_chunk_context.py index a58e9ae8..0fcc259b 100644 --- a/tests/integration/test_astrolabe_chunk_context.py +++ b/tests/integration/test_astrolabe_chunk_context.py @@ -133,7 +133,6 @@ async def test_chunk_context_endpoint_uses_app_password( try: await login_to_nextcloud(page, username, password) auth_result = await complete_astrolabe_authorization(page, username, password) - assert auth_result["step1"], "OAuth authorization did not complete" assert auth_result["step2"], "App password provisioning did not complete" auth_header = _build_basic_auth_header(username, password) diff --git a/tests/integration/test_astrolabe_login_flow_provisioning.py b/tests/integration/test_astrolabe_login_flow_provisioning.py deleted file mode 100644 index 15d2abbb..00000000 --- a/tests/integration/test_astrolabe_login_flow_provisioning.py +++ /dev/null @@ -1,139 +0,0 @@ -"""Integration test for Astrolabe's "Enable Semantic Search" OAuth flow on -the `mcp-login-flow` profile. - -Cross-system interface test. Brings together Astrolabe (Nextcloud PHP app -installed at container start by ``app-hooks/post-installation``) with the -``mcp-login-flow`` MCP server over OAuth + the management API. Mirrors -the production-shaped flow that PR #773's recent -`ALLOWED_MGMT_CLIENT` ↔ `astrolabeMcpClientOAuth00000000000` drift was -masking — every management API call from Astrolabe (e.g. -``/api/v1/users/admin/session``) was returning 401 because the -real-deployment client id was not in the test-fixture allowlist, so the -Astrolabe settings page never updated to reflect a successful -authorization. - -This test is **regression coverage** for that class of drift. If the -Astrolabe client id ever falls out of `mcp-login-flow`'s -``ALLOWED_MGMT_CLIENT`` again, the post-redirect assertions here will -fail because the page state stays on ``oauth-required.php``. - -Requires the login-flow stack to be running: - - MCP_SERVER_URL=http://mcp-login-flow:8004 \\ - docker compose --profile login-flow up -d app db mcp-login-flow - -The ``app-hooks/before-starting/26-configure-astrolabe-oauth.sh`` hook -creates the OAuth client with the production-shaped id -``astrolabeMcpClientOAuth00000000000`` automatically when -``MCP_SERVER_URL`` is set, so no fixture-level OIDC client creation is -needed here. -""" - -import logging -import os -import re - -import pytest -from playwright.async_api import Page - -# Reuse helpers from the multi-user-basic Astrolabe test for login + nav. -from tests.integration.test_astrolabe_multi_user_background_sync import ( - login_to_nextcloud, - navigate_to_astrolabe_settings, -) - -logger = logging.getLogger(__name__) - -pytestmark = [pytest.mark.integration, pytest.mark.login_flow] - -NEXTCLOUD_URL = "http://localhost:8080" -ASTROLABE_SETTINGS_URL = f"{NEXTCLOUD_URL}/settings/user/astrolabe" - - -async def _click_enable_semantic_search(page: Page) -> None: - """Click the "Enable Semantic Search" OAuth link on the - ``oauth-required.php`` template that login-flow mode renders to a - not-yet-authorized user. - - Astrolabe's own e2e helper (``third_party/astrolabe/tests/e2e/helpers/ - authorize.ts``) targets the same link by accessible name. - """ - enable_link = page.get_by_role("link", name="Enable Semantic Search") - await enable_link.wait_for(state="visible", timeout=10_000) - logger.info("Clicking 'Enable Semantic Search' OAuth link") - await enable_link.click() - - -async def _grant_oidc_consent(page: Page) -> None: - """Click "Allow" on the Nextcloud OIDC consent screen, if shown. - - Nextcloud may auto-redirect for already-trusted clients, in which - case the consent button never appears — that's not an error. - """ - allow_button = page.get_by_role("button", name=re.compile(r"^allow$", re.I)) - try: - await allow_button.wait_for(state="visible", timeout=10_000) - logger.info("Clicking 'Allow' on OIDC consent") - await allow_button.click(force=True) - except Exception: - logger.info( - "OIDC consent screen not visible — assuming auto-grant for " - "already-trusted client" - ) - - -@pytest.mark.timeout(180) -async def test_enable_semantic_search_completes_oauth_for_login_flow(browser): - """Click the "Enable Semantic Search" link, grant consent, and assert - the post-redirect page reflects a completed authorization. - - The success criterion is intentionally negative: after the OAuth - flow, the original "Enable Semantic Search" link must be gone. If - Astrolabe's management API call is rejected by the MCP server (HTTP - 401, the original bug), the page falls back to the same - ``oauth-required.php`` template and the link reappears — making this - test the canary for the drift class. - """ - admin_password = os.getenv("NEXTCLOUD_PASSWORD") - if admin_password is None: - raise RuntimeError("NEXTCLOUD_PASSWORD must be set") - page = await browser.new_page() - try: - await login_to_nextcloud(page, "admin", admin_password) - await navigate_to_astrolabe_settings(page) - - # Sanity-check we're on the not-yet-authorized template. - enable_link = page.get_by_role("link", name="Enable Semantic Search") - if await enable_link.count() == 0: - pytest.skip( - "Astrolabe is already authorized for admin (oauth-required.php " - "not rendered). Reset by clearing the user's OAuth tokens " - "before re-running this test." - ) - - await _click_enable_semantic_search(page) - await _grant_oidc_consent(page) - - # OAuth callback returns to /apps/astrolabe/oauth/callback then the - # controller redirects to /settings/user/astrolabe. - await page.wait_for_url(re.compile(r"/settings/user/astrolabe"), timeout=30_000) - await page.wait_for_load_state("networkidle", timeout=15_000) - - # Regression assertion for the ALLOWED_MGMT_CLIENT drift bug: - # the page must have moved past oauth-required.php. If - # Astrolabe's management API call to /api/v1/users/{id}/session - # is rejected (401), the session lookup falls back to "no token", - # and the same oauth-required.php template re-renders with the - # link still present. - post_auth_count = await page.get_by_role( - "link", name="Enable Semantic Search" - ).count() - assert post_auth_count == 0, ( - "'Enable Semantic Search' link still visible after completing " - "OAuth flow — Astrolabe could not read the user's session from " - "the MCP server. Most likely cause: " - "`astrolabeMcpClientOAuth00000000000` missing from " - "`ALLOWED_MGMT_CLIENT` on `mcp-login-flow`." - ) - finally: - await page.close() diff --git a/tests/integration/test_astrolabe_multi_user_background_sync.py b/tests/integration/test_astrolabe_multi_user_background_sync.py index e25058e9..eeb32c23 100644 --- a/tests/integration/test_astrolabe_multi_user_background_sync.py +++ b/tests/integration/test_astrolabe_multi_user_background_sync.py @@ -7,19 +7,19 @@ lives in a separate repository (https://github.com/cbcoutinho/astrolabe). This test verifies that multiple users can independently: 1. Log in to Nextcloud -2. Generate an app password in Security settings -3. Enter the app password in Astrolabe personal settings -4. Enable background sync for the mcp-multi-user-basic service -5. Verify app password is stored in the database +2. Click the one-click "Enable background indexing" opt-in in Astrolabe settings +3. Have a dedicated app password minted from their session and handed to the + MCP server (core/getapppassword — no Security-settings step, no copy-paste) +4. Verify the app password is stored in the database -Tests the complete app password provisioning flow: -user login → Security settings → app password generation → Astrolabe settings → -app password entry → background sync activation → database verification. +Tests the one-click background-indexing provisioning flow: +user login → Astrolabe settings → Enable background indexing → session app +password minted + forwarded to MCP → background sync active → DB verification. """ import logging -import re import subprocess +import tempfile import anyio import pytest @@ -91,621 +91,64 @@ async def navigate_to_astrolabe_settings(page: Page): logger.info("✓ Successfully loaded Astrolabe settings page") -async def authorize_search_access(page: Page, username: str) -> bool: - """Complete Step 1: OAuth Authorization for Astrolabe. +async def enable_background_sync(page: Page, username: str) -> bool: + """Provision background indexing via the one-click opt-in button. - Handles the OAuth flow: - 1. Check if already authorized (Step 1 shows "Complete") - 2. Click "Authorize" link - 3. Handle Nextcloud OIDC consent screen - 4. Wait for redirect back to Astrolabe settings - 5. Verify "Complete" badge appears on Step 1 + The refactored settings page mints a dedicated app password from the + current Nextcloud session (core/getapppassword) and hands it to the MCP + server — there is no app-password generation in Security settings and no + copy-paste. Idempotent: if already enabled (the revoke form is shown + instead of the enable button), returns True without acting. Args: - page: Playwright page instance (must be on Astrolabe settings page) - username: Username for logging - - Returns: - True if authorization completed successfully - """ - nextcloud_url = "http://localhost:8080" - - logger.info("Authorizing search access (Step 1) for %s...", username) - - # Check if already on Astrolabe settings page, if not navigate there - if "/settings/user/astrolabe" not in page.url: - await navigate_to_astrolabe_settings(page) - - # Wait for page to fully render - await anyio.sleep(1) - - # Check if already authorized (either "Active" badge or Step 1 "Complete" badge) - try: - # Check for "Active" badge (fully configured state) - active_badge = page.get_by_text("Active", exact=True) - if await active_badge.count() > 0 and await active_badge.is_visible(): - logger.info("✓ Already fully authorized for %s (Active badge)", username) - return True - except Exception: - pass - - try: - step1_section = page.locator('h4:has-text("Step 1")') - if await step1_section.count() > 0: - # Look for "Complete" text in the Step 1 section's parent - step1_parent = step1_section.locator("..") - complete_badge = step1_parent.get_by_text("Complete", exact=True) - if await complete_badge.count() > 0 and await complete_badge.is_visible(): - logger.info("✓ Step 1 already complete for %s", username) - return True - except Exception: - pass - - # Find and click the "Authorize" button - authorize_button = page.locator('a.button.primary:has-text("Authorize")') - - try: - await authorize_button.wait_for(timeout=5000, state="visible") - logger.info("Found Authorize button for %s", username) - except Exception: - # Take screenshot for debugging - screenshot_path = f"/tmp/astrolabe_no_authorize_button_{username}.png" - await page.screenshot(path=screenshot_path) - logger.error( - "Could not find Authorize button for %s. Screenshot: %s", - username, - screenshot_path, - ) - raise ValueError(f"Authorize button not found for {username}") - - # Click the Authorize button - this will redirect to OAuth provider - # Use force=True to bypass stability check which can timeout due to CSS transitions - await authorize_button.click(force=True) - logger.info("Clicked Authorize button for %s", username) - - # Wait for OAuth redirect to complete - await page.wait_for_load_state("networkidle", timeout=30000) - logger.info("After networkidle, current URL: %s", page.url) - - # Take screenshot to see current state - await page.screenshot(path=f"/tmp/astrolabe_after_authorize_{username}.png") - logger.info("Screenshot saved: /tmp/astrolabe_after_authorize_%s.png", username) - - # Handle OIDC consent screen if present - consent_handled = await _handle_oauth_consent_screen(page, username) - if consent_handled: - logger.info("✓ OAuth consent granted for %s", username) - else: - logger.info( - "No consent screen required for %s (may be previously authorized)", username - ) - - # Wait for redirect back to Astrolabe settings - # The OAuth callback will redirect back to /settings/user/astrolabe - try: - await page.wait_for_url( - f"**{nextcloud_url}/settings/user/astrolabe**", timeout=30000 - ) - logger.info("Redirected back to Astrolabe settings for %s", username) - except Exception: - # Check if we're already on settings page - if "/settings/user/astrolabe" not in page.url: - logger.warning( - "Not redirected to Astrolabe settings, current URL: %s", page.url - ) - # Navigate manually - await page.goto( - f"{nextcloud_url}/settings/user/astrolabe", wait_until="networkidle" - ) - - # Wait for page to reload and render - await anyio.sleep(2) - - # Verify authorization completed - check for various success indicators - # When fully configured, shows "Active" badge; when only Step 1 done, shows "Complete" - try: - # First check if "Active" badge is shown (fully configured state) - active_badge = page.get_by_text("Active", exact=True) - if await active_badge.count() > 0 and await active_badge.is_visible(): - logger.info( - "✓ OAuth authorization complete for %s (Active badge)", username - ) - return True - except Exception: - pass - - try: - # Check for Step 1 "Complete" badge (partial configuration) - step1_section = page.locator('h4:has-text("Step 1")') - if await step1_section.count() > 0: - step1_parent = step1_section.locator("..") - complete_badge = step1_parent.get_by_text("Complete", exact=True) - await complete_badge.wait_for(timeout=5000, state="visible") - logger.info("✓ Step 1 OAuth authorization complete for %s", username) - return True - except Exception: - pass - - # Neither badge found - authorization failed - screenshot_path = f"/tmp/astrolabe_step1_not_complete_{username}.png" - await page.screenshot(path=screenshot_path) - logger.error( - "Authorization badge not visible for %s. Screenshot: %s", - username, - screenshot_path, - ) - raise ValueError(f"OAuth authorization did not complete for {username}") - - -async def _handle_oauth_consent_screen(page: Page, username: str) -> bool: - """Handle the OIDC consent screen during OAuth flow. - - Reuses the proven pattern from tests/conftest.py. - - Args: - page: Playwright page instance - username: Username for logging - - Returns: - True if consent was handled, False if no consent screen was found - """ - try: - logger.info("Checking for consent screen at URL: %s", page.url) - - # Check if consent screen is present - try multiple selectors - # The consent screen may be #oidc-consent or use a different format - consent_div = await page.query_selector("#oidc-consent") - - if consent_div: - logger.info("Consent screen detected via #oidc-consent for %s", username) - # Get consent screen data attributes for logging - client_name = await consent_div.get_attribute("data-client-name") - scopes_attr = await consent_div.get_attribute("data-scopes") - logger.info(" Client: %s", client_name) - logger.info(" Requested scopes: %s", scopes_attr) - else: - # Check for Allow button directly (different consent screen format) - allow_button = page.locator('button:has-text("Allow")') - if await allow_button.count() > 0: - logger.info("Consent screen detected via Allow button for %s", username) - else: - logger.info("No consent screen found for %s at %s", username, page.url) - await page.screenshot(path=f"/tmp/no_consent_screen_{username}.png") - logger.info("Screenshot: /tmp/no_consent_screen_%s.png", username) - return False - - # Wait for Vue.js to render the Allow button - try: - await page.wait_for_selector('button:has-text("Allow")', timeout=10000) - logger.info(" Allow button rendered by Vue.js") - except Exception as e: - screenshot_path = f"/tmp/consent_no_allow_button_{username}.png" - await page.screenshot(path=screenshot_path) - logger.error(" Timeout waiting for Allow button: %s", e) - raise - - # Check all scope checkboxes - scope_checkboxes = await page.query_selector_all('input[type="checkbox"]') - if scope_checkboxes: - logger.info(" Found %s scope checkboxes", len(scope_checkboxes)) - for i, checkbox in enumerate(scope_checkboxes): - is_checked = await checkbox.is_checked() - is_disabled = await checkbox.is_disabled() - if not is_checked and not is_disabled: - await checkbox.check() - logger.info(" ✓ Checked scope checkbox %s", i + 1) - - # Click the Allow button using JavaScript (handles viewport issues) - allow_button_locator = page.locator('button:has-text("Allow")') - - # Debug: take screenshot before clicking Allow - await page.screenshot(path=f"/tmp/consent_before_allow_{username}.png") - logger.info( - " Screenshot before Allow: /tmp/consent_before_allow_%s.png", username - ) - - button_count = await allow_button_locator.count() - logger.info(" Found %s Allow button(s)", button_count) - - if button_count > 0: - current_url = page.url - logger.info(" Current URL: %s", current_url) - logger.info(" Clicking Allow button for %s...", username) - - # Use JavaScript click to handle consent buttons (proven pattern from conftest.py) - # This is more reliable than Playwright's click for Vue.js rendered buttons - await page.evaluate( - """ - const buttons = document.querySelectorAll('button'); - for (const btn of buttons) { - if (btn.textContent.trim() === 'Allow') { - btn.click(); - break; - } - } - """ - ) - - # Wait for URL to change (Vue.js uses window.location.href after fetch) - # networkidle doesn't detect fetch-based redirects - try: - await page.wait_for_url( - lambda url: url != current_url, - timeout=30000, - ) - logger.info(" URL changed to: %s", page.url) - except Exception as wait_error: - # If URL didn't change, check console for errors - logger.warning(" URL didn't change after click: %s", wait_error) - await page.screenshot(path=f"/tmp/consent_after_allow_{username}.png") - - # Try alternative: manually POST consent and navigate - logger.info(" Trying manual consent submission...") - try: - redirect_url = await page.evaluate( - """ - async () => { - const selectedScopes = Array.from(document.querySelectorAll('input[type="checkbox"]:checked')) - .map(cb => cb.value).join(' '); - - const response = await fetch('/index.php/apps/oidc/consent/grant', { - method: 'POST', - headers: { - 'Content-Type': 'application/x-www-form-urlencoded', - 'requesttoken': OC.requestToken, - }, - body: 'scopes=' + encodeURIComponent(selectedScopes), - redirect: 'follow', - }); - - return response.url || '/index.php/apps/oidc/authorize'; - } - """ - ) - logger.info(" Manual consent returned URL: %s", redirect_url) - await page.goto(redirect_url, wait_until="networkidle") - except Exception as manual_error: - logger.error(" Manual consent also failed: %s", manual_error) - raise - - await page.screenshot(path=f"/tmp/consent_after_allow_{username}.png") - logger.info(" Consent granted for %s", username) - return True - else: - logger.error(" Allow button not found for %s", username) - return False - - except Exception as e: - logger.error("Error handling consent screen for %s: %s", username, e) - raise - - -async def generate_app_password( - page: Page, username: str, app_name: str = "Astrolabe Background Sync" -) -> str: - """Generate an app password in Nextcloud Security settings. - - Args: - page: Playwright page instance (must be authenticated) + page: Playwright page instance (must be logged in) username: Username (for logging) - app_name: Name for the app password Returns: - The generated app password string + True once background indexing is enabled. """ - logger.info("Generating app password for %s...", username) - - nextcloud_url = "http://localhost:8080" - - # Navigate to Security settings - await page.goto(f"{nextcloud_url}/settings/user/security", wait_until="networkidle") - logger.info("Navigated to Security settings") - - # Fill the app password input field (selector confirmed via Playwright MCP) - app_password_input = page.locator('input[placeholder="App name"]') - await app_password_input.fill(app_name) - logger.info("Entered app name: %s", app_name) - - # Wait for Vue.js to react and enable the button (needs 1 second, not 0.5) - await anyio.sleep(1.0) - logger.info("Waited for Vue.js to process input and enable button") - - # Click the create button - use force=True to bypass stability check (CSS transitions) - create_button = page.locator( - 'button[type="submit"]:has-text("Create new app password")' - ) - try: - await create_button.click(force=True, timeout=10000) - except Exception: - # Fallback: JavaScript click - logger.info("Using JavaScript click for create button...") - await page.evaluate( - """ - const btn = document.querySelector('button[type="submit"]'); - if (btn) btn.click(); - """ - ) - logger.info("Clicked create app password button") - - # Wait for app password to be generated and displayed in the dialog - await anyio.sleep(3) # Give it more time to generate and display - - # Debug screenshot after clicking create - await page.screenshot(path=f"/tmp/app_password_after_create_{username}.png") - logger.info( - "Screenshot after create: /tmp/app_password_after_create_%s.png", username - ) - - # Find the Login input field which should have the username value - # Then find the Password input field which is in the same form - app_password = None - try: - # Wait for heading "New app password" to appear - await page.wait_for_selector('text="New app password"', timeout=10000) - logger.info("App password dialog appeared with heading") - - # Get all visible input elements - all_inputs = await page.locator('input[type="text"]').all() - logger.info("Found %s text input elements", len(all_inputs)) - - # Check each input to find the one with the app password - for idx, input_elem in enumerate(all_inputs): - try: - value = await input_elem.input_value() - if value and "-" in value and len(value) > 20: - app_password = value.strip() - logger.info( - "Found app password in input %s: '%s' (length: %s)", - idx, - app_password, - len(app_password), - ) - break - except Exception as e: - logger.debug("Could not get value from input %s: %s", idx, e) - continue - - except Exception as e: - logger.error("Failed to find app password dialog or extract password: %s", e) - - if not app_password: - # Take screenshot for debugging - screenshot_path = f"/tmp/app_password_generation_{username}.png" - await page.screenshot(path=screenshot_path) - raise ValueError( - f"Could not find generated app password. Screenshot: {screenshot_path}" - ) - - # Validate password format before returning - - if not re.match( - r"^[a-zA-Z0-9]{5}-[a-zA-Z0-9]{5}-[a-zA-Z0-9]{5}-[a-zA-Z0-9]{5}-[a-zA-Z0-9]{5}$", - app_password, - ): - logger.error( - "Extracted password does not match expected format: '%s'", app_password - ) - logger.error("Password repr: %s", repr(app_password)) - screenshot_path = f"/tmp/app_password_invalid_format_{username}.png" - await page.screenshot(path=screenshot_path) - raise ValueError( - f"App password format validation failed. Screenshot: {screenshot_path}" - ) - - logger.info( - "✓ Generated app password for %s: %s... (validated)", - username, - app_password[:10], - ) - - # Close dialog with Escape key (bypasses CSS layout issues with h2 intercepting clicks) - logger.info("Closing app password dialog with Escape key...") - await page.keyboard.press("Escape") - await anyio.sleep(0.5) # Wait for dialog close animation - logger.info("Closed app password dialog") - - return app_password - - -async def enable_background_sync_via_app_password( - page: Page, username: str, app_password: str -): - """Enable background sync by entering app password in Astrolabe settings. - - Args: - page: Playwright page instance - username: Username (for logging) - app_password: App password to enter - - Returns: - True if background sync was enabled successfully - """ - logger.info("Enabling background sync via app password for %s...", username) - - nextcloud_url = "http://localhost:8080" - - # Set up network request and console listeners BEFORE navigation - network_requests = [] - network_responses = [] - console_messages = [] - - def log_request(req): - network_requests.append(f"{req.method} {req.url}") - - def log_response(resp): - response_info = f"{resp.status} {resp.url}" - network_responses.append(response_info) - logger.info("Response: %s", response_info) - - def log_console(msg): - console_messages.append(f"[{msg.type}] {msg.text}") - - page.on("request", log_request) - page.on("response", log_response) - page.on("console", log_console) - - # Navigate to Astrolabe settings + logger.info("Enabling background indexing for %s...", username) await page.goto( - f"{nextcloud_url}/settings/user/astrolabe", wait_until="networkidle" + "http://localhost:8080/settings/user/astrolabe", wait_until="networkidle" ) - - # Wait for page to load await anyio.sleep(1) - # Check if already complete (look for Step 2 "Complete" badge or overall "Active" state) - try: - # First check for overall "Active" badge (both steps complete) - active_text = page.get_by_text("Active", exact=True) - if await active_text.is_visible(timeout=2000): - logger.info("✓ Background sync already active for %s", username) - return True - except Exception: - pass + if await page.locator("#mcp-revoke-background-button").count() > 0: + logger.info("✓ Background indexing already enabled for %s", username) + return True - try: - # Check for Step 2 "Complete" badge (app password already set) - step2_section = page.locator('h4:has-text("Step 2")') - if await step2_section.count() > 0: - step2_parent = step2_section.locator("..") - complete_badge = step2_parent.get_by_text("Complete", exact=True) - if await complete_badge.count() > 0 and await complete_badge.is_visible(): - logger.info("✓ Step 2 (app password) already complete for %s", username) - return True - except Exception: - pass - - # Find the app password input field using the placeholder text - # Based on manual testing: textbox with placeholder "xxxxx-xxxxx-xxxxx-xxxxx-xxxxx" - app_password_input = page.get_by_placeholder("xxxxx-xxxxx-xxxxx-xxxxx-xxxxx") + enable_button = page.locator("#mcp-enable-background-button") + await enable_button.wait_for(timeout=5000, state="visible") + await enable_button.click() + logger.info("Clicked 'Enable background indexing' for %s", username) + # On success the page JS reloads to the enabled state (revoke form shown). try: - await app_password_input.wait_for(timeout=5000, state="visible") - logger.info("Found app password input field") + await page.locator("#mcp-revoke-background-button").wait_for( + timeout=15000, state="visible" + ) + logger.info("✓ Background indexing enabled for %s", username) + return True except Exception: - # Take screenshot for debugging - screenshot_path = f"/tmp/astrolabe_no_password_field_{username}.png" + screenshot_path = ( + f"{tempfile.gettempdir()}/astrolabe_enable_failed_{username}.png" + ) await page.screenshot(path=screenshot_path) raise ValueError( - f"Could not find app password input field for {username}. Screenshot: {screenshot_path}" + f"Background indexing did not enable for {username}. " + f"Screenshot: {screenshot_path}" ) - # Enter the app password - await app_password_input.fill(app_password) - logger.info("Entered app password for %s", username) - - # Wait a moment for any validation to complete - await anyio.sleep(0.5) - - # Take screenshot before clicking Save to check for warnings - screenshot_path = f"/tmp/before_save_{username}.png" - await page.screenshot(path=screenshot_path) - logger.info("Screenshot taken before Save: %s", screenshot_path) - - # Find and click the Save button - save_button = page.get_by_role("button", name="Save") - - # Check if Save button is enabled - is_disabled = await save_button.is_disabled() - logger.info("Save button disabled state: %s", is_disabled) - - await save_button.click() - logger.info("Clicked Save button") - - # Give the request time to complete before checking logs - await anyio.sleep(0.5) - - # Log network requests after clicking Save - logger.info("Network requests after Save for %s:", username) - for req in network_requests[-10:]: # Last 10 requests - logger.info(" %s", req) - - # Log network responses after clicking Save - logger.info("Network responses after Save for %s:", username) - for resp in network_responses[-10:]: # Last 10 responses - logger.info(" %s", resp) - - # Check specifically for the credentials POST response - credentials_responses = [ - r for r in network_responses if "background-sync/credentials" in r - ] - if credentials_responses: - logger.info("Credentials endpoint response: %s", credentials_responses[-1]) - if "200" not in credentials_responses[-1]: - logger.error( - "Credentials POST did not return 200 OK: %s", credentials_responses[-1] - ) - else: - logger.warning("No response found for credentials endpoint!") - - # Wait for the page to reload after successful save - # The JavaScript in personalSettings.js does: setTimeout(() => window.location.reload(), 1000) - await page.wait_for_load_state("networkidle", timeout=15000) - await anyio.sleep(2) - - # Log any console messages - if console_messages: - logger.info("Console messages for %s:", username) - for msg in console_messages: - logger.info(" %s", msg) - - # Check for error notifications (toast messages) - try: - error_toast = page.locator(".toastify.toast-error, .toast-error") - if await error_toast.count() > 0: - error_text = await error_toast.first.text_content() - logger.error("Error notification for %s: %s", username, error_text) - except Exception: - pass - - # Verify Step 2 "Complete" badge or overall "Active" badge appears after reload - try: - # First try to find "Active" badge (both steps complete) - active_text = page.get_by_text("Active", exact=True) - if await active_text.count() > 0: - await active_text.wait_for(timeout=5000, state="visible") - logger.info( - "✓ Background sync enabled for %s - Active badge visible", username - ) - return True - except Exception: - pass - - try: - # Check for Step 2 "Complete" badge - step2_section = page.locator('h4:has-text("Step 2")') - if await step2_section.count() > 0: - step2_parent = step2_section.locator("..") - complete_badge = step2_parent.get_by_text("Complete", exact=True) - await complete_badge.wait_for(timeout=5000, state="visible") - logger.info( - "✓ Step 2 (app password) enabled for %s - Complete badge visible", - username, - ) - return True - except Exception: - pass - - # If neither badge found, raise error - screenshot_path = f"/tmp/astrolabe_after_password_{username}.png" - await page.screenshot(path=screenshot_path) - logger.error( - "Neither Active nor Complete badge appeared for %s. Screenshot: %s", - username, - screenshot_path, - ) - raise ValueError(f"Background sync setup did not complete for {username}") - async def complete_astrolabe_authorization( page: Page, username: str, password: str ) -> dict: - """Complete full Astrolabe two-step authorization. + """Provision background indexing for a user (one-click app-password opt-in). - Performs the complete authorization flow: - 1. Navigate to Astrolabe settings - 2. OAuth authorization (Step 1) if needed - 3. Generate app password in Security settings - 4. App password entry (Step 2) if needed + The auth refactor dropped the per-user OAuth step entirely — search now + uses a session-minted JWT, so the only remaining "authorization" is the + one-click background-indexing opt-in (a dedicated app password minted from + the session and handed to the MCP server). Args: page: Playwright page instance (must be logged in) @@ -713,64 +156,15 @@ async def complete_astrolabe_authorization( password: Nextcloud password (for reference, not used directly) Returns: - Dict with {"step1": bool, "step2": bool, "app_password": str | None} + Dict with {"step1": True (no-op, kept for caller compat), + "step2": bool, "app_password": None} """ - logger.info("Starting full Astrolabe authorization for %s...", username) + logger.info("Provisioning Astrolabe background indexing for %s...", username) - result = {"step1": False, "step2": False, "app_password": None} - - # Navigate to Astrolabe settings - await navigate_to_astrolabe_settings(page) - - # Step 1: OAuth authorization - try: - result["step1"] = await authorize_search_access(page, username) - logger.info("✓ Step 1 complete for %s", username) - except Exception as e: - logger.error("Step 1 failed for %s: %s", username, e) - raise - - # Navigate back to settings if needed (OAuth might have redirected elsewhere) - if "/settings/user/astrolabe" not in page.url: - await navigate_to_astrolabe_settings(page) - - # Check if Step 2 is already complete - try: - step2_section = page.locator('h4:has-text("Step 2")') - if await step2_section.count() > 0: - step2_parent = step2_section.locator("..") - complete_badge = step2_parent.get_by_text("Complete", exact=True) - if await complete_badge.count() > 0 and await complete_badge.is_visible(): - logger.info("✓ Step 2 already complete for %s", username) - result["step2"] = True - return result - except Exception: - pass - - # Also check for overall "Active" badge - try: - active_text = page.get_by_text("Active", exact=True) - if await active_text.count() > 0 and await active_text.is_visible(): - logger.info("✓ Authorization already fully active for %s", username) - result["step2"] = True - return result - except Exception: - pass - - # Step 2: Generate app password and enter it - app_password = await generate_app_password(page, username) - result["app_password"] = app_password - - try: - result["step2"] = await enable_background_sync_via_app_password( - page, username, app_password - ) - logger.info("✓ Step 2 complete for %s", username) - except Exception as e: - logger.error("Step 2 failed for %s: %s", username, e) - raise - - logger.info("✓ Full Astrolabe authorization complete for %s", username) + # step1 is retained as always-True for backward compat with callers — there + # is no longer an OAuth authorize step to perform. + result = {"step1": True, "step2": False, "app_password": None} + result["step2"] = await enable_background_sync(page, username) return result @@ -993,15 +387,11 @@ async def test_multi_user_astrolabe_background_sync_enablement( # Step 1: Login to Nextcloud await login_to_nextcloud(page, username, password) - # Step 2: Generate app password in Security settings - app_password = await generate_app_password(page, username) + # Step 2: One-click "Enable background indexing" (mints a dedicated + # app password from the session and hands it to the MCP server). + sync_enabled = await enable_background_sync(page, username) - # Step 3: Enable background sync by entering app password in Astrolabe - sync_enabled = await enable_background_sync_via_app_password( - page, username, app_password - ) - - # Step 4: Verify app password was stored in database + # Step 3: Verify app password was stored in database app_password_stored = await verify_app_password_created(username) # Give it time to complete @@ -1009,7 +399,6 @@ async def test_multi_user_astrolabe_background_sync_enablement( results[username] = { "settings_accessed": True, - "app_password_generated": bool(app_password), "sync_enabled": sync_enabled, "app_password_stored": app_password_stored, "background_sync_active": sync_enabled and app_password_stored, @@ -1017,7 +406,6 @@ async def test_multi_user_astrolabe_background_sync_enablement( logger.info("\\n%s results:", username) logger.info(" Settings accessed: ✓") - logger.info(" App password generated: %s", "✓" if app_password else "✗") logger.info(" Sync enabled: %s", "✓" if sync_enabled else "✗") logger.info( " App password stored: %s", "✓" if app_password_stored else "✗" @@ -1061,9 +449,6 @@ async def test_multi_user_astrolabe_background_sync_enablement( assert result["settings_accessed"], ( f"{username} could not access Astrolabe settings" ) - assert result["app_password_generated"], ( - f"{username} app password was not generated" - ) assert result["sync_enabled"], ( f"{username} background sync enablement did not complete successfully" ) @@ -1081,7 +466,7 @@ async def test_multi_user_astrolabe_background_sync_enablement( async def revoke_background_sync_access(page: Page, username: str) -> bool: - """Revoke background sync access by clicking the Revoke Access button. + """Revoke background sync access by clicking the "Disable background indexing" button. Args: page: Playwright page instance (must be authenticated) @@ -1122,37 +507,37 @@ async def revoke_background_sync_access(page: Page, username: str) -> bool: # Wait for page to load await anyio.sleep(1) - # Check if "Active" badge is visible (indicating background sync is enabled) + # The revoke form (#mcp-revoke-background-form) is only rendered while + # background indexing is enabled. + revoke_button = page.locator("#mcp-revoke-background-button") try: - active_text = page.get_by_text("Active", exact=True) - if not await active_text.is_visible(timeout=2000): + if await revoke_button.count() == 0: logger.warning( - "Background sync not active for %s, nothing to revoke", username + "Background indexing not enabled for %s, nothing to revoke", username ) return False except Exception: - logger.warning("Could not find Active badge for %s", username) + logger.warning("Could not find revoke button for %s", username) return False - # Find the "Revoke Access" button - revoke_button = page.get_by_role("button", name="Revoke Access") - try: await revoke_button.wait_for(timeout=5000, state="visible") - logger.info("Found Revoke Access button") + logger.info("Found 'Disable background indexing' button") except Exception: - screenshot_path = f"/tmp/astrolabe_no_revoke_button_{username}.png" + screenshot_path = ( + f"{tempfile.gettempdir()}/astrolabe_no_revoke_button_{username}.png" + ) await page.screenshot(path=screenshot_path) raise ValueError( - f"Could not find Revoke Access button for {username}. Screenshot: {screenshot_path}" + f"Could not find revoke button for {username}. Screenshot: {screenshot_path}" ) # Set up dialog handler for confirmation dialog page.once("dialog", lambda dialog: dialog.accept()) - # Click the Revoke Access button + # Click the "Disable background indexing" button await revoke_button.click() - logger.info("Clicked Revoke Access button") + logger.info("Clicked the revoke button") # Wait for the request to complete and page to reload await page.wait_for_load_state("networkidle", timeout=15000) @@ -1178,7 +563,9 @@ async def revoke_background_sync_access(page: Page, username: str) -> bool: else: logger.warning("No response found for credentials/revoke endpoint!") # Take screenshot for debugging - screenshot_path = f"/tmp/astrolabe_revoke_no_response_{username}.png" + screenshot_path = ( + f"{tempfile.gettempdir()}/astrolabe_revoke_no_response_{username}.png" + ) await page.screenshot(path=screenshot_path) return False @@ -1198,12 +585,14 @@ async def revoke_background_sync_access(page: Page, username: str) -> bool: except Exception: pass - # Verify "Active" badge is no longer visible + # After revoke + reload the settings page returns to the un-provisioned + # state: the revoke button is gone and the app-password input is shown again. try: - active_text = page.get_by_text("Active", exact=True) - if await active_text.is_visible(timeout=2000): - logger.error("Active badge still visible for %s after revoke!", username) - screenshot_path = f"/tmp/astrolabe_revoke_still_active_{username}.png" + if await page.locator("#mcp-revoke-background-button").is_visible(timeout=2000): + logger.error("Revoke button still visible for %s after revoke!", username) + screenshot_path = ( + f"{tempfile.gettempdir()}/astrolabe_revoke_still_enabled_{username}.png" + ) await page.screenshot(path=screenshot_path) return False except Exception: @@ -1280,11 +669,11 @@ async def test_revoke_background_sync_access( test_users_setup, configure_astrolabe_for_mcp_server, ): - """Test that users can revoke background sync access via the Revoke Access button. + """Test that users can revoke background sync access via the "Disable background indexing" button. This test verifies: 1. User enables background sync via app password - 2. User clicks "Revoke Access" button + 2. User clicks "Disable background indexing" button 3. Confirmation dialog is handled 4. POST request is sent to /api/v1/background-sync/credentials/revoke 5. "Active" badge disappears from settings page @@ -1319,14 +708,9 @@ async def test_revoke_background_sync_access( # Step 1: Login to Nextcloud await login_to_nextcloud(page, username, password) - # Step 2: Complete full authorization (OAuth Step 1 + App Password Step 2) + # Provision background indexing (app-password opt-in; no OAuth step). auth_result = await complete_astrolabe_authorization(page, username, password) - assert auth_result["step1"], ( - f"OAuth authorization (Step 1) failed for {username}" - ) - assert auth_result["step2"], ( - f"App password setup (Step 2) failed for {username}" - ) + assert auth_result["step2"], f"App password provisioning failed for {username}" # Step 3: Verify background sync is enabled assert await verify_app_password_created(username), ( diff --git a/tests/integration/test_astrolabe_plotly_visualization.py b/tests/integration/test_astrolabe_plotly_visualization.py index dbe62212..33607174 100644 --- a/tests/integration/test_astrolabe_plotly_visualization.py +++ b/tests/integration/test_astrolabe_plotly_visualization.py @@ -108,7 +108,7 @@ async def navigate_to_astrolabe_main(page: Page): @pytest.mark.multi_user_basic @pytest.mark.timeout( 300 -) # 5 minutes - this test involves OAuth, app password, and vector sync +) # 5 minutes - this test involves app-password provisioning + vector sync async def test_astrolabe_plotly_visualization_with_basic_auth( browser, test_users_setup, @@ -143,7 +143,7 @@ async def test_astrolabe_plotly_visualization_with_basic_auth( page = await context.new_page() try: - # Phase 2: Complete full Astrolabe authorization (OAuth + app password) + # Phase 2: Provision background indexing (app-password opt-in; no OAuth) await login_to_nextcloud(page, username, password) auth_result = await complete_astrolabe_authorization(page, username, password) logger.info("Authorization result: %s", auth_result) diff --git a/tests/integration/test_astrolabe_session_jwt_search.py b/tests/integration/test_astrolabe_session_jwt_search.py new file mode 100644 index 00000000..f3a26915 --- /dev/null +++ b/tests/integration/test_astrolabe_session_jwt_search.py @@ -0,0 +1,109 @@ +"""Astrolabe session-derived JWT search path (card 120 auth refactor). + +Cross-system interface test. The astrolabe app was refactored to mint a +short-lived JWT for the current Nextcloud session user on demand (via the +`oidc` app's ``TokenGenerationRequestEvent``), replacing the old OAuth +authorize + offline_access + stored-refresh-token flow. This test proves the +new path end-to-end: + + logged-in NC user → GET /apps/astrolabe/api/search → astrolabe mints a JWT + (McpTokenMinter) → calls the MCP server with ``Authorization: Bearer`` → + MCP validates the JWT (unified_verifier, aud=astrolabe_client_id) → results. + +The headline behavioural change is that a user needs **no provisioning** to +search: there is no authorize redirect and ``has_background_access`` stays +False (app-password provisioning is now only for *background indexing*, a +separate opt-in covered by the background-sync tests). + +Astrolabe is installed + configured (astrolabe_client_id, mcp_server_url) by +the container app-hooks; the test skips if that wiring is absent. Driven over +HTTP with BasicAuth (which establishes a Nextcloud session for the request) — +no browser needed. +""" + +import os + +import httpx +import pytest + +pytestmark = [pytest.mark.integration, pytest.mark.login_flow] + +NEXTCLOUD_URL = "http://localhost:8080" +ASTROLABE_API = f"{NEXTCLOUD_URL}/apps/astrolabe/api" +_HEADERS = {"OCS-APIRequest": "true"} + + +async def _astrolabe_configured(client: httpx.AsyncClient, auth) -> bool: + """Readiness probe: astrolabe must be able to reach its MCP server.""" + try: + resp = await client.get( + f"{ASTROLABE_API}/vector-status", auth=auth, headers=_HEADERS + ) + except httpx.HTTPError: + return False + if resp.status_code != 200: + return False + return bool(resp.json().get("success")) + + +async def test_session_user_searches_without_provisioning(test_users_setup): + """A non-admin session user searches with no OAuth/provisioning step. + + success=True proves astrolabe minted a JWT from the session and the MCP + server accepted it. Results may be empty (nothing indexed) — the auth + chain, not recall, is under test here. + """ + auth = httpx.BasicAuth("bob", test_users_setup["bob"]["password"]) + async with httpx.AsyncClient(timeout=30) as client: + if not await _astrolabe_configured(client, auth): + pytest.skip("Astrolabe not wired to an MCP server in this stack") + + # No provisioning: search must work purely from the session JWT. + status = await client.get( + f"{ASTROLABE_API}/v1/background-sync/status", auth=auth, headers=_HEADERS + ) + assert status.json()["has_background_access"] is False, ( + "precondition: bob has not opted into background indexing" + ) + + resp = await client.get( + f"{ASTROLABE_API}/search", + params={"query": "quarterly planning", "limit": 3}, + auth=auth, + headers=_HEADERS, + ) + + assert resp.status_code == 200, resp.text + body = resp.json() + assert body["success"] is True, ( + f"session-JWT search must succeed without provisioning; got {body}" + ) + assert "results" in body and "algorithm_used" in body + + +async def test_admin_session_search_succeeds(): + """The same JWT-mint path works for the admin session user.""" + admin_pw = os.environ["NEXTCLOUD_PASSWORD"] + auth = httpx.BasicAuth(os.environ["NEXTCLOUD_USERNAME"], admin_pw) + async with httpx.AsyncClient(timeout=30) as client: + if not await _astrolabe_configured(client, auth): + pytest.skip("Astrolabe not wired to an MCP server in this stack") + resp = await client.get( + f"{ASTROLABE_API}/search", + params={"query": "infrastructure", "limit": 3}, + auth=auth, + headers=_HEADERS, + ) + assert resp.status_code == 200, resp.text + assert resp.json()["success"] is True + + +async def test_search_requires_authentication(): + """Unauthenticated search is rejected (no anonymous JWT minting).""" + async with httpx.AsyncClient(follow_redirects=False, timeout=30) as client: + resp = await client.get( + f"{ASTROLABE_API}/search", + params={"query": "x"}, + headers=_HEADERS, + ) + assert resp.status_code in (401, 302, 303, 307, 308), resp.status_code diff --git a/tests/integration/test_astrolabe_settings_buttons.py b/tests/integration/test_astrolabe_settings_buttons.py index a8c279ee..f8004477 100644 --- a/tests/integration/test_astrolabe_settings_buttons.py +++ b/tests/integration/test_astrolabe_settings_buttons.py @@ -1,96 +1,63 @@ -"""Integration tests for Astrolabe personal settings page buttons. +"""Integration tests for Astrolabe personal-settings background-sync endpoints. -Cross-system interface test: Tests the MCP server's integration with the -Astrolabe Nextcloud app, which is installed from the Nextcloud app store via -app-hooks/post-installation/20-install-astrolabe-app.sh. Astrolabe source -lives in a separate repository (https://github.com/cbcoutinho/astrolabe). +Cross-system interface test. The astrolabe app (installed by +app-hooks/post-installation/20-install-astrolabe-app.sh; source in +./third_party/astrolabe) was refactored to session-minted JWTs — the old +per-user OAuth flow and its ``/apps/astrolabe/oauth/disconnect`` route are +gone. Background indexing is now an app-password opt-in with a single revoke +endpoint. -Tests the button functionality on /settings/user/astrolabe: -1. Disable Indexing button (POST to /apps/astrolabe/api/revoke) -2. Disconnect button (POST to /apps/astrolabe/oauth/disconnect) - -These tests verify that: -- The endpoints respond correctly to POST requests -- CSRF token validation works -- User actions are properly handled -- Appropriate redirects occur +These tests assert the *current* HTTP surface of the settings page: +- the revoke endpoint exists and is auth-gated + (POST /apps/astrolabe/api/v1/background-sync/credentials/revoke) +- the obsolete OAuth disconnect route is gone (404) +- the personal settings page route resolves """ import httpx import pytest +pytestmark = pytest.mark.integration -@pytest.mark.integration -async def test_disable_indexing_button_endpoint_exists(): - """Test that the Disable Indexing endpoint is accessible.""" - async with httpx.AsyncClient() as client: - # Try without authentication - should return 401 or redirect - response = await client.post( - "http://localhost:8080/apps/astrolabe/api/revoke", - follow_redirects=False, - ) +NEXTCLOUD_URL = "http://localhost:8080" +ASTROLABE = f"{NEXTCLOUD_URL}/apps/astrolabe" - # Should get 401 Unauthorized or 30x redirect - assert response.status_code in [401, 301, 302, 303, 307, 308], ( - f"Expected 401 or redirect without auth, got {response.status_code}" +# Auth failures (no session) surface as 401 or a login redirect. +_UNAUTH = {401, 302, 303, 307, 308} + + +async def test_revoke_endpoint_requires_auth(): + """The background-sync revoke endpoint exists and rejects anonymous calls.""" + async with httpx.AsyncClient(follow_redirects=False) as client: + resp = await client.post( + f"{ASTROLABE}/api/v1/background-sync/credentials/revoke", + headers={"OCS-APIRequest": "true"}, ) + # Must NOT be 404 — the route must exist — and must be auth-gated. + assert resp.status_code != 404, "revoke route missing" + assert resp.status_code in _UNAUTH, ( + f"expected auth rejection, got {resp.status_code}" + ) -@pytest.mark.integration -async def test_disconnect_button_endpoint_exists(): - """Test that the Disconnect endpoint is accessible.""" - async with httpx.AsyncClient() as client: - # Try without authentication - should return 401 or redirect - response = await client.post( - "http://localhost:8080/apps/astrolabe/oauth/disconnect", - follow_redirects=False, - ) +async def test_obsolete_oauth_disconnect_route_removed(): + """The pre-refactor OAuth disconnect route must no longer exist. - # Should get 401 Unauthorized or 30x redirect - assert response.status_code in [401, 301, 302, 303, 307, 308], ( - f"Expected 401 or redirect without auth, got {response.status_code}" - ) - - -@pytest.mark.integration -async def test_settings_page_renders_buttons(): - """Test that the settings page template includes button forms. - - This test verifies that the PHP template renders the form elements. - It doesn't require authentication since we're just checking the route exists. + Regression guard for the auth refactor: ``/apps/astrolabe/oauth/disconnect`` + (and the rest of the OAuth authorize/callback/disconnect surface) was + removed in favour of session-minted JWTs. """ async with httpx.AsyncClient(follow_redirects=False) as client: - # Try to access settings page - response = await client.get("http://localhost:8080/settings/user/astrolabe") - - # Should get 401/redirect if not authenticated (expected) - # or 200 if user session exists from browser testing - assert response.status_code in [200, 401, 302, 303, 307, 308], ( - f"Unexpected status code: {response.status_code}" - ) + resp = await client.post(f"{ASTROLABE}/oauth/disconnect") + assert resp.status_code == 404, ( + f"obsolete oauth/disconnect route still resolves ({resp.status_code})" + ) -@pytest.mark.integration -@pytest.mark.skip( - reason="Requires manual authentication - test with Playwright instead" -) -async def test_disconnect_button_functionality(): - """Test that clicking Disconnect button clears user OAuth tokens. - - NOTE: This test is skipped because programmatic login to Nextcloud is complex. - Use Playwright-based tests or manual testing instead. - """ - pass - - -@pytest.mark.integration -@pytest.mark.skip( - reason="Requires manual authentication - test with Playwright instead" -) -async def test_disable_indexing_button_functionality(): - """Test that clicking Disable Indexing button revokes background access. - - NOTE: This test is skipped because programmatic login to Nextcloud is complex. - Use Playwright-based tests or manual testing instead. - """ - pass +async def test_settings_page_route_resolves(): + """The personal settings page route exists (auth-gated when no session).""" + async with httpx.AsyncClient(follow_redirects=False) as client: + resp = await client.get(f"{NEXTCLOUD_URL}/settings/user/astrolabe") + assert resp.status_code in ({200} | _UNAUTH), ( + f"unexpected status for settings page: {resp.status_code}" + ) diff --git a/tests/integration/test_astrolabe_token_refresh.py b/tests/integration/test_astrolabe_token_refresh.py deleted file mode 100644 index e9b9c553..00000000 --- a/tests/integration/test_astrolabe_token_refresh.py +++ /dev/null @@ -1,702 +0,0 @@ -"""Integration tests for Astrolabe token refresh flow. - -Cross-system interface test: Tests the MCP server's integration with the -Astrolabe Nextcloud app, which is installed from the Nextcloud app store via -app-hooks/post-installation/20-install-astrolabe-app.sh. Astrolabe source -lives in a separate repository (https://github.com/cbcoutinho/astrolabe). - -Tests the token refresh mechanism between Astrolabe (Nextcloud app) -and the MCP server backend in a multi-user basic auth deployment. - -This test verifies: -1. User provisions access via Astrolabe personal settings -2. Token is stored encrypted in Nextcloud database -3. Token expires (simulated via database manipulation) -4. MCP server requests new token via refresh -5. Astrolabe refreshes token with IdP -6. New token is stored and used successfully - -Note: The mcp-multi-user-basic deployment uses "hybrid mode" which requires -BOTH OAuth authorization AND app password for full configuration. These tests -focus on the app password/credential storage aspects and verify database state -directly rather than relying on UI elements that require both steps. -""" - -import logging -import re -import subprocess - -import anyio -import pytest -from playwright.async_api import Page - -pytestmark = [pytest.mark.integration, pytest.mark.multi_user_basic] - -logger = logging.getLogger(__name__) - - -async def login_to_nextcloud(page: Page, username: str, password: str): - """Helper function to login to Nextcloud via Playwright. - - Args: - page: Playwright page instance - username: Nextcloud username - password: Nextcloud password - """ - nextcloud_url = "http://localhost:8080" - - logger.info("Logging in to Nextcloud as %s...", username) - await page.goto(f"{nextcloud_url}/login", wait_until="networkidle") - - # Fill in login form - await page.wait_for_selector('input[name="user"]', timeout=10000) - await page.fill('input[name="user"]', username) - await page.fill('input[name="password"]', password) - - # Submit form - await page.click('button[type="submit"]') - await page.wait_for_load_state("networkidle", timeout=30000) - - # Verify logged in (should redirect away from login page) - current_url = page.url - assert "/login" not in current_url, ( - f"Login failed for {username}, still on login page" - ) - logger.info("✓ Successfully logged in as %s", username) - - -async def generate_app_password( - page: Page, username: str, app_name: str = "Astrolabe Test" -) -> str: - """Generate an app password in Nextcloud Security settings. - - Args: - page: Playwright page instance (must be authenticated) - username: Username (for logging) - app_name: Name for the app password - - Returns: - The generated app password string - """ - logger.info("Generating app password for %s...", username) - - nextcloud_url = "http://localhost:8080" - - # Navigate to Security settings - await page.goto(f"{nextcloud_url}/settings/user/security", wait_until="networkidle") - logger.info("Navigated to Security settings") - - # Fill the app password input field - app_password_input = page.locator('input[placeholder="App name"]') - await app_password_input.fill(app_name) - logger.info("Entered app name: %s", app_name) - - # Wait for Vue.js to react and enable the button - await anyio.sleep(1.0) - - # Click the create button - create_button = page.locator( - 'button[type="submit"]:has-text("Create new app password")' - ) - await create_button.click() - logger.info("Clicked create app password button") - - # Wait for app password to be generated - await anyio.sleep(3) - - # Find the generated app password - app_password = None - try: - await page.wait_for_selector('text="New app password"', timeout=10000) - logger.info("App password dialog appeared") - - all_inputs = await page.locator('input[type="text"]').all() - for idx, input_elem in enumerate(all_inputs): - try: - value = await input_elem.input_value() - if value and "-" in value and len(value) > 20: - app_password = value.strip() - logger.info("Found app password in input %s", idx) - break - except Exception: - continue - except Exception as e: - logger.error("Failed to find app password dialog: %s", e) - - if not app_password: - screenshot_path = f"/tmp/app_password_generation_{username}.png" - await page.screenshot(path=screenshot_path) - raise ValueError( - f"Could not find generated app password. Screenshot: {screenshot_path}" - ) - - # Validate password format - if not re.match( - r"^[a-zA-Z0-9]{5}-[a-zA-Z0-9]{5}-[a-zA-Z0-9]{5}-[a-zA-Z0-9]{5}-[a-zA-Z0-9]{5}$", - app_password, - ): - raise ValueError(f"App password format validation failed: {app_password}") - - logger.info("✓ Generated app password for %s", username) - - # Close the dialog - close_button = page.get_by_role("button", name="Close") - await close_button.click() - await anyio.sleep(0.5) - - return app_password - - -async def save_app_password_in_astrolabe( - page: Page, username: str, app_password: str -) -> bool: - """Save app password in Astrolabe settings (Step 2 of hybrid mode). - - This function only saves the app password - it does NOT verify the "Active" - badge since that requires both OAuth and app password in hybrid mode. - - Args: - page: Playwright page instance - username: Username (for logging) - app_password: App password to enter - - Returns: - True if the password was saved successfully (based on network response) - """ - logger.info("Saving app password in Astrolabe for %s...", username) - - nextcloud_url = "http://localhost:8080" - - # Track network responses - credentials_response_status = None - - def capture_response(resp): - nonlocal credentials_response_status - if "background-sync/credentials" in resp.url or "storeAppPassword" in resp.url: - credentials_response_status = resp.status - logger.info("Credentials endpoint response: %s %s", resp.status, resp.url) - - page.on("response", capture_response) - - # Navigate to Astrolabe settings - await page.goto( - f"{nextcloud_url}/settings/user/astrolabe", wait_until="networkidle" - ) - await anyio.sleep(1) - - # Check if Step 2 already shows "Complete" - try: - complete_badge = page.locator('text="Complete"').first - if await complete_badge.is_visible(timeout=2000): - logger.info("✓ App password already configured for %s", username) - return True - except Exception: - pass - - # Find the app password input field - app_password_input = page.get_by_placeholder("xxxxx-xxxxx-xxxxx-xxxxx-xxxxx") - - try: - await app_password_input.wait_for(timeout=5000, state="visible") - logger.info("Found app password input field") - except Exception: - screenshot_path = f"/tmp/astrolabe_no_password_field_{username}.png" - await page.screenshot(path=screenshot_path) - raise ValueError( - f"Could not find app password input field. Screenshot: {screenshot_path}" - ) - - # Enter the app password - await app_password_input.fill(app_password) - logger.info("Entered app password for %s", username) - - await anyio.sleep(0.5) - - # Click Save button - save_button = page.get_by_role("button", name="Save") - await save_button.click() - logger.info("Clicked Save button") - - # Wait for the request to complete and page to reload - await page.wait_for_load_state("networkidle", timeout=15000) - await anyio.sleep(2) - - # Verify the save was successful by checking network response - if credentials_response_status == 200: - logger.info("✓ App password saved successfully for %s", username) - return True - else: - logger.error( - "App password save failed for %s, status: %s", - username, - credentials_response_status, - ) - screenshot_path = f"/tmp/astrolabe_save_failed_{username}.png" - await page.screenshot(path=screenshot_path) - return False - - -def get_background_sync_credentials(username: str) -> dict | None: - """Get background sync credentials for a user from the database. - - Args: - username: Nextcloud username - - Returns: - Dict with credential details, or None if not found - """ - query = f""" - SELECT configkey, configvalue - FROM oc_preferences - WHERE userid = '{username}' - AND appid = 'astrolabe' - AND configkey IN ('background_sync_password', 'background_sync_type', 'background_sync_provisioned_at') - ORDER BY configkey; - """ - - try: - result = subprocess.run( - [ - "docker", - "compose", - "exec", - "-T", - "db", - "mariadb", - "-u", - "root", - "-ppassword", - "nextcloud", - "-e", - query, - ], - capture_output=True, - text=True, - timeout=10, - ) - - output = result.stdout - if "background_sync_type" in output: - return { - "has_password": "background_sync_password" in output, - "has_type": "background_sync_type" in output, - "has_timestamp": "background_sync_provisioned_at" in output, - "is_app_password": "app_password" in output, - } - return None - - except Exception as e: - logger.error("Error getting credentials for %s: %s", username, e) - return None - - -def delete_user_credentials(username: str) -> bool: - """Delete all stored credentials for a user (for cleanup). - - Args: - username: Nextcloud username - - Returns: - True if successful - """ - query = f""" - DELETE FROM oc_preferences - WHERE userid = '{username}' - AND appid = 'astrolabe' - AND configkey IN ('oauth_tokens', 'background_sync_password', 'background_sync_type', 'background_sync_provisioned_at'); - """ - - try: - result = subprocess.run( - [ - "docker", - "compose", - "exec", - "-T", - "db", - "mariadb", - "-u", - "root", - "-ppassword", - "nextcloud", - "-e", - query, - ], - capture_output=True, - text=True, - timeout=10, - ) - - logger.info("Deleted credentials for %s", username) - return result.returncode == 0 - - except Exception as e: - logger.error("Error deleting credentials for %s: %s", username, e) - return False - - -@pytest.mark.integration -@pytest.mark.multi_user_basic -async def test_app_password_storage_and_cleanup( - browser, - nc_client, - test_users_setup, - configure_astrolabe_for_mcp_server, -): - """Test that app passwords are stored and cleaned up correctly. - - This test verifies: - 1. User can save app password in Astrolabe settings - 2. Password is stored encrypted in the database - 3. Credentials can be revoked and are deleted from database - - Note: In hybrid mode (mcp-multi-user-basic), this only tests Step 2 - (app password storage). The "Active" badge requires both OAuth and - app password, which is tested separately. - """ - # Configure Astrolabe for mcp-multi-user-basic - logger.info("Configuring Astrolabe for mcp-multi-user-basic server...") - await configure_astrolabe_for_mcp_server( - mcp_server_internal_url="http://mcp-multi-user-basic:8000", - mcp_server_public_url="http://localhost:8003", - ) - - username = "alice" - user_config = test_users_setup[username] - password = user_config["password"] - - # Cleanup any existing credentials - delete_user_credentials(username) - - context = await browser.new_context(ignore_https_errors=True) - page = await context.new_page() - - try: - # Step 1: Login - await login_to_nextcloud(page, username, password) - - # Step 2: Verify no credentials exist initially - initial_creds = get_background_sync_credentials(username) - assert initial_creds is None, f"Expected no credentials, found: {initial_creds}" - logger.info("✓ Verified no initial credentials") - - # Step 3: Generate app password - app_password = await generate_app_password(page, username) - assert app_password, "Failed to generate app password" - - # Step 4: Save app password in Astrolabe - save_success = await save_app_password_in_astrolabe( - page, username, app_password - ) - assert save_success, "Failed to save app password" - - # Step 5: Verify credentials are stored in database - stored_creds = get_background_sync_credentials(username) - assert stored_creds is not None, "Expected credentials to be stored" - assert stored_creds["has_password"], "Expected password to be stored" - assert stored_creds["has_type"], "Expected type to be stored" - assert stored_creds["is_app_password"], "Expected type to be 'app_password'" - logger.info("✓ Verified credentials stored in database") - - # Step 6: Verify password is encrypted (not plaintext) - query = f""" - SELECT configvalue - FROM oc_preferences - WHERE userid = '{username}' - AND appid = 'astrolabe' - AND configkey = 'background_sync_password'; - """ - - result = subprocess.run( - [ - "docker", - "compose", - "exec", - "-T", - "db", - "mariadb", - "-u", - "root", - "-ppassword", - "nextcloud", - "-N", - "-e", - query, - ], - capture_output=True, - text=True, - timeout=10, - ) - - encrypted_value = result.stdout.strip() - assert app_password not in encrypted_value, "Password appears in plaintext!" - assert len(encrypted_value) > len(app_password), ( - "Encrypted value should be longer" - ) - logger.info("✓ Verified password is encrypted") - - finally: - await context.close() - # Cleanup - delete_user_credentials(username) - - -@pytest.mark.integration -@pytest.mark.multi_user_basic -async def test_credential_isolation_between_users( - browser, - nc_client, - test_users_setup, - configure_astrolabe_for_mcp_server, -): - """Test that credentials are properly isolated between users. - - This test verifies: - 1. Multiple users can provision credentials independently - 2. Each user's encrypted credentials are unique - 3. Deleting one user's credentials doesn't affect others - """ - await configure_astrolabe_for_mcp_server( - mcp_server_internal_url="http://mcp-multi-user-basic:8000", - mcp_server_public_url="http://localhost:8003", - ) - - test_users = ["alice", "bob"] - user_passwords = {} - - # Cleanup all users first - for username in test_users: - delete_user_credentials(username) - - # Provision each user - for username in test_users: - user_config = test_users_setup[username] - password = user_config["password"] - - context = await browser.new_context(ignore_https_errors=True) - page = await context.new_page() - - try: - await login_to_nextcloud(page, username, password) - app_password = await generate_app_password( - page, username, f"Test {username}" - ) - save_success = await save_app_password_in_astrolabe( - page, username, app_password - ) - - assert save_success, f"Failed to save app password for {username}" - user_passwords[username] = app_password - - # Verify stored - creds = get_background_sync_credentials(username) - assert creds is not None, f"Credentials not stored for {username}" - logger.info("✓ Credentials provisioned for %s", username) - - finally: - await context.close() - - # Verify isolation - get encrypted values - encrypted_values = {} - for username in test_users: - query = f""" - SELECT configvalue - FROM oc_preferences - WHERE userid = '{username}' - AND appid = 'astrolabe' - AND configkey = 'background_sync_password'; - """ - - result = subprocess.run( - [ - "docker", - "compose", - "exec", - "-T", - "db", - "mariadb", - "-u", - "root", - "-ppassword", - "nextcloud", - "-N", - "-e", - query, - ], - capture_output=True, - text=True, - timeout=10, - ) - encrypted_values[username] = result.stdout.strip() - - # Different users should have different encrypted values - assert encrypted_values["alice"] != encrypted_values["bob"], ( - "Different users should have different encrypted values" - ) - logger.info("✓ Verified credentials are unique per user") - - # Delete alice's credentials and verify bob's are unaffected - delete_user_credentials("alice") - - alice_creds = get_background_sync_credentials("alice") - bob_creds = get_background_sync_credentials("bob") - - assert alice_creds is None, "Alice's credentials should be deleted" - assert bob_creds is not None, "Bob's credentials should still exist" - logger.info("✓ Verified credential deletion is isolated") - - # Cleanup - for username in test_users: - delete_user_credentials(username) - - -@pytest.mark.integration -@pytest.mark.multi_user_basic -async def test_credential_revoke_and_reprovision( - browser, - nc_client, - test_users_setup, - configure_astrolabe_for_mcp_server, -): - """Test that credentials can be revoked and reprovisioned. - - This test verifies: - 1. User provisions credentials - 2. User revokes credentials (deletes from database) - 3. User provisions again with new app password - 4. New credentials are stored correctly - - Note: The UI prevents overwriting credentials directly - users must - revoke first before provisioning new credentials. - """ - await configure_astrolabe_for_mcp_server( - mcp_server_internal_url="http://mcp-multi-user-basic:8000", - mcp_server_public_url="http://localhost:8003", - ) - - username = "alice" - user_config = test_users_setup[username] - password = user_config["password"] - - delete_user_credentials(username) - - context = await browser.new_context(ignore_https_errors=True) - page = await context.new_page() - - try: - await login_to_nextcloud(page, username, password) - - # First provisioning - app_password_1 = await generate_app_password(page, username, "First Password") - await save_app_password_in_astrolabe(page, username, app_password_1) - - # Get first encrypted value - query = f""" - SELECT configvalue - FROM oc_preferences - WHERE userid = '{username}' - AND appid = 'astrolabe' - AND configkey = 'background_sync_password'; - """ - - result1 = subprocess.run( - [ - "docker", - "compose", - "exec", - "-T", - "db", - "mariadb", - "-u", - "root", - "-ppassword", - "nextcloud", - "-N", - "-e", - query, - ], - capture_output=True, - text=True, - timeout=10, - ) - first_encrypted = result1.stdout.strip() - assert first_encrypted, "First credential should be stored" - logger.info("✓ First credential stored") - - # Revoke credentials (simulating user clicking "Revoke Access") - delete_user_credentials(username) - logger.info("✓ Credentials revoked") - - # Verify credentials are gone - creds_after_revoke = get_background_sync_credentials(username) - assert creds_after_revoke is None, "Credentials should be deleted after revoke" - - # Second provisioning with different password - app_password_2 = await generate_app_password(page, username, "Second Password") - await save_app_password_in_astrolabe(page, username, app_password_2) - - result2 = subprocess.run( - [ - "docker", - "compose", - "exec", - "-T", - "db", - "mariadb", - "-u", - "root", - "-ppassword", - "nextcloud", - "-N", - "-e", - query, - ], - capture_output=True, - text=True, - timeout=10, - ) - second_encrypted = result2.stdout.strip() - assert second_encrypted, "Second credential should be stored" - logger.info("✓ Second credential stored") - - # Verify the encrypted values are different (different passwords) - assert first_encrypted != second_encrypted, ( - "Different passwords should produce different encrypted values" - ) - - # Verify only one row exists - count_query = f""" - SELECT COUNT(*) - FROM oc_preferences - WHERE userid = '{username}' - AND appid = 'astrolabe' - AND configkey = 'background_sync_password'; - """ - - count_result = subprocess.run( - [ - "docker", - "compose", - "exec", - "-T", - "db", - "mariadb", - "-u", - "root", - "-ppassword", - "nextcloud", - "-N", - "-e", - count_query, - ], - capture_output=True, - text=True, - timeout=10, - ) - count = int(count_result.stdout.strip()) - assert count == 1, f"Expected 1 credential row, found {count}" - logger.info("✓ Verified clean reprovision after revoke") - - finally: - await context.close() - delete_user_credentials(username) diff --git a/tests/integration/test_verify_on_read.py b/tests/integration/test_verify_on_read.py index ef0e27cf..bb405ca4 100644 --- a/tests/integration/test_verify_on_read.py +++ b/tests/integration/test_verify_on_read.py @@ -23,10 +23,11 @@ behaviour separately. """ import logging +import os import uuid import pytest -from httpx import HTTPStatusError +from httpx import BasicAuth, HTTPStatusError from nextcloud_mcp_server.client import NextcloudClient from nextcloud_mcp_server.search import verification @@ -48,6 +49,28 @@ 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. + return SearchResult( + id=file_id, + doc_type="file", + title=path.split("/")[-1], + excerpt="...", + score=0.9, + metadata={"path": path}, + ) + + +def _user_client(username: str, password: str) -> NextcloudClient: + return NextcloudClient( + base_url=os.environ["NEXTCLOUD_HOST"], + username=username, + auth=BasicAuth(username, password), + password=password, + ) + + async def test_verify_keeps_accessible_note( nc_client: NextcloudClient, temporary_note: dict, mocker ): @@ -170,3 +193,95 @@ async def test_verify_dedupes_chunks_of_same_document( assert dropped_count == 0 # ...but verification only fetched the note ONCE assert spy_get_note.await_count == 1 + + +# --------------------------------------------------------------------------- +# File verifier — cross-user shared access (ACL-aware search, PR #813) +# --------------------------------------------------------------------------- +# +# These exercise the verifier fix that makes ACL-aware search actually work +# end-to-end: a file an owner shared with another user must survive +# verify-on-read for the *recipient*, even when it lives in a subfolder of the +# owner's tree (Nextcloud mounts received shares at the recipient's root by +# basename, so the owner-relative path does NOT resolve under the recipient's +# root). The fix verifies by global file id, which is ACL-aware. + + +@pytest.fixture +async def alice_bob_clients(test_users_setup): + """Direct NextcloudClients for alice (owner) and bob (recipient).""" + alice = _user_client("alice", test_users_setup["alice"]["password"]) + bob = _user_client("bob", test_users_setup["bob"]["password"]) + try: + yield alice, bob + finally: + await alice._client.aclose() + await bob._client.aclose() + + +async def test_verify_keeps_nested_file_shared_with_recipient( + alice_bob_clients, mocker +): + """The PR #813 acceptance check at the verifier layer. + + 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. + """ + 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_{suffix}" + nested_dir = f"{test_dir}/reports" + shared_path = f"{nested_dir}/shared.txt" + + 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") + file_id = (await alice.webdav.get_file_info(shared_path))["id"] + + await alice.sharing.create_share( + path=f"/{shared_path}", share_with="bob", share_type=0, permissions=1 + ) + + try: + kept, dropped_count = await verify_search_results( + 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 dropped_count == 0 + spy_evict.assert_not_awaited() + finally: + 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.""" + 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" + + await alice.webdav.create_directory(test_dir) + await alice.webdav.write_file(private_path, b"alice's private note", "text/plain") + file_id = (await alice.webdav.get_file_info(private_path))["id"] + + try: + kept, dropped_count = await verify_search_results( + bob, [_result_for_file(file_id, private_path)] + ) + + 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) + finally: + await alice.webdav.delete_resource(test_dir) diff --git a/tests/unit/search/test_access_filter.py b/tests/unit/search/test_access_filter.py new file mode 100644 index 00000000..bf52b888 --- /dev/null +++ b/tests/unit/search/test_access_filter.py @@ -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" diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py index 3cee6981..ca480c3e 100644 --- a/tests/unit/search/test_verification.py +++ b/tests/unit/search/test_verification.py @@ -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. diff --git a/tests/unit/test_chunk_bbox_helper.py b/tests/unit/test_chunk_bbox_helper.py index f7abed91..d8a05b1b 100644 --- a/tests/unit/test_chunk_bbox_helper.py +++ b/tests/unit/test_chunk_bbox_helper.py @@ -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 diff --git a/tests/unit/test_login_flow.py b/tests/unit/test_login_flow.py index a5dfc9bc..c1c34377 100644 --- a/tests/unit/test_login_flow.py +++ b/tests/unit/test_login_flow.py @@ -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( diff --git a/tests/unit/test_management_app_password_endpoints.py b/tests/unit/test_management_app_password_endpoints.py index d80c9671..2f0fd0f3 100644 --- a/tests/unit/test_management_app_password_endpoints.py +++ b/tests/unit/test_management_app_password_endpoints.py @@ -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) diff --git a/tests/unit/test_oauth_tools_app_password_provisioning.py b/tests/unit/test_oauth_tools_app_password_provisioning.py new file mode 100644 index 00000000..81e96cdc --- /dev/null +++ b/tests/unit/test_oauth_tools_app_password_provisioning.py @@ -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() diff --git a/third_party/astrolabe b/third_party/astrolabe index 1d2dc122..c2dec999 160000 --- a/third_party/astrolabe +++ b/third_party/astrolabe @@ -1 +1 @@ -Subproject commit 1d2dc122801e6f69b01521810f891a46009f2198 +Subproject commit c2dec999063e15e652321b0867457fef6e0f8e5c