From 375ad7d0435a176ad593a32045a5f0182a36d505 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Sun, 20 Sep 2026 18:56:58 +0200 Subject: fix: removing someone who never redeemed their invitation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A member row appears only when a code is consumed, so revoking someone invited to the wrong group was refused for having no row — and the node's refusal aborted the browser's removal before its hub half, leaving them a member everywhere with a live code. Revoking now cancels unredeemed codes for that group, and a node refusal no longer cancels the hub removal. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 19 ++++ docs/MESHBAY_DESIGN.md | 8 ++ docs/USERGUIDE.md | 9 ++ .../src/meshbay_hub/static/group-settings.js | 19 +++- packages/meshbay-hub/tests/test_member_removal.py | 65 +++++++++++++ packages/meshbay-node/src/meshbay_node/daemon.py | 14 ++- packages/meshbay-node/src/meshbay_node/ops.py | 22 ++++- packages/meshbay-node/src/meshbay_node/roster.py | 15 +++ packages/meshbay-node/tests/test_roster_pairing.py | 104 +++++++++++++++++++++ 9 files changed, 267 insertions(+), 8 deletions(-) create mode 100644 packages/meshbay-hub/tests/test_member_removal.py diff --git a/CLAUDE.md b/CLAUDE.md index f0f90d5..928d526 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -438,6 +438,25 @@ do. Read them before writing anything that touches the same mechanism. waits for a reply already delivered elsewhere. A reply needs an identifier the protocol guarantees, not a field it happens to have +- **One errand, two halves, and the first one's refusal cancelled the second.** + Removing a member is the node half (stop wrapping the group key) then the hub + half (stop them reaching the node at all), in that order because it fails + safe. The node call was not guarded, so anything it refused ended the whole + callback — and it refused the commonest case there is: a person invited to + the wrong group has no member row on the node, only an unredeemed code, so + `set_status` matched nothing and answered "no such member in that group". + The removal never reached the hub. They stayed a member in the list (the hub + answers it), in the owner's other sessions and in the administrator's view, + the code emailed to them stayed live for its seven days, and the interface + offered no second way to take back either — the same button, every time, + reporting that the person it was listing was not there. Two rules, and the + second is the one that generalises: **a half that only ever removes access is + not the half to skip when the other is in doubt** — report the refusal after + doing what can be done, never instead of it; and **an operation that is + refused for the state it was invented to fix is not a refusal, it is a + missing case**. `test_member_removal.py` holds the seam, and the node tests + hold the case + - **A refusal that never rejects.** Denying Chromium's `fullscreen` permission does not make `requestFullscreen()` throw — the promise never settles. The deny-everything handler was written from a true sentence ("nothing here needs diff --git a/docs/MESHBAY_DESIGN.md b/docs/MESHBAY_DESIGN.md index b9fdbdc..555d0f1 100644 --- a/docs/MESHBAY_DESIGN.md +++ b/docs/MESHBAY_DESIGN.md @@ -383,6 +383,14 @@ Four properties, each load-bearing: rotation propagates by itself and revocation actually takes effect. (Rotating the key after a revocation is still required — the ex-member holds the current one, and no protocol can take that back.) +5. **Revoking somebody cancels the code they have not redeemed yet**, for that + group and no other. There is no membership until a code is consumed, so an + invitation sent to the wrong person is the whole of their access, and a + removal that left it usable would be a removal in name only. It is also the + case removal is asked for most: an invitation is undone before it is + accepted, not after. Nothing to rotate then — they never held the key, and + `member_revoke` says so by returning no reminder rather than by leaving the + caller to work it out. Node authority is established the same way, once per node: `meshbay-node operator pair` prints a code, the operator types it into their own browser, and the node diff --git a/docs/USERGUIDE.md b/docs/USERGUIDE.md index 9f7cea7..4d87aef 100644 --- a/docs/USERGUIDE.md +++ b/docs/USERGUIDE.md @@ -570,6 +570,15 @@ keeps the old one, which opens nothing written from now on. Anything they already downloaded stays theirs. Once a file has been copied, no software can reach back and take it away. +Or the group's **Members** tab, from a paired browser: *Remove*. It does both +halves — the node stops serving them, and the hub stops letting them reach it. + +**Somebody you invited by mistake** is removed the same way, from either, and +it also cancels the code you sent them: until it is redeemed there is no +membership yet, only an invitation, and taking one back has to take back the +other. Nothing to rotate in that case — they never had the key, and neither +the command nor the interface will tell you to. + If you mistype the username, `member revoke` says so rather than quietly doing nothing. diff --git a/packages/meshbay-hub/src/meshbay_hub/static/group-settings.js b/packages/meshbay-hub/src/meshbay_hub/static/group-settings.js index 4d67348..16f6901 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/group-settings.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/group-settings.js @@ -727,11 +727,23 @@ function GroupSettingsPanel({ groupId, group, token, transportRef, gekRef, * wrapped for them; if the hub removal then fails, they are a member on paper * with no key. The other order would leave them able to reach a node that * still serves them. + * + * **A node that refuses does not cancel the hub half.** It used to: the node + * call threw, the whole removal ended there, and the person stayed a member + * everywhere — the list here (the hub answers it), the owner's other + * sessions, the administrator's view — with nothing said about which half + * had failed. Someone invited to the wrong group hit it every time, because + * a node holds no member row for an invitation nobody has redeemed and + * answered "no such member in that group" to the only button offering to + * take them back out. The hub half only ever removes access, so it is not + * the half to skip when the other is in doubt; what the node said is + * reported once the removal has been done, rather than in place of it. */ const removeMember = useCallback(async (member) => { const transport = transportRef && transportRef.current; setError(''); setRemoving(member.user_id); + let nodeError = ''; try { if (platform.node.available) { try { @@ -747,14 +759,17 @@ function GroupSettingsPanel({ groupId, group, token, transportRef, gekRef, const signFn = (sk && window.MeshBayKeys) ? (transcript) => window.MeshBayKeys.signBytes(sk, transcript) : null; - await transport.revokeMember(member.user_id, signFn); + try { + await transport.revokeMember(member.user_id, signFn); + } catch (err) { nodeError = err.message; } } await hubFetch(`/v1/groups/${groupId}/members/${member.username}`, { method: 'DELETE', token, }); loadMembers(); + if (nodeError) setError(nodeError); } catch (err) { - setError(err.message); + setError(nodeError ? `${nodeError} — ${err.message}` : err.message); } finally { setRemoving(''); } diff --git a/packages/meshbay-hub/tests/test_member_removal.py b/packages/meshbay-hub/tests/test_member_removal.py new file mode 100644 index 0000000..609ef6a --- /dev/null +++ b/packages/meshbay-hub/tests/test_member_removal.py @@ -0,0 +1,65 @@ +""" +Removing a member is two halves, and one must not be able to cancel the other. + +Source-level, for the reason `test_spa_ordering.py` gives at more length: no +other test in this tree can see inside a callback in the SPA, and this class of +defect reached a live browser. The node half and the hub half each remove a +different kind of access — the group key, and the ability to reach the node at +all — and the node's refusal used to `throw` out of the whole callback, so +somebody invited to the wrong group could not be removed from anywhere: the +node holds no member row for a code nobody has redeemed, answered "no such +member in that group", and the hub row survived every attempt while the list +(which the hub answers) went on showing them. + +If `removeMember` is restructured, check the invariant still holds — a failing +node call still reaches the hub call — and then move the markers. +""" + +from pathlib import Path + +import pytest + +SETTINGS = (Path(__file__).resolve().parents[1] + / "src" / "meshbay_hub" / "static" / "group-settings.js") + +pytestmark = pytest.mark.skipif( + not SETTINGS.exists(), reason="SPA sources not present") + + +def _remove_member_body() -> str: + source = SETTINGS.read_text() + start = source.find("const removeMember = useCallback(") + assert start != -1, "removeMember is gone from group-settings.js" + end = source.find("\n }, [", start) + assert end != -1, "removeMember's dependency list is gone — update this test" + return source[start:end] + + +def test_the_hub_half_runs_even_when_the_node_refuses(): + body = _remove_member_body() + node_call = body.find("transport.revokeMember(") + hub_call = body.find("method: 'DELETE'") + assert node_call != -1, "the node is no longer asked to revoke" + assert hub_call != -1, "the hub membership is no longer deleted" + assert node_call < hub_call, ( + "the hub removal now runs first — a failure between the two would " + "leave them able to reach a node that still serves them") + # What the fix is: the node call carries its own catch, so the statements + # after it run whatever the node answered. Anything else — no catch, or one + # that only closes after the hub call — is the shape that shipped. + caught = body.find("catch", node_call) + assert caught != -1 and caught < hub_call, ( + "a node refusal escapes past the hub removal: the person stays a " + "member on the hub, in this list and in every other session") + + +def test_a_refusal_is_still_reported(): + """ + Swallowing it would be the opposite mistake — the group key is what the + node half withdraws, and an operator who is not told it failed believes + they have taken it back. + """ + body = _remove_member_body() + assert "nodeError" in body and "setError(nodeError" in body, ( + "the node's refusal is discarded; removal would report success while " + "the node goes on serving them the group key") diff --git a/packages/meshbay-node/src/meshbay_node/daemon.py b/packages/meshbay-node/src/meshbay_node/daemon.py index bb50bcf..c16e49f 100644 --- a/packages/meshbay-node/src/meshbay_node/daemon.py +++ b/packages/meshbay-node/src/meshbay_node/daemon.py @@ -2409,9 +2409,17 @@ def main() -> None: cfg, f"/api/members/{match['user_id']}/revoke?group_id={group_id}", method="POST") print(f"{args.target} revoked from {group_id[:8]}") - print("They stop receiving the group key on their next connection.") - print("They still hold the current one — rotate it:") - print(f" meshbay-node gek-init --group {group_id}") + if out.get("invites_dropped"): + print("Their unredeemed invitation was cancelled.") + # The node decides whether a rotation is warranted and says so in + # the reminder — somebody who never redeemed a code never held the + # key, and advising a rotation there is advice to ignore the next + # time it is real. Deciding it again here is the second + # implementation this file exists not to have. + if out.get("reminder"): + print("They stop receiving the group key on their next connection.") + print("They still hold the current one — rotate it:") + print(f" meshbay-node gek-init --group {group_id}") return if sub == "unpin": diff --git a/packages/meshbay-node/src/meshbay_node/ops.py b/packages/meshbay-node/src/meshbay_node/ops.py index baea365..b680d43 100644 --- a/packages/meshbay-node/src/meshbay_node/ops.py +++ b/packages/meshbay-node/src/meshbay_node/ops.py @@ -243,13 +243,29 @@ async def revoke_member(state: dict, user_id: str, group_id: str) -> dict: Takes effect on their next connection: the key is wrapped on demand, so there is no stored bundle left behind that would outlive this. Rotating the group key is still required — they hold the current one. + + **An unredeemed invite is a membership that has not happened yet**, so it is + revoked here too, and on its own it is enough for this to be a removal. A + member row appears only when a code is consumed: somebody invited to the + wrong group has none, this refused them with "no such member", and the + browser's removal — node half first, deliberately — died on that refusal + before it reached the hub half. They stayed a member on the hub, with a live + code, and the interface offered no other way to take either back. """ roster = _roster(state) - if not await roster.set_status(group_id, user_id, "revoked"): + revoked = await roster.set_status(group_id, user_id, "revoked") + dropped = await roster.drop_invites(group_id, user_id) + if not revoked and not dropped: raise OpError("No such member in that group", status=404) - log.info("Member revoked: user=%s group=%s", user_id[:8], group_id[:8]) + log.info("Member revoked: user=%s group=%s member=%s invites_dropped=%d", + user_id[:8], group_id[:8], revoked, dropped) return {"status": "revoked", "user_id": user_id, "group_id": group_id, - "reminder": "rotate the group key: meshbay-node gek rotate"} + "was_member": revoked, "invites_dropped": dropped, + # Only what is true: somebody who never redeemed a code never held + # the key, and telling an operator to rotate it teaches them that + # the advice is noise. + "reminder": ("rotate the group key: meshbay-node gek rotate" + if revoked else "")} async def unpin_member(state: dict, user_id: str) -> dict: diff --git a/packages/meshbay-node/src/meshbay_node/roster.py b/packages/meshbay-node/src/meshbay_node/roster.py index 9478a32..9533f25 100644 --- a/packages/meshbay-node/src/meshbay_node/roster.py +++ b/packages/meshbay-node/src/meshbay_node/roster.py @@ -1081,6 +1081,21 @@ class Roster: await self._db.commit() return code + async def drop_invites(self, group_id: str, user_id: str) -> int: + """ + Cancel any code this person has not redeemed yet for this group. + + A code already used is left alone: `used_at` is the record that the join + happened, and it is the only thing that makes a second attempt fail. + """ + assert self._db + cur = await self._db.execute( + "DELETE FROM invites WHERE group_id = ? AND user_id = ? AND used_at IS NULL", + (group_id, user_id), + ) + await self._db.commit() + return cur.rowcount + async def consume_invite(self, code: str, user_id: str) -> dict | None: """ Redeem a code for `user_id`, or return None. diff --git a/packages/meshbay-node/tests/test_roster_pairing.py b/packages/meshbay-node/tests/test_roster_pairing.py index 82336e2..91191c1 100644 --- a/packages/meshbay-node/tests/test_roster_pairing.py +++ b/packages/meshbay-node/tests/test_roster_pairing.py @@ -1160,6 +1160,110 @@ async def test_revoking_is_confined_to_the_group_it_was_asked_for(tmp_path, rost "the pinned identity was dropped; that is `member unpin`, not this") +async def test_revoking_someone_who_never_redeemed_their_code(tmp_path, roster): + """ + An invitation sent to the wrong person is the case removal has to cover. + + A member row exists only once a code is consumed, so until then there is + nothing for `set_status` to update, and this answered "no such member in + that group" — a refusal that stopped the browser before it reached the hub + half, leaving the person a member there and their code live. Nothing else + offered to take either back. + """ + from meshbay_common.adminop import admin_transcript + + sk_op, pk_op, pk_x_op = _keypair() + await roster.pin_identity("grenet", "grenet", pk_op, pk_x_op, "code") + await roster.set_member("", "grenet", ROLE_OPERATOR, "active", "local-cli") + + code = await roster.create_invite( + group_id="g1", user_id="misinvited", role=ROLE_MEMBER, + created_by="grenet", username="misinvited") + assert await roster.get_member("g1", "misinvited") is None + + session = _session(tmp_path, roster, group_id="g1", gek=generate_gek()) + session._admin_ops = {} + session._ctx["has_admin_authority"] = True + session._ctx["peers"] = {} + + transcript = admin_transcript( + op="member_revoke", node_pk_b64=session._node_pk_b64(), group_id="g1", + subject="misinvited", nonce=b"\x55" * 32, ts=int(time.time())) + await session._admin_exec_member_revoke( + {"op": "member_revoke", "subject": "misinvited"}, transcript, + sk_op.sign(transcript)) + + assert _last(session)["type"] == "member_revoke_ack", ( + f"the node refused: {_last(session).get('detail')!r}") + # The half that matters: the code that was emailed out no longer works. + assert await roster.consume_invite(code, "misinvited") is None + assert [i for i in await roster.list_invites() + if i["user_id"] == "misinvited"] == [] + + +async def test_revoking_does_not_cancel_their_invitation_to_another_group( + tmp_path, roster): + """ + Inviting somebody to the wrong group and then to the right one is one + errand, not two, and undoing the first must not undo the second — the + second code has usually not been redeemed yet when the mistake is noticed. + """ + from meshbay_common.adminop import admin_transcript + + sk_op, pk_op, pk_x_op = _keypair() + await roster.pin_identity("grenet", "grenet", pk_op, pk_x_op, "code") + await roster.set_member("", "grenet", ROLE_OPERATOR, "active", "local-cli") + + await roster.create_invite(group_id="g1", user_id="misinvited", + role=ROLE_MEMBER, created_by="grenet") + keeper = await roster.create_invite(group_id="g2", user_id="misinvited", + role=ROLE_MEMBER, created_by="grenet") + + session = _session(tmp_path, roster, group_id="g1", gek=generate_gek()) + session._admin_ops = {} + session._ctx["has_admin_authority"] = True + session._ctx["peers"] = {} + + transcript = admin_transcript( + op="member_revoke", node_pk_b64=session._node_pk_b64(), group_id="g1", + subject="misinvited", nonce=b"\x66" * 32, ts=int(time.time())) + await session._admin_exec_member_revoke( + {"op": "member_revoke", "subject": "misinvited"}, transcript, + sk_op.sign(transcript)) + + assert _last(session)["type"] == "member_revoke_ack" + assert await roster.consume_invite(keeper, "misinvited") is not None, ( + "the invitation to the other group was cancelled as well") + + +async def test_revoking_a_stranger_is_still_refused(tmp_path, roster): + """ + Neither a member row nor a code: the 404 is what tells an operator they + named the wrong person or the wrong group, and dropping it would make every + typo look like a removal. + """ + from meshbay_common.adminop import admin_transcript + + sk_op, pk_op, pk_x_op = _keypair() + await roster.pin_identity("grenet", "grenet", pk_op, pk_x_op, "code") + await roster.set_member("", "grenet", ROLE_OPERATOR, "active", "local-cli") + + session = _session(tmp_path, roster, group_id="g1", gek=generate_gek()) + session._admin_ops = {} + session._ctx["has_admin_authority"] = True + session._ctx["peers"] = {} + + transcript = admin_transcript( + op="member_revoke", node_pk_b64=session._node_pk_b64(), group_id="g1", + subject="nobody", nonce=b"\x77" * 32, ts=int(time.time())) + await session._admin_exec_member_revoke( + {"op": "member_revoke", "subject": "nobody"}, transcript, + sk_op.sign(transcript)) + + assert _last(session)["type"] == "error" + assert "No such member" in _last(session)["detail"] + + async def test_an_operator_cannot_revoke_themselves(tmp_path, roster): """It would leave the group with nobody able to invite or remove.""" session = _session(tmp_path, roster, group_id="g1", gek=generate_gek()) -- cgit v1.2.3