From c526095d4f0c5c365a848ac546a4ae8e00262f71 Mon Sep 17 00:00:00 2001 From: Zomatree Date: Thu, 5 Jun 2025 18:59:10 +0100 Subject: [PATCH 1/9] feat: initial bulk role reorder route --- crates/core/database/src/events/client.rs | 3 + crates/core/models/src/v0/servers.rs | 6 ++ crates/delta/src/routes/servers/mod.rs | 4 +- crates/delta/src/routes/servers/roles_edit.rs | 11 +-- .../routes/servers/roles_edit_positions.rs | 80 +++++++++++++++++++ 5 files changed, 94 insertions(+), 10 deletions(-) create mode 100644 crates/delta/src/routes/servers/roles_edit_positions.rs diff --git a/crates/core/database/src/events/client.rs b/crates/core/database/src/events/client.rs index af480d5a..2d0829a4 100644 --- a/crates/core/database/src/events/client.rs +++ b/crates/core/database/src/events/client.rs @@ -165,6 +165,9 @@ pub enum EventV1 { /// Server role deleted ServerRoleDelete { id: String, role_id: String }, + /// Server roles ranks updated + ServerRoleRanksUpdate { ranks: Vec }, + /// Update existing user UserUpdate { id: String, diff --git a/crates/core/models/src/v0/servers.rs b/crates/core/models/src/v0/servers.rs index ae5a50a2..9e181176 100644 --- a/crates/core/models/src/v0/servers.rs +++ b/crates/core/models/src/v0/servers.rs @@ -267,6 +267,7 @@ auto_derived!( pub hoist: Option, /// Ranking position /// + /// **Deprecated** - use the edit server role positions route. /// Smaller values take priority. pub rank: Option, /// Fields to remove from role object @@ -286,4 +287,9 @@ auto_derived!( /// Whether to not send a leave message pub leave_silently: Option, } + + /// New role positions + pub struct DataEditRoleRanks { + pub ranks: Vec, + } ); diff --git a/crates/delta/src/routes/servers/mod.rs b/crates/delta/src/routes/servers/mod.rs index a7553625..b656b401 100644 --- a/crates/delta/src/routes/servers/mod.rs +++ b/crates/delta/src/routes/servers/mod.rs @@ -18,6 +18,7 @@ mod roles_create; mod roles_delete; mod roles_edit; mod roles_fetch; +mod roles_edit_positions; mod server_ack; mod server_create; mod server_delete; @@ -47,6 +48,7 @@ pub fn routes() -> (Vec, OpenApi) { roles_delete::delete, permissions_set::set_role_permission, permissions_set_default::set_default_permissions, - emoji_list::list_emoji + emoji_list::list_emoji, + roles_edit_positions::edit_role_positions ] } diff --git a/crates/delta/src/routes/servers/roles_edit.rs b/crates/delta/src/routes/servers/roles_edit.rs index 72ec6635..af4e0cac 100644 --- a/crates/delta/src/routes/servers/roles_edit.rs +++ b/crates/delta/src/routes/servers/roles_edit.rs @@ -45,22 +45,15 @@ pub async fn edit( name, colour, hoist, - rank, remove, + // rank is deprecated + .. } = data; - // Prevent us from moving a role above other roles - if let Some(rank) = &rank { - if rank <= &member_rank { - return Err(create_error!(NotElevated)); - } - } - let partial = PartialRole { name, colour, hoist, - rank, ..Default::default() }; diff --git a/crates/delta/src/routes/servers/roles_edit_positions.rs b/crates/delta/src/routes/servers/roles_edit_positions.rs new file mode 100644 index 00000000..5a7e4438 --- /dev/null +++ b/crates/delta/src/routes/servers/roles_edit_positions.rs @@ -0,0 +1,80 @@ +use revolt_database::{events::client::EventV1, util::{permissions::DatabasePermissionQuery, reference::Reference}, Database, PartialRole, User}; +use revolt_permissions::{calculate_server_permissions, ChannelPermission}; +use rocket::{serde::json::Json, State}; +use revolt_result::{create_error, Result}; +use revolt_models::v0; + +/// # Edits server roles ranks +/// +/// Edit's server role's ranks. +#[openapi(tag = "Server Permissions")] +#[patch("//roles/ranks", data = "")] +pub async fn edit_role_ranks( + db: &State, + user: User, + target: Reference, + data: Json, +) -> Result> { + let data = data.into_inner(); + + let mut server = target.as_server(db).await?; + let mut query = DatabasePermissionQuery::new(db, &user).server(&server); + calculate_server_permissions(&mut query) + .await + .throw_if_lacking_channel_permission(ChannelPermission::ManageRole)?; + + let existing_order = server.roles.keys().cloned().collect::>(); + let new_order = data.ranks.clone().into_iter().collect::>(); + + // verify all roles are in the new ordering + if data.ranks.len() != server.roles.len() && server.roles.iter().all(|(id, _)| data.ranks.contains(id)) { + return Err(create_error!(InvalidOperation)) + } + + let member_top_rank = query.get_member_rank(); + + // find all roles above the member which we should not be able to reorder + let cant_modify = server.roles.clone() + .into_iter() + .filter(|(_, role)| if let Some(top_rank) = member_top_rank { role.rank >= top_rank } else { true }) + .collect::>(); + + // check if any roles which we cant reorder have tried to been reordered + if cant_modify.iter() + .any(|(id, _)| { + let existing_rank = existing_order.iter().position(|existing_id| id == existing_id); + let new_rank = new_order.iter().position(|new_id| id == new_id); + + existing_rank != new_rank + }) + { + return Err(create_error!(NotElevated)) + } + + // update the roles which have had their rank changed + for (role_id, role) in &mut server.roles { + let new_rank = new_order.iter().position(|id| id == role_id).unwrap() as i64; // unwrap - we check if its in the data earlier on + + if new_rank != role.rank { + // cant use Role::update here because we dont want to publish seperate role update events + let partial = PartialRole { + rank: Some(new_rank), + ..Default::default() + }; + + db.update_role(&server.id, role_id, &partial, Vec::new()).await?; + + role.apply_options(PartialRole { + rank: Some(new_rank), + ..Default::default() + }); + } + } + + // publish bulk update event + EventV1::ServerRoleRanksUpdate { + ranks: new_order + }.p(server.id.clone()).await; + + Ok(Json(server.into())) +} From 1b2c7b2fa1b9873e3ffdfd305413826356ae95ab Mon Sep 17 00:00:00 2001 From: Zomatree Date: Thu, 5 Jun 2025 22:37:10 +0100 Subject: [PATCH 2/9] feat: add roles migration --- .../admin_migrations/ops/mongodb/scripts.rs | 42 +++++++++++++++++-- crates/delta/src/routes/servers/mod.rs | 2 +- .../routes/servers/roles_edit_positions.rs | 4 +- 3 files changed, 42 insertions(+), 6 deletions(-) diff --git a/crates/core/database/src/models/admin_migrations/ops/mongodb/scripts.rs b/crates/core/database/src/models/admin_migrations/ops/mongodb/scripts.rs index 0f58082a..e671e43d 100644 --- a/crates/core/database/src/models/admin_migrations/ops/mongodb/scripts.rs +++ b/crates/core/database/src/models/admin_migrations/ops/mongodb/scripts.rs @@ -4,8 +4,7 @@ use crate::{ mongodb::{ bson::{doc, from_bson, from_document, to_document, Bson, DateTime, Document}, options::FindOptions, - }, - AbstractChannels, AbstractServers, Channel, Invite, MongoDb, User, DISCRIMINATOR_SEARCH_SPACE, + }, AbstractChannels, AbstractServers, Channel, Invite, MongoDb, Server, User, DISCRIMINATOR_SEARCH_SPACE }; use bson::{oid::ObjectId, to_bson}; use futures::StreamExt; @@ -22,7 +21,7 @@ struct MigrationInfo { revision: i32, } -pub const LATEST_REVISION: i32 = 41; // MUST BE +1 to last migration +pub const LATEST_REVISION: i32 = 42; // MUST BE +1 to last migration pub async fn migrate_database(db: &MongoDb) { let migrations = db.col::("migrations"); @@ -1165,6 +1164,43 @@ pub async fn run_migrations(db: &MongoDb, revision: i32) -> i32 { .expect("failed to update users"); } + if revision <= 41 { + info!("Running migration [revision 41 / 05-06-2025]: convert role ranks to uniform numbers."); + + let mut servers = db.db() + .collection::("servers") + .find(doc! { + "roles": { + "$exists": true, + "$ne": [] + } + }) + .await + .unwrap() + .filter_map(|s| async { s.ok() }) + .boxed(); + + while let Some(mut server) = servers.next().await { + let mut ordered_roles = server.roles.clone().into_iter().collect::>(); + ordered_roles.sort_by(|(_, role_a), (_, role_b)| role_a.rank.cmp(&role_b.rank)); + let ordered_roles = ordered_roles.into_iter().map(|(id, _)| id).collect::>(); + + for (id, role) in server.roles.iter_mut() { + role.rank = ordered_roles.iter().position(|x| id == x).unwrap() as i64; + } + + db.db().collection::("servers") + .update_one( + doc! { "_id": &server.id }, + doc! { "$set": { + "roles": bson::to_bson(&server.roles).unwrap() + } } + ) + .await + .unwrap(); + } + } + // Reminder to update LATEST_REVISION when adding new migrations. LATEST_REVISION.max(revision) } diff --git a/crates/delta/src/routes/servers/mod.rs b/crates/delta/src/routes/servers/mod.rs index b656b401..066fd32a 100644 --- a/crates/delta/src/routes/servers/mod.rs +++ b/crates/delta/src/routes/servers/mod.rs @@ -49,6 +49,6 @@ pub fn routes() -> (Vec, OpenApi) { permissions_set::set_role_permission, permissions_set_default::set_default_permissions, emoji_list::list_emoji, - roles_edit_positions::edit_role_positions + roles_edit_positions::edit_role_ranks ] } diff --git a/crates/delta/src/routes/servers/roles_edit_positions.rs b/crates/delta/src/routes/servers/roles_edit_positions.rs index 5a7e4438..8edc8d7f 100644 --- a/crates/delta/src/routes/servers/roles_edit_positions.rs +++ b/crates/delta/src/routes/servers/roles_edit_positions.rs @@ -8,7 +8,7 @@ use revolt_models::v0; /// /// Edit's server role's ranks. #[openapi(tag = "Server Permissions")] -#[patch("//roles/ranks", data = "")] +#[patch("//roles/ranks", data = "", rank = 1)] pub async fn edit_role_ranks( db: &State, user: User, @@ -36,7 +36,7 @@ pub async fn edit_role_ranks( // find all roles above the member which we should not be able to reorder let cant_modify = server.roles.clone() .into_iter() - .filter(|(_, role)| if let Some(top_rank) = member_top_rank { role.rank >= top_rank } else { true }) + .filter(|(_, role)| if let Some(top_rank) = member_top_rank { role.rank <= top_rank } else { true }) .collect::>(); // check if any roles which we cant reorder have tried to been reordered From e00603f27665cb3857ba6b249bff99a0cc5a6a3d Mon Sep 17 00:00:00 2001 From: Zomatree Date: Thu, 5 Jun 2025 23:09:27 +0100 Subject: [PATCH 3/9] fix: route ranking --- crates/delta/src/routes/servers/roles_edit.rs | 2 +- .../routes/servers/roles_edit_positions.rs | 35 ++++++++++--------- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/crates/delta/src/routes/servers/roles_edit.rs b/crates/delta/src/routes/servers/roles_edit.rs index af4e0cac..78143bf2 100644 --- a/crates/delta/src/routes/servers/roles_edit.rs +++ b/crates/delta/src/routes/servers/roles_edit.rs @@ -12,7 +12,7 @@ use validator::Validate; /// /// Edit a role by its id. #[openapi(tag = "Server Permissions")] -#[patch("//roles/", data = "")] +#[patch("//roles/", data = "", rank = 1)] pub async fn edit( db: &State, user: User, diff --git a/crates/delta/src/routes/servers/roles_edit_positions.rs b/crates/delta/src/routes/servers/roles_edit_positions.rs index 8edc8d7f..e62e7023 100644 --- a/crates/delta/src/routes/servers/roles_edit_positions.rs +++ b/crates/delta/src/routes/servers/roles_edit_positions.rs @@ -8,7 +8,7 @@ use revolt_models::v0; /// /// Edit's server role's ranks. #[openapi(tag = "Server Permissions")] -#[patch("//roles/ranks", data = "", rank = 1)] +#[patch("//roles/ranks", data = "")] pub async fn edit_role_ranks( db: &State, user: User, @@ -31,24 +31,27 @@ pub async fn edit_role_ranks( return Err(create_error!(InvalidOperation)) } - let member_top_rank = query.get_member_rank(); + // dont have to check what the user cant modify if they are the server owner + if server.owner != user.id { + let member_top_rank = query.get_member_rank(); - // find all roles above the member which we should not be able to reorder - let cant_modify = server.roles.clone() - .into_iter() - .filter(|(_, role)| if let Some(top_rank) = member_top_rank { role.rank <= top_rank } else { true }) - .collect::>(); + // find all roles above the member which we should not be able to reorder + let cant_modify = server.roles.clone() + .into_iter() + .filter(|(_, role)| if let Some(top_rank) = member_top_rank { role.rank <= top_rank } else { true }) + .collect::>(); - // check if any roles which we cant reorder have tried to been reordered - if cant_modify.iter() - .any(|(id, _)| { - let existing_rank = existing_order.iter().position(|existing_id| id == existing_id); - let new_rank = new_order.iter().position(|new_id| id == new_id); + // check if any roles which we cant reorder have tried to been reordered + if cant_modify.iter() + .any(|(id, _)| { + let existing_rank = existing_order.iter().position(|existing_id| id == existing_id); + let new_rank = new_order.iter().position(|new_id| id == new_id); - existing_rank != new_rank - }) - { - return Err(create_error!(NotElevated)) + existing_rank != new_rank + }) + { + return Err(create_error!(NotElevated)) + } } // update the roles which have had their rank changed From 77daf82b948395406ec75a4b88ac28995a273762 Mon Sep 17 00:00:00 2001 From: Zomatree Date: Sun, 8 Jun 2025 01:40:48 +0100 Subject: [PATCH 4/9] chore: refactor new role rankings code --- .../admin_migrations/ops/mongodb/scripts.rs | 29 ++++++++---- .../core/database/src/models/servers/model.rs | 27 +++++++++++ .../routes/servers/roles_edit_positions.rs | 45 +++---------------- 3 files changed, 55 insertions(+), 46 deletions(-) diff --git a/crates/core/database/src/models/admin_migrations/ops/mongodb/scripts.rs b/crates/core/database/src/models/admin_migrations/ops/mongodb/scripts.rs index e671e43d..40efe91a 100644 --- a/crates/core/database/src/models/admin_migrations/ops/mongodb/scripts.rs +++ b/crates/core/database/src/models/admin_migrations/ops/mongodb/scripts.rs @@ -1,4 +1,4 @@ -use std::{collections::HashSet, ops::BitXor, time::Duration}; +use std::{collections::{HashMap, HashSet}, ops::BitXor, time::Duration}; use crate::{ mongodb::{ @@ -1167,6 +1167,19 @@ pub async fn run_migrations(db: &MongoDb, revision: i32) -> i32 { if revision <= 41 { info!("Running migration [revision 41 / 05-06-2025]: convert role ranks to uniform numbers."); + #[derive(Serialize, Deserialize, Clone)] + struct Role { + pub rank: i64, + } + + #[derive(Serialize, Deserialize, Clone)] + struct Server { + #[serde(rename = "_id")] + pub id: String, + #[serde(default = "HashMap::::new")] + pub roles: HashMap, + } + let mut servers = db.db() .collection::("servers") .find(doc! { @@ -1180,21 +1193,21 @@ pub async fn run_migrations(db: &MongoDb, revision: i32) -> i32 { .filter_map(|s| async { s.ok() }) .boxed(); - while let Some(mut server) = servers.next().await { + while let Some(server) = servers.next().await { let mut ordered_roles = server.roles.clone().into_iter().collect::>(); ordered_roles.sort_by(|(_, role_a), (_, role_b)| role_a.rank.cmp(&role_b.rank)); let ordered_roles = ordered_roles.into_iter().map(|(id, _)| id).collect::>(); - for (id, role) in server.roles.iter_mut() { - role.rank = ordered_roles.iter().position(|x| id == x).unwrap() as i64; - } + let mut doc = doc! {}; + + for id in server.roles.keys() { + doc.insert(format!("roles.{id}.rank"), ordered_roles.iter().position(|x| id == x).unwrap() as i64); + }; db.db().collection::("servers") .update_one( doc! { "_id": &server.id }, - doc! { "$set": { - "roles": bson::to_bson(&server.roles).unwrap() - } } + doc! { "$set": doc } ) .await .unwrap(); diff --git a/crates/core/database/src/models/servers/model.rs b/crates/core/database/src/models/servers/model.rs index c69b1196..65d7d282 100644 --- a/crates/core/database/src/models/servers/model.rs +++ b/crates/core/database/src/models/servers/model.rs @@ -253,6 +253,33 @@ impl Server { Err(create_error!(NotFound)) } } + + /// reorders the server's roles rankings + pub async fn update_role_rankings(&mut self, db: &Database, new_order: Vec) -> Result<()> { + // new_order must always contain every role + debug_assert_eq!(self.roles.len(), new_order.len()); + + // set the role's ranks to the positions in the vec + for (rank, id) in new_order.iter().enumerate() { + self.roles.get_mut(id).unwrap().rank = rank as i64; + } + + db.update_server( + &self.id, + &PartialServer { + roles: Some(self.roles.clone()), + ..Default::default() + }, + Vec::new() + ).await?; + + // publish bulk update event + EventV1::ServerRoleRanksUpdate { + ranks: new_order + }.p(self.id.clone()).await; + + Ok(()) + } } impl Role { diff --git a/crates/delta/src/routes/servers/roles_edit_positions.rs b/crates/delta/src/routes/servers/roles_edit_positions.rs index e62e7023..7e3b4d73 100644 --- a/crates/delta/src/routes/servers/roles_edit_positions.rs +++ b/crates/delta/src/routes/servers/roles_edit_positions.rs @@ -1,4 +1,4 @@ -use revolt_database::{events::client::EventV1, util::{permissions::DatabasePermissionQuery, reference::Reference}, Database, PartialRole, User}; +use revolt_database::{events::client::EventV1, util::{permissions::DatabasePermissionQuery, reference::Reference}, Database, PartialRole, PartialServer, User}; use revolt_permissions::{calculate_server_permissions, ChannelPermission}; use rocket::{serde::json::Json, State}; use revolt_result::{create_error, Result}; @@ -35,49 +35,18 @@ pub async fn edit_role_ranks( if server.owner != user.id { let member_top_rank = query.get_member_rank(); - // find all roles above the member which we should not be able to reorder - let cant_modify = server.roles.clone() - .into_iter() - .filter(|(_, role)| if let Some(top_rank) = member_top_rank { role.rank <= top_rank } else { true }) - .collect::>(); - + // find all roles above the member which we should not be able to reorder then // check if any roles which we cant reorder have tried to been reordered - if cant_modify.iter() - .any(|(id, _)| { - let existing_rank = existing_order.iter().position(|existing_id| id == existing_id); - let new_rank = new_order.iter().position(|new_id| id == new_id); - - existing_rank != new_rank - }) + if server.roles + .iter() + .filter(|(_, role)| if let Some(top_rank) = member_top_rank { role.rank <= top_rank } else { true }) + .any(|(id, _)| existing_order.iter().position(|existing_id| id == existing_id) != new_order.iter().position(|new_id| id == new_id)) { return Err(create_error!(NotElevated)) } } - // update the roles which have had their rank changed - for (role_id, role) in &mut server.roles { - let new_rank = new_order.iter().position(|id| id == role_id).unwrap() as i64; // unwrap - we check if its in the data earlier on - - if new_rank != role.rank { - // cant use Role::update here because we dont want to publish seperate role update events - let partial = PartialRole { - rank: Some(new_rank), - ..Default::default() - }; - - db.update_role(&server.id, role_id, &partial, Vec::new()).await?; - - role.apply_options(PartialRole { - rank: Some(new_rank), - ..Default::default() - }); - } - } - - // publish bulk update event - EventV1::ServerRoleRanksUpdate { - ranks: new_order - }.p(server.id.clone()).await; + server.update_role_rankings(db, new_order).await?; Ok(Json(server.into())) } From 4e4e598daf0e8583b3e3675a316dd75ac440da29 Mon Sep 17 00:00:00 2001 From: izzy Date: Sun, 8 Jun 2025 10:37:38 +0100 Subject: [PATCH 5/9] refactor: minor formatting changes --- .../core/database/src/models/servers/model.rs | 21 +++++---- crates/core/models/src/v0/servers.rs | 3 +- crates/delta/src/routes/servers/roles_edit.rs | 1 - .../routes/servers/roles_edit_positions.rs | 47 +++++++++++++------ 4 files changed, 44 insertions(+), 28 deletions(-) diff --git a/crates/core/database/src/models/servers/model.rs b/crates/core/database/src/models/servers/model.rs index 65d7d282..150f4f53 100644 --- a/crates/core/database/src/models/servers/model.rs +++ b/crates/core/database/src/models/servers/model.rs @@ -254,12 +254,12 @@ impl Server { } } - /// reorders the server's roles rankings - pub async fn update_role_rankings(&mut self, db: &Database, new_order: Vec) -> Result<()> { - // new_order must always contain every role + /// Reorders the server's roles rankings + pub async fn set_role_ordering(&mut self, db: &Database, new_order: Vec) -> Result<()> { + // New order must always contain every role debug_assert_eq!(self.roles.len(), new_order.len()); - // set the role's ranks to the positions in the vec + // Set the role's ranks to the positions in the vec for (rank, id) in new_order.iter().enumerate() { self.roles.get_mut(id).unwrap().rank = rank as i64; } @@ -270,13 +270,14 @@ impl Server { roles: Some(self.roles.clone()), ..Default::default() }, - Vec::new() - ).await?; + Vec::new(), + ) + .await?; - // publish bulk update event - EventV1::ServerRoleRanksUpdate { - ranks: new_order - }.p(self.id.clone()).await; + // Publish bulk update event + EventV1::ServerRoleRanksUpdate { ranks: new_order } + .p(self.id.clone()) + .await; Ok(()) } diff --git a/crates/core/models/src/v0/servers.rs b/crates/core/models/src/v0/servers.rs index 9e181176..4c7e1ee8 100644 --- a/crates/core/models/src/v0/servers.rs +++ b/crates/core/models/src/v0/servers.rs @@ -267,8 +267,7 @@ auto_derived!( pub hoist: Option, /// Ranking position /// - /// **Deprecated** - use the edit server role positions route. - /// Smaller values take priority. + /// **Removed** - no effect, use the edit server role positions route pub rank: Option, /// Fields to remove from role object #[cfg_attr(feature = "validator", validate(length(min = 1)))] diff --git a/crates/delta/src/routes/servers/roles_edit.rs b/crates/delta/src/routes/servers/roles_edit.rs index 78143bf2..f8152efe 100644 --- a/crates/delta/src/routes/servers/roles_edit.rs +++ b/crates/delta/src/routes/servers/roles_edit.rs @@ -46,7 +46,6 @@ pub async fn edit( colour, hoist, remove, - // rank is deprecated .. } = data; diff --git a/crates/delta/src/routes/servers/roles_edit_positions.rs b/crates/delta/src/routes/servers/roles_edit_positions.rs index 7e3b4d73..492f2511 100644 --- a/crates/delta/src/routes/servers/roles_edit_positions.rs +++ b/crates/delta/src/routes/servers/roles_edit_positions.rs @@ -1,8 +1,11 @@ -use revolt_database::{events::client::EventV1, util::{permissions::DatabasePermissionQuery, reference::Reference}, Database, PartialRole, PartialServer, User}; -use revolt_permissions::{calculate_server_permissions, ChannelPermission}; -use rocket::{serde::json::Json, State}; -use revolt_result::{create_error, Result}; +use revolt_database::{ + util::{permissions::DatabasePermissionQuery, reference::Reference}, + Database, User, +}; use revolt_models::v0; +use revolt_permissions::{calculate_server_permissions, ChannelPermission}; +use revolt_result::{create_error, Result}; +use rocket::{serde::json::Json, State}; /// # Edits server roles ranks /// @@ -26,27 +29,41 @@ pub async fn edit_role_ranks( let existing_order = server.roles.keys().cloned().collect::>(); let new_order = data.ranks.clone().into_iter().collect::>(); - // verify all roles are in the new ordering - if data.ranks.len() != server.roles.len() && server.roles.iter().all(|(id, _)| data.ranks.contains(id)) { - return Err(create_error!(InvalidOperation)) + // Verify all roles are in the new ordering + if data.ranks.len() != server.roles.len() + && server.roles.iter().all(|(id, _)| data.ranks.contains(id)) + { + return Err(create_error!(InvalidOperation)); } - // dont have to check what the user cant modify if they are the server owner + // Don't have to check what the user cant modify if they are the server owner if server.owner != user.id { let member_top_rank = query.get_member_rank(); - // find all roles above the member which we should not be able to reorder then - // check if any roles which we cant reorder have tried to been reordered - if server.roles + if server + .roles .iter() - .filter(|(_, role)| if let Some(top_rank) = member_top_rank { role.rank <= top_rank } else { true }) - .any(|(id, _)| existing_order.iter().position(|existing_id| id == existing_id) != new_order.iter().position(|new_id| id == new_id)) + // Find all roles above the member which we should not be able to reorder + .filter(|(_, role)| { + if let Some(top_rank) = member_top_rank { + role.rank <= top_rank + } else { + true + } + }) + // Check if user is trying to reorder roles they can't reorder (as found previously) + .any(|(id, _)| { + existing_order + .iter() + .position(|existing_id| id == existing_id) + != new_order.iter().position(|new_id| id == new_id) + }) { - return Err(create_error!(NotElevated)) + return Err(create_error!(NotElevated)); } } - server.update_role_rankings(db, new_order).await?; + server.set_role_ordering(db, new_order).await?; Ok(Json(server.into())) } From 73b576a75f66717a31c4f2506460db6611318614 Mon Sep 17 00:00:00 2001 From: izzy Date: Sun, 8 Jun 2025 10:50:55 +0100 Subject: [PATCH 6/9] fix: ensure server ID is fanned out with role ranks update fix: ensure original order is correctly sorted on edit route --- crates/core/database/src/events/client.rs | 2 +- crates/core/database/src/models/servers/model.rs | 16 +++++++++++++--- .../src/routes/servers/roles_edit_positions.rs | 9 +++++++-- 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/crates/core/database/src/events/client.rs b/crates/core/database/src/events/client.rs index 2d0829a4..a887e9f1 100644 --- a/crates/core/database/src/events/client.rs +++ b/crates/core/database/src/events/client.rs @@ -166,7 +166,7 @@ pub enum EventV1 { ServerRoleDelete { id: String, role_id: String }, /// Server roles ranks updated - ServerRoleRanksUpdate { ranks: Vec }, + ServerRoleRanksUpdate { id: String, ranks: Vec }, /// Update existing user UserUpdate { diff --git a/crates/core/database/src/models/servers/model.rs b/crates/core/database/src/models/servers/model.rs index 150f4f53..eea8f392 100644 --- a/crates/core/database/src/models/servers/model.rs +++ b/crates/core/database/src/models/servers/model.rs @@ -228,6 +228,13 @@ impl Server { } } + /// Ordered roles list + pub fn ordered_roles(&self) -> Vec<(String, Role)> { + let mut ordered_roles = self.roles.clone().into_iter().collect::>(); + ordered_roles.sort_by(|(_, role_a), (_, role_b)| role_a.rank.cmp(&role_b.rank)); + ordered_roles + } + /// Set role permission on a server pub async fn set_role_permission( &mut self, @@ -275,9 +282,12 @@ impl Server { .await?; // Publish bulk update event - EventV1::ServerRoleRanksUpdate { ranks: new_order } - .p(self.id.clone()) - .await; + EventV1::ServerRoleRanksUpdate { + id: self.id.clone(), + ranks: new_order, + } + .p(self.id.clone()) + .await; Ok(()) } diff --git a/crates/delta/src/routes/servers/roles_edit_positions.rs b/crates/delta/src/routes/servers/roles_edit_positions.rs index 492f2511..16fae2b5 100644 --- a/crates/delta/src/routes/servers/roles_edit_positions.rs +++ b/crates/delta/src/routes/servers/roles_edit_positions.rs @@ -26,7 +26,12 @@ pub async fn edit_role_ranks( .await .throw_if_lacking_channel_permission(ChannelPermission::ManageRole)?; - let existing_order = server.roles.keys().cloned().collect::>(); + let existing_order = server + .ordered_roles() + .into_iter() + .map(|(id, _)| id) + .collect::>(); + let new_order = data.ranks.clone().into_iter().collect::>(); // Verify all roles are in the new ordering @@ -36,7 +41,7 @@ pub async fn edit_role_ranks( return Err(create_error!(InvalidOperation)); } - // Don't have to check what the user cant modify if they are the server owner + // Don't have to check what the user can't modify if they are the server owner if server.owner != user.id { let member_top_rank = query.get_member_rank(); From 99f400bc7bded575de1d93615710db5a16f55789 Mon Sep 17 00:00:00 2001 From: izzy Date: Sun, 8 Jun 2025 10:56:54 +0100 Subject: [PATCH 7/9] fix: logic error in initial data check --- crates/delta/src/routes/servers/roles_edit_positions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/delta/src/routes/servers/roles_edit_positions.rs b/crates/delta/src/routes/servers/roles_edit_positions.rs index 16fae2b5..2c4a6fe5 100644 --- a/crates/delta/src/routes/servers/roles_edit_positions.rs +++ b/crates/delta/src/routes/servers/roles_edit_positions.rs @@ -36,7 +36,7 @@ pub async fn edit_role_ranks( // Verify all roles are in the new ordering if data.ranks.len() != server.roles.len() - && server.roles.iter().all(|(id, _)| data.ranks.contains(id)) + || !server.roles.iter().all(|(id, _)| data.ranks.contains(id)) { return Err(create_error!(InvalidOperation)); } From 3e8a4010776f2c43d57a3ac2552ad47f91420b93 Mon Sep 17 00:00:00 2001 From: izzy Date: Sun, 8 Jun 2025 11:18:44 +0100 Subject: [PATCH 8/9] test: begin writing test for role editing --- .../database/fixtures/server_with_roles.json | 18 ++- crates/delta/fixtures/group_with_members.json | 28 +++++ crates/delta/fixtures/server_with_roles.json | 111 ++++++++++++++++++ .../routes/servers/roles_edit_positions.rs | 51 ++++++++ crates/delta/src/util/test.rs | 49 +++++--- 5 files changed, 237 insertions(+), 20 deletions(-) create mode 100644 crates/delta/fixtures/group_with_members.json create mode 100644 crates/delta/fixtures/server_with_roles.json diff --git a/crates/core/database/fixtures/server_with_roles.json b/crates/core/database/fixtures/server_with_roles.json index 01ec436b..68e6045b 100644 --- a/crates/core/database/fixtures/server_with_roles.json +++ b/crates/core/database/fixtures/server_with_roles.json @@ -52,7 +52,7 @@ "a": 545270208, "d": 0 }, - "rank": 3 + "rank": 1 }, "__ID:6__": { "name": "Owner", @@ -61,6 +61,22 @@ "d": 0 }, "rank": 0 + }, + "__ID:7__": { + "name": "Lower Rank 1", + "permissions": { + "a": 0, + "d": 0 + }, + "rank": 2 + }, + "__ID:8__": { + "name": "Lower Rank 2", + "permissions": { + "a": 0, + "d": 0 + }, + "rank": 2 } }, "default_permissions": 4000322560 diff --git a/crates/delta/fixtures/group_with_members.json b/crates/delta/fixtures/group_with_members.json new file mode 100644 index 00000000..a4544176 --- /dev/null +++ b/crates/delta/fixtures/group_with_members.json @@ -0,0 +1,28 @@ +[ + { + "_object_type": "User", + "_id": "__ID:0__", + "username": "Owner", + "discriminator": "0001" + }, + { + "_object_type": "User", + "_id": "__ID:1__", + "username": "Member", + "discriminator": "0001" + }, + { + "_object_type": "User", + "_id": "__ID:2__", + "username": "Member", + "discriminator": "0002" + }, + { + "_object_type": "Channel", + "_id": "__ID:3__", + "channel_type": "Group", + "name": "My Group", + "owner": "__ID:0__", + "recipients": ["__ID:0__", "__ID:1__"] + } +] diff --git a/crates/delta/fixtures/server_with_roles.json b/crates/delta/fixtures/server_with_roles.json new file mode 100644 index 00000000..8eb61601 --- /dev/null +++ b/crates/delta/fixtures/server_with_roles.json @@ -0,0 +1,111 @@ +[ + { + "_object_type": "User", + "_id": "__ID:0__", + "username": "Owner", + "discriminator": "0001" + }, + { + "_object_type": "User", + "_id": "__ID:1__", + "username": "Moderator", + "discriminator": "0001" + }, + { + "_object_type": "User", + "_id": "__ID:2__", + "username": "User", + "discriminator": "0001" + }, + { + "_object_type": "Channel", + "_id": "__ID:3__", + "channel_type": "TextChannel", + "name": "General", + "server": "__ID:4__", + "default_permissions": { + "a": 0, + "d": 1048576 + }, + "role_permissions": { + "__ID:5__": { + "a": 1048576, + "d": 0 + } + } + }, + { + "_object_type": "Server", + "_id": "__ID:4__", + "owner": "__ID:0__", + "name": "Server", + "channels": [ + "__ID:3__" + ], + "roles": { + "__ID:5__": { + "name": "Moderator", + "permissions": { + "a": 545270208, + "d": 0 + }, + "rank": 1 + }, + "__ID:6__": { + "name": "Owner", + "permissions": { + "a": 0, + "d": 0 + }, + "rank": 0 + }, + "__ID:7__": { + "name": "Lower Rank 1", + "permissions": { + "a": 0, + "d": 0 + }, + "rank": 2 + }, + "__ID:8__": { + "name": "Lower Rank 2", + "permissions": { + "a": 0, + "d": 0 + }, + "rank": 2 + } + }, + "default_permissions": 4000322560 + }, + { + "_object_type": "ServerMember", + "_id": { + "user": "__ID:0__", + "server": "__ID:4__" + }, + "roles": [ + "__ID:6__" + ], + "joined_at": 1698318340195 + }, + { + "_object_type": "ServerMember", + "_id": { + "user": "__ID:1__", + "server": "__ID:4__" + }, + "roles": [ + "__ID:5__" + ], + "joined_at": 1698318340195 + }, + { + "_object_type": "ServerMember", + "_id": { + "user": "__ID:2__", + "server": "__ID:4__" + }, + "joined_at": 1698318340195 + } +] \ No newline at end of file diff --git a/crates/delta/src/routes/servers/roles_edit_positions.rs b/crates/delta/src/routes/servers/roles_edit_positions.rs index 2c4a6fe5..6bd80317 100644 --- a/crates/delta/src/routes/servers/roles_edit_positions.rs +++ b/crates/delta/src/routes/servers/roles_edit_positions.rs @@ -72,3 +72,54 @@ pub async fn edit_role_ranks( Ok(Json(server.into())) } + +#[cfg(test)] +mod test { + use revolt_database::fixture; + use revolt_models::v0; + use rocket::http::{ContentType, Header, Status}; + + use crate::util::test::TestHarness; + + #[rocket::async_test] + async fn edit_role_rankings() { + let harness = TestHarness::new().await; + + fixture!(harness.db, "server_with_roles", + owner user 0 + moderator user 1 + user user 2 + server server 4); + + // Moderator can re-order the roles below them + let (_, moderator_session) = harness.account_from_user(moderator.id).await; + let mut target_order: Vec = server + .ordered_roles() + .into_iter() + .map(|(id, _)| id) + .collect(); + + // Swap the two lower ranked roles + target_order.swap(2, 3); + + let response = harness + .client + .post(format!("/servers/{}/roles/ranks", server.id)) + .header(ContentType::JSON) + .body( + json!(v0::DataEditRoleRanks { + ranks: target_order.clone() + }) + .to_string(), + ) + .header(Header::new( + "x-session-token", + moderator_session.token.to_string(), + )) + .dispatch() + .await; + + assert_eq!(response.status(), Status::Ok); + drop(response); + } +} diff --git a/crates/delta/src/util/test.rs b/crates/delta/src/util/test.rs index e600ac15..067f8bd0 100644 --- a/crates/delta/src/util/test.rs +++ b/crates/delta/src/util/test.rs @@ -1,5 +1,5 @@ use authifier::{ - models::{Account, Session}, + models::{Account, EmailVerification, Session}, Authifier, }; use futures::StreamExt; @@ -83,30 +83,41 @@ impl TestHarness { } pub async fn new_user(&self) -> (Account, Session, User) { - let account = Account::new( - &self.authifier, - format!("{}@revolt.chat", TestHarness::rand_string()), - "password".to_string(), - false, - ) - .await - .expect("`Account`"); + let user = User::create(&self.db, TestHarness::rand_string(), None, None) + .await + .expect("`User`"); + + let (account, session) = self.account_from_user(user.id.clone()).await; + + (account, session, user) + } + + pub async fn account_from_user(&self, id: String) -> (Account, Session) { + let account = Account { + id, + email: format!("{}@revolt.chat", TestHarness::rand_string()), + password: Default::default(), + email_normalised: Default::default(), + deletion: None, + disabled: false, + lockout: None, + mfa: Default::default(), + password_reset: None, + verification: EmailVerification::Verified, + }; + + self.authifier + .database + .save_account(&account) + .await + .expect("`Account`"); let session = account .create_session(&self.authifier, String::new()) .await .expect("`Session`"); - let user = User::create( - &self.db, - TestHarness::rand_string(), - account.id.to_string(), - None, - ) - .await - .expect("`User`"); - - (account, session, user) + (account, session) } pub async fn new_server(&self, user: &User) -> (Server, Vec) { From d30ceea373f978a5f8e47b6a56cc3f136eae263b Mon Sep 17 00:00:00 2001 From: izzy Date: Sun, 8 Jun 2025 11:32:42 +0100 Subject: [PATCH 9/9] test: complete test for editing role positions --- .../database/fixtures/server_with_roles.json | 16 ----- .../admin_migrations/ops/mongodb/scripts.rs | 36 ++++++++---- crates/delta/fixtures/group_with_members.json | 28 --------- ...roles.json => server_with_many_roles.json} | 5 +- .../routes/servers/roles_edit_positions.rs | 58 ++++++++++++++++++- 5 files changed, 83 insertions(+), 60 deletions(-) delete mode 100644 crates/delta/fixtures/group_with_members.json rename crates/delta/fixtures/{server_with_roles.json => server_with_many_roles.json} (89%) diff --git a/crates/core/database/fixtures/server_with_roles.json b/crates/core/database/fixtures/server_with_roles.json index 68e6045b..0a76489d 100644 --- a/crates/core/database/fixtures/server_with_roles.json +++ b/crates/core/database/fixtures/server_with_roles.json @@ -61,22 +61,6 @@ "d": 0 }, "rank": 0 - }, - "__ID:7__": { - "name": "Lower Rank 1", - "permissions": { - "a": 0, - "d": 0 - }, - "rank": 2 - }, - "__ID:8__": { - "name": "Lower Rank 2", - "permissions": { - "a": 0, - "d": 0 - }, - "rank": 2 } }, "default_permissions": 4000322560 diff --git a/crates/core/database/src/models/admin_migrations/ops/mongodb/scripts.rs b/crates/core/database/src/models/admin_migrations/ops/mongodb/scripts.rs index 40efe91a..d164cab1 100644 --- a/crates/core/database/src/models/admin_migrations/ops/mongodb/scripts.rs +++ b/crates/core/database/src/models/admin_migrations/ops/mongodb/scripts.rs @@ -1,10 +1,15 @@ -use std::{collections::{HashMap, HashSet}, ops::BitXor, time::Duration}; +use std::{ + collections::{HashMap, HashSet}, + ops::BitXor, + time::Duration, +}; use crate::{ mongodb::{ bson::{doc, from_bson, from_document, to_document, Bson, DateTime, Document}, options::FindOptions, - }, AbstractChannels, AbstractServers, Channel, Invite, MongoDb, Server, User, DISCRIMINATOR_SEARCH_SPACE + }, + AbstractChannels, AbstractServers, Channel, Invite, MongoDb, User, DISCRIMINATOR_SEARCH_SPACE, }; use bson::{oid::ObjectId, to_bson}; use futures::StreamExt; @@ -1165,7 +1170,9 @@ pub async fn run_migrations(db: &MongoDb, revision: i32) -> i32 { } if revision <= 41 { - info!("Running migration [revision 41 / 05-06-2025]: convert role ranks to uniform numbers."); + info!( + "Running migration [revision 41 / 05-06-2025]: convert role ranks to uniform numbers." + ); #[derive(Serialize, Deserialize, Clone)] struct Role { @@ -1180,7 +1187,8 @@ pub async fn run_migrations(db: &MongoDb, revision: i32) -> i32 { pub roles: HashMap, } - let mut servers = db.db() + let mut servers = db + .db() .collection::("servers") .find(doc! { "roles": { @@ -1196,19 +1204,23 @@ pub async fn run_migrations(db: &MongoDb, revision: i32) -> i32 { while let Some(server) = servers.next().await { let mut ordered_roles = server.roles.clone().into_iter().collect::>(); ordered_roles.sort_by(|(_, role_a), (_, role_b)| role_a.rank.cmp(&role_b.rank)); - let ordered_roles = ordered_roles.into_iter().map(|(id, _)| id).collect::>(); + let ordered_roles = ordered_roles + .into_iter() + .map(|(id, _)| id) + .collect::>(); let mut doc = doc! {}; for id in server.roles.keys() { - doc.insert(format!("roles.{id}.rank"), ordered_roles.iter().position(|x| id == x).unwrap() as i64); - }; + doc.insert( + format!("roles.{id}.rank"), + ordered_roles.iter().position(|x| id == x).unwrap() as i64, + ); + } - db.db().collection::("servers") - .update_one( - doc! { "_id": &server.id }, - doc! { "$set": doc } - ) + db.db() + .collection::("servers") + .update_one(doc! { "_id": &server.id }, doc! { "$set": doc }) .await .unwrap(); } diff --git a/crates/delta/fixtures/group_with_members.json b/crates/delta/fixtures/group_with_members.json deleted file mode 100644 index a4544176..00000000 --- a/crates/delta/fixtures/group_with_members.json +++ /dev/null @@ -1,28 +0,0 @@ -[ - { - "_object_type": "User", - "_id": "__ID:0__", - "username": "Owner", - "discriminator": "0001" - }, - { - "_object_type": "User", - "_id": "__ID:1__", - "username": "Member", - "discriminator": "0001" - }, - { - "_object_type": "User", - "_id": "__ID:2__", - "username": "Member", - "discriminator": "0002" - }, - { - "_object_type": "Channel", - "_id": "__ID:3__", - "channel_type": "Group", - "name": "My Group", - "owner": "__ID:0__", - "recipients": ["__ID:0__", "__ID:1__"] - } -] diff --git a/crates/delta/fixtures/server_with_roles.json b/crates/delta/fixtures/server_with_many_roles.json similarity index 89% rename from crates/delta/fixtures/server_with_roles.json rename to crates/delta/fixtures/server_with_many_roles.json index 8eb61601..49cfe276 100644 --- a/crates/delta/fixtures/server_with_roles.json +++ b/crates/delta/fixtures/server_with_many_roles.json @@ -3,18 +3,21 @@ "_object_type": "User", "_id": "__ID:0__", "username": "Owner", + "last_acknowledged_policy_change": "2025-06-07T04:04:48+0000", "discriminator": "0001" }, { "_object_type": "User", "_id": "__ID:1__", "username": "Moderator", + "last_acknowledged_policy_change": "2025-06-07T04:04:48+0000", "discriminator": "0001" }, { "_object_type": "User", "_id": "__ID:2__", "username": "User", + "last_acknowledged_policy_change": "2025-06-07T04:04:48+0000", "discriminator": "0001" }, { @@ -46,7 +49,7 @@ "__ID:5__": { "name": "Moderator", "permissions": { - "a": 545270208, + "a": 545270216, "d": 0 }, "rank": 1 diff --git a/crates/delta/src/routes/servers/roles_edit_positions.rs b/crates/delta/src/routes/servers/roles_edit_positions.rs index 6bd80317..59614162 100644 --- a/crates/delta/src/routes/servers/roles_edit_positions.rs +++ b/crates/delta/src/routes/servers/roles_edit_positions.rs @@ -85,10 +85,9 @@ mod test { async fn edit_role_rankings() { let harness = TestHarness::new().await; - fixture!(harness.db, "server_with_roles", + fixture!(harness.db, "server_with_many_roles", owner user 0 moderator user 1 - user user 2 server server 4); // Moderator can re-order the roles below them @@ -104,7 +103,7 @@ mod test { let response = harness .client - .post(format!("/servers/{}/roles/ranks", server.id)) + .patch(format!("/servers/{}/roles/ranks", server.id)) .header(ContentType::JSON) .body( json!(v0::DataEditRoleRanks { @@ -121,5 +120,58 @@ mod test { assert_eq!(response.status(), Status::Ok); drop(response); + + // ... but not above them + let mut target_order: Vec = server + .ordered_roles() + .into_iter() + .map(|(id, _)| id) + .collect(); + + // Swap the two lower ranked roles + target_order.swap(0, 1); + + let response = harness + .client + .patch(format!("/servers/{}/roles/ranks", server.id)) + .header(ContentType::JSON) + .body( + json!(v0::DataEditRoleRanks { + ranks: target_order.clone() + }) + .to_string(), + ) + .header(Header::new( + "x-session-token", + moderator_session.token.to_string(), + )) + .dispatch() + .await; + + assert_eq!(response.status(), Status::Forbidden); + drop(response); + + // The owner can set any order they want + let (_, owner_session) = harness.account_from_user(owner.id).await; + + let response = harness + .client + .patch(format!("/servers/{}/roles/ranks", server.id)) + .header(ContentType::JSON) + .body( + json!(v0::DataEditRoleRanks { + ranks: target_order.clone() + }) + .to_string(), + ) + .header(Header::new( + "x-session-token", + owner_session.token.to_string(), + )) + .dispatch() + .await; + + assert_eq!(response.status(), Status::Ok); + drop(response); } }