fix: restrict who can remove aliases

Previously, anyone could remove any local alias, meaning that someone could re-route a popular alias elsewhere
Now, only the creator of the alias, users who can set canonical aliases for the room, server admins and the server user can delete aliases

added some additional changes/fixes to adapt to our codebase

Co-authored-by: strawberry <[email protected]>
Signed-off-by: strawberry <[email protected]>

src/admin/room/room_alias_commands.rs

@@ -1,11 +1,14 @@
 use std::fmt::Write;
 
-use ruma::{events::room::message::RoomMessageEventContent, RoomAliasId};
+use ruma::{events::room::message::RoomMessageEventContent, RoomAliasId, UserId};
 
 use super::RoomAliasCommand;
 use crate::{escape_html, services, Result};
 
 pub(crate) async fn process(command: RoomAliasCommand, _body: Vec<&str>) -> Result<RoomMessageEventContent> {
+	let server_user = UserId::parse_with_server_name(String::from("conduit"), services().globals.server_name())
+		.expect("server's username is valid");
+
 	match command {
 		RoomAliasCommand::Set {
 			ref room_alias_localpart,
@@ -28,7 +31,11 @@ pub(crate) async fn process(command: RoomAliasCommand, _body: Vec<&str>) -> Resu
 					room_id,
 					..
 				} => match (force, services().rooms.alias.resolve_local_alias(&room_alias)) {
-					(true, Ok(Some(id))) => match services().rooms.alias.set_alias(&room_alias, &room_id) {
+					(true, Ok(Some(id))) => match services()
+						.rooms
+						.alias
+						.set_alias(&room_alias, &room_id, &server_user)
+					{
 						Ok(()) => Ok(RoomMessageEventContent::text_plain(format!(
 							"Successfully overwrote alias (formerly {id})"
 						))),
@@ -37,7 +44,11 @@ pub(crate) async fn process(command: RoomAliasCommand, _body: Vec<&str>) -> Resu
 					(false, Ok(Some(id))) => Ok(RoomMessageEventContent::text_plain(format!(
 						"Refusing to overwrite in use alias for {id}, use -f or --force to overwrite"
 					))),
-					(_, Ok(None)) => match services().rooms.alias.set_alias(&room_alias, &room_id) {
+					(_, Ok(None)) => match services()
+						.rooms
+						.alias
+						.set_alias(&room_alias, &room_id, &server_user)
+					{
 						Ok(()) => Ok(RoomMessageEventContent::text_plain("Successfully set alias")),
 						Err(err) => Ok(RoomMessageEventContent::text_plain(format!("Failed to remove alias: {err}"))),
 					},
@@ -46,7 +57,12 @@ pub(crate) async fn process(command: RoomAliasCommand, _body: Vec<&str>) -> Resu
 				RoomAliasCommand::Remove {
 					..
 				} => match services().rooms.alias.resolve_local_alias(&room_alias) {
-					Ok(Some(id)) => match services().rooms.alias.remove_alias(&room_alias) {
+					Ok(Some(id)) => match services()
+						.rooms
+						.alias
+						.remove_alias(&room_alias, &server_user)
+						.await
+					{
 						Ok(()) => Ok(RoomMessageEventContent::text_plain(format!("Removed alias from {id}"))),
 						Err(err) => Ok(RoomMessageEventContent::text_plain(format!("Failed to remove alias: {err}"))),
 					},

src/api/client/alias.rs

@@ -22,6 +22,8 @@ use crate::{
 ///
 /// Creates a new room alias on this server.
 pub(crate) async fn create_alias_route(body: Ruma<create_alias::v3::Request>) -> Result<create_alias::v3::Response> {
+	let sender_user = body.sender_user.as_ref().expect("user is authenticated");
+
 	alias_checks(&body.room_alias, &body.appservice_info).await?;
 
 	// this isn't apart of alias_checks or delete alias route because we should
@@ -43,17 +45,10 @@ pub(crate) async fn create_alias_route(body: Ruma<create_alias::v3::Request>) ->
 		return Err(Error::Conflict("Alias already exists."));
 	}
 
-	if services()
+	services()
 		.rooms
 		.alias
-		.set_alias(&body.room_alias, &body.room_id)
-		.is_err()
-	{
-		return Err(Error::BadRequest(
-			ErrorKind::InvalidParam,
-			"Invalid room alias. Alias must be in the form of '#localpart:server_name'",
-		));
-	};
+		.set_alias(&body.room_alias, &body.room_id, sender_user)?;
 
 	Ok(create_alias::v3::Response::new())
 }
@@ -62,10 +57,12 @@ pub(crate) async fn create_alias_route(body: Ruma<create_alias::v3::Request>) ->
 ///
 /// Deletes a room alias from this server.
 ///
-/// - TODO: additional access control checks
 /// - TODO: Update canonical alias event
 pub(crate) async fn delete_alias_route(body: Ruma<delete_alias::v3::Request>) -> Result<delete_alias::v3::Response> {
+	let sender_user = body.sender_user.as_ref().expect("user is authenticated");
+
 	alias_checks(&body.room_alias, &body.appservice_info).await?;
+
 	if services()
 		.rooms
 		.alias
@@ -75,17 +72,11 @@ pub(crate) async fn delete_alias_route(body: Ruma<delete_alias::v3::Request>) ->
 		return Err(Error::BadRequest(ErrorKind::NotFound, "Alias does not exist."));
 	}
 
-	if services()
+	services()
 		.rooms
 		.alias
-		.remove_alias(&body.room_alias)
-		.is_err()
-	{
-		return Err(Error::BadRequest(
-			ErrorKind::InvalidParam,
-			"Invalid room alias. Alias must be in the form of '#localpart:server_name'",
-		));
-	};
+		.remove_alias(&body.room_alias, sender_user)
+		.await?;
 
 	// TODO: update alt_aliases?
 

src/api/client/room.rs

@@ -467,7 +467,10 @@ pub(crate) async fn create_room_route(body: Ruma<create_room::v3::Request>) -> R
 
 	// Homeserver specific stuff
 	if let Some(alias) = alias {
-		services().rooms.alias.set_alias(&alias, &room_id)?;
+		services()
+			.rooms
+			.alias
+			.set_alias(&alias, &room_id, sender_user)?;
 	}
 
 	if body.visibility == room::Visibility::Public {
@@ -787,7 +790,7 @@ pub(crate) async fn upgrade_room_route(body: Ruma<upgrade_room::v3::Request>) ->
 		services()
 			.rooms
 			.alias
-			.set_alias(&alias, &replacement_room)?;
+			.set_alias(&alias, &replacement_room, sender_user)?;
 	}
 
 	// Get the old room power levels

src/database/kvdatabase.rs

@@ -57,6 +57,7 @@ pub struct KeyValueDatabase {
 	pub eventid_pduid: Arc<dyn KvTree>,
 	pub roomid_pduleaves: Arc<dyn KvTree>,
 	pub alias_roomid: Arc<dyn KvTree>,
+	pub alias_userid: Arc<dyn KvTree>,  // UserId = AliasId (User who created the alias)
 	pub aliasid_alias: Arc<dyn KvTree>, // AliasId = RoomId + Count
 	pub publicroomids: Arc<dyn KvTree>,
 
@@ -193,6 +194,7 @@ impl KeyValueDatabase {
 			roomid_pduleaves: builder.open_tree("roomid_pduleaves")?,
 
 			alias_roomid: builder.open_tree("alias_roomid")?,
+			alias_userid: builder.open_tree("alias_userid")?,
 			aliasid_alias: builder.open_tree("aliasid_alias")?,
 			publicroomids: builder.open_tree("publicroomids")?,
 

src/service/admin.rs

@@ -72,8 +72,9 @@ impl Service {
 		let Ok(Some(admin_room)) = Self::get_admin_room().await else {
 			return Ok(());
 		};
-		let server_name = services().globals.server_name();
-		let server_user = UserId::parse(format!("@conduit:{server_name}")).expect("server's username is valid");
+
+		let server_user = UserId::parse_with_server_name(String::from("conduit"), services().globals.server_name())
+			.expect("server's username is valid");
 
 		loop {
 			debug_assert!(!receiver.is_closed(), "channel closed");
@@ -386,7 +387,10 @@ impl Service {
 			)
 			.await?;
 
-		services().rooms.alias.set_alias(&alias, &room_id)?;
+		services()
+			.rooms
+			.alias
+			.set_alias(&alias, &room_id, &server_user)?;
 
 		// 7. (ad-hoc) Disable room previews for everyone by default
 		services()
@@ -533,4 +537,13 @@ impl Service {
 
 		Ok(())
 	}
+
+	/// Checks whether a given user is an admin of this server
+	pub async fn user_is_admin(&self, user_id: &UserId) -> Result<bool> {
+		let Ok(Some(admin_room)) = Self::get_admin_room().await else {
+			return Ok(false);
+		};
+
+		services().rooms.state_cache.is_joined(user_id, &admin_room)
+	}
 }

src/service/rooms/alias/data.rs

@@ -1,17 +1,20 @@
-use ruma::{api::client::error::ErrorKind, OwnedRoomAliasId, OwnedRoomId, RoomAliasId, RoomId};
+use ruma::{api::client::error::ErrorKind, OwnedRoomAliasId, OwnedRoomId, OwnedUserId, RoomAliasId, RoomId, UserId};
 
 use crate::{services, utils, Error, KeyValueDatabase, Result};
 
 pub trait Data: Send + Sync {
 	/// Creates or updates the alias to the given room id.
-	fn set_alias(&self, alias: &RoomAliasId, room_id: &RoomId) -> Result<()>;
+	fn set_alias(&self, alias: &RoomAliasId, room_id: &RoomId, user_id: &UserId) -> Result<()>;
 
 	/// Forgets about an alias. Returns an error if the alias did not exist.
 	fn remove_alias(&self, alias: &RoomAliasId) -> Result<()>;
 
 	/// Looks up the roomid for the given alias.
 	fn resolve_local_alias(&self, alias: &RoomAliasId) -> Result<Option<OwnedRoomId>>;
 
+	/// Finds the user who assigned the given alias to a room
+	fn who_created_alias(&self, alias: &RoomAliasId) -> Result<Option<OwnedUserId>>;
+
 	/// Returns all local aliases that point to the given room
 	fn local_aliases_for_room<'a>(
 		&'a self, room_id: &RoomId,
@@ -22,13 +25,19 @@ pub trait Data: Send + Sync {
 }
 
 impl Data for KeyValueDatabase {
-	fn set_alias(&self, alias: &RoomAliasId, room_id: &RoomId) -> Result<()> {
+	fn set_alias(&self, alias: &RoomAliasId, room_id: &RoomId, user_id: &UserId) -> Result<()> {
+		// Comes first as we don't want a stuck alias
+		self.alias_userid
+			.insert(alias.alias().as_bytes(), user_id.as_bytes())?;
+
 		self.alias_roomid
 			.insert(alias.alias().as_bytes(), room_id.as_bytes())?;
+
 		let mut aliasid = room_id.as_bytes().to_vec();
 		aliasid.push(0xFF);
 		aliasid.extend_from_slice(&services().globals.next_count()?.to_be_bytes());
 		self.aliasid_alias.insert(&aliasid, alias.as_bytes())?;
+
 		Ok(())
 	}
 
@@ -40,10 +49,14 @@ impl Data for KeyValueDatabase {
 			for (key, _) in self.aliasid_alias.scan_prefix(prefix) {
 				self.aliasid_alias.remove(&key)?;
 			}
+
 			self.alias_roomid.remove(alias.alias().as_bytes())?;
+
+			self.alias_userid.remove(alias.alias().as_bytes())?;
 		} else {
-			return Err(Error::BadRequest(ErrorKind::NotFound, "Alias does not exist."));
+			return Err(Error::BadRequest(ErrorKind::NotFound, "Alias does not exist or is invalid."));
 		}
+
 		Ok(())
 	}
 
@@ -60,6 +73,19 @@ impl Data for KeyValueDatabase {
 			.transpose()
 	}
 
+	fn who_created_alias(&self, alias: &RoomAliasId) -> Result<Option<OwnedUserId>> {
+		self.alias_userid
+			.get(alias.alias().as_bytes())?
+			.map(|bytes| {
+				UserId::parse(
+					utils::string_from_bytes(&bytes)
+						.map_err(|_| Error::bad_database("User ID in alias_userid is invalid unicode."))?,
+				)
+				.map_err(|_| Error::bad_database("User ID in alias_roomid is invalid."))
+			})
+			.transpose()
+	}
+
 	fn local_aliases_for_room<'a>(
 		&'a self, room_id: &RoomId,
 	) -> Box<dyn Iterator<Item = Result<OwnedRoomAliasId>> + 'a> {

src/service/rooms/alias/mod.rs

@@ -3,20 +3,38 @@ mod data;
 use std::sync::Arc;
 
 use data::Data;
-use ruma::{OwnedRoomAliasId, OwnedRoomId, RoomAliasId, RoomId};
+use ruma::{
+	api::client::error::ErrorKind,
+	events::{
+		room::power_levels::{RoomPowerLevels, RoomPowerLevelsEventContent},
+		StateEventType,
+	},
+	OwnedRoomAliasId, OwnedRoomId, RoomAliasId, RoomId, UserId,
+};
 
-use crate::Result;
+use crate::{services, Error, Result};
 
 pub struct Service {
 	pub db: Arc<dyn Data>,
 }
 
 impl Service {
 	#[tracing::instrument(skip(self))]
-	pub fn set_alias(&self, alias: &RoomAliasId, room_id: &RoomId) -> Result<()> { self.db.set_alias(alias, room_id) }
+	pub fn set_alias(&self, alias: &RoomAliasId, room_id: &RoomId, user_id: &UserId) -> Result<()> {
+		self.db.set_alias(alias, room_id, user_id)
+	}
 
 	#[tracing::instrument(skip(self))]
-	pub fn remove_alias(&self, alias: &RoomAliasId) -> Result<()> { self.db.remove_alias(alias) }
+	pub async fn remove_alias(&self, alias: &RoomAliasId, user_id: &UserId) -> Result<()> {
+		if self.user_can_remove_alias(alias, user_id).await? {
+			self.db.remove_alias(alias)
+		} else {
+			Err(Error::BadRequest(
+				ErrorKind::forbidden(),
+				"User is not permitted to remove this alias.",
+			))
+		}
+	}
 
 	#[tracing::instrument(skip(self))]
 	pub fn resolve_local_alias(&self, alias: &RoomAliasId) -> Result<Option<OwnedRoomId>> {
@@ -34,4 +52,50 @@ impl Service {
 	pub fn all_local_aliases<'a>(&'a self) -> Box<dyn Iterator<Item = Result<(OwnedRoomId, String)>> + 'a> {
 		self.db.all_local_aliases()
 	}
+
+	async fn user_can_remove_alias(&self, alias: &RoomAliasId, user_id: &UserId) -> Result<bool> {
+		let Some(room_id) = self.resolve_local_alias(alias)? else {
+			return Err(Error::BadRequest(ErrorKind::NotFound, "Alias not found."));
+		};
+
+		let server_user =
+			UserId::parse_with_server_name(String::from("conduit"), services().globals.server_name()).unwrap();
+
+		// The creator of an alias can remove it
+		if self
+            .db
+            .who_created_alias(alias)?
+            .is_some_and(|user| user == user_id)
+            // Server admins can remove any local alias
+            || services().admin.user_is_admin(user_id).await?
+            // Always allow the server service account to remove the alias, since there may not be an admin room
+            || server_user == user_id
+		{
+			Ok(true)
+		// Checking whether the user is able to change canonical aliases of the
+		// room
+		} else if let Some(event) =
+			services()
+				.rooms
+				.state_accessor
+				.room_state_get(&room_id, &StateEventType::RoomPowerLevels, "")?
+		{
+			serde_json::from_str(event.content.get())
+				.map_err(|_| Error::bad_database("Invalid event content for m.room.power_levels"))
+				.map(|content: RoomPowerLevelsEventContent| {
+					RoomPowerLevels::from(content).user_can_send_state(user_id, StateEventType::RoomCanonicalAlias)
+				})
+		// If there is no power levels event, only the room creator can change
+		// canonical aliases
+		} else if let Some(event) =
+			services()
+				.rooms
+				.state_accessor
+				.room_state_get(&room_id, &StateEventType::RoomCreate, "")?
+		{
+			Ok(event.sender == user_id)
+		} else {
+			Err(Error::bad_database("Room has no m.room.create event"))
+		}
+	}
 }