diff --git a/changelog.d/2287-close-ownership-guard.md b/changelog.d/2287-close-ownership-guard.md new file mode 100644 index 000000000..24883f9dd --- /dev/null +++ b/changelog.d/2287-close-ownership-guard.md @@ -0,0 +1,3 @@ +### Fixed + +- Closing a claimed task is refused unless you are the claim holder or the project lead; a non-claimer now gets 409 instead of silently closing someone else's card (#2287). diff --git a/tests/projects/test_task_store.py b/tests/projects/test_task_store.py index d08cdd6f4..6cbbd348d 100644 --- a/tests/projects/test_task_store.py +++ b/tests/projects/test_task_store.py @@ -371,3 +371,53 @@ async def test_get_task_context_project_falls_back_without_project_store(store): t = await store.create_task(project_id="p", title="T", created_by="u") ctx = await store.get_task_context(t["id"]) assert ctx["project"]["id"] == "p" + + +# ── close_task ownership guard ────────────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_close_by_claimer_passes(store): + """Claim holder can close their own claimed card.""" + t = await store.create_task(project_id="p", title="A", created_by="u") + await store.claim_task(t["id"], claimer_id="agent-1") + ok = await store.close_task(t["id"], closed_by="agent-1", reason="done") + assert ok is True + again = await store.get_task(t["id"]) + assert again["status"] == "closed" + assert again["closed_by"] == "agent-1" + + +@pytest.mark.asyncio +async def test_close_by_stranger_rejected(store): + """A non-claimer cannot close a claimed card (ownership guard).""" + t = await store.create_task(project_id="p", title="A", created_by="u") + await store.claim_task(t["id"], claimer_id="agent-1") + ok = await store.close_task(t["id"], closed_by="agent-2", reason="intruder") + assert ok is False + again = await store.get_task(t["id"]) + assert again["status"] == "claimed" + assert again["claimed_by"] == "agent-1" + + +@pytest.mark.asyncio +async def test_close_by_lead_passes(store): + """Lead/curator can force-close a card claimed by someone else.""" + t = await store.create_task(project_id="p", title="A", created_by="u") + await store.claim_task(t["id"], claimer_id="agent-1") + ok = await store.close_task(t["id"], closed_by="lead", reason="escalation", force=True) + assert ok is True + again = await store.get_task(t["id"]) + assert again["status"] == "closed" + assert again["closed_by"] == "lead" + + +@pytest.mark.asyncio +async def test_close_unclaimed_unchanged(store): + """Unclaimed cards can still be closed by any authorised caller.""" + t = await store.create_task(project_id="p", title="A", created_by="u") + ok = await store.close_task(t["id"], closed_by="reviewer", reason="stale") + assert ok is True + again = await store.get_task(t["id"]) + assert again["status"] == "closed" + assert again["closed_by"] == "reviewer" diff --git a/tests/test_routes_projects_agent_tasks.py b/tests/test_routes_projects_agent_tasks.py index bc130ddc1..729e2ab9c 100644 --- a/tests/test_routes_projects_agent_tasks.py +++ b/tests/test_routes_projects_agent_tasks.py @@ -64,7 +64,7 @@ async def _new_task(ctx, pid, title="T"): return resp.json()["id"] -async def _mint_agent(ctx, project_id, scopes=("project_tasks",)): +async def _mint_agent(ctx, project_id, scopes=("project_tasks",), handle="@grok"): registry = ctx.app.state.agent_registry grants = ctx.app.state.agent_grants priv, _pub = ctx.app.state.agent_registry_keypair @@ -72,7 +72,7 @@ async def _mint_agent(ctx, project_id, scopes=("project_tasks",)): framework="grok", display_name="Grok", origin="external-selfjoin", - handle="@grok", + handle=handle, ) cid = rec["canonical_id"] await registry.set_status(cid, "active") @@ -530,6 +530,64 @@ async def test_admin_claims_and_closes_as_any_actor(self, ctx): assert close.status_code == 200 assert close.json()["status"] == "closed" + async def test_admin_session_closes_agent_claimed_card_lead_is_agent(self, ctx): + """Issue #2191 repro: the board lead is an AGENT registry id, so a + session admin's actor id (a USER id) can never equal lead_member_id. + The ownership guard's force bypass must cover owner + session admin, + not just the lead, or every admin-session close of an agent-claimed + card returns 409.""" + pid = await _new_project(ctx, "alpha") + tid = await _new_task(ctx, pid) + lead_cid, _lead_token = await _mint_agent(ctx, pid, handle="@lead") + pstore = ctx.app.state.project_store + await pstore.add_member(pid, lead_cid, "native") + await pstore.set_lead(pid, lead_cid) + # A different lane agent claims the card. + lane_cid, lane_token = await _mint_agent(ctx, pid, handle="@lane") + async with _bare(ctx.app) as bare: + claim = await bare.post( + f"/api/projects/{pid}/tasks/{tid}/claim", + json={"claimer_id": lane_cid}, + headers=_hdr(lane_token), + ) + assert claim.status_code == 200, claim.text + assert claim.json()["claimed_by"] == lane_cid + # The admin session closes it, recording the closer as its own user id. + close = await ctx.client.post( + f"/api/projects/{pid}/tasks/{tid}/close", + json={"closed_by": ctx.uid}, + ) + assert close.status_code == 200, close.text + assert close.json()["status"] == "closed" + assert close.json()["closed_by"] == ctx.uid + + async def test_lead_agent_closes_other_agents_card(self, ctx): + """Control (merge-gate path): the lead-agent bypass is preserved — a + lead agent still force-closes a card claimed by a different lane + agent.""" + pid = await _new_project(ctx, "alpha") + tid = await _new_task(ctx, pid) + lead_cid, lead_token = await _mint_agent(ctx, pid, handle="@lead") + pstore = ctx.app.state.project_store + await pstore.add_member(pid, lead_cid, "native") + await pstore.set_lead(pid, lead_cid) + lane_cid, lane_token = await _mint_agent(ctx, pid, handle="@lane") + async with _bare(ctx.app) as bare: + claim = await bare.post( + f"/api/projects/{pid}/tasks/{tid}/claim", + json={"claimer_id": lane_cid}, + headers=_hdr(lane_token), + ) + assert claim.status_code == 200, claim.text + close = await bare.post( + f"/api/projects/{pid}/tasks/{tid}/close", + json={"closed_by": lead_cid}, + headers=_hdr(lead_token), + ) + assert close.status_code == 200, close.text + assert close.json()["status"] == "closed" + assert close.json()["closed_by"] == lead_cid + async def test_unauthenticated_still_401(self, ctx): pid = await _new_project(ctx, "alpha") async with _bare(ctx.app) as bare: diff --git a/tinyagentos/projects/task_store.py b/tinyagentos/projects/task_store.py index cc9c6c277..6b8b717f9 100644 --- a/tinyagentos/projects/task_store.py +++ b/tinyagentos/projects/task_store.py @@ -370,14 +370,25 @@ async def close_task( task_id: str, closed_by: str, reason: str | None = None, + *, + force: bool = False, ) -> bool: now = time.time() - cursor = await self._db.execute( - """UPDATE project_tasks - SET status = 'closed', closed_by = ?, closed_at = ?, close_reason = ?, updated_at = ? - WHERE id = ? AND status NOT IN ('closed', 'cancelled')""", - (closed_by, now, reason, now, task_id), - ) + if force: + cursor = await self._db.execute( + """UPDATE project_tasks + SET status = 'closed', closed_by = ?, closed_at = ?, close_reason = ?, updated_at = ? + WHERE id = ? AND status NOT IN ('closed', 'cancelled')""", + (closed_by, now, reason, now, task_id), + ) + else: + cursor = await self._db.execute( + """UPDATE project_tasks + SET status = 'closed', closed_by = ?, closed_at = ?, close_reason = ?, updated_at = ? + WHERE id = ? AND status NOT IN ('closed', 'cancelled') + AND (claimed_by IS NULL OR claimed_by = ?)""", + (closed_by, now, reason, now, task_id, closed_by), + ) await self._db.commit() changed = cursor.rowcount == 1 if changed: diff --git a/tinyagentos/routes/projects.py b/tinyagentos/routes/projects.py index 1fb79e134..9e59c13ca 100644 --- a/tinyagentos/routes/projects.py +++ b/tinyagentos/routes/projects.py @@ -994,8 +994,20 @@ async def close_task( existing = await store.get_task(task_id) if existing is None or existing["project_id"] != project_id: return JSONResponse({"error": "not found"}, status_code=404) - ok = await store.close_task(task_id, closed_by=closed_by, reason=payload.reason) + # Ownership-guard bypass: a card claimed by one agent may still be closed + # by the project lead (lead_member_id), the project owner (user_id), or a + # session admin. The lead is typically an AGENT registry id, so an + # admin/owner session caller (a USER id) can never equal it — widen the + # bypass to cover all three (issue #2191). + force = ( + project.get("lead_member_id") == actor_id + or project.get("user_id") == actor_id + or bool(getattr(request.state, "is_admin", False)) + ) + ok = await store.close_task(task_id, closed_by=closed_by, reason=payload.reason, force=force) if not ok: + if existing.get("claimed_by") and existing["claimed_by"] != closed_by: + return JSONResponse({"error": "not claimed by you"}, status_code=409) return JSONResponse({"error": "cannot close"}, status_code=409) _beads_mark_dirty(request, project_id) await pstore.log_activity(project_id, closed_by, "task.closed", {"task_id": task_id})