Don't set new room alias before potential 403

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")

tests/handlers/test_directory.py

@@ -433,10 +433,9 @@ def test_allowed(self):
 
 
 class TestCreatePublishedRoomACL(unittest.HomeserverTestCase):
-    user_id = "@test:test"
-    data_template = (
-        '{"room_alias_name": "%%23unofficial_test%%3Atest", "visibility": "%s"}'
-    )
+    user_id = "@admin:test"
+    denied_user_id = "@test:test"
+    data = {"room_alias_name": "unofficial_test"}
 
     servlets = [directory.register_servlets, room.register_servlets]
 
@@ -445,7 +444,8 @@ def prepare(self, reactor, clock, hs):
         config = {}
         config["alias_creation_rules"] = []
         config["room_list_publication_rules"] = [
-            {"user_id": "*", "alias": "*", "action": "deny"}
+            {"user_id": "*", "alias": "*", "action": "deny"},
+            {"user_id": "@admin:test", "alias": "*", "action": "allow"},
         ]
 
         rd_config = RoomDirectoryConfig()
@@ -458,20 +458,32 @@ def prepare(self, reactor, clock, hs):
         return hs
 
     def test_denied(self):
-        channel = self.make_request(
-            "POST", "createRoom", (self.data_template % ("public",)).encode("ascii")
+        # 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
         )
-        self.assertEquals(403, channel.code, channel.result)
 
-    def test_allowed(self):
-        channel = self.make_request(
-            "POST", "createRoom", (self.data_template % ("private",)).encode("ascii")
+    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,
         )
-        self.assertEquals(200, channel.code, channel.result)
 
-    def test_denied_then_allowed(self):
+    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()
+        self.test_allowed_as_allowed()
 
 
 class TestRoomListSearchDisabled(unittest.HomeserverTestCase):