Decouple (un)locking from (re/de)activation

Unify the admin API, CLI, and GraphQL API in not having the unlock
command also reactivate, or the deactivate command also lock.

Still let the unlock command of the CLI and GraphQL API to also
reactivate the target user, albeit as a non-default option.

Author: AndrewFerr

crates/cli/src/commands/manage.rs

@@ -149,6 +149,10 @@ enum Subcommand {
     UnlockUser {
         /// User to unlock
         username: String,
+
+        /// Whether to reactivate the user if it had been deactivated
+        #[arg(long)]
+        reactivate: bool,
     },
 
     /// Register a user
@@ -527,8 +531,12 @@ impl Options {
                 Ok(ExitCode::SUCCESS)
             }
 
-            SC::UnlockUser { username } => {
-                let _span = info_span!("cli.manage.lock_user", user.username = username).entered();
+            SC::UnlockUser {
+                username,
+                reactivate,
+            } => {
+                let _span =
+                    info_span!("cli.manage.unlock_user", user.username = username).entered();
                 let config = DatabaseConfig::extract_or_default(figment)?;
                 let mut conn = database_connection_from_config(&config).await?;
                 let txn = conn.begin().await?;
@@ -540,10 +548,14 @@ impl Options {
                     .await?
                     .context("User not found")?;
 
-                warn!(%user.id, "User scheduling user reactivation");
-                repo.queue_job()
-                    .schedule_job(&mut rng, &clock, ReactivateUserJob::new(&user))
-                    .await?;
+                if reactivate {
+                    warn!(%user.id, "Scheduling user reactivation");
+                    repo.queue_job()
+                        .schedule_job(&mut rng, &clock, ReactivateUserJob::new(&user))
+                        .await?;
+                } else {
+                    repo.user().unlock(user).await?;
+                }
 
                 repo.into_inner().commit().await?;
 

crates/handlers/src/admin/v1/users/deactivate.rs

@@ -12,8 +12,6 @@ use mas_storage::{
     BoxRng,
     queue::{DeactivateUserJob, QueueJobRepositoryExt as _},
 };
-use schemars::JsonSchema;
-use serde::Deserialize;
 use tracing::info;
 use ulid::Ulid;
 
@@ -51,36 +49,21 @@ impl IntoResponse for RouteError {
     }
 }
 
-/// # JSON payload for the `POST /api/admin/v1/users/:id/deactivate` endpoint
-#[derive(Default, Deserialize, JsonSchema)]
-#[serde(rename = "DeactivateUserRequest")]
-pub struct Request {
-    /// Whether to skip locking the user before deactivation.
-    #[serde(default)]
-    skip_lock: bool,
-}
-
-pub fn doc(mut operation: TransformOperation) -> TransformOperation {
-    operation
-        .inner_mut()
-        .request_body
-        .as_mut()
-        .unwrap()
-        .as_item_mut()
-        .unwrap()
-        .required = false;
-
+pub fn doc(operation: TransformOperation) -> TransformOperation {
     operation
         .id("deactivateUser")
         .summary("Deactivate a user")
-        .description("Calling this endpoint will lock and deactivate the user, preventing them from doing any action.
-This invalidates any existing session, and will ask the homeserver to make them leave all rooms.")
+        .description(
+            "Calling this endpoint will deactivate the user, preventing them from doing any action.
+This invalidates any existing session, and will ask the homeserver to make them leave all rooms.",
+        )
         .tag("user")
         .response_with::<200, Json<SingleResponse<User>>, _>(|t| {
             // In the samples, the third user is the one locked
             let [_alice, _bob, charlie, ..] = User::samples();
             let id = charlie.id();
-            let response = SingleResponse::new(charlie, format!("/api/admin/v1/users/{id}/deactivate"));
+            let response =
+                SingleResponse::new(charlie, format!("/api/admin/v1/users/{id}/deactivate"));
             t.description("User was deactivated").example(response)
         })
         .response_with::<404, RouteError, _>(|t| {
@@ -96,19 +79,15 @@ pub async fn handler(
     }: CallContext,
     NoApi(mut rng): NoApi<BoxRng>,
     id: UlidPathParam,
-    body: Option<Json<Request>>,
 ) -> Result<Json<SingleResponse<User>>, RouteError> {
-    let Json(params) = body.unwrap_or_default();
     let id = *id;
-    let mut user = repo
+    let user = repo
         .user()
         .lookup(id)
         .await?
         .ok_or(RouteError::NotFound(id))?;
 
-    if !params.skip_lock && user.locked_at.is_none() {
-        user = repo.user().lock(&clock, user).await?;
-    }
+    let user = repo.user().deactivate(&clock, user).await?;
 
     info!(%user.id, "Scheduling deactivation of user");
     repo.queue_job()
@@ -127,13 +106,14 @@ pub async fn handler(
 mod tests {
     use chrono::Duration;
     use hyper::{Request, StatusCode};
-    use insta::{allow_duplicates, assert_json_snapshot};
+    use insta::assert_json_snapshot;
     use mas_storage::{Clock, RepositoryAccess, user::UserRepository};
     use sqlx::PgPool;
 
     use crate::test_utils::{RequestBuilderExt, ResponseExt, TestState, setup};
 
-    async fn test_deactivate_user_helper(pool: PgPool, skip_lock: Option<bool>) {
+    #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")]
+    async fn test_deactivate_user(pool: PgPool) {
         setup();
         let mut state = TestState::from_pool(pool.clone()).await.unwrap();
         let token = state.token_with_scope("urn:mas:admin").await;
@@ -146,27 +126,23 @@ mod tests {
             .unwrap();
         repo.save().await.unwrap();
 
-        let request =
-            Request::post(format!("/api/admin/v1/users/{}/deactivate", user.id)).bearer(&token);
-        let request = match skip_lock {
-            None => request.empty(),
-            Some(skip_lock) => request.json(serde_json::json!({
-                "skip_lock": skip_lock,
-            })),
-        };
+        let request = Request::post(format!("/api/admin/v1/users/{}/deactivate", user.id))
+            .bearer(&token)
+            .empty();
         let response = state.request(request).await;
         response.assert_status(StatusCode::OK);
         let body: serde_json::Value = response.json();
 
-        // The locked_at timestamp should be the same as the current time, or null if
-        // not locked
+        // The deactivated_at timestamp should be the same as the current time
+        assert_eq!(
+            body["data"]["attributes"]["deactivated_at"],
+            serde_json::json!(state.clock.now())
+        );
+
+        // Deactivating the user should not lock it
         assert_eq!(
             body["data"]["attributes"]["locked_at"],
-            if skip_lock.unwrap_or(false) {
-                serde_json::Value::Null
-            } else {
-                serde_json::json!(state.clock.now())
-            }
+            serde_json::Value::Null
         );
 
         // Make sure to run the jobs in the queue
@@ -179,15 +155,15 @@ mod tests {
         response.assert_status(StatusCode::OK);
         let body: serde_json::Value = response.json();
 
-        allow_duplicates!(assert_json_snapshot!(body, @r#"
+        assert_json_snapshot!(body, @r#"
         {
           "data": {
             "type": "user",
             "id": "01FSHN9AG0MZAA6S4AF7CTV32E",
             "attributes": {
               "username": "alice",
               "created_at": "2022-01-16T14:40:00Z",
-              "locked_at": "2022-01-16T14:40:00Z",
+              "locked_at": null,
               "deactivated_at": "2022-01-16T14:40:00Z",
               "admin": false
             },
@@ -199,17 +175,7 @@ mod tests {
             "self": "/api/admin/v1/users/01FSHN9AG0MZAA6S4AF7CTV32E"
           }
         }
-        "#));
-    }
-
-    #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")]
-    async fn test_deactivate_user(pool: PgPool) {
-        test_deactivate_user_helper(pool, Option::None).await;
-    }
-
-    #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")]
-    async fn test_deactivate_user_skip_lock(pool: PgPool) {
-        test_deactivate_user_helper(pool, Option::Some(true)).await;
+        "#);
     }
 
     #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")]
@@ -237,14 +203,16 @@ mod tests {
         response.assert_status(StatusCode::OK);
         let body: serde_json::Value = response.json();
 
-        // The locked_at timestamp should be different from the current time
-        assert_ne!(
-            body["data"]["attributes"]["locked_at"],
+        // The deactivated_at timestamp should be the same as the current time
+        assert_eq!(
+            body["data"]["attributes"]["deactivated_at"],
             serde_json::json!(state.clock.now())
         );
+
+        // The deactivated_at timestamp should be different from the locked_at timestamp
         assert_ne!(
+            body["data"]["attributes"]["deactivated_at"],
             body["data"]["attributes"]["locked_at"],
-            serde_json::Value::Null
         );
 
         // Make sure to run the jobs in the queue

crates/handlers/src/admin/v1/users/lock.rs

@@ -72,15 +72,13 @@ pub async fn handler(
     id: UlidPathParam,
 ) -> Result<Json<SingleResponse<User>>, RouteError> {
     let id = *id;
-    let mut user = repo
+    let user = repo
         .user()
         .lookup(id)
         .await?
         .ok_or(RouteError::NotFound(id))?;
 
-    if user.locked_at.is_none() {
-        user = repo.user().lock(&clock, user).await?;
-    }
+    let user = repo.user().lock(&clock, user).await?;
 
     repo.save().await?;
 

crates/handlers/src/admin/v1/users/reactivate.rs

@@ -53,7 +53,8 @@ pub fn doc(operation: TransformOperation) -> TransformOperation {
     operation
         .id("reactivateUser")
         .summary("Reactivate a user")
-        .description("Calling this endpoint will reactivate a deactivated user, both locally and on the Matrix homeserver.")
+        .description("Calling this endpoint will reactivate a deactivated user.
+This DOES NOT unlock a locked user, which is still prevented from doing any action until it is explicitly unlocked.")
         .tag("user")
         .response_with::<200, Json<SingleResponse<User>>, _>(|t| {
             // In the samples, the third user is the one locked