Don't set new room alias before potential 403

changelog.d/10930.bugfix

@@ -0,0 +1 @@
+Newly-created public rooms are now only assigned an alias if the room's creation has not been blocked by permission settings. Contributed by @AndrewFerr.

synapse/handlers/directory.py

@@ -146,7 +146,7 @@ async def create_association(
             if not self.config.roomdirectory.is_alias_creation_allowed(
                 user_id, room_id, room_alias_str
             ):
-                # Lets just return a generic message, as there may be all sorts of
+                # 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 create alias")
@@ -462,7 +462,7 @@ async def edit_published_room_list(
             if not self.config.roomdirectory.is_publishing_room_allowed(
                 user_id, room_id, room_aliases
             ):
-                # Lets just return a generic message, as there may be all sorts of
+                # 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")

synapse/handlers/room.py

@@ -752,6 +752,15 @@ async def create_room(
         if not allowed_by_third_party_rules:
             raise SynapseError(403, "Room visibility value not allowed.")
 
+        if is_public:
+            if not self.config.roomdirectory.is_publishing_room_allowed(
+                user_id, room_id, room_alias
+            ):
+                # 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")
+
         directory_handler = self.hs.get_directory_handler()
         if room_alias:
             await directory_handler.create_association(
@@ -762,15 +771,6 @@ async def create_room(
                 check_membership=False,
             )
 
-        if is_public:
-            if not self.config.roomdirectory.is_publishing_room_allowed(
-                user_id, room_id, room_alias
-            ):
-                # Lets 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")
-
         preset_config = config.get(
             "preset",
             RoomCreationPreset.PRIVATE_CHAT

tests/handlers/test_directory.py

@@ -432,6 +432,60 @@ def test_allowed(self):
         self.assertEquals(200, channel.code, channel.result)
 
 
+class TestCreatePublishedRoomACL(unittest.HomeserverTestCase):
+    user_id = "@admin:test"
+    denied_user_id = "@test:test"
+    data = {"room_alias_name": "unofficial_test"}
+
+    servlets = [directory.register_servlets, room.register_servlets]
+
+    def prepare(self, reactor, clock, hs):
+        # This time we add custom room list publication rules
+        config = {}
+        config["alias_creation_rules"] = []
+        config["room_list_publication_rules"] = [
+            {"user_id": "*", "alias": "*", "action": "deny"},
+            {"user_id": "@admin:test", "alias": "*", "action": "allow"},
+        ]
+
+        rd_config = RoomDirectoryConfig()
+        rd_config.read_config(config)
+
+        self.hs.config.roomdirectory.is_publishing_room_allowed = (
+            rd_config.is_publishing_room_allowed
+        )
+
+        return hs
+
+    def test_denied(self):
+        # NOTE Setting is_public=True isn't enough
+        self.data["visibility"] = "public"
+        self.helper.create_room_as(
+            self.denied_user_id, extra_content=self.data, expect_code=403
+        )
+
+    def test_allowed_without_publish(self):
+        self.helper.create_room_as(
+            self.denied_user_id,
+            extra_content=self.data,
+            is_public=False,
+            expect_code=200,
+        )
+
+    def test_allowed_as_allowed(self):
+        self.helper.create_room_as(
+            self.user_id, extra_content=self.data, is_public=False, expect_code=200
+        )
+
+    def test_denied_then_retry_without_publish(self):
+        self.test_denied()
+        self.test_allowed_without_publish()
+
+    def test_denied_then_retry_as_allowed(self):
+        self.test_denied()
+        self.test_allowed_as_allowed()
+
+
 class TestRoomListSearchDisabled(unittest.HomeserverTestCase):
     user_id = "@test:test"