From fae38e704e711ace2d65cbb6ea742fb3e42525ed Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Wed, 13 Oct 2021 15:44:55 +0100 Subject: [PATCH 1/7] Work around cyclic import error --- tests/rest/client/test_third_party_rules.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index 38ac9be11346..9fec4d1895e2 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -12,13 +12,12 @@ # See the License for the specific language governing permissions and # limitations under the License. import threading -from typing import Dict +from typing import TYPE_CHECKING, Dict from unittest.mock import Mock from synapse.api.constants import EventTypes 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 import login, room from synapse.types import Requester, StateMap @@ -26,11 +25,14 @@ from tests import unittest +if TYPE_CHECKING: + from synapse.module_api import ModuleApi + thread_local = threading.local() class LegacyThirdPartyRulesTestModule: - def __init__(self, config: Dict, module_api: ModuleApi): + 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. thread_local.rules_module = self @@ -50,7 +52,7 @@ def parse_config(config): class LegacyDenyNewRooms(LegacyThirdPartyRulesTestModule): - def __init__(self, config: Dict, module_api: ModuleApi): + def __init__(self, config: Dict, module_api: "ModuleApi"): super().__init__(config, module_api) def on_create_room( @@ -60,7 +62,7 @@ def on_create_room( class LegacyChangeEvents(LegacyThirdPartyRulesTestModule): - def __init__(self, config: Dict, module_api: ModuleApi): + def __init__(self, config: Dict, module_api: "ModuleApi"): super().__init__(config, module_api) async def check_event_allowed(self, event: EventBase, state: StateMap[EventBase]): From b0aa1db24f1b9e07217117a60dc3bb54b37ca31e Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Wed, 13 Oct 2021 15:53:55 +0100 Subject: [PATCH 2/7] Add a test for the workaround introduced in #11042 --- tests/rest/client/test_third_party_rules.py | 48 ++++++++++++++++++++- 1 file changed, 46 insertions(+), 2 deletions(-) diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index 9fec4d1895e2..bfd38f9c4a90 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -12,15 +12,16 @@ # See the License for the specific language governing permissions and # limitations under the License. import threading -from typing import TYPE_CHECKING, Dict +from typing import TYPE_CHECKING, Dict, Optional, Tuple from unittest.mock import Mock from synapse.api.constants import EventTypes +from synapse.api.errors import SynapseError from synapse.events import EventBase from synapse.events.third_party_rules import load_legacy_third_party_event_rules from synapse.rest import admin from synapse.rest.client import login, room -from synapse.types import Requester, StateMap +from synapse.types import JsonDict, Requester, StateMap from synapse.util.frozenutils import unfreeze from tests import unittest @@ -138,6 +139,49 @@ async def check(ev, state): ) self.assertEquals(channel.result["code"], b"403", channel.result) + def test_third_party_rules_workaround_syanpse_errors_pass_through(self): + """ + Tests that the workaround introduced by https://github.com/matrix-org/synapse/pull/11042 + is functional: that SynapseErrors are passed through from check_event_allowed + and bubble up to the web resource. + + NEW MODULES SHOULD NOT MAKE USE OF THIS WORKAROUND! + This is a temporary workaround! + """ + + class NastyHackException(SynapseError): + def error_dict(self): + """ + This overrides SynapseError's `error_dict` to nastily inject + JSON into the error response. + """ + return super().error_dict() | {"nasty": "very"} + + # patch the rules module with a Mock which will raise our hacky exception + # type + async def check(ev, state) -> Tuple[bool, Optional[JsonDict]]: + raise NastyHackException(429, "message") + + callback = Mock(spec=[], side_effect=check) + self.hs.get_third_party_event_rules()._check_event_allowed_callbacks = [ + callback + ] + + # Make a request + channel = self.make_request( + "PUT", + "/_matrix/client/r0/rooms/%s/send/foo.bar.forbidden/2" % self.room_id, + {}, + access_token=self.tok, + ) + # Check the error code + self.assertEquals(channel.result["code"], b"429", channel.result) + # Check the JSON body has had the `nasty` key injected + self.assertEqual( + channel.json_body, + {"errcode": "M_UNKNOWN", "error": "message", "nasty": "very"}, + ) + def test_cannot_modify_event(self): """cannot accidentally modify an event before it is persisted""" From 3f817ed66696efb3af602f9298f897095fec4577 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Wed, 13 Oct 2021 15:56:03 +0100 Subject: [PATCH 3/7] Newsfile Signed-off-by: Olivier Wilkinson (reivilibre) --- changelog.d/11071.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11071.misc diff --git a/changelog.d/11071.misc b/changelog.d/11071.misc new file mode 100644 index 000000000000..33a11abdd5cf --- /dev/null +++ b/changelog.d/11071.misc @@ -0,0 +1 @@ +Add a test for the workaround introduced in [\#11042](https://github.com/matrix-org/synapse/pull/11042) concerning the behaviour of third-party rule modules and `SynapseError`s. From 30ecd64ffc97320b6f5e73ed40ad9f3324782059 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Wed, 13 Oct 2021 15:56:16 +0100 Subject: [PATCH 4/7] Fix typo Signed-off-by: Olivier Wilkinson (reivilibre) --- tests/rest/client/test_third_party_rules.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index bfd38f9c4a90..843d11838f36 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -139,7 +139,7 @@ async def check(ev, state): ) self.assertEquals(channel.result["code"], b"403", channel.result) - def test_third_party_rules_workaround_syanpse_errors_pass_through(self): + def test_third_party_rules_workaround_synapse_errors_pass_through(self): """ Tests that the workaround introduced by https://github.com/matrix-org/synapse/pull/11042 is functional: that SynapseErrors are passed through from check_event_allowed From 1031225dfae75b622ac122e6553866f381413aed Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Wed, 13 Oct 2021 17:07:48 +0100 Subject: [PATCH 5/7] Avoid using Python 3.9 dict `|` syntax --- tests/rest/client/test_third_party_rules.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index 843d11838f36..34f624d03ef2 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -155,7 +155,9 @@ def error_dict(self): This overrides SynapseError's `error_dict` to nastily inject JSON into the error response. """ - return super().error_dict() | {"nasty": "very"} + result = super().error_dict() + result["nasty"] = "very" + return result # patch the rules module with a Mock which will raise our hacky exception # type From c943a4ff3c7cc1b71de4e51f8ccbe5a8865e444c Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Wed, 13 Oct 2021 17:08:34 +0100 Subject: [PATCH 6/7] Don't bother wrapping `check` in a `Mock` --- tests/rest/client/test_third_party_rules.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index 34f624d03ef2..56515dd259de 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -164,9 +164,8 @@ def error_dict(self): async def check(ev, state) -> Tuple[bool, Optional[JsonDict]]: raise NastyHackException(429, "message") - callback = Mock(spec=[], side_effect=check) self.hs.get_third_party_event_rules()._check_event_allowed_callbacks = [ - callback + check ] # Make a request From 86a67ed8f3900fdda16caa6a9befe109fb511b15 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Wed, 13 Oct 2021 17:38:15 +0100 Subject: [PATCH 7/7] Fix stale comment --- tests/rest/client/test_third_party_rules.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index 56515dd259de..531f09c48b87 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -159,14 +159,11 @@ def error_dict(self): result["nasty"] = "very" return result - # patch the rules module with a Mock which will raise our hacky exception - # type + # add a callback that will raise our hacky exception async def check(ev, state) -> Tuple[bool, Optional[JsonDict]]: raise NastyHackException(429, "message") - self.hs.get_third_party_event_rules()._check_event_allowed_callbacks = [ - check - ] + self.hs.get_third_party_event_rules()._check_event_allowed_callbacks = [check] # Make a request channel = self.make_request(