diff --git a/plane_mcp/tools/work_items.py b/plane_mcp/tools/work_items.py index ecba3875..e25e4157 100644 --- a/plane_mcp/tools/work_items.py +++ b/plane_mcp/tools/work_items.py @@ -49,6 +49,86 @@ def _ids(items: Any) -> list[str]: return ids +# Plane's project role ladder: guest=5, member=15, admin=20. Only member and +# above can hold a work item assignment. +_MEMBER_ROLE = 15 + + +def _is_assignable(member: Any) -> bool: + """Whether a project member can hold a work item assignment. + + `role` and `is_active` are missing from some deployments' member payloads. + Only reject on a field the server actually reported — an unreported role + must not disqualify every member. + """ + if getattr(member, "is_active", None) is False: + return False + role = getattr(member, "role", None) + return role is None or role >= _MEMBER_ROLE + + +def _assert_assignable(client: Any, workspace_slug: str, project_id: str, assignees: list[str]) -> None: + """Reject assignees Plane would drop, before a write that would clear the field. + + Plane filters unassignable ids out of `assignees` during validation rather + than rejecting them, and an update deletes the work item's existing + assignees before writing that filtered list. A single unassignable id + therefore clears the assignees and still answers 200, so the caller sees a + successful write that both failed and destroyed data. Checking first turns + it into an error naming the ids that would work. + + Args: + client: Plane client. + workspace_slug: The workspace slug identifier. + project_id: UUID of the project the work item belongs to. + assignees: User IDs the caller asked to assign. + + Raises: + ValueError: If any id is not an assignable member of the project. + """ + if not assignees: + return + + try: + members = client.projects.get_members(workspace_slug=workspace_slug, project_id=project_id) + known = {str(m.id): m for m in members} + assignable = {uid: m for uid, m in known.items() if _is_assignable(m)} + # A pre-check must not become a new failure mode: if the lookup is + # unavailable or answers in an unexpected shape, let the write proceed + # exactly as it did before. + except Exception as e: + logger.warning("Could not verify assignees against project %s members: %s", project_id, e) + return + + rejected = [uid for uid in assignees if uid not in assignable] + if not rejected: + return + + details = [] + for uid in rejected: + member = known.get(uid) + if member is None: + details.append(f"{uid} (not a member of this project)") + elif getattr(member, "is_active", None) is False: + details.append(f"{uid} ({member.email}, project membership is inactive)") + else: + details.append(f"{uid} ({member.email}, project role {member.role} is below member)") + + roster = sorted(f"{m.email}={uid}" for uid, m in assignable.items()) + shown = ", ".join(roster[:25]) or "(none)" + if len(roster) > 25: + shown += f", … and {len(roster) - 25} more" + + raise ValueError( + "Plane silently drops assignees it will not accept, and an update clears the work item's " + "existing assignees in the process. Rejected: " + + "; ".join(details) + + ". Only active project members at member role or above can be assigned. " + + f"Assignable members of this project: {shown}. " + + "Add the user to the project first, or use one of the IDs above." + ) + + def _resolve_description_html(description_html: str | None, description_stripped: str | None) -> str | None: """Resolve the description_html to persist. @@ -262,6 +342,8 @@ def create_work_item( priority if priority in get_args(PriorityEnum) else None # type: ignore[assignment] ) + _assert_assignable(client, workspace_slug, project_id, assignees or []) + data = CreateWorkItem( name=name, assignees=assignees, @@ -445,6 +527,8 @@ def update_work_item( priority if priority in get_args(PriorityEnum) else None # type: ignore[assignment] ) + _assert_assignable(client, workspace_slug, project_id, assignees or []) + data = UpdateWorkItem( name=name, assignees=assignees, @@ -508,6 +592,12 @@ def manage_work_item_assignee( Updated WorkItem object """ client, workspace_slug = get_plane_client_context() + # The update below rewrites the whole list, so an unassignable add_user_id + # would clear the very assignees this tool exists to preserve. Only the + # incoming id is checked: an already-assigned member who has since lost + # access must not block a removal. + _assert_assignable(client, workspace_slug, project_id, [add_user_id] if add_user_id else []) + current = client.work_items.retrieve( workspace_slug=workspace_slug, project_id=project_id, work_item_id=work_item_id ) diff --git a/tests/test_work_items.py b/tests/test_work_items.py index b370424e..f7720161 100644 --- a/tests/test_work_items.py +++ b/tests/test_work_items.py @@ -2,7 +2,10 @@ import asyncio +import pytest from fastmcp import Client, FastMCP +from fastmcp.exceptions import ToolError +from plane.models.projects import ProjectMember from plane.models.work_items import WorkItem, WorkItemDetail from plane_mcp.tools import work_items as work_item_tools @@ -14,6 +17,12 @@ class FakeWorkItems: def __init__(self): self.updated = None + self.created = None + + def create(self, workspace_slug, project_id, data): + """Record the create so a test can assert it was never issued.""" + self.created = data + return WorkItem.model_validate({"id": "w", "name": data.name}) def retrieve(self, workspace_slug, project_id, work_item_id): return WorkItemDetail.model_validate( @@ -95,3 +104,139 @@ def test_manage_assignee_remove_bare_string(monkeypatch): {"project_id": "p", "work_item_id": "w", "remove_user_id": "existing-user"}, ) assert client.work_items.updated.assignees == [] + + +# --- assignee assignability guard ------------------------------------------- +# +# Plane filters unassignable ids out of `assignees` during validation instead of +# rejecting them, then deletes the work item's existing assignees before writing +# that filtered list. One bad id clears the field and still answers 200, so the +# guard has to run before the write. + +MEMBER = {"id": "member-1", "email": "member@example.com", "role": 20, "is_active": True} +AT_THRESHOLD = {"id": "member-2", "email": "member2@example.com", "role": 15, "is_active": True} +GUEST = {"id": "guest-1", "email": "guest@example.com", "role": 5, "is_active": True} +INACTIVE = {"id": "inactive-1", "email": "inactive@example.com", "role": 20, "is_active": False} +BARE = {"id": "bare-1", "email": "bare@example.com"} # CE omits role/is_active + + +class FakeProjects: + """`get_members` returns a bare list, the shape the non-lite endpoint serves.""" + + def __init__(self, members, error=None): + self.members = members + self.error = error + self.calls = 0 + + def get_members(self, workspace_slug, project_id): + """Return the configured members, or raise to simulate the lookup being unavailable.""" + self.calls += 1 + if self.error: + raise self.error + return [ProjectMember.model_validate(m) for m in self.members] + + +class MemberAwareClient(FakeClient): + """A FakeClient that can also answer the project-member lookup the guard makes.""" + + def __init__(self, members, error=None): + super().__init__() + self.projects = FakeProjects(members, error) + + +def _assign(monkeypatch, client, assignees, tool="update_work_item"): + """Call a write tool with the given assignees, defaulting to the destructive update path.""" + args = {"project_id": "p", "work_item_id": "w", "assignees": assignees} + if tool == "create_work_item": + args = {"project_id": "p", "name": "X", "assignees": assignees} + return _call(monkeypatch, client, tool, args) + + +@pytest.mark.parametrize( + "members,bad_id,reason", + [ + ([MEMBER], "stranger-1", "stranger-1 (not a member of this project)"), + ([MEMBER, GUEST], "guest-1", "project role 5 is below member"), + ([MEMBER, INACTIVE], "inactive-1", "project membership is inactive"), + ], + ids=["not-a-member", "below-member-role", "inactive-membership"], +) +def test_unassignable_assignee_is_rejected_before_the_write(monkeypatch, members, bad_id, reason): + """Each way Plane filters an id out must raise, name why, and leave the write unsent. + + The error also has to name an id that would work — a caller cannot guess one. + """ + client = MemberAwareClient(members) + with pytest.raises(ToolError) as exc: + _assign(monkeypatch, client, [bad_id]) + assert reason in str(exc.value) + assert "member@example.com=member-1" in str(exc.value) + assert client.work_items.updated is None, "the destructive update must not be issued" + + +@pytest.mark.parametrize("member", [MEMBER, AT_THRESHOLD], ids=["above-threshold", "exactly-at-threshold"]) +def test_assignable_member_passes_through(monkeypatch, member): + """The happy path is unchanged, including a role sitting exactly on the member floor. + + Plane's cutoff is `role >= 15`, so role 15 is the value that decides whether the + comparison is inclusive — an off-by-one there would lock out every plain member. + """ + client = MemberAwareClient([member]) + _assign(monkeypatch, client, [member["id"]]) + assert client.work_items.updated.assignees == [member["id"]] + + +def test_members_without_role_are_accepted_on_membership_alone(monkeypatch): + """Some deployments omit role/is_active; an unreported role must not fail everyone.""" + client = MemberAwareClient([BARE]) + _assign(monkeypatch, client, ["bare-1"]) + assert client.work_items.updated.assignees == ["bare-1"] + + +def test_member_lookup_failure_does_not_block_the_write(monkeypatch): + """A pre-check must not become a new failure mode when the lookup itself breaks.""" + client = MemberAwareClient([], error=RuntimeError("members endpoint unavailable")) + _assign(monkeypatch, client, ["member-1"]) + assert client.work_items.updated.assignees == ["member-1"] + + +def test_empty_assignees_skips_the_member_lookup(monkeypatch): + """Writes that carry no assignees must not pay for an extra request.""" + client = MemberAwareClient([MEMBER]) + _call(monkeypatch, client, "update_work_item", {"project_id": "p", "work_item_id": "w", "name": "Y"}) + assert client.projects.calls == 0 + + +def test_create_work_item_rejects_unassignable_assignee(monkeypatch): + """Create cannot wipe anything, but the requested assignment still silently fails.""" + client = MemberAwareClient([MEMBER]) + with pytest.raises(ToolError) as exc: + _assign(monkeypatch, client, ["stranger-1"], tool="create_work_item") + assert "not a member of this project" in str(exc.value) + assert client.work_items.created is None + + +def test_manage_assignee_rejects_unassignable_add(monkeypatch): + """The read-modify-write in manage_work_item_assignee is what makes this destructive.""" + client = MemberAwareClient([MEMBER]) + with pytest.raises(ToolError): + _call( + monkeypatch, + client, + "manage_work_item_assignee", + {"project_id": "p", "work_item_id": "w", "add_user_id": "stranger-1"}, + ) + assert client.work_items.updated is None, "existing-user must survive a rejected add" + + +def test_manage_assignee_removal_is_not_blocked_by_a_stale_assignee(monkeypatch): + """Only the incoming id is checked, so losing access does not trap the existing list.""" + client = MemberAwareClient([MEMBER]) # "existing-user" is no longer a member + _call( + monkeypatch, + client, + "manage_work_item_assignee", + {"project_id": "p", "work_item_id": "w", "remove_user_id": "existing-user"}, + ) + assert client.work_items.updated.assignees == [] + assert client.projects.calls == 0, "a pure removal introduces no id to check"