From 07fb57be06447e0f98393008aeba436e27e26cdb Mon Sep 17 00:00:00 2001 From: Olivia Lee Date: Sat, 17 May 2025 18:52:05 -0700 Subject: [PATCH 1/4] remove local aliases from old room on upgrade Without explicitly removing the old alias when reassigning aliases, we were listing the alias on both rooms in the GET client/v3/rooms/{roomId}/aliases endpoint. Resolving the alias still always pointed to the correct room. --- book/changelog.md | 3 +++ src/service/rooms/alias.rs | 3 +++ 2 files changed, 6 insertions(+) diff --git a/book/changelog.md b/book/changelog.md index b0baf25f..8426be1a 100644 --- a/book/changelog.md +++ b/book/changelog.md @@ -242,6 +242,9 @@ This will be the first release of Grapevine since it was forked from Conduit ([!158](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/158)) 28. Fix read receipts not being sent over federation (or only arbitrarily late) ([!162](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/162)) +29. Remove local aliases from old room when assigning them to a new room during + a room upgrade. + ([!186](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/186)) ### Added diff --git a/src/service/rooms/alias.rs b/src/service/rooms/alias.rs index 638d4d92..29d72096 100644 --- a/src/service/rooms/alias.rs +++ b/src/service/rooms/alias.rs @@ -40,6 +40,9 @@ impl Service { )); } + if self.resolve_local_alias(alias)?.is_some() { + self.remove_alias(alias, user_id)?; + } self.db.set_alias(alias, room_id) } From a34bca3986c77ace3e56279bd5aaf74735ec3510 Mon Sep 17 00:00:00 2001 From: Olivia Lee Date: Sun, 18 May 2025 12:29:32 -0700 Subject: [PATCH 2/4] transfer local canonical aliases to new room on upgrade When upgrading rooms, we reassign any local aliases from the old room to the new room. This commit updates the m.room.canonical_alias events in the old and new rooms to reflect which aliases were moved. The spec is unclear on whether the server should do this[1], but it's consistent with synapse's behavior. I went with putting the canonical alias update logic inline, rather than something like add_canonical_alias and remove_canonical_alias helper functions to the alias service, because it's desirable to have the alias updates be sent as a single event than a separate event for each change. [1]: https://github.com/matrix-org/matrix-spec/issues/2142 --- book/changelog.md | 2 + src/api/client_server/room.rs | 75 +++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/book/changelog.md b/book/changelog.md index 8426be1a..3f12e7d0 100644 --- a/book/changelog.md +++ b/book/changelog.md @@ -327,3 +327,5 @@ This will be the first release of Grapevine since it was forked from Conduit ([!124](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/124)) 26. Allow adding canonical aliases from remote servers. ([!158](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/158)) +27. Transfer local canonical aliases to the new room when upgrading a room. + ([!186](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/186)) diff --git a/src/api/client_server/room.rs b/src/api/client_server/room.rs index ebb5a26b..5f1dd87a 100644 --- a/src/api/client_server/room.rs +++ b/src/api/client_server/room.rs @@ -777,6 +777,20 @@ pub(crate) async fn upgrade_room_route( } // Moves any local aliases to the new room + let mut old_canonical_alias: RoomCanonicalAliasEventContent = services() + .rooms + .state_accessor + .room_state_get(&body.room_id, &StateEventType::RoomCanonicalAlias, "")? + .and_then(|event| { + serde_json::from_str(event.content.get()) + .inspect_err(|_| { + Error::bad_database("invalid m.room.canonical_alias event"); + }) + .ok() + }) + .unwrap_or_default(); + let mut new_canonical_alias = RoomCanonicalAliasEventContent::default(); + for alias in services() .rooms .alias @@ -788,6 +802,67 @@ pub(crate) async fn upgrade_room_route( &replacement_room, sender_user, )?; + + if old_canonical_alias.alias.as_ref() == Some(&alias) { + new_canonical_alias.alias = Some(alias.clone()); + old_canonical_alias.alias = None; + } + let mut is_alt_alias = false; + old_canonical_alias.alt_aliases.retain(|a| { + let equal = a == &alias; + is_alt_alias |= equal; + !equal + }); + if is_alt_alias { + new_canonical_alias.alt_aliases.push(alias); + } + } + + // Send updated events to both rooms if we moved any canonical aliases + let new_canonical_alias_empty = new_canonical_alias.alias.is_none() + && new_canonical_alias.alt_aliases.is_empty(); + if !new_canonical_alias_empty { + if let Err(error) = services() + .rooms + .timeline + .build_and_append_pdu( + PduBuilder { + event_type: TimelineEventType::RoomCanonicalAlias, + content: to_raw_value(&old_canonical_alias) + .expect("event is valid"), + unsigned: None, + state_key: Some(String::new()), + redacts: None, + }, + sender_user, + &original_room_token, + ) + .await + { + // The sender may not have had permission to send + // m.room.canonical_alias in the old room. Don't treat + // this as fatal. + warn!( + %error, + "Failed to remove local canonical aliases from old room" + ); + } + services() + .rooms + .timeline + .build_and_append_pdu( + PduBuilder { + event_type: TimelineEventType::RoomCanonicalAlias, + content: to_raw_value(&new_canonical_alias) + .expect("event is valid"), + unsigned: None, + state_key: Some(String::new()), + redacts: None, + }, + sender_user, + &replacement_room_token, + ) + .await?; } // Get the old room power levels From 5f9e85cb49b95ff8b3af0e513cf40f77c5cf1b2b Mon Sep 17 00:00:00 2001 From: Olivia Lee Date: Sun, 18 May 2025 13:30:42 -0700 Subject: [PATCH 3/4] return an error when attempting to delete a nonexistent local alias The spec doesn't spell out explicitly that the server should check this, but it does list M_NOT_FOUND "room alias ... not found" as an example error response. --- book/changelog.md | 2 ++ src/api/client_server/alias.rs | 7 +++++++ 2 files changed, 9 insertions(+) diff --git a/book/changelog.md b/book/changelog.md index 3f12e7d0..d552c3fa 100644 --- a/book/changelog.md +++ b/book/changelog.md @@ -153,6 +153,8 @@ This will be the first release of Grapevine since it was forked from Conduit 14. Use trust-dns for all DNS queries, instead of only for SRV records and SRV record targets in server discovery. ([!156](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/156)) +15. Return an error on when attempting to delete a nonexistent local alias. + ([!186](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/186)) ### Fixed diff --git a/src/api/client_server/alias.rs b/src/api/client_server/alias.rs index b2fbcf65..5ec165f5 100644 --- a/src/api/client_server/alias.rs +++ b/src/api/client_server/alias.rs @@ -75,6 +75,13 @@ pub(crate) async fn delete_alias_route( )); } + if services().rooms.alias.resolve_local_alias(&body.room_alias)?.is_none() { + return Err(Error::BadRequest( + ErrorKind::NotFound, + "Alias is not assigned.", + )); + } + if let Some(info) = &body.appservice_info { if !info.aliases.is_match(body.room_alias.as_str()) { return Err(Error::BadRequest( From 0f9568902d960b490a9fbe2eba8b3ef245a0e79d Mon Sep 17 00:00:00 2001 From: Olivia Lee Date: Sun, 18 May 2025 13:54:52 -0700 Subject: [PATCH 4/4] attempt to remove canonical alias entries when deleting local aliases The spec says that this behavior is optional, but it's what synapse does and we had a TODO for it. --- book/changelog.md | 3 ++ src/api/client_server/alias.rs | 82 ++++++++++++++++++++++++++++++---- 2 files changed, 77 insertions(+), 8 deletions(-) diff --git a/book/changelog.md b/book/changelog.md index d552c3fa..c02ba451 100644 --- a/book/changelog.md +++ b/book/changelog.md @@ -331,3 +331,6 @@ This will be the first release of Grapevine since it was forked from Conduit ([!158](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/158)) 27. Transfer local canonical aliases to the new room when upgrading a room. ([!186](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/186)) +28. Attempt to remove alias from m.room.canonical_alias event when deleting a + local alias. + ([!186](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/186)) diff --git a/src/api/client_server/alias.rs b/src/api/client_server/alias.rs index 5ec165f5..06febd74 100644 --- a/src/api/client_server/alias.rs +++ b/src/api/client_server/alias.rs @@ -8,10 +8,16 @@ use ruma::{ }, federation, }, + events::{ + room::canonical_alias::RoomCanonicalAliasEventContent, StateEventType, + TimelineEventType, + }, OwnedRoomAliasId, }; +use serde_json::value::to_raw_value; +use tracing::info; -use crate::{services, Ar, Error, Ra, Result}; +use crate::{service::pdu::PduBuilder, services, Ar, Error, Ra, Result}; /// # `PUT /_matrix/client/r0/directory/room/{roomAlias}` /// @@ -75,12 +81,13 @@ pub(crate) async fn delete_alias_route( )); } - if services().rooms.alias.resolve_local_alias(&body.room_alias)?.is_none() { - return Err(Error::BadRequest( - ErrorKind::NotFound, - "Alias is not assigned.", - )); - } + let room_id = services() + .rooms + .alias + .resolve_local_alias(&body.room_alias)? + .ok_or_else(|| { + Error::BadRequest(ErrorKind::NotFound, "Alias is not assigned.") + })?; if let Some(info) = &body.appservice_info { if !info.aliases.is_match(body.room_alias.as_str()) { @@ -98,7 +105,66 @@ pub(crate) async fn delete_alias_route( services().rooms.alias.remove_alias(&body.room_alias, sender_user)?; - // TODO: update alt_aliases? + // Attempt to remove the alias from the m.room.canonical_alias event, if it + // was present. + let canonical_alias: Option = services() + .rooms + .state_accessor + .room_state_get(&room_id, &StateEventType::RoomCanonicalAlias, "")? + .and_then(|event| { + serde_json::from_str(event.content.get()) + .inspect_err(|_| { + Error::bad_database("invalid m.room.canonical_alias event"); + }) + .ok() + }); + if let Some(mut canonical_alias) = canonical_alias { + let mut changed = false; + if canonical_alias.alias.as_ref() == Some(&body.room_alias) { + canonical_alias.alias = None; + changed = true; + } + canonical_alias.alt_aliases.retain(|a| { + let equal = a == &body.room_alias; + changed |= equal; + !equal + }); + + if changed { + let room_token = services() + .globals + .roomid_mutex_state + .lock_key(room_id.clone()) + .await; + + if let Err(error) = services() + .rooms + .timeline + .build_and_append_pdu( + PduBuilder { + event_type: TimelineEventType::RoomCanonicalAlias, + content: to_raw_value(&canonical_alias) + .expect("event is valid"), + unsigned: None, + state_key: Some(String::new()), + redacts: None, + }, + sender_user, + &room_token, + ) + .await + { + // > Servers which choose to update the canonical alias event + // > are recommended to, in addition to their other relevant + // > permission checks, delete the alias and return a successful + // > response even if the user does not have permission to + // > update the m.room.canonical_alias event. + // + // + info!(%error, "Failed to remove canonical alias"); + } + } + } Ok(Ra(delete_alias::v3::Response::new())) }