From 0dbfae03f96ae14672bbdc5cf5c3aecd93636803 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 19 Mar 2019 11:04:12 +0000 Subject: [PATCH 1/7] Enforce hs_disabled_message correctly Fixes a bug where hs_disabled_message was not enforced for 3pid-based requests if there was no server_notices_mxid configured. --- changelog.d/4888.bugfix | 2 ++ synapse/api/auth.py | 8 +++++--- tests/api/test_auth.py | 17 +++++++++++++++++ 3 files changed, 24 insertions(+), 3 deletions(-) create mode 100644 changelog.d/4888.bugfix diff --git a/changelog.d/4888.bugfix b/changelog.d/4888.bugfix new file mode 100644 index 000000000000..0e193709e54c --- /dev/null +++ b/changelog.d/4888.bugfix @@ -0,0 +1,2 @@ +Fix a bug where hs_disabled_message was sometimes not correctly enforced. + diff --git a/synapse/api/auth.py b/synapse/api/auth.py index 5992d30623c8..ee646a97e810 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -788,9 +788,11 @@ def check_auth_blocking(self, user_id=None, threepid=None): # Never fail an auth check for the server notices users or support user # This can be a problem where event creation is prohibited due to blocking - is_support = yield self.store.is_support_user(user_id) - if user_id == self.hs.config.server_notices_mxid or is_support: - return + if user_id is not None: + if user_id == self.hs.config.server_notices_mxid: + return + if (yield self.store.is_support_user(user_id)): + return if self.hs.config.hs_disabled: raise ResourceLimitError( diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index d77f20e87612..d0d36f96fa7e 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -344,6 +344,23 @@ def test_hs_disabled(self): self.assertEquals(e.exception.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) self.assertEquals(e.exception.code, 403) + @defer.inlineCallbacks + def test_hs_disabled_no_server_notices_user(self): + """Check that 'hs_disabled_message' works correctly when there is no + server_notices user. + """ + # this should be the default, but we had a bug where the test was doing the wrong + # thing, so let's make it explicit + self.hs.config.server_notices_mxid = None + + self.hs.config.hs_disabled = True + self.hs.config.hs_disabled_message = "Reason for being disabled" + with self.assertRaises(ResourceLimitError) as e: + yield self.auth.check_auth_blocking() + self.assertEquals(e.exception.admin_contact, self.hs.config.admin_contact) + self.assertEquals(e.exception.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) + self.assertEquals(e.exception.code, 403) + @defer.inlineCallbacks def test_server_notices_mxid_special_cased(self): self.hs.config.hs_disabled = True From 8c1774e821762f2f6feda605f64a45a5b8365860 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 15 Mar 2019 16:49:47 +0000 Subject: [PATCH 2/7] Fix email test The Mailer expects the config object to have `email_smtp_pass` and `email_riot_base_url` attributes (and it won't by default, because the default config impl doesn't set any of the attributes unless email_enable_notifs is set). --- tests/push/test_email.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/push/test_email.py b/tests/push/test_email.py index 50ee6910d16a..be3fed8de3ee 100644 --- a/tests/push/test_email.py +++ b/tests/push/test_email.py @@ -63,8 +63,10 @@ def sendmail(*args, **kwargs): config.email_smtp_port = 20 config.require_transport_security = False config.email_smtp_user = None + config.email_smtp_pass = None config.email_app_name = "Matrix" config.email_notif_from = "test@example.com" + config.email_riot_base_url = None hs = self.setup_test_homeserver(config=config, sendmail=sendmail) From 45bb54a6c666b419d7a22af3dde4fc38a6431cbd Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 19 Mar 2019 11:01:02 +0000 Subject: [PATCH 3/7] Fix registration test * Set allow_guest_access = True, since we rely on it * config doesn't have a `hostname` attribute; it is `server_name` --- tests/rest/client/v2_alpha/test_register.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/v2_alpha/test_register.py b/tests/rest/client/v2_alpha/test_register.py index 8fb525d3bf5c..a45e6e5e1f42 100644 --- a/tests/rest/client/v2_alpha/test_register.py +++ b/tests/rest/client/v2_alpha/test_register.py @@ -20,6 +20,7 @@ def make_homeserver(self, reactor, clock): self.hs.config.registrations_require_3pid = [] self.hs.config.auto_join_rooms = [] self.hs.config.enable_registration_captcha = False + self.hs.config.allow_guest_access = True return self.hs @@ -28,7 +29,7 @@ def test_POST_appservice_registration_valid(self): as_token = "i_am_an_app_service" appservice = ApplicationService( - as_token, self.hs.config.hostname, + as_token, self.hs.config.server_name, id="1234", namespaces={ "users": [{"regex": r"@as_user.*", "exclusive": True}], From 053c50bcb318af74b5b2d20dc7ae1d85689527b6 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 19 Mar 2019 11:06:56 +0000 Subject: [PATCH 4/7] Fix resource limits tests Make sure that we have a `server_notices_mxid` set, given that we are relying on it. --- .../server_notices/test_resource_limits_server_notices.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/server_notices/test_resource_limits_server_notices.py b/tests/server_notices/test_resource_limits_server_notices.py index b1551df7ca01..3bd9f1e9c125 100644 --- a/tests/server_notices/test_resource_limits_server_notices.py +++ b/tests/server_notices/test_resource_limits_server_notices.py @@ -9,13 +9,16 @@ ) from tests import unittest -from tests.utils import setup_test_homeserver +from tests.utils import default_config, setup_test_homeserver class TestResourceLimitsServerNotices(unittest.TestCase): @defer.inlineCallbacks def setUp(self): - self.hs = yield setup_test_homeserver(self.addCleanup) + hs_config = default_config(name="test") + hs_config.server_notices_mxid = "@server:test" + + self.hs = yield setup_test_homeserver(self.addCleanup, config=hs_config) self.server_notices_sender = self.hs.get_server_notices_sender() # relying on [1] is far from ideal, but the only case where From 13bc1e0746aa0442aa5d43555cbbc2dc75e8ef43 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 15 Mar 2019 15:50:37 +0000 Subject: [PATCH 5/7] Use a regular HomeServerConfig object for unit tests Rather than using a Mock for the homeserver config, use a genuine HomeServerConfig object. This makes for a more realistic test, and means that we don't have to keep remembering to add things to the mock config every time we add a new config setting. --- synapse/config/_base.py | 5 ++++- synapse/config/key.py | 7 ++++++- tests/utils.py | 26 +++++++++++++++----------- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/synapse/config/_base.py b/synapse/config/_base.py index 5613f38e4d6f..a219a835502f 100644 --- a/synapse/config/_base.py +++ b/synapse/config/_base.py @@ -405,7 +405,10 @@ def read_config_files(self, config_files, keys_directory=None, generate_keys=Fal self.invoke_all("generate_files", config) return - self.invoke_all("read_config", config) + self.parse_config_dict(config) + + def parse_config_dict(self, config_dict): + self.invoke_all("read_config", config_dict) def find_config_files(search_paths): diff --git a/synapse/config/key.py b/synapse/config/key.py index 2bd5531acb05..933928885ae2 100644 --- a/synapse/config/key.py +++ b/synapse/config/key.py @@ -38,7 +38,12 @@ class KeyConfig(Config): def read_config(self, config): - self.signing_key = self.read_signing_key(config["signing_key_path"]) + # the signing key can be specified inline or in a separate file + if "signing_key" in config: + self.signing_key = read_signing_keys([config["signing_key"]]) + else: + self.signing_key = self.read_signing_key(config["signing_key_path"]) + self.old_signing_keys = self.read_old_signing_keys( config.get("old_signing_keys", {}) ) diff --git a/tests/utils.py b/tests/utils.py index b58b674aa44f..eeb4bce5a2db 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -28,7 +28,7 @@ from synapse.api.constants import EventTypes, RoomVersions from synapse.api.errors import CodeMessageException, cs_error -from synapse.config.server import ServerConfig +from synapse.config.homeserver import HomeServerConfig from synapse.federation.transport import server as federation_server from synapse.http.server import HttpServer from synapse.server import HomeServer @@ -111,14 +111,25 @@ def default_config(name): """ Create a reasonable test config. """ - config = Mock() - config.signing_key = [MockKey()] + config_dict = { + "server_name": name, + "media_store_path": "media", + "uploads_path": "uploads", + + # the test signing key is just an arbitrary ed25519 key to keep the config + # parser happy + "signing_key": "ed25519 a_lPym qvioDNmfExFBRPgdTU+wtFYKq4JfwFRv7sYVgWvmgJg", + } + + config = HomeServerConfig() + config.parse_config_dict(config_dict) + + # TODO: move this stuff into config_dict or get rid of it config.event_cache_size = 1 config.enable_registration = True config.enable_registration_captcha = False config.macaroon_secret_key = "not even a little secret" config.expire_access_token = False - config.server_name = name config.trusted_third_party_id_servers = [] config.room_invite_state_types = [] config.password_providers = [] @@ -176,13 +187,6 @@ def default_config(name): # background, which upsets the test runner. config.update_user_directory = False - def is_threepid_reserved(threepid): - return ServerConfig.is_threepid_reserved( - config.mau_limits_reserved_threepids, threepid - ) - - config.is_threepid_reserved.side_effect = is_threepid_reserved - return config From 07f057ac8008d1dbd2f66930aa5e746b4aa7766c Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 19 Mar 2019 11:36:58 +0000 Subject: [PATCH 6/7] changelog --- changelog.d/4889.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/4889.misc diff --git a/changelog.d/4889.misc b/changelog.d/4889.misc new file mode 100644 index 000000000000..f1948db65e6b --- /dev/null +++ b/changelog.d/4889.misc @@ -0,0 +1 @@ +Use a regular HomeServerConfig object for unit tests rater than a Mock. From b5d48560c72ac11845af3099f52ba1021617aaa5 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 19 Mar 2019 12:05:05 +0000 Subject: [PATCH 7/7] Fix RegistrationTestCase turns out this relies on there being a `user_consent_version` set. --- tests/handlers/test_register.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index 010e65829ed1..2217eb2a108b 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -22,7 +22,7 @@ from synapse.handlers.register import RegistrationHandler from synapse.types import RoomAlias, UserID, create_requester -from tests.utils import setup_test_homeserver +from tests.utils import default_config, setup_test_homeserver from .. import unittest @@ -40,8 +40,16 @@ def setUp(self): self.mock_distributor = Mock() self.mock_distributor.declare("registered_user") self.mock_captcha_client = Mock() + + hs_config = default_config("test") + + # some of the tests rely on us having a user consent version + hs_config.user_consent_version = "test_consent_version" + hs_config.max_mau_value = 50 + self.hs = yield setup_test_homeserver( self.addCleanup, + config=hs_config, expire_access_token=True, ) self.macaroon_generator = Mock( @@ -50,7 +58,6 @@ def setUp(self): self.hs.get_macaroon_generator = Mock(return_value=self.macaroon_generator) self.handler = self.hs.get_registration_handler() self.store = self.hs.get_datastore() - self.hs.config.max_mau_value = 50 self.lots_of_users = 100 self.small_number_of_users = 1