diff options
| author | Christophe Besson <cbesson@gmail.com> | 2026-09-06 17:48:36 +0200 |
|---|---|---|
| committer | Christophe Besson <cbesson@gmail.com> | 2026-09-06 17:48:36 +0200 |
| commit | ea56b8c79538323875c00db2e7006b255f7cd494 (patch) | |
| tree | ee08835bc190a75e49a6a8e78755111aef0e678f /packages/meshbay-node | |
| parent | e76e27868b30a2b00b1ba42dd8e7ee6071e0c0d7 (diff) | |
| download | meshbay-ea56b8c79538323875c00db2e7006b255f7cd494.tar.gz | |
fix(groups): finish Phase 1 — MNP root management, upload targets, eject state
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us
Diffstat (limited to 'packages/meshbay-node')
19 files changed, 1086 insertions, 324 deletions
diff --git a/packages/meshbay-node/src/meshbay_node/daemon.py b/packages/meshbay-node/src/meshbay_node/daemon.py index f45101f..4fc07ad 100644 --- a/packages/meshbay-node/src/meshbay_node/daemon.py +++ b/packages/meshbay-node/src/meshbay_node/daemon.py @@ -36,6 +36,7 @@ from pathlib import Path import uvicorn +from meshbay_common.paths import fold from meshbay_common import MNP_VERSION from meshbay_common.protocol import MNP from meshbay_node.audit import AuditStore @@ -128,6 +129,11 @@ class _WsSender: # ── Daemon ──────────────────────────────────────────────────────────────────── +def _root_shape(roots) -> set[tuple]: + """What has to match for a group's roots to count as unchanged on reload.""" + return {(r.name, str(r.path), r.writable, r.removable) for r in roots} + + class NodeDaemon: def __init__(self, config: Config, config_path: Path = DEFAULT_CONFIG_PATH): self._config = config @@ -303,7 +309,7 @@ class NodeDaemon: continue try: - roots = RootSet.build([asdict(r) for r in group_cfg.roots]) + roots = await self._build_roots(group_cfg) except RootError as e: # Configuration the operator has to fix; guessing would put # a member's file on the wrong disk or index one twice. @@ -330,7 +336,7 @@ class NodeDaemon: log.info("No GEK yet for group %s — will accept first setup", group_cfg.name) - # Read once at load, like member_upload/enabled_apps below — + # Read once at load, like enabled_apps below — # kept current in place afterwards by set_scan_settings # (ops.py), which updates both this indexer object directly # and roster.db, so a restart picks up the same values. @@ -347,6 +353,7 @@ class NodeDaemon: sk_node=keys.sk_ed25519, gek=gek, on_change=self._on_index_change, + on_root_ejected=self._eject_persister(group_cfg.id), cache=self._index_cache, reconcile_secs=scan_settings["reconcile_interval_secs"], debounce_secs=scan_settings["debounce_secs"], @@ -374,22 +381,19 @@ class NodeDaemon: "note_activity": indexer.note_activity, # Shown to the operator in Settings, and kept current in # place by set_scan_settings (ops.py) — same reasoning as - # member_upload below. + # enabled_apps below. "reconcile_interval_secs": scan_settings["reconcile_interval_secs"], "debounce_secs": scan_settings["debounce_secs"], "visibility": group_cfg.visibility, # Admission policy comes from node.toml, never from the hub: # a hub that could declare a group open would be handed its key. "join_policy": group_cfg.join_policy, - # Whether ordinary members may upload. Read once here, into - # the context, because the upload handler is synchronous and - # a database round trip per chunk would be absurd. The - # signed operation that changes it updates this dict in - # place, so the two never drift within a run. - "member_upload": await self._roster.member_upload_allowed( - group_cfg.id) if self._roster else True, - # Same reasoning: read once at load, kept current in place - # by the signed operation that changes it. + # Read once at load, kept current in place by the signed + # operation that changes it — the upload handler is + # synchronous and a database round trip per chunk would be + # absurd. (Whether a member may upload is not here any + # more: it is `writable` on the root being written to, + # which the RootSet above already carries.) "enabled_apps": await self._roster.enabled_apps( group_cfg.id) if self._roster else list(Roster.DEFAULT_APPS), # Which folder is the Videos app's entry point for this @@ -745,14 +749,16 @@ class NodeDaemon: if not ctx: continue try: - roots = RootSet.build([asdict(r) for r in group_cfg.roots]) + roots = await self._build_roots(group_cfg) except RootError as e: log.error("Group %r: %s — keeping the roots already loaded", group_cfg.name, e) continue - before = {(r.name, str(r.path)) for r in ctx["roots"]} - after = {(r.name, str(r.path)) for r in roots} - if before == after: + # `writable` and `removable` are in the comparison because an + # operator editing node.toml by hand and reloading is a supported + # way to change them, and a set compared on name and path alone + # reports "nothing changed" for exactly that edit. + if _root_shape(ctx["roots"]) == _root_shape(roots): continue roots.refresh_availability() indexer = next((i for i in self._indexers @@ -786,7 +792,7 @@ class NodeDaemon: continue try: - roots = RootSet.build([asdict(r) for r in group_cfg.roots]) + roots = await self._build_roots(group_cfg) except RootError as e: log.error("New group %r: %s — skipping", group_cfg.name, e) continue @@ -812,6 +818,7 @@ class NodeDaemon: sk_node=sk_ed, gek=gek, on_change=self._on_index_change, + on_root_ejected=self._eject_persister(group_cfg.id), cache=self._index_cache, reconcile_secs=scan_settings["reconcile_interval_secs"], debounce_secs=scan_settings["debounce_secs"], @@ -844,9 +851,6 @@ class NodeDaemon: "debounce_secs": scan_settings["debounce_secs"], "visibility": group_cfg.visibility, "join_policy": group_cfg.join_policy, - "member_upload": ( - await self._roster.member_upload_allowed(group_cfg.id) - if self._roster else True), "enabled_apps": ( await self._roster.enabled_apps(group_cfg.id) if self._roster else list(Roster.DEFAULT_APPS)), @@ -1052,6 +1056,35 @@ class NodeDaemon: log.debug("Index progress pushed to %d peer(s) for group %s", pushed, group_id[:8]) + async def _build_roots(self, group_cfg) -> RootSet: + """ + Build a group's RootSet from node.toml, with the ejected state restored. + + node.toml carries configuration (`writable`, `removable`); the roster + carries the runtime answer to "is this drive ejected right now". They + are merged here, in the one place every caller goes through, because a + root that quietly comes back available across a restart is exactly the + surprise unplug that eject exists to survive. + """ + specs = [asdict(r) for r in group_cfg.roots] + if self._roster: + ejected = await self._roster.ejected_roots(group_cfg.id) + if ejected: + for spec in specs: + name = spec.get("name") or Path(spec.get("path", "")).name + if fold(name) in ejected: + spec["ejected"] = True + return RootSet.build(specs) + + def _eject_persister(self, group_id: str): + """`on_root_ejected` bound to one group, for that group's indexer.""" + async def persist(root_name: str, ejected: bool) -> None: + if self._roster: + await self._roster.set_root_ejected( + group_id, root_name, ejected, + set_by=self._state.get("node_user_id", "")) + return persist + async def _on_index_change(self, indexer: DirectoryIndexer) -> None: """ Called when a DirectoryIndexer detects file changes — once per @@ -1716,10 +1749,10 @@ def main() -> None: help="group id (optional if only one is configured)") parser.add_argument("--writable", action="store_true", default=None, dest="writable", - help="mark root as read-write (root set/add)") + help="root accepts member uploads (root add/set)") parser.add_argument("--no-writable", action="store_false", dest="writable", - help="mark root as read-only (root set)") + help="root is read-only (root add/set, group add)") parser.add_argument("--removable", action="store_true", default=None, dest="removable", help="mark root as removable (root set/add)") @@ -2481,20 +2514,22 @@ def main() -> None: sys.exit(1) cfg = load_config(args.config or DEFAULT_CONFIG_PATH) - body = {"name": args.target, "shared_dir": args.dir} + # Writable unless the operator says otherwise: a brand-new group that + # cannot receive a single file until its owner finds a second command + # is not a working group. Every root added *later* is read-only by + # default, which is the opposite rule and the right one there. + writable = args.writable is not False + body = {"name": args.target, "shared_dir": args.dir, + "writable": writable} if args.upload_dir: - import warnings - warnings.warn( - "--upload-dir is deprecated; the main root is writable by " - "default. Use 'meshbay-node root add' for additional roots.", - DeprecationWarning, stacklevel=1) print("WARNING: --upload-dir is deprecated. The shared directory is " - "writable by default. Use 'meshbay-node root add' for " - "additional roots.") + "read-write by default; use 'meshbay-node root add " + "<path> --writable' for a second one.") body["upload_dir"] = args.upload_dir out = _daemon_api(cfg, "/api/groups/attach", method="POST", body=body) print(f"{out['name']} ({out['group_id'][:8]}) added to {out['config']}") - print(f" shared_dir {out['shared_dir']} (writable)") + print(f" shared_dir {out['shared_dir']}" + f" ({'read-write' if writable else 'read-only'})") if out.get("upload_dir"): print(f" upload_dir {out['upload_dir']}") print() @@ -2512,7 +2547,7 @@ def main() -> None: group_id = _resolve_group(cfg, args.group) if sub == "list": - out = _daemon_api(cfg, f"/api/groups") + out = _daemon_api(cfg, "/api/groups") group = next((g for g in out.get("groups", []) if g["id"] == group_id), None) if not group: diff --git a/packages/meshbay-node/src/meshbay_node/indexer/indexer.py b/packages/meshbay-node/src/meshbay_node/indexer/indexer.py index 2911278..f7ffdca 100644 --- a/packages/meshbay-node/src/meshbay_node/indexer/indexer.py +++ b/packages/meshbay-node/src/meshbay_node/indexer/indexer.py @@ -263,12 +263,17 @@ class DirectoryIndexer: cache: IndexCache | None = None, reconcile_secs: float = DEFAULT_RECONCILE_SECS, debounce_secs: float = DEFAULT_DEBOUNCE_SECS, + on_root_ejected: Callable[[str, bool], Awaitable[None]] | None = None, ): self.roots = roots self.group_id = group_id self.sk_node = sk_node self.gek = gek self.on_change = on_change + # Called with (root_name, ejected) whenever this indexer changes a + # root's ejected state by itself — the surprise-unplug safety net. + # The daemon writes it to the roster, so a restart does not undo it. + self.on_root_ejected = on_root_ejected self.reconcile_secs = reconcile_secs self.debounce_secs = debounce_secs # Current backoff delay — starts at reconcile_secs, doubles on every @@ -582,6 +587,17 @@ class DirectoryIndexer: changed = self.roots.refresh_availability() touched = False + # Drained before the loop below, because persisting the flag is what + # makes the safety net survive a restart — and a restart is exactly + # what an operator does after noticing a drive fell off. + while self.roots.auto_ejected: + name = self.roots.auto_ejected.pop(0) + if self.on_root_ejected: + try: + await self.on_root_ejected(name, True) + except Exception: + log.exception("Could not persist the auto-eject of root %r", name) + for root, available in changed: if available: log.info("Root %r is back — rescanning", root.name) diff --git a/packages/meshbay-node/src/meshbay_node/ops.py b/packages/meshbay-node/src/meshbay_node/ops.py index c3a3f9c..13a7250 100644 --- a/packages/meshbay-node/src/meshbay_node/ops.py +++ b/packages/meshbay-node/src/meshbay_node/ops.py @@ -390,7 +390,7 @@ async def list_groups(state: dict) -> dict: async def attach_group(state: dict, name: str, shared_dir: str, - upload_dir: str = "") -> dict: + upload_dir: str = "", writable: bool = True) -> dict: """ Write a new [[groups]] block into node.toml. @@ -448,7 +448,7 @@ async def attach_group(state: dict, name: str, shared_dir: str, # Forward slashes: a Windows path in a TOML basic string is a # parse error (`\U`, `\a`, ... are escapes). pathlib reads `/`. f' path = "{path.as_posix()}"\n' - f' writable = true\n') + f' writable = {"true" if writable else "false"}\n') try: with conf_path.open("a", encoding="utf-8", newline="\n") as f: f.write(block) @@ -457,6 +457,7 @@ async def attach_group(state: dict, name: str, shared_dir: str, result = {"group_id": group["id"], "name": group["name"], "shared_dir": str(path), "config": str(conf_path), + "writable": writable, "note": "restart the node to pick it up"} return result @@ -629,7 +630,7 @@ def _remove_roots_block(conf_path: Path, group_id: str, conf_path.write_text("\n".join(new_lines), encoding="utf-8", newline="\n") return - raise OpError(f"Root path not found in config", status=404) + raise OpError("Root path not found in config", status=404) async def add_root(state: dict, group_id: str, path: str, *, @@ -670,9 +671,9 @@ async def add_root(state: dict, group_id: str, path: str, *, if kind != "generic": root_block += f'\n kind = "{added.kind}"' if writable: - root_block += f'\n writable = true' + root_block += '\n writable = true' if removable: - root_block += f'\n removable = true' + root_block += '\n removable = true' _insert_roots_block(conf_path, group_id, root_block) from meshbay_node.config import RootSpec @@ -822,18 +823,17 @@ async def eject_root(state: dict, group_id: str, root_name: str) -> dict: return {"status": "already_ejected", "name": root_name, "group_id": group_id, "roots": roots.describe()} - root.ejected = True - root.available = False - - roster = _roster(state) - if roster: - await roster.set_setting( - group_id, f"root_ejected:{fold(root_name)}", "1", - set_by=state.get("node_user_id", "")) - + # The indexer stops its watchdog and freezes the entries; it holds the same + # RootSet object, but the flags are set here too so a context whose indexer + # was replaced by a retarget cannot be left disagreeing with the roster. indexer = state.get("indexers", {}).get(group_id) if indexer: indexer.eject_root(root_name) + root.ejected = True + root.available = False + + await _roster(state).set_root_ejected( + group_id, root_name, True, set_by=state.get("node_user_id", "")) log.info("Root ejected: %s from group %s", root_name, group_id[:8]) return {"status": "ejected", "name": root_name, "group_id": group_id, @@ -869,18 +869,17 @@ async def plug_root(state: dict, group_id: str, root_name: str) -> dict: f"Directory not found: {root.path}. Is the device connected?", status=409) - root.ejected = False - root.available = True - - roster = _roster(state) - if roster: - await roster.set_setting( - group_id, f"root_ejected:{fold(root_name)}", "0", - set_by=state.get("node_user_id", "")) + # Persisted before the rescan, which can take minutes on a large library: + # a crash halfway through must leave the root plugged, not ejected with + # entries half rebuilt. + await _roster(state).set_root_ejected( + group_id, root_name, False, set_by=state.get("node_user_id", "")) indexer = state.get("indexers", {}).get(group_id) if indexer: await indexer.plug_root(root_name) + root.ejected = False + root.available = root.is_live() log.info("Root plugged: %s in group %s", root_name, group_id[:8]) return {"status": "plugged", "name": root_name, "group_id": group_id, @@ -947,7 +946,7 @@ def _update_root_field(conf_path: Path, group_id: str, conf_path.write_text("\n".join(lines), encoding="utf-8", newline="\n") return - raise OpError(f"Root path not found in config", status=404) + raise OpError("Root path not found in config", status=404) # ── Files ──────────────────────────────────────────────────────────────────── @@ -1125,6 +1124,8 @@ async def set_enabled_apps(state: dict, group_id: str, apps: list[str]) -> dict: """ roster = _roster(state) ctx = _group_ctx(state, group_id) + # See the same guard in webrtc_server._do_apps_enabled: Files cannot be + # turned off, and both writers put it at the front so the two agree. if "files" not in apps: apps = ["files"] + list(apps) await roster.set_enabled_apps(group_id, apps, diff --git a/packages/meshbay-node/src/meshbay_node/roots.py b/packages/meshbay-node/src/meshbay_node/roots.py index a83b729..9d3f7cb 100644 --- a/packages/meshbay-node/src/meshbay_node/roots.py +++ b/packages/meshbay-node/src/meshbay_node/roots.py @@ -165,6 +165,13 @@ class RootSet: roots: list[Root] = field(default_factory=list) + # Roots this set ejected by itself — a removable device that went away + # without the operator clicking Eject. Drained by the indexer, which is + # the only caller holding a roster to write the state to. Without that + # the flag is lost on the next restart, and the surprise unplug looks + # like a deletion all over again on the pass after it. + auto_ejected: list[str] = field(default_factory=list) + # ── Construction ───────────────────────────────────────────────────────── @classmethod @@ -212,9 +219,14 @@ class RootSet: # Backward compat: old configs use `upload` instead of `writable` writable = bool(spec.get("writable", spec.get("upload", False))) + # `ejected` is runtime state, not configuration — it reaches here + # only from the roster, restored at startup so a drive ejected + # before a restart does not come back on its own. root = Root(name=name, path=path, kind=kind, writable=writable, removable=bool(spec.get("removable", False)), + ejected=bool(spec.get("ejected", False)), + available=not bool(spec.get("ejected", False)), direct=bool(spec.get("direct", False))) _refuse_nesting(root, roots) roots.append(root) @@ -331,8 +343,13 @@ class RootSet: changed.append((root, False)) continue live = root.is_live() - if not live and root.removable and not root.ejected: + if not live and root.removable: root.ejected = True + # Recorded for the caller to persist. A flag that only lives + # in memory would be forgotten on the next restart, and the + # rescan that followed would read an empty mount point as an + # erased library — the exact outcome eject exists to prevent. + self.auto_ejected.append(root.name) log.warning("Root %r auto-ejected (device disappeared): %s", root.name, root.path) if live != root.available: diff --git a/packages/meshbay-node/src/meshbay_node/roster.py b/packages/meshbay-node/src/meshbay_node/roster.py index e2f749f..5f38acd 100644 --- a/packages/meshbay-node/src/meshbay_node/roster.py +++ b/packages/meshbay-node/src/meshbay_node/roster.py @@ -31,6 +31,8 @@ from pathlib import Path import aiosqlite +from meshbay_common.paths import fold + log = logging.getLogger(__name__) # Crockford base32 without I, L, O and U: no character pair a human can confuse @@ -543,10 +545,40 @@ class Roster: # ── Group settings ────────────────────────────────────────────────────── - # Whether members who are not the operator may upload. Default is yes: a - # group that nobody may add to is the unusual case, and an existing node - # must not change behaviour because a table was added under it. - SETTING_MEMBER_UPLOAD = "member_upload" + # Whether a root is ejected. Runtime state, one key per root, keyed by the + # *folded* name so it agrees with the case-insensitive comparison the rest + # of the root code makes. It lives here rather than in node.toml because it + # is not configuration — an operator's hand-written config file should not + # be rewritten because a USB drive was unplugged — and it has to survive a + # restart, or the rescan that follows reads an empty mount point as an + # erased library, which is the whole thing eject exists to prevent. + SETTING_ROOT_EJECTED_PREFIX = "root_ejected:" + + @classmethod + def root_ejected_key(cls, root_name: str) -> str: + return cls.SETTING_ROOT_EJECTED_PREFIX + fold(root_name) + + async def set_root_ejected(self, group_id: str, root_name: str, + ejected: bool, set_by: str = "") -> None: + await self.set_setting(group_id, self.root_ejected_key(root_name), + "1" if ejected else "0", set_by) + + async def ejected_roots(self, group_id: str) -> set[str]: + """ + The folded names of this group's ejected roots. + + Matched in Python rather than with `LIKE 'root_ejected:%'`: `_` is a + single-character wildcard there, so that pattern also matches keys this + does not own. A group has a handful of settings rows, so reading them + all costs nothing and the prefix test is then exact. + """ + prefix = self.SETTING_ROOT_EJECTED_PREFIX + async with self._db.execute( + "SELECT key, value FROM group_settings WHERE group_id = ?", + (group_id,)) as cur: + rows = await cur.fetchall() + return {r["key"][len(prefix):] for r in rows + if r["key"].startswith(prefix) and r["value"] == "1"} async def get_setting(self, group_id: str, key: str, default: str | None = None) -> str | None: @@ -567,17 +599,6 @@ class Roster: (group_id, key, value, set_by, _now())) await self._db.commit() - async def member_upload_allowed(self, group_id: str) -> bool: - """Whether an ordinary member may upload to this group.""" - value = await self.get_setting(group_id, self.SETTING_MEMBER_UPLOAD, "1") - return value != "0" - - async def set_member_upload(self, group_id: str, allowed: bool, - set_by: str = "") -> bool: - await self.set_setting(group_id, self.SETTING_MEMBER_UPLOAD, - "1" if allowed else "0", set_by) - return allowed - # Which group "applications" (Chat, Files, and whatever registers later in # apps.js) are shown to members. Unset means every app that exists — an # existing group's tabs must not disappear because a node was upgraded. @@ -609,7 +630,7 @@ class Roster: # user_id)` authorizing the operator node-wide (desktop-client-v1.md # §6.3). Unset means "the shipped default token, TMDB's own default # language" — the same "absent means the old behaviour" discipline - # member_upload/enabled_apps already follow. + # enabled_apps already follows. # # Whether TMDB is used *at all*, though, is per-group (moved off the # node-wide sentinel below, 2026-08-24): an operator running a real media diff --git a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py index 4d6dd34..b99affc 100644 --- a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py +++ b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py @@ -1787,7 +1787,7 @@ class WebRTCPeerSession: """ Turn a group "application" on or off for everyone, for this group. - Signed like `member_upload`: this decides what a member sees, and an + Signed like the root ops: this decides what a member sees, and an unsigned message would let any member turn a disabled one back on. """ apps = msg.get("apps") @@ -1799,8 +1799,12 @@ class WebRTCPeerSession: self._send({"type": "error", "detail": f"Unknown app(s): {', '.join(sorted(unknown))}"}) return + # Files is not a toggle: MNP permits root exploration regardless of + # what this list says, so hiding the tab only ever misled. Added at the + # front, the same order ops.set_enabled_apps writes, so the landing-tab + # preference sees one list and not two. if "files" not in apps: - apps.append("files") + apps.insert(0, "files") if not self._has_admin_authority(): self._send({"type": "error", "detail": "No authorized key for this"}) return @@ -1884,7 +1888,7 @@ class WebRTCPeerSession: self._audit("tmdb_config", pending["subject"]) # Node-wide setting: every connected peer in every group is told, not - # just this group's peers (unlike apps_enabled/member_upload/the + # just this group's peers (unlike apps_enabled/the root ops/the # per-group tmdb_enabled below). notice = { "type": MNP.TMDB_CONFIG_ACK, "v": MNP_VERSION, @@ -3795,24 +3799,34 @@ class WebRTCPeerSession: "filename": filename}) return - # The client names the target root. If absent, pick the first writable - # one (backward compat with old clients that don't send it). - target_root_name = msg.get("root") + # The client names the root it is uploading into — it is browsing one, + # and with several writable roots any other choice is a guess. It names + # a root, never a path: the destination inside it is decided below and + # is not negotiable, which is what keeps C5a closed. + # + # An unknown name is refused rather than falling back to a writable + # root, because "the file went somewhere else" is discovered weeks + # later — the same reason the old single upload root was never guessed. + # A client that names nothing is an MNP 1.0 one, and there was exactly + # one destination in its world: the first writable root. + target_root_name = str(msg.get("root") or "").strip() upload_root = None if target_root_name: - from meshbay_common.paths import fold - target_folded = fold(target_root_name) - for r in roots: - if fold(r.name) == target_folded: - upload_root = r - break + upload_root = roots.by_name(target_root_name) + if upload_root is None: + self._send({"type": "error", + "detail": f"No directory named " + f"{target_root_name!r} in this group", + "code": "no_such_root", + "filename": filename}) + return else: writable = roots.writable_roots upload_root = writable[0] if writable else None if upload_root is None: self._send({"type": "error", - "detail": "No writable directory found for uploads", + "detail": "No writable directory in this group", "code": "no_writable_root", "filename": filename}) return @@ -3827,6 +3841,7 @@ class WebRTCPeerSession: self._send({"type": "error", "detail": f"Directory '{upload_root.name}' is " f"currently unavailable", + "code": "root_unavailable", "filename": filename}) return diff --git a/packages/meshbay-node/src/meshbay_node/ui/app.py b/packages/meshbay-node/src/meshbay_node/ui/app.py index 3d24000..ef86ce3 100644 --- a/packages/meshbay-node/src/meshbay_node/ui/app.py +++ b/packages/meshbay-node/src/meshbay_node/ui/app.py @@ -176,6 +176,7 @@ def create_ui_app(state: dict) -> FastAPI: (payload.get("name") or "").strip(), (payload.get("shared_dir") or "").strip(), upload_dir=(payload.get("upload_dir") or "").strip(), + writable=bool(payload.get("writable", True)), )) reload_fn = state.get("reload_fn") if reload_fn: @@ -419,14 +420,6 @@ def create_ui_app(state: dict) -> FastAPI: "current_dir": progress.current_dir, } - # ── Upload toggle (DEPRECATED — per-root writable replaces this) ──── - - @app.put("/api/groups/{group_id}/member-upload") - async def set_member_upload(group_id: str, payload: dict): - log.warning("PUT member-upload is deprecated — use PATCH roots/{name} " - "with writable instead") - return {"deprecated": True, "message": "Use per-root writable flag"} - # ── Enabled apps (operator only, localhost) ──────────────────────────── # # Same loopback shape as member-upload: the Create Group wizard sets this diff --git a/packages/meshbay-node/tests/conftest.py b/packages/meshbay-node/tests/conftest.py index 20724aa..3dc9cd9 100644 --- a/packages/meshbay-node/tests/conftest.py +++ b/packages/meshbay-node/tests/conftest.py @@ -24,14 +24,18 @@ win32_todo = pytest.mark.skipif( ) -def one_root(path: Path, *, name: str = "", kind: str = "generic") -> RootSet: +def one_root(path: Path, *, name: str = "", kind: str = "generic", + writable: bool = True) -> RootSet: """ - A RootSet with a single root over `path`, receiving uploads. + A RootSet with a single writable root over `path`. The equivalent of the old `shared_dir`. Note what it implies for assertions: a file directly in `path` now has `entry.path == <basename of path>`, not `""` — every index path carries its root name, in a group with one root as much as in a group with five. + + Writable by default because most callers are testing something else and + want a root an upload can reach. `writable=False` is the read-only group. """ return RootSet.build([{"path": str(path), "name": name, "kind": kind, - "upload": True}]) + "writable": writable}]) diff --git a/packages/meshbay-node/tests/test_apps_enabled_policy.py b/packages/meshbay-node/tests/test_apps_enabled_policy.py index 671005a..ac44ab3 100644 --- a/packages/meshbay-node/tests/test_apps_enabled_policy.py +++ b/packages/meshbay-node/tests/test_apps_enabled_policy.py @@ -1,7 +1,7 @@ """ The operator decides which group "applications" (Chat, Files, ...) are shown. -Same shape as `test_member_upload_policy.py`, because it is the same kind of +Same shape as `test_root_writable_policy.py`, because it is the same kind of setting: changed by a signed operator instruction, stored on the node rather than the hub, and safe for an existing group to have never heard of. The two things specific to this one: the whole set is signed in one message rather @@ -88,7 +88,7 @@ async def test_a_request_with_nobody_to_authorize_it_is_refused(tmp_path): async def test_changing_it_needs_a_signature(tmp_path): """The request only ever produces a challenge. Nothing is applied until a - signature over the transcript verifies — the same path as member_upload.""" + signature over the transcript verifies — the same path as the root ops.""" session = _session(tmp_path, "op", operator="op") session._has_admin_authority = lambda: True issued = [] diff --git a/packages/meshbay-node/tests/test_cli_dispatch.py b/packages/meshbay-node/tests/test_cli_dispatch.py index f58c020..2ba251f 100644 --- a/packages/meshbay-node/tests/test_cli_dispatch.py +++ b/packages/meshbay-node/tests/test_cli_dispatch.py @@ -26,6 +26,15 @@ VERBS = [ ["status"], ["group", "list"], ["group", "add"], # missing --dir: usage, then exit + ["group", "add", "g", "--dir", "/tmp/media", "--no-writable"], + ["root", "list"], + ["root", "add"], # missing path: usage, then exit + ["root", "add", "/tmp/media", "--writable", "--removable"], + ["root", "remove", "media", "--yes"], + ["root", "set", "media", "--no-writable"], + ["root", "set", "media"], # nothing to change: usage, then exit + ["root", "eject", "media"], + ["root", "plug", "media"], ["gek", "init"], ["gek", "rotate", "--yes"], ["gek-init"], diff --git a/packages/meshbay-node/tests/test_member_upload_policy.py b/packages/meshbay-node/tests/test_member_upload_policy.py deleted file mode 100644 index b1dc0cb..0000000 --- a/packages/meshbay-node/tests/test_member_upload_policy.py +++ /dev/null @@ -1,176 +0,0 @@ -""" -The operator can close uploading to everyone but themselves. - -The point of these tests is the difference between a hidden button and a closed -door. The interface stops offering the control, which is a courtesy to the -people who are not trying; **the node refuses the upload**, which is the part -that holds against someone who is. A member who kept an old tab open, or who -speaks MNP directly, gets the same answer as everyone else. - -Two further things are worth holding: - -* the setting is changed by a **signed** operator instruction. A node that took - it from an unsigned message would let any member turn it back on, and the - control would be a suggestion; -* it is stored on the **node**, not the hub. A hub that could decide who may - write to the operator's disk is a hub with authority over the node, which is - the thing this whole design is arranged to avoid. -""" - -import base64 -from pathlib import Path - -import pytest -from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey - -from meshbay_common.adminop import OP_MEMBER_UPLOAD -from meshbay_node.indexer.group_index import GroupIndex -from meshbay_node.roster import Roster -from meshbay_node.transport.webrtc_server import WebRTCPeerSession - -from conftest import one_root - -pytestmark = pytest.mark.asyncio - - -def _session(tmp_path: Path, user_id: str, *, member_upload: bool, - operator: str | None = None) -> WebRTCPeerSession: - shared_root = tmp_path / "shared" - shared_root.mkdir(exist_ok=True) - index = GroupIndex(group_id="g" * 32, sk_node=Ed25519PrivateKey.generate()) - ctx = { - "roots": one_root(shared_root), - "index": index, - "sk_node": index.sk_node, - "member_upload": member_upload, - "node_user_id": operator, - } - session = WebRTCPeerSession.__new__(WebRTCPeerSession) - session._ctx = ctx - session._group_id = None - session._user_id = user_id - session._pk_user = "" - session._uploads = {} - session.sent = [] - session._send = session.sent.append - session._audit = lambda *a, **k: None - return session - - -def _upload(session, filename="clip.mp4", body=b"bytes"): - session._do_file_upload({ - "filename": filename, "chunk_index": 0, "total_chunks": 1, - "data": base64.b64encode(body).decode(), - }) - - -def _uploads_dir(session) -> Path: - return session._ctx["roots"].upload_root.path / "uploads" - - -# ── The door, not the button ──────────────────────────────────────────────── - -async def test_a_member_cannot_upload_when_it_is_turned_off(tmp_path): - session = _session(tmp_path, "member-1", member_upload=False, - operator="the-operator") - _upload(session) - - assert not (_uploads_dir(session) / "clip.mp4").exists(), ( - "the file was written even though uploading is off — the setting is " - "decorative and the hidden button was the whole control") - refusal = [m for m in session.sent if m.get("type") == "error"] - assert refusal and refusal[0].get("code") == "member_upload_off" - - -async def test_the_operator_can_still_upload(tmp_path): - """Otherwise turning it off locks the operator out of their own node, and - the only way back is a config file and a restart.""" - session = _session(tmp_path, "the-operator", member_upload=False, - operator="the-operator") - _upload(session) - - assert (_uploads_dir(session) / "clip.mp4").read_bytes() == b"bytes" - - -async def test_members_upload_normally_when_it_is_on(tmp_path): - session = _session(tmp_path, "member-1", member_upload=True, - operator="the-operator") - _upload(session) - - assert (_uploads_dir(session) / "clip.mp4").read_bytes() == b"bytes" - - -async def test_a_node_that_never_heard_of_the_setting_still_accepts_uploads(tmp_path): - """An existing node's context has no such key. The absence must read as - "allowed", or upgrading the node silently closes every group.""" - session = _session(tmp_path, "member-1", member_upload=True, - operator="the-operator") - del session._ctx["member_upload"] - _upload(session) - - assert (_uploads_dir(session) / "clip.mp4").read_bytes() == b"bytes" - - -# ── Who may change it ─────────────────────────────────────────────────────── - -async def test_changing_it_needs_a_signature(tmp_path): - """ - The request only ever produces a challenge. Nothing is applied until a - signature over the transcript verifies — the same path as removing a member. - """ - session = _session(tmp_path, "member-1", member_upload=True, - operator="the-operator") - session._has_admin_authority = lambda: True - issued = [] - session._issue_admin_challenge = lambda op, subject: issued.append((op, subject)) - - session._do_member_upload({"allowed": False}) - - assert issued == [(OP_MEMBER_UPLOAD, "off")] - assert session._ctx["member_upload"] is True, "applied before it was signed" - - -async def test_the_subject_names_the_outcome_not_the_operation(tmp_path): - """The operator is shown the subject before signing. "member_upload" tells - them nothing; "off" tells them what they are about to do.""" - session = _session(tmp_path, "op", member_upload=False, operator="op") - session._has_admin_authority = lambda: True - issued = [] - session._issue_admin_challenge = lambda op, subject: issued.append((op, subject)) - - session._do_member_upload({"allowed": True}) - - assert issued == [(OP_MEMBER_UPLOAD, "on")] - - -async def test_a_request_with_nobody_to_authorize_it_is_refused(tmp_path): - session = _session(tmp_path, "member-1", member_upload=True, - operator="the-operator") - session._has_admin_authority = lambda: False - - session._do_member_upload({"allowed": False}) - - assert [m for m in session.sent if m.get("type") == "error"] - - -# ── Where it is stored ────────────────────────────────────────────────────── - -async def test_the_setting_lives_on_the_node_and_survives_a_restart(tmp_path): - roster = Roster(db_path=tmp_path / "roster.db") - await roster.open() - try: - assert await roster.member_upload_allowed("g1") is True, ( - "absent must mean allowed, or an upgrade closes every group") - await roster.set_member_upload("g1", False, set_by="op") - assert await roster.member_upload_allowed("g1") is False - finally: - await roster.close() - - reopened = Roster(db_path=tmp_path / "roster.db") - await reopened.open() - try: - assert await reopened.member_upload_allowed("g1") is False - assert await reopened.member_upload_allowed("g2") is True, ( - "one group's setting must not answer for another") - finally: - await reopened.close() diff --git a/packages/meshbay-node/tests/test_node_status.py b/packages/meshbay-node/tests/test_node_status.py index b56eb6e..091b1db 100644 --- a/packages/meshbay-node/tests/test_node_status.py +++ b/packages/meshbay-node/tests/test_node_status.py @@ -255,7 +255,7 @@ async def test_add_root_creates_directory_and_returns_info(tmp_path): from meshbay_node.config import NodeConfig, GroupConfig, RootSpec cfg = GroupConfig(id=GROUP, name="test", roots=[ - RootSpec(path=str(shared), name="shared", kind="generic", upload=True), + RootSpec(path=str(shared), name="shared", kind="generic", writable=True), ]) conf = tmp_path / "node.toml" @@ -295,7 +295,7 @@ async def test_remove_root_requires_at_least_one_remaining(tmp_path): from meshbay_node.config import GroupConfig, RootSpec, NodeConfig cfg = GroupConfig(id=GROUP, name="test", roots=[ - RootSpec(path=str(shared), name="shared", kind="generic", upload=True), + RootSpec(path=str(shared), name="shared", kind="generic", writable=True), ]) node_cfg = NodeConfig.__new__(NodeConfig) node_cfg.groups = [cfg] @@ -314,16 +314,22 @@ async def test_remove_root_requires_at_least_one_remaining(tmp_path): await ops.remove_root(state, GROUP, "shared") -async def test_remove_root_refuses_upload_root(tmp_path): - d1 = tmp_path / "uploads" +async def test_removing_a_writable_root_is_allowed(tmp_path): + """ + It used to be refused: with one designated upload root, removing it left + the group with nowhere to put an upload and no way to say so. Several roots + can be writable now, and a group with none is a valid read-only group — so + the refusal would be protecting a state that is no longer special. + """ + d1 = tmp_path / "incoming" d2 = tmp_path / "shared" d1.mkdir() d2.mkdir() from meshbay_node.config import GroupConfig, RootSpec, NodeConfig cfg = GroupConfig(id=GROUP, name="test", roots=[ - RootSpec(path=str(d1), name="uploads", kind="generic", upload=True), - RootSpec(path=str(d2), name="shared", kind="generic", upload=False), + RootSpec(path=str(d1), name="incoming", kind="generic", writable=True), + RootSpec(path=str(d2), name="shared", kind="generic", writable=False), ]) node_cfg = NodeConfig.__new__(NodeConfig) node_cfg.groups = [cfg] @@ -331,7 +337,7 @@ async def test_remove_root_refuses_upload_root(tmp_path): conf = tmp_path / "node.toml" conf.write_text( f'[[groups]]\nid = "{GROUP}"\nname = "test"\n\n' - f' [[groups.roots]]\n path = "{d1}"\n name = "uploads"\n upload = true\n\n' + f' [[groups.roots]]\n path = "{d1}"\n name = "incoming"\n writable = true\n\n' f' [[groups.roots]]\n path = "{d2}"\n name = "shared"\n') roots = RootSet.build([asdict(r) for r in cfg.roots]) index = GroupIndex(group_id=GROUP, sk_node=Ed25519PrivateKey.generate()) @@ -340,8 +346,97 @@ async def test_remove_root_refuses_upload_root(tmp_path): "config_path": str(conf), "groups_ctx": {GROUP: {"index": index, "roots": roots, "gek": b"\x01" * 32}}, } - with pytest.raises(ops.OpError, match="upload root"): - await ops.remove_root(state, GROUP, "uploads") + result = await ops.remove_root(state, GROUP, "incoming") + assert result["status"] == "removed" + assert [r["name"] for r in result["roots"]] == ["shared"] + assert conf.read_text().count("[[groups.roots]]") == 1 + + +async def test_update_root_rewrites_the_flags_in_node_toml(tmp_path): + """ + The flags live in the operator's config file, so they survive a restart — + and the file is hand-written and full of comments, so the change is a line + edit rather than a round trip through a TOML writer that would discard + every one of them. + """ + d1 = tmp_path / "media" + d1.mkdir() + + from meshbay_node.config import GroupConfig, RootSpec, NodeConfig + cfg = GroupConfig(id=GROUP, name="test", roots=[ + RootSpec(path=str(d1), name="media", kind="generic", writable=False), + ]) + node_cfg = NodeConfig.__new__(NodeConfig) + node_cfg.groups = [cfg] + + conf = tmp_path / "node.toml" + conf.write_text( + f'[[groups]]\nid = "{GROUP}"\nname = "test"\n\n' + f' [[groups.roots]]\n' + f' # the operator explained this one to themselves\n' + f' path = "{d1}"\n name = "media"\n') + roots = RootSet.build([asdict(r) for r in cfg.roots]) + index = GroupIndex(group_id=GROUP, sk_node=Ed25519PrivateKey.generate()) + state = { + "config": node_cfg, + "config_path": str(conf), + "groups_ctx": {GROUP: {"index": index, "roots": roots, "gek": b"\x01" * 32}}, + } + + result = await ops.update_root(state, GROUP, "media", + writable=True, removable=True) + assert result["status"] == "updated" + text = conf.read_text() + assert "writable = true" in text + assert "removable = true" in text + assert "the operator explained this one to themselves" in text, ( + "the config file was rewritten instead of edited") + + # And the live root set agrees immediately, without waiting for a reload: + # the loopback API reads it, and an operator who toggles a switch and sees + # it snap back assumes the change did not take. + assert roots.roots[0].writable is True + assert roots.roots[0].removable is True + + # A second call that changes nothing must not append a duplicate line. + await ops.update_root(state, GROUP, "media", writable=True, removable=True) + assert conf.read_text().count("writable =") == 1 + + +async def test_update_root_replaces_a_legacy_upload_line(tmp_path): + """ + A config written before the refactor says `upload = true`. Leaving it in + place next to a new `writable` line would give the file two answers, and + `RootSet.build` prefers `writable` — so the stale one would sit there + contradicting the running node for as long as anyone read it. + """ + d1 = tmp_path / "media" + d1.mkdir() + + from meshbay_node.config import GroupConfig, RootSpec, NodeConfig + cfg = GroupConfig(id=GROUP, name="test", roots=[ + RootSpec(path=str(d1), name="media", kind="generic", writable=True), + ]) + node_cfg = NodeConfig.__new__(NodeConfig) + node_cfg.groups = [cfg] + + conf = tmp_path / "node.toml" + conf.write_text( + f'[[groups]]\nid = "{GROUP}"\nname = "test"\n\n' + f' [[groups.roots]]\n path = "{d1}"\n name = "media"\n' + f' upload = true\n') + roots = RootSet.build([asdict(r) for r in cfg.roots]) + index = GroupIndex(group_id=GROUP, sk_node=Ed25519PrivateKey.generate()) + state = { + "config": node_cfg, + "config_path": str(conf), + "groups_ctx": {GROUP: {"index": index, "roots": roots, "gek": b"\x01" * 32}}, + } + + await ops.update_root(state, GROUP, "media", writable=False) + text = conf.read_text() + assert "upload = true" not in text + assert "writable = false" in text async def test_remove_root_succeeds_with_two_roots(tmp_path): @@ -352,8 +447,8 @@ async def test_remove_root_succeeds_with_two_roots(tmp_path): from meshbay_node.config import GroupConfig, RootSpec, NodeConfig cfg = GroupConfig(id=GROUP, name="test", roots=[ - RootSpec(path=str(d1), name="dir1", kind="generic", upload=True), - RootSpec(path=str(d2), name="dir2", kind="generic", upload=False), + RootSpec(path=str(d1), name="dir1", kind="generic", writable=True), + RootSpec(path=str(d2), name="dir2", kind="generic", writable=False), ]) node_cfg = NodeConfig.__new__(NodeConfig) node_cfg.groups = [cfg] diff --git a/packages/meshbay-node/tests/test_ops.py b/packages/meshbay-node/tests/test_ops.py index 92e32bf..b3f0378 100644 --- a/packages/meshbay-node/tests/test_ops.py +++ b/packages/meshbay-node/tests/test_ops.py @@ -12,11 +12,13 @@ call them. import asyncio import inspect from pathlib import Path +from types import SimpleNamespace import pytest from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey from meshbay_node import ops from meshbay_node.indexer.group_index import GroupIndex +from meshbay_node.roots import RootSet from meshbay_node.transport.quic_server import Denylist from conftest import one_root @@ -64,8 +66,8 @@ def test_the_http_adapter_adds_no_logic(): # Every endpoint that performs an operation routes through _op(...). for endpoint in ("operator_pair", "create_invite", "revoke_member", "unpin_member", "init_gek", "attach_group", "delete_file", - "add_root", "remove_root", "set_member_upload", - "reload_config"): + "add_root", "remove_root", "update_root", + "eject_root", "plug_root", "reload_config"): start = source.index(f"async def {endpoint}(") body = source[start:start + 700] assert "_op(" in body.split("\n\n")[0] + body, ( @@ -181,25 +183,103 @@ async def test_an_unhosted_group_offers_what_it_does_host(tmp_path): assert exc.value.extra.get("available") -# ── Upload policy (set_member_upload) ─────────────────────────────────────── +# ── Upload policy (per-root writable) ─────────────────────────────────────── -async def test_set_member_upload_toggles_and_persists(tmp_path): +async def test_the_group_wide_upload_switch_is_gone(tmp_path): + """ + `set_member_upload` was the whole of the old policy, and it is deliberately + not here any more — RO/RW on the root replaced it. A wrapper kept "for + compatibility" would be a second way to decide who writes to the operator's + disk, and two answers to that question is how C1 and C6 both happened. + """ + assert not hasattr(ops, "set_member_upload") + from meshbay_node.roster import Roster + assert not hasattr(Roster, "set_member_upload") + assert not hasattr(Roster, "member_upload_allowed") + + +async def test_eject_and_plug_persist_through_the_roster(tmp_path): + """ + The state has to outlive the process: an operator ejects a drive, unplugs + it, and restarts the node — and the rescan that follows must not read the + empty mount point as an erased library. + """ from meshbay_node.roster import Roster state = _state(tmp_path) + usb = tmp_path / "USB" + usb.mkdir() + state["groups_ctx"]["g" * 32]["roots"] = RootSet.build( + [{"path": str(usb), "removable": True, "writable": True}]) + state["config"] = SimpleNamespace( + groups=[SimpleNamespace(id="g" * 32, roots=[])]) roster = Roster(db_path=tmp_path / "roster.db") await roster.open() state["roster"] = roster state["node_user_id"] = "operator" + try: + out = await ops.eject_root(state, "g" * 32, "USB") + assert out["status"] == "ejected" + assert await roster.ejected_roots("g" * 32) == {"usb"} + assert out["roots"][0]["ejected"] is True + assert out["roots"][0]["available"] is False + + out = await ops.plug_root(state, "g" * 32, "USB") + assert out["status"] == "plugged" + assert await roster.ejected_roots("g" * 32) == set() + finally: + await roster.close() - out = await ops.set_member_upload(state, "g" * 32, True) - assert out["allowed"] is True - assert state["groups_ctx"]["g" * 32]["member_upload"] is True +async def test_a_root_that_is_not_removable_cannot_be_ejected(tmp_path): + """ + Eject means "I am about to unplug this". On a directory that is not on a + removable device it would hide a library with no way for the safety net to + notice anything happened, and nothing to plug back in. + """ + from meshbay_node.roster import Roster + state = _state(tmp_path) + fixed = tmp_path / "Fixed" + fixed.mkdir() + state["groups_ctx"]["g" * 32]["roots"] = RootSet.build( + [{"path": str(fixed), "writable": True}]) + state["config"] = SimpleNamespace( + groups=[SimpleNamespace(id="g" * 32, roots=[])]) + roster = Roster(db_path=tmp_path / "roster.db") + await roster.open() + state["roster"] = roster + try: + with pytest.raises(ops.OpError, match="removable"): + await ops.eject_root(state, "g" * 32, "Fixed") + finally: + await roster.close() - out2 = await ops.set_member_upload(state, "g" * 32, False) - assert out2["allowed"] is False - assert state["groups_ctx"]["g" * 32]["member_upload"] is False +async def test_plugging_a_drive_that_is_not_there_is_refused(tmp_path): + """ + Clearing the flag while the device is still absent would restart the + watchdog on a missing path and hand the next reconcile an empty directory — + the deletion storm the eject was there to prevent, produced by the recovery. + """ + from meshbay_node.roster import Roster + state = _state(tmp_path) + usb = tmp_path / "USB" + usb.mkdir() + roots = RootSet.build([{"path": str(usb), "removable": True}]) + roots.roots[0].ejected = True + roots.roots[0].available = False + state["groups_ctx"]["g" * 32]["roots"] = roots + state["config"] = SimpleNamespace( + groups=[SimpleNamespace(id="g" * 32, roots=[])]) + roster = Roster(db_path=tmp_path / "roster.db") + await roster.open() + state["roster"] = roster + usb.rmdir() + try: + with pytest.raises(ops.OpError, match="device connected"): + await ops.plug_root(state, "g" * 32, "USB") + assert roots.roots[0].ejected is True + finally: + await roster.close() # ── Reload ────────────────────────────────────────────────────────────────── diff --git a/packages/meshbay-node/tests/test_root_availability.py b/packages/meshbay-node/tests/test_root_availability.py index 0201c1f..d514dee 100644 --- a/packages/meshbay-node/tests/test_root_availability.py +++ b/packages/meshbay-node/tests/test_root_availability.py @@ -26,9 +26,12 @@ from meshbay_node.roots import RootSet pytestmark = pytest.mark.asyncio -def _roots(*paths: Path) -> RootSet: +def _roots(*paths: Path, removable: bool = False) -> RootSet: specs = [{"path": str(p)} for p in paths] - specs[0]["upload"] = True + specs[0]["writable"] = True + if removable: + for spec in specs: + spec["removable"] = True return RootSet.build(specs) @@ -117,7 +120,9 @@ async def test_members_are_told_which_roots_are_unavailable(tmp_path): idx = await _indexer(_roots(films)) assert idx.index.roots == [ - {"name": "Films", "kind": "generic", "available": True, "upload": True}] + {"name": "Films", "kind": "generic", "available": True, + "writable": True, "removable": False, "ejected": False, + "upload": True}] (films / "a.mkv").unlink() films.rmdir() diff --git a/packages/meshbay-node/tests/test_root_eject.py b/packages/meshbay-node/tests/test_root_eject.py new file mode 100644 index 0000000..0ec36a4 --- /dev/null +++ b/packages/meshbay-node/tests/test_root_eject.py @@ -0,0 +1,268 @@ +""" +Safe eject, and the surprise unplug it exists to survive. + +`test_root_availability.py` pins the freeze: a root that goes away keeps its +entries. This pins the half the operator drives — telling the node the drive is +about to leave, and telling it the drive is back. + +The distinction that makes any of this work is that `ejected` and `is_live()` +are separate answers. Between clicking Eject and physically unplugging, the +directory is still readable; a design that recomputed availability from the +filesystem alone would flip the root straight back to available and start +serving files from a disk somebody has their hand on. + +The other property here is that the flag is *persisted*. It reached the roster +in the first implementation and was never read back, so a restart — which is +exactly what an operator does after noticing a drive fell off — silently undid +the eject, and the next scan read an empty mount point as an erased library. +""" + +import asyncio +from pathlib import Path + +import pytest +from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey + +from meshbay_node.indexer.indexer import DirectoryIndexer +from meshbay_node.roots import RootSet +from meshbay_node.roster import Roster + +pytestmark = pytest.mark.asyncio + + +def _roots(*paths: Path, removable: bool = True) -> RootSet: + return RootSet.build([ + {"path": str(p), "removable": removable} for p in paths]) + + +async def _indexer(roots: RootSet, **kw) -> DirectoryIndexer: + idx = DirectoryIndexer(roots=roots, group_id="g" * 32, + sk_node=Ed25519PrivateKey.generate(), gek=None, **kw) + await idx.initial_scan() + return idx + + +def _names(idx: DirectoryIndexer) -> set[str]: + return {e.name for e in idx.index.entries} + + +# ── The two states are not the same question ───────────────────────────────── + +async def test_ejecting_hides_a_root_that_is_still_readable(tmp_path): + """ + The whole point of an eject button: the operator says the drive is leaving + *before* it leaves. The directory is still there and still readable at this + moment, so anything deriving availability from the filesystem would refuse + to believe it. + """ + films = tmp_path / "Films" + films.mkdir() + (films / "a.mkv").write_bytes(b"a") + + roots = _roots(films) + idx = await _indexer(roots) + idx.eject_root("Films") + + assert films.is_dir(), "the drive has not been unplugged yet" + assert roots.roots[0].is_live() is True + assert roots.roots[0].available is False + assert idx.index.roots[0]["ejected"] is True + assert idx.index.roots[0]["available"] is False + + +async def test_an_eject_freezes_entries_rather_than_dropping_them(tmp_path): + films = tmp_path / "Films" + films.mkdir() + (films / "a.mkv").write_bytes(b"a") + (films / "b.mkv").write_bytes(b"b") + + idx = await _indexer(_roots(films)) + idx.eject_root("Films") + + assert _names(idx) == {"a.mkv", "b.mkv"}, "eject deleted entries" + + +async def test_reconciling_does_not_un_eject_a_root(tmp_path): + """ + The backstop runs every minute regardless. An ejected root whose directory + is still readable must stay ejected, or the operator's eject lasts until + the next tick. + """ + films = tmp_path / "Films" + films.mkdir() + (films / "a.mkv").write_bytes(b"a") + + roots = _roots(films) + idx = await _indexer(roots) + idx.eject_root("Films") + await idx.reconcile() + + assert roots.roots[0].ejected is True + assert roots.roots[0].available is False + + +async def test_plugging_back_relists_the_files(tmp_path): + films = tmp_path / "Films" + films.mkdir() + (films / "a.mkv").write_bytes(b"a") + + roots = _roots(films) + idx = await _indexer(roots) + idx.eject_root("Films") + await idx.plug_root("Films") + + assert roots.roots[0].ejected is False + assert roots.roots[0].available is True + assert _names(idx) == {"a.mkv"} + + +async def test_what_changed_while_unplugged_is_picked_up_on_plug(tmp_path): + """ + A drive people take away comes back different. The plug pass has to see + that, or the index describes a library that no longer exists on the disk + the node is about to serve from. + """ + films = tmp_path / "Films" + films.mkdir() + (films / "a.mkv").write_bytes(b"a") + + roots = _roots(films) + idx = await _indexer(roots) + idx.eject_root("Films") + + (films / "a.mkv").unlink() + (films / "c.mkv").write_bytes(b"c") + + await idx.plug_root("Films") + assert _names(idx) == {"c.mkv"} + + +# ── The surprise unplug ────────────────────────────────────────────────────── + +async def test_a_removable_root_that_vanishes_is_auto_ejected(tmp_path): + """ + Nobody clicks Eject when they are in a hurry. A removable root whose path + disappears is treated as ejected rather than merely unavailable, so it does + not silently come back the moment the same mount point is readable again — + which on a machine with automount is any other drive, or an empty stub. + """ + films = tmp_path / "Films" + films.mkdir() + (films / "a.mkv").write_bytes(b"a") + + roots = _roots(films) + idx = await _indexer(roots) + + (films / "a.mkv").unlink() + films.rmdir() + await idx.reconcile() + + assert roots.roots[0].ejected is True + assert _names(idx) == {"a.mkv"}, "the library was treated as erased" + + +async def test_a_non_removable_root_is_not_auto_ejected(tmp_path): + """ + The counter-property. Auto-eject requires the operator to have said the + device is removable; an ordinary directory that briefly fails to stat must + keep the old behaviour and come back on its own. + """ + films = tmp_path / "Films" + films.mkdir() + (films / "a.mkv").write_bytes(b"a") + + roots = _roots(films, removable=False) + idx = await _indexer(roots) + + (films / "a.mkv").unlink() + films.rmdir() + await idx.reconcile() + assert roots.roots[0].ejected is False + assert roots.roots[0].available is False + + films.mkdir() + (films / "a.mkv").write_bytes(b"a") + await idx.reconcile() + assert roots.roots[0].available is True + + +async def test_an_auto_eject_is_reported_so_it_can_be_persisted(tmp_path): + """ + The flag has to outlive the process. The first version of this set it in + memory only, so restarting the node — which is what an operator does after + noticing a drive fell off — cleared it, and the scan that followed read the + empty mount point as a deletion of the whole library. + """ + films = tmp_path / "Films" + films.mkdir() + (films / "a.mkv").write_bytes(b"a") + + seen: list[tuple[str, bool]] = [] + + async def record(name: str, ejected: bool) -> None: + seen.append((name, ejected)) + + roots = _roots(films) + idx = await _indexer(roots, on_root_ejected=record) + + (films / "a.mkv").unlink() + films.rmdir() + await idx.reconcile() + + assert seen == [("Films", True)] + + # And only once, however many times the backstop runs afterwards. + await idx.reconcile() + await idx.reconcile() + assert seen == [("Films", True)] + + +# ── Restoring the flag ─────────────────────────────────────────────────────── + +async def test_a_root_built_as_ejected_starts_unavailable(tmp_path): + """ + What the daemon does with what the roster remembers. `available` must not + be left at its default `True` here, or the group serves a drive that is not + there for as long as it takes the first reconcile to run. + """ + films = tmp_path / "Films" + films.mkdir() + roots = RootSet.build([{"path": str(films), "removable": True, + "ejected": True}]) + assert roots.roots[0].ejected is True + assert roots.roots[0].available is False + + +async def test_the_roster_round_trips_the_ejected_set(tmp_path): + roster = Roster(db_path=tmp_path / "roster.db") + await roster.open() + try: + assert await roster.ejected_roots("g1") == set() + + await roster.set_root_ejected("g1", "Films", True, set_by="op") + await roster.set_root_ejected("g1", "Music", False, set_by="op") + assert await roster.ejected_roots("g1") == {"films"} + + # Another group's drives are its own. + assert await roster.ejected_roots("g2") == set() + + await roster.set_root_ejected("g1", "Films", False, set_by="op") + assert await roster.ejected_roots("g1") == set() + finally: + await roster.close() + + +async def test_the_ejected_key_is_case_folded(tmp_path): + """ + Root names are compared without regard to case everywhere else, and a key + that did not fold would let `Films` and `films` disagree about the same + drive — on Windows and macOS, the same directory. + """ + roster = Roster(db_path=tmp_path / "roster.db") + await roster.open() + try: + await roster.set_root_ejected("g1", "FILMS", True, set_by="op") + assert await roster.ejected_roots("g1") == {"films"} + assert Roster.root_ejected_key("Films") == Roster.root_ejected_key("FILMS") + finally: + await roster.close() diff --git a/packages/meshbay-node/tests/test_root_writable_policy.py b/packages/meshbay-node/tests/test_root_writable_policy.py new file mode 100644 index 0000000..da95032 --- /dev/null +++ b/packages/meshbay-node/tests/test_root_writable_policy.py @@ -0,0 +1,203 @@ +""" +Who may write to the operator's disk, now that RO/RW on the root decides it. + +This replaces `test_member_upload_policy.py`. The old model had two orthogonal +controls — one root designated as the upload target, and a group-wide +`member_upload` switch — and collapsed into one property per root: `writable`. +The properties worth keeping from the old file survive the change unaltered: + +* the interface hiding a control is a courtesy to the people who are not + trying; **the node refusing is the part that holds** against someone who is. + A member with an old tab open, or one speaking MNP directly, gets the same + answer. That half is pinned in `test_security_regressions.py`, next to the + overwrite properties it belongs with; +* the setting is changed by a **signed** operator instruction, or it is a + suggestion any member can undo; +* it is stored on the **node**, never the hub. A hub that could decide who + writes to the operator's disk would have authority over the node. + +And one that is new: the *old* message must no longer be able to change +anything. A deprecated instruction that still works is not deprecated, and this +one would reopen uploads group-wide. +""" + +import base64 +from pathlib import Path + +import pytest +from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey + +from meshbay_common.adminop import OP_ROOT_UPDATE, OP_ROOT_EJECT, OP_ROOT_PLUG +from meshbay_common.protocol import MNP +from meshbay_node.indexer.group_index import GroupIndex +from meshbay_node.roots import RootSet +from meshbay_node.transport.webrtc_server import WebRTCPeerSession + +pytestmark = pytest.mark.asyncio + + +def _session(tmp_path: Path, user_id: str, *, + writable: bool = True, + operator: str | None = None) -> WebRTCPeerSession: + shared_root = tmp_path / "shared" + shared_root.mkdir(exist_ok=True) + index = GroupIndex(group_id="g" * 32, sk_node=Ed25519PrivateKey.generate()) + ctx = { + "roots": RootSet.build([{"path": str(shared_root), "writable": writable}]), + "index": index, + "sk_node": index.sk_node, + "node_user_id": operator, + } + session = WebRTCPeerSession.__new__(WebRTCPeerSession) + session._ctx = ctx + session._group_id = "g" * 32 + session._user_id = user_id + session._pk_user = "" + session._uploads = {} + session.sent = [] + session._send = session.sent.append + session._audit = lambda *a, **k: None + return session + + +def _upload(session, filename="clip.mp4", body=b"bytes"): + session._do_file_upload({ + "filename": filename, "root": "shared", + "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(body).decode(), + }) + + +def _uploads_dir(session) -> Path: + return session._ctx["roots"].roots[0].path / "uploads" + + +# ── The door, not the button ───────────────────────────────────────────────── + +async def test_a_member_cannot_upload_to_a_read_only_root(tmp_path): + session = _session(tmp_path, "member-1", writable=False) + _upload(session) + + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "root_read_only" + assert not _uploads_dir(session).exists() + + +async def test_members_upload_normally_to_a_writable_root(tmp_path): + session = _session(tmp_path, "member-1", writable=True) + _upload(session) + + assert not [m for m in session.sent if m.get("type") == "error"] + assert (_uploads_dir(session) / "clip.mp4").read_bytes() == b"bytes" + + +async def test_read_only_binds_the_operator_too(tmp_path): + """ + The old model exempted the operator, because the switch was about *members*. + RO is about the directory: a published library is read-only for everyone, and + an exception for admin authority is how a rule turns into a default. + """ + session = _session(tmp_path, "the-operator", writable=False, + operator="the-operator") + session._is_node_admin = lambda: True + _upload(session) + + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "root_read_only" + + +# ── Signed, or it is a suggestion ──────────────────────────────────────────── + +def _capture_challenges(session) -> list[tuple[str, str]]: + issued: list[tuple[str, str]] = [] + + def issue(op, subject, **kw): + issued.append((op, subject)) + + session._issue_admin_challenge = issue + session._has_admin_authority = lambda: True + return issued + + +async def test_changing_a_roots_flags_needs_a_signature(tmp_path): + """The flags are not applied by the request — only by the signed response.""" + session = _session(tmp_path, "the-operator", operator="the-operator") + issued = _capture_challenges(session) + + session._do_root_update({"group_id": "g" * 32, "root_name": "shared", + "writable": False}) + + assert [op for op, _ in issued] == [OP_ROOT_UPDATE] + assert session._ctx["roots"].roots[0].writable is True, ( + "applied before it was signed") + + +async def test_the_subject_names_the_outcome_not_the_operation(tmp_path): + """ + The operator is shown the subject before signing, so it has to say what will + be true afterwards. "shared" alone would have them authorize a change they + cannot see the direction of. + """ + session = _session(tmp_path, "op", operator="op") + issued = _capture_challenges(session) + + session._do_root_update({"group_id": "g" * 32, "root_name": "shared", + "writable": True, "removable": True}) + + assert issued == [(OP_ROOT_UPDATE, "shared:rw=on,rem=on")] + + +async def test_eject_and_plug_are_signed_too(tmp_path): + """ + Hiding a group's whole library from every member is not a lesser act than + changing a flag. An unsigned one would let any member black out a group. + """ + session = _session(tmp_path, "op", operator="op") + issued = _capture_challenges(session) + + session._do_root_eject({"group_id": "g" * 32, "root_name": "shared"}) + session._do_root_plug({"group_id": "g" * 32, "root_name": "shared"}) + + assert issued == [(OP_ROOT_EJECT, "shared"), (OP_ROOT_PLUG, "shared")] + + +async def test_a_request_with_nobody_to_authorize_it_is_refused(tmp_path): + """ + An unpaired node has no key to check a signature against, so the challenge + is never issued rather than issued and then unverifiable. + """ + session = _session(tmp_path, "member-1") + issued = _capture_challenges(session) + session._has_admin_authority = lambda: False + + session._do_root_update({"group_id": "g" * 32, "root_name": "shared", + "writable": True}) + + assert issued == [] + assert [m for m in session.sent if m.get("type") == "error"] + + +# ── The deprecated message must not still work ─────────────────────────────── + +async def test_the_old_member_upload_message_changes_nothing(tmp_path): + """ + MNP still parses `member_upload` so an old client gets an answer instead of + a dropped request. What it must not do is act: this instruction could + reopen uploads for a whole group, and a client old enough to send it is + exactly one that knows nothing about read-only roots. + """ + session = _session(tmp_path, "member-1", writable=False) + session._has_admin_authority = lambda: True + issued = _capture_challenges(session) + + session._do_member_upload({"allowed": True}) + + assert issued == [], "a deprecated instruction asked to be signed" + assert session._ctx["roots"].roots[0].writable is False + acks = [m for m in session.sent if m.get("type") == MNP.MEMBER_UPLOAD_ACK] + assert acks and acks[0].get("deprecated") is True + + # And the door is still shut. + _upload(session) + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "root_read_only" diff --git a/packages/meshbay-node/tests/test_roots.py b/packages/meshbay-node/tests/test_roots.py index 1beb220..505091b 100644 --- a/packages/meshbay-node/tests/test_roots.py +++ b/packages/meshbay-node/tests/test_roots.py @@ -108,31 +108,61 @@ def test_a_sibling_with_a_shared_prefix_is_fine(tmp_path): assert roots.names == ["Media", "Media2"] -# ── Uploads ────────────────────────────────────────────────────────────────── +# ── Writable roots ─────────────────────────────────────────────────────────── -def test_a_single_root_receives_uploads_without_being_asked(tmp_path): +def test_a_root_is_read_only_unless_it_says_otherwise(tmp_path): + """ + The default is the safe one. An operator who shares a directory has not + thereby agreed to let anyone write into it, and the version of this that + guessed — one root, so it must be the upload target — meant adding a + second directory silently changed what the first one was. + """ (tmp_path / "Media").mkdir() roots = RootSet.build([_spec(tmp_path / "Media")]) - assert roots.upload_root is roots.roots[0] + assert roots.roots[0].writable is False + assert roots.writable_roots == [] + + +def test_several_roots_can_be_writable_at_once(tmp_path): + (tmp_path / "A").mkdir() + (tmp_path / "B").mkdir() + (tmp_path / "C").mkdir() + roots = RootSet.build([_spec(tmp_path / "A", writable=True), + _spec(tmp_path / "B"), + _spec(tmp_path / "C", writable=True)]) + assert [r.name for r in roots.writable_roots] == ["A", "C"] -def test_several_roots_and_no_designation_means_no_uploads(tmp_path): +def test_a_fully_read_only_group_is_valid(tmp_path): """ - Refused, never guessed: picking one would send a member's file to a disk the - operator did not intend, and that is discovered weeks later. + A group that only publishes is the point of the read-only model, not a + misconfiguration — build must not refuse it, and nothing downstream may + promote a root to writable to have somewhere to put an upload. """ (tmp_path / "A").mkdir() (tmp_path / "B").mkdir() roots = RootSet.build([_spec(tmp_path / "A"), _spec(tmp_path / "B")]) - assert roots.upload_root is None + assert roots.writable_roots == [] + assert len(roots) == 2 -def test_two_upload_roots_are_refused(tmp_path): - (tmp_path / "A").mkdir() - (tmp_path / "B").mkdir() - with pytest.raises(RootError, match="exactly one"): - RootSet.build([_spec(tmp_path / "A", upload=True), - _spec(tmp_path / "B", upload=True)]) +def test_the_old_upload_flag_still_reads_as_writable(tmp_path): + """A node.toml written before this refactor must not change meaning.""" + (tmp_path / "Media").mkdir() + roots = RootSet.build([_spec(tmp_path / "Media", upload=True)]) + assert roots.roots[0].writable is True + assert roots.describe()[0]["writable"] is True + + +def test_writable_wins_over_a_leftover_upload_flag(tmp_path): + """ + A config carrying both is one a migration touched. `writable` is the field + the operator's tooling writes now, so it is the one that decides — reading + the legacy field there would undo the migration on the next load. + """ + (tmp_path / "Media").mkdir() + roots = RootSet.build([_spec(tmp_path / "Media", upload=True, writable=False)]) + assert roots.roots[0].writable is False # ── Resolution ─────────────────────────────────────────────────────────────── @@ -236,18 +266,35 @@ def test_availability_follows_the_directory(tmp_path): def test_describe_reports_what_a_member_needs(tmp_path): (tmp_path / "Media").mkdir() (tmp_path / "Music").mkdir() - roots = RootSet.build([_spec(tmp_path / "Media", upload=True), - _spec(tmp_path / "Music", kind="audio")]) + roots = RootSet.build([_spec(tmp_path / "Media", writable=True), + _spec(tmp_path / "Music", kind="audio", + removable=True)]) described = roots.describe() assert described == [ - {"name": "Media", "kind": "generic", "available": True, "upload": True}, - {"name": "Music", "kind": "audio", "available": True, "upload": False}, + {"name": "Media", "kind": "generic", "available": True, + "writable": True, "removable": False, "ejected": False, + "upload": True}, + {"name": "Music", "kind": "audio", "available": True, + "writable": False, "removable": True, "ejected": False, + "upload": False}, ] # Deliberately no paths: a member is told what exists and whether it is # readable, not where on the operator's disk it lives. assert not any("path" in d for d in described) +def test_describe_still_carries_upload_for_mnp_1_0_clients(tmp_path): + """ + `upload` is `writable` under its old name, kept because an MNP 1.0 client + reads no other field and would otherwise decide the group takes no uploads + at all. It is derived, never stored — the two can never disagree. + """ + (tmp_path / "Media").mkdir() + roots = RootSet.build([_spec(tmp_path / "Media", writable=True)]) + described = roots.describe()[0] + assert described["upload"] == described["writable"] is True + + # ── SAFE_UPLOAD_NAME ──────────────────────────────────────────────────────── def test_safe_name_accepts_unicode_letters(): diff --git a/packages/meshbay-node/tests/test_scan_settings_policy.py b/packages/meshbay-node/tests/test_scan_settings_policy.py index 719b988..94f4421 100644 --- a/packages/meshbay-node/tests/test_scan_settings_policy.py +++ b/packages/meshbay-node/tests/test_scan_settings_policy.py @@ -2,7 +2,7 @@ The operator can tune how often the indexer's reconciliation backstop runs, and how long it waits after a file's last write before hashing it. -Same shape as test_apps_enabled_policy.py / test_member_upload_policy.py: +Same shape as test_apps_enabled_policy.py / test_root_writable_policy.py: changed by a signed operator instruction, stored on the node rather than the hub. Unlike those two, there is also a *live* DirectoryIndexer object to update — see test_set_scan_settings_updates_the_live_indexer below. diff --git a/packages/meshbay-node/tests/test_security_regressions.py b/packages/meshbay-node/tests/test_security_regressions.py index 7f71da5..9db8ac1 100644 --- a/packages/meshbay-node/tests/test_security_regressions.py +++ b/packages/meshbay-node/tests/test_security_regressions.py @@ -18,6 +18,7 @@ from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey from meshbay_common.protocol import IndexEntry from meshbay_node.indexer.group_index import GroupIndex +from meshbay_node.roots import RootSet from conftest import one_root from meshbay_node.transport.webrtc_server import WebRTCPeerSession @@ -132,14 +133,14 @@ def test_the_node_never_generates_a_name_it_would_refuse(tmp_path): def _uploads_dir(session) -> Path: """ - Where this session's uploads land: uploads/ inside the group's upload root. + Where this session's uploads land: uploads/ inside its first writable root. Asked of the root set rather than assembled by hand, so a test cannot pass while agreeing with a wrong answer the code also produced. """ - root = session._ctx["roots"].upload_root - assert root is not None, "the fixture must designate an upload root" - return root.path / "uploads" + writable = session._ctx["roots"].writable_roots + assert writable, "the fixture must give the group a writable root" + return writable[0].path / "uploads" def _session(tmp_path: Path, user_id: str) -> WebRTCPeerSession: @@ -227,14 +228,17 @@ def test_dir_create_cannot_escape_the_shared_root(tmp_path, bad): def test_upload_ignores_any_directory_the_client_asks_for(tmp_path): """ - Uploads land in uploads/, chosen by the node. A client that names somewhere - else — or nowhere at all — changes nothing, so the traversal surface that a - client-chosen destination would open does not exist on this path. + The destination inside a root is the node's decision, and stays so. + + A client now names the *root* it is uploading into — it has to, once a group + can have several writable ones — but that is a name looked up in the root + table, never a path. Everything below the root is still chosen here, so the + traversal surface a client-chosen destination would open does not exist. """ session = _session(tmp_path, "user-1") session._do_file_upload({ - "filename": "note.txt", "dir": "../../etc", + "filename": "note.txt", "dir": "../../etc", "path": "/etc", "chunk_index": 0, "total_chunks": 1, "data": base64.b64encode(b"x").decode(), }) @@ -243,6 +247,131 @@ def test_upload_ignores_any_directory_the_client_asks_for(tmp_path): assert not (tmp_path / "etc").exists() +@pytest.mark.parametrize("named_root", [ + "../../etc", "/etc", "shared/../..", "Shared/uploads", "nope", +]) +def test_a_root_name_is_looked_up_never_joined(tmp_path, named_root): + """ + The name the client sends is matched against the group's root table and + refused when it matches nothing. A version that joined it to a path — or + that quietly fell back to the first writable root — would turn "which + directory" into either a traversal or a file on a disk the operator did + not intend, and the second is discovered weeks later. + """ + session = _session(tmp_path, "user-1") + before = set(tmp_path.rglob("*")) + + session._do_file_upload({ + "filename": "note.txt", "root": named_root, + "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(b"x").decode(), + }) + + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "no_such_root", named_root + assert set(tmp_path.rglob("*")) == before, f"wrote something via {named_root!r}" + + +def test_an_upload_goes_to_the_root_it_names(tmp_path): + """ + With two writable roots there is no defensible default, and the client is + the only party that knows which directory the person is looking at. The + node picking one meant a file uploaded from a folder on screen landed in a + different one — the same "uploads went somewhere else" the single upload + root was never allowed to guess about. + """ + media = tmp_path / "Media" + incoming = tmp_path / "Incoming" + media.mkdir() + incoming.mkdir() + session = _session(tmp_path, "user-1") + session._ctx["roots"] = RootSet.build([ + {"path": str(media), "writable": True}, + {"path": str(incoming), "writable": True}, + ]) + + session._do_file_upload({ + "filename": "note.txt", "root": "Incoming", + "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(b"x").decode(), + }) + + assert (incoming / "uploads" / "note.txt").read_bytes() == b"x" + assert not (media / "uploads").exists(), "it went to the first root instead" + + +def test_a_read_only_root_refuses_an_upload(tmp_path): + """ + RO is the mechanism now, not a hidden button. It binds the operator too: + "read-only for everyone" is what makes a published library one, and an + exception for whoever happens to hold admin authority is the sort of + carve-out that later reads as the rule. + """ + published = tmp_path / "Published" + published.mkdir() + session = _session(tmp_path, "user-1") + session._ctx["roots"] = RootSet.build([{"path": str(published)}]) + session._is_node_admin = lambda: True + + session._do_file_upload({ + "filename": "note.txt", "root": "Published", + "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(b"x").decode(), + }) + + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "root_read_only" + assert not (published / "uploads").exists() + + +def test_a_fully_read_only_group_refuses_an_unaddressed_upload(tmp_path): + """ + An MNP 1.0 client names no root, so the node falls back to the first + writable one. There isn't one here, and the fallback must refuse rather + than write into whatever root happens to come first. + """ + published = tmp_path / "Published" + published.mkdir() + session = _session(tmp_path, "user-1") + session._ctx["roots"] = RootSet.build([{"path": str(published)}]) + + session._do_file_upload({ + "filename": "note.txt", + "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(b"x").decode(), + }) + + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "no_writable_root" + assert not (published / "uploads").exists() + + +def test_an_ejected_root_refuses_an_upload(tmp_path): + """ + Writing to a drive somebody has their hand on is the thing eject exists to + stop. `writable` is still true — that is configuration — so availability + has to be checked separately, which is what an earlier version conflated. + """ + usb = tmp_path / "USB" + usb.mkdir() + session = _session(tmp_path, "user-1") + roots = RootSet.build([{"path": str(usb), "writable": True, + "removable": True}]) + roots.roots[0].ejected = True + roots.roots[0].available = False + session._ctx["roots"] = roots + + session._do_file_upload({ + "filename": "note.txt", "root": "USB", + "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(b"x").decode(), + }) + + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "root_unavailable" + assert not (usb / "uploads").exists() + + def test_two_members_can_send_the_same_filename(tmp_path): """ One shared uploads/ means collisions are ordinary — every camera produces |