Merge branch 'olivia/fix-alias-reassignment' into 'main'

canonical alias fixes

Closes #116

See merge request matrix/grapevine!186

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
 
@@ -242,6 +244,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
 
@@ -324,3 +329,8 @@ 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))
+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))

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,6 +81,14 @@ pub(crate) async fn delete_alias_route(
         ));
     }
 
+    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()) {
             return Err(Error::BadRequest(
@@ -91,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<RoomCanonicalAliasEventContent> = 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.
+                //
+                // <https://spec.matrix.org/v1.14/client-server-api/#delete_matrixclientv3directoryroomroomalias>
+                info!(%error, "Failed to remove canonical alias");
+            }
+        }
+    }
 
     Ok(Ra(delete_alias::v3::Response::new()))
 }

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

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)
     }