From 83ece4c18238ae9f0c6bdcd53badac54bdd4e93d Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 5 Oct 2020 21:31:29 +0100 Subject: [PATCH 1/5] Optimise and test state fetching for 3p event rules Getting all the events at once is much more efficient than getting them individually --- synapse/events/third_party_rules.py | 8 ++-- tests/rest/client/test_third_party_rules.py | 52 +++++++++++++++------ 2 files changed, 42 insertions(+), 18 deletions(-) diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py index 1ca77519d59a..f7820bb04eb3 100644 --- a/synapse/events/third_party_rules.py +++ b/synapse/events/third_party_rules.py @@ -61,12 +61,10 @@ async def check_event_allowed( prev_state_ids = await context.get_prev_state_ids() # Retrieve the state events from the database. - state_events = {} - for key, event_id in prev_state_ids.items(): - state_events[key] = await self.store.get_event(event_id, allow_none=True) + events = await self.store.get_events(prev_state_ids.values()) + state_events = {(ev.type, ev.state_key): ev for ev in events.values()} - ret = await self.third_party_rules.check_event_allowed(event, state_events) - return ret + 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 diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index 7b322f526c86..f3b502f0c307 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -12,33 +12,43 @@ # 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 +from synapse.types import Requester, StateMap from tests import unittest +thread_local = threading.local() + class ThirdPartyRulesTestModule: - def __init__(self, config, *args, **kwargs): - pass + 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, context): - if event.type == "foo.bar.forbidden": - return False - else: - 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, @@ -46,15 +56,13 @@ class ThirdPartyRulesTestCase(unittest.HomeserverTestCase): room.register_servlets, ] - def make_homeserver(self, reactor, clock): - config = self.default_config() + def default_config(self): + config = super().default_config() config["third_party_event_rules"] = { "module": __name__ + ".ThirdPartyRulesTestModule", "config": {}, } - - self.hs = self.setup_test_homeserver(config=config) - return self.hs + return config def prepare(self, reactor, clock, homeserver): # Create a user and room to play with during the tests @@ -67,6 +75,14 @@ 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, @@ -76,6 +92,16 @@ def test_third_party_rules(self): 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 cheks + 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, From a9160daa6b811f158c5d167db41d0e857340155d Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 5 Oct 2020 21:38:25 +0100 Subject: [PATCH 2/5] Test that 3p event rules can modify events --- synapse/events/third_party_rules.py | 4 +++ tests/rest/client/test_third_party_rules.py | 31 +++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py index f7820bb04eb3..e38b8e67fb8c 100644 --- a/synapse/events/third_party_rules.py +++ b/synapse/events/third_party_rules.py @@ -64,6 +64,10 @@ async def check_event_allowed( events = await self.store.get_events(prev_state_ids.values()) state_events = {(ev.type, ev.state_key): ev for ev in events.values()} + # The module can modify the event slightly if it wants, but caution should be + # 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( diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index f3b502f0c307..75a523ac7031 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -110,3 +110,34 @@ async def check(ev, state): ) 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"} + + 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") From 3ece5444e7f9b72938bde287c2c37c88dbfdab73 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 5 Oct 2020 21:39:03 +0100 Subject: [PATCH 3/5] changelog --- changelog.d/8468.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/8468.misc diff --git a/changelog.d/8468.misc b/changelog.d/8468.misc new file mode 100644 index 000000000000..5384557feaac --- /dev/null +++ b/changelog.d/8468.misc @@ -0,0 +1 @@ +Additional testing for ThirdPartyEventRules. From 456f454fed36a39f0b5da7fc84b6a9dda533ee5f Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Tue, 6 Oct 2020 11:23:35 +0100 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> --- changelog.d/8468.misc | 2 +- tests/rest/client/test_third_party_rules.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/changelog.d/8468.misc b/changelog.d/8468.misc index 5384557feaac..32ba991e6421 100644 --- a/changelog.d/8468.misc +++ b/changelog.d/8468.misc @@ -1 +1 @@ -Additional testing for ThirdPartyEventRules. +Additional testing for `ThirdPartyEventRules`. diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index 75a523ac7031..1115714e2be5 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -94,7 +94,7 @@ async def check(ev, state): callback.assert_called_once() - # there should be various state events in the state arg: do some basic cheks + # 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) From c64d822bacda7aab75bc94e6370b35294b9d9a35 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 6 Oct 2020 11:28:37 +0100 Subject: [PATCH 5/5] fix test --- tests/rest/client/test_third_party_rules.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index 1115714e2be5..c12518c93105 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -117,6 +117,7 @@ def test_modify_event(self): # 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