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" ); } }