mark_deleted: atomic last-admin guard; review minors

crates/adapters/http-axum/src/extractors/mod.rs

@@ -8,13 +8,13 @@ use axum::{
 };
 use domain::{errors::DomainError, value_objects::UserId};
 
+use crate::{errors::ApiError, state::AppState};
+
 async fn ensure_active(state: &AppState, user_id: &UserId) -> Result<(), DomainError> {
     application::users::ensure_active::execute(&state.app_ctx.deps.users.ensure_active, user_id)
         .await
 }
 
-use crate::{errors::ApiError, state::AppState};
-
 pub struct AuthenticatedUser(pub UserId);
 
 impl<S> FromRequestParts<S> for AuthenticatedUser
@@ -82,8 +82,16 @@ where
             .await
             .ok();
         let user_id = match user_id {
-            Some(id) if ensure_active(&app_state, &id).await.is_ok() => Some(id),
-            _ => None,
+            Some(id) => match ensure_active(&app_state, &id).await {
+                Ok(()) => Some(id),
+                Err(e) => {
+                    if e.is_transient() {
+                        tracing::warn!("ensure_active failed, treating as anonymous: {e:?}");
+                    }
+                    None
+                }
+            },
+            None => None,
         };
         Ok(OptionalCookieUser(user_id))
     }
@@ -138,6 +146,7 @@ where
         )
         .await
         .map_err(ApiError)?
+        // `None` only if the account vanished between `ensure_active` and here.
         .ok_or_else(|| ApiError(DomainError::NotFound("user not found".into())))?;
         if is_admin {
             Ok(AdminApiUser(user_id))
@@ -166,6 +175,7 @@ where
         )
         .await
         .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR.into_response())?
+        // `None` only if the account vanished between `ensure_active` and here.
         .ok_or_else(|| StatusCode::UNAUTHORIZED.into_response())?;
         if is_admin {
             Ok(AdminUser(user_id))

crates/adapters/sqlite/src/repositories/users/mod.rs

@@ -169,15 +169,32 @@ impl UserRepository for SqliteUserRepository {
             .begin()
             .await
             .map_err(crate::shared::map_sqlx_error)?;
-        let stamped =
-            sqlx::query("UPDATE users SET deleted_at = ? WHERE id = ? AND deleted_at IS NULL")
-                .bind(Utc::now().format("%Y-%m-%dT%H:%M:%SZ").to_string())
-                .bind(&id_str)
-                .execute(&mut *tx)
-                .await
-                .map_err(crate::shared::map_sqlx_error)?;
+        // The last-admin guard lives in this single statement, so two admins
+        // deleting each other concurrently cannot both pass it: the write lock
+        // serialises them and the loser sees the winner's tombstone.
+        let stamped = sqlx::query(
+            "UPDATE users SET deleted_at = ?1 WHERE id = ?2 AND deleted_at IS NULL
+             AND (role != 'admin' OR EXISTS (
+                 SELECT 1 FROM users WHERE role = 'admin' AND deleted_at IS NULL AND id != ?2))",
+        )
+        .bind(Utc::now().format("%Y-%m-%dT%H:%M:%SZ").to_string())
+        .bind(&id_str)
+        .execute(&mut *tx)
+        .await
+        .map_err(crate::shared::map_sqlx_error)?;
         if stamped.rows_affected() == 0 {
-            return Err(DomainError::NotFound("User not found".into()));
+            let live: i64 = sqlx::query_scalar(
+                "SELECT COUNT(*) FROM users WHERE id = ? AND deleted_at IS NULL",
+            )
+            .bind(&id_str)
+            .fetch_one(&mut *tx)
+            .await
+            .map_err(crate::shared::map_sqlx_error)?;
+            return Err(if live == 0 {
+                DomainError::NotFound("User not found".into())
+            } else {
+                DomainError::Conflict("the last active admin cannot be deleted".into())
+            });
         }
         sqlx::query(
             "INSERT OR IGNORE INTO retired_usernames (username) SELECT username FROM users WHERE id = ?",

crates/adapters/sqlite/tests/repositories/user_repository_test.rs

@@ -114,13 +114,19 @@ async fn add_session(pool: &SqlitePool, user_id: &UserId, token: &str) {
 
 #[tokio::test]
 async fn marking_a_user_deleted_hides_them_from_every_lookup() {
-    let (_, repo) = setup().await;
+    let (pool, repo) = setup().await;
     let admin =
         domain::testing::test_user_with_role(UserId::generate(), domain::models::UserRole::Admin);
     repo.save(&admin).await.unwrap();
     let other = test_user("other");
     repo.save(&other).await.unwrap();
-    assert_eq!(repo.count_active_admins().await.unwrap(), 1);
+    // A second admin, so the first is not the last one standing.
+    sqlx::query("UPDATE users SET role = 'admin' WHERE id = ?")
+        .bind(other.id().value().to_string())
+        .execute(&pool)
+        .await
+        .unwrap();
+    assert_eq!(repo.count_active_admins().await.unwrap(), 2);
 
     repo.mark_deleted(admin.id(), &[deleted_event(&admin)])
         .await
@@ -138,7 +144,7 @@ async fn marking_a_user_deleted_hides_them_from_every_lookup() {
     let emails: Vec<_> = listed.iter().map(|u| u.email()).collect();
     assert!(emails.contains(&other.email().value()));
     assert!(!emails.contains(&admin.email().value()));
-    assert_eq!(repo.count_active_admins().await.unwrap(), 0);
+    assert_eq!(repo.count_active_admins().await.unwrap(), 1);
     // The purge can still read the tombstone; a live user is not one.
     assert!(
         repo.find_tombstoned_by_id(admin.id())
@@ -214,3 +220,58 @@ async fn retired_names_match_case_insensitively() {
     assert!(repo.is_username_retired(&lower).await.unwrap());
     assert!(repo.is_username_retired(&upper).await.unwrap());
 }
+
+#[tokio::test]
+async fn marking_the_last_admin_deleted_is_refused_atomically() {
+    let dir = tempfile::tempdir().unwrap();
+    let opts = sqlx::sqlite::SqliteConnectOptions::new()
+        .filename(dir.path().join("u.db"))
+        .create_if_missing(true)
+        .journal_mode(sqlx::sqlite::SqliteJournalMode::Wal)
+        .busy_timeout(std::time::Duration::from_secs(5));
+    let pool = sqlx::sqlite::SqlitePoolOptions::new()
+        .max_connections(4)
+        .connect_with(opts)
+        .await
+        .unwrap();
+    sqlite::migrate(&pool).await.unwrap();
+    let repo = std::sync::Arc::new(SqliteUserRepository::new(pool.clone()));
+    let a = test_user("admin_a");
+    let b = test_user("admin_b");
+    repo.save(&a).await.unwrap();
+    repo.save(&b).await.unwrap();
+    sqlx::query("UPDATE users SET role = 'admin' WHERE id IN (?, ?)")
+        .bind(a.id().value().to_string())
+        .bind(b.id().value().to_string())
+        .execute(&pool)
+        .await
+        .unwrap();
+    assert_eq!(repo.count_active_admins().await.unwrap(), 2);
+
+    let (ra, rb) = (repo.clone(), repo.clone());
+    let (ida, idb) = (a.id().clone(), b.id().clone());
+    let (ea, eb) = (deleted_event(&a), deleted_event(&b));
+    let (x, y) = tokio::join!(
+        tokio::spawn(async move { ra.mark_deleted(&ida, &[ea]).await }),
+        tokio::spawn(async move { rb.mark_deleted(&idb, &[eb]).await }),
+    );
+    let results = [x.unwrap(), y.unwrap()];
+
+    assert_eq!(
+        results.iter().filter(|r| r.is_ok()).count(),
+        1,
+        "{results:?}"
+    );
+    assert!(
+        results
+            .iter()
+            .any(|r| matches!(r, Err(domain::errors::DomainError::Conflict(_)))),
+        "{results:?}"
+    );
+    assert_eq!(repo.count_active_admins().await.unwrap(), 1);
+    assert_eq!(
+        count(&pool, "event_queue").await,
+        1,
+        "loser enqueued nothing"
+    );
+}

crates/application/tests/users/request_account_deletion_test.rs

@@ -16,6 +16,54 @@ use application::{
 
 use crate::support::{TestContextBuilder, register_command};
 
+use async_trait::async_trait;
+use domain::models::{User, UserSummary};
+use domain::ports::UserRepository as UserRepositoryPort;
+use domain::value_objects::Username;
+
+/// Delegates to a store but reports a stale admin count, as a concurrent
+/// deletion would leave the use case's pre-check.
+struct StaleAdminCount(Arc<InMemoryStore>);
+
+#[async_trait]
+impl UserRepositoryPort for StaleAdminCount {
+    async fn find_by_email(&self, e: &Email) -> Result<Option<User>, DomainError> {
+        self.0.find_by_email(e).await
+    }
+    async fn find_by_username(&self, u: &Username) -> Result<Option<User>, DomainError> {
+        self.0.find_by_username(u).await
+    }
+    async fn save(&self, u: &User) -> Result<(), DomainError> {
+        UserRepositoryPort::save(&*self.0, u).await
+    }
+    async fn find_by_id(&self, id: &UserId) -> Result<Option<User>, DomainError> {
+        self.0.find_by_id(id).await
+    }
+    async fn list_with_stats(&self) -> Result<Vec<UserSummary>, DomainError> {
+        self.0.list_with_stats().await
+    }
+    async fn update_profile(
+        &self,
+        id: &UserId,
+        p: &domain::models::UserProfile,
+        ev: &[DomainEvent],
+    ) -> Result<(), DomainError> {
+        self.0.update_profile(id, p, ev).await
+    }
+    async fn mark_deleted(&self, id: &UserId, ev: &[DomainEvent]) -> Result<(), DomainError> {
+        self.0.mark_deleted(id, ev).await
+    }
+    async fn find_tombstoned_by_id(&self, id: &UserId) -> Result<Option<User>, DomainError> {
+        self.0.find_tombstoned_by_id(id).await
+    }
+    async fn is_username_retired(&self, u: &Username) -> Result<bool, DomainError> {
+        self.0.is_username_retired(u).await
+    }
+    async fn count_active_admins(&self) -> Result<u64, DomainError> {
+        Ok(2)
+    }
+}
+
 const PASSWORD: &str = "password123";
 
 struct Fixture {
@@ -183,3 +231,53 @@ async fn an_already_deleted_target_is_not_found() {
 
     assert!(matches!(err, DomainError::NotFound(_)), "got {err:?}");
 }
+
+#[tokio::test]
+async fn a_conflict_from_the_repository_is_propagated() {
+    let f = fixture();
+    let admin = f.user("root", UserRole::Admin).await;
+    let deps = RequestAccountDeletionDeps {
+        user: Arc::new(StaleAdminCount(Arc::clone(&f.store))),
+        password_hasher: f.deps.password_hasher.clone(),
+    };
+
+    let err = request_account_deletion::execute(&deps, admin.clone(), admin.clone(), PASSWORD)
+        .await
+        .unwrap_err();
+
+    assert!(matches!(err, DomainError::Conflict(_)), "got {err:?}");
+    assert!(f.is_live(&admin).await);
+}
+
+#[tokio::test]
+async fn an_unknown_or_deleted_actor_is_unauthorized() {
+    let f = fixture();
+    let admin = f.user("root", UserRole::Admin).await;
+    let bob = f.user("bob", UserRole::Standard).await;
+
+    let err = request_account_deletion::execute(&f.deps, UserId::generate(), bob.clone(), PASSWORD)
+        .await
+        .unwrap_err();
+    assert!(matches!(err, DomainError::Unauthorized(_)), "got {err:?}");
+
+    request_account_deletion::execute(&f.deps, admin.clone(), bob.clone(), PASSWORD)
+        .await
+        .unwrap();
+    let err = request_account_deletion::execute(&f.deps, bob.clone(), bob, PASSWORD)
+        .await
+        .unwrap_err();
+    assert!(matches!(err, DomainError::Unauthorized(_)), "got {err:?}");
+}
+
+#[tokio::test]
+async fn a_non_admin_gets_forbidden_before_any_password_check() {
+    let f = fixture();
+    let alice = f.user("alice", UserRole::Standard).await;
+    let bob = f.user("bob", UserRole::Standard).await;
+
+    let err = request_account_deletion::execute(&f.deps, alice, bob, "wrong")
+        .await
+        .unwrap_err();
+
+    assert!(matches!(err, DomainError::Forbidden(_)), "got {err:?}");
+}

crates/domain/src/testing/fakes/store_user.rs

@@ -83,6 +83,17 @@ impl UserRepository for InMemoryStore {
         let Some(user) = users.get(&id).filter(|_| !gone.contains(&id)) else {
             return Err(DomainError::NotFound("User not found".into()));
         };
+        if matches!(user.role(), crate::models::UserRole::Admin)
+            && !users.values().any(|u| {
+                matches!(u.role(), crate::models::UserRole::Admin)
+                    && u.id().value() != id
+                    && !gone.contains(&u.id().value())
+            })
+        {
+            return Err(DomainError::Conflict(
+                "the last active admin cannot be deleted".into(),
+            ));
+        }
         self.retired_usernames
             .write()
             .unwrap()