From ea56b8c79538323875c00db2e7006b255f7cd494 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Sun, 6 Sep 2026 17:48:36 +0200 Subject: fix(groups): finish Phase 1 — MNP root management, upload targets, eject state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the Phase 1 commit found the RO/RW model sound but three paths unfinished, each of which broke the flow the phase exists to deliver. Plus 29 test failures it introduced and no coverage for anything it added. Uploads went to the wrong directory. The node read a `root` field on file_upload that no client ever sent, so every upload landed in the first writable root while the Files toolbar offered its button based on the root being browsed — with two writable roots, uploading from one wrote into the other. Files now names the root it is showing; Chat names one chosen in the shell (an operator-configured directory arrives in Phase 2); the node refuses an unknown name rather than falling back, and refuses read-only and ejected roots by code. Shared directories were unreachable on the web. The table read its roots only from the loopback API, which resolves to "not available" in a browser, so the section rendered for nobody there — while the Uploads controls it replaced had worked — and the transport.updateRoot/ejectRoot/plugRoot methods beside it were dead. MNP is now the path, loopback the fallback for a local node with no live connection, and adding a root over MNP takes a typed path since no web page can browse a remote disk. Ejecting updated nobody's screen. transport.js resolves an admin ack against the pending request and returns, which is right for every op whose caller knows the value it chose; the root acks carry state only the node can compute, so the operator who clicked Eject was the one client that never saw it happen. And the ejected flag reached roster.db but was never read back, so a restart undid it and the next scan read an empty mount point as an erased library. Also: the member-upload endpoint answered 200 and did nothing (removed); the wizard ignored the first root's RW switch; reload compared roots on name and path, so editing writable in node.toml did nothing; the table had no path column, which is the only thing separating two libraries sharing a basename; apps_enabled normalisation differed between the two sides of a signed subject. Tests: eject/plug, per-root upload refusal and the node.toml rewrite had no coverage at all. test_member_upload_policy.py is replaced by test_root_writable_policy.py — it tested a removed feature — and every property worth keeping from it moved rather than being dropped. Docs: draft-v6 structural decision 9 is annotated as superseded (the operator can no longer have a directory only they may write to — a real capability removed, flagged rather than hidden), the man page documents the root verb and the RO/RW fields, and refactor-groups.md §7b records what the plan got wrong. Suite: 41 failures before, 13 after — all 13 pre-existing on main. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- .../tests/test_upload_controls_hidden.py | 224 +++++++++++++++------ 1 file changed, 163 insertions(+), 61 deletions(-) (limited to 'packages/meshbay-hub/tests') diff --git a/packages/meshbay-hub/tests/test_upload_controls_hidden.py b/packages/meshbay-hub/tests/test_upload_controls_hidden.py index 94917d2..ae1f444 100644 --- a/packages/meshbay-hub/tests/test_upload_controls_hidden.py +++ b/packages/meshbay-hub/tests/test_upload_controls_hidden.py @@ -1,14 +1,22 @@ """ -When the operator closes uploading, the controls go — both of them. +When a directory is read-only, the controls that write to it go — both of them. -There are two ways to put a file into a group and they are in different +There are two ways to put a file into a group and they live in different components: the Upload button in the Files toolbar, and the paperclip in the chat composer. Hiding one and forgetting the other is the obvious mistake, and -the second one is the easier to forget because it does not look like an upload. +the paperclip is the easier to forget because it does not look like an upload. Nothing here is a security property. **The node refuses the upload** — that is -`test_member_upload_policy.py` in the node package. This is about not offering -somebody a button whose only outcome is an error message. +`test_root_writable_policy.py` and `test_security_regressions.py` in the node +package. This is about not offering somebody a button whose only outcome is an +error message. + +What the RO/RW refactor changed: there is no group-wide answer any more. Files +uploads into *the root being browsed*, so its button follows that root's +`writable`. Chat has no folder on screen, so the shell picks one for it. The +two therefore read different things on purpose, and the tests below pin that +each reads the right one — a stronger claim than the old "both read one +boolean", which is why that assertion is gone rather than adapted. """ import re @@ -17,10 +25,6 @@ from pathlib import Path import pytest STATIC = Path(__file__).resolve().parents[1] / "src" / "meshbay_hub" / "static" -# The group-page refactor split what used to be one app.js into one file per -# "application" plus the group shell. mayUpload itself is still derived once, -# in the shell (group-page.js) — Files and Chat each moved to their own file -# and receive it as a prop, the same shape ChatPanel already took. APP = STATIC / "app.js" GROUP_PAGE = STATIC / "group-page.js" FILES_APP = STATIC / "files-app.js" @@ -36,45 +40,93 @@ def app() -> str: return GROUP_PAGE.read_text(encoding="utf-8") -def _component(app: str, name: str) -> str: - start = app.index(f"\nfunction {name}(") - end = app.find("\nfunction ", start + 1) - return app[start:end if end != -1 else len(app)] +def _component(source: str, name: str) -> str: + start = source.index(f"\nfunction {name}(") + end = source.find("\nfunction ", start + 1) + return source[start:end if end != -1 else len(source)] # ── Both controls ─────────────────────────────────────────────────────────── def test_the_files_toolbar_hides_its_upload_button(): + """ + Gated on the root being browsed, not on a group-wide answer: with one + writable root and one read-only one, a single boolean would offer the + button in both and produce a refusal in one of them. + """ page = _component(FILES_APP.read_text(encoding="utf-8"), "FilesPanel") toolbar = page[page.index("file-toolbar"):] toolbar = toolbar[:toolbar.index("breadcrumbs")] - assert "mayUpload &&" in toolbar, "the Upload button is offered regardless" + assert "currentRootWritable" in toolbar, ( + "the Upload button is offered regardless of the directory's own flag") + + +def test_the_files_upload_button_is_not_offered_at_the_top_of_a_group(): + """ + The top level is the set of roots, which is the operator's configuration + and not a directory on anyone's disk. There is nothing to upload *into* + there, and no root name to give the node. + """ + page = _component(FILES_APP.read_text(encoding="utf-8"), "FilesPanel") + toolbar = page[page.index("file-toolbar"):] + toolbar = toolbar[:toolbar.index("breadcrumbs")] + assert "currentPath &&" in toolbar def test_the_chat_composer_hides_its_paperclip(): chat = _component(CHAT_APP.read_text(encoding="utf-8"), "ChatPanel") composer = chat[chat.index("chat-input-row"):] - assert "mayUpload &&" in composer, ( + assert "attachRoot ?" in composer, ( "the chat attachment is the second way in and is still offered") -def test_both_read_the_same_answer(app): - """Two derivations would eventually disagree, and the disagreement would - be one of them offering an upload the node refuses.""" - assert re.search(r"const mayUpload = memberUpload \|\| isNodeAdmin;", app), ( - "mayUpload is no longer derived in one place") - # Files and Chat both receive it from the same `commonProps` object the - # shell spreads into whichever app tab is active — one derivation feeding - # one object, rather than two hand-written prop attributes that could - # drift apart. - props = app[app.index("const commonProps = {"):app.index("return html`")] - assert "mayUpload," in props or "mayUpload:" in props, ( - "mayUpload is not in the shared props object every app receives") +def test_the_paperclip_says_why_rather_than_vanishing(): + """ + A control that disappears leaves the reader no way to find out what would + bring it back. A group with no writable directory is a state an operator + can fix, so it is worth naming. + """ + chat = _component(CHAT_APP.read_text(encoding="utf-8"), "ChatPanel") + composer = chat[chat.index("chat-input-row"):] + assert "chat.attach_read_only" in composer + +# ── One derivation, in the shell ──────────────────────────────────────────── -def test_the_operator_keeps_their_own_controls(app): - assert "memberUpload || isNodeAdmin" in app, ( - "turning uploads off would hide the operator's own upload button") +def test_the_attachment_directory_is_decided_once(app): + """ + Two derivations would eventually disagree, and the disagreement would be + one of them offering an upload the node refuses. + """ + assert re.search(r"const attachRoot = ", app), ( + "attachRoot is no longer derived in one place") + props = app[app.index("const commonProps = {"):app.index("return html`")] + assert "attachRoot," in props or "attachRoot:" in props, ( + "attachRoot is not in the shared props object every app receives") + + +def test_an_unavailable_root_is_not_offered_as_a_destination(app): + """ + `writable` is configuration and stays true while a drive is unplugged or + ejected. Offering it anyway produces a refusal from the node with no + explanation on screen. + """ + block = app[app.index("const writableRoots"):] + block = block[:block.index("const attachRoot")] + assert "available" in block + + +def test_files_uploads_into_the_root_it_is_showing(): + """ + The client has to name the destination now, because the node cannot choose + between several writable roots without guessing — and a guess here means a + file landing in a directory nobody was looking at. + """ + page = _component(FILES_APP.read_text(encoding="utf-8"), "FilesPanel") + upload = page[page.index("const uploadFile"):] + upload = upload[:upload.index("const makeDirectory")] + assert "root: uploadRoot" in upload, "the node is left to choose" + assert "currentPath.split('/')[0]" in upload # ── Learning the answer ───────────────────────────────────────────────────── @@ -82,54 +134,104 @@ def test_the_operator_keeps_their_own_controls(app): def test_the_answer_comes_from_the_node(app): """Not from the hub, which has no say in what may be written to someone else's disk, and no way to be believed about it.""" - assert "ack.member_upload !== false" in app, ( - "the handshake ack is what carries this") - assert "hubFetch" not in app[app.index("ack.member_upload") - 400: - app.index("ack.member_upload")] + assert "if (indexMsg.roots) setNodeRoots(indexMsg.roots)" in app, ( + "the roots table in the index payload is what carries this") + idx = app.index("setNodeRoots(indexMsg.roots)") + assert "hubFetch" not in app[idx - 400:idx] def test_an_older_node_is_treated_as_permissive(app): - """A node that predates the setting sends no such field. Reading a missing - field as "off" would close every group on the older half of the network.""" + """ + A node speaking MNP 1.0 sends roots with no `writable` at all, plus the old + group-wide flag. Reading a missing field as "read-only" would close every + group on the older half of the network. + """ + assert "ack.member_upload !== false" in app assert "!== false" in app[app.index("ack.member_upload"): app.index("ack.member_upload") + 60] + block = app[app.index("const legacyNode"):] + block = block[:block.index("const commonProps")] + assert "writable === undefined" in block, ( + "nothing distinguishes a 1.0 node from one with no writable roots") def test_a_change_reaches_people_already_connected(app): - """The operator may be someone else entirely, changing it while you have - the group open. A button that survives until the next reconnection is a - button somebody presses.""" - assert "transport.onUploadPolicy" in app + """ + The operator may be someone else entirely, ejecting a drive while you have + the group open. A file list that survives until the next reconnection is a + list somebody clicks. + """ + assert "transport.onRootsChanged" in app transport = TRANSPORT.read_text(encoding="utf-8") - assert "member_upload_ack" in transport, "nothing routes the node's notice" + assert "root_eject_ack" in transport, "nothing routes the node's notice" + +def test_the_notice_also_answers_the_operators_own_request(): + """ + The same message is both a broadcast and the reply to the request that + caused it. -def test_the_notice_still_answers_the_operators_own_request(app): - """The same message is both a broadcast and the reply to the request that - caused it — returning early on it would leave that request hanging until it - timed out.""" + Every other admin ack can be resolved and dropped, because its caller + already knows what it asked for and updates local state from that. The root + acks carry a whole table only the node can compute — availability, the name + it settled on, the eject a failed plug left in place — so resolving one + without handing it on left the operator who clicked Eject as the only + client that never saw it happen. + """ transport = TRANSPORT.read_text(encoding="utf-8") - # Scoped to member_upload_ack's own handler, not everything up to the next - # occurrence of "index_sync" — other handlers with their own, legitimate - # early `return` (index_progress, set_scan_settings_ack: neither is ever a - # reply anyone awaits) now sit between the two in the file. - block = transport[transport.index("member_upload_ack"):] - block = block[:block.index("apps_enabled_ack")] - assert "return" not in block + block = transport[transport.index("msg.type.endsWith('_ack')"):] + block = block[:block.index("_uploaders")] + assert "ROOT_ACK_TYPES" in block and "_onRootsChanged" in block, ( + "the initiating client resolves the ack and learns nothing from it") # ── Changing it ───────────────────────────────────────────────────────────── -def test_changing_it_is_signed(app): +def test_changing_a_root_is_signed(): transport = TRANSPORT.read_text(encoding="utf-8") - method = transport[transport.index("async setMemberUpload("):] - method = method[:method.index("\n async ", 1)] - assert "admin_challenge" in method and "_authorizeAdminOp" in method, ( - "an unsigned instruction would let any member turn uploads back on") + for method in ("updateRoot", "ejectRoot", "plugRoot"): + body = transport[transport.index(f"async {method}("):] + body = body[:body.index("\n async ", 1)] + assert "admin_challenge" in body and "_authorizeAdminOp" in body, ( + f"{method} is unsigned — any member could use it") def test_only_the_operator_is_offered_the_setting(): - panel = _component(GROUP_SETTINGS.read_text(encoding="utf-8"), "GroupSettingsPanel") - section = panel[panel.index("members.uploads_title") - 400: - panel.index("members.uploads_title")] - assert "isNodeAdmin && connected" in section + panel = _component(GROUP_SETTINGS.read_text(encoding="utf-8"), + "GroupSettingsPanel") + section = panel[panel.index("settings_node.shared_directories_title") - 600: + panel.index("settings_node.shared_directories_title")] + assert "isNodeAdmin &&" in section + + +def test_the_operator_is_offered_it_on_the_web_too(): + """ + An operator is not necessarily sitting at their node. The first version of + this section required the loopback API, which resolves to "not available" + in a browser — so it rendered for nobody on the web, while the upload + controls it replaced had worked there. + """ + source = GROUP_SETTINGS.read_text(encoding="utf-8") + panel = _component(source, "GroupSettingsPanel") + section = panel[panel.index("settings_node.shared_directories_title") - 600: + panel.index("settings_node.shared_directories_title")] + assert "connected ||" in section, ( + "the shared directories section still requires a local node") + + table = _component(source, "SharedDirectoriesTable") + for call in ("transport.updateRoot", "transport.ejectRoot", + "transport.plugRoot", "transport.removeRoot", + "transport.addRoot"): + assert call in table, f"{call} has no MNP route from the table" + + +def test_the_roots_shown_come_from_the_live_connection_when_there_is_one(): + """ + The loopback list is a second source, and the two drift: it is read once on + mount and after a change, while the MNP one is pushed. Preferring MNP also + keeps this table on the same data Files reads, so an eject shows in both at + the same instant. + """ + panel = _component(GROUP_SETTINGS.read_text(encoding="utf-8"), + "GroupSettingsPanel") + assert "const effectiveRoots = (connected && mnpRoots" in panel -- cgit v1.2.3