diff --git a/docs/setup.md b/docs/setup.md index 8e333d5f..76235dbd 100644 --- a/docs/setup.md +++ b/docs/setup.md @@ -54,10 +54,6 @@ surfaces (the swarm UI below) are gated on that group, and an account without it authenticates successfully and is then refused — which reads like a broken login rather than a missing group. -If an account already exists without it, `user add` will refuse rather -than amend — adding the group afterwards is `swarmctl user update mara ---add-group admins`. - Detail, including what the password is and why this stays manual: [`swarm/sso.md`](swarm/sso.md). diff --git a/docs/swarm/sso.md b/docs/swarm/sso.md index 4f49ca84..a0775860 100644 --- a/docs/swarm/sso.md +++ b/docs/swarm/sso.md @@ -57,37 +57,6 @@ 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 1462dab8..fab4c561 100644 --- a/docs/tools/swarmctl-cli.md +++ b/docs/tools/swarmctl-cli.md @@ -7,7 +7,6 @@ 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` @@ -38,7 +37,6 @@ 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 @@ -60,27 +58,6 @@ 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 416bbee1..d8c78c25 100644 --- a/swarmctl/src/main.rs +++ b/swarmctl/src/main.rs @@ -149,14 +149,6 @@ 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)] @@ -174,25 +166,6 @@ 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 { @@ -202,9 +175,6 @@ 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(()) @@ -236,59 +206,11 @@ 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 @@ -297,7 +219,12 @@ fn publish(paths: &Paths, store: &UserStore) -> Result<()> { write_atomic(&paths.store, &format!("{store_json}\n"))?; write_atomic(&paths.users_file, &rendered)?; - restart_authelia(&paths.machine, &paths.unit) + 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(()) } /// 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 8d38e911..16c679ad 100644 --- a/swarmctl/src/users.rs +++ b/swarmctl/src/users.rs @@ -157,117 +157,6 @@ 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. /// @@ -408,130 +297,6 @@ 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]