Allow room creation but not publishing to continue if room publication rules are violated when creating a new room. (#16811)

Prior to this PR, if a request to create a public (public as in
published to the rooms directory) room violated the room list
publication rules set in the
[config](https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html#room_list_publication_rules),
the request to create the room was denied and the room was not created.

This PR changes the behavior such that when a request to create a room
published to the directory violates room list publication rules, the
room is still created but the room is not published to the directory.

changelog.d/16811.misc

@@ -0,0 +1,2 @@
+Allow room creation but not publishing to continue if room publication rules are violated when creating
+a new room.

docs/usage/configuration/config_documentation.md

@@ -3959,6 +3959,9 @@ The first rule that matches decides if the request is allowed or denied. If no
 rule matches, the request is denied. In particular, this means that configuring
 an empty list of rules will deny every alias creation request.
 
+Requests to create a public (public as in published to the room directory) room which violates
+the configured rules will result in the room being created but not published to the room directory.
+
 Each rule is a YAML object containing four fields, each of which is an optional string:
 
 * `user_id`: a glob pattern that matches against the user publishing the room.

synapse/handlers/room.py

@@ -916,10 +916,8 @@ class RoomCreationHandler:
             if not self.config.roomdirectory.is_publishing_room_allowed(
                 user_id, room_id, room_aliases
             ):
-                # Let's just return a generic message, as there may be all sorts of
-                # reasons why we said no. TODO: Allow configurable error messages
-                # per alias creation rule?
-                raise SynapseError(403, "Not allowed to publish room")
+                # allow room creation to continue but do not publish room
+                await self.store.set_room_is_public(room_id, False)
 
         directory_handler = self.hs.get_directory_handler()
         if room_alias:

tests/config/test_room_directory.py

@@ -17,15 +17,31 @@
 # [This file includes modifications made by New Vector Limited]
 #
 #
-
 import yaml
 
+from twisted.test.proto_helpers import MemoryReactor
+
+import synapse.rest.admin
+import synapse.rest.client.login
+import synapse.rest.client.room
 from synapse.config.room_directory import RoomDirectoryConfig
+from synapse.server import HomeServer
+from synapse.util import Clock
 
 from tests import unittest
+from tests.unittest import override_config
+
 
+class RoomDirectoryConfigTestCase(unittest.HomeserverTestCase):
+    def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None:
+        self.store = hs.get_datastores().main
+
+    servlets = [
+        synapse.rest.admin.register_servlets,
+        synapse.rest.client.login.register_servlets,
+        synapse.rest.client.room.register_servlets,
+    ]
 
-class RoomDirectoryConfigTestCase(unittest.TestCase):
     def test_alias_creation_acl(self) -> None:
         config = yaml.safe_load(
             """
@@ -167,3 +183,20 @@ class RoomDirectoryConfigTestCase(unittest.TestCase):
                 aliases=["#unofficial_st:example.com", "#blah:example.com"],
             )
         )
+
+    @override_config({"room_list_publication_rules": []})
+    def test_room_creation_when_publishing_denied(self) -> None:
+        """
+        Test that when room publishing is denied via the config that new rooms can
+        still be created and that the newly created room is not public.
+        """
+
+        user = self.register_user("alice", "pass")
+        token = self.login("alice", "pass")
+        room_id = self.helper.create_room_as(user, is_public=True, tok=token)
+
+        res = self.get_success(self.store.get_room(room_id))
+        assert res is not None
+        is_public, _ = res
+
+        self.assertFalse(is_public)

tests/handlers/test_directory.py

@@ -496,19 +496,27 @@ class TestCreatePublishedRoomACL(unittest.HomeserverTestCase):
         self.denied_user_id = self.register_user("denied", "pass")
         self.denied_access_token = self.login("denied", "pass")
 
+        self.store = hs.get_datastores().main
+
     def test_denied_without_publication_permission(self) -> None:
         """
-        Try to create a room, register an alias for it, and publish it,
+        Try to create a room, register a valid alias for it, and publish it,
         as a user without permission to publish rooms.
-        (This is used as both a standalone test & as a helper function.)
+        The room should be created but not published.
         """
-        self.helper.create_room_as(
+        room_id = self.helper.create_room_as(
             self.denied_user_id,
             tok=self.denied_access_token,
             extra_content=self.data,
             is_public=True,
-            expect_code=403,
+            expect_code=200,
         )
+        res = self.get_success(self.store.get_room(room_id))
+        assert res is not None
+        is_public, _ = res
+
+        # room creation completes but room is not published to directory
+        self.assertEqual(is_public, False)
 
     def test_allowed_when_creating_private_room(self) -> None:
         """
@@ -526,9 +534,8 @@ class TestCreatePublishedRoomACL(unittest.HomeserverTestCase):
 
     def test_allowed_with_publication_permission(self) -> None:
         """
-        Try to create a room, register an alias for it, and publish it,
+        Try to create a room, register a valid alias for it, and publish it,
         as a user WITH permission to publish rooms.
-        (This is used as both a standalone test & as a helper function.)
         """
         self.helper.create_room_as(
             self.allowed_user_id,
@@ -540,38 +547,26 @@ class TestCreatePublishedRoomACL(unittest.HomeserverTestCase):
 
     def test_denied_publication_with_invalid_alias(self) -> None:
         """
-        Try to create a room, register an alias for it, and publish it,
+        Try to create a room, register an invalid alias for it, and publish it,
         as a user WITH permission to publish rooms.
         """
-        self.helper.create_room_as(
+        room_id = self.helper.create_room_as(
             self.allowed_user_id,
             tok=self.allowed_access_token,
             extra_content={"room_alias_name": "foo"},
             is_public=True,
-            expect_code=403,
+            expect_code=200,
         )
 
-    def test_can_create_as_private_room_after_rejection(self) -> None:
-        """
-        After failing to publish a room with an alias as a user without publish permission,
-        retry as the same user, but without publishing the room.
+        # the room is created with the requested alias, but the room is not published
+        res = self.get_success(self.store.get_room(room_id))
+        assert res is not None
+        is_public, _ = res
 
-        This should pass, but used to fail because the alias was registered by the first
-        request, even though the room creation was denied.
-        """
-        self.test_denied_without_publication_permission()
-        self.test_allowed_when_creating_private_room()
-
-    def test_can_create_with_permission_after_rejection(self) -> None:
-        """
-        After failing to publish a room with an alias as a user without publish permission,
-        retry as someone with permission, using the same alias.
+        self.assertFalse(is_public)
 
-        This also used to fail because of the alias having been registered by the first
-        request, leaving it unavailable for any other user's new rooms.
-        """
-        self.test_denied_without_publication_permission()
-        self.test_allowed_with_publication_permission()
+        aliases = self.get_success(self.store.get_aliases_for_room(room_id))
+        self.assertEqual(aliases[0], "#foo:test")
 
 
 class TestRoomListSearchDisabled(unittest.HomeserverTestCase):