merge: pull request #418 from revoltchat/feat/role-ranks-v2

crates/core/database/fixtures/server_with_roles.json

@@ -52,7 +52,7 @@
           "a": 545270208,
           "d": 0
         },
-        "rank": 3
+        "rank": 1
       },
       "__ID:6__": {
         "name": "Owner",

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 { id: String, ranks: Vec<String> },
+
     /// Update existing user
     UserUpdate {
         id: String,

crates/core/database/src/models/admin_migrations/ops/mongodb/scripts.rs

@@ -1,4 +1,8 @@
-use std::{collections::HashSet, ops::BitXor, time::Duration};
+use std::{
+    collections::{HashMap, HashSet},
+    ops::BitXor,
+    time::Duration,
+};
 
 use crate::{
     mongodb::{
@@ -22,7 +26,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::<Document>("migrations");
@@ -1165,6 +1169,63 @@ 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."
+        );
+
+        #[derive(Serialize, Deserialize, Clone)]
+        struct Role {
+            pub rank: i64,
+        }
+
+        #[derive(Serialize, Deserialize, Clone)]
+        struct Server {
+            #[serde(rename = "_id")]
+            pub id: String,
+            #[serde(default = "HashMap::<String, Role>::new")]
+            pub roles: HashMap<String, Role>,
+        }
+
+        let mut servers = db
+            .db()
+            .collection::<Server>("servers")
+            .find(doc! {
+                "roles": {
+                    "$exists": true,
+                    "$ne": []
+                }
+            })
+            .await
+            .unwrap()
+            .filter_map(|s| async { s.ok() })
+            .boxed();
+
+        while let Some(server) = servers.next().await {
+            let mut ordered_roles = server.roles.clone().into_iter().collect::<Vec<_>>();
+            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::<Vec<_>>();
+
+            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::<Server>("servers")
+                .update_one(doc! { "_id": &server.id }, doc! { "$set": doc })
+                .await
+                .unwrap();
+        }
+    }
+
     // Reminder to update LATEST_REVISION when adding new migrations.
     LATEST_REVISION.max(revision)
 }

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::<Vec<_>>();
+        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,
@@ -253,6 +260,37 @@ impl Server {
             Err(create_error!(NotFound))
         }
     }
+
+    /// Reorders the server's roles rankings
+    pub async fn set_role_ordering(&mut self, db: &Database, new_order: Vec<String>) -> 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 {
+            id: self.id.clone(),
+            ranks: new_order,
+        }
+        .p(self.id.clone())
+        .await;
+
+        Ok(())
+    }
 }
 
 impl Role {

crates/core/models/src/v0/servers.rs

@@ -267,7 +267,7 @@ auto_derived!(
         pub hoist: Option<bool>,
         /// Ranking position
         ///
-        /// Smaller values take priority.
+        /// **Removed** - no effect, use the edit server role positions route
         pub rank: Option<i64>,
         /// Fields to remove from role object
         #[cfg_attr(feature = "validator", validate(length(min = 1)))]
@@ -286,4 +286,9 @@ auto_derived!(
         /// Whether to not send a leave message
         pub leave_silently: Option<bool>,
     }
+
+    /// New role positions
+    pub struct DataEditRoleRanks {
+        pub ranks: Vec<String>,
+    }
 );

crates/delta/fixtures/server_with_many_roles.json

@@ -0,0 +1,114 @@
+[
+  {
+    "_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"
+  },
+  {
+    "_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": 545270216,
+          "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
+  }
+]

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<Route>, 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_ranks
     ]
 }

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("/<target>/roles/<role_id>", data = "<data>")]
+#[patch("/<target>/roles/<role_id>", data = "<data>", rank = 1)]
 pub async fn edit(
     db: &State<Database>,
     user: User,
@@ -45,22 +45,14 @@ pub async fn edit(
             name,
             colour,
             hoist,
-            rank,
             remove,
+            ..
         } = 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()
         };
 

crates/delta/src/routes/servers/roles_edit_positions.rs

@@ -0,0 +1,177 @@
+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
+///
+/// Edit's server role's ranks.
+#[openapi(tag = "Server Permissions")]
+#[patch("/<target>/roles/ranks", data = "<data>")]
+pub async fn edit_role_ranks(
+    db: &State<Database>,
+    user: User,
+    target: Reference,
+    data: Json<v0::DataEditRoleRanks>,
+) -> Result<Json<v0::Server>> {
+    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
+        .ordered_roles()
+        .into_iter()
+        .map(|(id, _)| id)
+        .collect::<Vec<_>>();
+
+    let new_order = data.ranks.clone().into_iter().collect::<Vec<_>>();
+
+    // 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));
+    }
+
+    // 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();
+
+        if server
+            .roles
+            .iter()
+            // 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));
+        }
+    }
+
+    server.set_role_ordering(db, new_order).await?;
+
+    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_many_roles",
+            owner user 0
+            moderator user 1
+            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<String> = server
+            .ordered_roles()
+            .into_iter()
+            .map(|(id, _)| id)
+            .collect();
+
+        // Swap the two lower ranked roles
+        target_order.swap(2, 3);
+
+        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::Ok);
+        drop(response);
+
+        // ... but not above them
+        let mut target_order: Vec<String> = 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);
+    }
+}

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(
+        let user = User::create(&self.db, TestHarness::rand_string(), None, None)
-            &self.authifier,
+            .await
-            format!("{}@revolt.chat", TestHarness::rand_string()),
+            .expect("`User`");
-            "password".to_string(),
+
-            false,
+        let (account, session) = self.account_from_user(user.id.clone()).await;
-        )
+
-        .await
+        (account, session, user)
-        .expect("`Account`");
+    }
+
+    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(
+        (account, session)
-            &self.db,
-            TestHarness::rand_string(),
-            account.id.to_string(),
-            None,
-        )
-        .await
-        .expect("`User`");
-
-        (account, session, user)
     }
 
     pub async fn new_server(&self, user: &User) -> (Server, Vec<Channel>) {