From 24ee0990a2be733e56f5215be42713f9f3b1e9b2 Mon Sep 17 00:00:00 2001 From: atlas Date: Wed, 12 Aug 2026 18:32:33 +0200 Subject: [PATCH] =?UTF-8?q?feat(3201):=20swarmctl=20user=20update=20?= =?UTF-8?q?=E2=80=94=20change=20an=20existing=20subject's=20attributes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `user add` refuses on an existing name, so the `--group` flag it takes at creation time could not be added afterwards at all: repairing an account meant hand-editing both users.json and the rendered users.yml as root. mara, on #3167: "i will not edit those files by hand, we will have the same issues elsewhere". The merge rules live in users.rs as a pure function over a UserUpdate, so they are testable without a command line, a container or a running authelia — main.rs's arm only loads, applies, publishes and prints. Removals are strict and everything else is idempotent, which is the one asymmetry here and is deliberate: a --remove-group naming a group the user does not have fails, because a revocation that reports success without revoking is the outcome nobody re-checks; while refusing an already-satisfied set would make the multi-attribute call this verb exists for break whenever one of the values was already right. A command that changes nothing at all still fails — it would otherwise rewrite both files and restart the SSO provider to no effect. Passwords are out of scope: regenerating a credential is a different intent from editing an attribute, and folded together an attribute edit can invalidate a login by accident. Extracts publish() from user_add so both verbs share the render -> store -> users.yml -> restart ordering and the comment that explains why that order, rather than the second verb copying it. --- docs/swarm/sso.md | 31 +++++ docs/tools/swarmctl-cli.md | 23 ++++ swarmctl/src/main.rs | 89 ++++++++++++-- swarmctl/src/users.rs | 235 +++++++++++++++++++++++++++++++++++++ 4 files changed, 370 insertions(+), 8 deletions(-) diff --git a/docs/swarm/sso.md b/docs/swarm/sso.md index a0775860..4f49ca84 100644 --- a/docs/swarm/sso.md +++ b/docs/swarm/sso.md @@ -57,6 +57,37 @@ non-interactively means a secret arriving from somewhere — a file, an env var, a nix expression — and every one of those is worse than an operator typing one command once. +### Changing a subject afterwards + +`user add` only ever adds: on a name that already exists it refuses, +rather than resurfacing as a second account or a silent overwrite. +Editing an existing subject is `user update`, and the flags compose, so +one call can change several things: + +```console +# swarmctl user update mara --add-group admins --email mara@example.com +added to group "admins" +email: unset -> "mara@example.com" +mara is now in groups: admins +``` + +Two behaviours worth knowing before you rely on them: + +- **`--remove-group` fails if the user is not in that group.** Every + other flag is idempotent — setting what is already set is fine, so a + "make these four things true" call does not break when one of them + already was. Revocation is the exception on purpose: a typo'd group + name that reported success would leave an account holding access you + believe you took away, and that is the one outcome nobody re-checks. +- **The resulting group list is printed** because group names have no + registry anywhere. A misspelled `--add-group` creates a real group that + no access-control rule mentions, so the user gains nothing and no error + is possible — reading the line back is the only check there is. + +Passwords are deliberately out of scope here: regenerating a credential +is a different intent from editing an attribute, and folding them means +an attribute edit can invalidate a login by accident. + ## What secrets exist, and where each one lives | secret | generated by | rests in | read by | diff --git a/docs/tools/swarmctl-cli.md b/docs/tools/swarmctl-cli.md index fab4c561..1462dab8 100644 --- a/docs/tools/swarmctl-cli.md +++ b/docs/tools/swarmctl-cli.md @@ -7,6 +7,7 @@ This document contains the help content for the `swarmctl` command-line program. * [`swarmctl`↴](#swarmctl) * [`swarmctl user`↴](#swarmctl-user) * [`swarmctl user add`↴](#swarmctl-user-add) +* [`swarmctl user update`↴](#swarmctl-user-update) ## `swarmctl` @@ -37,6 +38,7 @@ Manage subjects in the swarm's SSO provider ###### **Subcommands:** * `add` — Add a user, generating a password for them +* `update` — Change an existing user's attributes @@ -58,6 +60,27 @@ Add a user, generating a password for them +## `swarmctl user update` + +Change an existing user's attributes. + +Every flag is optional and they compose, so one call can set several things at once. Deliberately does **not** touch the password: regenerating a credential is a different intent from editing an attribute, and folded together an attribute edit can invalidate a login by accident. + +**Usage:** `swarmctl user update [OPTIONS] ` + +###### **Arguments:** + +* `` — Login name of an existing user + +###### **Options:** + +* `--display-name ` — Name shown in the SSO UI +* `--email
` +* `--add-group ` — Repeatable. Adding a group the user is already in is not an error +* `--remove-group ` — Repeatable. Fails if the user is not in the group — a revocation that reports success without revoking is the failure nobody re-checks + + +
diff --git a/swarmctl/src/main.rs b/swarmctl/src/main.rs index d8c78c25..416bbee1 100644 --- a/swarmctl/src/main.rs +++ b/swarmctl/src/main.rs @@ -149,6 +149,14 @@ enum Verb { enum UserVerb { /// Add a user, generating a password for them. Add(AddArgs), + /// Change an existing user's attributes. + /// + /// Every flag is optional and they compose, so one call can set + /// several things at once. Deliberately does **not** touch the + /// password: regenerating a credential is a different intent from + /// editing an attribute, and folded together an attribute edit can + /// invalidate a login by accident. + Update(UpdateArgs), } #[derive(Args)] @@ -166,6 +174,25 @@ struct AddArgs { groups: Vec, } +#[derive(Args)] +struct UpdateArgs { + /// Login name of an existing user. + username: String, + /// Name shown in the SSO UI. + #[arg(long, value_name = "TEXT")] + display_name: Option, + #[arg(long, value_name = "ADDRESS")] + email: Option, + /// Repeatable. Adding a group the user is already in is not an error. + #[arg(long = "add-group", value_name = "GROUP")] + add_groups: Vec, + /// Repeatable. Fails if the user is not in the group — a revocation + /// that reports success without revoking is the failure nobody + /// re-checks. + #[arg(long = "remove-group", value_name = "GROUP")] + remove_groups: Vec, +} + fn main() -> Result<()> { let Cli { paths, command } = Cli::parse(); match command { @@ -175,6 +202,9 @@ fn main() -> Result<()> { Verb::User { command: UserVerb::Add(args), } => user_add(&paths.resolve()?, args), + Verb::User { + command: UserVerb::Update(args), + } => user_update(&paths.resolve()?, args), Verb::MarkdownDocs => { print!("{}", clap_markdown::help_markdown::()); Ok(()) @@ -206,11 +236,59 @@ fn user_add(paths: &Paths, args: AddArgs) -> Result<()> { }, ); + publish(paths, &store)?; + + println!("added {} to {}", args.username, paths.users_file.display()); + println!("password: {}", generated.password); + println!("this password is stored nowhere — record it now"); + Ok(()) +} + +fn user_update(paths: &Paths, args: UpdateArgs) -> Result<()> { + let mut store = load_store(&paths.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() + ); + }; + + let changes = users::apply_update( + user, + &users::UserUpdate { + displayname: args.display_name, + email: args.email, + add_groups: args.add_groups, + remove_groups: args.remove_groups, + }, + )?; + // Read back before the borrow ends: this is what the operator gets + // instead of a group registry we don't have — a typo'd `--add-group` + // is a real group with nobody reading it, and seeing the resulting + // list is the only way to notice. + let groups = users::fmt_groups(&user.groups); + + publish(paths, &store)?; + + for change in &changes { + println!("{change}"); + } + println!("{} is now in groups: {groups}", args.username); + Ok(()) +} + +/// Write the store + the rendered users file, then restart authelia. +/// +/// 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")?; + let rendered = users::render_yaml(store)?; + let store_json = serde_json::to_string_pretty(store).context("serialising the user store")?; // 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 @@ -219,12 +297,7 @@ fn user_add(paths: &Paths, args: AddArgs) -> Result<()> { write_atomic(&paths.store, &format!("{store_json}\n"))?; write_atomic(&paths.users_file, &rendered)?; - restart_authelia(&paths.machine, &paths.unit)?; - - println!("added {} to {}", args.username, paths.users_file.display()); - println!("password: {}", generated.password); - println!("this password is stored nowhere — record it now"); - Ok(()) + restart_authelia(&paths.machine, &paths.unit) } /// Load the canonical store, or start an empty one if this deployment has diff --git a/swarmctl/src/users.rs b/swarmctl/src/users.rs index 16c679ad..8d38e911 100644 --- a/swarmctl/src/users.rs +++ b/swarmctl/src/users.rs @@ -157,6 +157,117 @@ pub fn render_yaml(store: &UserStore) -> Result { Ok(out) } +/// A requested change to an existing user. +/// +/// A plain struct rather than the CLI's `Args` type, so the merge rules in +/// [`apply_update`] can be tested without building a command line — and so +/// the single place that decides what *update* means has no opinion about +/// how it is spelled. +#[derive(Debug, Default)] +pub struct UserUpdate { + pub displayname: Option, + pub email: Option, + pub add_groups: Vec, + pub remove_groups: Vec, +} + +/// Apply `update` to `user`, returning one line per change actually made. +/// +/// **Removals are strict; everything else is idempotent.** That asymmetry +/// is the whole safety argument of this function, so it is deliberate +/// rather than an oversight: +/// +/// - `--remove-group` on a group the user does not have **fails**. A +/// revocation that reports success without revoking is the one outcome +/// here nobody re-checks — you typo the group, the command says ok, and +/// the account keeps the access you believe you took away. +/// - Setting an attribute to the value it already holds, or adding a group +/// the user is already in, is **not** an error: the end state matches the +/// intent, and refusing would make the multi-attribute call this verb +/// exists for brittle — "set these four things" should not fail because +/// one of them was already right. +/// +/// A command that changes *nothing at all* still fails, because it would +/// otherwise rewrite both files and restart the SSO provider to no effect. +/// +/// On failure the `user` it was handed may be **partially mutated** — the +/// guarantee is not in-place atomicity but that the caller publishes +/// nothing on an error, so neither file and neither process ever sees a +/// half-applied update. Say it plainly rather than implying a rollback +/// this doesn't do. +/// +/// Note what cannot be validated here: group names are free-form strings +/// with no registry, so a typo'd `--add-group` creates a group nothing +/// references, and the user silently gains no access. The caller prints the +/// resulting group list for exactly that reason — it is the only signal +/// available. +pub fn apply_update(user: &mut User, update: &UserUpdate) -> Result> { + if let Some(dup) = update + .add_groups + .iter() + .find(|g| update.remove_groups.contains(g)) + { + bail!("group {dup:?} is both added and removed; refusing to guess an order"); + } + + let mut changes = Vec::new(); + + if let Some(name) = &update.displayname + && *name != user.displayname + { + reject_control_chars("displayname", name)?; + changes.push(format!("displayname: {:?} -> {name:?}", user.displayname)); + user.displayname.clone_from(name); + } + + if let Some(email) = &update.email + && user.email.as_deref() != Some(email.as_str()) + { + reject_control_chars("email", email)?; + changes.push(match &user.email { + Some(old) => format!("email: {old:?} -> {email:?}"), + None => format!("email: unset -> {email:?}"), + }); + user.email = Some(email.clone()); + } + + for group in &update.remove_groups { + let Some(at) = user.groups.iter().position(|g| g == group) else { + bail!( + "user is not in group {group:?}, so there is nothing to revoke \ + (groups: {})", + fmt_groups(&user.groups) + ); + }; + user.groups.remove(at); + changes.push(format!("removed from group {group:?}")); + } + + for group in &update.add_groups { + if user.groups.iter().any(|g| g == group) { + continue; + } + reject_control_chars("group", group)?; + user.groups.push(group.clone()); + changes.push(format!("added to group {group:?}")); + } + + if changes.is_empty() { + bail!("nothing to change — every requested value is already set"); + } + Ok(changes) +} + +/// Group list for a message, so an empty one reads as a word rather than +/// as a missing value. +pub fn fmt_groups(groups: &[String]) -> String { + if groups.is_empty() { + "none".to_owned() + } else { + groups.join(", ") + } +} + /// 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. /// @@ -297,6 +408,130 @@ mod tests { } } + fn with_groups(groups: &[&str]) -> User { + let mut u = user("$argon2id$x"); + u.groups = groups.iter().map(|g| (*g).to_owned()).collect(); + u + } + + /// The verb exists to change several things in one call, so the + /// composed case is the one that has to work. + #[test] + fn one_update_can_change_several_attributes() { + let mut u = with_groups(&["users"]); + let changes = apply_update( + &mut u, + &UserUpdate { + displayname: Some("Mara".to_owned()), + email: Some("mara@example.com".to_owned()), + add_groups: vec!["admins".to_owned()], + remove_groups: vec!["users".to_owned()], + }, + ) + .expect("applies"); + + assert_eq!(u.displayname, "Mara"); + assert_eq!(u.email.as_deref(), Some("mara@example.com")); + assert_eq!(u.groups, ["admins"]); + assert_eq!(changes.len(), 4, "every change is reported: {changes:?}"); + } + + /// ⭐ The asymmetry that is the point of this function. A revocation + /// that reports success without revoking is the failure nobody + /// re-checks — so a `--remove-group` naming a group the user is not + /// in must fail, and must leave the user untouched. + #[test] + fn removing_a_group_the_user_lacks_fails_and_changes_nothing() { + let mut u = with_groups(&["admins"]); + let err = apply_update( + &mut u, + &UserUpdate { + // The realistic shape: a typo for `admins`. + remove_groups: vec!["admin".to_owned()], + ..UserUpdate::default() + }, + ) + .expect_err("a no-op revocation must not report success"); + + assert!(err.to_string().contains("nothing to revoke"), "{err}"); + assert_eq!(u.groups, ["admins"], "the user must be untouched"); + } + + /// The other half of the asymmetry: setting what is already set is + /// fine, because the end state matches the intent. Refusing would + /// make "set these four things" fail when one was already right. + #[test] + fn already_satisfied_additions_are_not_errors() { + let mut u = with_groups(&["admins"]); + u.displayname = "Mara".to_owned(); + + let changes = apply_update( + &mut u, + &UserUpdate { + displayname: Some("Mara".to_owned()), + add_groups: vec!["admins".to_owned(), "ops".to_owned()], + ..UserUpdate::default() + }, + ) + .expect("a partially-satisfied update still applies the rest"); + + assert_eq!(u.groups, ["admins", "ops"], "no duplicate `admins`"); + assert_eq!( + changes.len(), + 1, + "only the real change reports: {changes:?}" + ); + } + + /// A command that changes nothing would still rewrite both files and + /// restart the SSO provider, so it is an error rather than a no-op. + #[test] + fn an_update_that_changes_nothing_fails() { + let mut u = with_groups(&["admins"]); + let err = apply_update( + &mut u, + &UserUpdate { + add_groups: vec!["admins".to_owned()], + ..UserUpdate::default() + }, + ) + .expect_err("a no-op must not restart authelia"); + assert!(err.to_string().contains("nothing to change"), "{err}"); + } + + #[test] + fn adding_and_removing_the_same_group_is_refused() { + let mut u = with_groups(&["admins"]); + let err = apply_update( + &mut u, + &UserUpdate { + add_groups: vec!["admins".to_owned()], + remove_groups: vec!["admins".to_owned()], + ..UserUpdate::default() + }, + ) + .expect_err("contradictory flags must not pick a winner silently"); + assert!(err.to_string().contains("both added and removed"), "{err}"); + } + + /// The update path writes the same file `render_yaml` validates, so a + /// control character has to be refused *before* it reaches the store — + /// not at render time, with the store already mutated. + #[test] + fn a_control_character_is_refused_by_the_update_path_too() { + let mut u = with_groups(&[]); + let err = apply_update( + &mut u, + &UserUpdate { + displayname: Some("bad\nname".to_owned()), + ..UserUpdate::default() + }, + ) + .expect_err("must refuse"); + assert!(err.to_string().contains("control character"), "{err}"); + 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. #[test]