From e55c9d5caa997c0432b0af63949a1b7b882ddcaa Mon Sep 17 00:00:00 2001 From: wtclaude Date: Wed, 5 Aug 2026 03:59:26 -0500 Subject: [PATCH] fix(installer): keep user_created sticky across re-runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found on the first real systemd host this installer has ever run on: a container with systemd as PID 1, installing into /usr/bin, /etc and /var/lib for real. `service::prepare` answers "did THIS run create the service account", which is false on every run after the first — by then the account exists. Recording that verbatim made the field describe the run rather than the state, with two consequences: - `install.json` changed on an otherwise-identical second run, breaking the Phase 1 promise that a re-run with nothing new to do writes nothing. - `uninstall` removes only an account it created, so after any second `install` it silently left behind the very user this tool had added. Reproduced before the fix: "left the runicgateway account alone — this installer did not create it", on a host where it plainly had. The record now inherits `true` from a prior record naming the same account, and only that account: inheriting across a rename would authorize deleting a user this installer never made. Not visible on Windows, where the SCM's virtual account is never created by us and goes with the service — which is why three phases of Windows smoke runs never showed it. Verified after the fix on the same host: fresh install records user_created true, an identical second run leaves install.json byte-identical, and uninstall then removes the account, the unit, the service and the binary — leaving sidecar.toml, the database and all 24 overlay files in the ServUO tree exactly where they were. Co-Authored-By: Claude --- src/install.rs | 90 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 88 insertions(+), 2 deletions(-) diff --git a/src/install.rs b/src/install.rs index b06447d..017ff0b 100644 --- a/src/install.rs +++ b/src/install.rs @@ -249,7 +249,14 @@ pub fn deploy(cli: &Cli, mode: Mode) -> Result<()> { )?; // ── The sidecar and its service ────────────────────────────────────────── - let sidecar = install_sidecar(cli, &bundle, &sidecar_asset, &layout, scratch.path())?; + let sidecar = install_sidecar( + cli, + &bundle, + &sidecar_asset, + &layout, + scratch.path(), + prior.as_ref(), + )?; // ── Record ─────────────────────────────────────────────────────────────── let record = build_record( @@ -415,6 +422,7 @@ fn install_sidecar( asset: &bundle::Asset, layout: &paths::Layout, scratch: &Path, + prior: Option<&InstallRecord>, ) -> Result> { ui::heading("uo-link sidecar"); @@ -539,7 +547,7 @@ fn install_sidecar( name: name.clone(), unit_path: unit_path.as_ref().map(|p| p.display().to_string()), user: user.clone(), - user_created: *user_created, + user_created: *user_created || created_by_an_earlier_run(prior, user.as_deref()), }) } service::Outcome::Skipped { reason, manual } => { @@ -572,6 +580,29 @@ fn install_sidecar( })) } +/// Whether an earlier run of this installer created the service account. +/// +/// **`user_created` has to be sticky, and this is why.** `service::prepare` answers "did *this run* +/// create the account", which is `false` on every run after the first — the account exists by then. +/// Recording that verbatim makes the field describe the run instead of the state, with two +/// consequences: `install.json` changes on an otherwise-identical second run (breaking Phase 1's +/// "a second run writes nothing"), and `uninstall` — which removes only an account it created — +/// silently leaves behind the very account this tool added. Both were caught on the first real +/// systemd host the installer ever ran on, and neither is visible on Windows, where the SCM's +/// virtual account is never "created" by us at all. +/// +/// Matched on the account *name*: a record naming a different user describes a different account, +/// and inheriting `true` from it would authorize deleting one this installer never made. +fn created_by_an_earlier_run(prior: Option<&InstallRecord>, user: Option<&str>) -> bool { + let (Some(prior), Some(user)) = (prior, user) else { + return false; + }; + prior + .link_record() + .and_then(|link| link.service) + .is_some_and(|service| service.user.as_deref() == Some(user) && service.user_created) +} + /// Fails the run before it writes anything if this process cannot write where it must. /// /// `create_dir_all` succeeding is not the same question as "can this process write here" — it @@ -759,6 +790,61 @@ mod tests { } } + #[test] + fn an_account_this_installer_created_stays_recorded_as_created() { + // Caught on the first real systemd host: `prepare` reports "did THIS run create it", which + // is false from the second run on. Recording that verbatim rewrote install.json on an + // identical re-run and, worse, left `uninstall` believing the account was somebody else's. + let mut record = record_for("/opt/ServUO"); + let service = |user: &str, created: bool| ServiceRecord { + kind: "systemd".into(), + name: "runicgateway-link.service".into(), + unit_path: Some("/etc/systemd/system/runicgateway-link.service".into()), + user: Some(user.into()), + user_created: created, + }; + let with_service = |record: &mut InstallRecord, service: ServiceRecord| { + record.link = serde_json::to_value(LinkRecord { + repo: "RunicGateway/link".into(), + tag: "v1.1.0".into(), + version: "1.1.0".into(), + protocol: 3, + binary: BinaryRef { + path: "/usr/bin/runicgateway-link".into(), + sha256: "aa".into(), + }, + config_path: "/etc/runicgateway/sidecar.toml".into(), + db_path: "/var/lib/runicgateway/uo-link.db".into(), + service: Some(service), + }) + .ok(); + }; + + with_service(&mut record, service("runicgateway", true)); + assert!(created_by_an_earlier_run( + Some(&record), + Some("runicgateway") + )); + + // An account this installer found already there stays somebody else's, forever. + with_service(&mut record, service("runicgateway", false)); + assert!(!created_by_an_earlier_run( + Some(&record), + Some("runicgateway") + )); + + // A record naming a different account says nothing about this one — inheriting `true` + // there would authorize deleting a user this installer never made. + with_service(&mut record, service("someone-else", true)); + assert!(!created_by_an_earlier_run( + Some(&record), + Some("runicgateway") + )); + + assert!(!created_by_an_earlier_run(None, Some("runicgateway"))); + assert!(!created_by_an_earlier_run(Some(&record), None)); + } + #[test] fn a_record_for_this_tree_is_used() { let record = record_for("/opt/ServUO");