diff --git a/nextcloud_mcp_server/client/contacts.py b/nextcloud_mcp_server/client/contacts.py index 75359f27..ae0aef81 100644 --- a/nextcloud_mcp_server/client/contacts.py +++ b/nextcloud_mcp_server/client/contacts.py @@ -217,6 +217,63 @@ class ContactsClient(BaseNextcloudClient): """Helper to get the base CardDAV path for contacts.""" return f"/remote.php/dav/addressbooks/users/{self.username}" + async def _list_object_names(self, addressbook: str) -> list[str]: + """Return the CardDAV object filenames stored in ``addressbook``. + + A lightweight ``PROPFIND`` (Depth: 1, ``getetag`` only) over the + collection. The DAV object filename is independent of the vCard's + internal ``UID`` and is *not* guaranteed to be ``.vcf`` — see + issue #874 — so callers that need to address a specific object must + discover its real name rather than constructing one. + """ + carddav_path = self._get_carddav_base_path() + propfind_body = """ + """ + headers = { + "Depth": "1", + "Content-Type": "application/xml", + "Accept": "application/xml", + } + response = await self._make_request( + "PROPFIND", + f"{carddav_path}/{addressbook}", + content=propfind_body, + headers=headers, + ) + + ns = {"d": "DAV:"} + root = ET.fromstring(response.content) + names: list[str] = [] + for response_elem in root.findall(".//d:response", ns): + href = response_elem.find(".//d:href", ns) + if href is None or not href.text: + continue + # The collection itself is reported with a trailing slash; skip it + # so only contact objects remain. + if href.text.endswith("/"): + continue + names.append(href.text.rstrip("/").split("/")[-1]) + return names + + async def _resolve_object_name(self, addressbook: str, uid: str) -> str | None: + """Map a surfaced contact id back to its real CardDAV object filename. + + ``list_contacts`` surfaces ``vcard_id`` with any ``.vcf`` suffix + stripped, so reverse that transform here: return the object whose + filename reduces to ``uid``. The conventional ``.vcf`` is + preferred when present (deterministic for the common case), otherwise + the first matching name is returned. ``None`` when no object matches. + """ + candidates = [ + name + for name in await self._list_object_names(addressbook) + if name.replace(".vcf", "") == uid + ] + if not candidates: + return None + conventional = f"{uid}.vcf" + return conventional if conventional in candidates else candidates[0] + async def list_addressbooks(self): """List all available addressbooks for the user.""" @@ -338,9 +395,17 @@ class ContactsClient(BaseNextcloudClient): await self._make_request("PUT", url, content=vcard, headers=headers) async def delete_contact(self, *, addressbook: str, uid: str): - """Delete a contact.""" + """Delete a contact regardless of its CardDAV object filename. + + The object filename is independent of the vCard ``UID`` and may lack a + ``.vcf`` extension (e.g. the stock ``default`` sample contact), so the + real object name is resolved before deleting rather than assuming + ``.vcf`` (issue #874). Falls back to the conventional name when no + object matches so a genuinely missing contact still surfaces a 404. + """ carddav_path = self._get_carddav_base_path() - url = f"{carddav_path}/{addressbook}/{uid}.vcf" + object_name = await self._resolve_object_name(addressbook, uid) or f"{uid}.vcf" + url = f"{carddav_path}/{addressbook}/{object_name}" await self._make_request("DELETE", url) async def update_contact( @@ -353,7 +418,10 @@ class ContactsClient(BaseNextcloudClient): ): """Update an existing contact while preserving all existing properties.""" carddav_path = self._get_carddav_base_path() - url = f"{carddav_path}/{addressbook}/{uid}.vcf" + # Resolve the real object filename (may differ from ``.vcf``) so the + # GET and the PUT target the same resource — see issue #874. + object_name = await self._resolve_object_name(addressbook, uid) or f"{uid}.vcf" + url = f"{carddav_path}/{addressbook}/{object_name}" # Canonicalise aliases up front so both code paths (merge + fallback) agree. contact_data = _normalize_contact_data(contact_data) @@ -362,8 +430,8 @@ class ContactsClient(BaseNextcloudClient): raw_vcard_content = "" if not etag: try: - raw_vcard_content, current_etag = await self._get_raw_vcard( - addressbook, uid + raw_vcard_content, current_etag = await self._fetch_raw_vcard( + addressbook, object_name ) etag = current_etag except Exception: @@ -433,12 +501,18 @@ class ContactsClient(BaseNextcloudClient): # logger.info("# Skip non-addressbook resources") # continue - # Extract vcard id from href - vcard_id = href_text.rstrip("/").split("/")[-1] - if not vcard_id: + # The real CardDAV object: its full DAV path and bare filename. The + # filename is independent of the vCard UID and may lack a ``.vcf`` + # extension, so preserve it verbatim for callers that need to + # address the object reliably (issue #874). + object_path = href_text + object_name = href_text.rstrip("/").split("/")[-1] + if not object_name: logger.info("Skip missing vcard_id") continue - vcard_id = vcard_id.replace(".vcf", "") + # ``vcard_id`` keeps the historical ``.vcf``-stripped form for + # backward compatibility with callers that use it as the contact id. + vcard_id = object_name.replace(".vcf", "") # Get properties propstat = response_elem.find(".//d:propstat", ns) @@ -477,6 +551,8 @@ class ContactsClient(BaseNextcloudClient): contacts.append( { "vcard_id": vcard_id, + "object_path": object_path, + "object_name": object_name, "getetag": getetag, "contact": { "fullname": contact.fn, @@ -501,16 +577,27 @@ class ContactsClient(BaseNextcloudClient): return contacts async def _get_raw_vcard(self, addressbook: str, uid: str) -> tuple[str, str]: - """Get raw vCard content for a contact without parsing.""" + """Get raw vCard content for a contact without parsing. + + Resolves the real object filename first (it may not be ``.vcf`` — + issue #874) before fetching. + """ + object_name = await self._resolve_object_name(addressbook, uid) or f"{uid}.vcf" + return await self._fetch_raw_vcard(addressbook, object_name) + + async def _fetch_raw_vcard( + self, addressbook: str, object_name: str + ) -> tuple[str, str]: + """Fetch raw vCard content + etag for an already-resolved object name.""" carddav_path = self._get_carddav_base_path() - url = f"{carddav_path}/{addressbook}/{uid}.vcf" + url = f"{carddav_path}/{addressbook}/{object_name}" try: response = await self._make_request("GET", url) etag = response.headers.get("etag", "") return response.text, etag except Exception as e: - logger.error("Error getting raw vCard for %s: %s", uid, e) + logger.error("Error getting raw vCard for %s: %s", object_name, e) raise def _merge_vcard_properties( diff --git a/nextcloud_mcp_server/models/contacts.py b/nextcloud_mcp_server/models/contacts.py index d3e0fe37..b018f03c 100644 --- a/nextcloud_mcp_server/models/contacts.py +++ b/nextcloud_mcp_server/models/contacts.py @@ -34,6 +34,14 @@ class Contact(BaseModel): """Model for a Nextcloud contact.""" uid: str = Field(description="Contact UID") + resource_path: Optional[str] = Field( + None, + description=( + "Full CardDAV path to the underlying object. The object filename is " + "independent of the UID and may lack a '.vcf' extension; this is the " + "authoritative path for the resource." + ), + ) fn: str = Field(description="Full name (formatted name)") given_name: Optional[str] = Field(None, description="Given name") family_name: Optional[str] = Field(None, description="Family name") diff --git a/nextcloud_mcp_server/server/contacts.py b/nextcloud_mcp_server/server/contacts.py index 7a9fef3e..b5a16571 100644 --- a/nextcloud_mcp_server/server/contacts.py +++ b/nextcloud_mcp_server/server/contacts.py @@ -102,6 +102,7 @@ def _raw_contact_to_model(raw: dict) -> Contact: return Contact( uid=raw["vcard_id"], + resource_path=raw.get("object_path"), fn=contact_info.get("fullname", ""), etag=raw.get("getetag"), organization=contact_info.get("org"), diff --git a/tests/client/contacts/test_contacts_operations.py b/tests/client/contacts/test_contacts_operations.py index c894a52b..d55f01a0 100644 --- a/tests/client/contacts/test_contacts_operations.py +++ b/tests/client/contacts/test_contacts_operations.py @@ -88,6 +88,58 @@ async def test_full_contact_workflow( assert contact_uid not in contact_uids +async def test_delete_contact_without_vcf_extension( + nc_client: NextcloudClient, temporary_addressbook: str +): + """Regression for issue #874: a contact whose CardDAV object filename has no + ``.vcf`` extension (like the stock ``default`` sample contact) must still be + deletable via the public API. + + ``create_contact`` can't reproduce the precondition — it always PUTs to + ``.vcf`` — so seed the object directly at a bare path, then exercise + ``list_contacts`` (real path exposure) and ``delete_contact`` (resolution). + """ + addressbook = temporary_addressbook + contacts = nc_client.contacts + object_name = f"noext-{uuid.uuid4().hex[:8]}" # filename WITHOUT .vcf + carddav_path = contacts._get_carddav_base_path() + vcard = ( + "BEGIN:VCARD\r\nVERSION:3.0\r\n" + f"UID:{object_name}\r\nFN:No Ext\r\nEMAIL:noext@example.com\r\n" + "END:VCARD\r\n" + ) + + # Seed the pathological object at a path that lacks the .vcf extension. + await contacts._make_request( + "PUT", + f"{carddav_path}/{addressbook}/{object_name}", + content=vcard, + headers={"Content-Type": "text/vcard; charset=utf-8"}, + ) + + try: + # list_contacts surfaces it and exposes the real object path/name. + listed = await contacts.list_contacts(addressbook=addressbook) + match = next((c for c in listed if c["vcard_id"] == object_name), None) + assert match is not None, "seeded no-.vcf contact not listed" + assert match["object_name"] == object_name # no .vcf extension + assert match["object_path"].endswith(f"/{addressbook}/{object_name}") + + # Delete via the public API — pre-#874 this hit .vcf and 404'd. + await contacts.delete_contact(addressbook=addressbook, uid=object_name) + + remaining = await contacts.list_contacts(addressbook=addressbook) + assert object_name not in [c["vcard_id"] for c in remaining] + finally: + # Best-effort cleanup in case the assertions above failed before delete. + try: + await contacts._make_request( + "DELETE", f"{carddav_path}/{addressbook}/{object_name}" + ) + except Exception: + pass + + async def test_create_contact_persists_all_documented_fields( nc_client: NextcloudClient, temporary_addressbook: str ): diff --git a/tests/unit/client/test_contacts.py b/tests/unit/client/test_contacts.py index d9db7641..c95de897 100644 --- a/tests/unit/client/test_contacts.py +++ b/tests/unit/client/test_contacts.py @@ -10,6 +10,7 @@ from datetime import date import pytest from nextcloud_mcp_server.client.contacts import ( + ContactsClient, _build_contact_from_data, _first_custom, _normalize_contact_data, @@ -202,6 +203,100 @@ def test_missing_fn_logs_warning(caplog): assert any("fn" in r.message.lower() for r in caplog.records) +def _multistatus(*object_names: str, addressbook: str = "contacts") -> bytes: + """Build a minimal PROPFIND multistatus body for ``_list_object_names``. + + Includes the collection itself (trailing-slash href) plus one ``response`` + per object name so the parser's collection-skipping is exercised. + """ + base = f"/remote.php/dav/addressbooks/users/testuser/{addressbook}" + responses = [ + f"{base}/" + ""col"" + "HTTP/1.1 200 OK" + ] + for name in object_names: + responses.append( + f"{base}/{name}" + ""e"" + "HTTP/1.1 200 OK" + ) + body = ( + '' + '' + "".join(responses) + "" + ) + return body.encode() + + +class TestObjectNameResolution: + """Issue #874: the CardDAV object filename is independent of the vCard UID + and may lack a ``.vcf`` extension, so write paths must resolve the real + object name instead of assuming ``.vcf``. + """ + + @staticmethod + def _client(mocker, multistatus: bytes) -> ContactsClient: + client = ContactsClient.__new__(ContactsClient) # no HTTP / no __init__ + client.username = "testuser" + response = mocker.Mock() + response.content = multistatus + client._make_request = mocker.AsyncMock(return_value=response) + return client + + async def test_list_object_names_skips_collection(self, mocker): + client = self._client(mocker, _multistatus("alice.vcf", "default")) + names = await client._list_object_names("contacts") + assert names == ["alice.vcf", "default"] # collection href omitted + + async def test_resolves_conventional_vcf_name(self, mocker): + client = self._client(mocker, _multistatus("alice.vcf")) + assert await client._resolve_object_name("contacts", "alice") == "alice.vcf" + + async def test_resolves_name_without_vcf_extension(self, mocker): + """The #874 case: object stored at ``.../default`` (no extension).""" + client = self._client(mocker, _multistatus("default")) + assert await client._resolve_object_name("contacts", "default") == "default" + + async def test_prefers_vcf_when_both_present(self, mocker): + """Deterministic tie-break: ``.vcf`` wins over a bare ````.""" + client = self._client(mocker, _multistatus("dup", "dup.vcf")) + assert await client._resolve_object_name("contacts", "dup") == "dup.vcf" + + async def test_returns_none_when_no_match(self, mocker): + client = self._client(mocker, _multistatus("alice.vcf")) + assert await client._resolve_object_name("contacts", "missing") is None + + 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" + mocker.patch.object( + client, "_resolve_object_name", mocker.AsyncMock(return_value="default") + ) + make_request = mocker.patch.object(client, "_make_request", mocker.AsyncMock()) + await client.delete_contact(addressbook="contacts", uid="default") + make_request.assert_awaited_once_with( + "DELETE", + "/remote.php/dav/addressbooks/users/testuser/contacts/default", + ) + + async def test_delete_falls_back_to_vcf_when_unresolved(self, mocker): + """A genuinely missing contact resolves to None → fall back to the + conventional name so the caller still gets a clean 404 from the DELETE. + """ + client = ContactsClient.__new__(ContactsClient) + client.username = "testuser" + mocker.patch.object( + client, "_resolve_object_name", mocker.AsyncMock(return_value=None) + ) + make_request = mocker.patch.object(client, "_make_request", mocker.AsyncMock()) + await client.delete_contact(addressbook="contacts", uid="ghost") + make_request.assert_awaited_once_with( + "DELETE", + "/remote.php/dav/addressbooks/users/testuser/contacts/ghost.vcf", + ) + + class TestFirstCustom: """``_first_custom`` is the read-side companion to PR #719 — it pulls ORG / TITLE / unencoded PHOTO out of pythonvCard4's ``custom`` dict because diff --git a/tests/unit/test_response_models.py b/tests/unit/test_response_models.py index fd9dc84e..006b199f 100644 --- a/tests/unit/test_response_models.py +++ b/tests/unit/test_response_models.py @@ -313,6 +313,35 @@ def test_contact_mapping_preserves_email_birthday_nickname(): assert contact.custom_fields["nickname"] == "JD" +@pytest.mark.unit +def test_contact_mapping_exposes_resource_path(): + """Issue #874: the real CardDAV object path must surface on the model so + callers can address objects whose filename isn't ``.vcf``. + """ + raw_contact = { + "vcard_id": "default", + "object_path": "/remote.php/dav/addressbooks/users/admin/contacts/default", + "object_name": "default", + "getetag": '"etag-val"', + "contact": {"fullname": "No Ext"}, + } + + contact = _map_contact(raw_contact) + + assert contact.uid == "default" + assert ( + contact.resource_path + == "/remote.php/dav/addressbooks/users/admin/contacts/default" + ) + + +@pytest.mark.unit +def test_contact_mapping_resource_path_optional(): + """Mapping must not require ``object_path`` (older callers / partial dicts).""" + contact = _map_contact({"vcard_id": "x", "contact": {"fullname": "X"}}) + assert contact.resource_path is None + + @pytest.mark.unit def test_contact_mapping_birthday_datetime_date_object(): """Test that a datetime.date birthday is converted to ISO string.