From 2dc86862418f8e761b25e3e2eb94a25bc0401ea4 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Tue, 13 Jul 2021 18:46:24 +0100
Subject: [PATCH 01/13] First cut at porting the ThirdPartyEventRules module
 interface to the new generic one

---
 docs/sample_config.yaml                   |  13 --
 synapse/app/_base.py                      |   2 +
 synapse/config/third_party_event_rules.py |  15 --
 synapse/events/third_party_rules.py       | 180 ++++++++++++++++++----
 synapse/module_api/__init__.py            |   6 +
 5 files changed, 158 insertions(+), 58 deletions(-)

diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml
index a45732a246b3..be1f192d6691 100644
--- a/docs/sample_config.yaml
+++ b/docs/sample_config.yaml
@@ -2739,19 +2739,6 @@ stats:
 #    action: allow
 
 
-# Server admins can define a Python module that implements extra rules for
-# allowing or denying incoming events. In order to work, this module needs to
-# override the methods defined in synapse/events/third_party_rules.py.
-#
-# This feature is designed to be used in closed federations only, where each
-# participating server enforces the same rules.
-#
-#third_party_event_rules:
-#  module: "my_custom_project.SuperRulesSet"
-#  config:
-#    example_option: 'things'
-
-
 ## Opentracing ##
 
 # These settings enable opentracing, which implements distributed tracing.
diff --git a/synapse/app/_base.py b/synapse/app/_base.py
index b30571fe495b..50a02f51f54a 100644
--- a/synapse/app/_base.py
+++ b/synapse/app/_base.py
@@ -38,6 +38,7 @@
 from synapse.config.homeserver import HomeServerConfig
 from synapse.crypto import context_factory
 from synapse.events.spamcheck import load_legacy_spam_checkers
+from synapse.events.third_party_rules import load_legacy_third_party_event_rules
 from synapse.logging.context import PreserveLoggingContext
 from synapse.metrics.background_process_metrics import wrap_as_background_process
 from synapse.metrics.jemalloc import setup_jemalloc_stats
@@ -368,6 +369,7 @@ def run_sighup(*args, **kwargs):
         module(config=config, api=module_api)
 
     load_legacy_spam_checkers(hs)
+    load_legacy_third_party_event_rules(hs)
 
     # If we've configured an expiry time for caches, start the background job now.
     setup_expire_lru_cache_entries(hs)
diff --git a/synapse/config/third_party_event_rules.py b/synapse/config/third_party_event_rules.py
index f502ff539e27..a3fae0242082 100644
--- a/synapse/config/third_party_event_rules.py
+++ b/synapse/config/third_party_event_rules.py
@@ -28,18 +28,3 @@ def read_config(self, config, **kwargs):
             self.third_party_event_rules = load_module(
                 provider, ("third_party_event_rules",)
             )
-
-    def generate_config_section(self, **kwargs):
-        return """\
-        # Server admins can define a Python module that implements extra rules for
-        # allowing or denying incoming events. In order to work, this module needs to
-        # override the methods defined in synapse/events/third_party_rules.py.
-        #
-        # This feature is designed to be used in closed federations only, where each
-        # participating server enforces the same rules.
-        #
-        #third_party_event_rules:
-        #  module: "my_custom_project.SuperRulesSet"
-        #  config:
-        #    example_option: 'things'
-        """
diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py
index f7944fd8344f..9b43928ff824 100644
--- a/synapse/events/third_party_rules.py
+++ b/synapse/events/third_party_rules.py
@@ -11,17 +11,96 @@
 # 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 TYPE_CHECKING, Union, Callable, List, Optional, Awaitable
 
-from typing import TYPE_CHECKING, Union
-
+from synapse.api.errors import SynapseError
 from synapse.events import EventBase
 from synapse.events.snapshot import EventContext
 from synapse.types import Requester, StateMap
+from synapse.util.async_helpers import maybe_awaitable
 
 if TYPE_CHECKING:
     from synapse.server import HomeServer
 
 
+CHECK_EVENT_ALLOWED_CALLBACK = Callable[
+    [EventBase, EventContext], Awaitable[Union[bool, dict]]
+]
+ON_CREATE_ROOM_CALLBACK = Callable[[Requester, dict, bool], Awaitable]
+CHECK_THREEPID_CAN_BE_INVITED_CALLBACK = Callable[
+    [str, str, StateMap[EventBase]], Awaitable[bool]
+]
+CHECK_VISIBILITY_CAN_BE_MODIFIED_CALLBACK = Callable[
+    [str, StateMap[EventBase], str], Awaitable[bool]
+]
+
+
+def load_legacy_third_party_event_rules(hs: "HomeServer"):
+    """Wrapper that loads a third party event rules module configured using the old
+    configuration, and registers the hooks they implement.
+    """
+    if hs.config.third_party_event_rules:
+        module, config = hs.config.third_party_event_rules
+    else:
+        return
+
+    api = hs.get_module_api()
+    third_party_rules = module(config=config, module_api=api)
+
+    # The known hooks. If a module implements a method which name appears in this set,
+    # we'll want to register it.
+    third_party_event_rules_methods = {
+        "check_event_allowed",
+        "on_create_room",
+        "check_threepid_can_be_invited",
+        "check_visibility_can_be_modified",
+    }
+
+    def async_wrapper(f: Optional[Callable]) -> Optional[Callable[..., Awaitable]]:
+        # f might be None if the callback isn't implemented by the module. In this
+        # case we don't want to register a callback at all so we return None.
+        if f is None:
+            return None
+
+        if f.__name__ == "on_create_room":
+            # We return a separate wrapper for on_create_room because, in order to wrap
+            # it correctly, we need to await its result. Therefore it doesn't make a lot
+            # of sense to make it go through the run() wrapper.
+            async def wrapper(
+                requester: Requester, config: dict, is_requester_admin: bool
+            ) -> None:
+                # We've already made sure f is not None above, but mypy doesn't do well
+                # across function boundaries so we need to tell it f is definitely not
+                # None.
+                assert f is not None
+
+                res = await f(requester, config, is_requester_admin)
+                if res is False:
+                    raise SynapseError(
+                        403,
+                        "Room creation forbidden with these parameters",
+                    )
+
+            return wrapper
+
+        def run(*args, **kwargs):
+            # mypy doesn't do well across function boundaries so we need to tell it
+            # f is definitely not None.
+            assert f is not None
+
+            return maybe_awaitable(f(*args, **kwargs))
+
+        return run
+
+    # Register the hooks through the module API.
+    hooks = {
+        hook: async_wrapper(getattr(third_party_rules, hook, None))
+        for hook in third_party_event_rules_methods
+    }
+
+    api.register_spam_checker_callbacks(**hooks)
+
+
 class ThirdPartyEventRules:
     """Allows server admins to provide a Python module implementing an extra
     set of rules to apply when processing events.
@@ -46,6 +125,43 @@ def __init__(self, hs: "HomeServer"):
                 module_api=hs.get_module_api(),
             )
 
+        self._check_event_allowed_callbacks: List[CHECK_EVENT_ALLOWED_CALLBACK] = []
+        self._on_create_room_callbacks: List[ON_CREATE_ROOM_CALLBACK] = []
+        self._check_threepid_can_be_invited_callbacks: List[
+            CHECK_THREEPID_CAN_BE_INVITED_CALLBACK
+        ] = []
+        self._check_visibility_can_be_modified_callbacks: List[
+            CHECK_VISIBILITY_CAN_BE_MODIFIED_CALLBACK
+        ] = []
+
+    def register_third_party_rules_callbacks(
+        self,
+        check_event_allowed: Optional[CHECK_EVENT_ALLOWED_CALLBACK] = None,
+        on_create_room: Optional[ON_CREATE_ROOM_CALLBACK] = None,
+        check_threepid_can_be_invited: Optional[
+            CHECK_THREEPID_CAN_BE_INVITED_CALLBACK
+        ] = None,
+        check_visibility_can_be_modified: Optional[
+            CHECK_VISIBILITY_CAN_BE_MODIFIED_CALLBACK
+        ] = None,
+    ):
+        """Register callbacks from module for each hook."""
+        if check_event_allowed is not None:
+            self._check_event_allowed_callbacks.append(check_event_allowed)
+
+        if on_create_room is not None:
+            self._on_create_room_callbacks.append(on_create_room)
+
+        if check_threepid_can_be_invited is not None:
+            self._check_threepid_can_be_invited_callbacks.append(
+                check_threepid_can_be_invited,
+            )
+
+        if check_visibility_can_be_modified is not None:
+            self._check_visibility_can_be_modified_callbacks.append(
+                check_visibility_can_be_modified,
+            )
+
     async def check_event_allowed(
         self, event: EventBase, context: EventContext
     ) -> Union[bool, dict]:
@@ -63,7 +179,8 @@ async def check_event_allowed(
         Returns:
             The result from the ThirdPartyRules module, as above
         """
-        if self.third_party_rules is None:
+        # Bail out early without hitting the store if we don't have any callback
+        if len(self._check_event_allowed_callbacks) == 0:
             return True
 
         prev_state_ids = await context.get_prev_state_ids()
@@ -77,29 +194,31 @@ async def check_event_allowed(
         # the hashes and signatures.
         event.freeze()
 
-        return await self.third_party_rules.check_event_allowed(event, state_events)
+        for callback in self._check_event_allowed_callbacks:
+            res = await callback(event, state_events)
+            # Return if the event shouldn't be allowed or if the module came up with a
+            # replacement content for the event.
+            # TODO: is this really the right place to let modules replace the event's
+            #  content (or should it happen in another callback, or as a second return
+            #  argument)?
+            if not res or isinstance(res, dict):
+                return res
+
+        return True
 
     async def on_create_room(
         self, requester: Requester, config: dict, is_requester_admin: bool
-    ) -> bool:
-        """Intercept requests to create room to allow, deny or update the
-        request config.
+    ) -> None:
+        """Intercept requests to create room to maybe deny it (via an exception) or
+        update the request config.
 
         Args:
             requester
             config: The creation config from the client.
             is_requester_admin: If the requester is an admin
-
-        Returns:
-            Whether room creation is allowed or denied.
         """
-
-        if self.third_party_rules is None:
-            return True
-
-        return await self.third_party_rules.on_create_room(
-            requester, config, is_requester_admin
-        )
+        for callback in self._on_create_room_callbacks:
+            await callback(requester, config, is_requester_admin)
 
     async def check_threepid_can_be_invited(
         self, medium: str, address: str, room_id: str
@@ -114,15 +233,17 @@ async def check_threepid_can_be_invited(
         Returns:
             True if the 3PID can be invited, False if not.
         """
-
-        if self.third_party_rules is None:
+        # Bail out early without hitting the store if we don't have any callback
+        if len(self._check_threepid_can_be_invited_callbacks) == 0:
             return True
 
         state_events = await self._get_state_map_for_room(room_id)
 
-        return await self.third_party_rules.check_threepid_can_be_invited(
-            medium, address, state_events
-        )
+        for callback in self._check_threepid_can_be_invited_callbacks:
+            if await callback(medium, address, state_events) is False:
+                return False
+
+        return True
 
     async def check_visibility_can_be_modified(
         self, room_id: str, new_visibility: str
@@ -137,18 +258,17 @@ async def check_visibility_can_be_modified(
         Returns:
             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", None
-        )
-        if not check_func or not callable(check_func):
+        # Bail out early without hitting the store if we don't have any callback
+        if len(self._check_visibility_can_be_modified_callbacks) == 0:
             return True
 
         state_events = await self._get_state_map_for_room(room_id)
 
-        return await check_func(room_id, state_events, new_visibility)
+        for callback in self._check_visibility_can_be_modified_callbacks:
+            if await callback(room_id, state_events, new_visibility) is False:
+                return False
+
+        return True
 
     async def _get_state_map_for_room(self, room_id: str) -> StateMap[EventBase]:
         """Given a room ID, return the state events of that room.
diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py
index 721c45abac70..098f2ac09999 100644
--- a/synapse/module_api/__init__.py
+++ b/synapse/module_api/__init__.py
@@ -58,6 +58,7 @@ def __init__(self, hs: "HomeServer", auth_handler):
         self._public_room_list_manager = PublicRoomListManager(hs)
 
         self._spam_checker = hs.get_spam_checker()
+        self._third_party_event_rules = hs.get_third_party_event_rules()
 
     #################################################################################
     # The following methods should only be called during the module's initialisation.
@@ -67,6 +68,11 @@ def register_spam_checker_callbacks(self):
         """Registers callbacks for spam checking capabilities."""
         return self._spam_checker.register_callbacks
 
+    @property
+    def register_third_party_rules_callbacks(self):
+        """Registers callbacks for third party event rules capabilities."""
+        return self._third_party_event_rules.register_third_party_rules_callbacks
+
     def register_web_resource(self, path: str, resource: IResource):
         """Registers a web resource to be served at the given path.
 

From 0101a8fdf5fdb2e796bdf00ecd6341b7ac02871b Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Wed, 14 Jul 2021 11:59:39 +0100
Subject: [PATCH 02/13] Don't expect a return value from on_create_room

---
 synapse/handlers/room.py | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py
index 579b1b93c5fa..2438ccd665ad 100644
--- a/synapse/handlers/room.py
+++ b/synapse/handlers/room.py
@@ -618,15 +618,11 @@ async def create_room(
         else:
             is_requester_admin = await self.auth.is_server_admin(requester.user)
 
-        # Check whether the third party rules allows/changes the room create
-        # request.
-        event_allowed = await self.third_party_event_rules.on_create_room(
+        # Let the third party rules modify the room creation config if needed, or abort
+        # the room creation entirely with an exception.
+        await self.third_party_event_rules.on_create_room(
             requester, config, is_requester_admin=is_requester_admin
         )
-        if not event_allowed:
-            raise SynapseError(
-                403, "You are not permitted to create rooms", Codes.FORBIDDEN
-            )
 
         if not is_requester_admin and not await self.spam_checker.user_may_create_room(
             user_id

From 9d7fe3f3e8aa3be00ca1ab4d038425e4dffcb8db Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Wed, 14 Jul 2021 12:11:34 +0100
Subject: [PATCH 03/13] Make return values of check_event_allowed slightly less
 hacky

---
 synapse/events/third_party_rules.py | 50 ++++++++++++++++++++---------
 synapse/handlers/federation.py      |  4 +--
 synapse/handlers/message.py         |  8 ++---
 3 files changed, 41 insertions(+), 21 deletions(-)

diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py
index 9b43928ff824..f17faa140cc7 100644
--- a/synapse/events/third_party_rules.py
+++ b/synapse/events/third_party_rules.py
@@ -11,7 +11,7 @@
 # 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 TYPE_CHECKING, Union, Callable, List, Optional, Awaitable
+from typing import TYPE_CHECKING, Awaitable, Callable, List, Optional, Tuple, Union
 
 from synapse.api.errors import SynapseError
 from synapse.events import EventBase
@@ -24,7 +24,7 @@
 
 
 CHECK_EVENT_ALLOWED_CALLBACK = Callable[
-    [EventBase, EventContext], Awaitable[Union[bool, dict]]
+    [EventBase, EventContext], Awaitable[Tuple[bool, Optional[dict]]]
 ]
 ON_CREATE_ROOM_CALLBACK = Callable[[Requester, dict, bool], Awaitable]
 CHECK_THREEPID_CAN_BE_INVITED_CALLBACK = Callable[
@@ -62,10 +62,29 @@ def async_wrapper(f: Optional[Callable]) -> Optional[Callable[..., Awaitable]]:
         if f is None:
             return None
 
+        # We return a separate wrapper for these methods because, in order to wrap them
+        # correctly, we need to await its result. Therefore it doesn't make a lot of
+        # sense to make it go through the run() wrapper.
+        if f.__name__ == "check_event_allowed":
+            async def wrapper(
+                event: EventBase,
+                state_events: StateMap[EventBase],
+            ) -> Tuple[bool, Optional[dict]]:
+                # We've already made sure f is not None above, but mypy doesn't do well
+                # across function boundaries so we need to tell it f is definitely not
+                # None.
+                assert f is not None
+
+                res = await f(event, state_events)
+                if isinstance(res, dict):
+                    return True, res
+                else:
+                    return res, None
+
+            return wrapper
+
         if f.__name__ == "on_create_room":
-            # We return a separate wrapper for on_create_room because, in order to wrap
-            # it correctly, we need to await its result. Therefore it doesn't make a lot
-            # of sense to make it go through the run() wrapper.
+
             async def wrapper(
                 requester: Requester, config: dict, is_requester_admin: bool
             ) -> None:
@@ -164,20 +183,22 @@ def register_third_party_rules_callbacks(
 
     async def check_event_allowed(
         self, event: EventBase, context: EventContext
-    ) -> Union[bool, dict]:
+    ) -> Tuple[bool, dict]:
         """Check if a provided event should be allowed in the given context.
 
         The module can return:
             * True: the event is allowed.
             * False: the event is not allowed, and should be rejected with M_FORBIDDEN.
-            * a dict: replacement event data.
+
+        If the event is allowed, the module can also return a dictionary to use as a
+        replacement for the event.
 
         Args:
             event: The event to be checked.
             context: The context of the event.
 
         Returns:
-            The result from the ThirdPartyRules module, as above
+            The result from the ThirdPartyRules module, as above.
         """
         # Bail out early without hitting the store if we don't have any callback
         if len(self._check_event_allowed_callbacks) == 0:
@@ -195,16 +216,15 @@ async def check_event_allowed(
         event.freeze()
 
         for callback in self._check_event_allowed_callbacks:
-            res = await callback(event, state_events)
+            res, new_content = await callback(event, state_events)
             # Return if the event shouldn't be allowed or if the module came up with a
             # replacement content for the event.
-            # TODO: is this really the right place to let modules replace the event's
-            #  content (or should it happen in another callback, or as a second return
-            #  argument)?
-            if not res or isinstance(res, dict):
-                return res
+            if res is False:
+                return res, None
+            elif isinstance(new_content, dict):
+                return True, new_content
 
-        return True
+        return True, None
 
     async def on_create_room(
         self, requester: Requester, config: dict, is_requester_admin: bool
diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index 991ec9919a95..c73abe696b48 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -1931,7 +1931,7 @@ async def on_make_knock_request(
             builder=builder
         )
 
-        event_allowed = await self.third_party_event_rules.check_event_allowed(
+        event_allowed, _ = await self.third_party_event_rules.check_event_allowed(
             event, context
         )
         if not event_allowed:
@@ -2023,7 +2023,7 @@ async def on_send_membership_event(
         # for knock events, we run the third-party event rules. It's not entirely clear
         # why we don't do this for other sorts of membership events.
         if event.membership == Membership.KNOCK:
-            event_allowed = await self.third_party_event_rules.check_event_allowed(
+            event_allowed, _ = await self.third_party_event_rules.check_event_allowed(
                 event, context
             )
             if not event_allowed:
diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py
index e06655f3d43b..ab567f458990 100644
--- a/synapse/handlers/message.py
+++ b/synapse/handlers/message.py
@@ -953,10 +953,10 @@ async def create_new_client_event(
         if requester:
             context.app_service = requester.app_service
 
-        third_party_result = await self.third_party_event_rules.check_event_allowed(
+        res, new_content = await self.third_party_event_rules.check_event_allowed(
             event, context
         )
-        if not third_party_result:
+        if res is False:
             logger.info(
                 "Event %s forbidden by third-party rules",
                 event,
@@ -964,11 +964,11 @@ async def create_new_client_event(
             raise SynapseError(
                 403, "This event is not allowed in this context", Codes.FORBIDDEN
             )
-        elif isinstance(third_party_result, dict):
+        elif new_content is not None:
             # the third-party rules want to replace the event. We'll need to build a new
             # event.
             event, context = await self._rebuild_event_after_third_party_rules(
-                third_party_result, event
+                new_content, event
             )
 
         self.validator.validate_new(event, self.config)

From 761eb6a539b4a57e0e5cd6f3c8fa56e5b7aa5817 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Wed, 14 Jul 2021 15:06:58 +0100
Subject: [PATCH 04/13] Add docs and deprecation notice

---
 changelog.d/10386.removal           |  1 +
 docs/modules.md                     | 57 ++++++++++++++++++++++++++++-
 docs/upgrade.md                     | 13 +++++++
 synapse/events/third_party_rules.py | 11 +++---
 4 files changed, 76 insertions(+), 6 deletions(-)
 create mode 100644 changelog.d/10386.removal

diff --git a/changelog.d/10386.removal b/changelog.d/10386.removal
new file mode 100644
index 000000000000..800a6143d7f8
--- /dev/null
+++ b/changelog.d/10386.removal
@@ -0,0 +1 @@
+The third-party event rules module interface is deprecated in favour of the generic module interface introduced in Synapse v1.37.0. See the [upgrade notes](https://matrix-org.github.io/synapse/latest/upgrade.html#upgrading-to-v1390) for more information.
diff --git a/docs/modules.md b/docs/modules.md
index bec1c06d15f4..81478aa84fea 100644
--- a/docs/modules.md
+++ b/docs/modules.md
@@ -181,13 +181,68 @@ The arguments passed to this callback are:
 ```python
 async def check_media_file_for_spam(
     file_wrapper: "synapse.rest.media.v1.media_storage.ReadableFileWrapper",
-    file_info: "synapse.rest.media.v1._base.FileInfo"
+    file_info: "synapse.rest.media.v1._base.FileInfo",
 ) -> bool
 ```
 
 Called when storing a local or remote file. The module must return a boolean indicating
 whether the given file can be stored in the homeserver's media store.
 
+#### Third party rules callbacks
+
+Third party rules callbacks allow modules developers to add extra checks to verify the
+validity of incoming events. Third party event rules callbacks can be registered using
+the module API's `register_third_party_rules_callbacks` method.
+
+The available third party rules callbacks are:
+
+```python
+async def check_event_allowed(
+    event: "synapse.events.EventBase",
+    state_events: "synapse.types.StateMap",
+) -> Tuple[bool, Optional[dict]]
+```
+
+Called when processing any incoming event, with the event and the a `StateMap`
+representing the current state of the room the event is being sent into. A `StateMap` is
+a dictionary indexed on tuples containing an event type and a state key; for example
+retrieving the room's `m.room.create` event from the `state_events` argument looks like
+this: `state_events.get(("m.room.create", ""))`. The module must return a boolean
+indicating whether the event can be allowed.
+
+Note that this callback function processes incoming events coming via federation
+traffic (on top of client traffic). This means denying an event might cause the local
+copy of the room's history to diverge from the ones of remote servers. This may cause
+federation issues in the room. It is strongly recommended to only deny events using this
+callback function if the sender is a local user, or in a private federation in which all
+servers are using the same module, with the same configuration.
+
+If the boolean returned by the module is `True`, it may also tell Synapse to replace the
+event with new data by returning the new event's data as a dictionary. In order to do
+that, it is recommended the module calls `event.get_dict()` to get the current event as a
+dictionary, and modify the returned dictionary accordingly.
+
+Note that replacing the event only works for events sent by local users, not for events
+received over federation.
+
+```python
+async def on_create_room(
+    requester: "synapse.types.Requester",
+    request_content: dict,
+    is_requester_admin: bool,
+) -> None
+```
+
+Called when processing a room creation request, with the `Requester` object for the user
+performing the request, a dictionary representing the room creation request's JSON body
+(see [the spec](https://matrix.org/docs/spec/client_server/latest#post-matrix-client-r0-createroom)
+for a list of possible parameters), and a boolean indicating whether the user performing
+the request is a server admin.
+
+Modules can modify the `request_content` (by e.g. adding events to its `initial_state`),
+or deny the room's creation by raising a `module_api.errors.SynapseError`.
+
+
 ### Porting an existing module that uses the old interface
 
 In order to port a module that uses Synapse's old module interface, its author needs to:
diff --git a/docs/upgrade.md b/docs/upgrade.md
index db0450f563a2..a45193880971 100644
--- a/docs/upgrade.md
+++ b/docs/upgrade.md
@@ -86,6 +86,19 @@ process, for example:
     ```
 
 
+# Upgrading to v1.39.0
+
+## Deprecation of the current third-party rules module interface
+
+The current third-party rules module interface is deprecated in favour of the new generic
+modules system introduced in Synapse v1.37.0. Authors of third-party rules modules can refer
+to [this documentation](modules.md#porting-an-existing-module-that-uses-the-old-interface)
+to update their modules. Synapse administrators can refer to [this documentation](modules.md#using-modules)
+to update their configuration once the modules they are using have been updated.
+
+We plan to remove support for the current spam checker interface in September 2021.
+
+
 # Upgrading to v1.38.0
 
 ## Re-indexing of `events` table on Postgres databases
diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py
index f17faa140cc7..2d61cdd88e57 100644
--- a/synapse/events/third_party_rules.py
+++ b/synapse/events/third_party_rules.py
@@ -24,7 +24,7 @@
 
 
 CHECK_EVENT_ALLOWED_CALLBACK = Callable[
-    [EventBase, EventContext], Awaitable[Tuple[bool, Optional[dict]]]
+    [EventBase, StateMap[EventBase]], Awaitable[Tuple[bool, Optional[dict]]]
 ]
 ON_CREATE_ROOM_CALLBACK = Callable[[Requester, dict, bool], Awaitable]
 CHECK_THREEPID_CAN_BE_INVITED_CALLBACK = Callable[
@@ -66,6 +66,7 @@ def async_wrapper(f: Optional[Callable]) -> Optional[Callable[..., Awaitable]]:
         # correctly, we need to await its result. Therefore it doesn't make a lot of
         # sense to make it go through the run() wrapper.
         if f.__name__ == "check_event_allowed":
+
             async def wrapper(
                 event: EventBase,
                 state_events: StateMap[EventBase],
@@ -216,13 +217,13 @@ async def check_event_allowed(
         event.freeze()
 
         for callback in self._check_event_allowed_callbacks:
-            res, new_content = await callback(event, state_events)
+            res, replacement_data = await callback(event, state_events)
             # Return if the event shouldn't be allowed or if the module came up with a
-            # replacement content for the event.
+            # replacement dict for the event.
             if res is False:
                 return res, None
-            elif isinstance(new_content, dict):
-                return True, new_content
+            elif isinstance(replacement_data, dict):
+                return True, replacement_data
 
         return True, None
 

From 56e35f9eefa4921804408e76a2992df765a84612 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Wed, 14 Jul 2021 15:48:12 +0100
Subject: [PATCH 05/13] Make mypy happy

---
 synapse/events/third_party_rules.py | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py
index 2d61cdd88e57..ec5aaa33c8ff 100644
--- a/synapse/events/third_party_rules.py
+++ b/synapse/events/third_party_rules.py
@@ -67,7 +67,7 @@ def async_wrapper(f: Optional[Callable]) -> Optional[Callable[..., Awaitable]]:
         # sense to make it go through the run() wrapper.
         if f.__name__ == "check_event_allowed":
 
-            async def wrapper(
+            async def wrap_check_event_allowed(
                 event: EventBase,
                 state_events: StateMap[EventBase],
             ) -> Tuple[bool, Optional[dict]]:
@@ -82,11 +82,11 @@ async def wrapper(
                 else:
                     return res, None
 
-            return wrapper
+            return wrap_check_event_allowed
 
         if f.__name__ == "on_create_room":
 
-            async def wrapper(
+            async def wrap_on_create_room(
                 requester: Requester, config: dict, is_requester_admin: bool
             ) -> None:
                 # We've already made sure f is not None above, but mypy doesn't do well
@@ -101,7 +101,7 @@ async def wrapper(
                         "Room creation forbidden with these parameters",
                     )
 
-            return wrapper
+            return wrap_on_create_room
 
         def run(*args, **kwargs):
             # mypy doesn't do well across function boundaries so we need to tell it
@@ -184,7 +184,7 @@ def register_third_party_rules_callbacks(
 
     async def check_event_allowed(
         self, event: EventBase, context: EventContext
-    ) -> Tuple[bool, dict]:
+    ) -> Tuple[bool, Optional[dict]]:
         """Check if a provided event should be allowed in the given context.
 
         The module can return:
@@ -203,7 +203,7 @@ async def check_event_allowed(
         """
         # Bail out early without hitting the store if we don't have any callback
         if len(self._check_event_allowed_callbacks) == 0:
-            return True
+            return True, None
 
         prev_state_ids = await context.get_prev_state_ids()
 

From d0a8542e14de3737386f6f092efb7c8bae5fcb31 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Wed, 14 Jul 2021 16:16:21 +0100
Subject: [PATCH 06/13] Remove unused import

---
 synapse/events/third_party_rules.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py
index ec5aaa33c8ff..73b417277ce9 100644
--- a/synapse/events/third_party_rules.py
+++ b/synapse/events/third_party_rules.py
@@ -11,7 +11,7 @@
 # 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 TYPE_CHECKING, Awaitable, Callable, List, Optional, Tuple, Union
+from typing import TYPE_CHECKING, Awaitable, Callable, List, Optional, Tuple
 
 from synapse.api.errors import SynapseError
 from synapse.events import EventBase

From 2d8d12d5a89b3b38cfd0683e41110b17db6774dc Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Fri, 16 Jul 2021 11:48:14 +0100
Subject: [PATCH 07/13] Fix tests

---
 docs/modules.md                             |   4 +
 synapse/events/third_party_rules.py         |  13 +--
 tests/rest/client/test_third_party_rules.py | 106 +++++++++++++++-----
 3 files changed, 84 insertions(+), 39 deletions(-)

diff --git a/docs/modules.md b/docs/modules.md
index 81478aa84fea..b85819018812 100644
--- a/docs/modules.md
+++ b/docs/modules.md
@@ -203,6 +203,10 @@ async def check_event_allowed(
 ) -> Tuple[bool, Optional[dict]]
 ```
 
+**<span style="color:red">
+This callback is very experimental and can and will break without notice.
+</span>**
+
 Called when processing any incoming event, with the event and the a `StateMap`
 representing the current state of the room the event is being sent into. A `StateMap` is
 a dictionary indexed on tuples containing an event type and a state key; for example
diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py
index 73b417277ce9..3d92da99982d 100644
--- a/synapse/events/third_party_rules.py
+++ b/synapse/events/third_party_rules.py
@@ -118,7 +118,7 @@ def run(*args, **kwargs):
         for hook in third_party_event_rules_methods
     }
 
-    api.register_spam_checker_callbacks(**hooks)
+    api.register_third_party_rules_callbacks(**hooks)
 
 
 class ThirdPartyEventRules:
@@ -134,17 +134,6 @@ def __init__(self, hs: "HomeServer"):
 
         self.store = hs.get_datastore()
 
-        module = None
-        config = None
-        if hs.config.third_party_event_rules:
-            module, config = hs.config.third_party_event_rules
-
-        if module is not None:
-            self.third_party_rules = module(
-                config=config,
-                module_api=hs.get_module_api(),
-            )
-
         self._check_event_allowed_callbacks: List[CHECK_EVENT_ALLOWED_CALLBACK] = []
         self._on_create_room_callbacks: List[ON_CREATE_ROOM_CALLBACK] = []
         self._check_threepid_can_be_invited_callbacks: List[
diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py
index c5e1c5458bef..504e1b61283e 100644
--- a/tests/rest/client/test_third_party_rules.py
+++ b/tests/rest/client/test_third_party_rules.py
@@ -16,17 +16,19 @@
 from unittest.mock import Mock
 
 from synapse.events import EventBase
+from synapse.events.third_party_rules import load_legacy_third_party_event_rules
 from synapse.module_api import ModuleApi
 from synapse.rest import admin
 from synapse.rest.client.v1 import login, room
 from synapse.types import Requester, StateMap
+from synapse.util.frozenutils import unfreeze
 
 from tests import unittest
 
 thread_local = threading.local()
 
 
-class ThirdPartyRulesTestModule:
+class LegacyThirdPartyRulesTestModule:
     def __init__(self, config: Dict, module_api: ModuleApi):
         # keep a record of the "current" rules module, so that the test can patch
         # it if desired.
@@ -36,20 +38,20 @@ def __init__(self, config: Dict, module_api: ModuleApi):
     async def on_create_room(
         self, requester: Requester, config: dict, is_requester_admin: bool
     ):
-        return True
+        return False
 
     async def check_event_allowed(self, event: EventBase, state: StateMap[EventBase]):
-        return True
+        d = event.get_dict()
+        content = unfreeze(event.content)
+        content["foo"] = "bar"
+        d["content"] = content
+        return d
 
     @staticmethod
     def parse_config(config):
         return config
 
 
-def current_rules_module() -> ThirdPartyRulesTestModule:
-    return thread_local.rules_module
-
-
 class ThirdPartyRulesTestCase(unittest.HomeserverTestCase):
     servlets = [
         admin.register_servlets,
@@ -57,20 +59,23 @@ class ThirdPartyRulesTestCase(unittest.HomeserverTestCase):
         room.register_servlets,
     ]
 
-    def default_config(self):
-        config = super().default_config()
-        config["third_party_event_rules"] = {
-            "module": __name__ + ".ThirdPartyRulesTestModule",
-            "config": {},
-        }
-        return config
+    def make_homeserver(self, reactor, clock):
+        hs = self.setup_test_homeserver()
+
+        load_legacy_third_party_event_rules(hs)
+
+        return hs
 
     def prepare(self, reactor, clock, homeserver):
         # Create a user and room to play with during the tests
         self.user_id = self.register_user("kermit", "monkey")
         self.tok = self.login("kermit", "monkey")
 
-        self.room_id = self.helper.create_room_as(self.user_id, tok=self.tok)
+        # Some tests might prevent room creation on purpose.
+        try:
+            self.room_id = self.helper.create_room_as(self.user_id, tok=self.tok)
+        except Exception as e:
+            pass
 
     def test_third_party_rules(self):
         """Tests that a forbidden event is forbidden from being sent, but an allowed one
@@ -79,10 +84,10 @@ def test_third_party_rules(self):
         # patch the rules module with a Mock which will return False for some event
         # types
         async def check(ev, state):
-            return ev.type != "foo.bar.forbidden"
+            return ev.type != "foo.bar.forbidden", None
 
         callback = Mock(spec=[], side_effect=check)
-        current_rules_module().check_event_allowed = callback
+        self.hs.get_third_party_event_rules()._check_event_allowed_callbacks = [callback]
 
         channel = self.make_request(
             "PUT",
@@ -116,9 +121,9 @@ def test_cannot_modify_event(self):
         # first patch the event checker so that it will try to modify the event
         async def check(ev: EventBase, state):
             ev.content = {"x": "y"}
-            return True
+            return True, None
 
-        current_rules_module().check_event_allowed = check
+        self.hs.get_third_party_event_rules()._check_event_allowed_callbacks = [check]
 
         # now send the event
         channel = self.make_request(
@@ -135,9 +140,9 @@ def test_modify_event(self):
         async def check(ev: EventBase, state):
             d = ev.get_dict()
             d["content"] = {"x": "y"}
-            return d
+            return True, d
 
-        current_rules_module().check_event_allowed = check
+        self.hs.get_third_party_event_rules()._check_event_allowed_callbacks = [check]
 
         # now send the event
         channel = self.make_request(
@@ -168,9 +173,9 @@ async def check(ev: EventBase, state):
                 "msgtype": "m.text",
                 "body": d["content"]["body"].upper(),
             }
-            return d
+            return True, d
 
-        current_rules_module().check_event_allowed = check
+        self.hs.get_third_party_event_rules()._check_event_allowed_callbacks = [check]
 
         # Send an event, then edit it.
         channel = self.make_request(
@@ -222,7 +227,7 @@ async def check(ev: EventBase, state):
         self.assertEqual(ev["content"]["body"], "EDITED BODY")
 
     def test_send_event(self):
-        """Tests that the module can send an event into a room via the module api"""
+        """Tests that a module can send an event into a room via the module api"""
         content = {
             "msgtype": "m.text",
             "body": "Hello!",
@@ -234,12 +239,59 @@ def test_send_event(self):
             "sender": self.user_id,
         }
         event: EventBase = self.get_success(
-            current_rules_module().module_api.create_and_send_event_into_room(
-                event_dict
-            )
+            self.hs.get_module_api().create_and_send_event_into_room(event_dict)
         )
 
         self.assertEquals(event.sender, self.user_id)
         self.assertEquals(event.room_id, self.room_id)
         self.assertEquals(event.type, "m.room.message")
         self.assertEquals(event.content, content)
+
+    @unittest.override_config(
+        {
+            "third_party_event_rules": {
+                "module": __name__ + ".LegacyThirdPartyRulesTestModule",
+                "config": {},
+            }
+        }
+    )
+    def test_legacy_check_event_allowed(self):
+        """Tests that the wrapper for legacy check_event_allowed callbacks works
+        correctly.
+        """
+        channel = self.make_request(
+            "PUT",
+            "/_matrix/client/r0/rooms/%s/send/m.room.message/1" % self.room_id,
+            {
+                "msgtype": "m.text",
+                "body": "Original body",
+            },
+            access_token=self.tok,
+        )
+        self.assertEqual(channel.result["code"], b"200", channel.result)
+
+        event_id = channel.json_body["event_id"]
+
+        channel = self.make_request(
+            "GET",
+            "/_matrix/client/r0/rooms/%s/event/%s" % (self.room_id, event_id),
+            access_token=self.tok,
+        )
+        self.assertEqual(channel.result["code"], b"200", channel.result)
+
+        self.assertIn("foo", channel.json_body["content"].keys())
+        self.assertEqual(channel.json_body["content"]["foo"], "bar")
+
+    @unittest.override_config(
+        {
+            "third_party_event_rules": {
+                "module": __name__ + ".LegacyThirdPartyRulesTestModule",
+                "config": {},
+            }
+        }
+    )
+    def test_legacy_on_create_room(self):
+        """Tests that the wrapper for legacy on_create_room callbacks works
+        correctly.
+        """
+        self.helper.create_room_as(self.user_id, tok=self.tok, expect_code=403)
\ No newline at end of file

From a1e4dcc5b8a931abbfed7eeb952c0083451c6f1e Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Fri, 16 Jul 2021 13:41:19 +0200
Subject: [PATCH 08/13] Apply suggestions from code review

Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
---
 docs/modules.md                     |  6 +++---
 docs/upgrade.md                     |  2 +-
 synapse/events/third_party_rules.py | 12 ++++++------
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/docs/modules.md b/docs/modules.md
index b85819018812..5bc1e478b311 100644
--- a/docs/modules.md
+++ b/docs/modules.md
@@ -190,7 +190,7 @@ whether the given file can be stored in the homeserver's media store.
 
 #### Third party rules callbacks
 
-Third party rules callbacks allow modules developers to add extra checks to verify the
+Third party rules callbacks allow module developers to add extra checks to verify the
 validity of incoming events. Third party event rules callbacks can be registered using
 the module API's `register_third_party_rules_callbacks` method.
 
@@ -207,7 +207,7 @@ async def check_event_allowed(
 This callback is very experimental and can and will break without notice.
 </span>**
 
-Called when processing any incoming event, with the event and the a `StateMap`
+Called when processing any incoming event, with the event and a `StateMap`
 representing the current state of the room the event is being sent into. A `StateMap` is
 a dictionary indexed on tuples containing an event type and a state key; for example
 retrieving the room's `m.room.create` event from the `state_events` argument looks like
@@ -216,7 +216,7 @@ indicating whether the event can be allowed.
 
 Note that this callback function processes incoming events coming via federation
 traffic (on top of client traffic). This means denying an event might cause the local
-copy of the room's history to diverge from the ones of remote servers. This may cause
+copy of the room's history to diverge from that of remote servers. This may cause
 federation issues in the room. It is strongly recommended to only deny events using this
 callback function if the sender is a local user, or in a private federation in which all
 servers are using the same module, with the same configuration.
diff --git a/docs/upgrade.md b/docs/upgrade.md
index a45193880971..c8f4a2c17172 100644
--- a/docs/upgrade.md
+++ b/docs/upgrade.md
@@ -96,7 +96,7 @@ to [this documentation](modules.md#porting-an-existing-module-that-uses-the-old-
 to update their modules. Synapse administrators can refer to [this documentation](modules.md#using-modules)
 to update their configuration once the modules they are using have been updated.
 
-We plan to remove support for the current spam checker interface in September 2021.
+We plan to remove support for the current third-party rules interface in September 2021.
 
 
 # Upgrading to v1.38.0
diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py
index 3d92da99982d..dac67d0a95dc 100644
--- a/synapse/events/third_party_rules.py
+++ b/synapse/events/third_party_rules.py
@@ -39,11 +39,11 @@ def load_legacy_third_party_event_rules(hs: "HomeServer"):
     """Wrapper that loads a third party event rules module configured using the old
     configuration, and registers the hooks they implement.
     """
-    if hs.config.third_party_event_rules:
-        module, config = hs.config.third_party_event_rules
-    else:
+    if hs.config.third_party_event_rules is None:
         return
 
+    module, config = hs.config.third_party_event_rules
+
     api = hs.get_module_api()
     third_party_rules = module(config=config, module_api=api)
 
@@ -154,7 +154,7 @@ def register_third_party_rules_callbacks(
             CHECK_VISIBILITY_CAN_BE_MODIFIED_CALLBACK
         ] = None,
     ):
-        """Register callbacks from module for each hook."""
+        """Register callbacks from modules for each hook."""
         if check_event_allowed is not None:
             self._check_event_allowed_callbacks.append(check_event_allowed)
 
@@ -190,7 +190,7 @@ async def check_event_allowed(
         Returns:
             The result from the ThirdPartyRules module, as above.
         """
-        # Bail out early without hitting the store if we don't have any callback
+        # Bail out early without hitting the store if we don't have any callbacks to run.
         if len(self._check_event_allowed_callbacks) == 0:
             return True, None
 
@@ -243,7 +243,7 @@ async def check_threepid_can_be_invited(
         Returns:
             True if the 3PID can be invited, False if not.
         """
-        # Bail out early without hitting the store if we don't have any callback
+        # Bail out early without hitting the store if we don't have any callbacks to run.
         if len(self._check_threepid_can_be_invited_callbacks) == 0:
             return True
 

From 2256debf829352a5eb7140f65fa6623887860bf0 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Fri, 16 Jul 2021 15:44:48 +0100
Subject: [PATCH 09/13] Incorporate review

---
 docs/modules.md                             | 11 ++++---
 synapse/events/third_party_rules.py         | 35 +++++++++++++++++----
 tests/rest/client/test_third_party_rules.py |  8 +++--
 3 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/docs/modules.md b/docs/modules.md
index 5bc1e478b311..e066f4911687 100644
--- a/docs/modules.md
+++ b/docs/modules.md
@@ -204,15 +204,16 @@ async def check_event_allowed(
 ```
 
 **<span style="color:red">
-This callback is very experimental and can and will break without notice.
+This callback is very experimental and can and will break without notice. Module developers
+are encouraged to implement `check_event_for_spam` from the spam checker category instead.
 </span>**
 
 Called when processing any incoming event, with the event and a `StateMap`
 representing the current state of the room the event is being sent into. A `StateMap` is
-a dictionary indexed on tuples containing an event type and a state key; for example
-retrieving the room's `m.room.create` event from the `state_events` argument looks like
-this: `state_events.get(("m.room.create", ""))`. The module must return a boolean
-indicating whether the event can be allowed.
+a dictionary that maps tuples containing an event type and a state key to the
+corresponding state event. For example retrieving the room's `m.room.create` event from
+the `state_events` argument would look like this: `state_events.get(("m.room.create", ""))`.
+The module must return a boolean indicating whether the event can be allowed.
 
 Note that this callback function processes incoming events coming via federation
 traffic (on top of client traffic). This means denying an event might cause the local
diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py
index dac67d0a95dc..a7bcf410c9a0 100644
--- a/synapse/events/third_party_rules.py
+++ b/synapse/events/third_party_rules.py
@@ -11,6 +11,7 @@
 # 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.
+import logging
 from typing import TYPE_CHECKING, Awaitable, Callable, List, Optional, Tuple
 
 from synapse.api.errors import SynapseError
@@ -22,6 +23,8 @@
 if TYPE_CHECKING:
     from synapse.server import HomeServer
 
+logger = logging.getLogger(__name__)
+
 
 CHECK_EVENT_ALLOWED_CALLBACK = Callable[
     [EventBase, StateMap[EventBase]], Awaitable[Tuple[bool, Optional[dict]]]
@@ -67,6 +70,9 @@ def async_wrapper(f: Optional[Callable]) -> Optional[Callable[..., Awaitable]]:
         # sense to make it go through the run() wrapper.
         if f.__name__ == "check_event_allowed":
 
+            # We need to wrap check_event_allowed because its old form would return either
+            # a boolean or a dict, but now we want to return the dict separately from the
+            # boolean.
             async def wrap_check_event_allowed(
                 event: EventBase,
                 state_events: StateMap[EventBase],
@@ -86,6 +92,9 @@ async def wrap_check_event_allowed(
 
         if f.__name__ == "on_create_room":
 
+            # We need to wrap on_create_room because its old form would return a boolean
+            # if the room creation is denied, but now we just want it to raise an
+            # exception.
             async def wrap_on_create_room(
                 requester: Requester, config: dict, is_requester_admin: bool
             ) -> None:
@@ -206,7 +215,12 @@ async def check_event_allowed(
         event.freeze()
 
         for callback in self._check_event_allowed_callbacks:
-            res, replacement_data = await callback(event, state_events)
+            try:
+                res, replacement_data = await callback(event, state_events)
+            except Exception as e:
+                logger.warning("Failed to run callback %s: %s", callback, e)
+                continue
+
             # Return if the event shouldn't be allowed or if the module came up with a
             # replacement dict for the event.
             if res is False:
@@ -228,7 +242,10 @@ async def on_create_room(
             is_requester_admin: If the requester is an admin
         """
         for callback in self._on_create_room_callbacks:
-            await callback(requester, config, is_requester_admin)
+            try:
+                await callback(requester, config, is_requester_admin)
+            except Exception as e:
+                logger.warning("Failed to run callback %s: %s", callback, e)
 
     async def check_threepid_can_be_invited(
         self, medium: str, address: str, room_id: str
@@ -250,8 +267,11 @@ async def check_threepid_can_be_invited(
         state_events = await self._get_state_map_for_room(room_id)
 
         for callback in self._check_threepid_can_be_invited_callbacks:
-            if await callback(medium, address, state_events) is False:
-                return False
+            try:
+                if await callback(medium, address, state_events) is False:
+                    return False
+            except Exception as e:
+                logger.warning("Failed to run callback %s: %s", callback, e)
 
         return True
 
@@ -275,8 +295,11 @@ async def check_visibility_can_be_modified(
         state_events = await self._get_state_map_for_room(room_id)
 
         for callback in self._check_visibility_can_be_modified_callbacks:
-            if await callback(room_id, state_events, new_visibility) is False:
-                return False
+            try:
+                if await callback(room_id, state_events, new_visibility) is False:
+                    return False
+            except Exception as e:
+                logger.warning("Failed to run callback %s: %s", callback, e)
 
         return True
 
diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py
index 504e1b61283e..fb797eef8e60 100644
--- a/tests/rest/client/test_third_party_rules.py
+++ b/tests/rest/client/test_third_party_rules.py
@@ -74,7 +74,7 @@ def prepare(self, reactor, clock, homeserver):
         # Some tests might prevent room creation on purpose.
         try:
             self.room_id = self.helper.create_room_as(self.user_id, tok=self.tok)
-        except Exception as e:
+        except Exception:
             pass
 
     def test_third_party_rules(self):
@@ -87,7 +87,9 @@ async def check(ev, state):
             return ev.type != "foo.bar.forbidden", None
 
         callback = Mock(spec=[], side_effect=check)
-        self.hs.get_third_party_event_rules()._check_event_allowed_callbacks = [callback]
+        self.hs.get_third_party_event_rules()._check_event_allowed_callbacks = [
+            callback
+        ]
 
         channel = self.make_request(
             "PUT",
@@ -294,4 +296,4 @@ def test_legacy_on_create_room(self):
         """Tests that the wrapper for legacy on_create_room callbacks works
         correctly.
         """
-        self.helper.create_room_as(self.user_id, tok=self.tok, expect_code=403)
\ No newline at end of file
+        self.helper.create_room_as(self.user_id, tok=self.tok, expect_code=403)

From 93a58458ea3f9040a33d6c065eaba5419d0e6114 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Mon, 19 Jul 2021 14:34:56 +0100
Subject: [PATCH 10/13] Fix tests

---
 synapse/events/third_party_rules.py         |  8 +++++++-
 tests/rest/client/test_third_party_rules.py | 14 +++++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py
index a7bcf410c9a0..4c47b7ac4f90 100644
--- a/synapse/events/third_party_rules.py
+++ b/synapse/events/third_party_rules.py
@@ -245,7 +245,13 @@ async def on_create_room(
             try:
                 await callback(requester, config, is_requester_admin)
             except Exception as e:
-                logger.warning("Failed to run callback %s: %s", callback, e)
+                # Don't silence the errors raised by this callback since we expect it to
+                # raise an exception to deny the creation of the room; instead make sure
+                # it's a SynapseError we can send to clients.
+                if not isinstance(e, SynapseError):
+                    e = SynapseError(403, "Room creation forbidden with these parameters")
+
+                raise e
 
     async def check_threepid_can_be_invited(
         self, medium: str, address: str, room_id: str
diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py
index fb797eef8e60..80dfb5022029 100644
--- a/tests/rest/client/test_third_party_rules.py
+++ b/tests/rest/client/test_third_party_rules.py
@@ -134,7 +134,19 @@ async def check(ev: EventBase, state):
             {"x": "x"},
             access_token=self.tok,
         )
-        self.assertEqual(channel.result["code"], b"500", channel.result)
+        # check_event_allowed has some error handling, so it shouldn't 500 just because a
+        # module did something bad.
+        self.assertEqual(channel.code, 200, channel.result)
+        event_id = channel.json_body["event_id"]
+
+        channel = self.make_request(
+            "GET",
+            "/_matrix/client/r0/rooms/%s/event/%s" % (self.room_id, event_id),
+            access_token=self.tok,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+        ev = channel.json_body
+        self.assertEqual(ev["content"]["x"], "x")
 
     def test_modify_event(self):
         """The module can return a modified version of the event"""

From a56c5931a994922eee7d148bb010b9da31743652 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Mon, 19 Jul 2021 14:39:38 +0100
Subject: [PATCH 11/13] Lint

---
 synapse/events/third_party_rules.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py
index 4c47b7ac4f90..1776074db4c0 100644
--- a/synapse/events/third_party_rules.py
+++ b/synapse/events/third_party_rules.py
@@ -249,7 +249,9 @@ async def on_create_room(
                 # raise an exception to deny the creation of the room; instead make sure
                 # it's a SynapseError we can send to clients.
                 if not isinstance(e, SynapseError):
-                    e = SynapseError(403, "Room creation forbidden with these parameters")
+                    e = SynapseError(
+                        403, "Room creation forbidden with these parameters"
+                    )
 
                 raise e
 

From 13d1c863485a2edfc5ad7bda7abc78bb53f4a126 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Mon, 19 Jul 2021 15:24:31 +0100
Subject: [PATCH 12/13] Fix legacy compat test

---
 tests/rest/client/test_third_party_rules.py | 30 ++++++++++++++++-----
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py
index 80dfb5022029..28dd47a28bf4 100644
--- a/tests/rest/client/test_third_party_rules.py
+++ b/tests/rest/client/test_third_party_rules.py
@@ -37,9 +37,31 @@ def __init__(self, config: Dict, module_api: ModuleApi):
 
     async def on_create_room(
         self, requester: Requester, config: dict, is_requester_admin: bool
+    ):
+        return True
+
+    async def check_event_allowed(self, event: EventBase, state: StateMap[EventBase]):
+        return True
+
+    @staticmethod
+    def parse_config(config):
+        return config
+
+
+class LegacyDenyNewRooms(LegacyThirdPartyRulesTestModule):
+    def __init__(self, config: Dict, module_api: ModuleApi):
+        super().__init__(config, module_api)
+
+    def on_create_room(
+        self, requester: Requester, config: dict, is_requester_admin: bool
     ):
         return False
 
+
+class LegacyChangeEvents(LegacyThirdPartyRulesTestModule):
+    def __init__(self, config: Dict, module_api: ModuleApi):
+        super().__init__(config, module_api)
+
     async def check_event_allowed(self, event: EventBase, state: StateMap[EventBase]):
         d = event.get_dict()
         content = unfreeze(event.content)
@@ -47,10 +69,6 @@ async def check_event_allowed(self, event: EventBase, state: StateMap[EventBase]
         d["content"] = content
         return d
 
-    @staticmethod
-    def parse_config(config):
-        return config
-
 
 class ThirdPartyRulesTestCase(unittest.HomeserverTestCase):
     servlets = [
@@ -264,7 +282,7 @@ def test_send_event(self):
     @unittest.override_config(
         {
             "third_party_event_rules": {
-                "module": __name__ + ".LegacyThirdPartyRulesTestModule",
+                "module": __name__ + ".LegacyChangeEvents",
                 "config": {},
             }
         }
@@ -299,7 +317,7 @@ def test_legacy_check_event_allowed(self):
     @unittest.override_config(
         {
             "third_party_event_rules": {
-                "module": __name__ + ".LegacyThirdPartyRulesTestModule",
+                "module": __name__ + ".LegacyDenyNewRooms",
                 "config": {},
             }
         }

From 6f665bc397fbbd095ae2629d62fdb1c088a43080 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Tue, 20 Jul 2021 12:30:48 +0200
Subject: [PATCH 13/13] Apply suggestions from code review

Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
---
 synapse/events/third_party_rules.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py
index 1776074db4c0..7a6eb3e51667 100644
--- a/synapse/events/third_party_rules.py
+++ b/synapse/events/third_party_rules.py
@@ -218,7 +218,7 @@ async def check_event_allowed(
             try:
                 res, replacement_data = await callback(event, state_events)
             except Exception as e:
-                logger.warning("Failed to run callback %s: %s", callback, e)
+                logger.warning("Failed to run module API callback %s: %s", callback, e)
                 continue
 
             # Return if the event shouldn't be allowed or if the module came up with a
@@ -279,7 +279,7 @@ async def check_threepid_can_be_invited(
                 if await callback(medium, address, state_events) is False:
                     return False
             except Exception as e:
-                logger.warning("Failed to run callback %s: %s", callback, e)
+                logger.warning("Failed to run module API callback %s: %s", callback, e)
 
         return True
 
@@ -307,7 +307,7 @@ async def check_visibility_can_be_modified(
                 if await callback(room_id, state_events, new_visibility) is False:
                     return False
             except Exception as e:
-                logger.warning("Failed to run callback %s: %s", callback, e)
+                logger.warning("Failed to run module API callback %s: %s", callback, e)
 
         return True