From 43c72cf9e8853cd5b2f59d4a4acd87729b0ddae0 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Fri, 25 Sep 2026 13:58:40 +0200 Subject: fix(hub): make group revoke admin-only and route it through the broadcasting path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two coupled gaps in the moderation surface (docs/MESHBAY_DESIGN.md §7.5): admin_patch_group let a moderator set a group to "revoked", while the user handler makes revoke admin-only; and a "revoked" set through either PATCH was never broadcast to nodes — unlike POST /v1/admin/revoke and account deletion — so it behaved like "suspended" on nodes while claiming to be the signed, node-enforced state H4 promises. PATCH now refuses "revoked" on both users and groups (400, pointing at POST /v1/admin/revoke, which signs and broadcasts), a moderator setting a group to revoked is refused with 403 as on the user handler, and moving an entity out of "revoked" requires admin — a moderator flipping the hub row back to active would only disagree with the nodes still enforcing the broadcast. Revoke has one door and it broadcasts. The admin SPA already revokes through POST /v1/admin/revoke, so this is not a UI-visible change. test_revoke_is_one_path.py holds the refusals and the working path; verified red against the pre-fix admin.py and green after. Co-Authored-By: Claude Opus 4.8 --- packages/meshbay-hub/src/meshbay_hub/api/admin.py | 42 ++++++ .../meshbay-hub/tests/test_revoke_is_one_path.py | 164 +++++++++++++++++++++ 2 files changed, 206 insertions(+) create mode 100644 packages/meshbay-hub/tests/test_revoke_is_one_path.py diff --git a/packages/meshbay-hub/src/meshbay_hub/api/admin.py b/packages/meshbay-hub/src/meshbay_hub/api/admin.py index b4b2f4f..381378c 100644 --- a/packages/meshbay-hub/src/meshbay_hub/api/admin.py +++ b/packages/meshbay-hub/src/meshbay_hub/api/admin.py @@ -329,6 +329,25 @@ async def admin_patch_user( raise HTTPException( status_code=422, detail="status must be active, suspended, or revoked") + # Revoking is not a status write. It is signed and broadcast to every + # node so the account is refused there at once; PATCH would change only + # the hub row and leave the nodes unaware — a "revoked" that is not the + # revoked the design promises (§7.5). One door, and it broadcasts. + # (A moderator was already refused above by `privileged`; this is the + # admin, who is told the right door rather than given a broken one.) + if body.status == "revoked": + raise HTTPException( + status_code=400, + detail="Revoke through POST /v1/admin/revoke — it signs the " + "revocation and broadcasts it to every node.") + # Leaving `revoked` undoes a signed, node-enforced action, so it is an + # admin's call, never a moderator's: the nodes still deny this account + # from the broadcast, and a moderator flipping the hub row back to + # active would only disagree with them. + if user.status == "revoked" and not user_is_admin(current_user): + raise HTTPException( + status_code=403, + detail="Only an admin can change a revoked account.") user.status = body.status log.info("User %s status changed to %s by %s", user.username, body.status, current_user.username) @@ -489,6 +508,29 @@ async def admin_patch_group( raise HTTPException( status_code=422, detail="status must be active, suspended, or revoked") + # Suspend/unsuspend is a moderator's reversible, hub-only lever (§7.5). + # Revoke is neither: it is signed and broadcast to every node, and it is + # an administrative act — the same line `admin_patch_user` draws. Two + # gaps used to sit here: a moderator could set `revoked`, and a + # `revoked` set through this PATCH was never broadcast, so it behaved + # like `suspended` on nodes while claiming to be the signed, enforced + # state. Revoke has one door, `POST /v1/admin/revoke`, and it broadcasts. + if body.status == "revoked": + if not user_is_admin(current_user): + raise HTTPException( + status_code=403, + detail="Revoking a group requires admin rights.") + raise HTTPException( + status_code=400, + detail="Revoke a group through POST /v1/admin/revoke — it signs " + "the revocation and broadcasts it to every node.") + # Leaving `revoked` undoes that broadcast and is an admin's call: the + # nodes still enforce the revocation, and a moderator flipping the hub + # row back would only disagree with them. + if group.status == "revoked" and not user_is_admin(current_user): + raise HTTPException( + status_code=403, + detail="Only an admin can change a revoked group.") group.status = body.status log.info("Group %s status changed to %s by %s", group.name, body.status, current_user.username) diff --git a/packages/meshbay-hub/tests/test_revoke_is_one_path.py b/packages/meshbay-hub/tests/test_revoke_is_one_path.py new file mode 100644 index 0000000..2acb46c --- /dev/null +++ b/packages/meshbay-hub/tests/test_revoke_is_one_path.py @@ -0,0 +1,164 @@ +"""Revoke is one door, it is admin-only, and it broadcasts. + +Two coupled gaps used to sit in the moderation surface (docs/MESHBAY_DESIGN.md +§7.5): + + * `admin_patch_group` let a *moderator* set a group to `revoked`, while the + user handler makes revoke admin-only; + * a `revoked` set through either PATCH was **never broadcast** to nodes — + unlike `POST /v1/admin/revoke` and account deletion — so it behaved like + `suspended` on nodes while claiming to be the signed, node-enforced state. + +Revoke now has one path, `POST /v1/admin/revoke` (admin-only, signs and +broadcasts). PATCH refuses `revoked` and refuses to move an entity *out* of +`revoked` unless the caller is an admin. +""" + +import pytest + +from meshbay_hub.api.deps import set_admin_usernames + + +async def _register(client, username): + r = await client.post("/v1/users/register", json={ + "username": username, "email": f"{username}@test.local", "auth_key": "k" * 44}) + assert r.status_code == 201 + return r.json()["user_id"] + + +async def _login(client, username): + r = await client.post("/v1/users/login", json={ + "username": username, "auth_key": "k" * 44}) + assert r.status_code == 200 + return r.json()["access_token"] + + +def _h(token): + return {"Authorization": f"Bearer {token}"} + + +async def _admin(client, name="admin_rev_test"): + await _register(client, name) + set_admin_usernames([name]) + return await _login(client, name) + + +async def _moderator(client, admin_token, name="mod_rev_test"): + uid = await _register(client, name) + r = await client.patch(f"/v1/admin/users/{uid}", json={"role": "moderator"}, + headers=_h(admin_token)) + assert r.status_code == 200 + return uid, await _login(client, name) + + +async def _a_group(client, owner="owner_rev_test"): + await _register(client, owner) + tok = await _login(client, owner) + r = await client.post("/v1/groups", headers=_h(tok), + json={"name": "g", "visibility": "private", "join_policy": "invite"}) + assert r.status_code == 201 + return r.json()["group_id"] + + +async def _group_status(client, admin_token, group_id): + data = (await client.get("/v1/admin/groups?limit=200", headers=_h(admin_token))).json() + return next(g["status"] for g in data["groups"] if g["id"] == group_id) + + +# ── PATCH cannot revoke ────────────────────────────────────────────────────── + +@pytest.mark.asyncio +async def test_moderator_cannot_revoke_a_group_via_patch(client): + admin_token = await _admin(client) + _, mod_token = await _moderator(client, admin_token) + gid = await _a_group(client) + r = await client.patch(f"/v1/admin/groups/{gid}", json={"status": "revoked"}, + headers=_h(mod_token)) + assert r.status_code == 403 + assert await _group_status(client, admin_token, gid) == "active" + + +@pytest.mark.asyncio +async def test_admin_patch_revoked_group_is_redirected_not_silently_applied(client): + admin_token = await _admin(client) + gid = await _a_group(client) + r = await client.patch(f"/v1/admin/groups/{gid}", json={"status": "revoked"}, + headers=_h(admin_token)) + assert r.status_code == 400 + assert "revoke" in r.json()["detail"].lower() + # And it was not quietly applied. + assert await _group_status(client, admin_token, gid) == "active" + + +@pytest.mark.asyncio +async def test_admin_patch_revoked_user_is_redirected(client): + admin_token = await _admin(client) + victim = await _register(client, "vic_rev_test") + r = await client.patch(f"/v1/admin/users/{victim}", json={"status": "revoked"}, + headers=_h(admin_token)) + assert r.status_code == 400 + + +# ── The one door: /v1/admin/revoke, admin-only, and it broadcasts ──────────── + +@pytest.mark.asyncio +async def test_admin_revoke_group_broadcasts_and_sets_status(client): + admin_token = await _admin(client) + gid = await _a_group(client) + r = await client.post("/v1/admin/revoke", headers=_h(admin_token), + json={"target": "group", "target_id": gid}) + assert r.status_code == 200 + body = r.json() + assert body["status"] == "revoked" + assert "nodes_notified" in body # it went through the broadcast path + assert await _group_status(client, admin_token, gid) == "revoked" + + +@pytest.mark.asyncio +async def test_moderator_cannot_reach_the_revoke_endpoint(client): + admin_token = await _admin(client) + _, mod_token = await _moderator(client, admin_token) + gid = await _a_group(client) + r = await client.post("/v1/admin/revoke", headers=_h(mod_token), + json={"target": "group", "target_id": gid}) + assert r.status_code == 403 + + +# ── Leaving `revoked` is an admin's call ───────────────────────────────────── + +@pytest.mark.asyncio +async def test_moderator_cannot_unrevoke_a_group(client): + admin_token = await _admin(client) + _, mod_token = await _moderator(client, admin_token) + gid = await _a_group(client) + await client.post("/v1/admin/revoke", headers=_h(admin_token), + json={"target": "group", "target_id": gid}) + r = await client.patch(f"/v1/admin/groups/{gid}", json={"status": "active"}, + headers=_h(mod_token)) + assert r.status_code == 403 + assert await _group_status(client, admin_token, gid) == "revoked" + + +@pytest.mark.asyncio +async def test_moderator_cannot_unrevoke_a_user(client): + admin_token = await _admin(client) + _, mod_token = await _moderator(client, admin_token) + victim = await _register(client, "vic2_rev_test") + await client.post("/v1/admin/revoke", headers=_h(admin_token), + json={"target": "user", "target_id": victim}) + r = await client.patch(f"/v1/admin/users/{victim}", json={"status": "active"}, + headers=_h(mod_token)) + assert r.status_code == 403 + + +# ── Regression: suspend/unsuspend by a moderator still works ───────────────── + +@pytest.mark.asyncio +async def test_moderator_can_still_suspend_and_restore(client): + admin_token = await _admin(client) + _, mod_token = await _moderator(client, admin_token) + gid = await _a_group(client) + assert (await client.patch(f"/v1/admin/groups/{gid}", json={"status": "suspended"}, + headers=_h(mod_token))).status_code == 200 + assert (await client.patch(f"/v1/admin/groups/{gid}", json={"status": "active"}, + headers=_h(mod_token))).status_code == 200 -- cgit v1.2.3