From af1203c78babba37dc913b50d45d48c424213baa Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 18 Aug 2026 10:25:26 +0200 Subject: [PATCH] fix(#3422): swarmctl reads and writes users.yml directly The second half of making one file canonical. swarmctl kept its own private JSON store and rendered users.yml from it, against the same physical file the bridge wrote -- the seam that made `swarm agent create` refuse to run. - users.yml is read before it is written, so users another writer added are loaded rather than treated as a file to refuse or clobber. The overwrite guard and the seed check go with the second store: they existed to police two stores that could disagree. - serde_norway replaces the hand-rolled emitter. Unknown top-level and per-user keys round-trip through `extra`, so two writers cannot delete each other's fields. - The synthetic email moves from render time to the write path and is stored. With the file as the store, "rendered but not persisted" has nowhere left to live, and mara ruled the stored address correct. - `--store` is removed rather than deprecated: a flag whose only remaining effect is nothing reads as accepted and does nothing. Three tests asserted the emitter's exact bytes, and one asserted the guard. Rewritten rather than deleted -- as round-trips for the former, and inverted for the latter, since "a populated file is READ" is the behaviour this change is for and deleting its test would leave it unpinned. --- swarmctl/src/main.rs | 152 +++++++++---------- swarmctl/src/users.rs | 342 +++++++++++++++++++++++------------------- 2 files changed, 260 insertions(+), 234 deletions(-) diff --git a/swarmctl/src/main.rs b/swarmctl/src/main.rs index 925605f6..7630a59f 100644 --- a/swarmctl/src/main.rs +++ b/swarmctl/src/main.rs @@ -34,13 +34,6 @@ use clap::{Args, Parser, Subcommand}; use users::{User, UserStore}; -/// Canonical user store. A compiled-in default is legitimate here for the -/// same reason it is on the daemon's socket path: this is a path this -/// process **creates**, not an address it hopes to find something at. It -/// lives under the controller's state directory because the controller is -/// this store's eventual reader. -const DEFAULT_STORE: &str = "/var/lib/swarm-controller/users.json"; - #[derive(Parser)] #[command(name = "swarmctl", version, about = "swarm-level operator CLI")] struct Cli { @@ -67,17 +60,20 @@ struct PathArgs { authelia_bin: Option, /// Host-side path of authelia's users database — i.e. the path inside /// the container, prefixed with the container's root. + /// + /// ⚠️ This is the **only** user store. There used to be a `--store` + /// flag naming a private canonical JSON that this file was rendered + /// from; it is gone rather than deprecated, because a flag whose only + /// remaining effect would be nothing is worse than an unknown-argument + /// error — the operator sets it, sees success, and gets none of what + /// they asked for. #[arg(long, value_name = "PATH")] users_file: Option, - /// Canonical user store. - #[arg(long, value_name = "PATH")] - store: Option, } struct Paths { authelia_bin: PathBuf, users_file: PathBuf, - store: PathBuf, } impl PathArgs { @@ -85,10 +81,6 @@ impl PathArgs { Ok(Paths { authelia_bin: path_from(self.authelia_bin, "SWARMCTL_AUTHELIA_BIN")?, users_file: path_from(self.users_file, "SWARMCTL_AUTHELIA_USERS_FILE")?, - store: self - .store - .or_else(|| std::env::var_os("SWARMCTL_STORE").map(PathBuf::from)) - .unwrap_or_else(|| PathBuf::from(DEFAULT_STORE)), }) } } @@ -238,12 +230,12 @@ fn main() -> Result<()> { fn user_add(paths: &Paths, args: AddArgs) -> Result<()> { users::validate_username(&args.username)?; - let mut store = load_store(&paths.store, &paths.users_file)?; + let mut store = load_store(&paths.users_file)?; if store.users.contains_key(&args.username) { bail!( "user {:?} already exists in {}", args.username, - paths.store.display() + paths.users_file.display() ); } @@ -256,10 +248,11 @@ fn user_add(paths: &Paths, args: AddArgs) -> Result<()> { password: generated.digest, email: args.email, groups: args.groups, + extra: std::collections::BTreeMap::new(), }, ); - publish(paths, &store)?; + publish(paths, &mut store)?; println!("added {} to {}", args.username, paths.users_file.display()); println!("password: {}", generated.password); @@ -268,12 +261,12 @@ fn user_add(paths: &Paths, args: AddArgs) -> Result<()> { } fn user_update(paths: &Paths, args: UpdateArgs) -> Result<()> { - let mut store = load_store(&paths.store, &paths.users_file)?; + let mut store = load_store(&paths.users_file)?; let Some(user) = store.users.get_mut(&args.username) else { bail!( "no user {:?} in {} — `swarmctl user add` creates one", args.username, - paths.store.display() + paths.users_file.display() ); }; @@ -292,7 +285,7 @@ fn user_update(paths: &Paths, args: UpdateArgs) -> Result<()> { // list is the only way to notice. let groups = users::fmt_groups(&user.groups); - publish(paths, &store)?; + publish(paths, &mut store)?; for change in &changes { println!("{change}"); @@ -306,9 +299,9 @@ fn user_update(paths: &Paths, args: UpdateArgs) -> Result<()> { fn user_list(paths: &Paths) -> Result<()> { use std::fmt::Write as _; - let store = load_store(&paths.store, &paths.users_file)?; + let store = load_store(&paths.users_file)?; if store.users.is_empty() { - println!("no users in {}", paths.store.display()); + println!("no users in {}", paths.users_file.display()); return Ok(()); } for (username, user) in &store.users { @@ -324,23 +317,23 @@ fn user_list(paths: &Paths) -> Result<()> { Ok(()) } -/// Write the store + the rendered users file, then restart authelia. +/// Write the users database. /// -/// Shared by every verb that mutates the store, so the ordering rules -/// below hold for all of them rather than for whichever one was written -/// first. -fn publish(paths: &Paths, store: &UserStore) -> Result<()> { - // Render before writing anything: a value this refuses to emit should - // stop the whole operation, not leave the canonical store one user - // ahead of the file authelia reads. - let rendered = users::render_yaml(store)?; - let store_json = serde_json::to_string_pretty(store).context("serialising the user store")?; +/// Shared by every verb that mutates it, so the rules below hold for all of +/// them rather than for whichever one was written first. +fn publish(paths: &Paths, store: &mut UserStore) -> Result<()> { + // Before rendering, not at creation: a user can also arrive by being + // *read* — from a file the bridge wrote, or one an operator edited — + // and an authelia subject with no `email` breaks any relying party that + // asks for the claim (grafana's OIDC login is the measured case, + // #3393). Filling it here is the only place no entry point can skip. + for name in users::fill_missing_emails(store) { + println!("note: {name} had no email; set to a synthetic address"); + } - // Store first, and the order matters. If the store lands and the - // users file does not, the next run re-renders and repairs it. The - // other order loses a user: the store would not know about someone - // authelia does, and the next render would silently drop them. - write_atomic(&paths.store, &format!("{store_json}\n"))?; + // Render before writing: a value this refuses to emit should stop the + // whole operation rather than land a partial file. + let rendered = users::render_yaml(store)?; write_atomic(&paths.users_file, &rendered)?; // No restart: authelia watches this file @@ -351,38 +344,25 @@ fn publish(paths: &Paths, store: &UserStore) -> Result<()> { Ok(()) } -/// Load the canonical store, or start an empty one if this deployment has -/// never had a user added. +/// Load the users database, or start empty when it does not exist yet. /// -/// The guard exists because starting empty means the next write -/// **overwrites** authelia's users file. That is only safe when the file -/// is the untouched first-boot seed; anything else is a user database -/// somebody meant to be there, and losing it is the one unrecoverable -/// mistake this tool can make. -fn load_store(store_path: &Path, users_file: &Path) -> Result { - match fs::read_to_string(store_path) { - Ok(raw) => serde_json::from_str(&raw) - .with_context(|| format!("parsing the user store at {}", store_path.display())), - Err(e) if e.kind() == std::io::ErrorKind::NotFound => { - match fs::read_to_string(users_file) { - Ok(existing) if !users::is_untouched_seed(&existing) => bail!( - "no user store at {} but {} already holds users — refusing to \ - overwrite it. Reconstruct the store, or move the file aside if \ - it is disposable.", - store_path.display(), - users_file.display() - ), - Ok(_) => Ok(UserStore::default()), - Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(UserStore::default()), - Err(e) => Err(e).with_context(|| { - format!( - "reading {} to check it is safe to take over", - users_file.display() - ) - }), - } - } - Err(e) => Err(e).with_context(|| format!("reading {}", store_path.display())), +/// ⚠️ **The overwrite guard that used to live here is gone, and its absence +/// is the fix rather than a regression.** It refused to write when this +/// crate's own JSON store was missing but `users.yml` held users — it +/// existed to police *two* stores that could disagree, and there is now one. +/// Reading the same file we are about to write means we cannot clobber users +/// we did not know about: we just read them. +/// +/// An absent or empty file is an empty store, not an error. First boot is a +/// legitimate state, and the seed document authelia's own unit writes +/// (`users: {}`) deserialises to exactly that with no special case. +fn load_store(users_file: &Path) -> Result { + match fs::read_to_string(users_file) { + Ok(raw) if raw.trim().is_empty() => Ok(UserStore::default()), + Ok(raw) => serde_norway::from_str(&raw) + .with_context(|| format!("parsing the users database at {}", users_file.display())), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(UserStore::default()), + Err(e) => Err(e).with_context(|| format!("reading {}", users_file.display())), } } @@ -562,25 +542,37 @@ mod tests { fs::remove_dir_all(&dir).ok(); } - /// The guard that stands between a missing store and an overwritten - /// user database. + /// Replaces `load_store_refuses_to_take_over_a_populated_users_file`, + /// which pinned the guard this change removes. + /// + /// The guard refused to write when this crate's own JSON store was + /// missing but `users.yml` held users — the state every real hive is in, + /// and the reason `swarm agent create` failed. Deleting the test with + /// the guard would have left the *new* behaviour unpinned, so it is + /// inverted rather than dropped: a populated file must now be **read**, + /// and users somebody else wrote must survive. #[test] - fn load_store_refuses_to_take_over_a_populated_users_file() { - let dir = std::env::temp_dir().join(format!("swarmctl-guard-{}", std::process::id())); + fn a_populated_users_file_is_read_rather_than_refused() { + let dir = std::env::temp_dir().join(format!("swarmctl-takeover-{}", std::process::id())); fs::create_dir_all(&dir).expect("temp dir"); - let store = dir.join("absent.json"); let users_file = dir.join("users.yml"); - fs::write(&users_file, "users:\n mara:\n password: \"x\"\n").expect("write"); - let err = load_store(&store, &users_file).expect_err("must refuse"); + fs::write( + &users_file, + "users:\n mara:\n displayname: \"mara\"\n password: \"x\"\n", + ) + .expect("write"); + let store = load_store(&users_file).expect("a populated file is readable"); assert!( - err.to_string().contains("refusing to overwrite"), - "unexpected error: {err}" + store.users.contains_key("mara"), + "a user this process did not write must be read, not refused" ); fs::write(&users_file, "users: {}\n").expect("seed"); - let taken = load_store(&store, &users_file).expect("the seed is takeable"); - assert!(taken.users.is_empty()); + assert!( + load_store(&users_file).expect("the seed loads").users.is_empty(), + "the first-boot seed is an empty store, with no special case" + ); fs::remove_dir_all(&dir).ok(); } diff --git a/swarmctl/src/users.rs b/swarmctl/src/users.rs index a423d10d..0b36270c 100644 --- a/swarmctl/src/users.rs +++ b/swarmctl/src/users.rs @@ -1,49 +1,46 @@ -//! The swarm's SSO user store, and the authelia users database rendered -//! from it. +//! The swarm's SSO user store — authelia's own `users.yml`, read and +//! written directly. //! -//! **Two files, and only one of them is authoritative.** `users.json` is -//! ours and canonical; `users.yml` is a rendered artifact for authelia, -//! written and never read back. +//! **One file.** There used to be a private `users.json` here, canonical, +//! with `users.yml` rendered from it — and `swarm-authelia-bridge` kept its +//! own pair against the *same* physical `users.yml`. Two canonical stores +//! for one file is a seam, and it bit: a writer whose own store was missing +//! could not tell *"nothing here yet"* from *"someone else's users"*, and +//! refused to write at all. //! -//! That split is not a preference, it is what lets this crate exist -//! without a YAML parser: the workspace has none, and adding one costs a -//! crates.io fetch, a lock update and a vendor hash — for a schema we -//! fully control and only ever emit. Rendering in one direction needs no -//! parser at all. +//! The JSON bought nothing. The old module doc argued it let this crate +//! exist without a YAML parser — but `load_store` read the JSON back on +//! every run, so the round-trip was already being paid; the two files +//! differed only in *format*. //! -//! The tempting shortcut — *"JSON is a subset of YAML, so just write JSON -//! into the `.yml` and read it back with `serde_json`"* — is rejected on -//! purpose. authelia **refuses to start** on a users file it cannot -//! parse, so it fronts the whole SSO provider's boot, and "go-yaml -//! almost certainly accepts flow style" is not a claim worth betting that -//! on without running it. Block style is what upstream's own examples -//! show. +//! ⚠️ Consequence, deliberate: this file is **round-tripped**, so comments +//! and hand-formatting do not survive a write. An operator editing it +//! directly keeps their *values* and loses their *comments*. Unknown keys +//! survive (see `extra`), so a field this binary does not model is not +//! deleted by it. use std::collections::BTreeMap; -use std::fmt::Write as _; -use anyhow::{Result, bail}; +use anyhow::{Context, Result, bail}; use serde::{Deserialize, Serialize}; -/// What the swarm-authelia module's first-boot unit writes into -/// `users.yml` when there is no database yet. +/// The canonical store — this *is* `users.yml`, deserialised. /// -/// Matched (after trimming) so a first `user add` can take the file over -/// without a flag, while anything *else* already in place is treated as -/// content somebody meant to be there. Overwriting a live SSO user -/// database because a canonical store happened to be missing is the one -/// unrecoverable mistake available here. -pub const SEED_USERS_FILE: &str = "users: {}"; - -/// The canonical store, serialised as JSON. -/// -/// `BTreeMap` rather than `HashMap` so the rendered YAML is stable -/// between runs: a diffable artifact is worth more than the ordering -/// being meaningless, and a random-order rewrite makes every change look -/// like a whole-file change. +/// `BTreeMap` rather than `HashMap` so the file is stable between runs: a +/// diffable artifact is worth more than the ordering being meaningless, and +/// a random-order rewrite makes every change look like a whole-file change. #[derive(Debug, Default, Serialize, Deserialize)] pub struct UserStore { pub users: BTreeMap, + /// Top-level keys this binary does not model, carried through a + /// round-trip untouched. + /// + /// Two processes write this file and neither is authoritative about the + /// other's fields. Without this, whichever writes second silently + /// deletes whatever the first added — the same shape as the bug that + /// made one file canonical in the first place, one level down. + #[serde(flatten)] + pub extra: BTreeMap, } #[derive(Debug, Serialize, Deserialize)] @@ -57,6 +54,10 @@ pub struct User { pub email: Option, #[serde(default, skip_serializing_if = "Vec::is_empty")] pub groups: Vec, + /// Per-user keys this binary does not model (authelia's `disabled`, for + /// one). Same preservation rule as [`UserStore::extra`]. + #[serde(flatten)] + pub extra: BTreeMap, } /// Usernames are map **keys** in a YAML document, and they also reach @@ -100,28 +101,6 @@ fn reject_control_chars(field: &str, value: &str) -> Result<()> { Ok(()) } -/// A double-quoted YAML scalar. -/// -/// Everything is quoted, including values that would be fine bare: the -/// argon2 digest alone contains `$`, `=`, `,` and `/`, and deciding -/// per-value which of those need quoting is exactly the kind of judgement -/// that is right until one day it isn't. Control characters are excluded -/// upstream by [`reject_control_chars`], so `"` and `\` are the complete -/// escape set. -fn quote(s: &str) -> String { - let mut out = String::with_capacity(s.len() + 2); - out.push('"'); - for c in s.chars() { - match c { - '"' => out.push_str("\\\""), - '\\' => out.push_str("\\\\"), - _ => out.push(c), - } - } - out.push('"'); - out -} - /// Domain for an address this crate invents. /// /// The same one `hive-c0re` already gives every agent's forge account @@ -145,52 +124,66 @@ const SYNTHETIC_EMAIL_DOMAIN: &str = "hyperhive.local"; /// implement, and the login dies with `InternalError` rather than anything /// naming the missing field. /// -/// Synthesised in the **renderer**, never written to the store: `users.json` -/// stays honest that no address was supplied, so an operator who later sets a -/// real one is not fighting a value swarmctl invented, and every existing user -/// is fixed by the next render with no migration step. +/// Synthesised on the **write path** and stored, because the file *is* the +/// store now: "rendered but not persisted" no longer has anywhere to live. +/// mara ruled it directly — *"email in yml is what is already there and +/// correct"* — so a synthesised address is simply the user's address, and +/// the next `user update --email` overwrites it like any other value. fn synthetic_email(username: &str) -> String { format!("{username}@{SYNTHETIC_EMAIL_DOMAIN}") } -/// Render the store as authelia's block-style YAML users database. +/// Give every user an address before the store is written. /// -/// Fallible because it re-runs validation over every value it is about to -/// emit. That is not belt-and-braces: it makes "no control character ever -/// reaches the file" a property of the *only* path that writes it, rather -/// than a rule the call sites have to remember. -pub fn render_yaml(store: &UserStore) -> Result { - if store.users.is_empty() { - return Ok(format!("{SEED_USERS_FILE}\n")); +/// Returns the names it filled, so a caller can say what it did rather than +/// changing the file silently. +/// +/// On the write path rather than at creation, because users also arrive by +/// being *read* — from a file another writer produced, or one an operator +/// edited. Fixing them where they enter would need every entry point to +/// remember; fixing them where they leave cannot be forgotten. +pub fn fill_missing_emails(store: &mut UserStore) -> Vec { + let mut filled = Vec::new(); + for (name, user) in &mut store.users { + if user.email.is_none() { + user.email = Some(synthetic_email(name)); + filled.push(name.clone()); + } } + filled +} - let mut out = String::from("users:\n"); +/// Serialise the store as authelia's YAML users database. +/// +/// Serialisation is `serde_norway`'s: quoting an argon2 digest (`$`, `=`, +/// `,`, `/`) is exactly the kind of thing a real emitter gets right and a +/// hand-rolled one gets right until it doesn't. +/// +/// ⚠️ **Still fallible, and that is the load-bearing part.** It re-runs +/// validation over every value it is about to emit, which is what makes +/// *"no control character ever reaches this file"* a property of the **one +/// path that writes it** rather than a rule every call site must remember. +/// A bare `to_string(&store)` serialises perfectly and drops that silently. +pub fn render_yaml(store: &UserStore) -> Result { + validate(store)?; + serde_norway::to_string(store).context("serialising the users database") +} + +/// Everything [`render_yaml`] refuses to write. Separate so the rule can be +/// tested directly, without going through serialisation. +fn validate(store: &UserStore) -> Result<()> { for (name, user) in &store.users { validate_username(name)?; reject_control_chars("displayname", &user.displayname)?; reject_control_chars("password digest", &user.password)?; - - writeln!(out, " {name}:")?; - writeln!(out, " displayname: {}", quote(&user.displayname))?; - writeln!(out, " password: {}", quote(&user.password))?; - // Unconditional: an absent email is the failure mode, not a valid - // rendering. The synthetic address is validated on the same path as - // a supplied one so neither can smuggle a control character. - let email = match &user.email { - Some(supplied) => supplied.clone(), - None => synthetic_email(name), - }; - reject_control_chars("email", &email)?; - writeln!(out, " email: {}", quote(&email))?; - if !user.groups.is_empty() { - writeln!(out, " groups:")?; - for group in &user.groups { - reject_control_chars("group", group)?; - writeln!(out, " - {}", quote(group))?; - } + if let Some(email) = &user.email { + reject_control_chars("email", email)?; + } + for group in &user.groups { + reject_control_chars("group", group)?; } } - Ok(out) + Ok(()) } /// A requested change to an existing user. @@ -304,17 +297,6 @@ pub fn fmt_groups(groups: &[String]) -> String { } } -/// Whether an existing `users.yml` is safe to take over when no canonical -/// store exists yet — i.e. it is the untouched first-boot seed. -/// -/// Empty counts: a zero-byte file holds no users to destroy, and authelia -/// would refuse to start on it anyway, so treating it as untouched turns -/// a dead deployment into a working one instead of demanding a flag. -pub fn is_untouched_seed(contents: &str) -> bool { - let trimmed = contents.trim(); - trimmed.is_empty() || trimmed == SEED_USERS_FILE -} - #[cfg(test)] mod tests { use super::*; @@ -325,39 +307,91 @@ mod tests { password: password.to_owned(), email: None, groups: Vec::new(), + extra: BTreeMap::new(), } } #[test] - fn an_empty_store_renders_the_seed_document() { + fn an_empty_store_round_trips_as_an_empty_store() { let out = render_yaml(&UserStore::default()).expect("empty store renders"); - assert_eq!(out, "users: {}\n"); + let back: UserStore = serde_norway::from_str(&out).expect("re-reads"); assert!( - is_untouched_seed(&out), - "the empty rendering must itself read as the seed, or removing the \ - last user would produce a file the next run refuses to take over" + back.users.is_empty(), + "removing the last user must leave a file the next run can load, got:\n{out}" ); } /// The digest is the value most likely to break a naive emitter: it /// carries `$`, `=`, `,` and `/`, and `,` in particular terminates a - /// YAML flow scalar. + /// YAML flow scalar. Asserted through a **round-trip** rather than + /// against a literal quoting style: which scalars a real emitter chooses + /// to quote is its business, and pinning the spelling would make this + /// test fail on a serializer upgrade that broke nothing. #[test] - fn an_argon2_digest_survives_quoting() { + fn an_argon2_digest_survives_a_round_trip() { let digest = "$argon2id$v=19$m=65536,t=3,p=4$c29tZXNhbHQ$aGFzaA+/w=="; let mut store = UserStore::default(); store.users.insert("mara".to_owned(), user(digest)); let out = render_yaml(&store).expect("renders"); - assert!( - out.contains(&format!("password: \"{digest}\"")), - "digest must be emitted verbatim inside double quotes, got:\n{out}" + let back: UserStore = serde_norway::from_str(&out).expect("re-reads"); + assert_eq!( + back.users["mara"].password, digest, + "digest must survive verbatim, got:\n{out}" ); } + /// Two processes write this file and neither knows the other's fields, + /// so anything unmodelled has to survive being read and written back — + /// otherwise the second writer silently deletes the first's work, which + /// is the same class of bug as the two-canonical-stores seam this change + /// removed, one level down. #[test] - fn quoting_escapes_backslash_and_quote() { - assert_eq!(quote(r#"a"b\c"#), r#""a\"b\\c""#); + fn unknown_keys_survive_a_round_trip() { + let raw = "\ +theme: dark +users: + mara: + displayname: \"mara\" + password: \"$argon2id$x\" + disabled: true +"; + let mut store: UserStore = serde_norway::from_str(raw).expect("parses"); + let out = render_yaml(&store).expect("renders"); + assert!( + out.contains("theme"), + "a top-level key this binary does not model was dropped:\n{out}" + ); + assert!( + out.contains("disabled"), + "a per-user key this binary does not model was dropped:\n{out}" + ); + + // ...and it must still survive once we have touched the entry the + // way a real verb does. + fill_missing_emails(&mut store); + let out = render_yaml(&store).expect("renders"); + assert!(out.contains("disabled"), "dropped after a write:\n{out}"); + } + + /// mara: *"email in yml is what is already there and correct"* — with the + /// file as the store, an address is filled in on the write path and kept, + /// rather than invented afresh by each render. + #[test] + fn a_user_with_no_email_gets_one_stored() { + let mut store = UserStore::default(); + store.users.insert("mara".to_owned(), user("$argon2id$x")); + + let filled = fill_missing_emails(&mut store); + assert_eq!(filled, vec!["mara".to_owned()]); + assert_eq!( + store.users["mara"].email.as_deref(), + Some("mara@hyperhive.local") + ); + + // Idempotent: a second pass must not report a change, or every + // publish would print a note about a user it did not touch. + assert!(fill_missing_emails(&mut store).is_empty()); } #[test] @@ -372,22 +406,9 @@ mod tests { ); } - /// Email is deliberately NOT in the test above any more. It used to - /// assert that an absent one is omitted, which pinned the behaviour that - /// broke grafana's login: authelia serves no `email` claim, and a relying - /// party that wants one fails rather than degrading. - #[test] - fn a_user_with_no_email_still_renders_one_from_the_shared_domain() { - let mut store = UserStore::default(); - store.users.insert("mara".to_owned(), user("$argon2id$x")); - - let out = render_yaml(&store).expect("renders"); - assert!( - out.contains(r#"email: "mara@hyperhive.local""#), - "a user with no email must still render one:\n{out}" - ); - } - + /// The #3414 property, moved from the renderer to the write path along + /// with the synthesis itself: a user who supplied an address keeps it, + /// and no invented one appears beside it. #[test] fn a_supplied_email_is_never_replaced_by_the_synthetic_one() { let mut u = user("$argon2id$x"); @@ -395,11 +416,16 @@ mod tests { let mut store = UserStore::default(); store.users.insert("mara".to_owned(), u); - let out = render_yaml(&store).expect("renders"); assert!( - out.contains(r#"email: "real@elsewhere.example""#), - "the supplied address must win:\n{out}" + fill_missing_emails(&mut store).is_empty(), + "a user with an address is not missing one" ); + assert_eq!( + store.users["mara"].email.as_deref(), + Some("real@elsewhere.example"), + "the supplied address must win" + ); + let out = render_yaml(&store).expect("renders"); assert!( !out.contains("mara@hyperhive.local"), "the synthetic address must not also appear:\n{out}" @@ -416,8 +442,13 @@ mod tests { assert_eq!(synthetic_email("mara"), "mara@hyperhive.local"); } + /// Was `groups_render_as_a_block_sequence`, a byte-for-byte assertion on + /// the hand-rolled emitter's output — including which scalars it chose to + /// quote. That is the serializer's business now, and pinning it would + /// fail on an upgrade that broke nothing. What has to hold is that the + /// values come back, in order. #[test] - fn groups_render_as_a_block_sequence() { + fn a_users_groups_survive_a_round_trip_in_order() { let mut u = user("$argon2id$x"); u.email = Some("mara@example.com".to_owned()); u.groups = vec!["admins".to_owned(), "operators".to_owned()]; @@ -425,19 +456,12 @@ mod tests { store.users.insert("mara".to_owned(), u); let out = render_yaml(&store).expect("renders"); - assert_eq!( - out, - concat!( - "users:\n", - " mara:\n", - " displayname: \"Test User\"\n", - " password: \"$argon2id$x\"\n", - " email: \"mara@example.com\"\n", - " groups:\n", - " - \"admins\"\n", - " - \"operators\"\n", - ) - ); + let back: UserStore = serde_norway::from_str(&out).expect("re-reads"); + let mara = &back.users["mara"]; + assert_eq!(mara.displayname, "Test User"); + assert_eq!(mara.password, "$argon2id$x"); + assert_eq!(mara.email.as_deref(), Some("mara@example.com")); + assert_eq!(mara.groups, ["admins", "operators"], "order is meaningful"); } /// Ordering is a property of the artifact, not an accident: an @@ -608,17 +632,27 @@ mod tests { assert_eq!(u.displayname, "Test User", "the user must be untouched"); } - /// The seed check gates an irreversible overwrite, so it has to be - /// tolerant of trailing whitespace and intolerant of everything else. + /// Replaces `only_the_untouched_seed_reads_as_takeable`. That test + /// pinned the seed check, which existed to decide whether overwriting + /// `users.yml` was safe — a question that only arose because a *second* + /// store claimed to be canonical. There is no overwrite to gate now: the + /// file is read before it is written. + /// + /// What still has to hold is that the first-boot seed and a zero-byte + /// file both mean "no users yet" rather than an error, so a fresh + /// deployment is not stranded. #[test] - fn only_the_untouched_seed_reads_as_takeable() { - assert!(is_untouched_seed("users: {}")); - assert!(is_untouched_seed("users: {}\n")); - assert!(!is_untouched_seed("users:\n mara:\n")); + fn the_first_boot_seed_and_an_empty_file_both_mean_no_users() { + let seeded: UserStore = serde_norway::from_str("users: {}").expect("the seed parses"); + assert!(seeded.users.is_empty()); + + // The empty-file case is handled before deserialisation (an empty + // document is not valid YAML for this type), so it is asserted where + // it lives — `main::load_store` — rather than reproduced here. assert!( - is_untouched_seed(""), - "a zero-byte file has no users to lose, and authelia will not start \ - on it — refusing here would strand the deployment" + serde_norway::from_str::("").is_err(), + "if this ever starts parsing, load_store's empty-file arm is \ + redundant rather than load-bearing, and should be revisited" ); } }