From 23a87a02d2aba58821f420e9af2bb3adb0746599 Mon Sep 17 00:00:00 2001
From: Andrew Ferrazzutti <andrewf@element.io>
Date: Mon, 14 Jul 2025 01:02:51 -0400
Subject: [PATCH] Negate erase option and make optional

This makes it more intuitive for an empty request body to be equivalent
to the option being set to false.
---
 .../handlers/src/admin/v1/users/deactivate.rs | 66 +++++--------------
 docs/api/spec.json                            |  8 +--
 2 files changed, 18 insertions(+), 56 deletions(-)

diff --git a/crates/handlers/src/admin/v1/users/deactivate.rs b/crates/handlers/src/admin/v1/users/deactivate.rs
index f268d9b15..076c06914 100644
--- a/crates/handlers/src/admin/v1/users/deactivate.rs
+++ b/crates/handlers/src/admin/v1/users/deactivate.rs
@@ -52,17 +52,13 @@ impl IntoResponse for RouteError {
 }
 
 /// # JSON payload for the `POST /api/admin/v1/users/:id/deactivate` endpoint
-#[derive(Deserialize, JsonSchema)]
+#[derive(Default, Deserialize, JsonSchema)]
 #[serde(rename = "DeactivateUserRequest")]
 pub struct Request {
-    /// Whether the user should be GDPR-erased from the homeserver.
-    erase: bool,
-}
-
-impl Default for Request {
-    fn default() -> Self {
-        Self { erase: true }
-    }
+    /// Whether to skip requesting the homeserver to GDPR-erase the user upon
+    /// deactivation.
+    #[serde(default)]
+    skip_erase: bool,
 }
 
 pub fn doc(mut operation: TransformOperation) -> TransformOperation {
@@ -120,7 +116,7 @@ pub async fn handler(
         .schedule_job(
             &mut rng,
             &clock,
-            DeactivateUserJob::new(&user, params.erase),
+            DeactivateUserJob::new(&user, !params.skip_erase),
         )
         .await?;
 
@@ -142,7 +138,7 @@ mod tests {
 
     use crate::test_utils::{RequestBuilderExt, ResponseExt, TestState, setup};
 
-    async fn test_deactivate_user_helper(pool: PgPool, erase: Option<bool>) {
+    async fn test_deactivate_user_helper(pool: PgPool, skip_erase: Option<bool>) {
         setup();
         let mut state = TestState::from_pool(pool.clone()).await.unwrap();
         let token = state.token_with_scope("urn:mas:admin").await;
@@ -157,10 +153,10 @@ mod tests {
 
         let request =
             Request::post(format!("/api/admin/v1/users/{}/deactivate", user.id)).bearer(&token);
-        let request = match erase {
+        let request = match skip_erase {
             None => request.empty(),
-            Some(erase) => request.json(serde_json::json!({
-                "erase": erase,
+            Some(skip_erase) => request.json(serde_json::json!({
+                "skip_erase": skip_erase,
             })),
         };
         let response = state.request(request).await;
@@ -182,7 +178,10 @@ mod tests {
         .await
         .expect("Deactivation job to be scheduled");
         assert_eq!(job["user_id"], serde_json::json!(user.id));
-        assert_eq!(job["hs_erase"], serde_json::json!(erase.unwrap_or(true)));
+        assert_eq!(
+            job["hs_erase"],
+            serde_json::json!(!skip_erase.unwrap_or(false))
+        );
 
         // Make sure to run the jobs in the queue
         state.run_jobs_in_queue().await;
@@ -223,45 +222,10 @@ mod tests {
     }
 
     #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")]
-    async fn test_deactivate_user_with_explicit_erase(pool: PgPool) {
+    async fn test_deactivate_user_skip_erase(pool: PgPool) {
         test_deactivate_user_helper(pool, Option::Some(true)).await;
     }
 
-    #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")]
-    async fn test_deactivate_user_without_erase(pool: PgPool) {
-        test_deactivate_user_helper(pool, Option::Some(false)).await;
-    }
-
-    #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")]
-    async fn test_deactivate_user_missing_erase(pool: PgPool) {
-        setup();
-        let mut state = TestState::from_pool(pool.clone()).await.unwrap();
-        let token = state.token_with_scope("urn:mas:admin").await;
-
-        let mut repo = state.repository().await.unwrap();
-        let user = repo
-            .user()
-            .add(&mut state.rng(), &state.clock, "alice".to_owned())
-            .await
-            .unwrap();
-        repo.save().await.unwrap();
-
-        let request = Request::post(format!("/api/admin/v1/users/{}/deactivate", user.id))
-            .bearer(&token)
-            .json(serde_json::json!({}));
-        let response = state.request(request).await;
-        response.assert_status(StatusCode::UNPROCESSABLE_ENTITY);
-
-        // It should have not scheduled a deactivation job for the user
-        let count: i64 = sqlx::query_scalar(
-            "SELECT COUNT(1) FROM queue_jobs WHERE queue_name = 'deactivate-user'",
-        )
-        .fetch_one(&pool)
-        .await
-        .unwrap();
-        assert_eq!(count, 0);
-    }
-
     #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")]
     async fn test_deactivate_locked_user(pool: PgPool) {
         setup();
diff --git a/docs/api/spec.json b/docs/api/spec.json
index f2a04ea9b..5cb1beba1 100644
--- a/docs/api/spec.json
+++ b/docs/api/spec.json
@@ -3884,12 +3884,10 @@
       "DeactivateUserRequest": {
         "title": "JSON payload for the `POST /api/admin/v1/users/:id/deactivate` endpoint",
         "type": "object",
-        "required": [
-          "erase"
-        ],
         "properties": {
-          "erase": {
-            "description": "Whether the user should be GDPR-erased from the homeserver.",
+          "skip_erase": {
+            "description": "Whether to skip requesting the homeserver to GDPR-erase the user upon deactivation.",
+            "default": false,
             "type": "boolean"
           }
         }