Remove now-irrelevant test

I decided to split up the add and remove functions in order to make it easier for
callers to figure out what arguments were optional when adding vs. removing an entry.

changelog.d/9773.feature

@@ -0,0 +1 @@
+Allow homeserver admins to remove appservice rooms from the public room directory.

docs/admin_api/rooms.md

@@ -529,6 +529,21 @@ You will have to manually handle, if you so choose, the following:
 * Removal of the Content Violation room if desired.
 
 
+# Delist Room From Public Directory API
+
+Allows delisting a room from both the public room directory and any application service-specific
+room lists. The room will still exist, but not be publicly visible. Note that this **does not**
+prevent room owners or application services from adding the room to public room lists again
+afterwards.
+
+```
+    DELETE /_synapse/admin/v1/rooms/directory/<room_id_or_alias>
+```
+
+The room will be removed from both the public directory and any application service
+directories. The response body for a successful request is empty.
+
+
 # Make Room Admin API
 
 Grants another user the highest power available to a local user who is in the room.

synapse/handlers/directory.py

@@ -468,23 +468,37 @@ class DirectoryHandler(BaseHandler):
 
         await self.store.set_room_is_public(room_id, making_public)
 
-    async def edit_published_appservice_room_list(
-        self, appservice_id: str, network_id: str, room_id: str, visibility: str
-    ):
-        """Add or remove a room from the appservice/network specific public
-        room list.
+    async def add_room_to_published_appservice_room_list(
+        self, room_id: str, appservice_id: str, network_id: str
+    ) -> None:
+        """Add a room to the appservice/network specific public room list.
 
         Args:
-            appservice_id: ID of the appservice that owns the list
-            network_id: The ID of the network the list is associated with
-            room_id
-            visibility: either "public" or "private"
+            room_id: The ID of the room to add to the directory.
+            appservice_id: The ID of the appservice that owns the list.
+            network_id: The ID of the network the list is associated with.
         """
-        if visibility not in ["public", "private"]:
-            raise SynapseError(400, "Invalid visibility setting")
-
         await self.store.set_room_is_public_appservice(
-            room_id, appservice_id, network_id, visibility == "public"
+            room_id, appservice_id, network_id, True
+        )
+
+    async def remove_room_from_published_appservice_room_list(
+        self,
+        room_id: str,
+        appservice_id: Optional[str] = None,
+        network_id: Optional[str] = None,
+    ) -> None:
+        """Remove a room from the appservice/network specific public room list.
+
+        Args:
+            room_id: The ID of the room to remove from the directory.
+            appservice_id: The ID of the appservice that owns the list. If None, this function
+                will attempt to remove the room from all appservice lists.
+            network_id: The ID of the network the list is associated with. If None, this function
+                will attempt to remove the room from all appservice network lists.
+        """
+        await self.store.set_room_is_public_appservice(
+            room_id, appservice_id, network_id, False
         )
 
     async def get_aliases_for_room(

synapse/rest/admin/__init__.py

@@ -38,6 +38,7 @@ from synapse.rest.admin.media import ListMediaInRoom, register_servlets_for_medi
 from synapse.rest.admin.purge_room_servlet import PurgeRoomServlet
 from synapse.rest.admin.rooms import (
     DeleteRoomRestServlet,
+    DelistRoomFromDirectoryRestServlet,
     ForwardExtremitiesRestServlet,
     JoinRoomAliasServlet,
     ListRoomRestServlet,
@@ -214,6 +215,7 @@ def register_servlets(hs, http_server):
     Register all the admin servlets.
     """
     register_servlets_for_client_rest_resource(hs, http_server)
+    DelistRoomFromDirectoryRestServlet(hs).register(http_server)
     ListRoomRestServlet(hs).register(http_server)
     RoomStateRestServlet(hs).register(http_server)
     RoomRestServlet(hs).register(http_server)

synapse/rest/admin/rooms.py

@@ -192,6 +192,50 @@ class DeleteRoomRestServlet(RestServlet):
         return (200, ret)
 
 
+class DelistRoomFromDirectoryRestServlet(ResolveRoomIdMixin, RestServlet):
+    """Modifies both the the public room and application service directory listings
+    for a room.
+    """
+
+    PATTERNS = admin_patterns("/rooms/directory/(?P<room_identifier>[^/]*)$")
+
+    def __init__(self, hs: "HomeServer"):
+        super().__init__(hs)
+        self.hs = hs
+        self.auth = hs.get_auth()
+        self.directory_handler = hs.get_directory_handler()
+
+    async def on_DELETE(
+        self, request: SynapseRequest, room_identifier: str
+    ) -> Tuple[int, JsonDict]:
+        """Removes a room from both the public and appservice room directories by its ID
+
+        Args:
+            request: The request.
+            room_identifier: The ID of the room to remove, or an alias leading to it.
+
+        Returns:
+            A tuple of status code, response JSON dict.
+        """
+        requester = await self.auth.get_user_by_req(request)
+        await assert_user_is_admin(self.auth, requester.user)
+
+        # Get the ID of the room if an alias was provided
+        room_id, _ = await self.resolve_room_id(room_identifier)
+
+        # Remove the room from the public room directory
+        await self.directory_handler.edit_published_room_list(
+            requester, room_id, "private"
+        )
+
+        # Remove the room from all application-service public room directories
+        await self.directory_handler.remove_room_from_published_appservice_room_list(
+            room_id
+        )
+
+        return 200, {}
+
+
 class ListRoomRestServlet(RestServlet):
     """
     List all rooms that are known to the homeserver. Results are returned

synapse/rest/client/v1/directory.py

@@ -179,8 +179,17 @@ class ClientAppserviceDirectoryListServer(RestServlet):
                 403, "Only appservices can edit the appservice published room list"
             )
 
-        await self.directory_handler.edit_published_appservice_room_list(
-            requester.app_service.id, network_id, room_id, visibility
-        )
+        if visibility == "public":
+            await self.directory_handler.add_room_to_published_appservice_room_list(
+                room_id, requester.app_service.id, network_id
+            )
+        elif visibility == "private":
+            await self.directory_handler.remove_room_from_published_appservice_room_list(
+                room_id, requester.app_service.id, network_id
+            )
+        else:
+            raise SynapseError(
+                400, "Invalid visibility setting", errcode=Codes.INVALID_PARAM
+            )
 
         return 200, {}

synapse/storage/databases/main/room.py

@@ -1359,67 +1359,58 @@ class RoomStore(RoomBackgroundUpdateStore, RoomWorkerStore, SearchStore):
         self.hs.get_notifier().on_new_replication_data()
 
     async def set_room_is_public_appservice(
-        self, room_id, appservice_id, network_id, is_public
-    ):
-        """Edit the appservice/network specific public room list.
+        self,
+        room_id: str,
+        appservice_id: Optional[str],
+        network_id: Optional[str],
+        is_public: bool,
+    ) -> None:
+        """Edit a room's visibility across one or more appservice/network-specific public
+        room lists.
 
         Each appservice can have a number of published room lists associated
         with them, keyed off of an appservice defined `network_id`, which
         basically represents a single instance of a bridge to a third party
         network.
 
+        Both the appservice and network ID are optional only when removing a room from the directory.
+        If argument appservice_id or network_id are None, the room may be removed from multiple
+        appservices/networks.
+
         Args:
-            room_id (str)
-            appservice_id (str)
-            network_id (str)
-            is_public (bool): Whether to publish or unpublish the room from the
-                list.
+            room_id: The ID of the room to modify.
+            appservice_id: The ID of the network the listing is associated with. If None,
+                any room entries matching the other provided identifiers will have its
+                visibility modified.
+            network_id: The ID of the network the listing is associated with. If None,
+                any room entries matching the other provided identifiers will have its
+                visibility modified.
+            is_public: Whether to publish or unpublish the room from the list.
         """
-
-        def set_room_is_public_appservice_txn(txn, next_id):
-            if is_public:
-                try:
-                    self.db_pool.simple_insert_txn(
-                        txn,
-                        table="appservice_room_list",
-                        values={
-                            "appservice_id": appservice_id,
-                            "network_id": network_id,
-                            "room_id": room_id,
-                        },
-                    )
-                except self.database_engine.module.IntegrityError:
-                    # We've already inserted, nothing to do.
-                    return
-            else:
-                self.db_pool.simple_delete_txn(
-                    txn,
-                    table="appservice_room_list",
-                    keyvalues={
-                        "appservice_id": appservice_id,
-                        "network_id": network_id,
-                        "room_id": room_id,
-                    },
-                )
-
-            entries = self.db_pool.simple_select_list_txn(
-                txn,
-                table="public_room_list_stream",
-                keyvalues={
-                    "room_id": room_id,
-                    "appservice_id": appservice_id,
-                    "network_id": network_id,
-                },
-                retcols=("stream_id", "visibility"),
+        if is_public and (not appservice_id or not network_id):
+            raise RuntimeError(
+                "appservice_id and network_id must be set when adding "
+                "to an application service public room list"
             )
 
-            entries.sort(key=lambda r: r["stream_id"])
+        async def _add_room_to_appservice_list_txn(txn):
+            # Attempt to insert the new room entry
+            try:
+                self.db_pool.simple_insert_txn(
+                    txn,
+                    table="appservice_room_list",
+                    values={
+                        "room_id": room_id,
+                        "appservice_id": appservice_id,
+                        "network_id": network_id,
+                    },
+                )
+            except self.database_engine.module.IntegrityError:
+                # This entry already exists, so there's nothing to do.
+                return
 
-            add_to_stream = True
-            if entries:
-                add_to_stream = bool(entries[-1]["visibility"]) != is_public
-
-            if add_to_stream:
+            # Update the public room list stream with the change
+            async with self._public_room_id_gen.get_next() as next_id:
                 self.db_pool.simple_insert_txn(
                     txn,
                     table="public_room_list_stream",
@@ -1432,12 +1423,62 @@ class RoomStore(RoomBackgroundUpdateStore, RoomWorkerStore, SearchStore):
                     },
                 )
 
-        async with self._public_room_id_gen.get_next() as next_id:
-            await self.db_pool.runInteraction(
-                "set_room_is_public_appservice",
-                set_room_is_public_appservice_txn,
-                next_id,
+        async def _remove_room_from_appservice_list_txn(txn):
+            # Only filter rooms by appservice and network ID if they have been provided.
+            # This allows calls to this method to remove a room across multiple
+            # appservices and/or networks at once.
+            room_search_values = {
+                "room_id": room_id,
+            }
+            if appservice_id:
+                room_search_values["appservice_id"] = appservice_id
+            if network_id:
+                room_search_values["network_id"] = network_id
+
+            # Retrieve a list of all room/appservice/network tuples that match our search terms.
+            room_entries = self.db_pool.simple_select_list_txn(
+                txn,
+                table="appservice_room_list",
+                keyvalues=room_search_values,
+                retcols=("room_id", "appservice_id", "network_id"),
             )
+
+            # Remove the found entries and update the public room list stream with the changes
+            for entry in room_entries:
+                sql = """
+                DELETE FROM appservice_room_list
+                WHERE room_id = ?
+                    AND appservice_id = ?
+                    AND network_id = ?
+                """
+                txn.execute(
+                    sql, (entry["room_id"], entry["appservice_id"], entry["network_id"])
+                )
+
+                async with self._public_room_id_gen.get_next() as next_id:
+                    self.db_pool.simple_insert_txn(
+                        txn,
+                        table="public_room_list_stream",
+                        values={
+                            "stream_id": next_id,
+                            "room_id": entry["room_id"],
+                            "visibility": "private",
+                            "appservice_id": entry["appservice_id"],
+                            "network_id": entry["network_id"],
+                        },
+                    )
+
+        if is_public:
+            await self.db_pool.runInteraction(
+                "add_room_to_appservice_list_txn",
+                _add_room_to_appservice_list_txn,
+            )
+        else:
+            await self.db_pool.runInteraction(
+                "remove_room_from_appservice_list_txn",
+                _remove_room_from_appservice_list_txn,
+            )
+
         self.hs.get_notifier().on_new_replication_data()
 
     async def add_event_report(

tests/handlers/test_directory.py

@@ -19,7 +19,9 @@ from mock import Mock
 import synapse
 import synapse.api.errors
 from synapse.api.constants import EventTypes
+from synapse.appservice import ApplicationService
 from synapse.config.room_directory import RoomDirectoryConfig
+from synapse.rest import admin
 from synapse.rest.client.v1 import directory, login, room
 from synapse.types import RoomAlias, create_requester
 
@@ -472,3 +474,169 @@ class TestRoomListSearchDisabled(unittest.HomeserverTestCase):
             "PUT", b"directory/list/room/%s" % (room_id.encode("ascii"),), b"{}"
         )
         self.assertEquals(403, channel.code, channel.result)
+
+
+class TestAppserviceRoomDirectoryList(unittest.HomeserverTestCase):
+
+    servlets = [
+        admin.register_servlets_for_client_rest_resource,
+        directory.register_servlets,
+        login.register_servlets,
+        room.register_servlets,
+    ]
+
+    def prepare(self, reactor, clock, homeserver):
+        # Create a fake appservice
+        self.as_token = "i_am_an_app_service"
+
+        appservice = ApplicationService(
+            self.as_token,
+            self.hs.config.server_name,
+            id="1234",
+            namespaces={"users": [{"regex": r"@as_user.*", "exclusive": True}]},
+            sender="@as:test",
+        )
+        self.hs.get_datastore().services_cache.append(appservice)
+
+        # Create a user and a room
+        self.room_owner = self.register_user("room_owner", "test")
+        self.room_owner_tok = self.login("room_owner", "test")
+
+        self.room_id = self.helper.create_room_as(
+            self.room_owner, tok=self.room_owner_tok
+        )
+
+        # Create another user to poll the room directory with
+        self.user = self.register_user("user", "test")
+        self.user_tok = self.login("user", "test")
+
+        # Create a server admin to attempt delisting rooms
+        self.admin_user = self.register_user("admin", "test", admin=True)
+        self.admin_tok = self.login("admin", "test")
+
+    def test_normal_user_cannot_edit_appservice_room_directory(self):
+        """Tests that a normal user (not an appservice) cannot edit the appservice-specific
+        room directory.
+        """
+        # Attempt to list the room as a normal user
+        self._set_visibility_of_appservice_room(
+            self.room_id,
+            visible=True,
+            access_token=self.user_tok,
+            expect_code=403,
+        )
+
+        # Attempt to delist the room as a normal user
+        self._set_visibility_of_appservice_room(
+            self.room_id,
+            visible=False,
+            access_token=self.user_tok,
+            expect_code=403,
+        )
+
+    def test_add_and_remove_from_appservice_room_directory(self):
+        """Tests that we can add and remove rooms from the appservice-specific
+        room directory.
+        """
+        # Check that the room is not currently in the public room directory
+        self.assertFalse(self._is_room_in_public_room_directory(self.room_id))
+
+        # Attempt to list the room as an appservice
+        self._set_visibility_of_appservice_room(
+            self.room_id,
+            visible=True,
+        )
+
+        # Check that the room is currently in the public room directory
+        self.assertTrue(self._is_room_in_public_room_directory(self.room_id))
+
+        # Attempt to remove the room as an appservice
+        self._set_visibility_of_appservice_room(
+            self.room_id,
+            visible=False,
+        )
+
+        # Check that the room is no longer in the public room directory
+        self.assertFalse(self._is_room_in_public_room_directory(self.room_id))
+
+    def _set_visibility_of_appservice_room(
+        self,
+        room_id: str,
+        visible: bool,
+        access_token: str = None,
+        expect_code: int = 200,
+    ):
+        """Adds or removes a room from the appservice room list.
+
+        Args:
+            room_id: The ID of the room.
+            visible: True to list the room, False to delist it.
+            access_token: The access token to use for the request. If None,
+                self.as_token is used.
+            expect_code: The expected HTTP status code in the response. If None,
+                will expect 200 OK.
+        """
+        # Allow modifying the token used
+        if not access_token:
+            access_token = self.as_token
+
+        # Build the directory room list URL
+        network_id = "some_network"
+        url = f"/_matrix/client/api/v1/directory/list/appservice/{network_id}/{room_id}"
+
+        if visible:
+            # List the room
+            channel = self.make_request(
+                "PUT",
+                url,
+                {"visibility": "public"},
+                access_token=access_token,
+                shorthand=False,
+            )
+            self.assertEquals(channel.code, expect_code, channel.json_body)
+        else:
+            # Delist the room
+            channel = self.make_request(
+                "DELETE",
+                url,
+                access_token=access_token,
+                shorthand=False,
+            )
+            self.assertEquals(channel.code, expect_code, channel.json_body)
+
+    def _is_room_in_public_room_directory(self, room_id: str) -> bool:
+        """Return True or False if a given room_id is currently listed in the
+        public room directory.
+
+        Sets "include_all_networks": True in the request so that appservice
+        rooms appear as well.
+
+        Args:
+            room_id: The room ID to check the public room directory for.
+
+        Returns:
+            True or False if the room is in the public room list.
+        """
+        channel = self.make_request(
+            "POST",
+            "publicRooms",
+            # We need to make sure to specify 'include_all_networks', or our specific network,
+            # in order for the room to appear.
+            {
+                "include_all_networks": True,
+            },
+            access_token=self.user_tok,
+        )
+        self.assertEquals(200, channel.code, channel.result)
+
+        # Pull out the returned public rooms
+        public_room_chunks = channel.json_body["chunk"]
+
+        # Check that we found our listed room
+        found_room = False
+        for room_chunk in public_room_chunks:
+            if room_chunk["room_id"] == room_id:
+                found_room = True
+
+        # Return whether we found the room
+        return found_room