diff --git a/changelog.d/+55af97cf8.feature.md b/changelog.d/+55af97cf8.feature.md new file mode 100644 index 000000000..ff32dead0 --- /dev/null +++ b/changelog.d/+55af97cf8.feature.md @@ -0,0 +1 @@ +Fixed MSC4323 suspend/lock admin endpoints: the lock endpoint now locks instead of suspending users, and error codes match the ratified spec. Contributed by @eleboucher. diff --git a/src/api/client/admin/lock.rs b/src/api/client/admin/lock.rs index 81b95dd61..abefe8d39 100644 --- a/src/api/client/admin/lock.rs +++ b/src/api/client/admin/lock.rs @@ -1,6 +1,5 @@ use axum::extract::State; use conduwuit::{Err, Result}; -use futures::future::{join, join3}; use ruma::api::client::admin::{is_user_locked, lock_user}; use crate::Ruma; @@ -12,17 +11,32 @@ pub(crate) async fn get_lock_status( State(services): State, body: Ruma, ) -> Result { - let (admin, status) = join( - services.users.is_admin(body.identity.expect_sender_user()?), - services.users.status(&body.user_id), - ) - .await; + let sender_user = body.identity.expect_sender_user()?; + let sender_is_admin = services.users.is_admin(sender_user).await; - if !admin { + if !sender_is_admin { return Err!(Request(Forbidden("Only server administrators can use this endpoint"))); } - status.ensure_active()?; + if body.user_id != *sender_user { + let target_is_admin = services.users.is_admin(&body.user_id).await; + + if target_is_admin { + return Err!(Request(Forbidden( + "You cannot view the lock status of another server administrator" + ))); + } + } + + if !services.globals.user_is_local(&body.user_id) { + return Err!(Request(InvalidParam("User does not belong to the local server"))); + } + + let status = services.users.status(&body.user_id).await; + + if !status.is_active() { + return Err!(Request(NotFound("This account does not exist"))); + } Ok(is_user_locked::v1::Response::new( services.users.is_locked(&body.user_id).await?, @@ -37,28 +51,31 @@ pub(crate) async fn put_lock_status( body: Ruma, ) -> Result { let sender_user = body.identity.expect_sender_user()?; + let sender_is_admin = services.users.is_admin(sender_user).await; - let (sender_admin, status, target_admin) = join3( - services.users.is_admin(sender_user), - services.users.status(&body.user_id), - services.users.is_admin(&body.user_id), - ) - .await; - - if !sender_admin { + if !sender_is_admin { return Err!(Request(Forbidden("Only server administrators can use this endpoint"))); } - status.ensure_active()?; - if body.user_id == *sender_user { return Err!(Request(Forbidden("You cannot lock yourself"))); } - if target_admin { + let target_is_admin = services.users.is_admin(&body.user_id).await; + if target_is_admin { return Err!(Request(Forbidden("You cannot lock another server administrator"))); } + if !services.globals.user_is_local(&body.user_id) { + return Err!(Request(InvalidParam("User does not belong to the local server"))); + } + + let status = services.users.status(&body.user_id).await; + + if !status.is_active() { + return Err!(Request(NotFound("This account does not exist"))); + } + if services.users.is_locked(&body.user_id).await? == body.locked { // No change return Ok(lock_user::v1::Response::new(body.locked)); @@ -67,16 +84,16 @@ pub(crate) async fn put_lock_status( let action = if body.locked { services .users - .suspend_account(&body.user_id, sender_user) + .lock_account(&body.user_id, sender_user) .await; "locked" } else { - services.users.unsuspend_account(&body.user_id).await; + services.users.unlock_account(&body.user_id).await; "unlocked" }; if services.config.admin_room_notices { - // Notify the admin room that an account has been un/suspended + // Notify the admin room that an account has been un/locked services .admin .send_text(&format!("{} has been {} by {}.", body.user_id, action, sender_user)) diff --git a/src/api/client/admin/suspend.rs b/src/api/client/admin/suspend.rs index fac32f12e..f8975257b 100644 --- a/src/api/client/admin/suspend.rs +++ b/src/api/client/admin/suspend.rs @@ -1,6 +1,5 @@ use axum::extract::State; use conduwuit::{Err, Result}; -use futures::future::{join, join3}; use ruma::api::client::admin::{is_user_suspended, suspend_user}; use crate::Ruma; @@ -12,17 +11,32 @@ pub(crate) async fn get_suspended_status( State(services): State, body: Ruma, ) -> Result { - let (admin, status) = join( - services.users.is_admin(body.identity.expect_sender_user()?), - services.users.status(&body.user_id), - ) - .await; + let sender_user = body.identity.expect_sender_user()?; + let sender_is_admin = services.users.is_admin(sender_user).await; - if !admin { + if !sender_is_admin { return Err!(Request(Forbidden("Only server administrators can use this endpoint"))); } - status.ensure_active()?; + if body.user_id != *sender_user { + let target_is_admin = services.users.is_admin(&body.user_id).await; + + if target_is_admin { + return Err!(Request(Forbidden( + "You cannot view the suspension status of another server administrator" + ))); + } + } + + if !services.globals.user_is_local(&body.user_id) { + return Err!(Request(InvalidParam("User does not belong to the local server"))); + } + + let status = services.users.status(&body.user_id).await; + + if !status.is_active() { + return Err!(Request(NotFound("This account does not exist"))); + } Ok(is_user_suspended::v1::Response::new( services.users.is_suspended(&body.user_id).await?, @@ -37,28 +51,31 @@ pub(crate) async fn put_suspended_status( body: Ruma, ) -> Result { let sender_user = body.identity.expect_sender_user()?; + let sender_is_admin = services.users.is_admin(sender_user).await; - let (sender_admin, status, target_admin) = join3( - services.users.is_admin(sender_user), - services.users.status(&body.user_id), - services.users.is_admin(&body.user_id), - ) - .await; - - if !sender_admin { + if !sender_is_admin { return Err!(Request(Forbidden("Only server administrators can use this endpoint"))); } - status.ensure_active()?; - if body.user_id == *sender_user { return Err!(Request(Forbidden("You cannot suspend yourself"))); } - if target_admin { + let target_is_admin = services.users.is_admin(&body.user_id).await; + if target_is_admin { return Err!(Request(Forbidden("You cannot suspend another server administrator"))); } + if !services.globals.user_is_local(&body.user_id) { + return Err!(Request(InvalidParam("User does not belong to the local server"))); + } + + let status = services.users.status(&body.user_id).await; + + if !status.is_active() { + return Err!(Request(NotFound("This account does not exist"))); + } + if services.users.is_suspended(&body.user_id).await? == body.suspended { // No change return Ok(suspend_user::v1::Response::new(body.suspended));