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.

2 files changed, 58 insertions, 30 deletions

diff --git a/tests/config/test_room_directory.py b/tests/config/test_room_directory.py
index 574697cfd9..e25f7787f4 100644
--- a/tests/config/test_room_directory.py
+++ b/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)
diff --git a/tests/handlers/test_directory.py b/tests/handlers/test_directory.py
index 03c360cca6..cb05fecf3c 100644
--- a/tests/handlers/test_directory.py
+++ b/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):