diff --git a/nextcloud_mcp_server/client/__init__.py b/nextcloud_mcp_server/client/__init__.py index e31806083..2a318a119 100644 --- a/nextcloud_mcp_server/client/__init__.py +++ b/nextcloud_mcp_server/client/__init__.py @@ -106,15 +106,15 @@ def __init__( password: str | None = None, token: str | None = None, ): - # ``username`` is the Nextcloud UID — it drives DAV/API path - # construction (e.g. ``/remote.php/dav/files//``). ``auth_username`` - # is the credential identity Nextcloud authenticates the app password - # against (the loginName), which differs from the UID for - # OIDC-provisioned users. Defaults to ``username`` so single-user and - # OAuth modes (where UID == loginName) are unchanged. Callers pass the - # matching ``auth=BasicAuth(auth_username, ...)`` for the httpx leg; - # ``auth_username`` is threaded to the CalDAV client, which builds its - # own auth object from the raw credential. + # ``username`` is the Nextcloud UID and DAV path fallback. Discovery can + # replace that fallback with the canonical principal id when Nextcloud + # exposes a different DAV identity. ``auth_username`` is the credential + # identity Nextcloud authenticates the app password against (the + # loginName), which differs from the UID for OIDC-provisioned users. + # Defaults to ``username`` so single-user and OAuth modes are unchanged. + # Callers pass the matching ``auth=BasicAuth(auth_username, ...)`` for + # the httpx leg; ``auth_username`` is threaded to the CalDAV client, + # which builds its own auth object from the raw credential. self.username = username auth_username = auth_username or username self._client = AsyncClient( @@ -348,7 +348,7 @@ async def find_files_by_tag( def _get_webdav_base_path(self) -> str: """Helper to get the base WebDAV path for the authenticated user.""" - return f"/remote.php/dav/files/{self.username}" + return self.webdav._get_webdav_base_path() async def __aenter__(self): """Async context manager entry.""" diff --git a/nextcloud_mcp_server/client/base.py b/nextcloud_mcp_server/client/base.py index 9e4bf74fd..428c30a07 100644 --- a/nextcloud_mcp_server/client/base.py +++ b/nextcloud_mcp_server/client/base.py @@ -2,11 +2,13 @@ import logging import time +import xml.etree.ElementTree as ET from abc import ABC from functools import wraps +from urllib.parse import unquote import anyio -from httpx import AsyncClient, HTTPStatusError, RequestError, codes +from httpx import AsyncClient, HTTPError, HTTPStatusError, RequestError, codes from nextcloud_mcp_server.observability.metrics import ( record_nextcloud_api_call, @@ -104,10 +106,66 @@ def __init__(self, http_client: AsyncClient, username: str): """ self._client = http_client self.username = username + self._principal_id: str | None = None + self._principal_discovered = False def _get_webdav_base_path(self) -> str: """Helper to get the base WebDAV path for the authenticated user.""" - return f"/remote.php/dav/files/{self.username}" + return f"/remote.php/dav/files/{self._principal_or_username()}" + + async def _ensure_principal_id(self) -> None: + """Discover the canonical DAV principal id via current-user-principal.""" + if getattr(self, "_principal_discovered", False): + return + + body = ( + '' + '' + "" + "" + ) + + try: + response = await self._make_request( + "PROPFIND", + "/remote.php/dav/", + content=body, + headers={"Depth": "0", "Content-Type": "application/xml"}, + ) + root = ET.fromstring(response.content) + href = None + for element in root.iter(): + if element.tag.split("}")[-1] != "current-user-principal": + continue + for child in element.iter(): + if child.tag.split("}")[-1] == "href" and child.text: + href = child.text.strip() + break + if href: + break + + if not href: + logger.warning( + "DAV principal discovery returned no href; using username path" + ) + return + + principal_id = unquote(href.rstrip("/").split("/")[-1]) + if not principal_id: + logger.warning( + "DAV principal discovery returned an empty principal id; " + "using username path" + ) + return + + self._principal_id = principal_id + self._principal_discovered = True + except (HTTPError, ET.ParseError, ValueError) as e: + logger.warning("DAV principal discovery failed; using username path: %s", e) + + def _principal_or_username(self) -> str: + """Return the discovered DAV principal id, falling back to username.""" + return getattr(self, "_principal_id", None) or self.username @staticmethod def _resolve_url(url: str) -> str: diff --git a/nextcloud_mcp_server/client/calendar.py b/nextcloud_mcp_server/client/calendar.py index a7f24252a..6b8cb7311 100644 --- a/nextcloud_mcp_server/client/calendar.py +++ b/nextcloud_mcp_server/client/calendar.py @@ -5,9 +5,11 @@ import logging import uuid from typing import Any +from urllib.parse import unquote from zoneinfo import ZoneInfo, ZoneInfoNotFoundError import anyio +import httpx import recurring_ical_events from caldav.aio import AsyncCalendar, AsyncDAVClient, AsyncEvent from caldav.elements import cdav, dav @@ -56,7 +58,7 @@ def __init__( Args: base_url: Nextcloud base URL - username: Nextcloud username (UID) — used for DAV path construction + username: Nextcloud username (UID) used as the DAV path fallback auth_username: Credential identity (loginName) the app password authenticates against; defaults to ``username``. Differs from the UID for OIDC-provisioned users. @@ -68,10 +70,11 @@ def __init__( """ self.username = username self.base_url = base_url - # The UID (``username``) drives DAV path construction; the loginName - # (``auth_username``) is the credential the app password authenticates - # against. They differ for OIDC-provisioned users. Defaults to the UID - # so existing single-user / OAuth callers are unchanged. + # The UID (``username``) is the DAV path fallback until principal + # discovery succeeds; the loginName (``auth_username``) is the + # credential the app password authenticates against. They differ for + # OIDC-provisioned users. Defaults to the UID so existing single-user / + # OAuth callers are unchanged. auth_username = auth_username or username auth_kwargs: dict[str, Any] = {} @@ -97,6 +100,85 @@ def __init__( **auth_kwargs, ) self._calendar_home_url = f"{base_url}/remote.php/dav/calendars/{username}/" + self._principal_resolved = False + + def _calendar_home_url_from_home_set(self, home_set: Any) -> str | None: + """Normalize a caldav CalendarSet or URL into an absolute home URL.""" + if home_set is None: + return None + + home_url = getattr(home_set, "url", home_set) + if home_url is None: + return None + + home_url = str(home_url) + if not home_url: + return None + if home_url.startswith("/"): + home_url = f"{self.base_url}{home_url}" + if not home_url.endswith("/"): + home_url = f"{home_url}/" + return home_url + + async def _calendar_home_url_from_principal(self, principal: Any) -> str | None: + """Resolve calendar-home-set without using caldav's async-unsafe property.""" + get_property = getattr(principal, "get_property", None) + if get_property is not None: + try: + home_set = await _maybe_await(get_property(cdav.CalendarHomeSet())) + calendar_home_url = self._calendar_home_url_from_home_set(home_set) + if calendar_home_url: + return calendar_home_url + except (caldav_error.DAVError, AttributeError, TypeError, ValueError) as e: + logger.warning( + "CalDAV calendar-home-set discovery failed; deriving from " + "principal URL: %s", + e, + ) + + try: + home_set = getattr(principal, "calendar_home_set", None) + home_set = await _maybe_await(home_set) + return self._calendar_home_url_from_home_set(home_set) + except (AttributeError, TypeError, ValueError) as e: + logger.warning( + "CalDAV calendar-home-set property unavailable; deriving from " + "principal URL: %s", + e, + ) + return None + + async def _ensure_calendar_home(self) -> None: + """Discover and cache the authenticated user's CalDAV calendar home.""" + if self._principal_resolved: + return + + try: + get_principal = getattr(self._dav_client, "get_principal", None) + if get_principal is None: + principal = await _maybe_await(self._dav_client.principal()) + else: + principal = await _maybe_await(get_principal()) + + calendar_home_url = await self._calendar_home_url_from_principal(principal) + if calendar_home_url: + self._calendar_home_url = calendar_home_url + self._principal_resolved = True + return + + principal_url = getattr(principal, "url", None) + if principal_url is None: + raise ValueError("CalDAV principal discovery returned no URL") + principal_id = unquote(str(principal_url).rstrip("/").split("/")[-1]) + if principal_id: + self._calendar_home_url = ( + f"{self.base_url}/remote.php/dav/calendars/{principal_id}/" + ) + self._principal_resolved = True + except (caldav_error.DAVError, httpx.HTTPError, ValueError) as e: + logger.warning( + "CalDAV principal discovery failed; using username path: %s", e + ) def _get_calendar_url(self, calendar_name: str) -> str: """Get the full URL for a calendar.""" @@ -197,6 +279,7 @@ async def list_calendars(self) -> list[dict[str, Any]]: (webcal/ICS feeds). Subscriptions are reported with ``read_only=True`` and a ``source`` URL pointing at the upstream feed (issue #830). """ + await self._ensure_calendar_home() # Use custom PROPFIND with CalendarServer namespace (cs:) for calendar-color. # caldav library's nsmap lacks "CS" namespace, and its CalendarColor uses # Apple iCal namespace which Nextcloud doesn't recognize. @@ -326,13 +409,12 @@ async def create_calendar( color: str = "#1976D2", ) -> dict[str, Any]: """Create a new calendar with retry on 429 errors.""" + await self._ensure_calendar_home() # Use custom MKCALENDAR XML instead of caldav library's make_calendar() due to: # 1. Missing CalendarServer namespace (cs:) in caldav's nsmap # 2. caldav's CalendarColor uses Apple iCal namespace, not cs:calendar-color # 3. make_calendar() doesn't support calendar-description or calendar-color params - calendar_url = ( - f"{self.base_url}/remote.php/dav/calendars/{self.username}/{calendar_name}/" - ) + calendar_url = self._get_calendar_url(calendar_name) mkcalendar_body = f""" @@ -372,10 +454,9 @@ async def create_calendar( async def delete_calendar(self, calendar_name: str) -> dict[str, Any]: """Delete a calendar.""" + await self._ensure_calendar_home() # Use absolute URL for deletion - calendar_url = ( - f"{self.base_url}/remote.php/dav/calendars/{self.username}/{calendar_name}/" - ) + calendar_url = self._get_calendar_url(calendar_name) await self._dav_client.delete(calendar_url) logger.debug("Deleted calendar: %s", calendar_name) @@ -391,6 +472,7 @@ async def get_calendar_events( limit: int = 50, ) -> list[dict[str, Any]]: """List events in a calendar within date range.""" + await self._ensure_calendar_home() calendar = self._get_calendar(calendar_name) if start_datetime or end_datetime: @@ -531,6 +613,7 @@ async def create_event( self, calendar_name: str, event_data: dict[str, Any] ) -> dict[str, Any]: """Create a new calendar event.""" + await self._ensure_calendar_home() calendar = self._get_calendar(calendar_name) event_uid = str(uuid.uuid4()) @@ -556,6 +639,7 @@ async def update_event( etag: str = "", ) -> dict[str, Any]: """Update an existing calendar event.""" + await self._ensure_calendar_home() calendar = self._get_calendar(calendar_name) # Find the event by UID using caldav library @@ -580,6 +664,7 @@ async def update_event( async def delete_event(self, calendar_name: str, event_uid: str) -> dict[str, Any]: """Delete a calendar event.""" + await self._ensure_calendar_home() calendar = self._get_calendar(calendar_name) try: @@ -597,6 +682,7 @@ async def get_event( self, calendar_name: str, event_uid: str ) -> tuple[dict[str, Any], str]: """Get detailed information about a specific event.""" + await self._ensure_calendar_home() calendar = self._get_calendar(calendar_name) event = await self._async_object_by_uid( @@ -621,6 +707,7 @@ async def search_events_across_calendars( filters: dict[str, Any] | None = None, ) -> list[dict[str, Any]]: """Search events across all calendars with advanced filtering.""" + await self._ensure_calendar_home() try: calendars = await self.list_calendars() all_events = [] @@ -661,6 +748,7 @@ async def list_todos( self, calendar_name: str, filters: dict[str, Any] | None = None ) -> list[dict[str, Any]]: """List todos/tasks in a calendar.""" + await self._ensure_calendar_home() calendar = self._get_calendar(calendar_name) # Get all todos including completed ones (filtering is done client-side) @@ -690,6 +778,7 @@ async def create_todo( self, calendar_name: str, todo_data: dict[str, Any] ) -> dict[str, Any]: """Create a new todo/task.""" + await self._ensure_calendar_home() calendar = self._get_calendar(calendar_name) todo_uid = str(uuid.uuid4()) @@ -715,6 +804,7 @@ async def update_todo( etag: str = "", ) -> dict[str, Any]: """Update an existing todo/task.""" + await self._ensure_calendar_home() calendar = self._get_calendar(calendar_name) try: @@ -754,6 +844,7 @@ async def update_todo( async def delete_todo(self, calendar_name: str, todo_uid: str) -> dict[str, Any]: """Delete a todo/task.""" + await self._ensure_calendar_home() calendar = self._get_calendar(calendar_name) try: @@ -771,6 +862,7 @@ async def search_todos_across_calendars( self, filters: dict[str, Any] | None = None ) -> list[dict[str, Any]]: """Search todos across all calendars.""" + await self._ensure_calendar_home() try: calendars = await self.list_calendars() all_todos = [] @@ -1457,6 +1549,7 @@ async def bulk_update_events( self, filter_criteria: dict[str, Any], update_data: dict[str, Any] ) -> dict[str, Any]: """Bulk update events matching filter criteria.""" + await self._ensure_calendar_home() try: start_datetime = None end_datetime = None diff --git a/nextcloud_mcp_server/client/contacts.py b/nextcloud_mcp_server/client/contacts.py index 0a0c9e313..61e311251 100644 --- a/nextcloud_mcp_server/client/contacts.py +++ b/nextcloud_mcp_server/client/contacts.py @@ -215,7 +215,7 @@ class ContactsClient(BaseNextcloudClient): def _get_carddav_base_path(self) -> str: """Helper to get the base CardDAV path for contacts.""" - return f"/remote.php/dav/addressbooks/users/{self.username}" + return f"/remote.php/dav/addressbooks/users/{self._principal_or_username()}" async def _list_object_names(self, addressbook: str) -> list[str]: """Return the CardDAV object filenames stored in ``addressbook``. @@ -226,6 +226,7 @@ async def _list_object_names(self, addressbook: str) -> list[str]: issue #874 — so callers that need to address a specific object must discover its real name rather than constructing one. """ + await self._ensure_principal_id() carddav_path = self._get_carddav_base_path() propfind_body = """ """ @@ -278,6 +279,7 @@ async def _resolve_object_name(self, addressbook: str, uid: str) -> str | None: async def list_addressbooks(self): """List all available addressbooks for the user.""" + await self._ensure_principal_id() carddav_path = self._get_carddav_base_path() propfind_body = """ @@ -314,7 +316,10 @@ async def list_addressbooks(self): # Extract addressbook name from href addressbook_name = href_text.rstrip("/").split("/")[-1] - if not addressbook_name or addressbook_name == self.username: + if ( + not addressbook_name + or addressbook_name == self._principal_or_username() + ): continue # Get properties @@ -349,6 +354,7 @@ async def list_addressbooks(self): async def create_addressbook(self, *, name: str, display_name: str): """Create a new addressbook.""" + await self._ensure_principal_id() carddav_path = self._get_carddav_base_path() url = f"{carddav_path}/{name}/" @@ -373,6 +379,7 @@ async def create_addressbook(self, *, name: str, display_name: str): async def delete_addressbook(self, *, name: str): """Delete an addressbook.""" + await self._ensure_principal_id() carddav_path = self._get_carddav_base_path() url = f"{carddav_path}/{name}/" await self._make_request("DELETE", url) @@ -381,6 +388,7 @@ async def create_contact( self, *, addressbook: str, uid: str, contact_data: dict[str, Any] ): """Create a new contact.""" + await self._ensure_principal_id() carddav_path = self._get_carddav_base_path() url = f"{carddav_path}/{addressbook}/{uid}.vcf" @@ -404,6 +412,7 @@ async def delete_contact(self, *, addressbook: str, uid: str): ``.vcf`` (issue #874). Falls back to the conventional name when no object matches so a genuinely missing contact still surfaces a 404. """ + await self._ensure_principal_id() carddav_path = self._get_carddav_base_path() object_name = await self._resolve_object_name(addressbook, uid) or f"{uid}.vcf" url = f"{carddav_path}/{addressbook}/{object_name}" @@ -418,6 +427,7 @@ async def update_contact( etag: str = "", ): """Update an existing contact while preserving all existing properties.""" + await self._ensure_principal_id() carddav_path = self._get_carddav_base_path() # Resolve the real object filename (may differ from ``.vcf``) so the # GET and the PUT target the same resource — see issue #874. @@ -462,6 +472,7 @@ async def update_contact( async def list_contacts(self, *, addressbook: str): """List all available contacts for addressbook.""" + await self._ensure_principal_id() carddav_path = self._get_carddav_base_path() report_body = """ @@ -588,6 +599,7 @@ async def _fetch_raw_vcard( must resolve it first via ``_resolve_object_name`` — see issue #874. ``update_contact`` does exactly that and passes the resolved name here. """ + await self._ensure_principal_id() carddav_path = self._get_carddav_base_path() url = f"{carddav_path}/{addressbook}/{object_name}" diff --git a/nextcloud_mcp_server/client/webdav.py b/nextcloud_mcp_server/client/webdav.py index e67bb5b66..7fb68b5c0 100644 --- a/nextcloud_mcp_server/client/webdav.py +++ b/nextcloud_mcp_server/client/webdav.py @@ -108,7 +108,7 @@ def _webdav_path(self, path: str) -> str: Percent-encodes the caller-supplied portion (see ``_encode_dav_path``) so names with ``#``, commas, or spaces don't truncate/404; the base - ``/remote.php/dav/files/`` segment is left as-is. + ``/remote.php/dav/files/`` segment is left as-is. Precondition: ``path`` is a **decoded** path (the convention everywhere in this client — PROPFIND/REPORT hrefs are ``unquote``d before storage, @@ -119,6 +119,7 @@ def _webdav_path(self, path: str) -> str: async def delete_resource(self, path: str) -> Dict[str, Any]: """Delete a resource (file or directory) via WebDAV DELETE.""" + await self._ensure_principal_id() # Ensure path ends with a slash if it's a directory if not path.endswith("/"): path_with_slash = f"{path}/" @@ -210,6 +211,7 @@ async def add_note_attachment( mime_type: Optional[str] = None, ) -> Dict[str, Any]: """Add/Update an attachment to a note via WebDAV PUT.""" + await self._ensure_principal_id() # Construct paths based on provided category. Encode via _webdav_path so # categories/filenames with '#', commas or spaces don't truncate/404. category_path_part = f"{category}/" if category else "" @@ -295,6 +297,7 @@ async def get_note_attachment( self, note_id: int, filename: str, category: Optional[str] = None ) -> Tuple[bytes, str]: """Fetch a specific attachment from a note via WebDAV GET.""" + await self._ensure_principal_id() category_path_part = f"{category}/" if category else "" attachment_dir_segment = f".attachments.{note_id}" attachment_path = self._webdav_path( @@ -339,6 +342,7 @@ async def get_note_attachment( async def list_directory(self, path: str = "") -> List[Dict[str, Any]]: """List files and directories in the specified path via WebDAV PROPFIND.""" + await self._ensure_principal_id() webdav_path = self._webdav_path(path) if not webdav_path.endswith("/"): webdav_path += "/" @@ -439,6 +443,7 @@ async def list_directory(self, path: str = "") -> List[Dict[str, Any]]: async def read_file(self, path: str) -> Tuple[bytes, str]: """Read a file's content via WebDAV GET.""" + await self._ensure_principal_id() webdav_path = self._webdav_path(path) logger.debug("Reading file: %s", path) @@ -466,6 +471,7 @@ async def write_file( self, path: str, content: bytes, content_type: Optional[str] = None ) -> Dict[str, Any]: """Write content to a file via WebDAV PUT.""" + await self._ensure_principal_id() webdav_path = self._webdav_path(path) logger.debug("Writing file: %s", path) @@ -497,6 +503,7 @@ async def create_directory( self, path: str, recursive: bool = False ) -> Dict[str, Any]: """Create a directory via WebDAV MKCOL.""" + await self._ensure_principal_id() webdav_path = self._webdav_path(path) if not webdav_path.endswith("/"): webdav_path += "/" @@ -555,6 +562,7 @@ async def move_resource( Returns: Dict with status_code and optional message """ + await self._ensure_principal_id() source_webdav_path = self._webdav_path(source_path) destination_webdav_path = self._webdav_path(destination_path) @@ -637,6 +645,7 @@ async def copy_resource( Returns: Dict with status_code and optional message """ + await self._ensure_principal_id() source_webdav_path = self._webdav_path(source_path) destination_webdav_path = self._webdav_path(destination_path) @@ -733,6 +742,7 @@ async def search_files( Returns: List of file/directory dictionaries with requested properties """ + await self._ensure_principal_id() # Default properties if not specified if properties is None: properties = [ @@ -963,8 +973,8 @@ def _build_search_xml( ) -> str: """Build the XML body for a SEARCH request.""" # Construct the scope path - username = self.username - scope_path = f"/files/{username}" + principal = self._principal_or_username() + scope_path = f"/files/{principal}" if scope: scope_path = f"{scope_path}/{scope.lstrip('/')}" # XML-escape before embedding in : a folder whose name contains @@ -1106,10 +1116,10 @@ def _parse_search_response( # RFC 3986 to be percent-encoded, so non-ASCII paths arrive # encoded — decode before exposing to callers (issue #776). href_text = unquote(href.text or "") - # Remove the /remote.php/dav/files/username/ prefix to get relative path + # Remove the /remote.php/dav/files// prefix to get relative path. path_parts = href_text.split("/files/") if len(path_parts) > 1: - # Get the path after username + # Get the path after the principal segment. path_after_user = "/".join(path_parts[1].split("/")[1:]) relative_path = path_after_user.rstrip("/") else: @@ -1638,6 +1648,7 @@ async def get_files_by_tag(self, tag_id: int) -> list[dict[str, Any]]: Returns: List of file info dictionaries with path, size, content_type, etc. """ + await self._ensure_principal_id() # Use WebDAV REPORT method with systemtag filter. resourcetype is # included so callers can distinguish folders from files (needed for # recursive exclusion of tagged directories — see issue #710). @@ -1713,7 +1724,7 @@ async def get_files_by_tag(self, tag_id: int) -> list[dict[str, Any]]: # so an adversarially-named directory could collide; strip # only the leading occurrence via startswith + slice. href_path = unquote(href_elem.text) - webdav_prefix = f"/remote.php/dav/files/{self.username}/" + webdav_prefix = f"/remote.php/dav/files/{self._principal_or_username()}/" if href_path.startswith(webdav_prefix): file_path = "/" + href_path[len(webdav_prefix) :] else: @@ -1783,6 +1794,7 @@ async def get_file_info(self, path: str) -> dict[str, Any] | None: distinguish a definitive absence (HTTP 404) from a brittle response (None). """ + await self._ensure_principal_id() webdav_path = self._webdav_path(path) propfind_body = """ diff --git a/tests/server/ldap/test_ldap_dav_principal.py b/tests/server/ldap/test_ldap_dav_principal.py index 6a86cb1f9..ea601ebed 100644 --- a/tests/server/ldap/test_ldap_dav_principal.py +++ b/tests/server/ldap/test_ldap_dav_principal.py @@ -2,24 +2,20 @@ The LDAP user `alice` logs in as `alice` but Nextcloud's `user_ldap` backend maps her to a canonical internal UID (the LDAP UUID). The multi-user BasicAuth -MCP server builds DAV paths from the loginName, so every WebDAV operation +MCP server would build DAV paths from the loginName, so a naive WebDAV operation targets ``/remote.php/dav/files/alice/`` — which does NOT resolve to her real home at ``/remote.php/dav/files//`` (unlike login-by-email, an LDAP login is not a files-path alias, so it 404s rather than silently resolving). -* **Without** the #980 client fix → the round-trip targets the non-existent - ``/files/alice/`` home and fails → the test below **fails** (RED). It is - therefore marked ``xfail(strict=True)``: on ``master`` (no fix) it xfails, so - CI stays green while still asserting the bug is present. -* **With** #980's ``BaseNextcloudClient._ensure_principal_id`` (a - ``PROPFIND /remote.php/dav/`` for ``current-user-principal``) the real UID is - discovered and the paths are rewritten → the round-trip succeeds → the test - **xpasses**. ``strict=True`` then turns the unexpected pass into a CI failure, - which is the signal to drop the ``xfail`` marker once #980 lands. +This is the regression guard for that bug: ``BaseNextcloudClient._ensure_principal_id`` +issues a ``PROPFIND /remote.php/dav/`` for ``current-user-principal``, discovers +the real UID, and rewrites the base path — so the WebDAV round-trip below lands +in alice's real home and **passes**. Before that fix (GH #980) it failed: the +round-trip targeted the non-existent ``/files/alice/`` home. -This is the live RED→GREEN reproduction that the Keycloak lane (PR #993) could -not provide, because email/`user_oidc` logins don't produce a non-resolvable -divergent path on the CI Nextcloud versions. +It is the live reproduction that the Keycloak lane (PR #993) could not provide, +because email/`user_oidc` logins don't produce a non-resolvable divergent path +on the CI Nextcloud versions. """ import json @@ -30,22 +26,15 @@ pytestmark = [pytest.mark.integration, pytest.mark.ldap] -@pytest.mark.xfail( - strict=True, - reason=( - "Reproduces GH #980: DAV paths built from the LDAP loginName miss the " - "canonical-UID home. Fixed by BaseNextcloudClient._ensure_principal_id " - "(PR #980) — drop this marker once that lands." - ), -) async def test_webdav_round_trip_resolves_ldap_principal( nc_mcp_ldap_alice_client: ClientSession, ): """A full WebDAV cycle as the divergent LDAP user must hit her real home. - create → write → read → list → delete, all as `alice`. Without #980 the - paths are built from the loginName `alice` and the very first operation - fails against the non-existent ``/files/alice/`` home. + create → write → read → list → delete, all as `alice`. Principal discovery + resolves the loginName `alice` to her canonical UID so every operation lands + in her real home; before GH #980's fix the first operation failed against + the non-existent ``/files/alice/`` home. """ dir_path = "/LdapPrincipalTest" file_path = f"{dir_path}/ldap_principal.txt" diff --git a/tests/unit/client/test_calendar.py b/tests/unit/client/test_calendar.py index 43983b41b..a9da7e879 100644 --- a/tests/unit/client/test_calendar.py +++ b/tests/unit/client/test_calendar.py @@ -99,12 +99,12 @@ def test_password_takes_precedence_over_token(mocker): assert call_kwargs["auth_type"] == "basic" -def test_auth_username_used_for_credential_uid_for_path(mocker): - """OIDC users: the loginName authenticates, the UID builds the DAV path. +def test_auth_username_used_for_credential_uid_for_fallback_path(mocker): + """OIDC users: the loginName authenticates, the UID seeds DAV fallback paths. Nextcloud keys app-password auth on the loginName (which can differ from - the UID), but ``/remote.php/dav/calendars//`` must use the UID. The - two must not be conflated. + the UID), but discovery starts from a UID-based calendar home fallback. The + two identities must not be conflated. """ mock_dav_client = mocker.patch( "nextcloud_mcp_server.client.calendar.AsyncDAVClient" @@ -121,7 +121,7 @@ def test_auth_username_used_for_credential_uid_for_path(mocker): # Credential identity → loginName assert mock_dav_client.call_args.kwargs["username"] == "ada@example.com" - # Path identity → UID + # Fallback path identity -> UID assert client.username == "Ada Lovelace" assert ( client._calendar_home_url diff --git a/tests/unit/client/test_contacts.py b/tests/unit/client/test_contacts.py index d332e4557..7a234fae2 100644 --- a/tests/unit/client/test_contacts.py +++ b/tests/unit/client/test_contacts.py @@ -270,6 +270,7 @@ async def test_delete_targets_real_no_extension_path(self, mocker): """Regression for #874: delete must hit ``.../default`` not ``.../default.vcf``.""" client = ContactsClient.__new__(ContactsClient) client.username = "testuser" + client._principal_discovered = True mocker.patch.object( client, "_resolve_object_name", mocker.AsyncMock(return_value="default") ) @@ -286,6 +287,7 @@ async def test_delete_falls_back_to_vcf_when_unresolved(self, mocker): """ client = ContactsClient.__new__(ContactsClient) client.username = "testuser" + client._principal_discovered = True mocker.patch.object( client, "_resolve_object_name", mocker.AsyncMock(return_value=None) ) @@ -302,6 +304,7 @@ async def test_update_targets_real_no_extension_path(self, mocker): """ client = ContactsClient.__new__(ContactsClient) client.username = "testuser" + client._principal_discovered = True mocker.patch.object( client, "_resolve_object_name", mocker.AsyncMock(return_value="default") ) @@ -329,6 +332,7 @@ async def test_update_falls_back_to_vcf_when_unresolved(self, mocker): """ client = ContactsClient.__new__(ContactsClient) client.username = "testuser" + client._principal_discovered = True mocker.patch.object( client, "_resolve_object_name", mocker.AsyncMock(return_value=None) ) diff --git a/tests/unit/client/test_dav_principal_discovery.py b/tests/unit/client/test_dav_principal_discovery.py new file mode 100644 index 000000000..7085f8b27 --- /dev/null +++ b/tests/unit/client/test_dav_principal_discovery.py @@ -0,0 +1,475 @@ +"""Unit coverage for DAV current-user-principal path discovery.""" + +from types import SimpleNamespace + +import httpx +import pytest +from caldav.lib import error as caldav_error + +from nextcloud_mcp_server.client.contacts import ContactsClient +from nextcloud_mcp_server.client.webdav import WebDAVClient + +pytestmark = pytest.mark.unit + +# Dev-only fixture credential passed to CalendarClient in the tests below. +_APP_PW = "app-pw" # NOSONAR(S2068) + + +def _http_response(content: bytes = b"", status_code: int = 207) -> httpx.Response: + return httpx.Response( + status_code, + content=content, + request=httpx.Request("PROPFIND", "https://cloud.example.org/remote.php/dav/"), + ) + + +def _principal_body(principal_id: str) -> bytes: + return f""" + + + + + + /remote.php/dav/principals/users/{principal_id}/ + + + HTTP/1.1 200 OK + + +""".encode() + + +def _principal_body_without_href() -> bytes: + return b""" + + + + + + + HTTP/1.1 200 OK + + +""" + + +def _empty_webdav_dir(principal_id: str) -> bytes: + return f""" + + + /remote.php/dav/files/{principal_id}/ + + + HTTP/1.1 200 OK + + +""".encode() + + +def _addressbooks_body(principal_id: str) -> bytes: + return f""" + + + /remote.php/dav/addressbooks/users/{principal_id}/contacts/ + + + Contacts + ctag-1 + + HTTP/1.1 200 OK + + +""".encode() + + +def _calendar_multistatus(principal_id: str) -> str: + return f""" + + + /remote.php/dav/calendars/{principal_id}/ + + + HTTP/1.1 200 OK + + + + /remote.php/dav/calendars/{principal_id}/personal/ + + + Personal + + #1976D2 + + HTTP/1.1 200 OK + + +""" + + +def _request_error() -> httpx.RequestError: + request = httpx.Request("PROPFIND", "https://cloud.example.org/remote.php/dav/") + return httpx.RequestError("temporary DAV discovery failure", request=request) + + +async def test_webdav_public_method_discovers_divergent_principal(mocker): + client = WebDAVClient(mocker.AsyncMock(spec=httpx.AsyncClient), "alice") + client._make_request = mocker.AsyncMock( + side_effect=[ + _http_response(_principal_body("alice_1234")), + _http_response(_empty_webdav_dir("alice_1234")), + ] + ) + + await client.list_directory("Documents") + + calls = client._make_request.await_args_list + assert calls[0].args[:2] == ("PROPFIND", "/remote.php/dav/") + assert calls[1].args[:2] == ( + "PROPFIND", + "/remote.php/dav/files/alice_1234/Documents/", + ) + + +def test_webdav_search_scope_uses_discovered_principal(mocker): + client = WebDAVClient(mocker.AsyncMock(spec=httpx.AsyncClient), "alice") + client._principal_id = "alice_1234" + + body = client._build_search_xml( + scope="Reports", + where_conditions=None, + properties=["displayname"], + order_by=None, + limit=None, + ) + + assert "/files/alice_1234/Reports" in body + + +async def test_webdav_equal_principal_keeps_username_path(mocker): + client = WebDAVClient(mocker.AsyncMock(spec=httpx.AsyncClient), "alice") + file_response = _http_response(b"hello", status_code=200) + file_response.headers["content-type"] = "text/plain" + client._make_request = mocker.AsyncMock( + side_effect=[_http_response(_principal_body("alice")), file_response] + ) + + content, _ = await client.read_file("notes.txt") + + assert content == b"hello" + calls = client._make_request.await_args_list + assert calls[0].args[:2] == ("PROPFIND", "/remote.php/dav/") + assert calls[1].args[:2] == ("GET", "/remote.php/dav/files/alice/notes.txt") + + +async def test_webdav_discovery_failure_falls_back_to_username(mocker): + client = WebDAVClient(mocker.AsyncMock(spec=httpx.AsyncClient), "alice") + file_response = _http_response(b"fallback", status_code=200) + file_response.headers["content-type"] = "text/plain" + client._make_request = mocker.AsyncMock( + side_effect=[_request_error(), file_response] + ) + + content, _ = await client.read_file("notes.txt") + + assert content == b"fallback" + calls = client._make_request.await_args_list + assert calls[0].args[:2] == ("PROPFIND", "/remote.php/dav/") + assert calls[1].args[:2] == ("GET", "/remote.php/dav/files/alice/notes.txt") + + +async def test_webdav_successful_discovery_is_cached_per_instance(mocker): + client = WebDAVClient(mocker.AsyncMock(spec=httpx.AsyncClient), "alice") + client._make_request = mocker.AsyncMock( + side_effect=[ + _http_response(_principal_body("alice_1234")), + _http_response(_empty_webdav_dir("alice_1234")), + _http_response(_empty_webdav_dir("alice_1234")), + ] + ) + + await client.list_directory("one") + await client.list_directory("two") + + calls = client._make_request.await_args_list + assert [call.args[:2] for call in calls] == [ + ("PROPFIND", "/remote.php/dav/"), + ("PROPFIND", "/remote.php/dav/files/alice_1234/one/"), + ("PROPFIND", "/remote.php/dav/files/alice_1234/two/"), + ] + + +async def test_webdav_principal_href_is_unquoted(mocker): + client = WebDAVClient(mocker.AsyncMock(spec=httpx.AsyncClient), "alice") + file_response = _http_response(b"mailbox", status_code=200) + file_response.headers["content-type"] = "text/plain" + client._make_request = mocker.AsyncMock( + side_effect=[ + _http_response(_principal_body("alice%40example.com")), + file_response, + ] + ) + + content, _ = await client.read_file("notes.txt") + + assert content == b"mailbox" + assert client._principal_id == "alice@example.com" + assert client._make_request.await_args_list[1].args[:2] == ( + "GET", + "/remote.php/dav/files/alice@example.com/notes.txt", + ) + + +async def test_webdav_missing_principal_href_falls_back_without_caching(mocker): + client = WebDAVClient(mocker.AsyncMock(spec=httpx.AsyncClient), "alice") + first_file = _http_response(b"fallback", status_code=200) + first_file.headers["content-type"] = "text/plain" + second_file = _http_response(b"discovered", status_code=200) + second_file.headers["content-type"] = "text/plain" + client._make_request = mocker.AsyncMock( + side_effect=[ + _http_response(_principal_body_without_href()), + first_file, + _http_response(_principal_body("alice_1234")), + second_file, + ] + ) + + first_content, _ = await client.read_file("one.txt") + second_content, _ = await client.read_file("two.txt") + + assert first_content == b"fallback" + assert second_content == b"discovered" + calls = client._make_request.await_args_list + assert [call.args[:2] for call in calls] == [ + ("PROPFIND", "/remote.php/dav/"), + ("GET", "/remote.php/dav/files/alice/one.txt"), + ("PROPFIND", "/remote.php/dav/"), + ("GET", "/remote.php/dav/files/alice_1234/two.txt"), + ] + + +async def test_carddav_public_method_discovers_divergent_principal(mocker): + client = ContactsClient(mocker.AsyncMock(spec=httpx.AsyncClient), "alice") + client._make_request = mocker.AsyncMock( + side_effect=[ + _http_response(_principal_body("alice_1234")), + _http_response(_addressbooks_body("alice_1234")), + ] + ) + + addressbooks = await client.list_addressbooks() + + assert addressbooks[0]["name"] == "contacts" + calls = client._make_request.await_args_list + assert calls[0].args[:2] == ("PROPFIND", "/remote.php/dav/") + assert calls[1].args[:2] == ( + "PROPFIND", + "/remote.php/dav/addressbooks/users/alice_1234", + ) + + +def _calendar_client_with_principal( + mocker, principal_id: str, *, calendar_home_id: str | None = None +): + mock_dav_client = mocker.patch( + "nextcloud_mcp_server.client.calendar.AsyncDAVClient" + ) + dav_client = mock_dav_client.return_value + principal = SimpleNamespace( + url=f"https://cloud.example.org/remote.php/dav/principals/users/{principal_id}/" + ) + if calendar_home_id is not None: + principal.calendar_home_set = SimpleNamespace( + url=f"https://cloud.example.org/remote.php/dav/calendars/{calendar_home_id}/" + ) + dav_client.get_principal = mocker.AsyncMock(return_value=principal) + response_id = calendar_home_id or principal_id + dav_client.propfind = mocker.AsyncMock( + return_value=mocker.Mock(raw=_calendar_multistatus(response_id)) + ) + + from nextcloud_mcp_server.client.calendar import CalendarClient + + client = CalendarClient("https://cloud.example.org", "alice", password=_APP_PW) + return client, dav_client + + +async def test_caldav_list_calendars_discovers_divergent_principal(mocker): + client, dav_client = _calendar_client_with_principal(mocker, "alice_1234") + + calendars = await client.list_calendars() + + assert [calendar["name"] for calendar in calendars] == ["personal"] + dav_client.get_principal.assert_awaited_once() + dav_client.propfind.assert_awaited_once() + assert ( + dav_client.propfind.await_args.args[0] + == "https://cloud.example.org/remote.php/dav/calendars/alice_1234/" + ) + + +async def test_caldav_prefers_discovered_calendar_home_set(mocker): + client, dav_client = _calendar_client_with_principal( + mocker, "alice_1234", calendar_home_id="calendar_home_5678" + ) + + calendars = await client.list_calendars() + + assert [calendar["name"] for calendar in calendars] == ["personal"] + dav_client.get_principal.assert_awaited_once() + assert ( + dav_client.propfind.await_args.args[0] + == "https://cloud.example.org/remote.php/dav/calendars/calendar_home_5678/" + ) + + +async def test_caldav_uses_async_safe_calendar_home_property_lookup(mocker): + mock_dav_client = mocker.patch( + "nextcloud_mcp_server.client.calendar.AsyncDAVClient" + ) + dav_client = mock_dav_client.return_value + + class AsyncPrincipal: + url = "https://cloud.example.org/remote.php/dav/principals/users/alice_1234/" + + async def get_property(self, prop): + return "/remote.php/dav/calendars/calendar_home_5678/" + + @property + def calendar_home_set(self): + raise TypeError("argument of type 'coroutine' is not iterable") + + dav_client.get_principal = mocker.AsyncMock(return_value=AsyncPrincipal()) + dav_client.propfind = mocker.AsyncMock( + return_value=mocker.Mock(raw=_calendar_multistatus("calendar_home_5678")) + ) + + from nextcloud_mcp_server.client.calendar import CalendarClient + + client = CalendarClient("https://cloud.example.org", "alice", password=_APP_PW) + + await client.list_calendars() + + assert ( + dav_client.propfind.await_args.args[0] + == "https://cloud.example.org/remote.php/dav/calendars/calendar_home_5678/" + ) + + +async def test_caldav_falls_back_to_calendar_home_property_when_lookup_empty(mocker): + mock_dav_client = mocker.patch( + "nextcloud_mcp_server.client.calendar.AsyncDAVClient" + ) + dav_client = mock_dav_client.return_value + + class Principal: + url = "https://cloud.example.org/remote.php/dav/principals/users/alice_1234/" + + async def get_property(self, prop): + return None + + @property + def calendar_home_set(self): + return SimpleNamespace( + url="https://cloud.example.org/remote.php/dav/calendars/calendar_home_5678/" + ) + + dav_client.get_principal = mocker.AsyncMock(return_value=Principal()) + dav_client.propfind = mocker.AsyncMock( + return_value=mocker.Mock(raw=_calendar_multistatus("calendar_home_5678")) + ) + + from nextcloud_mcp_server.client.calendar import CalendarClient + + client = CalendarClient("https://cloud.example.org", "alice", password=_APP_PW) + + await client.list_calendars() + + assert ( + dav_client.propfind.await_args.args[0] + == "https://cloud.example.org/remote.php/dav/calendars/calendar_home_5678/" + ) + + +async def test_caldav_discovery_failure_falls_back_to_username(mocker): + client, dav_client = _calendar_client_with_principal(mocker, "alice") + dav_client.get_principal.side_effect = caldav_error.DAVError("temporary failure") + + calendars = await client.list_calendars() + + assert [calendar["name"] for calendar in calendars] == ["personal"] + dav_client.get_principal.assert_awaited_once() + assert ( + dav_client.propfind.await_args.args[0] + == "https://cloud.example.org/remote.php/dav/calendars/alice/" + ) + + +async def test_caldav_discovery_failure_retries_on_next_call(mocker): + client, dav_client = _calendar_client_with_principal(mocker, "alice_1234") + principal = dav_client.get_principal.return_value + dav_client.get_principal.side_effect = [ + caldav_error.DAVError("temporary failure"), + principal, + ] + dav_client.propfind.side_effect = [ + mocker.Mock(raw=_calendar_multistatus("alice")), + mocker.Mock(raw=_calendar_multistatus("alice_1234")), + ] + + first = await client.list_calendars() + second = await client.list_calendars() + + assert [calendar["name"] for calendar in first] == ["personal"] + assert [calendar["name"] for calendar in second] == ["personal"] + assert dav_client.get_principal.await_count == 2 + assert [call.args[0] for call in dav_client.propfind.await_args_list] == [ + "https://cloud.example.org/remote.php/dav/calendars/alice/", + "https://cloud.example.org/remote.php/dav/calendars/alice_1234/", + ] + + +async def test_caldav_create_calendar_uses_discovered_home_url(mocker): + client, dav_client = _calendar_client_with_principal(mocker, "alice_1234") + dav_client.mkcalendar = mocker.AsyncMock(return_value=SimpleNamespace(status=201)) + client._wait_for_calendar_propagation = mocker.AsyncMock() + + await client.create_calendar("team") + + dav_client.get_principal.assert_awaited_once() + assert ( + dav_client.mkcalendar.await_args.args[0] + == "https://cloud.example.org/remote.php/dav/calendars/alice_1234/team/" + ) + + +async def test_caldav_delete_calendar_uses_discovered_home_url(mocker): + client, dav_client = _calendar_client_with_principal(mocker, "alice_1234") + dav_client.delete = mocker.AsyncMock() + + await client.delete_calendar("team") + + dav_client.get_principal.assert_awaited_once() + dav_client.delete.assert_awaited_once_with( + "https://cloud.example.org/remote.php/dav/calendars/alice_1234/team/" + ) + + +async def test_caldav_event_operations_use_discovered_home_url(mocker): + client, dav_client = _calendar_client_with_principal(mocker, "alice_1234") + fake_calendar = mocker.Mock() + fake_calendar.save_event = mocker.AsyncMock( + return_value=SimpleNamespace(url="https://cloud.example.org/event.ics") + ) + mock_calendar = mocker.patch( + "nextcloud_mcp_server.client.calendar.AsyncCalendar", + return_value=fake_calendar, + ) + + await client.create_event("team", {"title": "Planning"}) + + dav_client.get_principal.assert_awaited_once() + assert ( + mock_calendar.call_args.kwargs["url"] + == "https://cloud.example.org/remote.php/dav/calendars/alice_1234/team/" + ) diff --git a/tests/unit/client/test_webdav.py b/tests/unit/client/test_webdav.py index 3d981c242..f38aa1406 100644 --- a/tests/unit/client/test_webdav.py +++ b/tests/unit/client/test_webdav.py @@ -561,6 +561,7 @@ async def test_read_file_encodes_special_chars(mocker): """ mock_http_client = AsyncMock() client = WebDAVClient(mock_http_client, "testuser") + client._principal_discovered = True mock_response = AsyncMock() mock_response.content = b"%PDF-1.4 data" @@ -587,6 +588,7 @@ async def test_read_file_ascii_path_unchanged(mocker): """A plain ASCII path must pass through unchanged (no spurious encoding).""" mock_http_client = AsyncMock() client = WebDAVClient(mock_http_client, "testuser") + client._principal_discovered = True mock_response = AsyncMock() mock_response.content = b"data" @@ -693,6 +695,7 @@ async def test_move_resource_encodes_destination_header(mocker): """The MOVE Destination header must be percent-encoded too (card 309).""" mock_http_client = AsyncMock() client = WebDAVClient(mock_http_client, "testuser") + client._principal_discovered = True mock_response = AsyncMock() mock_response.status_code = 201 @@ -714,6 +717,7 @@ async def test_copy_resource_encodes_destination_header(mocker): """The COPY Destination header must be percent-encoded too (card 309).""" mock_http_client = AsyncMock() client = WebDAVClient(mock_http_client, "testuser") + client._principal_discovered = True mock_response = AsyncMock() mock_response.status_code = 201