From c955e378683708acd5b88e9cb1980291e06dd9a7 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 23 Jun 2021 17:22:08 +0200 Subject: [PATCH] Fix wrapping of legacy check_registration_for_spam (#10238) Fixes #10234 --- changelog.d/10238.removal | 1 + synapse/events/spamcheck.py | 13 +++--- tests/handlers/test_register.py | 76 +++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 6 deletions(-) create mode 100644 changelog.d/10238.removal diff --git a/changelog.d/10238.removal b/changelog.d/10238.removal new file mode 100644 index 000000000..5fb7bfb47 --- /dev/null +++ b/changelog.d/10238.removal @@ -0,0 +1 @@ +The current spam checker interface is deprecated in favour of a new generic modules system. See the [upgrade notes](https://github.com/matrix-org/synapse/blob/master/UPGRADE.rst#deprecation-of-the-current-spam-checker-interface) for more information on how to update to the new system. diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 45ec96dfc..efec16c22 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -109,6 +109,8 @@ def load_legacy_spam_checkers(hs: "synapse.server.HomeServer"): if f is None: return None + wrapped_func = f + if f.__name__ == "check_registration_for_spam": checker_args = inspect.signature(f) if len(checker_args.parameters) == 3: @@ -133,19 +135,18 @@ def load_legacy_spam_checkers(hs: "synapse.server.HomeServer"): request_info, ) - f = wrapper + wrapped_func = wrapper elif len(checker_args.parameters) != 4: raise RuntimeError( "Bad signature for callback check_registration_for_spam", ) def run(*args, **kwargs): - # 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 + # mypy doesn't do well across function boundaries so we need to tell it + # wrapped_func is definitely not None. + assert wrapped_func is not None - return maybe_awaitable(f(*args, **kwargs)) + return maybe_awaitable(wrapped_func(*args, **kwargs)) return run diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index a9fd3036d..c5f6bc3c7 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -17,6 +17,7 @@ from unittest.mock import Mock from synapse.api.auth import Auth from synapse.api.constants import UserTypes from synapse.api.errors import Codes, ResourceLimitError, SynapseError +from synapse.events.spamcheck import load_legacy_spam_checkers from synapse.spam_checker_api import RegistrationBehaviour from synapse.types import RoomAlias, UserID, create_requester @@ -79,6 +80,39 @@ class BanBadIdPUser(TestSpamChecker): return RegistrationBehaviour.ALLOW +class TestLegacyRegistrationSpamChecker: + def __init__(self, config, api): + pass + + async def check_registration_for_spam( + self, + email_threepid, + username, + request_info, + ): + pass + + +class LegacyAllowAll(TestLegacyRegistrationSpamChecker): + async def check_registration_for_spam( + self, + email_threepid, + username, + request_info, + ): + return RegistrationBehaviour.ALLOW + + +class LegacyDenyAll(TestLegacyRegistrationSpamChecker): + async def check_registration_for_spam( + self, + email_threepid, + username, + request_info, + ): + return RegistrationBehaviour.DENY + + class RegistrationTestCase(unittest.HomeserverTestCase): """Tests the RegistrationHandler.""" @@ -95,6 +129,8 @@ class RegistrationTestCase(unittest.HomeserverTestCase): hs = self.setup_test_homeserver(config=hs_config) + load_legacy_spam_checkers(hs) + module_api = hs.get_module_api() for module, config in hs.config.modules.loaded_modules: module(config=config, api=module_api) @@ -535,6 +571,46 @@ class RegistrationTestCase(unittest.HomeserverTestCase): """A spam checker can deny registration, which results in an error.""" self.get_failure(self.handler.register_user(localpart="user"), SynapseError) + @override_config( + { + "spam_checker": [ + { + "module": TestSpamChecker.__module__ + ".LegacyAllowAll", + } + ] + } + ) + def test_spam_checker_legacy_allow(self): + """Tests that a legacy spam checker implementing the legacy 3-arg version of the + check_registration_for_spam callback is correctly called. + + In this test and the following one we test both success and failure to make sure + any failure comes from the spam checker (and not something else failing in the + call stack) and any success comes from the spam checker (and not because a + misconfiguration prevented it from being loaded). + """ + self.get_success(self.handler.register_user(localpart="user")) + + @override_config( + { + "spam_checker": [ + { + "module": TestSpamChecker.__module__ + ".LegacyDenyAll", + } + ] + } + ) + def test_spam_checker_legacy_deny(self): + """Tests that a legacy spam checker implementing the legacy 3-arg version of the + check_registration_for_spam callback is correctly called. + + In this test and the previous one we test both success and failure to make sure + any failure comes from the spam checker (and not something else failing in the + call stack) and any success comes from the spam checker (and not because a + misconfiguration prevented it from being loaded). + """ + self.get_failure(self.handler.register_user(localpart="user"), SynapseError) + @override_config( { "modules": [