diff --git a/changelog.d/1393.bugfix b/changelog.d/1393.bugfix new file mode 100644 index 00000000..3e8ecf22 --- /dev/null +++ b/changelog.d/1393.bugfix @@ -0,0 +1 @@ +Removed non-compliant nor functional room alias lookups over federation. Contributed by @nex diff --git a/src/admin/room/moderation.rs b/src/admin/room/moderation.rs index 53e3acf4..0c7c7612 100644 --- a/src/admin/room/moderation.rs +++ b/src/admin/room/moderation.rs @@ -89,13 +89,7 @@ async fn ban_room(&self, room: OwnedRoomOrAliasId) -> Result { locally, if not using get_alias_helper to fetch room ID remotely" ); - match self - .services - .rooms - .alias - .resolve_alias(room_alias, None) - .await - { + match self.services.rooms.alias.resolve_alias(room_alias).await { | Ok((room_id, servers)) => { debug!( %room_id, @@ -235,7 +229,7 @@ async fn ban_list_of_rooms(&self) -> Result { .services .rooms .alias - .resolve_alias(room_alias, None) + .resolve_alias(room_alias) .await { | Ok((room_id, servers)) => { @@ -388,13 +382,7 @@ async fn unban_room(&self, room: OwnedRoomOrAliasId) -> Result { room ID over federation" ); - match self - .services - .rooms - .alias - .resolve_alias(room_alias, None) - .await - { + match self.services.rooms.alias.resolve_alias(room_alias).await { | Ok((room_id, servers)) => { debug!( %room_id, diff --git a/src/api/client/alias.rs b/src/api/client/alias.rs index a08cf2c4..bad3d75b 100644 --- a/src/api/client/alias.rs +++ b/src/api/client/alias.rs @@ -1,12 +1,6 @@ use axum::extract::State; -use conduwuit::{Err, Result, debug}; -use conduwuit_service::Services; -use futures::StreamExt; -use rand::seq::SliceRandom; -use ruma::{ - OwnedServerName, RoomAliasId, RoomId, - api::client::alias::{create_alias, delete_alias, get_alias}, -}; +use conduwuit::{Err, Result}; +use ruma::api::client::alias::{create_alias, delete_alias, get_alias}; use crate::Ruma; @@ -96,65 +90,9 @@ pub(crate) async fn get_alias_route( ) -> Result { let room_alias = body.body.room_alias; - let Ok((room_id, servers)) = services.rooms.alias.resolve_alias(&room_alias, None).await - else { + let Ok((room_id, servers)) = services.rooms.alias.resolve_alias(&room_alias).await else { return Err!(Request(NotFound("Room with alias not found."))); }; - let servers = room_available_servers(&services, &room_id, &room_alias, servers).await; - debug!(%room_alias, %room_id, "available servers: {servers:?}"); - Ok(get_alias::v3::Response::new(room_id, servers)) } - -async fn room_available_servers( - services: &Services, - room_id: &RoomId, - room_alias: &RoomAliasId, - pre_servers: Vec, -) -> Vec { - // find active servers in room state cache to suggest - let mut servers: Vec = services - .rooms - .state_cache - .room_servers(room_id) - .map(ToOwned::to_owned) - .collect() - .await; - - // push any servers we want in the list already (e.g. responded remote alias - // servers, room alias server itself) - servers.extend(pre_servers); - - servers.sort_unstable(); - servers.dedup(); - - // shuffle list of servers randomly after sort and dedupe - servers.shuffle(&mut rand::thread_rng()); - - // insert our server as the very first choice if in list, else check if we can - // prefer the room alias server first - match servers - .iter() - .position(|server_name| services.globals.server_is_ours(server_name)) - { - | Some(server_index) => { - servers.swap_remove(server_index); - servers.insert(0, services.globals.server_name().to_owned()); - }, - | _ => { - match servers - .iter() - .position(|server| server == room_alias.server_name()) - { - | Some(alias_server_index) => { - servers.swap_remove(alias_server_index); - servers.insert(0, room_alias.server_name().into()); - }, - | _ => {}, - } - }, - } - - servers -} diff --git a/src/api/client/membership/join.rs b/src/api/client/membership/join.rs index efc85744..cbb82506 100644 --- a/src/api/client/membership/join.rs +++ b/src/api/client/membership/join.rs @@ -198,11 +198,7 @@ pub(crate) async fn join_room_by_id_or_alias_route( (servers, room_id) }, | Err(room_alias) => { - let (room_id, mut servers) = services - .rooms - .alias - .resolve_alias(&room_alias, Some(body.via.clone())) - .await?; + let (room_id, mut servers) = services.rooms.alias.resolve_alias(&room_alias).await?; banned_room_check( &services, diff --git a/src/api/client/membership/knock.rs b/src/api/client/membership/knock.rs index be888167..e589aa11 100644 --- a/src/api/client/membership/knock.rs +++ b/src/api/client/membership/knock.rs @@ -102,11 +102,7 @@ pub(crate) async fn knock_room_route( (servers, room_id) }, | Err(room_alias) => { - let (room_id, mut servers) = services - .rooms - .alias - .resolve_alias(&room_alias, Some(body.via.clone())) - .await?; + let (room_id, mut servers) = services.rooms.alias.resolve_alias(&room_alias).await?; banned_room_check( &services, diff --git a/src/api/client/state.rs b/src/api/client/state.rs index 63de57ea..323d0718 100644 --- a/src/api/client/state.rs +++ b/src/api/client/state.rs @@ -342,10 +342,10 @@ async fn allowed_to_send_state_event( } for alias in aliases { - let (alias_room_id, _servers) = services + let (alias_room_id, _) = services .rooms .alias - .resolve_alias(&alias, None) + .resolve_alias(&alias) .await .map_err(|e| { err!(Request(Unknown("Failed resolving alias \"{alias}\": {e}"))) diff --git a/src/database/engine/open.rs b/src/database/engine/open.rs index bd95bea0..b52d72f6 100644 --- a/src/database/engine/open.rs +++ b/src/database/engine/open.rs @@ -35,8 +35,7 @@ pub(crate) async fn open(ctx: Arc, desc: &[Descriptor]) -> Result, admin: Dep, appservice: Dep, globals: Dep, sending: Dep, state_accessor: Dep, + state_cache: Dep, } impl crate::Service for Service { @@ -47,13 +47,13 @@ impl crate::Service for Service { aliasid_alias: args.db["aliasid_alias"].clone(), }, services: Services { - server: args.server.clone(), admin: args.depend::("admin"), appservice: args.depend::("appservice"), globals: args.depend::("globals"), sending: args.depend::("sending"), state_accessor: args .depend::("rooms::state_accessor"), + state_cache: args.depend::("rooms::state_cache"), }, })) } @@ -117,6 +117,9 @@ impl Service { Ok(()) } + /// Resolves the given room ID or alias, returning the resolved room ID. + /// Unlike resolve_with_servers (the underlying call), potential resident + /// servers are not returned #[inline] pub async fn resolve(&self, room: &RoomOrAliasId) -> Result { self.resolve_with_servers(room, None) @@ -124,6 +127,14 @@ impl Service { .map(|(room_id, _)| room_id) } + /// Resolves the given room ID or alias, returning the resolved room ID, and + /// any servers that might be able to assist in fetching room data. + /// + /// If the input is a room ID, this simply returns it and . + /// If the input is an alias, this attempts to resolve it locally, then via + /// appservices, and finally remotely if the alias is not local. + /// If the alias is successfully resolved, the room ID and an empty list of + /// servers is returned. pub async fn resolve_with_servers( &self, room: &RoomOrAliasId, @@ -134,28 +145,26 @@ impl Service { Ok((room_id.to_owned(), servers.unwrap_or_default())) } else { let alias: &RoomAliasId = room.try_into().expect("valid RoomAliasId"); - self.resolve_alias(alias, servers).await + self.resolve_alias(alias).await } } + /// Resolves the given room alias, returning the resolved room ID and any + /// servers that might be in the room. #[tracing::instrument(skip(self), name = "resolve")] pub async fn resolve_alias( &self, room_alias: &RoomAliasId, - servers: Option>, ) -> Result<(OwnedRoomId, Vec)> { - let server_name = room_alias.server_name(); - let server_is_ours = self.services.globals.server_is_ours(server_name); - let servers_contains_ours = || { - servers - .as_ref() - .is_some_and(|servers| servers.contains(&self.services.server.name)) - }; + let server_is_ours = self + .services + .globals + .server_is_ours(room_alias.server_name()); - if !server_is_ours && !servers_contains_ours() { - return self - .remote_resolve(room_alias, servers.unwrap_or_default()) - .await; + if !server_is_ours { + // TODO: The spec advises servers may cache remote room aliases temporarily. + // We might want to look at doing that. + return self.remote_resolve(room_alias).await; } let room_id = match self.resolve_local_alias(room_alias).await { @@ -163,10 +172,18 @@ impl Service { | Err(_) => self.resolve_appservice_alias(room_alias).await?, }; - room_id.map_or_else( - || Err!(Request(NotFound("Room with alias not found."))), - |room_id| Ok((room_id, Vec::new())), - ) + if let Some(room_id) = room_id { + let servers: Vec = self + .services + .state_cache + .room_servers(&room_id) + .map(ToOwned::to_owned) + .collect() + .await; + return Ok((room_id, servers)); + } + + Err!(Request(NotFound("Alias does not exist."))) } #[tracing::instrument(skip(self), level = "debug")] @@ -206,12 +223,12 @@ impl Service { // The creator of an alias can remove it if self - .who_created_alias(alias).await - .is_ok_and(|user| user == user_id) - // Server admins can remove any local alias - || self.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 + .who_created_alias(alias).await + .is_ok_and(|user| user == user_id) + // Server admins can remove any local alias + || self.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 { return Ok(true); } diff --git a/src/service/rooms/alias/remote.rs b/src/service/rooms/alias/remote.rs index 60aed76d..3e8480d9 100644 --- a/src/service/rooms/alias/remote.rs +++ b/src/service/rooms/alias/remote.rs @@ -1,6 +1,4 @@ -use std::iter::once; - -use conduwuit::{Result, debug, debug_error, err, implement}; +use conduwuit::{Result, debug, error, implement}; use federation::query::get_room_information::v1::Response; use ruma::{OwnedRoomId, OwnedServerName, RoomAliasId, ServerName, api::federation}; @@ -8,40 +6,21 @@ use ruma::{OwnedRoomId, OwnedServerName, RoomAliasId, ServerName, api::federatio pub(super) async fn remote_resolve( &self, room_alias: &RoomAliasId, - servers: Vec, ) -> Result<(OwnedRoomId, Vec)> { - debug!(?room_alias, servers = ?servers, "resolve"); - let servers = once(room_alias.server_name()) - .map(ToOwned::to_owned) - .chain(servers.into_iter()); - - let mut resolved_servers = Vec::new(); - let mut resolved_room_id: Option = None; - for server in servers { - match self.remote_request(room_alias, &server).await { - | Err(e) => debug_error!("Failed to query for {room_alias:?} from {server}: {e}"), - | Ok(Response { room_id, servers }) => { - debug!( - "Server {server} answered with {room_id:?} for {room_alias:?} servers: \ - {servers:?}" - ); - - resolved_room_id.get_or_insert(room_id); - add_server(&mut resolved_servers, server); - - if !servers.is_empty() { - add_servers(&mut resolved_servers, servers); - break; - } - }, - } + debug!("Asking {} to resolve {room_alias:?}", room_alias.server_name()); + match self + .remote_request(room_alias, room_alias.server_name()) + .await + { + | Err(e) => { + error!("Unable to resolve remote room alias {}: {e}", room_alias); + Err(e) + }, + | Ok(Response { room_id, servers }) => { + debug!("Remote resolved {room_alias:?} to {room_id:?} with servers {servers:?}"); + Ok((room_id, servers)) + }, } - - resolved_room_id - .map(|room_id| (room_id, resolved_servers)) - .ok_or_else(|| { - err!(Request(NotFound("No servers could assist in resolving the room alias"))) - }) } #[implement(super::Service)] @@ -59,15 +38,3 @@ async fn remote_request( .send_federation_request(server, request) .await } - -fn add_servers(servers: &mut Vec, new: Vec) { - for server in new { - add_server(servers, server); - } -} - -fn add_server(servers: &mut Vec, server: OwnedServerName) { - if !servers.contains(&server) { - servers.push(server); - } -}