From 699fa5b61eea652b7795d677bc03c56905096858 Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Fri, 11 Sep 2020 16:09:30 +0100
Subject: [PATCH] Address review comments

Generalise public rooms check, allow blocking removal from public rooms dir as well.

Remove the check on alias association, that is unrelated.

Note that we now execute the same behaviour as when publishing the room is
blocked by the homeserver config. That is, we now 403 the /createRoom call
instead of just letting the room be created without entering it into the public
rooms directory.

Finally, only call check_visibility_can_be_modified if it exists.
---
 synapse/events/third_party_rules.py | 20 +++++++++++++-------
 synapse/handlers/room.py            | 28 +++++++++++++++-------------
 2 files changed, 28 insertions(+), 20 deletions(-)

diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py
index 253b025c27..2b16df8671 100644
--- a/synapse/events/third_party_rules.py
+++ b/synapse/events/third_party_rules.py
@@ -12,6 +12,8 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+from typing import Callable
+
 from synapse.events import EventBase
 from synapse.events.snapshot import EventContext
 from synapse.types import Requester, StateMap
@@ -151,25 +153,29 @@ class ThirdPartyEventRules:
         )
         return ret
 
-    async def check_room_can_be_added_to_public_rooms_directory(
-        self, room_id: str
+    async def check_visibility_can_be_modified(
+        self, room_id: str, new_visibility: str
     ) -> bool:
-        """Check if a room is allowed to be published to the public rooms directory.
+        """Check if a room is allowed to be published to, or removed from, the public rooms
+        directory.
 
         Args:
             room_id: The ID of the room.
+            new_visibility: The new visibility state. Either "public" or "private".
 
         Returns:
-            Whether the room is allowed to be published to the public rooms directory.
+            True if the room's visibility can be modified, False if not.
         """
         if self.third_party_rules is None:
             return True
 
+        check_func = getattr(self.third_party_rules, "check_visibility_can_be_modified")
+        if not check_func or not isinstance(check_func, Callable):
+            return True
+
         state_events = await self._get_state_events_dict_for_room(room_id)
 
-        return await self.third_party_rules.check_room_can_be_added_to_public_rooms_directory(
-            room_id, state_events
-        )
+        return await check_func(room_id, state_events, new_visibility)
 
     async def _get_state_events_dict_for_room(
         self, room_id: str
diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py
index 7a508e3157..4c76294221 100644
--- a/synapse/handlers/room.py
+++ b/synapse/handlers/room.py
@@ -681,22 +681,24 @@ class RoomCreationHandler(BaseHandler):
             creator_id=user_id, is_public=is_public, room_version=room_version,
         )
 
+        # Check whether this visibility value is blocked by a third party module
+        allowed_by_third_party_rules = await (
+            self.third_party_event_rules.check_visibility_can_be_modified(
+                room_id, visibility
+            )
+        )
+        if not allowed_by_third_party_rules:
+            raise SynapseError(403, "Room visibility value not allowed.")
+
         directory_handler = self.hs.get_handlers().directory_handler
         if room_alias:
-            # Check if publishing is blocked by a third party module
-            allowed_by_third_party_rules = await (
-                self.third_party_event_rules.check_room_can_be_added_to_public_rooms_directory(
-                    room_id
-                )
+            await directory_handler.create_association(
+                requester=requester,
+                room_id=room_id,
+                room_alias=room_alias,
+                servers=[self.hs.hostname],
+                check_membership=False,
             )
-            if allowed_by_third_party_rules:
-                await directory_handler.create_association(
-                    requester=requester,
-                    room_id=room_id,
-                    room_alias=room_alias,
-                    servers=[self.hs.hostname],
-                    check_membership=False,
-                )
 
         if is_public:
             if not self.config.is_publishing_room_allowed(user_id, room_id, room_alias):