diff --git a/crates/adapters/http-axum/src/extractors/mod.rs b/crates/adapters/http-axum/src/extractors/mod.rs index 3d01efce..38d4ec45 100644 --- a/crates/adapters/http-axum/src/extractors/mod.rs +++ b/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 FromRequestParts 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)) diff --git a/crates/adapters/sqlite/src/repositories/users/mod.rs b/crates/adapters/sqlite/src/repositories/users/mod.rs index 3ffd2d10..ef91e249 100644 --- a/crates/adapters/sqlite/src/repositories/users/mod.rs +++ b/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 = ?", diff --git a/crates/adapters/sqlite/tests/repositories/user_repository_test.rs b/crates/adapters/sqlite/tests/repositories/user_repository_test.rs index 5c86355e..9798efc9 100644 --- a/crates/adapters/sqlite/tests/repositories/user_repository_test.rs +++ b/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" + ); +} diff --git a/crates/application/tests/users/request_account_deletion_test.rs b/crates/application/tests/users/request_account_deletion_test.rs index 3bb78f94..456971fc 100644 --- a/crates/application/tests/users/request_account_deletion_test.rs +++ b/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); + +#[async_trait] +impl UserRepositoryPort for StaleAdminCount { + async fn find_by_email(&self, e: &Email) -> Result, DomainError> { + self.0.find_by_email(e).await + } + async fn find_by_username(&self, u: &Username) -> Result, 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, DomainError> { + self.0.find_by_id(id).await + } + async fn list_with_stats(&self) -> Result, 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, DomainError> { + self.0.find_tombstoned_by_id(id).await + } + async fn is_username_retired(&self, u: &Username) -> Result { + self.0.is_username_retired(u).await + } + async fn count_active_admins(&self) -> Result { + 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:?}"); +} diff --git a/crates/domain/src/testing/fakes/store_user.rs b/crates/domain/src/testing/fakes/store_user.rs index 28c222d6..4e59b0c4 100644 --- a/crates/domain/src/testing/fakes/store_user.rs +++ b/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()