swarm-controller: refuse linking over an existing account

The matrix, forge and github link routes wrote their credential
unconditionally, so linking a name that was already linked replaced the
working account. For matrix that lost the device the agent's crypto store
belongs to (#4838).

Each route now reads the account's store path first and answers 409,
naming the existing account, when something is stored there. Nothing is
written. Replacing an account takes the delete from #4899, then a link.

The matrix route checks before password mode's login, so a refused link
mints no new device at the homeserver.

The check is a read then a write, not an atomic step; two concurrent
links to one name can still both pass it.

Closes #4856

swarm-controller/src/linked_accounts.rs

@@ -1,6 +1,9 @@
 //! The accounts linked to one agent, as kind, name and host: what the swarm
 //! UI's agent panel lists, and deletes.
 //!
+//! [`link`] is the write the three link routes share: it refuses a path that
+//! already holds an account, so replacing one takes a delete first.
+//!
 //! Read from the paths [`crate::matrix_account`], [`crate::forge_account`] and
 //! [`crate::github_account`] write, plus the agent's own `main` matrix account,
 //! which `matrix_account::agent_token` mints into the same directory. Each
@@ -57,8 +60,8 @@ pub struct LinkedAccount {
     reserved: bool,
 }
 
-/// The store calls a listing or a delete makes, so a test can stand in for the
-/// store.
+/// The store calls a listing, a link or a delete makes, so a test can stand in
+/// for the store.
 pub(crate) trait AccountStore {
     /// As [`SecretStore::list`].
     fn list(&self, dir: &str) -> impl Future<Output = Result<Vec<String>, Error>> + Send;
@@ -69,6 +72,13 @@ pub(crate) trait AccountStore {
         path: &str,
     ) -> impl Future<Output = Result<Option<T>, Error>> + Send;
 
+    /// As [`SecretStore::write`].
+    fn write<T: Serialize + Sync>(
+        &self,
+        path: &str,
+        value: &T,
+    ) -> impl Future<Output = Result<(), Error>> + Send;
+
     /// As [`SecretStore::delete_all_versions`].
     fn delete_all_versions(&self, path: &str) -> impl Future<Output = Result<(), Error>> + Send;
 }
@@ -85,6 +95,10 @@ impl AccountStore for SecretStore {
         SecretStore::read_optional(self, path).await
     }
 
+    async fn write<T: Serialize + Sync>(&self, path: &str, value: &T) -> Result<(), Error> {
+        SecretStore::write(self, path, value).await
+    }
+
     async fn delete_all_versions(&self, path: &str) -> Result<(), Error> {
         SecretStore::delete_all_versions(self, path).await
     }
@@ -189,6 +203,54 @@ pub async fn get_linked_accounts(
     Ok(Json(accounts))
 }
 
+/// Why an account was not linked.
+#[derive(Debug)]
+pub(crate) enum Linking {
+    /// An account is stored at the path already. Nothing was written.
+    Exists,
+    /// The store refused or could not be reached.
+    Store(Error),
+}
+
+impl Linking {
+    /// `existing` names the stored account, as in
+    /// `agent janet already has matrix account "work"`.
+    pub(crate) fn problem(&self, existing: &str) -> problem_details::ProblemDetails {
+        match self {
+            Self::Exists => error_problem(
+                StatusCode::CONFLICT,
+                &format!("{existing} linked; delete it first"),
+            ),
+            Self::Store(e) => error_problem(StatusCode::INTERNAL_SERVER_ERROR, &e.to_string()),
+        }
+    }
+}
+
+/// [`Linking::Exists`] when an account is stored at `path`.
+pub(crate) async fn refuse_linked<T: DeserializeOwned + Send>(
+    store: &impl AccountStore,
+    path: &str,
+) -> Result<(), Linking> {
+    match store.read_optional::<T>(path).await {
+        Ok(None) => Ok(()),
+        Ok(Some(_)) => Err(Linking::Exists),
+        Err(e) => Err(Linking::Store(e)),
+    }
+}
+
+/// Write `value` at `path`, unless an account is stored there already.
+///
+/// A read then a write, not one atomic step: two links racing for one path can
+/// both pass the check, and the later write wins.
+pub(crate) async fn link<T: Serialize + DeserializeOwned + Send + Sync>(
+    store: &impl AccountStore,
+    path: &str,
+    value: &T,
+) -> Result<(), Linking> {
+    refuse_linked::<T>(store, path).await?;
+    store.write(path, value).await.map_err(Linking::Store)
+}
+
 /// Why an account was not deleted.
 #[derive(Debug)]
 pub(crate) enum Removal {
@@ -431,10 +493,11 @@ pub async fn delete_github_account(
 }
 
 #[cfg(test)]
-mod tests {
+pub(crate) mod tests {
     use std::collections::BTreeMap;
     use std::sync::Mutex;
 
+    use serde::Serialize;
     use serde::de::DeserializeOwned;
     use serde_json::{Value, json};
     use swarm_secret_client::Error;
@@ -447,19 +510,29 @@ mod tests {
 
     /// Objects by path. A list answers the next segment of every path under
     /// the directory, with a trailing `/` when it goes deeper, as the store
-    /// does. A delete is recorded in `deleted` and leaves `objects` as it is.
+    /// does. A write replaces the object and is recorded in `written`; a
+    /// delete is recorded in `deleted` and leaves `objects` as it is.
     #[derive(Default)]
-    struct FakeStore {
-        objects: BTreeMap<String, Value>,
+    pub(crate) struct FakeStore {
+        objects: Mutex<BTreeMap<String, Value>>,
         denied: bool,
+        written: Mutex<Vec<String>>,
         deleted: Mutex<Vec<String>>,
     }
 
     impl FakeStore {
-        fn with(mut self, path: &str, object: Value) -> Self {
-            self.objects.insert(path.to_owned(), object);
+        fn with(self, path: &str, object: Value) -> Self {
+            self.objects
+                .lock()
+                .expect("not poisoned")
+                .insert(path.to_owned(), object);
             self
         }
+
+        /// The paths written, in order.
+        pub(crate) fn written(&self) -> Vec<String> {
+            self.written.lock().expect("not poisoned").clone()
+        }
     }
 
     impl AccountStore for FakeStore {
@@ -470,6 +543,8 @@ mod tests {
             let prefix = format!("{dir}/");
             let mut keys: Vec<String> = self
                 .objects
+                .lock()
+                .expect("not poisoned")
                 .keys()
                 .filter_map(|p| p.strip_prefix(&prefix))
                 .map(|rest| match rest.split_once('/') {
@@ -490,10 +565,28 @@ mod tests {
             }
             Ok(self
                 .objects
+                .lock()
+                .expect("not poisoned")
                 .get(path)
                 .map(|v| serde_json::from_value(v.clone()).expect("fixture decodes")))
         }
 
+        async fn write<T: Serialize + Sync>(&self, path: &str, value: &T) -> Result<(), Error> {
+            if self.denied {
+                return Err(Error::MissingEnv("BAO_ADDR"));
+            }
+            let value = serde_json::to_value(value).expect("serialises");
+            self.objects
+                .lock()
+                .expect("not poisoned")
+                .insert(path.to_owned(), value);
+            self.written
+                .lock()
+                .expect("not poisoned")
+                .push(path.to_owned());
+            Ok(())
+        }
+
         async fn delete_all_versions(&self, path: &str) -> Result<(), Error> {
             if self.denied {
                 return Err(Error::MissingEnv("BAO_ADDR"));