operator admin sync: cap the grant at one send, split out response verdicts

The admin-room command stays in #admins' history, so a grant tuwunel
ignores must not be re-sent every five minutes. The controller now
sends `make-user-admin` at most once per operator per process; an
operator still missing from the room on a later pass is logged as
"make-user-admin for an operator failed to take" and left alone until
the controller restarts. The cap lives in the pure `plan()`.

swarm-matrix-client's `user_exists` and `joined_members` read their
responses through pure `profile_verdict` / `joined_verdict`, the shape
`whoami` / `whoami_verdict` already has, with every branch tested: only
M_NOT_FOUND is "no account", and a 403 or a list without `joined` is an
error rather than an answer.

The bridge's handling after authentication moves into `respond()`,
which takes the config and write lock instead of the whole AppState, so
a test can drive ListOperators through the real request path and see
the 403 without an HTTP client or an authelia.

docs/integrations/matrix.md no longer says @swarm is the only homeserver
admin.

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:<server_name>`, 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.
 
 <details><summary>Store precedence for a hive's sender account</summary>
 

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<BridgeResponse> {
+async fn handle(
+    cfg: &Config,
+    write_lock: &tokio::sync::Mutex<()>,
+    name: String,
+) -> Result<BridgeResponse> {
     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<BridgeResponse> {
 /// 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<BridgeResponse> {
-    let user_store = store::load_store(&state.config.users_file)?;
+fn list(config: &Config) -> Result<BridgeResponse> {
+    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<BridgeResponse> {
 }
 
 /// The operator group's members. No write lock, for [`list`]'s reason.
-fn list_operators(state: &AppState) -> Result<BridgeResponse> {
-    let user_store = store::load_store(&state.config.users_file)?;
+fn list_operators(config: &Config) -> Result<BridgeResponse> {
+    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<String> {
 
 #[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::<serde_json::Value>(&body).unwrap(),
+            serde_json::json!({"status": "operators", "operators": ["mara"]})
+        );
+        let _ = std::fs::remove_dir_all(config.users_file.parent().unwrap());
+    }
 }

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<String>,
+    /// `(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<String>) -> 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<String>,
+    sent: &BTreeSet<String>,
+) -> 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<String>) -> 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<Outcome> {
+async fn pass(bridge: &AuthBridge, base: &str, sent: &mut BTreeSet<String>) -> Result<Outcome> {
     let operators = bridge
         .list_operators()
         .await
@@ -130,7 +147,7 @@ async fn pass(bridge: &AuthBridge, base: &str) -> Result<Outcome> {
     .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<Outcome> {
              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<AuthBridge>, base: Arc<str>) {
     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"));

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<std::collections::BTreeSet<String>> {
     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<bool> {
     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::<Vec<_>>(),
+            ["@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");