From a8b8c8e31cc6df3153f137210694c61cfb4471ce Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 9 Jul 2025 08:25:25 -0400 Subject: [PATCH 1/7] Add admin API endpoint to reactivate user --- crates/cli/src/commands/manage.rs | 2 +- crates/handlers/src/admin/v1/mod.rs | 4 + .../handlers/src/admin/v1/users/deactivate.rs | 6 + crates/handlers/src/admin/v1/users/lock.rs | 4 + crates/handlers/src/admin/v1/users/mod.rs | 2 + .../handlers/src/admin/v1/users/reactivate.rs | 223 ++++++++++++++++++ crates/handlers/src/admin/v1/users/unlock.rs | 7 +- ...5903ac99d9bb8ca4d79c908b25a6d1209b9b1.json | 14 ++ crates/storage-pg/src/user/mod.rs | 33 +++ crates/storage/src/queue/tasks.rs | 17 +- crates/storage/src/user/mod.rs | 14 ++ crates/tasks/src/user.rs | 22 +- docs/api/spec.json | 70 ++++++ 13 files changed, 409 insertions(+), 9 deletions(-) create mode 100644 crates/handlers/src/admin/v1/users/reactivate.rs create mode 100644 crates/storage-pg/.sqlx/query-98a5491eb5f10997ac1f3718c835903ac99d9bb8ca4d79c908b25a6d1209b9b1.json diff --git a/crates/cli/src/commands/manage.rs b/crates/cli/src/commands/manage.rs index 97c019175..41b9a11f7 100644 --- a/crates/cli/src/commands/manage.rs +++ b/crates/cli/src/commands/manage.rs @@ -542,7 +542,7 @@ impl Options { warn!(%user.id, "User scheduling user reactivation"); repo.queue_job() - .schedule_job(&mut rng, &clock, ReactivateUserJob::new(&user)) + .schedule_job(&mut rng, &clock, ReactivateUserJob::new(&user, true)) .await?; repo.into_inner().commit().await?; diff --git a/crates/handlers/src/admin/v1/mod.rs b/crates/handlers/src/admin/v1/mod.rs index c1d29aad5..af1951019 100644 --- a/crates/handlers/src/admin/v1/mod.rs +++ b/crates/handlers/src/admin/v1/mod.rs @@ -94,6 +94,10 @@ where "/users/{id}/deactivate", post_with(self::users::deactivate, self::users::deactivate_doc), ) + .api_route( + "/users/{id}/reactivate", + post_with(self::users::reactivate, self::users::reactivate_doc), + ) .api_route( "/users/{id}/lock", post_with(self::users::lock, self::users::lock_doc), diff --git a/crates/handlers/src/admin/v1/users/deactivate.rs b/crates/handlers/src/admin/v1/users/deactivate.rs index 87c2361a2..7a6bd8e4e 100644 --- a/crates/handlers/src/admin/v1/users/deactivate.rs +++ b/crates/handlers/src/admin/v1/users/deactivate.rs @@ -137,6 +137,7 @@ mod tests { body["data"]["attributes"]["locked_at"], serde_json::json!(state.clock.now()) ); + // TODO: have test coverage on deactivated_at timestamp // Make sure to run the jobs in the queue state.run_jobs_in_queue().await; @@ -201,6 +202,11 @@ mod tests { body["data"]["attributes"]["locked_at"], serde_json::json!(state.clock.now()) ); + assert_ne!( + body["data"]["attributes"]["locked_at"], + serde_json::Value::Null + ); + // TODO: have test coverage on deactivated_at timestamp // Make sure to run the jobs in the queue state.run_jobs_in_queue().await; diff --git a/crates/handlers/src/admin/v1/users/lock.rs b/crates/handlers/src/admin/v1/users/lock.rs index ed99b6a75..9db2a065a 100644 --- a/crates/handlers/src/admin/v1/users/lock.rs +++ b/crates/handlers/src/admin/v1/users/lock.rs @@ -157,6 +157,10 @@ mod tests { body["data"]["attributes"]["locked_at"], serde_json::json!(state.clock.now()) ); + assert_ne!( + body["data"]["attributes"]["locked_at"], + serde_json::Value::Null + ); } #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] diff --git a/crates/handlers/src/admin/v1/users/mod.rs b/crates/handlers/src/admin/v1/users/mod.rs index b9c0b5ea6..37484b75b 100644 --- a/crates/handlers/src/admin/v1/users/mod.rs +++ b/crates/handlers/src/admin/v1/users/mod.rs @@ -10,6 +10,7 @@ mod deactivate; mod get; mod list; mod lock; +mod reactivate; mod set_admin; mod set_password; mod unlock; @@ -21,6 +22,7 @@ pub use self::{ get::{doc as get_doc, handler as get}, list::{doc as list_doc, handler as list}, lock::{doc as lock_doc, handler as lock}, + reactivate::{doc as reactivate_doc, handler as reactivate}, set_admin::{doc as set_admin_doc, handler as set_admin}, set_password::{doc as set_password_doc, handler as set_password}, unlock::{doc as unlock_doc, handler as unlock}, diff --git a/crates/handlers/src/admin/v1/users/reactivate.rs b/crates/handlers/src/admin/v1/users/reactivate.rs new file mode 100644 index 000000000..44c5ae88c --- /dev/null +++ b/crates/handlers/src/admin/v1/users/reactivate.rs @@ -0,0 +1,223 @@ +// Copyright 2025 New Vector Ltd. +// +// SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial +// Please see LICENSE files in the repository root for full details. + +use aide::{NoApi, OperationIo, transform::TransformOperation}; +use axum::{Json, response::IntoResponse}; +use hyper::StatusCode; +use mas_axum_utils::record_error; +use mas_storage::{ + BoxRng, + queue::{QueueJobRepositoryExt as _, ReactivateUserJob}, +}; +use tracing::info; +use ulid::Ulid; + +use crate::{ + admin::{ + call_context::CallContext, + model::{Resource, User}, + params::UlidPathParam, + response::{ErrorResponse, SingleResponse}, + }, + impl_from_error_for_route, +}; + +#[derive(Debug, thiserror::Error, OperationIo)] +#[aide(output_with = "Json")] +pub enum RouteError { + #[error(transparent)] + Internal(Box), + + #[error("User ID {0} not found")] + NotFound(Ulid), +} + +impl_from_error_for_route!(mas_storage::RepositoryError); + +impl IntoResponse for RouteError { + fn into_response(self) -> axum::response::Response { + let error = ErrorResponse::from_error(&self); + let sentry_event_id = record_error!(self, Self::Internal(_)); + let status = match self { + Self::Internal(_) => StatusCode::INTERNAL_SERVER_ERROR, + Self::NotFound(_) => StatusCode::NOT_FOUND, + }; + (status, sentry_event_id, Json(error)).into_response() + } +} + +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.") + .tag("user") + .response_with::<200, Json>, _>(|t| { + // In the samples, the third user is the one locked + let [sample, ..] = User::samples(); + let id = sample.id(); + let response = SingleResponse::new(sample, format!("/api/admin/v1/users/{id}/reactivate")); + t.description("User was reactivated").example(response) + }) + .response_with::<404, RouteError, _>(|t| { + let response = ErrorResponse::from_error(&RouteError::NotFound(Ulid::nil())); + t.description("User ID not found").example(response) + }) +} + +#[tracing::instrument(name = "handler.admin.v1.users.reactivate", skip_all)] +pub async fn handler( + CallContext { + mut repo, clock, .. + }: CallContext, + NoApi(mut rng): NoApi, + id: UlidPathParam, +) -> Result>, RouteError> { + let id = *id; + let user = repo + .user() + .lookup(id) + .await? + .ok_or(RouteError::NotFound(id))?; + + info!(%user.id, "Scheduling reactivation of user"); + repo.queue_job() + .schedule_job(&mut rng, &clock, ReactivateUserJob::new(&user, false)) + .await?; + + repo.save().await?; + + Ok(Json(SingleResponse::new( + User::from(user), + format!("/api/admin/v1/users/{id}/reactivate"), + ))) +} + +#[cfg(test)] +mod tests { + use hyper::{Request, StatusCode}; + use mas_matrix::{HomeserverConnection, ProvisionRequest}; + use mas_storage::{Clock, RepositoryAccess, user::UserRepository}; + use sqlx::{PgPool, types::Json}; + + use crate::test_utils::{RequestBuilderExt, ResponseExt, TestState, setup}; + + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_reactivate_deactivated_user(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(); + let user = repo.user().lock(&state.clock, user).await.unwrap(); + let user = repo.user().deactivate(&state.clock, user).await.unwrap(); + repo.save().await.unwrap(); + + // Provision and immediately deactivate the user on the homeserver, + // because this endpoint will try to reactivate it + let mxid = state.homeserver_connection.mxid(&user.username); + state + .homeserver_connection + .provision_user(&ProvisionRequest::new(&mxid, &user.sub)) + .await + .unwrap(); + state + .homeserver_connection + .delete_user(&mxid, true) + .await + .unwrap(); + + // The user should be deactivated on the homeserver + let mx_user = state.homeserver_connection.query_user(&mxid).await.unwrap(); + assert!(mx_user.deactivated); + + let request = Request::post(format!("/api/admin/v1/users/{}/reactivate", user.id)) + .bearer(&token) + .empty(); + let response = state.request(request).await; + response.assert_status(StatusCode::OK); + let body: serde_json::Value = response.json(); + + // The user should remain locked after being reactivated + assert_eq!( + body["data"]["attributes"]["locked_at"], + serde_json::json!(state.clock.now()) + ); + // TODO: have test coverage on deactivated_at timestamp + + // It should have scheduled a reactivation job for the user + // XXX: we don't have a good way to look for the reactivation job + let job: Json = sqlx::query_scalar( + "SELECT payload FROM queue_jobs WHERE queue_name = 'reactivate-user'", + ) + .fetch_one(&pool) + .await + .expect("Reactivation job to be scheduled"); + assert_eq!(job["user_id"], serde_json::json!(user.id)); + assert_eq!(job["unlock"], serde_json::Value::Bool(false)); + } + + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_reactivate_active_user(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/{}/reactivate", user.id)) + .bearer(&token) + .empty(); + let response = state.request(request).await; + response.assert_status(StatusCode::OK); + let body: serde_json::Value = response.json(); + + assert_eq!( + body["data"]["attributes"]["locked_at"], + serde_json::Value::Null + ); + // TODO: have test coverage on deactivated_at timestamp + + // It should have scheduled a reactivation job for the user + // XXX: we don't have a good way to look for the reactivation job + let job: Json = sqlx::query_scalar( + "SELECT payload FROM queue_jobs WHERE queue_name = 'reactivate-user'", + ) + .fetch_one(&pool) + .await + .expect("Reactivation job to be scheduled"); + assert_eq!(job["user_id"], serde_json::json!(user.id)); + assert_eq!(job["unlock"], serde_json::Value::Bool(false)); + } + + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_reactivate_unknown_user(pool: PgPool) { + setup(); + let mut state = TestState::from_pool(pool).await.unwrap(); + let token = state.token_with_scope("urn:mas:admin").await; + + let request = Request::post("/api/admin/v1/users/01040G2081040G2081040G2081/reactivate") + .bearer(&token) + .empty(); + let response = state.request(request).await; + response.assert_status(StatusCode::NOT_FOUND); + let body: serde_json::Value = response.json(); + assert_eq!( + body["errors"][0]["title"], + "User ID 01040G2081040G2081040G2081 not found" + ); + } +} diff --git a/crates/handlers/src/admin/v1/users/unlock.rs b/crates/handlers/src/admin/v1/users/unlock.rs index 6e0311eec..e74d80aea 100644 --- a/crates/handlers/src/admin/v1/users/unlock.rs +++ b/crates/handlers/src/admin/v1/users/unlock.rs @@ -141,7 +141,7 @@ mod tests { assert_eq!( body["data"]["attributes"]["locked_at"], - serde_json::json!(null) + serde_json::Value::Null ); } @@ -158,6 +158,7 @@ mod tests { .await .unwrap(); let user = repo.user().lock(&state.clock, user).await.unwrap(); + let user = repo.user().deactivate(&state.clock, user).await.unwrap(); repo.save().await.unwrap(); // Provision the user on the homeserver @@ -187,8 +188,10 @@ mod tests { assert_eq!( body["data"]["attributes"]["locked_at"], - serde_json::json!(null) + serde_json::Value::Null ); + // TODO: have test coverage on deactivated_at timestamp + // The user should be reactivated on the homeserver let mx_user = state.homeserver_connection.query_user(&mxid).await.unwrap(); assert!(!mx_user.deactivated); diff --git a/crates/storage-pg/.sqlx/query-98a5491eb5f10997ac1f3718c835903ac99d9bb8ca4d79c908b25a6d1209b9b1.json b/crates/storage-pg/.sqlx/query-98a5491eb5f10997ac1f3718c835903ac99d9bb8ca4d79c908b25a6d1209b9b1.json new file mode 100644 index 000000000..75f013b53 --- /dev/null +++ b/crates/storage-pg/.sqlx/query-98a5491eb5f10997ac1f3718c835903ac99d9bb8ca4d79c908b25a6d1209b9b1.json @@ -0,0 +1,14 @@ +{ + "db_name": "PostgreSQL", + "query": "\n UPDATE users\n SET deactivated_at = NULL\n WHERE user_id = $1\n ", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Uuid" + ] + }, + "nullable": [] + }, + "hash": "98a5491eb5f10997ac1f3718c835903ac99d9bb8ca4d79c908b25a6d1209b9b1" +} diff --git a/crates/storage-pg/src/user/mod.rs b/crates/storage-pg/src/user/mod.rs index 14957ba5f..6abc29d9a 100644 --- a/crates/storage-pg/src/user/mod.rs +++ b/crates/storage-pg/src/user/mod.rs @@ -384,6 +384,39 @@ impl UserRepository for PgUserRepository<'_> { Ok(user) } + #[tracing::instrument( + name = "db.user.reactivate", + skip_all, + fields( + db.query.text, + %user.id, + ), + err, + )] + async fn reactivate(&mut self, mut user: User) -> Result { + if user.deactivated_at.is_none() { + return Ok(user); + } + + let res = sqlx::query!( + r#" + UPDATE users + SET deactivated_at = NULL + WHERE user_id = $1 + "#, + Uuid::from(user.id), + ) + .traced() + .execute(&mut *self.conn) + .await?; + + DatabaseError::ensure_affected_rows(&res, 1)?; + + user.deactivated_at = None; + + Ok(user) + } + #[tracing::instrument( name = "db.user.set_can_request_admin", skip_all, diff --git a/crates/storage/src/queue/tasks.rs b/crates/storage/src/queue/tasks.rs index eb16f6e29..87fb41486 100644 --- a/crates/storage/src/queue/tasks.rs +++ b/crates/storage/src/queue/tasks.rs @@ -257,10 +257,11 @@ impl InsertableJob for DeactivateUserJob { const QUEUE_NAME: &'static str = "deactivate-user"; } -/// A job to reactivate a user +/// A job to reactivate and optionally unlock a user #[derive(Serialize, Deserialize, Debug, Clone)] pub struct ReactivateUserJob { user_id: Ulid, + unlock: bool, } impl ReactivateUserJob { @@ -269,9 +270,13 @@ impl ReactivateUserJob { /// # Parameters /// /// * `user` - The user to reactivate + /// * `unlock` - Whether the user should be unlocked on reactivation #[must_use] - pub fn new(user: &User) -> Self { - Self { user_id: user.id } + pub fn new(user: &User, unlock: bool) -> Self { + Self { + user_id: user.id, + unlock, + } } /// The ID of the user to reactivate @@ -279,6 +284,12 @@ impl ReactivateUserJob { pub fn user_id(&self) -> Ulid { self.user_id } + + /// Whether the user should be unlocked on reactivation + #[must_use] + pub fn unlock(&self) -> bool { + self.unlock + } } impl InsertableJob for ReactivateUserJob { diff --git a/crates/storage/src/user/mod.rs b/crates/storage/src/user/mod.rs index 64b1d6d79..f864157b1 100644 --- a/crates/storage/src/user/mod.rs +++ b/crates/storage/src/user/mod.rs @@ -244,6 +244,19 @@ pub trait UserRepository: Send + Sync { /// Returns [`Self::Error`] if the underlying repository fails async fn deactivate(&mut self, clock: &dyn Clock, user: User) -> Result; + /// Reactivate a [`User`] + /// + /// Returns the reactivated [`User`] + /// + /// # Parameters + /// + /// * `user`: The [`User`] to reactivate + /// + /// # Errors + /// + /// Returns [`Self::Error`] if the underlying repository fails + async fn reactivate(&mut self, user: User) -> Result; + /// Set whether a [`User`] can request admin /// /// Returns the [`User`] with the new `can_request_admin` value @@ -315,6 +328,7 @@ repository_impl!(UserRepository: async fn lock(&mut self, clock: &dyn Clock, user: User) -> Result; async fn unlock(&mut self, user: User) -> Result; async fn deactivate(&mut self, clock: &dyn Clock, user: User) -> Result; + async fn reactivate(&mut self, user: User) -> Result; async fn set_can_request_admin( &mut self, user: User, diff --git a/crates/tasks/src/user.rs b/crates/tasks/src/user.rs index b5f64dd42..290dab28f 100644 --- a/crates/tasks/src/user.rs +++ b/crates/tasks/src/user.rs @@ -137,9 +137,25 @@ impl RunnableJob for ReactivateUserJob { .await .map_err(JobError::retry)?; - // We want to unlock the user from our side only once it has been reactivated on - // the homeserver - let _user = repo.user().unlock(user).await.map_err(JobError::retry)?; + // Now reactivate the user in our database + let user = repo + .user() + .reactivate(user) + .await + .context("Failed to reactivate user") + .map_err(JobError::retry)?; + + if self.unlock() { + // We want to unlock the user from our side only once it has been reactivated on + // the homeserver + let _user = repo + .user() + .unlock(user) + .await + .context("Failed to unlock user") + .map_err(JobError::retry)?; + } + repo.save().await.map_err(JobError::retry)?; Ok(()) diff --git a/docs/api/spec.json b/docs/api/spec.json index 0082ea37c..3673c54bc 100644 --- a/docs/api/spec.json +++ b/docs/api/spec.json @@ -1409,6 +1409,76 @@ } } }, + "/api/admin/v1/users/{id}/reactivate": { + "post": { + "tags": [ + "user" + ], + "summary": "Reactivate a user", + "description": "Calling this endpoint will reactivate a deactivated user, both locally and on the Matrix homeserver.", + "operationId": "reactivateUser", + "parameters": [ + { + "in": "path", + "name": "id", + "required": true, + "schema": { + "title": "The ID of the resource", + "$ref": "#/components/schemas/ULID" + }, + "style": "simple" + } + ], + "responses": { + "200": { + "description": "User was reactivated", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SingleResponse_for_User" + }, + "example": { + "data": { + "type": "user", + "id": "030C1G60R30C1G60R30C1G60R3", + "attributes": { + "username": "charlie", + "created_at": "1970-01-01T00:00:00Z", + "locked_at": "1970-01-01T00:00:00Z", + "deactivated_at": null, + "admin": false + }, + "links": { + "self": "/api/admin/v1/users/030C1G60R30C1G60R30C1G60R3" + } + }, + "links": { + "self": "/api/admin/v1/users/030C1G60R30C1G60R30C1G60R3/reactivate" + } + } + } + } + }, + "404": { + "description": "User ID not found", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + }, + "example": { + "errors": [ + { + "title": "User ID 00000000000000000000000000 not found" + } + ] + } + } + } + } + } + } + }, "/api/admin/v1/users/{id}/lock": { "post": { "tags": [ From 13a21cc0182b5222233f0da3229d9de9c7283c68 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 9 Jul 2025 09:13:56 -0400 Subject: [PATCH 2/7] Update schema --- docs/api/spec.json | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/api/spec.json b/docs/api/spec.json index 3673c54bc..b4d07e84d 100644 --- a/docs/api/spec.json +++ b/docs/api/spec.json @@ -1440,20 +1440,20 @@ "example": { "data": { "type": "user", - "id": "030C1G60R30C1G60R30C1G60R3", + "id": "01040G2081040G2081040G2081", "attributes": { - "username": "charlie", + "username": "alice", "created_at": "1970-01-01T00:00:00Z", - "locked_at": "1970-01-01T00:00:00Z", + "locked_at": null, "deactivated_at": null, "admin": false }, "links": { - "self": "/api/admin/v1/users/030C1G60R30C1G60R30C1G60R3" + "self": "/api/admin/v1/users/01040G2081040G2081040G2081" } }, "links": { - "self": "/api/admin/v1/users/030C1G60R30C1G60R30C1G60R3/reactivate" + "self": "/api/admin/v1/users/01040G2081040G2081040G2081/reactivate" } } } From 6c1afee13d6065009570a42961710342675b2b05 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 14 Jul 2025 14:20:32 -0400 Subject: [PATCH 3/7] Separate active state from lock state in admin API - Allow the admin API to deactivate a user without locking it, and to unlock a user without reactivating it. - Make unlock-and-reactivate flows unset the "deactivated_at" timestamp. - Revert adding an "unlock" parameter on `ReactivateUserJob`, as the option is used only by the admin API which doesn't use a job. --- crates/cli/src/commands/manage.rs | 2 +- .../handlers/src/admin/v1/users/deactivate.rs | 66 +++++++++++---- .../handlers/src/admin/v1/users/reactivate.rs | 80 +++++++++--------- crates/handlers/src/admin/v1/users/unlock.rs | 84 ++++++++++++++----- crates/handlers/src/graphql/mutations/user.rs | 2 +- ...a18ded5613186104695524e85df9b6641ea4e.json | 14 ++++ crates/storage-pg/src/user/mod.rs | 34 ++++++++ crates/storage/src/queue/tasks.rs | 19 +---- crates/storage/src/user/mod.rs | 14 ++++ crates/tasks/src/user.rs | 22 +---- docs/api/spec.json | 40 +++++++++ 11 files changed, 266 insertions(+), 111 deletions(-) create mode 100644 crates/storage-pg/.sqlx/query-3e2d1ce1c7aba2952ed9c659972a18ded5613186104695524e85df9b6641ea4e.json diff --git a/crates/cli/src/commands/manage.rs b/crates/cli/src/commands/manage.rs index 41b9a11f7..97c019175 100644 --- a/crates/cli/src/commands/manage.rs +++ b/crates/cli/src/commands/manage.rs @@ -542,7 +542,7 @@ impl Options { warn!(%user.id, "User scheduling user reactivation"); repo.queue_job() - .schedule_job(&mut rng, &clock, ReactivateUserJob::new(&user, true)) + .schedule_job(&mut rng, &clock, ReactivateUserJob::new(&user)) .await?; repo.into_inner().commit().await?; diff --git a/crates/handlers/src/admin/v1/users/deactivate.rs b/crates/handlers/src/admin/v1/users/deactivate.rs index 7a6bd8e4e..316b882be 100644 --- a/crates/handlers/src/admin/v1/users/deactivate.rs +++ b/crates/handlers/src/admin/v1/users/deactivate.rs @@ -12,6 +12,8 @@ use mas_storage::{ BoxRng, queue::{DeactivateUserJob, QueueJobRepositoryExt as _}, }; +use schemars::JsonSchema; +use serde::Deserialize; use tracing::info; use ulid::Ulid; @@ -49,7 +51,25 @@ impl IntoResponse for RouteError { } } -pub fn doc(operation: TransformOperation) -> TransformOperation { +/// # 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; + operation .id("deactivateUser") .summary("Deactivate a user") @@ -76,7 +96,9 @@ pub async fn handler( }: CallContext, NoApi(mut rng): NoApi, id: UlidPathParam, + body: Option>, ) -> Result>, RouteError> { + let Json(params) = body.unwrap_or_default(); let id = *id; let mut user = repo .user() @@ -84,7 +106,7 @@ pub async fn handler( .await? .ok_or(RouteError::NotFound(id))?; - if user.locked_at.is_none() { + if !params.skip_lock && user.locked_at.is_none() { user = repo.user().lock(&clock, user).await?; } @@ -105,14 +127,13 @@ pub async fn handler( mod tests { use chrono::Duration; use hyper::{Request, StatusCode}; - use insta::assert_json_snapshot; + use insta::{allow_duplicates, assert_json_snapshot}; use mas_storage::{Clock, RepositoryAccess, user::UserRepository}; use sqlx::PgPool; use crate::test_utils::{RequestBuilderExt, ResponseExt, TestState, setup}; - #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] - async fn test_deactivate_user(pool: PgPool) { + async fn test_deactivate_user_helper(pool: PgPool, skip_lock: Option) { setup(); let mut state = TestState::from_pool(pool.clone()).await.unwrap(); let token = state.token_with_scope("urn:mas:admin").await; @@ -125,19 +146,27 @@ mod tests { .unwrap(); repo.save().await.unwrap(); - let request = Request::post(format!("/api/admin/v1/users/{}/deactivate", user.id)) - .bearer(&token) - .empty(); + 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 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 + // The locked_at timestamp should be the same as the current time, or null if not locked assert_eq!( body["data"]["attributes"]["locked_at"], - serde_json::json!(state.clock.now()) + if !skip_lock.unwrap_or(false) { + serde_json::json!(state.clock.now()) + } else { + serde_json::Value::Null + } ); - // TODO: have test coverage on deactivated_at timestamp // Make sure to run the jobs in the queue state.run_jobs_in_queue().await; @@ -149,7 +178,7 @@ mod tests { response.assert_status(StatusCode::OK); let body: serde_json::Value = response.json(); - assert_json_snapshot!(body, @r#" + allow_duplicates!(assert_json_snapshot!(body, @r#" { "data": { "type": "user", @@ -169,7 +198,17 @@ 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")] @@ -206,7 +245,6 @@ mod tests { body["data"]["attributes"]["locked_at"], serde_json::Value::Null ); - // TODO: have test coverage on deactivated_at timestamp // Make sure to run the jobs in the queue state.run_jobs_in_queue().await; diff --git a/crates/handlers/src/admin/v1/users/reactivate.rs b/crates/handlers/src/admin/v1/users/reactivate.rs index 44c5ae88c..ad73c4dba 100644 --- a/crates/handlers/src/admin/v1/users/reactivate.rs +++ b/crates/handlers/src/admin/v1/users/reactivate.rs @@ -3,15 +3,13 @@ // SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial // Please see LICENSE files in the repository root for full details. -use aide::{NoApi, OperationIo, transform::TransformOperation}; -use axum::{Json, response::IntoResponse}; +use std::sync::Arc; + +use aide::{OperationIo, transform::TransformOperation}; +use axum::{Json, extract::State, response::IntoResponse}; use hyper::StatusCode; use mas_axum_utils::record_error; -use mas_storage::{ - BoxRng, - queue::{QueueJobRepositoryExt as _, ReactivateUserJob}, -}; -use tracing::info; +use mas_matrix::HomeserverConnection; use ulid::Ulid; use crate::{ @@ -30,6 +28,9 @@ pub enum RouteError { #[error(transparent)] Internal(Box), + #[error(transparent)] + Homeserver(anyhow::Error), + #[error("User ID {0} not found")] NotFound(Ulid), } @@ -39,9 +40,9 @@ impl_from_error_for_route!(mas_storage::RepositoryError); impl IntoResponse for RouteError { fn into_response(self) -> axum::response::Response { let error = ErrorResponse::from_error(&self); - let sentry_event_id = record_error!(self, Self::Internal(_)); + let sentry_event_id = record_error!(self, Self::Internal(_) | Self::Homeserver(_)); let status = match self { - Self::Internal(_) => StatusCode::INTERNAL_SERVER_ERROR, + Self::Internal(_) | Self::Homeserver(_) => StatusCode::INTERNAL_SERVER_ERROR, Self::NotFound(_) => StatusCode::NOT_FOUND, }; (status, sentry_event_id, Json(error)).into_response() @@ -69,10 +70,8 @@ pub fn doc(operation: TransformOperation) -> TransformOperation { #[tracing::instrument(name = "handler.admin.v1.users.reactivate", skip_all)] pub async fn handler( - CallContext { - mut repo, clock, .. - }: CallContext, - NoApi(mut rng): NoApi, + CallContext { mut repo, .. }: CallContext, + State(homeserver): State>, id: UlidPathParam, ) -> Result>, RouteError> { let id = *id; @@ -82,10 +81,15 @@ pub async fn handler( .await? .ok_or(RouteError::NotFound(id))?; - info!(%user.id, "Scheduling reactivation of user"); - repo.queue_job() - .schedule_job(&mut rng, &clock, ReactivateUserJob::new(&user, false)) - .await?; + // Call the homeserver synchronously to reactivate the user + let mxid = homeserver.mxid(&user.username); + homeserver + .reactivate_user(&mxid) + .await + .map_err(RouteError::Homeserver)?; + + // Now reactivate the user in our database + let user = repo.user().reactivate(user).await?; repo.save().await?; @@ -100,7 +104,7 @@ mod tests { use hyper::{Request, StatusCode}; use mas_matrix::{HomeserverConnection, ProvisionRequest}; use mas_storage::{Clock, RepositoryAccess, user::UserRepository}; - use sqlx::{PgPool, types::Json}; + use sqlx::PgPool; use crate::test_utils::{RequestBuilderExt, ResponseExt, TestState, setup}; @@ -150,18 +154,10 @@ mod tests { body["data"]["attributes"]["locked_at"], serde_json::json!(state.clock.now()) ); - // TODO: have test coverage on deactivated_at timestamp - - // It should have scheduled a reactivation job for the user - // XXX: we don't have a good way to look for the reactivation job - let job: Json = sqlx::query_scalar( - "SELECT payload FROM queue_jobs WHERE queue_name = 'reactivate-user'", - ) - .fetch_one(&pool) - .await - .expect("Reactivation job to be scheduled"); - assert_eq!(job["user_id"], serde_json::json!(user.id)); - assert_eq!(job["unlock"], serde_json::Value::Bool(false)); + assert_eq!( + body["data"]["attributes"]["deactivated_at"], + serde_json::Value::Null, + ); } #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] @@ -178,6 +174,14 @@ mod tests { .unwrap(); repo.save().await.unwrap(); + // Provision the user on the homeserver + let mxid = state.homeserver_connection.mxid(&user.username); + state + .homeserver_connection + .provision_user(&ProvisionRequest::new(&mxid, &user.sub)) + .await + .unwrap(); + let request = Request::post(format!("/api/admin/v1/users/{}/reactivate", user.id)) .bearer(&token) .empty(); @@ -189,18 +193,10 @@ mod tests { body["data"]["attributes"]["locked_at"], serde_json::Value::Null ); - // TODO: have test coverage on deactivated_at timestamp - - // It should have scheduled a reactivation job for the user - // XXX: we don't have a good way to look for the reactivation job - let job: Json = sqlx::query_scalar( - "SELECT payload FROM queue_jobs WHERE queue_name = 'reactivate-user'", - ) - .fetch_one(&pool) - .await - .expect("Reactivation job to be scheduled"); - assert_eq!(job["user_id"], serde_json::json!(user.id)); - assert_eq!(job["unlock"], serde_json::Value::Bool(false)); + assert_eq!( + body["data"]["attributes"]["deactivated_at"], + serde_json::Value::Null + ); } #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] diff --git a/crates/handlers/src/admin/v1/users/unlock.rs b/crates/handlers/src/admin/v1/users/unlock.rs index e74d80aea..224d6a81b 100644 --- a/crates/handlers/src/admin/v1/users/unlock.rs +++ b/crates/handlers/src/admin/v1/users/unlock.rs @@ -11,6 +11,8 @@ use axum::{Json, extract::State, response::IntoResponse}; use hyper::StatusCode; use mas_axum_utils::record_error; use mas_matrix::HomeserverConnection; +use schemars::JsonSchema; +use serde::Deserialize; use ulid::Ulid; use crate::{ @@ -50,7 +52,25 @@ impl IntoResponse for RouteError { } } -pub fn doc(operation: TransformOperation) -> TransformOperation { +/// # JSON payload for the `POST /api/admin/v1/users/:id/unlock` endpoint +#[derive(Default, Deserialize, JsonSchema)] +#[serde(rename = "UnlockUserRequest")] +pub struct Request { + /// Whether to skip ensuring the user is active upon being unlocked. + #[serde(default)] + skip_reactivate: bool, +} + +pub fn doc(mut operation: TransformOperation) -> TransformOperation { + operation + .inner_mut() + .request_body + .as_mut() + .unwrap() + .as_item_mut() + .unwrap() + .required = false; + operation .id("unlockUser") .summary("Unlock a user") @@ -73,7 +93,9 @@ pub async fn handler( CallContext { mut repo, .. }: CallContext, State(homeserver): State>, id: UlidPathParam, + body: Option>, ) -> Result>, RouteError> { + let Json(params) = body.unwrap_or_default(); let id = *id; let user = repo .user() @@ -81,15 +103,17 @@ pub async fn handler( .await? .ok_or(RouteError::NotFound(id))?; - // Call the homeserver synchronously to unlock the user - let mxid = homeserver.mxid(&user.username); - homeserver - .reactivate_user(&mxid) - .await - .map_err(RouteError::Homeserver)?; - - // Now unlock the user in our database - let user = repo.user().unlock(user).await?; + let user = if !params.skip_reactivate { + // Call the homeserver synchronously to reactivate the user + let mxid = homeserver.mxid(&user.username); + homeserver + .reactivate_user(&mxid) + .await + .map_err(RouteError::Homeserver)?; + repo.user().reactivate_and_unlock(user).await? + } else { + repo.user().unlock(user).await? + }; repo.save().await?; @@ -103,7 +127,7 @@ pub async fn handler( mod tests { use hyper::{Request, StatusCode}; use mas_matrix::{HomeserverConnection, ProvisionRequest}; - use mas_storage::{RepositoryAccess, user::UserRepository}; + use mas_storage::{user::UserRepository, Clock, RepositoryAccess}; use sqlx::PgPool; use crate::test_utils::{RequestBuilderExt, ResponseExt, TestState, setup}; @@ -145,8 +169,7 @@ mod tests { ); } - #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] - async fn test_unlock_deactivated_user(pool: PgPool) { + async fn test_unlock_deactivated_user_helper(pool: PgPool, skip_reactivate: Option) { setup(); let mut state = TestState::from_pool(pool).await.unwrap(); let token = state.token_with_scope("urn:mas:admin").await; @@ -179,9 +202,13 @@ mod tests { let mx_user = state.homeserver_connection.query_user(&mxid).await.unwrap(); assert!(mx_user.deactivated); - let request = Request::post(format!("/api/admin/v1/users/{}/unlock", user.id)) - .bearer(&token) - .empty(); + let request = Request::post(format!("/api/admin/v1/users/{}/unlock", user.id)).bearer(&token); + let request = match skip_reactivate { + None => request.empty(), + Some(skip_reactivate) => request.json(serde_json::json!({ + "skip_reactivate": skip_reactivate, + })), + }; let response = state.request(request).await; response.assert_status(StatusCode::OK); let body: serde_json::Value = response.json(); @@ -190,11 +217,30 @@ mod tests { body["data"]["attributes"]["locked_at"], serde_json::Value::Null ); - // TODO: have test coverage on deactivated_at timestamp - // The user should be reactivated on the homeserver + let skip_reactivate = skip_reactivate.unwrap_or(false); + assert_eq!( + body["data"]["attributes"]["deactivated_at"], + if !skip_reactivate { + serde_json::Value::Null + } else { + serde_json::json!(state.clock.now()) + } + ); + + // Check whether the user should be reactivated on the homeserver let mx_user = state.homeserver_connection.query_user(&mxid).await.unwrap(); - assert!(!mx_user.deactivated); + assert_eq!(mx_user.deactivated, skip_reactivate); + } + + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_unlock_deactivated_user(pool: PgPool) { + test_unlock_deactivated_user_helper(pool, Option::None).await; + } + + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_unlock_deactivated_user_skip_reactivate(pool: PgPool) { + test_unlock_deactivated_user_helper(pool, Option::Some(true)).await; } #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] diff --git a/crates/handlers/src/graphql/mutations/user.rs b/crates/handlers/src/graphql/mutations/user.rs index a403d95ce..a5d7e0fc2 100644 --- a/crates/handlers/src/graphql/mutations/user.rs +++ b/crates/handlers/src/graphql/mutations/user.rs @@ -590,7 +590,7 @@ impl UserMutations { matrix.reactivate_user(&mxid).await?; // Now unlock the user in our database - let user = repo.user().unlock(user).await?; + let user = repo.user().reactivate_and_unlock(user).await?; repo.save().await?; diff --git a/crates/storage-pg/.sqlx/query-3e2d1ce1c7aba2952ed9c659972a18ded5613186104695524e85df9b6641ea4e.json b/crates/storage-pg/.sqlx/query-3e2d1ce1c7aba2952ed9c659972a18ded5613186104695524e85df9b6641ea4e.json new file mode 100644 index 000000000..738adae1c --- /dev/null +++ b/crates/storage-pg/.sqlx/query-3e2d1ce1c7aba2952ed9c659972a18ded5613186104695524e85df9b6641ea4e.json @@ -0,0 +1,14 @@ +{ + "db_name": "PostgreSQL", + "query": "\n UPDATE users\n SET deactivated_at = NULL, locked_at = NULL\n WHERE user_id = $1\n ", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Uuid" + ] + }, + "nullable": [] + }, + "hash": "3e2d1ce1c7aba2952ed9c659972a18ded5613186104695524e85df9b6641ea4e" +} diff --git a/crates/storage-pg/src/user/mod.rs b/crates/storage-pg/src/user/mod.rs index 6abc29d9a..a1f321e7c 100644 --- a/crates/storage-pg/src/user/mod.rs +++ b/crates/storage-pg/src/user/mod.rs @@ -417,6 +417,40 @@ impl UserRepository for PgUserRepository<'_> { Ok(user) } + #[tracing::instrument( + name = "db.user.reactivate_and_unlock", + skip_all, + fields( + db.query.text, + %user.id, + ), + err, + )] + async fn reactivate_and_unlock(&mut self, mut user: User) -> Result { + if user.deactivated_at.is_none() && user.locked_at.is_none() { + return Ok(user); + } + + let res = sqlx::query!( + r#" + UPDATE users + SET deactivated_at = NULL, locked_at = NULL + WHERE user_id = $1 + "#, + Uuid::from(user.id), + ) + .traced() + .execute(&mut *self.conn) + .await?; + + DatabaseError::ensure_affected_rows(&res, 1)?; + + user.deactivated_at = None; + user.locked_at = None; + + Ok(user) + } + #[tracing::instrument( name = "db.user.set_can_request_admin", skip_all, diff --git a/crates/storage/src/queue/tasks.rs b/crates/storage/src/queue/tasks.rs index 87fb41486..f59971ba4 100644 --- a/crates/storage/src/queue/tasks.rs +++ b/crates/storage/src/queue/tasks.rs @@ -257,26 +257,21 @@ impl InsertableJob for DeactivateUserJob { const QUEUE_NAME: &'static str = "deactivate-user"; } -/// A job to reactivate and optionally unlock a user +/// A job to reactivate and unlock a user #[derive(Serialize, Deserialize, Debug, Clone)] pub struct ReactivateUserJob { user_id: Ulid, - unlock: bool, } impl ReactivateUserJob { - /// Create a new job to reactivate a user + /// Create a new job to reactivate and unlock a user /// /// # Parameters /// /// * `user` - The user to reactivate - /// * `unlock` - Whether the user should be unlocked on reactivation #[must_use] - pub fn new(user: &User, unlock: bool) -> Self { - Self { - user_id: user.id, - unlock, - } + pub fn new(user: &User) -> Self { + Self { user_id: user.id } } /// The ID of the user to reactivate @@ -284,12 +279,6 @@ impl ReactivateUserJob { pub fn user_id(&self) -> Ulid { self.user_id } - - /// Whether the user should be unlocked on reactivation - #[must_use] - pub fn unlock(&self) -> bool { - self.unlock - } } impl InsertableJob for ReactivateUserJob { diff --git a/crates/storage/src/user/mod.rs b/crates/storage/src/user/mod.rs index f864157b1..f990af3e2 100644 --- a/crates/storage/src/user/mod.rs +++ b/crates/storage/src/user/mod.rs @@ -257,6 +257,19 @@ pub trait UserRepository: Send + Sync { /// Returns [`Self::Error`] if the underlying repository fails async fn reactivate(&mut self, user: User) -> Result; + /// Reactivate and unlock a [`User`] + /// + /// Returns the reactivated and unlocked [`User`] + /// + /// # Parameters + /// + /// * `user`: The [`User`] to reactivate and unlock + /// + /// # Errors + /// + /// Returns [`Self::Error`] if the underlying repository fails + async fn reactivate_and_unlock(&mut self, user: User) -> Result; + /// Set whether a [`User`] can request admin /// /// Returns the [`User`] with the new `can_request_admin` value @@ -329,6 +342,7 @@ repository_impl!(UserRepository: async fn unlock(&mut self, user: User) -> Result; async fn deactivate(&mut self, clock: &dyn Clock, user: User) -> Result; async fn reactivate(&mut self, user: User) -> Result; + async fn reactivate_and_unlock(&mut self, user: User) -> Result; async fn set_can_request_admin( &mut self, user: User, diff --git a/crates/tasks/src/user.rs b/crates/tasks/src/user.rs index 290dab28f..01864764a 100644 --- a/crates/tasks/src/user.rs +++ b/crates/tasks/src/user.rs @@ -137,25 +137,9 @@ impl RunnableJob for ReactivateUserJob { .await .map_err(JobError::retry)?; - // Now reactivate the user in our database - let user = repo - .user() - .reactivate(user) - .await - .context("Failed to reactivate user") - .map_err(JobError::retry)?; - - if self.unlock() { - // We want to unlock the user from our side only once it has been reactivated on - // the homeserver - let _user = repo - .user() - .unlock(user) - .await - .context("Failed to unlock user") - .map_err(JobError::retry)?; - } - + // We want to unlock the user from our side only once it has been reactivated on + // the homeserver + let _user = repo.user().reactivate_and_unlock(user).await.map_err(JobError::retry)?; repo.save().await.map_err(JobError::retry)?; Ok(()) diff --git a/docs/api/spec.json b/docs/api/spec.json index b4d07e84d..28d394254 100644 --- a/docs/api/spec.json +++ b/docs/api/spec.json @@ -1359,6 +1359,15 @@ "style": "simple" } ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/DeactivateUserRequest" + } + } + } + }, "responses": { "200": { "description": "User was deactivated", @@ -1568,6 +1577,15 @@ "style": "simple" } ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/UnlockUserRequest" + } + } + } + }, "responses": { "200": { "description": "User was unlocked", @@ -3942,6 +3960,28 @@ } } }, + "DeactivateUserRequest": { + "title": "JSON payload for the `POST /api/admin/v1/users/:id/deactivate` endpoint", + "type": "object", + "properties": { + "skip_lock": { + "description": "Whether to skip locking the user before deactivation.", + "default": false, + "type": "boolean" + } + } + }, + "UnlockUserRequest": { + "title": "JSON payload for the `POST /api/admin/v1/users/:id/unlock` endpoint", + "type": "object", + "properties": { + "skip_reactivate": { + "description": "Whether to skip ensuring the user is active upon being unlocked.", + "default": false, + "type": "boolean" + } + } + }, "UserEmailFilter": { "type": "object", "properties": { From eca22d335be6f4908682bfe2274a14e8d17edaac Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 14 Jul 2025 14:43:44 -0400 Subject: [PATCH 4/7] Format --- crates/handlers/src/admin/v1/users/deactivate.rs | 3 ++- crates/handlers/src/admin/v1/users/unlock.rs | 5 +++-- crates/tasks/src/user.rs | 6 +++++- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/crates/handlers/src/admin/v1/users/deactivate.rs b/crates/handlers/src/admin/v1/users/deactivate.rs index 316b882be..815178f62 100644 --- a/crates/handlers/src/admin/v1/users/deactivate.rs +++ b/crates/handlers/src/admin/v1/users/deactivate.rs @@ -158,7 +158,8 @@ mod tests { 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 locked_at timestamp should be the same as the current time, or null if + // not locked assert_eq!( body["data"]["attributes"]["locked_at"], if !skip_lock.unwrap_or(false) { diff --git a/crates/handlers/src/admin/v1/users/unlock.rs b/crates/handlers/src/admin/v1/users/unlock.rs index 224d6a81b..b82b716a9 100644 --- a/crates/handlers/src/admin/v1/users/unlock.rs +++ b/crates/handlers/src/admin/v1/users/unlock.rs @@ -127,7 +127,7 @@ pub async fn handler( mod tests { use hyper::{Request, StatusCode}; use mas_matrix::{HomeserverConnection, ProvisionRequest}; - use mas_storage::{user::UserRepository, Clock, RepositoryAccess}; + use mas_storage::{Clock, RepositoryAccess, user::UserRepository}; use sqlx::PgPool; use crate::test_utils::{RequestBuilderExt, ResponseExt, TestState, setup}; @@ -202,7 +202,8 @@ mod tests { let mx_user = state.homeserver_connection.query_user(&mxid).await.unwrap(); assert!(mx_user.deactivated); - let request = Request::post(format!("/api/admin/v1/users/{}/unlock", user.id)).bearer(&token); + let request = + Request::post(format!("/api/admin/v1/users/{}/unlock", user.id)).bearer(&token); let request = match skip_reactivate { None => request.empty(), Some(skip_reactivate) => request.json(serde_json::json!({ diff --git a/crates/tasks/src/user.rs b/crates/tasks/src/user.rs index 01864764a..f7ce2f9ff 100644 --- a/crates/tasks/src/user.rs +++ b/crates/tasks/src/user.rs @@ -139,7 +139,11 @@ impl RunnableJob for ReactivateUserJob { // We want to unlock the user from our side only once it has been reactivated on // the homeserver - let _user = repo.user().reactivate_and_unlock(user).await.map_err(JobError::retry)?; + let _user = repo + .user() + .reactivate_and_unlock(user) + .await + .map_err(JobError::retry)?; repo.save().await.map_err(JobError::retry)?; Ok(()) From df8032695a52d5714be44646f4ad07cd927e3a99 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 14 Jul 2025 15:00:54 -0400 Subject: [PATCH 5/7] Satisfy Clippy --- crates/handlers/src/admin/v1/users/deactivate.rs | 6 +++--- crates/handlers/src/admin/v1/users/unlock.rs | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/crates/handlers/src/admin/v1/users/deactivate.rs b/crates/handlers/src/admin/v1/users/deactivate.rs index 815178f62..ddefbf44c 100644 --- a/crates/handlers/src/admin/v1/users/deactivate.rs +++ b/crates/handlers/src/admin/v1/users/deactivate.rs @@ -162,10 +162,10 @@ mod tests { // not locked assert_eq!( body["data"]["attributes"]["locked_at"], - if !skip_lock.unwrap_or(false) { - serde_json::json!(state.clock.now()) - } else { + if skip_lock.unwrap_or(false) { serde_json::Value::Null + } else { + serde_json::json!(state.clock.now()) } ); diff --git a/crates/handlers/src/admin/v1/users/unlock.rs b/crates/handlers/src/admin/v1/users/unlock.rs index b82b716a9..78c4f8cc2 100644 --- a/crates/handlers/src/admin/v1/users/unlock.rs +++ b/crates/handlers/src/admin/v1/users/unlock.rs @@ -103,7 +103,9 @@ pub async fn handler( .await? .ok_or(RouteError::NotFound(id))?; - let user = if !params.skip_reactivate { + let user = if params.skip_reactivate { + repo.user().unlock(user).await? + } else { // Call the homeserver synchronously to reactivate the user let mxid = homeserver.mxid(&user.username); homeserver @@ -111,8 +113,6 @@ pub async fn handler( .await .map_err(RouteError::Homeserver)?; repo.user().reactivate_and_unlock(user).await? - } else { - repo.user().unlock(user).await? }; repo.save().await?; @@ -222,10 +222,10 @@ mod tests { let skip_reactivate = skip_reactivate.unwrap_or(false); assert_eq!( body["data"]["attributes"]["deactivated_at"], - if !skip_reactivate { - serde_json::Value::Null - } else { + if skip_reactivate { serde_json::json!(state.clock.now()) + } else { + serde_json::Value::Null } ); From d8079751377dbf4a512717dbc4ced794f4a8f6a6 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 16 Jul 2025 09:16:06 -0400 Subject: [PATCH 6/7] 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. --- crates/cli/src/commands/manage.rs | 24 +++-- .../handlers/src/admin/v1/users/deactivate.rs | 96 +++++++------------ crates/handlers/src/admin/v1/users/lock.rs | 6 +- .../handlers/src/admin/v1/users/reactivate.rs | 3 +- crates/handlers/src/admin/v1/users/unlock.rs | 88 +++-------------- crates/handlers/src/graphql/mutations/user.rs | 18 +++- ...a18ded5613186104695524e85df9b6641ea4e.json | 14 --- crates/storage-pg/src/user/mod.rs | 36 +------ crates/storage/src/queue/tasks.rs | 4 +- crates/storage/src/user/mod.rs | 14 --- crates/tasks/src/user.rs | 15 +-- docs/api/spec.json | 45 +-------- frontend/schema.graphql | 4 + frontend/src/gql/graphql.ts | 2 + 14 files changed, 99 insertions(+), 270 deletions(-) delete mode 100644 crates/storage-pg/.sqlx/query-3e2d1ce1c7aba2952ed9c659972a18ded5613186104695524e85df9b6641ea4e.json diff --git a/crates/cli/src/commands/manage.rs b/crates/cli/src/commands/manage.rs index 97c019175..12560a9ac 100644 --- a/crates/cli/src/commands/manage.rs +++ b/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?; diff --git a/crates/handlers/src/admin/v1/users/deactivate.rs b/crates/handlers/src/admin/v1/users/deactivate.rs index ddefbf44c..ac4943f93 100644 --- a/crates/handlers/src/admin/v1/users/deactivate.rs +++ b/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>, _>(|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, id: UlidPathParam, - body: Option>, ) -> Result>, 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) { + #[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,7 +155,7 @@ 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", @@ -187,7 +163,7 @@ mod tests { "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 diff --git a/crates/handlers/src/admin/v1/users/lock.rs b/crates/handlers/src/admin/v1/users/lock.rs index 9db2a065a..ec8159532 100644 --- a/crates/handlers/src/admin/v1/users/lock.rs +++ b/crates/handlers/src/admin/v1/users/lock.rs @@ -72,15 +72,13 @@ pub async fn handler( id: UlidPathParam, ) -> Result>, 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?; diff --git a/crates/handlers/src/admin/v1/users/reactivate.rs b/crates/handlers/src/admin/v1/users/reactivate.rs index ad73c4dba..37b38c6b6 100644 --- a/crates/handlers/src/admin/v1/users/reactivate.rs +++ b/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>, _>(|t| { // In the samples, the third user is the one locked diff --git a/crates/handlers/src/admin/v1/users/unlock.rs b/crates/handlers/src/admin/v1/users/unlock.rs index 78c4f8cc2..5584f4a69 100644 --- a/crates/handlers/src/admin/v1/users/unlock.rs +++ b/crates/handlers/src/admin/v1/users/unlock.rs @@ -4,15 +4,10 @@ // SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial // Please see LICENSE files in the repository root for full details. -use std::sync::Arc; - use aide::{OperationIo, transform::TransformOperation}; -use axum::{Json, extract::State, response::IntoResponse}; +use axum::{Json, response::IntoResponse}; use hyper::StatusCode; use mas_axum_utils::record_error; -use mas_matrix::HomeserverConnection; -use schemars::JsonSchema; -use serde::Deserialize; use ulid::Ulid; use crate::{ @@ -31,9 +26,6 @@ pub enum RouteError { #[error(transparent)] Internal(Box), - #[error(transparent)] - Homeserver(anyhow::Error), - #[error("User ID {0} not found")] NotFound(Ulid), } @@ -43,37 +35,21 @@ impl_from_error_for_route!(mas_storage::RepositoryError); impl IntoResponse for RouteError { fn into_response(self) -> axum::response::Response { let error = ErrorResponse::from_error(&self); - let sentry_event_id = record_error!(self, Self::Internal(_) | Self::Homeserver(_)); + let sentry_event_id = record_error!(self, Self::Internal(_)); let status = match self { - Self::Internal(_) | Self::Homeserver(_) => StatusCode::INTERNAL_SERVER_ERROR, + Self::Internal(_) => StatusCode::INTERNAL_SERVER_ERROR, Self::NotFound(_) => StatusCode::NOT_FOUND, }; (status, sentry_event_id, Json(error)).into_response() } } -/// # JSON payload for the `POST /api/admin/v1/users/:id/unlock` endpoint -#[derive(Default, Deserialize, JsonSchema)] -#[serde(rename = "UnlockUserRequest")] -pub struct Request { - /// Whether to skip ensuring the user is active upon being unlocked. - #[serde(default)] - skip_reactivate: 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("unlockUser") .summary("Unlock a user") + .description("Calling this endpoint will lift restrictions on user actions that had imposed by locking. +This DOES NOT reactivate a deactivated user, which will remain unavailable until it is explicitly reactivated.") .tag("user") .response_with::<200, Json>, _>(|t| { // In the samples, the third user is the one locked @@ -91,11 +67,8 @@ pub fn doc(mut operation: TransformOperation) -> TransformOperation { #[tracing::instrument(name = "handler.admin.v1.users.unlock", skip_all)] pub async fn handler( CallContext { mut repo, .. }: CallContext, - State(homeserver): State>, id: UlidPathParam, - body: Option>, ) -> Result>, RouteError> { - let Json(params) = body.unwrap_or_default(); let id = *id; let user = repo .user() @@ -103,17 +76,7 @@ pub async fn handler( .await? .ok_or(RouteError::NotFound(id))?; - let user = if params.skip_reactivate { - repo.user().unlock(user).await? - } else { - // Call the homeserver synchronously to reactivate the user - let mxid = homeserver.mxid(&user.username); - homeserver - .reactivate_user(&mxid) - .await - .map_err(RouteError::Homeserver)?; - repo.user().reactivate_and_unlock(user).await? - }; + let user = repo.user().unlock(user).await?; repo.save().await?; @@ -169,7 +132,8 @@ mod tests { ); } - async fn test_unlock_deactivated_user_helper(pool: PgPool, skip_reactivate: Option) { + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_unlock_deactivated_user(pool: PgPool) { setup(); let mut state = TestState::from_pool(pool).await.unwrap(); let token = state.token_with_scope("urn:mas:admin").await; @@ -202,14 +166,9 @@ mod tests { let mx_user = state.homeserver_connection.query_user(&mxid).await.unwrap(); assert!(mx_user.deactivated); - let request = - Request::post(format!("/api/admin/v1/users/{}/unlock", user.id)).bearer(&token); - let request = match skip_reactivate { - None => request.empty(), - Some(skip_reactivate) => request.json(serde_json::json!({ - "skip_reactivate": skip_reactivate, - })), - }; + let request = Request::post(format!("/api/admin/v1/users/{}/unlock", user.id)) + .bearer(&token) + .empty(); let response = state.request(request).await; response.assert_status(StatusCode::OK); let body: serde_json::Value = response.json(); @@ -218,30 +177,13 @@ mod tests { body["data"]["attributes"]["locked_at"], serde_json::Value::Null ); - - let skip_reactivate = skip_reactivate.unwrap_or(false); + // The user should remain deactivated assert_eq!( body["data"]["attributes"]["deactivated_at"], - if skip_reactivate { - serde_json::json!(state.clock.now()) - } else { - serde_json::Value::Null - } + serde_json::json!(state.clock.now()) ); - - // Check whether the user should be reactivated on the homeserver let mx_user = state.homeserver_connection.query_user(&mxid).await.unwrap(); - assert_eq!(mx_user.deactivated, skip_reactivate); - } - - #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] - async fn test_unlock_deactivated_user(pool: PgPool) { - test_unlock_deactivated_user_helper(pool, Option::None).await; - } - - #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] - async fn test_unlock_deactivated_user_skip_reactivate(pool: PgPool) { - test_unlock_deactivated_user_helper(pool, Option::Some(true)).await; + assert!(mx_user.deactivated); } #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] diff --git a/crates/handlers/src/graphql/mutations/user.rs b/crates/handlers/src/graphql/mutations/user.rs index a5d7e0fc2..80be76b24 100644 --- a/crates/handlers/src/graphql/mutations/user.rs +++ b/crates/handlers/src/graphql/mutations/user.rs @@ -144,6 +144,9 @@ impl LockUserPayload { struct UnlockUserInput { /// The ID of the user to unlock user_id: ID, + + /// Reactivate the user if it had been deactivated + reactivate: Option, } /// The status of the `unlockUser` mutation. @@ -585,12 +588,19 @@ impl UserMutations { return Ok(UnlockUserPayload::NotFound); }; - // Call the homeserver synchronously to unlock the user - let mxid = matrix.mxid(&user.username); - matrix.reactivate_user(&mxid).await?; + let user = if input.reactivate.unwrap_or(false) { + // Call the homeserver synchronously to reactivate the user + let mxid = matrix.mxid(&user.username); + matrix.reactivate_user(&mxid).await?; + + // Now reactivate the user in our database + repo.user().reactivate(user).await? + } else { + user + }; // Now unlock the user in our database - let user = repo.user().reactivate_and_unlock(user).await?; + let user = repo.user().unlock(user).await?; repo.save().await?; diff --git a/crates/storage-pg/.sqlx/query-3e2d1ce1c7aba2952ed9c659972a18ded5613186104695524e85df9b6641ea4e.json b/crates/storage-pg/.sqlx/query-3e2d1ce1c7aba2952ed9c659972a18ded5613186104695524e85df9b6641ea4e.json deleted file mode 100644 index 738adae1c..000000000 --- a/crates/storage-pg/.sqlx/query-3e2d1ce1c7aba2952ed9c659972a18ded5613186104695524e85df9b6641ea4e.json +++ /dev/null @@ -1,14 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "\n UPDATE users\n SET deactivated_at = NULL, locked_at = NULL\n WHERE user_id = $1\n ", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "Uuid" - ] - }, - "nullable": [] - }, - "hash": "3e2d1ce1c7aba2952ed9c659972a18ded5613186104695524e85df9b6641ea4e" -} diff --git a/crates/storage-pg/src/user/mod.rs b/crates/storage-pg/src/user/mod.rs index a1f321e7c..6d03e9bf7 100644 --- a/crates/storage-pg/src/user/mod.rs +++ b/crates/storage-pg/src/user/mod.rs @@ -379,7 +379,7 @@ impl UserRepository for PgUserRepository<'_> { DatabaseError::ensure_affected_rows(&res, 1)?; - user.deactivated_at = Some(user.created_at); + user.deactivated_at = Some(deactivated_at); Ok(user) } @@ -417,40 +417,6 @@ impl UserRepository for PgUserRepository<'_> { Ok(user) } - #[tracing::instrument( - name = "db.user.reactivate_and_unlock", - skip_all, - fields( - db.query.text, - %user.id, - ), - err, - )] - async fn reactivate_and_unlock(&mut self, mut user: User) -> Result { - if user.deactivated_at.is_none() && user.locked_at.is_none() { - return Ok(user); - } - - let res = sqlx::query!( - r#" - UPDATE users - SET deactivated_at = NULL, locked_at = NULL - WHERE user_id = $1 - "#, - Uuid::from(user.id), - ) - .traced() - .execute(&mut *self.conn) - .await?; - - DatabaseError::ensure_affected_rows(&res, 1)?; - - user.deactivated_at = None; - user.locked_at = None; - - Ok(user) - } - #[tracing::instrument( name = "db.user.set_can_request_admin", skip_all, diff --git a/crates/storage/src/queue/tasks.rs b/crates/storage/src/queue/tasks.rs index f59971ba4..eb16f6e29 100644 --- a/crates/storage/src/queue/tasks.rs +++ b/crates/storage/src/queue/tasks.rs @@ -257,14 +257,14 @@ impl InsertableJob for DeactivateUserJob { const QUEUE_NAME: &'static str = "deactivate-user"; } -/// A job to reactivate and unlock a user +/// A job to reactivate a user #[derive(Serialize, Deserialize, Debug, Clone)] pub struct ReactivateUserJob { user_id: Ulid, } impl ReactivateUserJob { - /// Create a new job to reactivate and unlock a user + /// Create a new job to reactivate a user /// /// # Parameters /// diff --git a/crates/storage/src/user/mod.rs b/crates/storage/src/user/mod.rs index f990af3e2..f864157b1 100644 --- a/crates/storage/src/user/mod.rs +++ b/crates/storage/src/user/mod.rs @@ -257,19 +257,6 @@ pub trait UserRepository: Send + Sync { /// Returns [`Self::Error`] if the underlying repository fails async fn reactivate(&mut self, user: User) -> Result; - /// Reactivate and unlock a [`User`] - /// - /// Returns the reactivated and unlocked [`User`] - /// - /// # Parameters - /// - /// * `user`: The [`User`] to reactivate and unlock - /// - /// # Errors - /// - /// Returns [`Self::Error`] if the underlying repository fails - async fn reactivate_and_unlock(&mut self, user: User) -> Result; - /// Set whether a [`User`] can request admin /// /// Returns the [`User`] with the new `can_request_admin` value @@ -342,7 +329,6 @@ repository_impl!(UserRepository: async fn unlock(&mut self, user: User) -> Result; async fn deactivate(&mut self, clock: &dyn Clock, user: User) -> Result; async fn reactivate(&mut self, user: User) -> Result; - async fn reactivate_and_unlock(&mut self, user: User) -> Result; async fn set_can_request_admin( &mut self, user: User, diff --git a/crates/tasks/src/user.rs b/crates/tasks/src/user.rs index f7ce2f9ff..245733aa5 100644 --- a/crates/tasks/src/user.rs +++ b/crates/tasks/src/user.rs @@ -41,14 +41,7 @@ impl RunnableJob for DeactivateUserJob { .context("User not found") .map_err(JobError::fail)?; - // Let's first lock & deactivate the user - let user = repo - .user() - .lock(clock, user) - .await - .context("Failed to lock user") - .map_err(JobError::retry)?; - + // Let's first deactivate the user let user = repo .user() .deactivate(clock, user) @@ -137,11 +130,11 @@ impl RunnableJob for ReactivateUserJob { .await .map_err(JobError::retry)?; - // We want to unlock the user from our side only once it has been reactivated on - // the homeserver + // We want to reactivate the user from our side only once it has been + // reactivated on the homeserver let _user = repo .user() - .reactivate_and_unlock(user) + .reactivate(user) .await .map_err(JobError::retry)?; repo.save().await.map_err(JobError::retry)?; diff --git a/docs/api/spec.json b/docs/api/spec.json index 28d394254..b30155b96 100644 --- a/docs/api/spec.json +++ b/docs/api/spec.json @@ -1345,7 +1345,7 @@ "user" ], "summary": "Deactivate a user", - "description": "Calling this endpoint will lock and deactivate the user, preventing them from doing any action.\nThis 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.\nThis invalidates any existing session, and will ask the homeserver to make them leave all rooms.", "operationId": "deactivateUser", "parameters": [ { @@ -1359,15 +1359,6 @@ "style": "simple" } ], - "requestBody": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/DeactivateUserRequest" - } - } - } - }, "responses": { "200": { "description": "User was deactivated", @@ -1424,7 +1415,7 @@ "user" ], "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.\nThis DOES NOT unlock a locked user, which is still prevented from doing any action until it is explicitly unlocked.", "operationId": "reactivateUser", "parameters": [ { @@ -1564,6 +1555,7 @@ "user" ], "summary": "Unlock a user", + "description": "Calling this endpoint will lift restrictions on user actions that had imposed by locking.\nThis DOES NOT reactivate a deactivated user, which will remain unavailable until it is explicitly reactivated.", "operationId": "unlockUser", "parameters": [ { @@ -1577,15 +1569,6 @@ "style": "simple" } ], - "requestBody": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/UnlockUserRequest" - } - } - } - }, "responses": { "200": { "description": "User was unlocked", @@ -3960,28 +3943,6 @@ } } }, - "DeactivateUserRequest": { - "title": "JSON payload for the `POST /api/admin/v1/users/:id/deactivate` endpoint", - "type": "object", - "properties": { - "skip_lock": { - "description": "Whether to skip locking the user before deactivation.", - "default": false, - "type": "boolean" - } - } - }, - "UnlockUserRequest": { - "title": "JSON payload for the `POST /api/admin/v1/users/:id/unlock` endpoint", - "type": "object", - "properties": { - "skip_reactivate": { - "description": "Whether to skip ensuring the user is active upon being unlocked.", - "default": false, - "type": "boolean" - } - } - }, "UserEmailFilter": { "type": "object", "properties": { diff --git a/frontend/schema.graphql b/frontend/schema.graphql index 0e71a519d..993a554a3 100644 --- a/frontend/schema.graphql +++ b/frontend/schema.graphql @@ -1842,6 +1842,10 @@ input UnlockUserInput { The ID of the user to unlock """ userId: ID! + """ + Reactivate the user if it had been deactivated + """ + reactivate: Boolean } """ diff --git a/frontend/src/gql/graphql.ts b/frontend/src/gql/graphql.ts index ff482af6c..b07b89cf5 100644 --- a/frontend/src/gql/graphql.ts +++ b/frontend/src/gql/graphql.ts @@ -1347,6 +1347,8 @@ export type StartEmailAuthenticationStatus = /** The input for the `unlockUser` mutation. */ export type UnlockUserInput = { + /** Reactivate the user if it had been deactivated */ + reactivate?: InputMaybe; /** The ID of the user to unlock */ userId: Scalars['ID']['input']; }; From 8ac2770cf7c2903721329c8f16630fd2ca7537cd Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 16 Jul 2025 13:49:00 -0400 Subject: [PATCH 7/7] Revert GraphQL's unlock to also reactivate Unlike the CLI and admin API, leave the behaviour of the GraphQL's unlock handler unchanged from before, so as to not break internal tooling that depends on it. Also update its documentation description to make note of the fact that it reactivates in addition to unlocks. --- crates/handlers/src/graphql/mutations/user.rs | 21 ++++++------------- frontend/schema.graphql | 6 +----- frontend/src/gql/graphql.ts | 4 +--- 3 files changed, 8 insertions(+), 23 deletions(-) diff --git a/crates/handlers/src/graphql/mutations/user.rs b/crates/handlers/src/graphql/mutations/user.rs index 80be76b24..26352db81 100644 --- a/crates/handlers/src/graphql/mutations/user.rs +++ b/crates/handlers/src/graphql/mutations/user.rs @@ -144,9 +144,6 @@ impl LockUserPayload { struct UnlockUserInput { /// The ID of the user to unlock user_id: ID, - - /// Reactivate the user if it had been deactivated - reactivate: Option, } /// The status of the `unlockUser` mutation. @@ -566,7 +563,7 @@ impl UserMutations { Ok(LockUserPayload::Locked(user)) } - /// Unlock a user. This is only available to administrators. + /// Unlock and reactivate a user. This is only available to administrators. async fn unlock_user( &self, ctx: &Context<'_>, @@ -588,18 +585,12 @@ impl UserMutations { return Ok(UnlockUserPayload::NotFound); }; - let user = if input.reactivate.unwrap_or(false) { - // Call the homeserver synchronously to reactivate the user - let mxid = matrix.mxid(&user.username); - matrix.reactivate_user(&mxid).await?; + // Call the homeserver synchronously to reactivate the user + let mxid = matrix.mxid(&user.username); + matrix.reactivate_user(&mxid).await?; - // Now reactivate the user in our database - repo.user().reactivate(user).await? - } else { - user - }; - - // Now unlock the user in our database + // Now reactivate & unlock the user in our database + let user = repo.user().reactivate(user).await?; let user = repo.user().unlock(user).await?; repo.save().await?; diff --git a/frontend/schema.graphql b/frontend/schema.graphql index 993a554a3..99da32010 100644 --- a/frontend/schema.graphql +++ b/frontend/schema.graphql @@ -886,7 +886,7 @@ type Mutation { """ lockUser(input: LockUserInput!): LockUserPayload! """ - Unlock a user. This is only available to administrators. + Unlock and reactivate a user. This is only available to administrators. """ unlockUser(input: UnlockUserInput!): UnlockUserPayload! """ @@ -1842,10 +1842,6 @@ input UnlockUserInput { The ID of the user to unlock """ userId: ID! - """ - Reactivate the user if it had been deactivated - """ - reactivate: Boolean } """ diff --git a/frontend/src/gql/graphql.ts b/frontend/src/gql/graphql.ts index b07b89cf5..b6f357170 100644 --- a/frontend/src/gql/graphql.ts +++ b/frontend/src/gql/graphql.ts @@ -604,7 +604,7 @@ export type Mutation = { setPrimaryEmail: SetPrimaryEmailPayload; /** Start a new email authentication flow */ startEmailAuthentication: StartEmailAuthenticationPayload; - /** Unlock a user. This is only available to administrators. */ + /** Unlock and reactivate a user. This is only available to administrators. */ unlockUser: UnlockUserPayload; }; @@ -1347,8 +1347,6 @@ export type StartEmailAuthenticationStatus = /** The input for the `unlockUser` mutation. */ export type UnlockUserInput = { - /** Reactivate the user if it had been deactivated */ - reactivate?: InputMaybe; /** The ID of the user to unlock */ userId: Scalars['ID']['input']; };