Port the ThirdPartyEventRules module interface to the new generic interface

docs/modules.md

@@ -193,9 +193,39 @@ async def check_media_file_for_spam(
 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.
 
+#### Account validity callbacks
+
+Account validity callbacks allow module developers to add extra steps to verify the
+validity on an account, i.e. see if a user can be granted access to their account on the
+Synapse instance. Account validity callbacks can be registered using the module API's
+`register_account_validity_callbacks` method.
+
+The available account validity callbacks are:
+
+```python
+async def is_user_expired(user: str) -> Optional[bool]
+```
+
+Called when processing any authenticated request (except for logout requests). The module
+can return a `bool` to indicate whether the user has expired and should be locked out of
+their account, or `None` if the module wasn't able to figure it out. The user is
+represented by their Matrix user ID (e.g. `@alice:example.com`).
+
+If the module returns `True`, the current request will be denied with the error code
+`ORG_MATRIX_EXPIRED_ACCOUNT` and the HTTP status code 403. Note that this doesn't
+invalidate the user's access token.
+
+```python
+async def on_user_registration(user: str) -> None
+```
+
+Called after successfully registering a user, in case the module needs to perform extra
+operations to keep track of them. (e.g. add them to a database table). The user is
+represented by their Matrix user ID.
+
 #### 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.
 
@@ -208,16 +238,21 @@ async def check_event_allowed(
 ) -> Tuple[bool, Optional[dict]]
 ```
 
-Called when processing any incoming event, with the event and the a `StateMap`
+**<span style="color:red">
+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
-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.

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

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]]]
@@ -39,11 +42,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)
 
@@ -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:
@@ -118,7 +127,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 +143,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[
@@ -165,7 +163,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)
 
@@ -201,7 +199,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
 
@@ -217,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:
@@ -239,7 +242,18 @@ 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:
+                # 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
@@ -254,15 +268,18 @@ 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
 
         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
 
@@ -286,8 +303,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
 

synapse/module_api/__init__.py

@@ -109,6 +109,7 @@ def __init__(self, hs: "HomeServer", auth_handler):
         self._public_room_list_manager = PublicRoomListManager(hs)
 
         self._spam_checker = hs.get_spam_checker()
+        self._account_validity_handler = hs.get_account_validity_handler()
         self._third_party_event_rules = hs.get_third_party_event_rules()
 
     #################################################################################
@@ -119,6 +120,11 @@ def register_spam_checker_callbacks(self):
         """Registers callbacks for spam checking capabilities."""
         return self._spam_checker.register_callbacks
 
+    @property
+    def register_account_validity_callbacks(self):
+        """Registers callbacks for account validity capabilities."""
+        return self._account_validity_handler.register_account_validity_callbacks
+
     @property
     def register_third_party_rules_callbacks(self):
         """Registers callbacks for third party event rules capabilities."""