Stop sending events when creating or deleting aliases (#6904)

* commit 'fe3941f6e':
  Stop sending events when creating or deleting aliases (#6904)

changelog.d/6904.removal

@@ -0,0 +1 @@
+Stop sending alias events during adding / removing aliases. Check alt_aliases in the latest canonical aliases event when deleting an alias.

synapse/handlers/directory.py

@@ -81,13 +81,7 @@ def _create_association(self, room_alias, room_id, servers=None, creator=None):
 
     @defer.inlineCallbacks
     def create_association(
-        self,
-        requester,
-        room_alias,
-        room_id,
-        servers=None,
-        send_event=True,
-        check_membership=True,
+        self, requester, room_alias, room_id, servers=None, check_membership=True,
     ):
         """Attempt to create a new alias
 
@@ -97,7 +91,6 @@ def create_association(
             room_id (str)
             servers (list[str]|None): List of servers that others servers
                 should try and join via
-            send_event (bool): Whether to send an updated m.room.aliases event
             check_membership (bool): Whether to check if the user is in the room
                 before the alias can be set (if the server's config requires it).
 
@@ -150,16 +143,9 @@ def create_association(
                 )
 
         yield self._create_association(room_alias, room_id, servers, creator=user_id)
-        if send_event:
-            try:
-                yield self.send_room_alias_update_event(requester, room_id)
-            except AuthError as e:
-                # sending the aliases event may fail due to the user not having
-                # permission in the room; this is permitted.
-                logger.info("Skipping updating aliases event due to auth error %s", e)
 
     @defer.inlineCallbacks
-    def delete_association(self, requester, room_alias, send_event=True):
+    def delete_association(self, requester, room_alias):
         """Remove an alias from the directory
 
         (this is only meant for human users; AS users should call
@@ -168,9 +154,6 @@ def delete_association(self, requester, room_alias, send_event=True):
         Args:
             requester (Requester):
             room_alias (RoomAlias):
-            send_event (bool): Whether to send an updated m.room.aliases event.
-                Note that, if we delete the canonical alias, we will always attempt
-                to send an m.room.canonical_alias event
 
         Returns:
             Deferred[unicode]: room id that the alias used to point to
@@ -206,9 +189,6 @@ def delete_association(self, requester, room_alias, send_event=True):
         room_id = yield self._delete_association(room_alias)
 
         try:
-            if send_event:
-                yield self.send_room_alias_update_event(requester, room_id)
-
             yield self._update_canonical_alias(
                 requester, requester.user.to_string(), room_id, room_alias
             )
@@ -319,25 +299,50 @@ def send_room_alias_update_event(self, requester, room_id):
 
     @defer.inlineCallbacks
     def _update_canonical_alias(self, requester, user_id, room_id, room_alias):
+        """
+        Send an updated canonical alias event if the removed alias was set as
+        the canonical alias or listed in the alt_aliases field.
+        """
         alias_event = yield self.state.get_current_state(
             room_id, EventTypes.CanonicalAlias, ""
         )
 
-        alias_str = room_alias.to_string()
-        if not alias_event or alias_event.content.get("alias", "") != alias_str:
+        # There is no canonical alias, nothing to do.
+        if not alias_event:
             return
 
-        yield self.event_creation_handler.create_and_send_nonmember_event(
-            requester,
-            {
-                "type": EventTypes.CanonicalAlias,
-                "state_key": "",
-                "room_id": room_id,
-                "sender": user_id,
-                "content": {},
-            },
-            ratelimit=False,
-        )
+        # Obtain a mutable version of the event content.
+        content = dict(alias_event.content)
+        send_update = False
+
+        # Remove the alias property if it matches the removed alias.
+        alias_str = room_alias.to_string()
+        if alias_event.content.get("alias", "") == alias_str:
+            send_update = True
+            content.pop("alias", "")
+
+        # Filter alt_aliases for the removed alias.
+        alt_aliases = content.pop("alt_aliases", None)
+        # If the aliases are not a list (or not found) do not attempt to modify
+        # the list.
+        if isinstance(alt_aliases, list):
+            send_update = True
+            alt_aliases = [alias for alias in alt_aliases if alias != alias_str]
+            if alt_aliases:
+                content["alt_aliases"] = alt_aliases
+
+        if send_update:
+            yield self.event_creation_handler.create_and_send_nonmember_event(
+                requester,
+                {
+                    "type": EventTypes.CanonicalAlias,
+                    "state_key": "",
+                    "room_id": room_id,
+                    "sender": user_id,
+                    "content": content,
+                },
+                ratelimit=False,
+            )
 
     @defer.inlineCallbacks
     def get_association_from_room_alias(self, room_alias):

synapse/handlers/room.py

@@ -492,9 +492,7 @@ def _move_aliases_to_new_room(
         for alias_str in aliases:
             alias = RoomAlias.from_string(alias_str)
             try:
-                yield directory_handler.delete_association(
-                    requester, alias, send_event=False
-                )
+                yield directory_handler.delete_association(requester, alias)
                 removed_aliases.append(alias_str)
             except SynapseError as e:
                 logger.warning("Unable to remove alias %s from old room: %s", alias, e)
@@ -525,7 +523,6 @@ def _move_aliases_to_new_room(
                     RoomAlias.from_string(alias),
                     new_room_id,
                     servers=(self.hs.hostname,),
-                    send_event=False,
                     check_membership=False,
                 )
                 logger.info("Moved alias %s to new room", alias)
@@ -681,7 +678,6 @@ def create_room(self, requester, config, ratelimit=True, creator_join_profile=No
                 room_id=room_id,
                 room_alias=room_alias,
                 servers=[self.hs.hostname],
-                send_event=False,
                 check_membership=False,
             )
 

tests/handlers/test_directory.py

@@ -18,9 +18,11 @@
 
 from twisted.internet import defer
 
+import synapse.api.errors
+from synapse.api.constants import EventTypes
 from synapse.config.room_directory import RoomDirectoryConfig
-from synapse.rest.client.v1 import directory, room
-from synapse.types import RoomAlias
+from synapse.rest.client.v1 import directory, login, room
+from synapse.types import RoomAlias, create_requester
 
 from tests import unittest
 
@@ -85,6 +87,38 @@ def test_get_remote_association(self):
             ignore_backoff=True,
         )
 
+    def test_delete_alias_not_allowed(self):
+        room_id = "!8765qwer:test"
+        self.get_success(
+            self.store.create_room_alias_association(self.my_room, room_id, ["test"])
+        )
+
+        self.get_failure(
+            self.handler.delete_association(
+                create_requester("@user:test"), self.my_room
+            ),
+            synapse.api.errors.AuthError,
+        )
+
+    def test_delete_alias(self):
+        room_id = "!8765qwer:test"
+        user_id = "@user:test"
+        self.get_success(
+            self.store.create_room_alias_association(
+                self.my_room, room_id, ["test"], user_id
+            )
+        )
+
+        result = self.get_success(
+            self.handler.delete_association(create_requester(user_id), self.my_room)
+        )
+        self.assertEquals(room_id, result)
+
+        # The alias should not be found.
+        self.get_failure(
+            self.handler.get_association(self.my_room), synapse.api.errors.SynapseError
+        )
+
     def test_incoming_fed_query(self):
         self.get_success(
             self.store.create_room_alias_association(
@@ -99,6 +133,122 @@ def test_incoming_fed_query(self):
         self.assertEquals({"room_id": "!8765asdf:test", "servers": ["test"]}, response)
 
 
+class CanonicalAliasTestCase(unittest.HomeserverTestCase):
+    """Test modifications of the canonical alias when delete aliases.
+    """
+
+    servlets = [
+        synapse.rest.admin.register_servlets,
+        login.register_servlets,
+        room.register_servlets,
+        directory.register_servlets,
+    ]
+
+    def prepare(self, reactor, clock, hs):
+        self.store = hs.get_datastore()
+        self.handler = hs.get_handlers().directory_handler
+        self.state_handler = hs.get_state_handler()
+
+        # Create user
+        self.admin_user = self.register_user("admin", "pass", admin=True)
+        self.admin_user_tok = self.login("admin", "pass")
+
+        # Create a test room
+        self.room_id = self.helper.create_room_as(
+            self.admin_user, tok=self.admin_user_tok
+        )
+
+        self.test_alias = "#test:test"
+        self.room_alias = RoomAlias.from_string(self.test_alias)
+
+        # Create a new alias to this room.
+        self.get_success(
+            self.store.create_room_alias_association(
+                self.room_alias, self.room_id, ["test"], self.admin_user
+            )
+        )
+
+    def test_remove_alias(self):
+        """Removing an alias that is the canonical alias should remove it there too."""
+        # Set this new alias as the canonical alias for this room
+        self.helper.send_state(
+            self.room_id,
+            "m.room.canonical_alias",
+            {"alias": self.test_alias, "alt_aliases": [self.test_alias]},
+            tok=self.admin_user_tok,
+        )
+
+        data = self.get_success(
+            self.state_handler.get_current_state(
+                self.room_id, EventTypes.CanonicalAlias, ""
+            )
+        )
+        self.assertEqual(data["content"]["alias"], self.test_alias)
+        self.assertEqual(data["content"]["alt_aliases"], [self.test_alias])
+
+        # Finally, delete the alias.
+        self.get_success(
+            self.handler.delete_association(
+                create_requester(self.admin_user), self.room_alias
+            )
+        )
+
+        data = self.get_success(
+            self.state_handler.get_current_state(
+                self.room_id, EventTypes.CanonicalAlias, ""
+            )
+        )
+        self.assertNotIn("alias", data["content"])
+        self.assertNotIn("alt_aliases", data["content"])
+
+    def test_remove_other_alias(self):
+        """Removing an alias listed as in alt_aliases should remove it there too."""
+        # Create a second alias.
+        other_test_alias = "#test2:test"
+        other_room_alias = RoomAlias.from_string(other_test_alias)
+        self.get_success(
+            self.store.create_room_alias_association(
+                other_room_alias, self.room_id, ["test"], self.admin_user
+            )
+        )
+
+        # Set the alias as the canonical alias for this room.
+        self.helper.send_state(
+            self.room_id,
+            "m.room.canonical_alias",
+            {
+                "alias": self.test_alias,
+                "alt_aliases": [self.test_alias, other_test_alias],
+            },
+            tok=self.admin_user_tok,
+        )
+
+        data = self.get_success(
+            self.state_handler.get_current_state(
+                self.room_id, EventTypes.CanonicalAlias, ""
+            )
+        )
+        self.assertEqual(data["content"]["alias"], self.test_alias)
+        self.assertEqual(
+            data["content"]["alt_aliases"], [self.test_alias, other_test_alias]
+        )
+
+        # Delete the second alias.
+        self.get_success(
+            self.handler.delete_association(
+                create_requester(self.admin_user), other_room_alias
+            )
+        )
+
+        data = self.get_success(
+            self.state_handler.get_current_state(
+                self.room_id, EventTypes.CanonicalAlias, ""
+            )
+        )
+        self.assertEqual(data["content"]["alias"], self.test_alias)
+        self.assertEqual(data["content"]["alt_aliases"], [self.test_alias])
+
+
 class TestCreateAliasACL(unittest.HomeserverTestCase):
     user_id = "@test:test"