From ca4c2cf88e267da7fb7050f9734b708ad62d1a05 Mon Sep 17 00:00:00 2001 From: Oleksandr Piskun Date: Tue, 29 Sep 2026 19:19:44 +0000 Subject: [PATCH 1/4] Keep team folders from being deleted along with their teams Signed-off-by: Oleksandr Piskun --- .github/workflows/tests-integration.yml | 2 + PROGRESS.md | 7 +- README.md | 8 +- src/nc_mcp_server/client.py | 4 +- src/nc_mcp_server/tools/circles.py | 118 ++++++++++++++++++--- src/nc_mcp_server/tools/collectives.py | 27 ++++- src/nc_mcp_server/tools/users.py | 3 + tests/integration/test_circles.py | 84 ++++++++++++++- tests/integration/test_collectives.py | 22 +++- tests/test_circles.py | 134 +++++++++++++++++++++++- tests/test_collectives_pages.py | 57 ++++++++++ 11 files changed, 433 insertions(+), 33 deletions(-) diff --git a/.github/workflows/tests-integration.yml b/.github/workflows/tests-integration.yml index b0422c5..54bd57d 100644 --- a/.github/workflows/tests-integration.yml +++ b/.github/workflows/tests-integration.yml @@ -103,6 +103,8 @@ jobs: $OCC "php occ app:install forms" || echo "forms already installed" $OCC "php occ app:install cospend" || echo "cospend already installed" $OCC "php occ app:enable circles" || echo "circles enable failed (may not be shipped)" + # Team folders: on Nextcloud 35, Circles gives new teams a folder that deleting the team deletes. + $OCC "php occ app:install groupfolders" # Dovecot refuses plaintext logins, and its image ships a self-signed certificate for STARTTLS. $OCC "php occ config:system:set app.mail.verify-tls-peer --value=false --type=boolean" SMTP4DEV_IP=$(docker inspect ${{ job.services.smtp4dev.id }} --format '{{range .NetworkSettings.Networks}}{{.IPAddress}}{{end}}') diff --git a/PROGRESS.md b/PROGRESS.md index f590854..1704af2 100644 --- a/PROGRESS.md +++ b/PROGRESS.md @@ -57,6 +57,7 @@ - [x] Activity search: get_activity takes search, start, end and actor on Nextcloud 35 (refused on older servers, which would ignore them) and returns an empty page instead of failing at the end of the feed; list_activity_filters, get_activity_counts (2026-09-26) - [x] search_files matches % and _ in the query literally instead of as database wildcards (2026-09-26) - [x] update_circle_member_level explains when Circles cannot transfer ownership (SQLite or PostgreSQL before nextcloud/circles#2916) instead of passing on the database error (2026-09-26) +- [x] Team folders (Nextcloud 35 with the Team folders app): create_circle makes one only with team_folder=true; delete_circle, leave_circle and delete_collective(delete_team) refuse to delete a team folder and its files unless delete_team_folder=true (2026-09-29) ### In Progress @@ -102,7 +103,7 @@ | Shares | 8 | 86 | | System Tags | 6 | 22 | | Mail | 10 | 102 | -| Collectives | 25 | 95 | +| Collectives | 25 | 100 | | App Management | 4 | 14 | | Calendar | 6 | 44 | | Contacts | 6 | 78 | @@ -118,11 +119,11 @@ | File Helpers | — | 36 | | File Reminders | 3 | 22 | | Forms | 25 | 34 | -| Circles | 14 | 37 | +| Circles | 14 | 58 | | Cospend | 16 | 41 | | Flow | 5 | 58 | | Pagination | — | 28 | -| **Total** | **223** | **1789** | +| **Total** | **223** | **1815** | The test counts are what pytest collects, recomputed with `python scripts/sync_progress.py --write`, which also fails when a test file is not assigned to a row. diff --git a/README.md b/README.md index 0a135e6..11a4ea7 100644 --- a/README.md +++ b/README.md @@ -448,7 +448,7 @@ instead of the placeholders Talk stores (`{mention-user1}`, `{file}`). | `share_collective` | write | Create a public link to a collective or one page, optionally editable and with a password | | `update_collective_share` | write | Change a public link's editing and password | | `trash_collective` | destructive | Move a collective to trash | -| `delete_collective` | destructive | Permanently delete a trashed collective, optionally with its team | +| `delete_collective` | destructive | Permanently delete a trashed collective, optionally with its team (and, when told, the team's folder) | | `trash_collective_page` | destructive | Move a page to trash | | `delete_collective_page` | destructive | Permanently delete a trashed page | | `delete_collective_tag` | destructive | Delete a tag, taking it off its pages | @@ -494,15 +494,15 @@ instead of the placeholders Talk stores (`{mention-user1}`, `{file}`). | `get_circle` | read | Get a single circle including the current user's membership | | `list_circle_members` | read | List members of a circle | | `search_circles` | read | Search circles and candidate members (users/groups/mail) by term | -| `create_circle` | write | Create a circle; caller becomes owner | +| `create_circle` | write | Create a circle; caller becomes owner. Optionally with a team folder (Nextcloud 35 + Team folders app) | | `update_circle_name` | write | Rename a circle | | `update_circle_description` | write | Update description | | `update_circle_config` | write | Update config bitmask (VISIBLE, OPEN, INVITE, HIDDEN, etc.) | | `add_circle_member` | write | Add a user, group, email, or nested circle as a member | | `update_circle_member_level` | write | Promote/demote a member (member/moderator/admin/owner) | | `join_circle` | write | Join an open circle | -| `leave_circle` | write | Leave a circle | -| `delete_circle` | destructive | Delete a circle | +| `leave_circle` | destructive | Leave a circle. The owner's leave passes ownership on, or destroys the circle when no one else is left; refuses to lose a team folder unless told | +| `delete_circle` | destructive | Delete a circle; refuses to delete its team folder and files unless told | | `remove_circle_member` | destructive | Kick a member | ### Cospend diff --git a/src/nc_mcp_server/client.py b/src/nc_mcp_server/client.py index 1821d87..2ccf005 100644 --- a/src/nc_mcp_server/client.py +++ b/src/nc_mcp_server/client.py @@ -261,7 +261,9 @@ async def renew_session(self) -> None: """Log in again, so the following requests run in a new server session. Nextcloud does not show an existing session the federated shares accepted during it, - while a session that starts after they were set up for the user sees them. + while a session that starts after they were set up for the user sees them. A login also + refreshes the user's cached mounts, which otherwise miss a new team folder for up to + five minutes (fs_mount_cache_duration). """ await self._reset_session() diff --git a/src/nc_mcp_server/tools/circles.py b/src/nc_mcp_server/tools/circles.py index 53270fa..bc9d59d 100644 --- a/src/nc_mcp_server/tools/circles.py +++ b/src/nc_mcp_server/tools/circles.py @@ -6,7 +6,7 @@ from mcp.server.fastmcp import FastMCP from ..annotations import ADDITIVE, ADDITIVE_IDEMPOTENT, DESTRUCTIVE, READONLY -from ..client import NextcloudError +from ..client import NextcloudClient, NextcloudError from ..permissions import PermissionLevel, require_permission from ..state import get_client @@ -25,6 +25,36 @@ "owner": 9, } +NO_TEAM_FOLDER = ( + "No team folder was created. Team folders need Nextcloud 35 or later with the Team folders app, are not made " + "for personal teams, and an admin can turn them off." +) + + +async def get_team_folder(client: NextcloudClient, circle_id: str) -> dict[str, Any] | None: + """The team folder of a team (Nextcloud 35+ with the Team folders app), or None if it has none. + + Deleting a team deletes its team folder with every file in it. Only members may ask; other errors propagate, + so callers guarding a deletion refuse instead of guessing. + """ + try: + folder: dict[str, Any] = await client.ocs_get(f"apps/circles/teams/{circle_id}/folder") + except NextcloudError as e: + # 404 covers a team without a folder, a server without a team folder provider, and servers before 35 + # that have no such route. + if e.status_code == 404: + return None + raise + return folder + + +def refuse_team_folder_loss(action: str, folder: dict[str, Any]) -> ValueError: + """The error for an action that would delete a team folder the caller did not agree to lose.""" + return ValueError( + f"{action} would also delete the team folder '{folder.get('mountPoint')}' and every file in it, for all " + "members. Nothing was changed. Move out what should be kept, or call again with delete_team_folder=true." + ) + def _register_read_tools(mcp: FastMCP) -> None: @mcp.tool(annotations=READONLY) @@ -114,7 +144,7 @@ async def search_circles(term: str) -> str: def _register_circle_writes(mcp: FastMCP) -> None: @mcp.tool(annotations=ADDITIVE) @require_permission(PermissionLevel.WRITE) - async def create_circle(name: str, personal: bool = False, local: bool = False) -> str: + async def create_circle(name: str, personal: bool = False, local: bool = False, team_folder: bool = False) -> str: """Create a new circle (team). Args: @@ -123,18 +153,33 @@ async def create_circle(name: str, personal: bool = False, local: bool = False) owner (useful for private contact groups). local: If True, mark the circle as local (not federated to other instances) even when global scope is enabled. + team_folder: If True, also create a team folder: a shared folder + named after the team that every member sees in Files. Needs + Nextcloud 35 or later with the Team folders app. Deleting the + team later deletes the folder and its files too. Returns: JSON of the new circle including its generated id. The caller is - automatically added as owner (level=9). + automatically added as owner (level=9). With team_folder, it also + has `team_folder` (id, mountPoint, quota) or null with + `team_folder_note` saying why none was created. """ client = get_client() - body: dict[str, Any] = {"name": name} + # Circles on Nextcloud 35 creates a team folder unless told not to; older servers ignore the flag. + body: dict[str, Any] = {"name": name, "createTeamFolder": team_folder} if personal: body["personal"] = True if local: body["local"] = True data = await client.ocs_post_json("apps/circles/circles", json_data=body) + if team_folder: + data["team_folder"] = await get_team_folder(client, data["id"]) + if data["team_folder"] is None: + data["team_folder_note"] = NO_TEAM_FOLDER + else: + # The user's mounts are cached until the next login, so without one the file tools would not + # find the new folder for minutes. + await client.renew_session() return json.dumps(data) @mcp.tool(annotations=ADDITIVE_IDEMPOTENT) @@ -215,6 +260,8 @@ async def update_circle_config(circle_id: str, config: int) -> str: ) return json.dumps(data) + +def _register_membership_tools(mcp: FastMCP) -> None: @mcp.tool(annotations=ADDITIVE_IDEMPOTENT) @require_permission(PermissionLevel.WRITE) async def join_circle(circle_id: str) -> str: @@ -236,26 +283,33 @@ async def join_circle(circle_id: str) -> str: @mcp.tool(annotations=DESTRUCTIVE) @require_permission(PermissionLevel.DESTRUCTIVE) - async def leave_circle(circle_id: str) -> str: + async def leave_circle(circle_id: str, delete_team_folder: bool = False) -> str: """Leave a circle the current user is a member of. - IMPORTANT: If the current user is the sole owner, leaving destroys - the entire circle (server behavior — no confirmation prompt). To - avoid this, promote another member to owner via - update_circle_member_level(..., level="owner") first. Because of - this implicit-destroy behavior, this tool requires DESTRUCTIVE - permission (matching leave_conversation in Talk). + IMPORTANT: When the owner leaves, the server makes another member the + owner (the highest level, then the longest-standing). When the owner + is the last member, leaving destroys the entire circle, with no + confirmation prompt. Because of this implicit destroy, this tool + requires DESTRUCTIVE permission (matching leave_conversation in Talk). Args: circle_id: String circle id. + delete_team_folder: Destroying the circle also deletes its team + folder (Nextcloud 35+ with the Team folders app) and every file + in it. When that would happen, the tool refuses unless this is + true. Returns: JSON of the circle the user just left (circle fields: id, name, config, population, initiator, …). Empty when the caller loses - visibility on the circle after leaving (e.g. sole-owner case - where the circle is destroyed). + visibility on the circle after leaving (e.g. when the circle is + destroyed). """ client = get_client() + if not delete_team_folder: + folder = await _folder_lost_by_leaving(client, circle_id) + if folder is not None: + raise refuse_team_folder_loss("Leaving as the owner and last member deletes the circle, which", folder) data = await client.ocs_put_json(f"apps/circles/circles/{circle_id}/leave", json_data={}) return json.dumps(data) @@ -330,6 +384,26 @@ async def update_circle_member_level(circle_id: str, member_id: str, level: str) return json.dumps(data) +async def _folder_lost_by_leaving(client: NextcloudClient, circle_id: str) -> dict[str, Any] | None: + """The team folder that leaving would delete: the caller is the owner and no other confirmed member remains.""" + try: + circle: dict[str, Any] = await client.ocs_get(f"apps/circles/circles/{circle_id}") + except NextcloudError as e: + # Invited or requesting users can leave without seeing the circle. They are never its owner. + if e.status_code in (403, 404): + return None + raise + initiator: dict[str, Any] = circle.get("initiator") or {} + if initiator.get("level") != MEMBER_LEVELS["owner"]: + return None + folder = await get_team_folder(client, circle_id) + if folder is None: + return None + members: list[dict[str, Any]] = await client.ocs_get(f"apps/circles/circles/{circle_id}/members") + others = [m for m in members if m.get("id") != initiator.get("id") and m.get("status") == "Member"] + return None if others else folder + + def _refused_row_lock(message: str) -> bool: """Whether the database refused Circles' SELECT ... FOR UPDATE (SQLite, or PostgreSQL on an outer join).""" return "FOR UPDATE" in message and ("not supported" in message or "outer join" in message) @@ -338,18 +412,29 @@ def _refused_row_lock(message: str) -> bool: def _register_destructive_tools(mcp: FastMCP) -> None: @mcp.tool(annotations=DESTRUCTIVE) @require_permission(PermissionLevel.DESTRUCTIVE) - async def delete_circle(circle_id: str) -> str: + async def delete_circle(circle_id: str, delete_team_folder: bool = False) -> str: """Delete a circle. Requires owner level. Removes all memberships. Args: circle_id: String circle id. + delete_team_folder: Deleting a circle also deletes its team folder + (Nextcloud 35+ with the Team folders app) and every file in it, + for all members. If the circle has one, the tool refuses unless + this is true. Returns: - Confirmation with the deleted id. + Confirmation with the deleted id, and `deleted_team_folder` (the + folder's name) when a team folder went with it. """ client = get_client() + folder = await get_team_folder(client, circle_id) + if folder is not None and not delete_team_folder: + raise refuse_team_folder_loss("Deleting this circle", folder) await client.ocs_delete(f"apps/circles/circles/{circle_id}") - return json.dumps({"deleted_circle_id": circle_id}) + result: dict[str, Any] = {"deleted_circle_id": circle_id} + if folder is not None: + result["deleted_team_folder"] = folder.get("mountPoint") + return json.dumps(result) @mcp.tool(annotations=DESTRUCTIVE) @require_permission(PermissionLevel.DESTRUCTIVE) @@ -372,5 +457,6 @@ def register(mcp: FastMCP) -> None: """Register Circles (Teams) tools with the MCP server.""" _register_read_tools(mcp) _register_circle_writes(mcp) + _register_membership_tools(mcp) _register_member_writes(mcp) _register_destructive_tools(mcp) diff --git a/src/nc_mcp_server/tools/collectives.py b/src/nc_mcp_server/tools/collectives.py index 06282b4..9789547 100644 --- a/src/nc_mcp_server/tools/collectives.py +++ b/src/nc_mcp_server/tools/collectives.py @@ -7,9 +7,10 @@ from mcp.server.fastmcp import FastMCP from ..annotations import ADDITIVE, ADDITIVE_IDEMPOTENT, DESTRUCTIVE, READONLY -from ..client import NextcloudError +from ..client import NextcloudClient, NextcloudError from ..permissions import PermissionLevel, require_permission from ..state import get_client, get_config +from .circles import get_team_folder, refuse_team_folder_loss API = "apps/collectives/api/v1.0" @@ -371,6 +372,16 @@ async def move_collective_page( return json.dumps(_format_page(data["page"]), default=str) +async def _trashed_team_folder(client: NextcloudClient, collective_id: int) -> dict[str, Any] | None: + """The team folder of a trashed collective's team, which deleting the collective with its team would delete.""" + data = await client.ocs_get(f"{API}/collectives/trash") + for collective in data.get("collectives", []): + if collective.get("id") == collective_id and collective.get("circleId"): + return await get_team_folder(client, collective["circleId"]) + # Not in the trash: the delete itself fails with the server's own message. + return None + + def _register_destructive_tools(mcp: FastMCP) -> None: @mcp.tool(annotations=DESTRUCTIVE) @require_permission(PermissionLevel.DESTRUCTIVE) @@ -409,7 +420,7 @@ async def restore_collective(collective_id: int) -> str: @mcp.tool(annotations=DESTRUCTIVE) @require_permission(PermissionLevel.DESTRUCTIVE) - async def delete_collective(collective_id: int, delete_team: bool = False) -> str: + async def delete_collective(collective_id: int, delete_team: bool = False, delete_team_folder: bool = False) -> str: """Permanently delete a collective from the trash. The collective must be in the trash first (use trash_collective). @@ -422,13 +433,25 @@ async def delete_collective(collective_id: int, delete_team: bool = False) -> st own the team; otherwise nothing is deleted. The team goes even if it existed before the collective. With false (default) it stays as an ordinary team. + delete_team_folder: Deleting the team also deletes its team folder + (Nextcloud 35+ with the Team folders app, which gives new + collectives' teams one) and every file in it. With delete_team, + the tool refuses if the team has a team folder unless this is true. Returns: Confirmation message. """ client = get_client() + folder = None + if delete_team: + folder = await _trashed_team_folder(client, collective_id) + if folder is not None and not delete_team_folder: + raise refuse_team_folder_loss("Deleting this collective's team", folder) suffix = "?circle=1" if delete_team else "" await client.ocs_delete(f"{API}/collectives/trash/{collective_id}{suffix}") + if folder is not None: + name = folder.get("mountPoint") + return f"Collective {collective_id} deleted permanently with its team and team folder '{name}'." return f"Collective {collective_id} deleted permanently" + (" with its team." if delete_team else ".") @mcp.tool(annotations=DESTRUCTIVE) diff --git a/src/nc_mcp_server/tools/users.py b/src/nc_mcp_server/tools/users.py index a4f6939..e8395d9 100644 --- a/src/nc_mcp_server/tools/users.py +++ b/src/nc_mcp_server/tools/users.py @@ -307,6 +307,9 @@ async def delete_user(user_id: str) -> str: """Permanently delete a Nextcloud user. Requires admin privileges. This cannot be undone. The user's data and files will be removed. + The user also leaves every team, which deletes the teams where they + were the last member, with those teams' team folders and their files + (Nextcloud 35+ with the Team folders app). To only block access, disable the account with set_user_enabled instead. Args: diff --git a/tests/integration/test_circles.py b/tests/integration/test_circles.py index 4b61266..edce1db 100644 --- a/tests/integration/test_circles.py +++ b/tests/integration/test_circles.py @@ -15,6 +15,7 @@ from nc_mcp_server.client import NextcloudClient, NextcloudError from nc_mcp_server.config import Config from nc_mcp_server.state import get_client, get_config, set_state +from nc_mcp_server.tools.circles import get_team_folder from .conftest import McpTestHelper @@ -404,12 +405,93 @@ async def test_leave_as_member(self, nc_mcp: McpTestHelper, circle_peer: str) -> @pytest.mark.asyncio async def test_sole_owner_leave_destroys_circle(self, nc_mcp: McpTestHelper) -> None: - """Sole-owner leave silently destroys the circle — documented server behavior.""" + """The owner leaving as the last member destroys the circle: documented server behavior.""" circle = await _make_circle(nc_mcp, "mcp-test-circle-sole-leave") await nc_mcp.call("leave_circle", circle_id=circle["id"]) assert await _wait_for_deletion(nc_mcp, circle["id"]) +async def _team_folder_circle(nc_mcp: McpTestHelper, name: str) -> tuple[dict[str, Any], dict[str, Any]]: + """Create a circle with a team folder, or skip the test where the server cannot make one.""" + created: dict[str, Any] = json.loads(await nc_mcp.call("create_circle", name=name, team_folder=True)) + if created["team_folder"] is None: + pytest.skip("team folders need Nextcloud 35+ with the Team folders app") + return created, created["team_folder"] + + +async def _wait_for_file_gone(nc_mcp: McpTestHelper, path: str) -> bool: + """Return True once the file can no longer be read, or False if it is still there after the timeout.""" + deadline = time.monotonic() + CIRCLES_ASYNC_TIMEOUT + while True: + try: + await nc_mcp.client.dav_get(path) + except NextcloudError: + return True + if time.monotonic() > deadline: + return False + await asyncio.sleep(0.5) + + +class TestTeamFolders: + @pytest.mark.asyncio + async def test_no_team_folder_unless_asked(self, nc_mcp: McpTestHelper) -> None: + """Nextcloud 35 gives every new team a team folder by default; the tool opts out.""" + circle = await _make_circle(nc_mcp, "mcp-test-circle-tf-default") + assert "team_folder" not in circle + assert await get_team_folder(nc_mcp.client, circle["id"]) is None + + @pytest.mark.asyncio + async def test_create_reports_the_folder(self, nc_mcp: McpTestHelper) -> None: + circle, folder = await _team_folder_circle(nc_mcp, "mcp-test-circle-tf-create") + assert folder["mountPoint"] == "mcp-test-circle-tf-create" + assert await get_team_folder(nc_mcp.client, circle["id"]) == folder + listing = json.loads(await nc_mcp.call("list_directory", path="/", limit=500)) + assert any(entry["path"].strip("/") == folder["mountPoint"] for entry in listing["data"]) + + @pytest.mark.asyncio + async def test_delete_keeps_the_folder_until_told(self, nc_mcp: McpTestHelper) -> None: + circle, folder = await _team_folder_circle(nc_mcp, "mcp-test-circle-tf-delete") + path = f"{folder['mountPoint']}/keep.txt" + await nc_mcp.call("upload_file", path=path, content="team data") + with pytest.raises(ToolError, match=r"team folder 'mcp-test-circle-tf-delete' and every file.*Nothing"): + await nc_mcp.call("delete_circle", circle_id=circle["id"]) + assert (await nc_mcp.client.dav_get(path))[0] == b"team data" + assert json.loads(await nc_mcp.call("get_circle", circle_id=circle["id"]))["id"] == circle["id"] + + result = json.loads(await nc_mcp.call("delete_circle", circle_id=circle["id"], delete_team_folder=True)) + assert result == {"deleted_circle_id": circle["id"], "deleted_team_folder": folder["mountPoint"]} + assert await _wait_for_deletion(nc_mcp, circle["id"]) + assert await _wait_for_file_gone(nc_mcp, path) + + @pytest.mark.asyncio + async def test_last_member_leave_keeps_the_folder_until_told(self, nc_mcp: McpTestHelper) -> None: + circle, folder = await _team_folder_circle(nc_mcp, "mcp-test-circle-tf-leave") + with pytest.raises(ToolError, match=r"last member deletes the circle.*team folder 'mcp-test-circle-tf-leave'"): + await nc_mcp.call("leave_circle", circle_id=circle["id"]) + assert await get_team_folder(nc_mcp.client, circle["id"]) == folder + + await nc_mcp.call("leave_circle", circle_id=circle["id"], delete_team_folder=True) + assert await _wait_for_deletion(nc_mcp, circle["id"]) + + @pytest.mark.asyncio + async def test_owner_leave_hands_the_team_over(self, nc_mcp: McpTestHelper, circle_peer: str) -> None: + """With another member left, the owner's leave passes ownership on and the folder stays.""" + circle, folder = await _team_folder_circle(nc_mcp, "mcp-test-circle-tf-handover") + await _add_member(nc_mcp, circle["id"], circle_peer) + await nc_mcp.call("leave_circle", circle_id=circle["id"]) + async with _as_peer(circle_peer, CIRCLE_TEST_PWD): + deadline = time.monotonic() + CIRCLES_ASYNC_TIMEOUT + while True: + members = json.loads(await nc_mcp.call("list_circle_members", circle_id=circle["id"])) + levels = {m.get("userId"): m["level"] for m in members} + if levels == {circle_peer: 9} or time.monotonic() > deadline: + break + await asyncio.sleep(0.5) + assert levels == {circle_peer: 9} + assert await get_team_folder(get_client(), circle["id"]) == folder + await nc_mcp.call("delete_circle", circle_id=circle["id"], delete_team_folder=True) + + class TestSearch: @pytest.mark.asyncio async def test_search_returns_list(self, nc_mcp: McpTestHelper) -> None: diff --git a/tests/integration/test_collectives.py b/tests/integration/test_collectives.py index 0a76844..a33b991 100644 --- a/tests/integration/test_collectives.py +++ b/tests/integration/test_collectives.py @@ -9,6 +9,7 @@ from mcp.server.fastmcp.exceptions import ToolError from nc_mcp_server.tools import collectives +from nc_mcp_server.tools.circles import get_team_folder from .conftest import McpTestHelper @@ -45,7 +46,7 @@ async def _destroy_collective(nc_mcp: McpTestHelper, collective_id: int) -> None with contextlib.suppress(Exception): await nc_mcp.call("trash_collective", collective_id=collective_id) with contextlib.suppress(Exception): - await nc_mcp.call("delete_collective", collective_id=collective_id, delete_team=True) + await nc_mcp.call("delete_collective", collective_id=collective_id, delete_team=True, delete_team_folder=True) async def _teams(nc_mcp: McpTestHelper) -> list[dict[str, Any]]: @@ -351,9 +352,22 @@ async def test_permanent_delete_with_team(self, nc_mcp: McpTestHelper) -> None: coll = await _create_collective(nc_mcp, "permdel") try: assert coll["name"] in await _team_names(nc_mcp) + team = next(t for t in await _teams(nc_mcp) if t.get("name") == coll["name"]) + folder = await get_team_folder(nc_mcp.client, team["id"]) await nc_mcp.call("trash_collective", collective_id=coll["id"]) - result = await nc_mcp.call("delete_collective", collective_id=coll["id"], delete_team=True) - assert result.endswith("deleted permanently with its team.") + if folder is None: + result = await nc_mcp.call("delete_collective", collective_id=coll["id"], delete_team=True) + assert result.endswith("deleted permanently with its team.") + else: + # Nextcloud 35 with the Team folders app gives a new collective's team a team folder + with pytest.raises(ToolError, match=r"team folder .*Nothing was changed"): + await nc_mcp.call("delete_collective", collective_id=coll["id"], delete_team=True) + assert coll["name"] in await _team_names(nc_mcp) + assert await get_team_folder(nc_mcp.client, team["id"]) == folder + result = await nc_mcp.call( + "delete_collective", collective_id=coll["id"], delete_team=True, delete_team_folder=True + ) + assert result.endswith(f"with its team and team folder '{folder['mountPoint']}'.") assert coll["name"] not in await _team_names(nc_mcp) finally: await _destroy_collective(nc_mcp, coll["id"]) @@ -370,7 +384,7 @@ async def test_permanent_delete_keeps_the_team_by_default(self, nc_mcp: McpTestH for team in await _teams(nc_mcp): if team.get("name") == coll["name"]: with contextlib.suppress(Exception): - await nc_mcp.call("delete_circle", circle_id=team["id"]) + await nc_mcp.call("delete_circle", circle_id=team["id"], delete_team_folder=True) class TestTrashAndRestorePage: diff --git a/tests/test_circles.py b/tests/test_circles.py index 40e0b29..c916109 100644 --- a/tests/test_circles.py +++ b/tests/test_circles.py @@ -1,7 +1,9 @@ -"""Unit tests for Circles member level changes.""" +"""Unit tests for Circles member level changes and team folder safety.""" from typing import Any -from unittest.mock import AsyncMock, MagicMock +from unittest.mock import AsyncMock, MagicMock, call + +import json import pytest from mcp.server.fastmcp import FastMCP @@ -69,3 +71,131 @@ async def test_the_lock_message_only_applies_to_owner(self, mcp: FastMCP, client client.ocs_put_json.side_effect = LOCK_ERROR with pytest.raises(ToolError, match="FOR UPDATE cannot be applied"): await _call(mcp, "update_circle_member_level", circle_id="c", member_id="m1", level="admin") + + +FOLDER = {"id": 7, "quota": -3, "mountPoint": "Team A"} +NO_FOLDER = NextcloudError("OCS GET x: No team folder linked to this team", 404) +OWNER = {"id": "c", "initiator": {"id": "m-owner", "level": 9}} + + +def _member(member_id: str, status: str = "Member") -> dict[str, Any]: + return {"id": member_id, "status": status} + + +@pytest.fixture +def destructive(client: MagicMock) -> MagicMock: + set_permission_level(PermissionLevel.DESTRUCTIVE) + client.ocs_get = AsyncMock() + client.ocs_delete = AsyncMock(return_value=[]) + return client + + +class TestCreateTeamFolder: + async def test_opts_out_by_default(self, mcp: FastMCP, client: MagicMock) -> None: + client.ocs_post_json = AsyncMock(return_value={"id": "c"}) + client.ocs_get = AsyncMock() + result = json.loads(await _call(mcp, "create_circle", name="Team A")) + client.ocs_post_json.assert_awaited_once_with( + "apps/circles/circles", json_data={"name": "Team A", "createTeamFolder": False} + ) + client.ocs_get.assert_not_awaited() + assert "team_folder" not in result + + async def test_reports_the_created_folder(self, mcp: FastMCP, client: MagicMock) -> None: + client.ocs_post_json = AsyncMock(return_value={"id": "c"}) + client.ocs_get = AsyncMock(return_value=FOLDER) + client.renew_session = AsyncMock() + result = json.loads(await _call(mcp, "create_circle", name="Team A", team_folder=True, local=True)) + client.renew_session.assert_awaited_once_with() + client.ocs_post_json.assert_awaited_once_with( + "apps/circles/circles", json_data={"name": "Team A", "createTeamFolder": True, "local": True} + ) + client.ocs_get.assert_awaited_once_with("apps/circles/teams/c/folder") + assert result["team_folder"] == FOLDER + assert "team_folder_note" not in result + + async def test_says_why_no_folder_came(self, mcp: FastMCP, client: MagicMock) -> None: + client.ocs_post_json = AsyncMock(return_value={"id": "c"}) + client.ocs_get = AsyncMock(side_effect=NextcloudError("OCS GET x: Invalid query", 404)) + client.renew_session = AsyncMock() + result = json.loads(await _call(mcp, "create_circle", name="Team A", team_folder=True)) + client.renew_session.assert_not_awaited() + assert result["team_folder"] is None + assert "Nextcloud 35" in result["team_folder_note"] + + +class TestDeleteCircle: + async def test_without_a_folder(self, mcp: FastMCP, destructive: MagicMock) -> None: + destructive.ocs_get.side_effect = NO_FOLDER + result = json.loads(await _call(mcp, "delete_circle", circle_id="c")) + assert result == {"deleted_circle_id": "c"} + destructive.ocs_delete.assert_awaited_once_with("apps/circles/circles/c") + + async def test_refuses_to_lose_the_folder(self, mcp: FastMCP, destructive: MagicMock) -> None: + destructive.ocs_get.return_value = FOLDER + with pytest.raises(ToolError, match=r"team folder 'Team A' and every file.*Nothing was changed"): + await _call(mcp, "delete_circle", circle_id="c") + destructive.ocs_delete.assert_not_awaited() + + async def test_deletes_the_folder_when_asked(self, mcp: FastMCP, destructive: MagicMock) -> None: + destructive.ocs_get.return_value = FOLDER + result = json.loads(await _call(mcp, "delete_circle", circle_id="c", delete_team_folder=True)) + assert result == {"deleted_circle_id": "c", "deleted_team_folder": "Team A"} + destructive.ocs_delete.assert_awaited_once_with("apps/circles/circles/c") + + async def test_a_failed_check_deletes_nothing(self, mcp: FastMCP, destructive: MagicMock) -> None: + destructive.ocs_get.side_effect = NextcloudError("OCS GET x: Insufficient permissions", 403) + with pytest.raises(ToolError, match="Insufficient permissions"): + await _call(mcp, "delete_circle", circle_id="c", delete_team_folder=True) + destructive.ocs_delete.assert_not_awaited() + + +class TestLeaveCircle: + async def test_members_leave_without_checks(self, mcp: FastMCP, destructive: MagicMock) -> None: + destructive.ocs_get.return_value = {"id": "c", "initiator": {"id": "m1", "level": 1}} + await _call(mcp, "leave_circle", circle_id="c") + destructive.ocs_get.assert_awaited_once_with("apps/circles/circles/c") + destructive.ocs_put_json.assert_awaited_once_with("apps/circles/circles/c/leave", json_data={}) + + async def test_the_owner_hands_over_to_another_member(self, mcp: FastMCP, destructive: MagicMock) -> None: + destructive.ocs_get.side_effect = [OWNER, FOLDER, [_member("m-owner"), _member("m2")]] + await _call(mcp, "leave_circle", circle_id="c") + destructive.ocs_put_json.assert_awaited_once_with("apps/circles/circles/c/leave", json_data={}) + + async def test_the_owner_without_a_folder(self, mcp: FastMCP, destructive: MagicMock) -> None: + destructive.ocs_get.side_effect = [OWNER, NO_FOLDER] + await _call(mcp, "leave_circle", circle_id="c") + destructive.ocs_put_json.assert_awaited_once() + + @pytest.mark.parametrize("others", [[], [_member("m2", "Invited"), _member("m3", "Requesting")]]) + async def test_refuses_when_the_last_member_would_destroy_the_folder( + self, mcp: FastMCP, destructive: MagicMock, others: list[dict[str, Any]] + ) -> None: + destructive.ocs_get.side_effect = [OWNER, FOLDER, [_member("m-owner"), *others]] + with pytest.raises(ToolError, match=r"last member deletes the circle.*team folder 'Team A'"): + await _call(mcp, "leave_circle", circle_id="c") + assert destructive.ocs_get.await_args_list == [ + call("apps/circles/circles/c"), + call("apps/circles/teams/c/folder"), + call("apps/circles/circles/c/members"), + ] + destructive.ocs_put_json.assert_not_awaited() + + async def test_leaves_with_the_folder_when_asked(self, mcp: FastMCP, destructive: MagicMock) -> None: + await _call(mcp, "leave_circle", circle_id="c", delete_team_folder=True) + destructive.ocs_get.assert_not_awaited() + destructive.ocs_put_json.assert_awaited_once_with("apps/circles/circles/c/leave", json_data={}) + + @pytest.mark.parametrize("status", [403, 404]) + async def test_pending_members_cannot_see_the_circle( + self, mcp: FastMCP, destructive: MagicMock, status: int + ) -> None: + destructive.ocs_get.side_effect = NextcloudError("OCS GET x: Insufficient permissions", status) + await _call(mcp, "leave_circle", circle_id="c") + destructive.ocs_put_json.assert_awaited_once() + + async def test_other_check_errors_leave_nothing(self, mcp: FastMCP, destructive: MagicMock) -> None: + destructive.ocs_get.side_effect = NextcloudError("OCS GET x: Internal Server Error", 500) + with pytest.raises(ToolError, match="Internal Server Error"): + await _call(mcp, "leave_circle", circle_id="c") + destructive.ocs_put_json.assert_not_awaited() diff --git a/tests/test_collectives_pages.py b/tests/test_collectives_pages.py index 3b72600..87e1341 100644 --- a/tests/test_collectives_pages.py +++ b/tests/test_collectives_pages.py @@ -218,15 +218,72 @@ async def test_copy_to_another_collective_finds_the_copy( assert result["id"] == 61 +TRASH = {"collectives": [{"id": 3, "circleId": "team3"}, {"id": 4, "circleId": "team4"}]} +FOLDER = {"id": 7, "quota": -3, "mountPoint": "Team 3"} +NO_FOLDER = NextcloudError("OCS GET x: No team folder linked to this team", 404) + + class TestDeleteCollective: @pytest.mark.parametrize(("delete_team", "suffix"), [(False, ""), (True, "?circle=1")]) async def test_team(self, mcp_with_mock_client: tuple[FastMCP, MagicMock], delete_team: bool, suffix: str) -> None: mcp, client = mcp_with_mock_client set_permission_level(PermissionLevel.DESTRUCTIVE) + client.ocs_get.side_effect = [TRASH, NO_FOLDER] client.ocs_delete = AsyncMock(return_value={}) await _call(mcp, "delete_collective", collective_id=3, delete_team=delete_team) client.ocs_delete.assert_awaited_once_with(f"apps/collectives/api/v1.0/collectives/trash/3{suffix}") + async def test_keeping_the_team_skips_the_folder_check( + self, mcp_with_mock_client: tuple[FastMCP, MagicMock] + ) -> None: + mcp, client = mcp_with_mock_client + set_permission_level(PermissionLevel.DESTRUCTIVE) + client.ocs_delete = AsyncMock(return_value={}) + await _call(mcp, "delete_collective", collective_id=3) + client.ocs_get.assert_not_awaited() + + async def test_refuses_to_lose_the_team_folder(self, mcp_with_mock_client: tuple[FastMCP, MagicMock]) -> None: + mcp, client = mcp_with_mock_client + set_permission_level(PermissionLevel.DESTRUCTIVE) + client.ocs_get.side_effect = [TRASH, FOLDER] + client.ocs_delete = AsyncMock(return_value={}) + with pytest.raises(ToolError, match=r"team folder 'Team 3'.*Nothing was changed.*delete_team_folder=true"): + await _call(mcp, "delete_collective", collective_id=3, delete_team=True) + assert client.ocs_get.await_args_list == [ + call("apps/collectives/api/v1.0/collectives/trash"), + call("apps/circles/teams/team3/folder"), + ] + client.ocs_delete.assert_not_awaited() + + async def test_deletes_the_team_folder_when_asked(self, mcp_with_mock_client: tuple[FastMCP, MagicMock]) -> None: + mcp, client = mcp_with_mock_client + set_permission_level(PermissionLevel.DESTRUCTIVE) + client.ocs_get.side_effect = [TRASH, FOLDER] + client.ocs_delete = AsyncMock(return_value={}) + result = await _call(mcp, "delete_collective", collective_id=3, delete_team=True, delete_team_folder=True) + assert "team folder 'Team 3'" in result + client.ocs_delete.assert_awaited_once_with("apps/collectives/api/v1.0/collectives/trash/3?circle=1") + + async def test_not_in_the_trash_leaves_the_error_to_the_server( + self, mcp_with_mock_client: tuple[FastMCP, MagicMock] + ) -> None: + mcp, client = mcp_with_mock_client + set_permission_level(PermissionLevel.DESTRUCTIVE) + client.ocs_get.return_value = TRASH + client.ocs_delete = AsyncMock(side_effect=NextcloudError("OCS DELETE x: Collective not found", 404)) + with pytest.raises(ToolError, match="Collective not found"): + await _call(mcp, "delete_collective", collective_id=9, delete_team=True) + client.ocs_get.assert_awaited_once_with("apps/collectives/api/v1.0/collectives/trash") + + async def test_a_failed_folder_check_deletes_nothing(self, mcp_with_mock_client: tuple[FastMCP, MagicMock]) -> None: + mcp, client = mcp_with_mock_client + set_permission_level(PermissionLevel.DESTRUCTIVE) + client.ocs_get.side_effect = [TRASH, NextcloudError("OCS GET x: Insufficient permissions", 403)] + client.ocs_delete = AsyncMock(return_value={}) + with pytest.raises(ToolError, match="Insufficient permissions"): + await _call(mcp, "delete_collective", collective_id=3, delete_team=True) + client.ocs_delete.assert_not_awaited() + class TestSearch: async def test_content_search(self, mcp_with_mock_client: tuple[FastMCP, MagicMock]) -> None: From 6dfe98826051ecd0a90d24c438ad1ec1a072e207 Mon Sep 17 00:00:00 2001 From: Oleksandr Piskun Date: Tue, 29 Sep 2026 19:48:20 +0000 Subject: [PATCH 2/4] Count pending members on leave and clarify team folder errors Signed-off-by: Oleksandr Piskun --- PROGRESS.md | 4 +- README.md | 2 +- src/nc_mcp_server/client.py | 5 +- src/nc_mcp_server/tools/circles.py | 71 ++++++++++++++++++-------- src/nc_mcp_server/tools/collectives.py | 8 +-- src/nc_mcp_server/tools/users.py | 7 +-- tests/integration/test_circles.py | 57 ++++++++++++++++----- tests/test_circles.py | 33 +++++++++--- tests/test_collectives_pages.py | 4 +- 9 files changed, 135 insertions(+), 56 deletions(-) diff --git a/PROGRESS.md b/PROGRESS.md index 1704af2..fcf577c 100644 --- a/PROGRESS.md +++ b/PROGRESS.md @@ -119,11 +119,11 @@ | File Helpers | — | 36 | | File Reminders | 3 | 22 | | Forms | 25 | 34 | -| Circles | 14 | 58 | +| Circles | 14 | 61 | | Cospend | 16 | 41 | | Flow | 5 | 58 | | Pagination | — | 28 | -| **Total** | **223** | **1815** | +| **Total** | **223** | **1818** | The test counts are what pytest collects, recomputed with `python scripts/sync_progress.py --write`, which also fails when a test file is not assigned to a row. diff --git a/README.md b/README.md index 11a4ea7..4c7d152 100644 --- a/README.md +++ b/README.md @@ -501,7 +501,7 @@ instead of the placeholders Talk stores (`{mention-user1}`, `{file}`). | `add_circle_member` | write | Add a user, group, email, or nested circle as a member | | `update_circle_member_level` | write | Promote/demote a member (member/moderator/admin/owner) | | `join_circle` | write | Join an open circle | -| `leave_circle` | destructive | Leave a circle. The owner's leave passes ownership on, or destroys the circle when no one else is left; refuses to lose a team folder unless told | +| `leave_circle` | destructive | Leave a circle. The owner's leave passes ownership to any other member (pending invitations count), or destroys the circle when no one else is left; refuses to lose a team folder unless told | | `delete_circle` | destructive | Delete a circle; refuses to delete its team folder and files unless told | | `remove_circle_member` | destructive | Kick a member | diff --git a/src/nc_mcp_server/client.py b/src/nc_mcp_server/client.py index 2ccf005..25e4280 100644 --- a/src/nc_mcp_server/client.py +++ b/src/nc_mcp_server/client.py @@ -261,9 +261,8 @@ async def renew_session(self) -> None: """Log in again, so the following requests run in a new server session. Nextcloud does not show an existing session the federated shares accepted during it, - while a session that starts after they were set up for the user sees them. A login also - refreshes the user's cached mounts, which otherwise miss a new team folder for up to - five minutes (fs_mount_cache_duration). + while a session that starts after they were set up for the user sees them. The same goes + for a new team folder, which an existing session can miss for minutes. """ await self._reset_session() diff --git a/src/nc_mcp_server/tools/circles.py b/src/nc_mcp_server/tools/circles.py index bc9d59d..faf1b35 100644 --- a/src/nc_mcp_server/tools/circles.py +++ b/src/nc_mcp_server/tools/circles.py @@ -48,11 +48,37 @@ async def get_team_folder(client: NextcloudClient, circle_id: str) -> dict[str, return folder +async def _report_team_folder(client: NextcloudClient, circle: dict[str, Any]) -> None: + """Add the new circle's team folder, or why there is none, to the circle.""" + try: + circle["team_folder"] = await get_team_folder(client, circle["id"]) + except NextcloudError as e: + # The circle exists already, so report it instead of failing and inviting a duplicate + circle["team_folder"] = None + circle["team_folder_note"] = f"The circle was created, but its team folder could not be read: {e}" + return + if circle["team_folder"] is None: + circle["team_folder_note"] = NO_TEAM_FOLDER + else: + # A session that started before the folder existed can miss it for minutes, a new login sees it + await client.renew_session() + + +async def team_folder_at_stake(client: NextcloudClient, circle_id: str) -> dict[str, Any] | None: + """get_team_folder for a guard before a deletion: a failed check stops the deletion, and says so.""" + try: + return await get_team_folder(client, circle_id) + except NextcloudError as e: + msg = f"Nothing was changed: checking the team folder of circle {circle_id} first failed: {e}" + raise NextcloudError(msg, e.status_code) from e + + def refuse_team_folder_loss(action: str, folder: dict[str, Any]) -> ValueError: """The error for an action that would delete a team folder the caller did not agree to lose.""" return ValueError( f"{action} would also delete the team folder '{folder.get('mountPoint')}' and every file in it, for all " - "members. Nothing was changed. Move out what should be kept, or call again with delete_team_folder=true." + "members. Nothing was changed. Move out what should be kept; to go ahead anyway, the circle's owner calls " + "again with delete_team_folder=true." ) @@ -154,9 +180,10 @@ async def create_circle(name: str, personal: bool = False, local: bool = False, local: If True, mark the circle as local (not federated to other instances) even when global scope is enabled. team_folder: If True, also create a team folder: a shared folder - named after the team that every member sees in Files. Needs - Nextcloud 35 or later with the Team folders app. Deleting the - team later deletes the folder and its files too. + every member sees in Files, named after the team (with a + number added when that name is taken; see its mountPoint). + Needs Nextcloud 35 or later with the Team folders app. + Deleting the team later deletes the folder and its files too. Returns: JSON of the new circle including its generated id. The caller is @@ -173,13 +200,7 @@ async def create_circle(name: str, personal: bool = False, local: bool = False, body["local"] = True data = await client.ocs_post_json("apps/circles/circles", json_data=body) if team_folder: - data["team_folder"] = await get_team_folder(client, data["id"]) - if data["team_folder"] is None: - data["team_folder_note"] = NO_TEAM_FOLDER - else: - # The user's mounts are cached until the next login, so without one the file tools would not - # find the new folder for minutes. - await client.renew_session() + await _report_team_folder(client, data) return json.dumps(data) @mcp.tool(annotations=ADDITIVE_IDEMPOTENT) @@ -287,10 +308,12 @@ async def leave_circle(circle_id: str, delete_team_folder: bool = False) -> str: """Leave a circle the current user is a member of. IMPORTANT: When the owner leaves, the server makes another member the - owner (the highest level, then the longest-standing). When the owner - is the last member, leaving destroys the entire circle, with no - confirmation prompt. Because of this implicit destroy, this tool - requires DESTRUCTIVE permission (matching leave_conversation in Talk). + owner (the highest level, then the longest-standing). Any entry of + list_circle_members counts, even a group, a nested circle or a + pending invitation or join request. When the owner is the last one, + leaving destroys the entire circle, with no confirmation prompt. + Because of this implicit destroy, this tool requires DESTRUCTIVE + permission (matching leave_conversation in Talk). Args: circle_id: String circle id. @@ -385,7 +408,7 @@ async def update_circle_member_level(circle_id: str, member_id: str, level: str) async def _folder_lost_by_leaving(client: NextcloudClient, circle_id: str) -> dict[str, Any] | None: - """The team folder that leaving would delete: the caller is the owner and no other confirmed member remains.""" + """The team folder that leaving would delete: the caller is the owner and no other member remains.""" try: circle: dict[str, Any] = await client.ocs_get(f"apps/circles/circles/{circle_id}") except NextcloudError as e: @@ -396,11 +419,13 @@ async def _folder_lost_by_leaving(client: NextcloudClient, circle_id: str) -> di initiator: dict[str, Any] = circle.get("initiator") or {} if initiator.get("level") != MEMBER_LEVELS["owner"]: return None - folder = await get_team_folder(client, circle_id) + folder = await team_folder_at_stake(client, circle_id) if folder is None: return None + # Circles hands the circle to any other member, even a pending invitation or join request, and destroys it + # only when the owner is the last one members: list[dict[str, Any]] = await client.ocs_get(f"apps/circles/circles/{circle_id}/members") - others = [m for m in members if m.get("id") != initiator.get("id") and m.get("status") == "Member"] + others = [m for m in members if m.get("id") != initiator.get("id")] return None if others else folder @@ -419,15 +444,17 @@ async def delete_circle(circle_id: str, delete_team_folder: bool = False) -> str circle_id: String circle id. delete_team_folder: Deleting a circle also deletes its team folder (Nextcloud 35+ with the Team folders app) and every file in it, - for all members. If the circle has one, the tool refuses unless - this is true. + for all members, also when it is an older group folder an admin + linked to the team. If the circle has one, the tool refuses + unless this is true. Returns: Confirmation with the deleted id, and `deleted_team_folder` (the - folder's name) when a team folder went with it. + folder's name) when a team folder goes with it. Circles finishes + the deletion in the background within seconds. """ client = get_client() - folder = await get_team_folder(client, circle_id) + folder = await team_folder_at_stake(client, circle_id) if folder is not None and not delete_team_folder: raise refuse_team_folder_loss("Deleting this circle", folder) await client.ocs_delete(f"apps/circles/circles/{circle_id}") diff --git a/src/nc_mcp_server/tools/collectives.py b/src/nc_mcp_server/tools/collectives.py index 9789547..038afc9 100644 --- a/src/nc_mcp_server/tools/collectives.py +++ b/src/nc_mcp_server/tools/collectives.py @@ -10,7 +10,7 @@ from ..client import NextcloudClient, NextcloudError from ..permissions import PermissionLevel, require_permission from ..state import get_client, get_config -from .circles import get_team_folder, refuse_team_folder_loss +from .circles import refuse_team_folder_loss, team_folder_at_stake API = "apps/collectives/api/v1.0" @@ -236,7 +236,9 @@ async def create_collective(name: str, emoji: str | None = None) -> str: """Create a new collective (shared knowledge base). A collective is a wiki-like space where team members can create and - edit pages together. It automatically creates a landing page. + edit pages together. It automatically creates a landing page, and a + team (circle) of the same name for its members. On Nextcloud 35+ with + the Team folders app, that team also gets a team folder. Args: name: Name of the collective (required, must be unique). @@ -377,7 +379,7 @@ async def _trashed_team_folder(client: NextcloudClient, collective_id: int) -> d data = await client.ocs_get(f"{API}/collectives/trash") for collective in data.get("collectives", []): if collective.get("id") == collective_id and collective.get("circleId"): - return await get_team_folder(client, collective["circleId"]) + return await team_folder_at_stake(client, collective["circleId"]) # Not in the trash: the delete itself fails with the server's own message. return None diff --git a/src/nc_mcp_server/tools/users.py b/src/nc_mcp_server/tools/users.py index e8395d9..ecd9a45 100644 --- a/src/nc_mcp_server/tools/users.py +++ b/src/nc_mcp_server/tools/users.py @@ -307,9 +307,10 @@ async def delete_user(user_id: str) -> str: """Permanently delete a Nextcloud user. Requires admin privileges. This cannot be undone. The user's data and files will be removed. - The user also leaves every team, which deletes the teams where they - were the last member, with those teams' team folders and their files - (Nextcloud 35+ with the Team folders app). + The user also leaves every team, which deletes the teams they own that + have no other member (a pending invitation counts as one), with those + teams' team folders and their files (Nextcloud 35+ with the Team + folders app). To only block access, disable the account with set_user_enabled instead. Args: diff --git a/tests/integration/test_circles.py b/tests/integration/test_circles.py index edce1db..87770cd 100644 --- a/tests/integration/test_circles.py +++ b/tests/integration/test_circles.py @@ -425,13 +425,23 @@ async def _wait_for_file_gone(nc_mcp: McpTestHelper, path: str) -> bool: while True: try: await nc_mcp.client.dav_get(path) - except NextcloudError: + except NextcloudError as e: + if e.status_code != 404: + raise return True if time.monotonic() > deadline: return False await asyncio.sleep(0.5) +async def _wait_for_levels(nc_mcp: McpTestHelper, circle_id: str, expected: dict[str, int]) -> dict[str, int]: + """Return the members' levels by user once they equal expected, or the last ones after the timeout.""" + members = await _wait_for_members( + nc_mcp, circle_id, lambda members: {m.get("userId"): m["level"] for m in members} == expected + ) + return {str(m.get("userId")): int(m["level"]) for m in members} + + class TestTeamFolders: @pytest.mark.asyncio async def test_no_team_folder_unless_asked(self, nc_mcp: McpTestHelper) -> None: @@ -443,7 +453,8 @@ async def test_no_team_folder_unless_asked(self, nc_mcp: McpTestHelper) -> None: @pytest.mark.asyncio async def test_create_reports_the_folder(self, nc_mcp: McpTestHelper) -> None: circle, folder = await _team_folder_circle(nc_mcp, "mcp-test-circle-tf-create") - assert folder["mountPoint"] == "mcp-test-circle-tf-create" + # Team folders numbers the name when an earlier folder of that name still exists + assert folder["mountPoint"].startswith("mcp-test-circle-tf-create") assert await get_team_folder(nc_mcp.client, circle["id"]) == folder listing = json.loads(await nc_mcp.call("list_directory", path="/", limit=500)) assert any(entry["path"].strip("/") == folder["mountPoint"] for entry in listing["data"]) @@ -453,7 +464,7 @@ async def test_delete_keeps_the_folder_until_told(self, nc_mcp: McpTestHelper) - circle, folder = await _team_folder_circle(nc_mcp, "mcp-test-circle-tf-delete") path = f"{folder['mountPoint']}/keep.txt" await nc_mcp.call("upload_file", path=path, content="team data") - with pytest.raises(ToolError, match=r"team folder 'mcp-test-circle-tf-delete' and every file.*Nothing"): + with pytest.raises(ToolError, match=r"team folder 'mcp-test-circle-tf-delete.*' and every file.*Nothing"): await nc_mcp.call("delete_circle", circle_id=circle["id"]) assert (await nc_mcp.client.dav_get(path))[0] == b"team data" assert json.loads(await nc_mcp.call("get_circle", circle_id=circle["id"]))["id"] == circle["id"] @@ -466,12 +477,16 @@ async def test_delete_keeps_the_folder_until_told(self, nc_mcp: McpTestHelper) - @pytest.mark.asyncio async def test_last_member_leave_keeps_the_folder_until_told(self, nc_mcp: McpTestHelper) -> None: circle, folder = await _team_folder_circle(nc_mcp, "mcp-test-circle-tf-leave") - with pytest.raises(ToolError, match=r"last member deletes the circle.*team folder 'mcp-test-circle-tf-leave'"): + path = f"{folder['mountPoint']}/keep.txt" + await nc_mcp.call("upload_file", path=path, content="team data") + with pytest.raises(ToolError, match=r"last member deletes the circle.*team folder 'mcp-test-circle-tf-leave"): await nc_mcp.call("leave_circle", circle_id=circle["id"]) assert await get_team_folder(nc_mcp.client, circle["id"]) == folder + assert (await nc_mcp.client.dav_get(path))[0] == b"team data" await nc_mcp.call("leave_circle", circle_id=circle["id"], delete_team_folder=True) assert await _wait_for_deletion(nc_mcp, circle["id"]) + assert await _wait_for_file_gone(nc_mcp, path) @pytest.mark.asyncio async def test_owner_leave_hands_the_team_over(self, nc_mcp: McpTestHelper, circle_peer: str) -> None: @@ -480,16 +495,30 @@ async def test_owner_leave_hands_the_team_over(self, nc_mcp: McpTestHelper, circ await _add_member(nc_mcp, circle["id"], circle_peer) await nc_mcp.call("leave_circle", circle_id=circle["id"]) async with _as_peer(circle_peer, CIRCLE_TEST_PWD): - deadline = time.monotonic() + CIRCLES_ASYNC_TIMEOUT - while True: - members = json.loads(await nc_mcp.call("list_circle_members", circle_id=circle["id"])) - levels = {m.get("userId"): m["level"] for m in members} - if levels == {circle_peer: 9} or time.monotonic() > deadline: - break - await asyncio.sleep(0.5) - assert levels == {circle_peer: 9} - assert await get_team_folder(get_client(), circle["id"]) == folder - await nc_mcp.call("delete_circle", circle_id=circle["id"], delete_team_folder=True) + try: + levels = await _wait_for_levels(nc_mcp, circle["id"], {circle_peer: 9}) + assert levels == {circle_peer: 9} + assert await get_team_folder(get_client(), circle["id"]) == folder + finally: + # Admin is no longer a member, so the cleanup could not remove the circle + await nc_mcp.call("delete_circle", circle_id=circle["id"], delete_team_folder=True) + + @pytest.mark.asyncio + async def test_owner_leave_hands_the_team_to_an_invitee(self, nc_mcp: McpTestHelper, circle_peer: str) -> None: + """A pending invitation is enough for Circles to hand the circle over instead of destroying it.""" + circle, folder = await _team_folder_circle(nc_mcp, "mcp-test-circle-tf-invitee") + await nc_mcp.call("update_circle_config", circle_id=circle["id"], config=32) # INVITE + added = json.loads(await nc_mcp.call("add_circle_member", circle_id=circle["id"], user_id=circle_peer)) + assert added["status"] == "Invited" + await _wait_for_members(nc_mcp, circle["id"], _has_user(circle_peer)) + await nc_mcp.call("leave_circle", circle_id=circle["id"]) + async with _as_peer(circle_peer, CIRCLE_TEST_PWD): + try: + levels = await _wait_for_levels(nc_mcp, circle["id"], {circle_peer: 9}) + assert levels == {circle_peer: 9} + assert await get_team_folder(get_client(), circle["id"]) == folder + finally: + await nc_mcp.call("delete_circle", circle_id=circle["id"], delete_team_folder=True) class TestSearch: diff --git a/tests/test_circles.py b/tests/test_circles.py index c916109..37418e1 100644 --- a/tests/test_circles.py +++ b/tests/test_circles.py @@ -1,10 +1,9 @@ """Unit tests for Circles member level changes and team folder safety.""" +import json from typing import Any from unittest.mock import AsyncMock, MagicMock, call -import json - import pytest from mcp.server.fastmcp import FastMCP from mcp.server.fastmcp.exceptions import ToolError @@ -123,6 +122,16 @@ async def test_says_why_no_folder_came(self, mcp: FastMCP, client: MagicMock) -> assert result["team_folder"] is None assert "Nextcloud 35" in result["team_folder_note"] + async def test_a_failed_folder_lookup_still_reports_the_circle(self, mcp: FastMCP, client: MagicMock) -> None: + client.ocs_post_json = AsyncMock(return_value={"id": "c"}) + client.ocs_get = AsyncMock(side_effect=NextcloudError("OCS GET x: Internal Server Error", 500)) + client.renew_session = AsyncMock() + result = json.loads(await _call(mcp, "create_circle", name="Team A", team_folder=True)) + assert result["id"] == "c" + assert result["team_folder"] is None + assert result["team_folder_note"].startswith("The circle was created, but its team folder could not be read") + client.renew_session.assert_not_awaited() + class TestDeleteCircle: async def test_without_a_folder(self, mcp: FastMCP, destructive: MagicMock) -> None: @@ -133,7 +142,7 @@ async def test_without_a_folder(self, mcp: FastMCP, destructive: MagicMock) -> N async def test_refuses_to_lose_the_folder(self, mcp: FastMCP, destructive: MagicMock) -> None: destructive.ocs_get.return_value = FOLDER - with pytest.raises(ToolError, match=r"team folder 'Team A' and every file.*Nothing was changed"): + with pytest.raises(ToolError, match=r"team folder 'Team A' and every file.*Nothing was changed.*owner calls"): await _call(mcp, "delete_circle", circle_id="c") destructive.ocs_delete.assert_not_awaited() @@ -145,7 +154,9 @@ async def test_deletes_the_folder_when_asked(self, mcp: FastMCP, destructive: Ma async def test_a_failed_check_deletes_nothing(self, mcp: FastMCP, destructive: MagicMock) -> None: destructive.ocs_get.side_effect = NextcloudError("OCS GET x: Insufficient permissions", 403) - with pytest.raises(ToolError, match="Insufficient permissions"): + with pytest.raises( + ToolError, match=r"Nothing was changed: checking the team folder of circle c .*Insufficient" + ): await _call(mcp, "delete_circle", circle_id="c", delete_team_folder=True) destructive.ocs_delete.assert_not_awaited() @@ -162,16 +173,24 @@ async def test_the_owner_hands_over_to_another_member(self, mcp: FastMCP, destru await _call(mcp, "leave_circle", circle_id="c") destructive.ocs_put_json.assert_awaited_once_with("apps/circles/circles/c/leave", json_data={}) + @pytest.mark.parametrize("status", ["Invited", "Requesting"]) + async def test_a_pending_member_takes_the_circle_over( + self, mcp: FastMCP, destructive: MagicMock, status: str + ) -> None: + """Circles picks any other member entry as the new owner, pending ones too, so nothing is destroyed.""" + destructive.ocs_get.side_effect = [OWNER, FOLDER, [_member("m-owner"), _member("m2", status)]] + await _call(mcp, "leave_circle", circle_id="c") + destructive.ocs_put_json.assert_awaited_once_with("apps/circles/circles/c/leave", json_data={}) + async def test_the_owner_without_a_folder(self, mcp: FastMCP, destructive: MagicMock) -> None: destructive.ocs_get.side_effect = [OWNER, NO_FOLDER] await _call(mcp, "leave_circle", circle_id="c") destructive.ocs_put_json.assert_awaited_once() - @pytest.mark.parametrize("others", [[], [_member("m2", "Invited"), _member("m3", "Requesting")]]) async def test_refuses_when_the_last_member_would_destroy_the_folder( - self, mcp: FastMCP, destructive: MagicMock, others: list[dict[str, Any]] + self, mcp: FastMCP, destructive: MagicMock ) -> None: - destructive.ocs_get.side_effect = [OWNER, FOLDER, [_member("m-owner"), *others]] + destructive.ocs_get.side_effect = [OWNER, FOLDER, [_member("m-owner")]] with pytest.raises(ToolError, match=r"last member deletes the circle.*team folder 'Team A'"): await _call(mcp, "leave_circle", circle_id="c") assert destructive.ocs_get.await_args_list == [ diff --git a/tests/test_collectives_pages.py b/tests/test_collectives_pages.py index 87e1341..9c5e407 100644 --- a/tests/test_collectives_pages.py +++ b/tests/test_collectives_pages.py @@ -280,7 +280,9 @@ async def test_a_failed_folder_check_deletes_nothing(self, mcp_with_mock_client: set_permission_level(PermissionLevel.DESTRUCTIVE) client.ocs_get.side_effect = [TRASH, NextcloudError("OCS GET x: Insufficient permissions", 403)] client.ocs_delete = AsyncMock(return_value={}) - with pytest.raises(ToolError, match="Insufficient permissions"): + with pytest.raises( + ToolError, match=r"Nothing was changed: checking the team folder of circle team3 .*Insufficient" + ): await _call(mcp, "delete_collective", collective_id=3, delete_team=True) client.ocs_delete.assert_not_awaited() From 5148abf6fd4b225992a7d3213d37f4423563419b Mon Sep 17 00:00:00 2001 From: Oleksandr Piskun Date: Tue, 29 Sep 2026 20:02:37 +0000 Subject: [PATCH 3/4] Offer safe alternatives in team folder refusals and prove deletions in tests Signed-off-by: Oleksandr Piskun --- PROGRESS.md | 6 ++-- src/nc_mcp_server/tools/circles.py | 48 +++++++++++++++++++------- src/nc_mcp_server/tools/collectives.py | 24 ++++++++++--- tests/integration/conftest.py | 23 +++++++++++- tests/integration/test_circles.py | 48 ++++++++++++++++++-------- tests/integration/test_collectives.py | 6 ++-- tests/test_circles.py | 24 +++++++++---- tests/test_collectives_pages.py | 34 +++++++++++++++--- 8 files changed, 165 insertions(+), 48 deletions(-) diff --git a/PROGRESS.md b/PROGRESS.md index fcf577c..6e3dde4 100644 --- a/PROGRESS.md +++ b/PROGRESS.md @@ -103,7 +103,7 @@ | Shares | 8 | 86 | | System Tags | 6 | 22 | | Mail | 10 | 102 | -| Collectives | 25 | 100 | +| Collectives | 25 | 103 | | App Management | 4 | 14 | | Calendar | 6 | 44 | | Contacts | 6 | 78 | @@ -119,11 +119,11 @@ | File Helpers | — | 36 | | File Reminders | 3 | 22 | | Forms | 25 | 34 | -| Circles | 14 | 61 | +| Circles | 14 | 63 | | Cospend | 16 | 41 | | Flow | 5 | 58 | | Pagination | — | 28 | -| **Total** | **223** | **1818** | +| **Total** | **223** | **1823** | The test counts are what pytest collects, recomputed with `python scripts/sync_progress.py --write`, which also fails when a test file is not assigned to a row. diff --git a/src/nc_mcp_server/tools/circles.py b/src/nc_mcp_server/tools/circles.py index faf1b35..db75312 100644 --- a/src/nc_mcp_server/tools/circles.py +++ b/src/nc_mcp_server/tools/circles.py @@ -26,8 +26,8 @@ } NO_TEAM_FOLDER = ( - "No team folder was created. Team folders need Nextcloud 35 or later with the Team folders app, are not made " - "for personal teams, and an admin can turn them off." + "No team folder was created. Team folders need Nextcloud 35 or later with the Team folders app, personal " + "circles get none, an admin can turn them off, and the server only logs a failed creation." ) @@ -69,16 +69,25 @@ async def team_folder_at_stake(client: NextcloudClient, circle_id: str) -> dict[ try: return await get_team_folder(client, circle_id) except NextcloudError as e: - msg = f"Nothing was changed: checking the team folder of circle {circle_id} first failed: {e}" - raise NextcloudError(msg, e.status_code) from e + what = f"checking the team folder of circle {circle_id} first failed" + if e.status_code == 403: + # Circles also answers 403 for circles that do not exist, and only the owner may delete one anyway + what = f"you are not a member of circle {circle_id}, or it does not exist; only its owner can delete it" + raise _unchanged(e, what) from e -def refuse_team_folder_loss(action: str, folder: dict[str, Any]) -> ValueError: - """The error for an action that would delete a team folder the caller did not agree to lose.""" +def _unchanged(error: NextcloudError, what: str) -> NextcloudError: + """A failed check before a deletion, saying that nothing was deleted.""" + return NextcloudError(f"Nothing was changed: {what}: {error}", error.status_code) + + +def refuse_team_folder_loss(lead: str, folder: dict[str, Any], advice: str) -> ValueError: + """The error for an action that would delete a team folder the caller did not agree to lose. + + lead ends where the folder is named, advice says how to keep it or delete it anyway. + """ return ValueError( - f"{action} would also delete the team folder '{folder.get('mountPoint')}' and every file in it, for all " - "members. Nothing was changed. Move out what should be kept; to go ahead anyway, the circle's owner calls " - "again with delete_team_folder=true." + f"{lead} team folder '{folder.get('mountPoint')}' and every file in it. Nothing was changed. {advice}" ) @@ -330,9 +339,17 @@ async def leave_circle(circle_id: str, delete_team_folder: bool = False) -> str: """ client = get_client() if not delete_team_folder: - folder = await _folder_lost_by_leaving(client, circle_id) + try: + folder = await _folder_lost_by_leaving(client, circle_id) + except NextcloudError as e: + raise _unchanged(e, "checking whether leaving would delete the circle failed") from e if folder is not None: - raise refuse_team_folder_loss("Leaving as the owner and last member deletes the circle, which", folder) + raise refuse_team_folder_loss( + "Leaving as the owner and last member deletes the circle, and with it its", + folder, + "To keep them, add a member first (the circle then passes to them) or move out what should be " + "kept; to delete the folder too, call again with delete_team_folder=true.", + ) data = await client.ocs_put_json(f"apps/circles/circles/{circle_id}/leave", json_data={}) return json.dumps(data) @@ -419,7 +436,7 @@ async def _folder_lost_by_leaving(client: NextcloudClient, circle_id: str) -> di initiator: dict[str, Any] = circle.get("initiator") or {} if initiator.get("level") != MEMBER_LEVELS["owner"]: return None - folder = await team_folder_at_stake(client, circle_id) + folder = await get_team_folder(client, circle_id) if folder is None: return None # Circles hands the circle to any other member, even a pending invitation or join request, and destroys it @@ -456,7 +473,12 @@ async def delete_circle(circle_id: str, delete_team_folder: bool = False) -> str client = get_client() folder = await team_folder_at_stake(client, circle_id) if folder is not None and not delete_team_folder: - raise refuse_team_folder_loss("Deleting this circle", folder) + raise refuse_team_folder_loss( + "Deleting this circle would also delete its", + folder, + "All its members lose these files. Move out what should be kept first; to delete the folder too, " + "the circle's owner calls again with delete_team_folder=true.", + ) await client.ocs_delete(f"apps/circles/circles/{circle_id}") result: dict[str, Any] = {"deleted_circle_id": circle_id} if folder is not None: diff --git a/src/nc_mcp_server/tools/collectives.py b/src/nc_mcp_server/tools/collectives.py index 038afc9..9d532ef 100644 --- a/src/nc_mcp_server/tools/collectives.py +++ b/src/nc_mcp_server/tools/collectives.py @@ -10,7 +10,7 @@ from ..client import NextcloudClient, NextcloudError from ..permissions import PermissionLevel, require_permission from ..state import get_client, get_config -from .circles import refuse_team_folder_loss, team_folder_at_stake +from .circles import get_team_folder, refuse_team_folder_loss, team_folder_at_stake API = "apps/collectives/api/v1.0" @@ -245,7 +245,8 @@ async def create_collective(name: str, emoji: str | None = None) -> str: emoji: Optional emoji icon for the collective (e.g. "📚"). Returns: - JSON object with the created collective details. + JSON object with the created collective details, and `team_folder` + (the folder's name) when its team got one. """ if not name.strip(): raise ValueError("Collective name cannot be empty.") @@ -255,7 +256,17 @@ async def create_collective(name: str, emoji: str | None = None) -> str: post_data["emoji"] = emoji data = await client.ocs_post_json(f"{API}/collectives", json_data=post_data) collective = data["collective"] - return json.dumps(_format_collective(collective), default=str) + result = _format_collective(collective) + if collective.get("circleId"): + try: + folder = await get_team_folder(client, collective["circleId"]) + except NextcloudError: + folder = None # the collective exists; its team folder is only extra information + if folder is not None: + result["team_folder"] = folder.get("mountPoint") + # A session that started before the folder existed can miss it for minutes, a new login sees it + await client.renew_session() + return json.dumps(result, default=str) @mcp.tool(annotations=ADDITIVE) @require_permission(PermissionLevel.WRITE) @@ -448,7 +459,12 @@ async def delete_collective(collective_id: int, delete_team: bool = False, delet if delete_team: folder = await _trashed_team_folder(client, collective_id) if folder is not None and not delete_team_folder: - raise refuse_team_folder_loss("Deleting this collective's team", folder) + raise refuse_team_folder_loss( + "Deleting this collective's circle (team) would also delete its", + folder, + "Call without delete_team to keep the circle and its folder; to delete them too, the circle's " + "owner calls again with delete_team=true and delete_team_folder=true.", + ) suffix = "?circle=1" if delete_team else "" await client.ocs_delete(f"{API}/collectives/trash/{collective_id}{suffix}") if folder is not None: diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 7d2efee..4cf3e43 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -1,12 +1,14 @@ """Integration test fixtures — require a running Nextcloud instance.""" import contextlib +import functools import os from collections.abc import AsyncGenerator from pathlib import Path from typing import Any from urllib.parse import quote +import niquests import pytest from mcp.server.fastmcp import FastMCP from mcp.types import ImageContent, TextContent @@ -15,7 +17,7 @@ from nc_mcp_server.config import Config from nc_mcp_server.permissions import PermissionLevel from nc_mcp_server.server import create_server -from nc_mcp_server.state import get_client +from nc_mcp_server.state import get_client, get_config pytestmark = pytest.mark.integration @@ -259,3 +261,22 @@ async def _cleanup(client: NextcloudClient) -> None: await _cleanup_forms(client) await _cleanup_circles(client) await _cleanup_cospend(client) + + +@functools.cache +def team_folders_expected() -> bool: + """Whether the server gives new circles team folders: Nextcloud 35+ with the Team folders app enabled.""" + config = get_config() + auth = (config.user, config.password) + headers = {"OCS-APIRequest": "true", "Accept": "application/json"} + status = niquests.get(f"{config.nextcloud_url}/status.php", timeout=10).json() + if int(status["version"].split(".")[0]) < 35: + return False + apps = niquests.get( + f"{config.nextcloud_url}/ocs/v2.php/cloud/apps", + params={"filter": "enabled"}, + auth=auth, + headers=headers, + timeout=10, + ) + return "groupfolders" in apps.json()["ocs"]["data"]["apps"] diff --git a/tests/integration/test_circles.py b/tests/integration/test_circles.py index 87770cd..d63da87 100644 --- a/tests/integration/test_circles.py +++ b/tests/integration/test_circles.py @@ -17,7 +17,7 @@ from nc_mcp_server.state import get_client, get_config, set_state from nc_mcp_server.tools.circles import get_team_folder -from .conftest import McpTestHelper +from .conftest import McpTestHelper, team_folders_expected pytestmark = pytest.mark.integration @@ -413,21 +413,36 @@ async def test_sole_owner_leave_destroys_circle(self, nc_mcp: McpTestHelper) -> async def _team_folder_circle(nc_mcp: McpTestHelper, name: str) -> tuple[dict[str, Any], dict[str, Any]]: """Create a circle with a team folder, or skip the test where the server cannot make one.""" - created: dict[str, Any] = json.loads(await nc_mcp.call("create_circle", name=name, team_folder=True)) - if created["team_folder"] is None: + if not team_folders_expected(): pytest.skip("team folders need Nextcloud 35+ with the Team folders app") + created: dict[str, Any] = json.loads(await nc_mcp.call("create_circle", name=name, team_folder=True)) + assert created["team_folder"] is not None, created.get("team_folder_note") return created, created["team_folder"] -async def _wait_for_file_gone(nc_mcp: McpTestHelper, path: str) -> bool: - """Return True once the file can no longer be read, or False if it is still there after the timeout.""" +def _group_folder_exists(folder_id: int) -> bool: + """Whether the Team folders app still has the folder, asked as the server admin who sees them all.""" + config = get_config() + response = niquests.get( + f"{config.nextcloud_url}/index.php/apps/groupfolders/folders/{folder_id}", + auth=(config.user, config.password), + headers={"OCS-APIRequest": "true", "Accept": "application/json"}, + timeout=10, + ) + if response.status_code == 404: + return False + response.raise_for_status() + return True + + +async def _wait_for_folder_gone(folder: dict[str, Any]) -> bool: + """Return True once the team folder no longer exists, or False if it is still there after the timeout. + + Checking a file inside would not do: the caller loses access with its membership either way. + """ deadline = time.monotonic() + CIRCLES_ASYNC_TIMEOUT while True: - try: - await nc_mcp.client.dav_get(path) - except NextcloudError as e: - if e.status_code != 404: - raise + if not _group_folder_exists(folder["id"]): return True if time.monotonic() > deadline: return False @@ -472,7 +487,7 @@ async def test_delete_keeps_the_folder_until_told(self, nc_mcp: McpTestHelper) - result = json.loads(await nc_mcp.call("delete_circle", circle_id=circle["id"], delete_team_folder=True)) assert result == {"deleted_circle_id": circle["id"], "deleted_team_folder": folder["mountPoint"]} assert await _wait_for_deletion(nc_mcp, circle["id"]) - assert await _wait_for_file_gone(nc_mcp, path) + assert await _wait_for_folder_gone(folder) @pytest.mark.asyncio async def test_last_member_leave_keeps_the_folder_until_told(self, nc_mcp: McpTestHelper) -> None: @@ -485,8 +500,8 @@ async def test_last_member_leave_keeps_the_folder_until_told(self, nc_mcp: McpTe assert (await nc_mcp.client.dav_get(path))[0] == b"team data" await nc_mcp.call("leave_circle", circle_id=circle["id"], delete_team_folder=True) + assert await _wait_for_folder_gone(folder) assert await _wait_for_deletion(nc_mcp, circle["id"]) - assert await _wait_for_file_gone(nc_mcp, path) @pytest.mark.asyncio async def test_owner_leave_hands_the_team_over(self, nc_mcp: McpTestHelper, circle_peer: str) -> None: @@ -500,8 +515,10 @@ async def test_owner_leave_hands_the_team_over(self, nc_mcp: McpTestHelper, circ assert levels == {circle_peer: 9} assert await get_team_folder(get_client(), circle["id"]) == folder finally: - # Admin is no longer a member, so the cleanup could not remove the circle - await nc_mcp.call("delete_circle", circle_id=circle["id"], delete_team_folder=True) + # Admin is no longer a member, so the cleanup could not remove the circle. If the handover did + # not happen, admin still owns it and the cleanup will; the assertion above then tells why. + with contextlib.suppress(ToolError): + await nc_mcp.call("delete_circle", circle_id=circle["id"], delete_team_folder=True) @pytest.mark.asyncio async def test_owner_leave_hands_the_team_to_an_invitee(self, nc_mcp: McpTestHelper, circle_peer: str) -> None: @@ -518,7 +535,8 @@ async def test_owner_leave_hands_the_team_to_an_invitee(self, nc_mcp: McpTestHel assert levels == {circle_peer: 9} assert await get_team_folder(get_client(), circle["id"]) == folder finally: - await nc_mcp.call("delete_circle", circle_id=circle["id"], delete_team_folder=True) + with contextlib.suppress(ToolError): + await nc_mcp.call("delete_circle", circle_id=circle["id"], delete_team_folder=True) class TestSearch: diff --git a/tests/integration/test_collectives.py b/tests/integration/test_collectives.py index a33b991..496d3e6 100644 --- a/tests/integration/test_collectives.py +++ b/tests/integration/test_collectives.py @@ -11,7 +11,7 @@ from nc_mcp_server.tools import collectives from nc_mcp_server.tools.circles import get_team_folder -from .conftest import McpTestHelper +from .conftest import McpTestHelper, team_folders_expected pytestmark = pytest.mark.integration @@ -354,6 +354,8 @@ async def test_permanent_delete_with_team(self, nc_mcp: McpTestHelper) -> None: assert coll["name"] in await _team_names(nc_mcp) team = next(t for t in await _teams(nc_mcp) if t.get("name") == coll["name"]) folder = await get_team_folder(nc_mcp.client, team["id"]) + assert (folder is not None) == team_folders_expected() + assert coll.get("team_folder") == (folder and folder["mountPoint"]) await nc_mcp.call("trash_collective", collective_id=coll["id"]) if folder is None: result = await nc_mcp.call("delete_collective", collective_id=coll["id"], delete_team=True) @@ -884,4 +886,4 @@ async def test_write_allows_create_but_blocks_trash(self, nc_mcp_write: McpTestH with contextlib.suppress(Exception): await client.ocs_delete(f"apps/collectives/api/v1.0/collectives/{coll['id']}") with contextlib.suppress(Exception): - await client.ocs_delete(f"apps/collectives/api/v1.0/collectives/trash/{coll['id']}") + await client.ocs_delete(f"apps/collectives/api/v1.0/collectives/trash/{coll['id']}?circle=1") diff --git a/tests/test_circles.py b/tests/test_circles.py index 37418e1..b2943e7 100644 --- a/tests/test_circles.py +++ b/tests/test_circles.py @@ -153,13 +153,17 @@ async def test_deletes_the_folder_when_asked(self, mcp: FastMCP, destructive: Ma destructive.ocs_delete.assert_awaited_once_with("apps/circles/circles/c") async def test_a_failed_check_deletes_nothing(self, mcp: FastMCP, destructive: MagicMock) -> None: - destructive.ocs_get.side_effect = NextcloudError("OCS GET x: Insufficient permissions", 403) - with pytest.raises( - ToolError, match=r"Nothing was changed: checking the team folder of circle c .*Insufficient" - ): + destructive.ocs_get.side_effect = NextcloudError("OCS GET x: Internal Server Error", 500) + with pytest.raises(ToolError, match=r"Nothing was changed: checking the team folder of circle c first failed"): await _call(mcp, "delete_circle", circle_id="c", delete_team_folder=True) destructive.ocs_delete.assert_not_awaited() + async def test_non_members_learn_why(self, mcp: FastMCP, destructive: MagicMock) -> None: + destructive.ocs_get.side_effect = NextcloudError("OCS GET x: Insufficient permissions", 403) + with pytest.raises(ToolError, match=r"Nothing was changed: you are not a member of circle c, or it does not"): + await _call(mcp, "delete_circle", circle_id="c") + destructive.ocs_delete.assert_not_awaited() + class TestLeaveCircle: async def test_members_leave_without_checks(self, mcp: FastMCP, destructive: MagicMock) -> None: @@ -191,7 +195,9 @@ async def test_refuses_when_the_last_member_would_destroy_the_folder( self, mcp: FastMCP, destructive: MagicMock ) -> None: destructive.ocs_get.side_effect = [OWNER, FOLDER, [_member("m-owner")]] - with pytest.raises(ToolError, match=r"last member deletes the circle.*team folder 'Team A'"): + with pytest.raises( + ToolError, match=r"last member deletes the circle.*team folder 'Team A'.*add a member first" + ): await _call(mcp, "leave_circle", circle_id="c") assert destructive.ocs_get.await_args_list == [ call("apps/circles/circles/c"), @@ -215,6 +221,12 @@ async def test_pending_members_cannot_see_the_circle( async def test_other_check_errors_leave_nothing(self, mcp: FastMCP, destructive: MagicMock) -> None: destructive.ocs_get.side_effect = NextcloudError("OCS GET x: Internal Server Error", 500) - with pytest.raises(ToolError, match="Internal Server Error"): + with pytest.raises(ToolError, match=r"Nothing was changed: checking whether leaving .*Internal Server Error"): + await _call(mcp, "leave_circle", circle_id="c") + destructive.ocs_put_json.assert_not_awaited() + + async def test_a_failed_member_list_leaves_nothing(self, mcp: FastMCP, destructive: MagicMock) -> None: + destructive.ocs_get.side_effect = [OWNER, FOLDER, NextcloudError("OCS GET x: Bad Gateway", 502)] + with pytest.raises(ToolError, match=r"Nothing was changed: checking whether leaving .*Bad Gateway"): await _call(mcp, "leave_circle", circle_id="c") destructive.ocs_put_json.assert_not_awaited() diff --git a/tests/test_collectives_pages.py b/tests/test_collectives_pages.py index 9c5e407..11b99c3 100644 --- a/tests/test_collectives_pages.py +++ b/tests/test_collectives_pages.py @@ -221,6 +221,34 @@ async def test_copy_to_another_collective_finds_the_copy( TRASH = {"collectives": [{"id": 3, "circleId": "team3"}, {"id": 4, "circleId": "team4"}]} FOLDER = {"id": 7, "quota": -3, "mountPoint": "Team 3"} NO_FOLDER = NextcloudError("OCS GET x: No team folder linked to this team", 404) +COLLECTIVE = {"id": 5, "name": "Team 5", "circleId": "team5"} + + +class TestCreateCollective: + async def test_reports_the_team_folder(self, mcp_with_mock_client: tuple[FastMCP, MagicMock]) -> None: + mcp, client = mcp_with_mock_client + set_permission_level(PermissionLevel.WRITE) + client.ocs_post_json = AsyncMock(return_value={"collective": COLLECTIVE}) + client.ocs_get.return_value = {**FOLDER, "mountPoint": "Team 5"} + client.renew_session = AsyncMock() + result = json.loads(await _call(mcp, "create_collective", name="Team 5")) + assert result["team_folder"] == "Team 5" + client.ocs_get.assert_awaited_once_with("apps/circles/teams/team5/folder") + client.renew_session.assert_awaited_once_with() + + @pytest.mark.parametrize("error", [NO_FOLDER, NextcloudError("OCS GET x: Internal Server Error", 500)]) + async def test_without_a_team_folder( + self, mcp_with_mock_client: tuple[FastMCP, MagicMock], error: NextcloudError + ) -> None: + mcp, client = mcp_with_mock_client + set_permission_level(PermissionLevel.WRITE) + client.ocs_post_json = AsyncMock(return_value={"collective": COLLECTIVE}) + client.ocs_get.side_effect = error + client.renew_session = AsyncMock() + result = json.loads(await _call(mcp, "create_collective", name="Team 5")) + assert result["id"] == 5 + assert "team_folder" not in result + client.renew_session.assert_not_awaited() class TestDeleteCollective: @@ -247,7 +275,7 @@ async def test_refuses_to_lose_the_team_folder(self, mcp_with_mock_client: tuple set_permission_level(PermissionLevel.DESTRUCTIVE) client.ocs_get.side_effect = [TRASH, FOLDER] client.ocs_delete = AsyncMock(return_value={}) - with pytest.raises(ToolError, match=r"team folder 'Team 3'.*Nothing was changed.*delete_team_folder=true"): + with pytest.raises(ToolError, match=r"team folder 'Team 3'.*Nothing was changed.*Call without delete_team"): await _call(mcp, "delete_collective", collective_id=3, delete_team=True) assert client.ocs_get.await_args_list == [ call("apps/collectives/api/v1.0/collectives/trash"), @@ -280,9 +308,7 @@ async def test_a_failed_folder_check_deletes_nothing(self, mcp_with_mock_client: set_permission_level(PermissionLevel.DESTRUCTIVE) client.ocs_get.side_effect = [TRASH, NextcloudError("OCS GET x: Insufficient permissions", 403)] client.ocs_delete = AsyncMock(return_value={}) - with pytest.raises( - ToolError, match=r"Nothing was changed: checking the team folder of circle team3 .*Insufficient" - ): + with pytest.raises(ToolError, match=r"Nothing was changed: you are not a member of circle team3"): await _call(mcp, "delete_collective", collective_id=3, delete_team=True) client.ocs_delete.assert_not_awaited() From 083cabe498c3b27936f6e6c0249a01a479608a0c Mon Sep 17 00:00:00 2001 From: Oleksandr Piskun Date: Tue, 29 Sep 2026 22:01:54 +0000 Subject: [PATCH 4/4] Let forced team deletions past failed folder checks Signed-off-by: Oleksandr Piskun --- PROGRESS.md | 6 +++--- src/nc_mcp_server/tools/circles.py | 30 ++++++++++++++++---------- src/nc_mcp_server/tools/collectives.py | 26 ++++++++++++++++------ tests/integration/conftest.py | 15 +++++++++++-- tests/test_circles.py | 9 +++++++- tests/test_collectives_pages.py | 27 ++++++++++++++++++++++- 6 files changed, 89 insertions(+), 24 deletions(-) diff --git a/PROGRESS.md b/PROGRESS.md index 6e3dde4..6041dfc 100644 --- a/PROGRESS.md +++ b/PROGRESS.md @@ -103,7 +103,7 @@ | Shares | 8 | 86 | | System Tags | 6 | 22 | | Mail | 10 | 102 | -| Collectives | 25 | 103 | +| Collectives | 25 | 106 | | App Management | 4 | 14 | | Calendar | 6 | 44 | | Contacts | 6 | 78 | @@ -119,11 +119,11 @@ | File Helpers | — | 36 | | File Reminders | 3 | 22 | | Forms | 25 | 34 | -| Circles | 14 | 63 | +| Circles | 14 | 64 | | Cospend | 16 | 41 | | Flow | 5 | 58 | | Pagination | — | 28 | -| **Total** | **223** | **1823** | +| **Total** | **223** | **1827** | The test counts are what pytest collects, recomputed with `python scripts/sync_progress.py --write`, which also fails when a test file is not assigned to a row. diff --git a/src/nc_mcp_server/tools/circles.py b/src/nc_mcp_server/tools/circles.py index db75312..fa07ce1 100644 --- a/src/nc_mcp_server/tools/circles.py +++ b/src/nc_mcp_server/tools/circles.py @@ -64,19 +64,27 @@ async def _report_team_folder(client: NextcloudClient, circle: dict[str, Any]) - await client.renew_session() -async def team_folder_at_stake(client: NextcloudClient, circle_id: str) -> dict[str, Any] | None: - """get_team_folder for a guard before a deletion: a failed check stops the deletion, and says so.""" +async def team_folder_at_stake( + client: NextcloudClient, circle_id: str, delete_team_folder: bool, circle: str +) -> dict[str, Any] | None: + """get_team_folder for a guard before deleting a circle, which circle describes in messages. + + Without delete_team_folder a failed check stops the deletion, and says so. With it the caller has agreed to lose + the folder, so an unknown folder does not hold the deletion up; the server still refuses what it would refuse. + """ try: return await get_team_folder(client, circle_id) except NextcloudError as e: - what = f"checking the team folder of circle {circle_id} first failed" + if delete_team_folder: + return None + what = f"checking the team folder of {circle} first failed" if e.status_code == 403: # Circles also answers 403 for circles that do not exist, and only the owner may delete one anyway - what = f"you are not a member of circle {circle_id}, or it does not exist; only its owner can delete it" - raise _unchanged(e, what) from e + what = f"you are not a member of {circle}, or it does not exist; only its owner can delete it" + raise unchanged(e, what) from e -def _unchanged(error: NextcloudError, what: str) -> NextcloudError: +def unchanged(error: NextcloudError, what: str) -> NextcloudError: """A failed check before a deletion, saying that nothing was deleted.""" return NextcloudError(f"Nothing was changed: {what}: {error}", error.status_code) @@ -342,7 +350,7 @@ async def leave_circle(circle_id: str, delete_team_folder: bool = False) -> str: try: folder = await _folder_lost_by_leaving(client, circle_id) except NextcloudError as e: - raise _unchanged(e, "checking whether leaving would delete the circle failed") from e + raise unchanged(e, "checking whether leaving would delete the circle failed") from e if folder is not None: raise refuse_team_folder_loss( "Leaving as the owner and last member deletes the circle, and with it its", @@ -471,13 +479,13 @@ async def delete_circle(circle_id: str, delete_team_folder: bool = False) -> str the deletion in the background within seconds. """ client = get_client() - folder = await team_folder_at_stake(client, circle_id) + folder = await team_folder_at_stake(client, circle_id, delete_team_folder, f"circle {circle_id}") if folder is not None and not delete_team_folder: raise refuse_team_folder_loss( - "Deleting this circle would also delete its", + "Deleting this circle would also delete, for all its members, its", folder, - "All its members lose these files. Move out what should be kept first; to delete the folder too, " - "the circle's owner calls again with delete_team_folder=true.", + "Move out what should be kept first; to delete the folder too, the circle's owner calls again with " + "delete_team_folder=true.", ) await client.ocs_delete(f"apps/circles/circles/{circle_id}") result: dict[str, Any] = {"deleted_circle_id": circle_id} diff --git a/src/nc_mcp_server/tools/collectives.py b/src/nc_mcp_server/tools/collectives.py index 9d532ef..58c1eb9 100644 --- a/src/nc_mcp_server/tools/collectives.py +++ b/src/nc_mcp_server/tools/collectives.py @@ -10,7 +10,12 @@ from ..client import NextcloudClient, NextcloudError from ..permissions import PermissionLevel, require_permission from ..state import get_client, get_config -from .circles import get_team_folder, refuse_team_folder_loss, team_folder_at_stake +from .circles import ( + get_team_folder, + refuse_team_folder_loss, + team_folder_at_stake, + unchanged, +) API = "apps/collectives/api/v1.0" @@ -385,12 +390,21 @@ async def move_collective_page( return json.dumps(_format_page(data["page"]), default=str) -async def _trashed_team_folder(client: NextcloudClient, collective_id: int) -> dict[str, Any] | None: +async def _trashed_team_folder( + client: NextcloudClient, collective_id: int, delete_team_folder: bool +) -> dict[str, Any] | None: """The team folder of a trashed collective's team, which deleting the collective with its team would delete.""" - data = await client.ocs_get(f"{API}/collectives/trash") + try: + data = await client.ocs_get(f"{API}/collectives/trash") + except NextcloudError as e: + if delete_team_folder: + return None + raise unchanged(e, "listing the trashed collectives to check the team folder first failed") from e for collective in data.get("collectives", []): if collective.get("id") == collective_id and collective.get("circleId"): - return await team_folder_at_stake(client, collective["circleId"]) + return await team_folder_at_stake( + client, collective["circleId"], delete_team_folder, "the collective's circle" + ) # Not in the trash: the delete itself fails with the server's own message. return None @@ -457,10 +471,10 @@ async def delete_collective(collective_id: int, delete_team: bool = False, delet client = get_client() folder = None if delete_team: - folder = await _trashed_team_folder(client, collective_id) + folder = await _trashed_team_folder(client, collective_id, delete_team_folder) if folder is not None and not delete_team_folder: raise refuse_team_folder_loss( - "Deleting this collective's circle (team) would also delete its", + "Deleting this collective's circle (team) would also delete, for all its members, its", folder, "Call without delete_team to keep the circle and its folder; to delete them too, the circle's " "owner calls again with delete_team=true and delete_team_folder=true.", diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 4cf3e43..1adc088 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -265,7 +265,7 @@ async def _cleanup(client: NextcloudClient) -> None: @functools.cache def team_folders_expected() -> bool: - """Whether the server gives new circles team folders: Nextcloud 35+ with the Team folders app enabled.""" + """Whether the server gives new circles team folders: Nextcloud 35+ with the Team folders app, not turned off.""" config = get_config() auth = (config.user, config.password) headers = {"OCS-APIRequest": "true", "Accept": "application/json"} @@ -279,4 +279,15 @@ def team_folders_expected() -> bool: headers=headers, timeout=10, ) - return "groupfolders" in apps.json()["ocs"]["data"]["apps"] + apps.raise_for_status() + if "groupfolders" not in apps.json()["ocs"]["data"]["apps"]: + return False + # An admin can turn automatic team folders off; the key is empty while unset (on) + toggle = niquests.get( + f"{config.nextcloud_url}/ocs/v2.php/apps/provisioning_api/api/v1/config/apps/circles/team_folder_auto_create", + auth=auth, + headers=headers, + timeout=10, + ) + toggle.raise_for_status() + return str(toggle.json()["ocs"]["data"]["data"]).lower() not in ("0", "false", "no") diff --git a/tests/test_circles.py b/tests/test_circles.py index b2943e7..2c8075c 100644 --- a/tests/test_circles.py +++ b/tests/test_circles.py @@ -155,9 +155,16 @@ async def test_deletes_the_folder_when_asked(self, mcp: FastMCP, destructive: Ma async def test_a_failed_check_deletes_nothing(self, mcp: FastMCP, destructive: MagicMock) -> None: destructive.ocs_get.side_effect = NextcloudError("OCS GET x: Internal Server Error", 500) with pytest.raises(ToolError, match=r"Nothing was changed: checking the team folder of circle c first failed"): - await _call(mcp, "delete_circle", circle_id="c", delete_team_folder=True) + await _call(mcp, "delete_circle", circle_id="c") destructive.ocs_delete.assert_not_awaited() + async def test_a_forced_delete_goes_past_a_failed_check(self, mcp: FastMCP, destructive: MagicMock) -> None: + """The caller agreed to lose the folder, so a check that cannot answer does not block the deletion.""" + destructive.ocs_get.side_effect = NextcloudError("OCS GET x: Internal Server Error", 500) + result = json.loads(await _call(mcp, "delete_circle", circle_id="c", delete_team_folder=True)) + assert result == {"deleted_circle_id": "c"} + destructive.ocs_delete.assert_awaited_once_with("apps/circles/circles/c") + async def test_non_members_learn_why(self, mcp: FastMCP, destructive: MagicMock) -> None: destructive.ocs_get.side_effect = NextcloudError("OCS GET x: Insufficient permissions", 403) with pytest.raises(ToolError, match=r"Nothing was changed: you are not a member of circle c, or it does not"): diff --git a/tests/test_collectives_pages.py b/tests/test_collectives_pages.py index 11b99c3..53fcbd5 100644 --- a/tests/test_collectives_pages.py +++ b/tests/test_collectives_pages.py @@ -303,12 +303,37 @@ async def test_not_in_the_trash_leaves_the_error_to_the_server( await _call(mcp, "delete_collective", collective_id=9, delete_team=True) client.ocs_get.assert_awaited_once_with("apps/collectives/api/v1.0/collectives/trash") + async def test_a_failed_trash_listing_deletes_nothing( + self, mcp_with_mock_client: tuple[FastMCP, MagicMock] + ) -> None: + mcp, client = mcp_with_mock_client + set_permission_level(PermissionLevel.DESTRUCTIVE) + client.ocs_get.side_effect = NextcloudError("OCS GET x: Internal Server Error", 500) + client.ocs_delete = AsyncMock(return_value={}) + with pytest.raises(ToolError, match=r"Nothing was changed: listing the trashed collectives"): + await _call(mcp, "delete_collective", collective_id=3, delete_team=True) + client.ocs_delete.assert_not_awaited() + + @pytest.mark.parametrize("failing", [0, 1]) + async def test_a_forced_delete_goes_past_a_failed_check( + self, mcp_with_mock_client: tuple[FastMCP, MagicMock], failing: int + ) -> None: + mcp, client = mcp_with_mock_client + set_permission_level(PermissionLevel.DESTRUCTIVE) + responses: list[Any] = [TRASH, FOLDER] + responses[failing] = NextcloudError("OCS GET x: Internal Server Error", 500) + client.ocs_get.side_effect = responses + client.ocs_delete = AsyncMock(return_value={}) + result = await _call(mcp, "delete_collective", collective_id=3, delete_team=True, delete_team_folder=True) + assert result.endswith("deleted permanently with its team.") + client.ocs_delete.assert_awaited_once_with("apps/collectives/api/v1.0/collectives/trash/3?circle=1") + async def test_a_failed_folder_check_deletes_nothing(self, mcp_with_mock_client: tuple[FastMCP, MagicMock]) -> None: mcp, client = mcp_with_mock_client set_permission_level(PermissionLevel.DESTRUCTIVE) client.ocs_get.side_effect = [TRASH, NextcloudError("OCS GET x: Insufficient permissions", 403)] client.ocs_delete = AsyncMock(return_value={}) - with pytest.raises(ToolError, match=r"Nothing was changed: you are not a member of circle team3"): + with pytest.raises(ToolError, match=r"Nothing was changed: you are not a member of the collective's circle"): await _call(mcp, "delete_collective", collective_id=3, delete_team=True) client.ocs_delete.assert_not_awaited()