diff --git a/docs/integrations/matrix.md b/docs/integrations/matrix.md index 90cb0772..6ce2b3cf 100644 --- a/docs/integrations/matrix.md +++ b/docs/integrations/matrix.md @@ -184,6 +184,10 @@ lands on. - **No account, no grant.** A member who has never logged in to matrix has no account to promote. The first pass after their first SSO login promotes them. +- **One command per member per controller run.** A member still missing + from `#admins` after it's sent gets a `make-user-admin … failed to take` + warning in the controller log on each later pass, and no second command + until the controller restarts. - Runs only on a controller with both `services.hyperhive.deploy.swarm-controller.authBridgeUrl` and `services.hyperhive.deploy.swarm-controller.matrixHomeserverUrl` set. @@ -222,8 +226,10 @@ tuwunel has none. Promoting a user to homeserver admin and resetting a password both need an admin **sender**: `!admin …` messages into `#admins:`, and tuwunel only treats a message as a command when its sender is already an -admin. Only `@swarm` is a homeserver admin, so both are swarm-level -operations. +admin. Of the swarm's own accounts only `@swarm` is a homeserver admin +(operators are too, see +[Operators are homeserver admins](#operators-are-homeserver-admins)), so +both are swarm-level operations.
Store precedence for a hive's sender account diff --git a/swarm-authelia-bridge/src/main.rs b/swarm-authelia-bridge/src/main.rs index 8c892c18..1dde363f 100644 --- a/swarm-authelia-bridge/src/main.rs +++ b/swarm-authelia-bridge/src/main.rs @@ -150,11 +150,19 @@ async fn handle_request( Ok(caller) => caller, Err(resp) => return resp, }; - if let Err(refusal) = permit( - &req, - caller.as_deref(), - &state.config.operator_reader_client_id, - ) { + respond(&state.config, &state.write_lock, caller.as_deref(), req).await +} + +/// Everything after authentication: the per-request [`permit`] check, then +/// the operation. Takes the config and the write lock rather than +/// [`AppState`], whose HTTP client only [`authorize`] uses. +async fn respond( + config: &Config, + write_lock: &tokio::sync::Mutex<()>, + caller: Option<&str>, + req: BridgeRequest, +) -> Response { + if let Err(refusal) = permit(&req, caller, &config.operator_reader_client_id) { // The client id is not a credential; naming it is what lets an // operator see which client asked for something it may not have. tracing::warn!(client_id = ?caller, request = ?req, "refused a request scoped to another client"); @@ -162,10 +170,10 @@ async fn handle_request( } let (op, result) = match req { BridgeRequest::EnsureAgentIdentity { name } => { - ("ensure_identity", handle(&state, name).await) + ("ensure_identity", handle(config, write_lock, name).await) } - BridgeRequest::ListAgentIdentities => ("list_identities", list(&state)), - BridgeRequest::ListOperators => ("list_operators", list_operators(&state)), + BridgeRequest::ListAgentIdentities => ("list_identities", list(config)), + BridgeRequest::ListOperators => ("list_operators", list_operators(config)), }; match result { Ok(body) => (StatusCode::OK, Json(body)).into_response(), @@ -304,20 +312,23 @@ fn refuse(refusal: Refusal) -> Response { .into_response() } -async fn handle(state: &AppState, name: String) -> Result { +async fn handle( + cfg: &Config, + write_lock: &tokio::sync::Mutex<()>, + name: String, +) -> Result { store::validate_username(&name)?; // After the charset check and before anything touches the store: a // reserved name is refused on its own terms, not as a side effect of a // collision with whatever happens to be in the file today. store::reject_reserved_name(&name)?; - let cfg = &state.config; // Held across the whole load → insert → publish sequence, not just the // write — two concurrent `EnsureAgentIdentity` calls must not both read // the same on-disk snapshot before either publishes. See the field's // doc comment on `AppState::write_lock` for why this is reachable in // practice, not just theoretically. - let _write_guard = state.write_lock.lock().await; + let _write_guard = write_lock.lock().await; let mut user_store = store::load_store(&cfg.users_file)?; if user_store.users.contains_key(&name) { @@ -365,8 +376,8 @@ async fn handle(state: &AppState, name: String) -> Result { /// reader sees either the whole old file or the whole new one — and holding /// the lock would make a read wait behind a password mint, which is an /// external process. -fn list(state: &AppState) -> Result { - let user_store = store::load_store(&state.config.users_file)?; +fn list(config: &Config) -> Result { + let user_store = store::load_store(&config.users_file)?; Ok(BridgeResponse::Agents { agents: store::agent_names(&user_store) .into_iter() @@ -395,10 +406,10 @@ fn list(state: &AppState) -> Result { } /// The operator group's members. No write lock, for [`list`]'s reason. -fn list_operators(state: &AppState) -> Result { - let user_store = store::load_store(&state.config.users_file)?; +fn list_operators(config: &Config) -> Result { + let user_store = store::load_store(&config.users_file)?; Ok(BridgeResponse::Operators { - operators: store::group_members(&user_store, &state.config.operator_group), + operators: store::group_members(&user_store, &config.operator_group), }) } @@ -444,7 +455,9 @@ async fn generate_password(bin: &std::path::Path) -> Result { #[cfg(test)] mod tests { - use super::{BridgeRequest, Refusal, StatusCode, classify, introspect::Verdict, permit}; + use super::{ + BridgeRequest, Config, Refusal, StatusCode, classify, introspect::Verdict, permit, respond, + }; /// Fail-closed, asserted per variant rather than as one negation: /// splitting blame out of a single denial is exactly the change that @@ -559,4 +572,63 @@ mod tests { "the token is valid; a 401 would send the caller off to mint a new one" ); } + + /// A config whose users database holds one operator, `mara`. Every field + /// the request path does not read is left empty. + fn config_with_one_operator() -> Config { + let dir = std::env::temp_dir().join(format!( + "swarm-authelia-bridge-respond-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + std::fs::create_dir_all(&dir).unwrap(); + let users_file = dir.join("users.yml"); + std::fs::write( + &users_file, + "users:\n mara:\n displayname: mara\n password: \"$argon2id$x\"\n groups: [admins]\n", + ) + .unwrap(); + Config { + bind: String::new(), + users_file, + authelia_bin: std::path::PathBuf::new(), + introspection_url: String::new(), + client_id: String::new(), + client_secret: String::new(), + operator_reader_client_id: CONTROLLER.to_owned(), + operator_group: "admins".to_owned(), + } + } + + /// The request path, not only [`permit`]: a dispatch that skipped the + /// check would answer the hive below with the operator list. + #[tokio::test] + async fn the_request_path_answers_the_operator_list_to_the_controller_only() { + let config = config_with_one_operator(); + let lock = tokio::sync::Mutex::new(()); + for caller in [Some("hive-alpha"), None] { + let refused = respond(&config, &lock, caller, BridgeRequest::ListOperators).await; + assert_eq!(refused.status(), StatusCode::FORBIDDEN, "{caller:?}"); + } + + let answered = respond( + &config, + &lock, + Some(CONTROLLER), + BridgeRequest::ListOperators, + ) + .await; + assert_eq!(answered.status(), StatusCode::OK); + let body = axum::body::to_bytes(answered.into_body(), usize::MAX) + .await + .unwrap(); + assert_eq!( + serde_json::from_slice::(&body).unwrap(), + serde_json::json!({"status": "operators", "operators": ["mara"]}) + ); + let _ = std::fs::remove_dir_all(config.users_file.parent().unwrap()); + } } diff --git a/swarm-controller/src/matrix_account/operator_admin.rs b/swarm-controller/src/matrix_account/operator_admin.rs index 09e1b9f7..3bc7d06e 100644 --- a/swarm-controller/src/matrix_account/operator_admin.rs +++ b/swarm-controller/src/matrix_account/operator_admin.rs @@ -19,9 +19,11 @@ //! login so far) gets no command either: tuwunel would only refuse it, every //! pass. //! -//! The bot's reply is not read. A command that did not take shows up as the -//! same operator still missing from the room on the next pass, which sends it -//! again. +//! The bot's reply is not read, and a grant is sent **at most once per +//! operator per controller process**. An operator still missing from the room +//! on a later pass is logged as `make-user-admin … failed to take` and not sent +//! again until the controller restarts: the command stays in the room's +//! history, so one tuwunel ignores must not be repeated every pass. use std::collections::BTreeSet; use std::sync::Arc; @@ -61,11 +63,20 @@ pub struct Plan { pub grant: Vec<(String, String)>, /// Operators whose username maps to no legal localpart. pub unmappable: Vec, + /// `(authelia username, user id)` for each operator this process already + /// sent a grant for who is still not an admin. Not sent again. + pub unconfirmed: Vec<(String, String)>, } /// Decide which of `operators` need a grant on `server`, given the user ids -/// already joined to the admin room. -pub fn plan(operators: &[String], server: &str, admins: &BTreeSet) -> Plan { +/// already joined to the admin room and those this process already sent a +/// grant for. +pub fn plan( + operators: &[String], + server: &str, + admins: &BTreeSet, + sent: &BTreeSet, +) -> Plan { let mut plan = Plan::default(); for name in operators { let Some(localpart) = localpart(name) else { @@ -73,7 +84,12 @@ pub fn plan(operators: &[String], server: &str, admins: &BTreeSet) -> Pl continue; }; let user_id = format!("@{localpart}:{server}"); - if !admins.contains(&user_id) { + if admins.contains(&user_id) { + continue; + } + if sent.contains(&user_id) { + plan.unconfirmed.push((name.clone(), user_id)); + } else { plan.grant.push((name.clone(), user_id)); } } @@ -98,12 +114,13 @@ struct Outcome { } /// One pass: read the operator group, and send a grant for each operator -/// who has a matrix account and is not an admin. +/// who has a matrix account, is not an admin, and is not in `sent` — the user +/// ids this process already sent one for, which a sent grant joins. /// /// Fails as a whole when the group, the token, the server name, the admin /// room or its members cannot be read: with any of them missing there is no /// decision to make. -async fn pass(bridge: &AuthBridge, base: &str) -> Result { +async fn pass(bridge: &AuthBridge, base: &str, sent: &mut BTreeSet) -> Result { let operators = bridge .list_operators() .await @@ -130,7 +147,7 @@ async fn pass(bridge: &AuthBridge, base: &str) -> Result { .await?; let admins = homeserver::joined_members(&http, base, &token, &room).await?; - let plan = plan(&operators, server, &admins); + let plan = plan(&operators, server, &admins, sent); for name in &plan.unmappable { tracing::warn!( operator = %name, @@ -138,10 +155,19 @@ async fn pass(bridge: &AuthBridge, base: &str) -> Result { have an account to make admin; skipped" ); } + for (name, user_id) in &plan.unconfirmed { + tracing::warn!( + operator = %name, + %user_id, + "make-user-admin for an operator failed to take: sent earlier by this process, \ + still not in the admin room; not sent again until the controller restarts" + ); + } let mut outcome = Outcome::default(); for (name, user_id) in &plan.grant { match grant(&http, base, &token, &room, user_id).await { Ok(true) => { + sent.insert(user_id.clone()); tracing::info!( operator = %name, %user_id, @@ -198,10 +224,11 @@ async fn grant( /// is logged and retried on the next tick; it never stops the daemon. pub fn spawn(bridge: Arc, base: Arc) { tokio::spawn(async move { + let mut sent = BTreeSet::new(); let mut ticker = tokio::time::interval(RECONCILE_INTERVAL); loop { ticker.tick().await; - match pass(&bridge, &base).await { + match pass(&bridge, &base, &mut sent).await { Ok(outcome) if outcome.failed > 0 => tracing::warn!( granted = outcome.granted, failed = outcome.failed, @@ -245,6 +272,7 @@ mod tests { &names(&["mara", "vinzenz"]), "t.local", &ids(&["@mara:t.local", "@swarm:t.local"]), + &BTreeSet::new(), ); assert_eq!( plan.grant, @@ -255,7 +283,12 @@ mod tests { #[test] fn a_pass_with_every_operator_admin_sends_nothing() { - let plan = plan(&names(&["mara"]), "t.local", &ids(&["@mara:t.local"])); + let plan = plan( + &names(&["mara"]), + "t.local", + &ids(&["@mara:t.local"]), + &BTreeSet::new(), + ); assert_eq!(plan, Plan::default()); } @@ -263,20 +296,61 @@ mod tests { fn an_uppercase_username_maps_to_the_lowercased_account() { // tuwunel lowercases the claim, so `Mara`'s login is `@mara:`, and that // account being admin already is a match, not a second grant. - let plan = plan(&names(&["Mara"]), "t.local", &ids(&["@mara:t.local"])); + let plan = plan( + &names(&["Mara"]), + "t.local", + &ids(&["@mara:t.local"]), + &BTreeSet::new(), + ); assert_eq!(plan, Plan::default()); assert_eq!(localpart("Mara.B_c").as_deref(), Some("mara.b_c")); } #[test] fn a_name_tuwunel_would_refuse_is_reported_not_granted() { - let plan = plan(&names(&["bad name", "ok"]), "t.local", &BTreeSet::new()); + let plan = plan( + &names(&["bad name", "ok"]), + "t.local", + &BTreeSet::new(), + &BTreeSet::new(), + ); assert_eq!(plan.grant, [("ok".to_owned(), "@ok:t.local".to_owned())]); assert_eq!(plan.unmappable, ["bad name"]); assert_eq!(localpart(""), None); assert_eq!(localpart("é"), None); } + #[test] + fn a_grant_already_sent_by_this_process_is_not_sent_again() { + // The cap on the admin room: a command tuwunel ignored leaves the + // operator outside the room, and that must not produce a second send. + let plan = plan( + &names(&["mara", "vinzenz"]), + "t.local", + &BTreeSet::new(), + &ids(&["@mara:t.local"]), + ); + assert_eq!( + plan.grant, + [("vinzenz".to_owned(), "@vinzenz:t.local".to_owned())] + ); + assert_eq!( + plan.unconfirmed, + [("mara".to_owned(), "@mara:t.local".to_owned())] + ); + } + + #[test] + fn a_sent_grant_that_took_is_neither_sent_nor_reported() { + let plan = plan( + &names(&["mara"]), + "t.local", + &ids(&["@mara:t.local"]), + &ids(&["@mara:t.local"]), + ); + assert_eq!(plan, Plan::default()); + } + #[test] fn the_server_name_comes_from_the_full_user_id() { assert_eq!(server_name("@swarm:t.local"), Some("t.local")); diff --git a/swarm-matrix-client/src/lib.rs b/swarm-matrix-client/src/lib.rs index ccaf9fad..8b2ab6bf 100644 --- a/swarm-matrix-client/src/lib.rs +++ b/swarm-matrix-client/src/lib.rs @@ -244,16 +244,21 @@ pub async fn joined_members( ) .await .with_context(|| format!("GET /rooms/{room_id}/joined_members"))?; + joined_verdict(status, &json).with_context(|| format!("the members of {room_id}")) +} + +/// [`joined_members`]'s reading of a response, apart from the IO. +fn joined_verdict( + status: reqwest::StatusCode, + json: &serde_json::Value, +) -> Result> { if !status.is_success() { - bail!( - "the homeserver refused to list {room_id}'s members: {}", - why(status, &json) - ); + bail!("the homeserver refused to list them: {}", why(status, json)); } json["joined"] .as_object() .map(|joined| joined.keys().cloned().collect()) - .with_context(|| format!("the homeserver's member list for {room_id} carried no `joined`")) + .context("the homeserver's member list carried no `joined`") } /// Whether `user_id` has an account on this homeserver. @@ -277,16 +282,22 @@ pub async fn user_exists( ) .await .with_context(|| format!("GET /profile/{user_id}"))?; + profile_verdict(status, &json).with_context(|| format!("{user_id}'s profile")) +} + +/// [`user_exists`]'s reading of a response, apart from the IO. +/// +/// Only `M_NOT_FOUND` means "no account". Any other refusal says nothing +/// about the account, so it is an error rather than a `false` a caller would +/// act on. +fn profile_verdict(status: reqwest::StatusCode, json: &serde_json::Value) -> Result { if status.is_success() { return Ok(true); } - if errcode(&json) == Some("M_NOT_FOUND") { + if errcode(json) == Some("M_NOT_FOUND") { return Ok(false); } - bail!( - "the homeserver refused {user_id}'s profile: {}", - why(status, &json) - ) + bail!("the homeserver refused: {}", why(status, json)) } /// Send `body` to `room_id` as an `m.text` message, and return its event id. @@ -539,6 +550,62 @@ mod tests { } } + #[test] + fn a_profile_answer_means_the_account_exists() { + let json = serde_json::json!({ "displayname": "mara" }); + assert!(profile_verdict(reqwest::StatusCode::OK, &json).expect("a verdict")); + } + + #[test] + fn not_found_is_no_account_and_not_an_error() { + let json = + serde_json::json!({ "errcode": "M_NOT_FOUND", "error": "Profile was not found." }); + assert!(!profile_verdict(reqwest::StatusCode::NOT_FOUND, &json).expect("a verdict")); + } + + #[test] + fn any_other_profile_refusal_is_an_error_and_not_no_account() { + // The control for the arm above: a 403 says nothing about whether the + // account exists, and reading it as "no account" would skip a grant. + for (status, json) in [ + ( + reqwest::StatusCode::FORBIDDEN, + serde_json::json!({ "errcode": "M_FORBIDDEN" }), + ), + (reqwest::StatusCode::NOT_FOUND, serde_json::json!({})), + (reqwest::StatusCode::BAD_GATEWAY, serde_json::json!({})), + ] { + assert!(profile_verdict(status, &json).is_err(), "{status} {json}"); + } + } + + #[test] + fn the_joined_members_are_the_keys_of_joined() { + let json = serde_json::json!({ "joined": { + "@swarm:t.local": { "display_name": "swarm" }, + "@mara:t.local": {}, + }}); + let members = joined_verdict(reqwest::StatusCode::OK, &json).expect("a member list"); + assert_eq!( + members.into_iter().collect::>(), + ["@mara:t.local", "@swarm:t.local"] + ); + } + + #[test] + fn a_member_list_without_joined_or_a_refusal_is_an_error() { + // Never an empty set: that would read as "nobody is admin yet" and + // send a grant for every operator. + assert!(joined_verdict(reqwest::StatusCode::OK, &serde_json::json!({})).is_err()); + assert!( + joined_verdict( + reqwest::StatusCode::FORBIDDEN, + &serde_json::json!({ "errcode": "M_FORBIDDEN" }) + ) + .is_err() + ); + } + #[test] fn a_path_segment_escapes_what_would_end_or_split_the_path() { assert_eq!(path_segment("#admins:t.local"), "%23admins%3At.local");