Additional tests for third-party event rules (#8468)

* Optimise and test state fetching for 3p event rules

Getting all the events at once is much more efficient than getting them
individually

* Test that 3p event rules can modify events

changelog.d/8465.bugfix

@@ -0,0 +1 @@
+Don't attempt to respond to some requests if the client has already disconnected.

changelog.d/8467.feature

@@ -0,0 +1 @@
+Allow `ThirdPartyEventRules` modules to query and manipulate whether a room is in the public rooms directory.

changelog.d/8468.misc

@@ -0,0 +1 @@
+Additional testing for `ThirdPartyEventRules`.

synapse/events/third_party_rules.py

@@ -61,12 +61,14 @@ class ThirdPartyEventRules:
         prev_state_ids = await context.get_prev_state_ids()
 
         # Retrieve the state events from the database.
-        state_events = {}
+        events = await self.store.get_events(prev_state_ids.values())
-        for key, event_id in prev_state_ids.items():
+        state_events = {(ev.type, ev.state_key): ev for ev in events.values()}
-            state_events[key] = await self.store.get_event(event_id, allow_none=True)
 
-        ret = await self.third_party_rules.check_event_allowed(event, state_events)
+        # The module can modify the event slightly if it wants, but caution should be
-        return ret
+        # exercised, and it's likely to go very wrong if applied to events received over
+        # federation.
+
+        return await self.third_party_rules.check_event_allowed(event, state_events)
 
     async def on_create_room(
         self, requester: Requester, config: dict, is_requester_admin: bool
@@ -131,7 +133,9 @@ class ThirdPartyEventRules:
         if self.third_party_rules is None:
             return True
 
-        check_func = getattr(self.third_party_rules, "check_visibility_can_be_modified")
+        check_func = getattr(
+            self.third_party_rules, "check_visibility_can_be_modified", None
+        )
         if not check_func or not isinstance(check_func, Callable):
             return True
 

synapse/http/server.py

@@ -651,6 +651,11 @@ def respond_with_json_bytes(
     Returns:
         twisted.web.server.NOT_DONE_YET if the request is still active.
     """
+    if request._disconnected:
+        logger.warning(
+            "Not sending response to request %s, already disconnected.", request
+        )
+        return
 
     request.setResponseCode(code)
     request.setHeader(b"Content-Type", b"application/json")

synapse/rest/media/v1/_base.py

@@ -213,6 +213,12 @@ async def respond_with_responder(
         file_size (int|None): Size in bytes of the media. If not known it should be None
         upload_name (str|None): The name of the requested file, if any.
     """
+    if request._disconnected:
+        logger.warning(
+            "Not sending response to request %s, already disconnected.", request
+        )
+        return
+
     if not responder:
         respond_404(request)
         return

tests/rest/client/test_third_party_rules.py

@@ -0,0 +1,144 @@
+# -*- coding: utf-8 -*-
+# Copyright 2019 The Matrix.org Foundation C.I.C.
+#
+# Licensed under the Apache License, Version 2.0 (the 'License');
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an 'AS IS' BASIS,
+# 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 threading
+
+from mock import Mock
+
+from synapse.events import EventBase
+from synapse.rest import admin
+from synapse.rest.client.v1 import login, room
+from synapse.types import Requester, StateMap
+
+from tests import unittest
+
+thread_local = threading.local()
+
+
+class ThirdPartyRulesTestModule:
+    def __init__(self, config, module_api):
+        # keep a record of the "current" rules module, so that the test can patch
+        # it if desired.
+        thread_local.rules_module = self
+
+    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
+
+
+def current_rules_module() -> ThirdPartyRulesTestModule:
+    return thread_local.rules_module
+
+
+class ThirdPartyRulesTestCase(unittest.HomeserverTestCase):
+    servlets = [
+        admin.register_servlets,
+        login.register_servlets,
+        room.register_servlets,
+    ]
+
+    def default_config(self):
+        config = super().default_config()
+        config["third_party_event_rules"] = {
+            "module": __name__ + ".ThirdPartyRulesTestModule",
+            "config": {},
+        }
+        return config
+
+    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)
+
+    def test_third_party_rules(self):
+        """Tests that a forbidden event is forbidden from being sent, but an allowed one
+        can be sent.
+        """
+        # 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"
+
+        callback = Mock(spec=[], side_effect=check)
+        current_rules_module().check_event_allowed = callback
+
+        request, channel = self.make_request(
+            "PUT",
+            "/_matrix/client/r0/rooms/%s/send/foo.bar.allowed/1" % self.room_id,
+            {},
+            access_token=self.tok,
+        )
+        self.render(request)
+        self.assertEquals(channel.result["code"], b"200", channel.result)
+
+        callback.assert_called_once()
+
+        # there should be various state events in the state arg: do some basic checks
+        state_arg = callback.call_args[0][1]
+        for k in (("m.room.create", ""), ("m.room.member", self.user_id)):
+            self.assertIn(k, state_arg)
+            ev = state_arg[k]
+            self.assertEqual(ev.type, k[0])
+            self.assertEqual(ev.state_key, k[1])
+
+        request, channel = self.make_request(
+            "PUT",
+            "/_matrix/client/r0/rooms/%s/send/foo.bar.forbidden/1" % self.room_id,
+            {},
+            access_token=self.tok,
+        )
+        self.render(request)
+        self.assertEquals(channel.result["code"], b"403", channel.result)
+
+    def test_modify_event(self):
+        """Tests that the module can successfully tweak an event before it is persisted.
+        """
+        # first patch the event checker so that it will modify the event
+        async def check(ev: EventBase, state):
+            ev.content = {"x": "y"}
+            return True
+
+        current_rules_module().check_event_allowed = check
+
+        # now send the event
+        request, channel = self.make_request(
+            "PUT",
+            "/_matrix/client/r0/rooms/%s/send/modifyme/1" % self.room_id,
+            {"x": "x"},
+            access_token=self.tok,
+        )
+        self.render(request)
+        self.assertEqual(channel.result["code"], b"200", channel.result)
+        event_id = channel.json_body["event_id"]
+
+        # ... and check that it got modified
+        request, channel = self.make_request(
+            "GET",
+            "/_matrix/client/r0/rooms/%s/event/%s" % (self.room_id, event_id),
+            access_token=self.tok,
+        )
+        self.render(request)
+        self.assertEqual(channel.result["code"], b"200", channel.result)
+        ev = channel.json_body
+        self.assertEqual(ev["content"]["x"], "y")

tests/rest/client/third_party_rules.py

@@ -1,86 +0,0 @@
-# -*- coding: utf-8 -*-
-# Copyright 2019 The Matrix.org Foundation C.I.C.
-#
-# Licensed under the Apache License, Version 2.0 (the 'License');
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#     http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an 'AS IS' BASIS,
-# 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 synapse.rest import admin
-from synapse.rest.client.v1 import login, room
-from synapse.types import Requester
-
-from tests import unittest
-
-
-class ThirdPartyRulesTestModule:
-    def __init__(self, config, *args, **kwargs):
-        pass
-
-    async def on_create_room(
-        self, requester: Requester, config: dict, is_requester_admin: bool
-    ):
-        return True
-
-    async def check_event_allowed(self, event, context):
-        if event.type == "foo.bar.forbidden":
-            return False
-        else:
-            return True
-
-    @staticmethod
-    def parse_config(config):
-        return config
-
-
-class ThirdPartyRulesTestCase(unittest.HomeserverTestCase):
-    servlets = [
-        admin.register_servlets,
-        login.register_servlets,
-        room.register_servlets,
-    ]
-
-    def make_homeserver(self, reactor, clock):
-        config = self.default_config()
-        config["third_party_event_rules"] = {
-            "module": "tests.rest.client.third_party_rules.ThirdPartyRulesTestModule",
-            "config": {},
-        }
-
-        self.hs = self.setup_test_homeserver(config=config)
-        return self.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)
-
-    def test_third_party_rules(self):
-        """Tests that a forbidden event is forbidden from being sent, but an allowed one
-        can be sent.
-        """
-        request, channel = self.make_request(
-            "PUT",
-            "/_matrix/client/r0/rooms/%s/send/foo.bar.allowed/1" % self.room_id,
-            {},
-            access_token=self.tok,
-        )
-        self.render(request)
-        self.assertEquals(channel.result["code"], b"200", channel.result)
-
-        request, channel = self.make_request(
-            "PUT",
-            "/_matrix/client/r0/rooms/%s/send/foo.bar.forbidden/1" % self.room_id,
-            {},
-            access_token=self.tok,
-        )
-        self.render(request)
-        self.assertEquals(channel.result["code"], b"403", channel.result)