diff --git a/docs/OPERATIONS.md b/docs/OPERATIONS.md index 2dc3a6f..b6d6c2c 100644 --- a/docs/OPERATIONS.md +++ b/docs/OPERATIONS.md @@ -132,7 +132,16 @@ and `secrets/`; without `--purge-data` it explicitly says both were kept. It the asks about the **system account** separately — `--remove-account` / `--keep-account` are the non-interactive answers, and keeping is the default, because chief's home holds its own message store and the dedicated Apple ID's -whole conversation lives there. +whole conversation lives there. The question is only asked when +`data/account-setup` records the account this install set chief up with — +created *or* adopted (`mode=existing`) — and that account still resolves to a +real passwd entry: a single-user install is never offered the deletion, and +`--remove-account` there is a no-op rather than a `userdel` aimed at whatever +pre-existing account happens to be named `chief`. An adopted account is +removable the same way a created one is, so on an `existing` install +`--remove-account` deletes a user that predates chief, home and all. The report +survives re-runs that skip or decline the account offer; only a fresh +create/adopt answer replaces it. ## The service diff --git a/docs/SECURITY.md b/docs/SECURITY.md index d9abc3d..b3d07be 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -312,8 +312,13 @@ What the boundary buys: just closed (`install/account.py`). - **The owner picks what chief may reach.** The wizard asks two questions — directories to read, directories to write — defaulting to **none**, applied - as group permissions. The home directory root is never offered - (`grant_reason`). + as group permissions. Only directories strictly *inside* the owner's home + can be granted — judged on the resolved path, so neither `~/..` nor a + symlink out smuggles anything in, and the home root itself, `/`, and every + system root are refused along with it (`chmod -R g+rwX /etc` would hand + chief group-write on `sudoers`, which is the escalation the bullet above + rules out). A directory that does not exist is refused too, rather than left + to fail its own step mid-plan (`install/dedicated_ask.py`, `grant_reason`). ### What the boundary does not cover diff --git a/install.sh b/install.sh index d58a745..99dba40 100755 --- a/install.sh +++ b/install.sh @@ -93,20 +93,34 @@ fi CHIEF_USER="" CHIEF_HOME="" ACCOUNT_REPORT="$REPO_DIR/data/account-setup" -rm -f "$ACCOUNT_REPORT" if [ "$SINGLE_USER" = 1 ]; then say "skipping the dedicated-account offer (--single-user)" elif [ "$NON_INTERACTIVE" = 1 ] || [ ! -r /dev/tty ]; then say "dedicated account: not offered (no terminal) — chief runs as you" else say "dedicated system account" + # Written aside and moved over the real report only on an account answer. + # Re-runs are idempotent and the offer can be skipped (--single-user, no + # tty) or declined, and none of those mean the account an earlier run set up + # has gone away — but `chief uninstall` reads this file to decide whether + # there is an account to remove at all, so erasing it strands one. + # With a template, not bare: BSD mktemp (macOS — the platform this targets) + # requires one, and this line is only reached on an interactive run, which + # no CI job makes. + FRESH_REPORT=$(mktemp "${TMPDIR:-/tmp}/chief-account.XXXXXX") uv run python -m chief.install account \ - --tree "$REPO_DIR" --report "$ACCOUNT_REPORT" < /dev/tty + --tree "$REPO_DIR" --report "$FRESH_REPORT" < /dev/tty # -E, not BRE alternation: BSD grep (macOS — the platform this targets) does # not understand \(a\|b\), and a silent no-match installs the wrong mode. - if grep -qE '^mode=(create|existing)$' "$ACCOUNT_REPORT" 2>/dev/null; then + if grep -qE '^mode=(create|existing)$' "$FRESH_REPORT" 2>/dev/null; then + mv "$FRESH_REPORT" "$ACCOUNT_REPORT" + # mktemp makes it 0600 owner-only, and the tree chown already ran, so + # without this chief cannot read its own install record. + chmod 0644 "$ACCOUNT_REPORT" CHIEF_USER=$(sed -n 's/^user=//p' "$ACCOUNT_REPORT") CHIEF_HOME=$(sed -n 's/^home=//p' "$ACCOUNT_REPORT") + else + rm -f "$FRESH_REPORT" fi fi diff --git a/src/chief/adapters/imessage.py b/src/chief/adapters/imessage.py index 511b2e9..3e0cb2a 100644 --- a/src/chief/adapters/imessage.py +++ b/src/chief/adapters/imessage.py @@ -70,6 +70,7 @@ def __init__( # or it deadlocks that thread until the card times out. self._resolve_approval = resolve_approval self._dedup = RecentDedup() + self._guids = RecentDedup() self._task: asyncio.Task[None] | None = None self._fifo = ThreadFifo(on_message, restart) self._stores = [Store(db_path, RowCursor(cursor_path))] @@ -132,7 +133,9 @@ async def poll_once(self) -> None: scope = frozenset() if self._dedicated else self._owner_handles for store in self._stores: rows = await asyncio.to_thread(store.fetch, scope) - for rowid, sender, text, from_me, group_chat, in_self, date in rows: + for rowid, sender, text, from_me, group_chat, in_self, date, guid in ( + rows + ): store.advance(rowid) if not store.mine and sender in self._self_handles: # Chief's own reply, seen from the owner's side as an @@ -141,6 +144,13 @@ async def poll_once(self) -> None: message = self._map(sender, text, from_me, group_chat, in_self) if message is None: continue + # A group chat both accounts are in holds every message twice, + # once per store, under different rowids. Same message, so + # same guid — which the text key cannot tell apart from two + # people typing "ok". Cheap in the single-store case: a guid + # is only ever read once, so nothing matches. + if guid and self._guids.is_duplicate((guid,), date): + continue if not self._dedicated and self._dedup.is_duplicate( (message.thread_key, message.sender, message.text), date ): diff --git a/src/chief/adapters/imessage_store.py b/src/chief/adapters/imessage_store.py index e836d60..76d9aa2 100644 --- a/src/chief/adapters/imessage_store.py +++ b/src/chief/adapters/imessage_store.py @@ -35,7 +35,8 @@ "MAX(CASE WHEN (chat.style IS NOT NULL AND chat.style != 45) " "OR chat.room_name IS NOT NULL THEN chat.chat_identifier END) AS group_chat, " "MAX(CASE WHEN self_chat.mid IS NOT NULL THEN 1 ELSE 0 END) AS in_self, " - "message.attributedBody AS body, message.date AS date " + "message.attributedBody AS body, message.date AS date, " + "message.guid AS guid " "FROM message JOIN handle ON message.handle_id = handle.ROWID " "LEFT JOIN chat_message_join ON chat_message_join.message_id = message.ROWID " "LEFT JOIN chat ON chat.ROWID = chat_message_join.chat_id " @@ -70,6 +71,13 @@ class RecentDedup: the same text (owner types it again minutes on) from being swallowed. State is in-memory: a fresh boot re-primes from the store cursor, never replaying an already-delivered twin. + + Also used, keyed on ``guid`` alone, for the other duplicate: one message + present in both stores. That one is exact rather than heuristic — the two + copies *are* the same message, so they carry the same guid, where self-DM + twins carry different ones. Read skew between the stores cannot defeat it + either: the window is measured on the row's own date, which is identical + in both. """ def __init__(self, window_ns: int = DEDUP_WINDOW_NS) -> None: @@ -121,7 +129,7 @@ def text_of(text: object, body: object) -> str: #: One polled row: (rowid, sender, text, from_me, group_chat, in_self, date). -PolledRow = tuple[int, str, str, int, str | None, int, int] +PolledRow = tuple[int, str, str, int, str | None, int, int, str] def fetch_rows( @@ -143,6 +151,7 @@ def fetch_rows( ( int(r[0]), str(r[1]), text_of(r[2], r[6]), int(r[3]), None if r[4] is None else str(r[4]), int(r[5]), int(r[7]), + str(r[8] or ""), ) for r in cur.fetchall() ] diff --git a/src/chief/install/account.py b/src/chief/install/account.py index 1315389..50321be 100644 --- a/src/chief/install/account.py +++ b/src/chief/install/account.py @@ -30,7 +30,6 @@ "Step", "account_plan", "default_home", - "grant_reason", "grant_steps", ] @@ -96,15 +95,6 @@ def _git_steps(tree: Path, user: str, email: str) -> tuple[Step, ...]: ) -def grant_reason(path: Path, home: Path) -> str | None: - """Why this directory may not be granted, or ``None`` if it may.""" - if not path.is_absolute(): - return f"{path} is not an absolute path" - if path in (home, Path("/")): - return f"{path} is a home or filesystem root — grant a subdirectory" - return None - - def grant_steps( *, group: str = DEFAULT_GROUP, diff --git a/src/chief/install/cli.py b/src/chief/install/cli.py index e6efde9..983f20d 100644 --- a/src/chief/install/cli.py +++ b/src/chief/install/cli.py @@ -8,6 +8,7 @@ from pathlib import Path from chief.install.lifecycle import DEFAULT_LAUNCHER +from chief.install.posture import ACCOUNT_REPORT def build_parser() -> argparse.ArgumentParser: @@ -24,7 +25,14 @@ def build_parser() -> argparse.ArgumentParser: account.add_argument("--non-interactive", action="store_true") account.add_argument("--tree", type=Path, default=None) account.add_argument( - "--report", type=Path, default=None, help="write key=value facts here" + # Defaulted, not None: `chief account` is the documented alternative to + # re-running install.sh (docs/OPERATIONS.md), and uninstall reads this + # file to decide there is an account at all. Written nowhere, the + # account it just made is one chief can never remove. + "--report", + type=Path, + default=ACCOUNT_REPORT, + help="write key=value facts here", ) service_install = sub.add_parser( "service-install", help="install + start the autostart service" diff --git a/src/chief/install/dedicated_ask.py b/src/chief/install/dedicated_ask.py index 42efc5a..90fb230 100644 --- a/src/chief/install/dedicated_ask.py +++ b/src/chief/install/dedicated_ask.py @@ -4,18 +4,44 @@ inspected and pinned without a terminal or a machine to change. """ +import re from dataclasses import dataclass, field from pathlib import Path -from chief.install.account import DEFAULT_USER, grant_reason +from chief.install.account import DEFAULT_USER from chief.install.session import password_conflict from chief.install.wizard_io import MIN_PASSWORD_LENGTH, WizardIO CREATE = "create" EXISTING = "existing" DECLINED = "declined" +ACCOUNT_NAME = re.compile(r"^[a-z_][a-z0-9_-]{0,31}$") -__all__ = ["CREATE", "DECLINED", "EXISTING", "Answers", "ask"] +__all__ = ["CREATE", "DECLINED", "EXISTING", "Answers", "ask", "grant_reason"] + + +def grant_reason(path: Path, home: Path) -> str | None: + """Why this directory may not be granted, or ``None`` if it may. + + The question asked is which of *your* directories chief may reach, and + strictly-inside-your-home is that question's own answer — which is also + what makes it the whole check. It refuses the home root and ``/``, ``~/..`` + and any other route out (judged on the *resolved* path, since the grant is + a recursive, irreversible ``chgrp``), and every system root: ``chmod -R + g+rwX /etc`` hands chief group-write on ``sudoers`` and is root by another + name, which the no-escalation promise in docs/SECURITY.md rules out. + + A path that does not exist is refused here rather than left to fail its own + step, which aborts the plan after the account and tree steps have landed. + """ + if not path.is_absolute(): + return f"{path} is not an absolute path" + target, root = path.resolve(), home.resolve() + if target == root or not target.is_relative_to(root): + return f"{path} is not inside {root} — grant a directory of your own" + if not target.is_dir(): + return f"{path} is not an existing directory" + return None @dataclass(frozen=True) @@ -53,6 +79,19 @@ def _password(io: WizardIO, user: str) -> str: return password +def _name(io: WizardIO) -> str: + """The name reaches argv unquoted and is joined onto the home root, where + an *absolute* one swallows the join whole: `Path("/Users") / "/etc"` is + `/etc`, i.e. `sysadminctl -addUser … -home /etc`.""" + while True: + user = ( + io.prompt(f" account name [{DEFAULT_USER}]: ").strip() or DEFAULT_USER + ) + if ACCOUNT_NAME.match(user): + return user + io.say(" lowercase letters, digits, _ and - only — try again.") + + def _dirs(io: WizardIO, verb: str, home: Path) -> tuple[Path, ...]: raw = io.prompt( f" directories chief may {verb}, space-separated (empty = none): " @@ -81,9 +120,7 @@ def ask(io: WizardIO, *, home: Path) -> Answers: if answer in ("n", "no"): io.say("account: declined — chief runs as you, exactly as before.") return Answers(DECLINED) - user = ( - io.prompt(f" account name [{DEFAULT_USER}]: ").strip() or DEFAULT_USER - ) + user = _name(io) choice = EXISTING if answer in ("e", "existing") else CREATE password = _password(io, user) if choice == CREATE else "" io.say("Which of your directories may chief reach? Default is none.") diff --git a/src/chief/install/lifecycle.py b/src/chief/install/lifecycle.py index bbc1b5a..a305e4c 100644 --- a/src/chief/install/lifecycle.py +++ b/src/chief/install/lifecycle.py @@ -12,9 +12,10 @@ import httpx from chief.config import load_config -from chief.install.account import DEFAULT_GROUP, DEFAULT_USER +from chief.install.account import DEFAULT_GROUP from chief.install.account_steps import remove_steps from chief.install.dedicated import StepRunner, default_step_runner +from chief.install.posture import ACCOUNT_REPORT, chief_account from chief.install.service import ServiceManager DEFAULT_LAUNCHER = Path.home() / ".local" / "bin" / "chief" @@ -52,7 +53,6 @@ def uninstall( assume_yes: bool, remove_account: bool = False, keep_account: bool = False, - user: str = DEFAULT_USER, group: str = DEFAULT_GROUP, confirm: Callable[[str], str] = input, say: Callable[[str], None] = print, @@ -62,7 +62,8 @@ def uninstall( The dedicated system account is only removed when asked for — its home holds chief's own message store. ``--remove-account`` / ``--keep-account`` - are the non-interactive answers; without either, uninstall asks. + are the non-interactive answers; without either, uninstall asks. A + single-user install has no such account, and is never asked. """ if purge_data and not assume_yes: answer = confirm( @@ -71,6 +72,10 @@ def uninstall( if answer.strip().lower() not in ("y", "yes"): say("aborted — nothing removed.") return 1 + # Read before the purge: --purge-data deletes data/, which is where the + # report lives, and a run told to remove the account would then find no + # record of one and report that it never existed. + account = chief_account(repo_dir / ACCOUNT_REPORT) service.uninstall() launcher.unlink(missing_ok=True) say("service + launcher removed.") @@ -80,15 +85,35 @@ def uninstall( say("data + secrets removed.") else: say("data + secrets kept (pass --purge-data to remove them).") - if not keep_account and _account_wanted( - remove_account, assume_yes, user, confirm + # Only this install's own report names the account it set chief up with; a + # bare `chief` in passwd may be someone else's, and userdel --remove takes + # the home with it. No report, no question and no steps. + if account is None: + say("no dedicated system account is recorded for this install.") + elif keep_account or not _account_wanted( + remove_account, assume_yes, account[0], confirm ): - for step in remove_steps(service.platform, user, group): - say(f" {step.description}") - execute(step) - say(f"system account {user} removed.") + say( + "system account kept " + f"(pass --remove-account to delete {account[0]})." + ) + if purge_data: + # The purge just took data/account-setup with it, and that is the + # only record of the account. Nothing here can remove it after + # this, so name the manual commands while they are still useful. + say( + f" its record is gone with data/ — remove {account[0]} by " + f"hand if you meant to: userdel --remove {account[0]}" + ) else: - say(f"system account kept (pass --remove-account to delete {user}).") + for step in remove_steps(service.platform, account[0], group): + say(f" {step.description}") + if execute(step).returncode != 0: + # Stop: groupdel --force after a failed userdel takes the group + # out from under an account that is still there. + say(f"system account {account[0]} could not be removed.") + return 1 + say(f"system account {account[0]} removed.") return 0 diff --git a/tests/test_imessage.py b/tests/test_imessage.py index f9a7ee9..ef5c4e4 100644 --- a/tests/test_imessage.py +++ b/tests/test_imessage.py @@ -4,6 +4,7 @@ import sqlite3 from collections.abc import Awaitable, Callable from pathlib import Path +from uuid import uuid4 import pytest @@ -31,7 +32,7 @@ CREATE TABLE message ( ROWID INTEGER PRIMARY KEY, handle_id INTEGER, text TEXT, is_from_me INTEGER DEFAULT 0, associated_message_type INTEGER DEFAULT 0, - attributedBody BLOB, date INTEGER DEFAULT 0 + attributedBody BLOB, date INTEGER DEFAULT 0, guid TEXT ); CREATE TABLE chat ( ROWID INTEGER PRIMARY KEY, style INTEGER, room_name TEXT, @@ -60,6 +61,7 @@ def add_message( chat: str | None = None, body: bytes | None = None, date: int = 0, + guid: str | None = None, ) -> None: """Insert one message, optionally in a direct chat (``chat`` = the chat_identifier) or a group. ``chat`` models the self-chat @@ -67,7 +69,9 @@ def add_message( room, so repeated calls land in one group thread. ``body`` sets attributedBody — how modern macOS stores the owner's own sends, with ``text`` left empty. ``date`` is the row's ns timestamp, used for - twin dedup.""" + twin dedup. ``guid`` defaults to a fresh one per row, as macOS does + even for self-DM twins; pass the same value into two stores to model + the one message both accounts received.""" with sqlite3.connect(self.path) as conn: row = conn.execute( "SELECT ROWID FROM handle WHERE id = ?", (sender,) @@ -81,9 +85,12 @@ def add_message( ) msg_id = conn.execute( "INSERT INTO message (handle_id, text, is_from_me, " - "associated_message_type, attributedBody, date) " - "VALUES (?, ?, ?, ?, ?, ?)", - (handle_id, text or None, from_me, tapback, body, date), + "associated_message_type, attributedBody, date, guid) " + "VALUES (?, ?, ?, ?, ?, ?, ?)", + ( + handle_id, text or None, from_me, tapback, body, date, + guid or f"{self.path.name}-{uuid4()}", + ), ).lastrowid chat_id: int | None = None if group and chat is None: @@ -643,6 +650,75 @@ async def test_chiefs_own_reply_in_the_owners_store_never_polls_back( assert [m.text for m in harness.delivered] == ["a friend texts the owner"] +async def test_a_group_both_accounts_are_in_is_delivered_once( + tmp_path: Path, +) -> None: + """Adding chief to a family group is the natural thing to do with a chief + that has its own contact card — and then every message in it exists in + BOTH stores under different rowids, one guid. Dedicated mode switches the + twin dedup off wholesale, which is right for chief's own store (no self-DM + twins there) but hands the owner's store a second delivery of every group + message: two stranger rows, two monitor runs, two classifier calls.""" + harness = Harness(tmp_path) + harness.dedicated = True + harness.owner_store = FakeStore(tmp_path / "owner.db") + harness.store.add_message( + "+15557776666", "dinner at 7", group=True, date=100, guid="g-dinner" + ) + harness.owner_store.add_message( + "+15557776666", "dinner at 7", group=True, date=100, guid="g-dinner" + ) + adapter = harness.adapter() + await adapter.poll_once() + await adapter.drain() + assert [m.text for m in harness.delivered] == ["dinner at 7"] + + +async def test_a_group_delivers_once_whichever_store_sees_it_first( + tmp_path: Path, +) -> None: + """The two stores keep independent cursors and two Messages processes write + them independently, so the owner's copy can land a tick ahead of chief's. + Suppressing only on the chief-first order leaves the other order double- + delivering — and read skew cannot defeat the guid key, because the window + is measured on the row's own date, identical in both stores.""" + harness = Harness(tmp_path) + harness.dedicated = True + harness.owner_store = FakeStore(tmp_path / "owner.db") + harness.owner_store.add_message( + "+15557776666", "dinner at 7", group=True, date=100, guid="g-dinner" + ) + adapter = harness.adapter() + await adapter.poll_once() # owner's store gets there first + await adapter.drain() + harness.store.add_message( + "+15557776666", "dinner at 7", group=True, date=100, guid="g-dinner" + ) + await adapter.poll_once() + await adapter.drain() + assert [m.text for m in harness.delivered] == ["dinner at 7"] + + +async def test_a_genuine_repeat_in_a_shared_group_is_not_swallowed( + tmp_path: Path, +) -> None: + """The cost of keying cross-store dedup on the text instead: someone says + "ok" twice in a group both accounts are in, and the second one is a real + message that main delivered. Distinct guids, so it survives.""" + harness = Harness(tmp_path) + harness.dedicated = True + harness.owner_store = FakeStore(tmp_path / "owner.db") + for n, when in ((1, 100), (2, 2_000_000_100)): # 2s apart + for store in (harness.store, harness.owner_store): + store.add_message( + "+15557776666", "ok", group=True, date=when, guid=f"g-ok-{n}" + ) + adapter = harness.adapter() + await adapter.poll_once() + await adapter.drain() + assert [m.text for m in harness.delivered] == ["ok", "ok"] + + async def test_each_store_keeps_its_own_cursor(tmp_path: Path) -> None: """Rowids are per-store: one shared cursor would skip whichever store is behind, silently dropping messages.""" diff --git a/tests/test_install.py b/tests/test_install.py index 280a544..7ae424a 100644 --- a/tests/test_install.py +++ b/tests/test_install.py @@ -23,6 +23,7 @@ from chief.install.account import Step, account_plan, default_home from chief.install.commands import ensure_config, main from chief.install.dedicated import existing_home, setup_account +from chief.install.dedicated_ask import ask, grant_reason from chief.install.lifecycle import uninstall from chief.install.posture import ON, UNKNOWN, Posture, chief_account, read_posture from chief.install.service import ServiceManager @@ -856,6 +857,41 @@ def test_granted_directories_are_group_permissions_and_never_the_home_root( assert f"sudo chgrp -R chief {home}" not in ran +def test_a_grant_that_reaches_past_the_home_root_is_refused( + tmp_path: Path, +) -> None: + """`grant_reason` backs a promise made in docs/SECURITY.md, and the grant + it gates is a recursive, irreversible chgrp. `~/..` resolves to the home's + parent — every account on the box. A typo'd path is refused here rather + than failing its step after the account and tree steps have landed.""" + home = tmp_path / "home" / "owner" + (home / "notes").mkdir(parents=True) + refused = ( + home / "..", # resolves to the parent of every account on the box + home, + Path("/"), + Path("notes"), # relative + home / "nope", # a typo, whose chgrp would abort the plan mid-flight + Path("/etc"), # group-write on sudoers/shadow is root, not a grant + ) + for path in refused: + assert grant_reason(path, home) is not None, path + assert grant_reason(home / "notes", home) is None + + +def test_the_account_name_must_be_a_plain_account_name(tmp_path: Path) -> None: + """It goes straight into argv and is joined onto the home root, where an + absolute name swallows the join whole: `Path("/Users") / "/etc"` is + `/etc`, i.e. `sysadminctl -addUser … -home /etc`.""" + prompts = iter(["e", "../etc", "chief bot", "chiefbot", "", ""]) + io = WizardIO( + prompt=lambda _: next(prompts), + prompt_secret=lambda _: "", + say=lambda _: None, + ) + assert ask(io, home=tmp_path).user == "chiefbot" + + def test_the_report_carries_what_install_sh_branches_on(tmp_path: Path) -> None: io, _, runner = _answers("c", "", "", "", "y") setup = setup_account( @@ -974,11 +1010,26 @@ def test_the_service_definition_can_be_written_without_starting_it( assert runner.calls == [] +def _account_report(repo_dir: Path, user: str = "nobody") -> None: + """The installer's record that *this* install owns an account named `user`. + + `nobody` on purpose: `chief_account` resolves the name through + `pwd.getpwnam`, so it must exist — and these tests build real + `sudo userdel --remove ` steps. They only stay inert because every + call site passes a recording executor; `uninstall`'s own default is a live + `subprocess.run`. The one account that exists everywhere and belongs to + nobody is the only safe name to write here. + """ + (repo_dir / "data").mkdir(parents=True, exist_ok=True) + (repo_dir / "data" / "account-setup").write_text(f"mode=create\nuser={user}\n") + + def test_uninstall_keeps_the_system_account_unless_asked(tmp_path: Path) -> None: runner = FakeRunner() manager = ServiceManager( platform="linux", home=tmp_path, runner=runner, uid=1000 ) + _account_report(tmp_path) said: list[str] = [] steps = RecordingRunner() uninstall( @@ -1000,6 +1051,8 @@ def test_uninstall_removes_the_account_when_asked(tmp_path: Path) -> None: manager = ServiceManager( platform="linux", home=tmp_path, runner=runner, uid=1000 ) + me = "nobody" + _account_report(tmp_path) steps = RecordingRunner() uninstall( service=manager, @@ -1012,6 +1065,164 @@ def test_uninstall_removes_the_account_when_asked(tmp_path: Path) -> None: execute=steps, ) assert [" ".join(s.command()) for s in steps.steps] == [ - "sudo userdel --remove chief", + f"sudo userdel --remove {me}", "sudo groupdel --force chief", ] + + +def test_keep_account_skips_the_question_and_the_steps(tmp_path: Path) -> None: + """--keep-account is the non-interactive "no", and had no coverage at all + on either side of this change.""" + runner = FakeRunner() + manager = ServiceManager( + platform="linux", home=tmp_path, runner=runner, uid=1000 + ) + _account_report(tmp_path) + said: list[str] = [] + steps = RecordingRunner() + code = uninstall( + service=manager, + launcher=tmp_path / "chief", + repo_dir=tmp_path, + purge_data=False, + assume_yes=False, + keep_account=True, + confirm=lambda _: pytest.fail("--keep-account already answered this"), + say=said.append, + execute=steps, + ) + assert code == 0 + assert steps.steps == [] + assert any("system account kept" in line for line in said) + + +def test_purging_data_warns_that_a_kept_account_becomes_unremovable( + tmp_path: Path, +) -> None: + """--purge-data --yes is the scripted teardown, and keeping is the default, + so the purge destroys the only record of an account it just kept. Nothing + can remove it after that, so the manual command has to be said out loud.""" + runner = FakeRunner() + manager = ServiceManager( + platform="linux", home=tmp_path, runner=runner, uid=1000 + ) + _account_report(tmp_path) + said: list[str] = [] + uninstall( + service=manager, + launcher=tmp_path / "chief", + repo_dir=tmp_path, + purge_data=True, + assume_yes=True, + say=said.append, + execute=RecordingRunner(), + ) + assert any("userdel --remove nobody" in line for line in said) + + +def test_uninstall_never_touches_an_account_this_install_did_not_create( + tmp_path: Path, +) -> None: + """A single-user install has no dedicated account, so the question must not + be asked — answering it yes would `userdel --remove` whatever pre-existing + account happens to be called `chief`.""" + runner = FakeRunner() + manager = ServiceManager( + platform="linux", home=tmp_path, runner=runner, uid=1000 + ) + said: list[str] = [] + steps = RecordingRunner() + code = uninstall( + service=manager, + launcher=tmp_path / "chief", + repo_dir=tmp_path, # no data/account-setup: nothing was ever created + purge_data=False, + assume_yes=False, + confirm=lambda _: pytest.fail("must not ask about an absent account"), + say=said.append, + execute=steps, + ) + assert code == 0 + assert steps.steps == [] + assert not any("system account chief removed." in line for line in said) + + +def test_purging_data_does_not_hide_the_account_from_the_same_run( + tmp_path: Path, +) -> None: + """--purge-data deletes data/, which is where the account report lives. Read + it before the rmtree or the run that was told to remove the account finds + no record of one, keeps it, and says it never existed.""" + runner = FakeRunner() + manager = ServiceManager( + platform="linux", home=tmp_path, runner=runner, uid=1000 + ) + _account_report(tmp_path) + said: list[str] = [] + steps = RecordingRunner() + uninstall( + service=manager, + launcher=tmp_path / "chief", + repo_dir=tmp_path, + purge_data=True, + assume_yes=True, + remove_account=True, + say=said.append, + execute=steps, + ) + assert "sudo userdel --remove nobody" in [ + " ".join(s.command()) for s in steps.steps + ] + assert "system account nobody removed." in said + + +def test_uninstall_stops_and_fails_when_a_removal_step_fails( + tmp_path: Path, +) -> None: + """groupdel --force after a failed userdel deletes the group out from under + an account that still exists, and a scripted uninstall reads exit 0 as + success.""" + runner = FakeRunner() + manager = ServiceManager( + platform="linux", home=tmp_path, runner=runner, uid=1000 + ) + _account_report(tmp_path) + steps = RecordingRunner(fail="userdel") + code = uninstall( + service=manager, + launcher=tmp_path / "chief", + repo_dir=tmp_path, + purge_data=False, + assume_yes=True, + remove_account=True, + say=lambda _: None, + execute=steps, + ) + assert code == 1 + assert not any("groupdel" in " ".join(s.command()) for s in steps.steps) + + +def test_uninstall_does_not_claim_removal_when_the_steps_fail( + tmp_path: Path, +) -> None: + """The return code was discarded, so a failed userdel still reported the + account gone — and the owner stops looking.""" + runner = FakeRunner() + manager = ServiceManager( + platform="linux", home=tmp_path, runner=runner, uid=1000 + ) + me = "nobody" + _account_report(tmp_path) + said: list[str] = [] + uninstall( + service=manager, + launcher=tmp_path / "chief", + repo_dir=tmp_path, + purge_data=False, + assume_yes=True, + remove_account=True, + say=said.append, + execute=RecordingRunner(fail="userdel"), + ) + assert f"system account {me} removed." not in said + assert any("could not be removed" in line for line in said)