diff --git a/changelog.d/19027.misc b/changelog.d/19027.misc new file mode 100644 index 00000000000..727f3ee5ffc --- /dev/null +++ b/changelog.d/19027.misc @@ -0,0 +1 @@ +Introduce `RootConfig.validate_config()` which can be subclassed in `HomeServerConfig` to do cross-config class validation. diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index 6ea412b9f4c..8d9b76e083c 100644 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -367,7 +367,7 @@ def create_homeserver( if config.worker.worker_app: raise ConfigError( - "You have specified `worker_app` in the config but are attempting to start a non-worker " + "You have specified `worker_app` in the config but are attempting to setup a non-worker " "instance. Please use `python -m synapse.app.generic_worker` instead (or remove the option if this is the main process)." ) @@ -377,22 +377,6 @@ def create_homeserver( if config.server.gc_seconds: synapse.metrics.MIN_TIME_BETWEEN_GCS = config.server.gc_seconds - if ( - config.registration.enable_registration - and not config.registration.enable_registration_without_verification - ): - if ( - not config.captcha.enable_registration_captcha - and not config.registration.registrations_require_3pid - and not config.registration.registration_requires_token - ): - raise ConfigError( - "You have enabled open registration without any verification. This is a known vector for " - "spam and abuse. If you would like to allow public registration, please consider adding email, " - "captcha, or token-based verification. Otherwise this check can be removed by setting the " - "`enable_registration_without_verification` config option to `true`." - ) - hs = SynapseHomeServer( hostname=config.server.server_name, config=config, diff --git a/synapse/config/_base.py b/synapse/config/_base.py index 6de4c12c960..5d0560e0f2e 100644 --- a/synapse/config/_base.py +++ b/synapse/config/_base.py @@ -545,18 +545,22 @@ def generate_config( @classmethod def load_config( - cls: Type[TRootConfig], description: str, argv: List[str] + cls: Type[TRootConfig], description: str, argv_options: List[str] ) -> TRootConfig: """Parse the commandline and config files Doesn't support config-file-generation: used by the worker apps. + Args: + description: TODO + argv_options: The options passed to Synapse. Usually `sys.argv[1:]`. + Returns: Config object. """ config_parser = argparse.ArgumentParser(description=description) cls.add_arguments_to_parser(config_parser) - obj, _ = cls.load_config_with_parser(config_parser, argv) + obj, _ = cls.load_config_with_parser(config_parser, argv_options) return obj @@ -609,6 +613,10 @@ def load_config_with_parser( Used for workers where we want to add extra flags/subcommands. + Note: This is the common denominator for loading config and is also used by + `load_config` and `load_or_generate_config`. Which is why we call + `validate_config()` here. + Args: parser argv_options: The options passed to Synapse. Usually `sys.argv[1:]`. @@ -642,6 +650,10 @@ def load_config_with_parser( obj.invoke_all("read_arguments", config_args) + # Now that we finally have the full config sections parsed, allow subclasses to + # do some extra validation across the entire config. + obj.validate_config() + return obj, config_args @classmethod @@ -842,15 +854,7 @@ def load_or_generate_config( ): return None - obj.parse_config_dict( - config_dict, - config_dir_path=config_dir_path, - data_dir_path=data_dir_path, - allow_secrets_in_config=config_args.secrets_in_config, - ) - obj.invoke_all("read_arguments", config_args) - - return obj + return cls.load_config(description, argv_options) def parse_config_dict( self, @@ -911,6 +915,20 @@ def reload_config_section(self, section_name: str) -> Config: existing_config.root = None return existing_config + def validate_config(self) -> None: + """ + Additional config validation across all config sections. + + Override this in subclasses to add extra validation. This is called once all + config option values have been populated. + + XXX: This should only validate, not modify the configuration, as the final + config state is required for proper validation across all config sections. + + Raises: + ConfigError: if the config is invalid. + """ + def read_config_files(config_files: Iterable[str]) -> Dict[str, Any]: """Read the config files and shallowly merge them into a dict. diff --git a/synapse/config/_base.pyi b/synapse/config/_base.pyi index ed16d5b313e..02543da3884 100644 --- a/synapse/config/_base.pyi +++ b/synapse/config/_base.pyi @@ -158,11 +158,11 @@ class RootConfig: ) -> str: ... @classmethod def load_or_generate_config( - cls: Type[TRootConfig], description: str, argv: List[str] + cls: Type[TRootConfig], description: str, argv_options: List[str] ) -> Optional[TRootConfig]: ... @classmethod def load_config( - cls: Type[TRootConfig], description: str, argv: List[str] + cls: Type[TRootConfig], description: str, argv_options: List[str] ) -> TRootConfig: ... @classmethod def add_arguments_to_parser( @@ -170,7 +170,7 @@ class RootConfig: ) -> None: ... @classmethod def load_config_with_parser( - cls: Type[TRootConfig], parser: argparse.ArgumentParser, argv: List[str] + cls: Type[TRootConfig], parser: argparse.ArgumentParser, argv_options: List[str] ) -> Tuple[TRootConfig, argparse.Namespace]: ... def generate_missing_files( self, config_dict: dict, config_dir_path: str diff --git a/synapse/config/homeserver.py b/synapse/config/homeserver.py index f46f41da319..94ebe583a4a 100644 --- a/synapse/config/homeserver.py +++ b/synapse/config/homeserver.py @@ -18,7 +18,8 @@ # [This file includes modifications made by New Vector Limited] # # -from ._base import RootConfig + +from ._base import ConfigError, RootConfig from .account_validity import AccountValidityConfig from .api import ApiConfig from .appservice import AppServiceConfig @@ -67,6 +68,10 @@ class HomeServerConfig(RootConfig): + """ + Top-level config object for Synapse homeserver (main process and workers). + """ + config_classes = [ ModulesConfig, ServerConfig, @@ -115,3 +120,22 @@ class HomeServerConfig(RootConfig): # This must be last, as it checks for conflicts with other config options. MasConfig, ] + + def validate_config( + self, + ) -> None: + if ( + self.registration.enable_registration + and not self.registration.enable_registration_without_verification + ): + if ( + not self.captcha.enable_registration_captcha + and not self.registration.registrations_require_3pid + and not self.registration.registration_requires_token + ): + raise ConfigError( + "You have enabled open registration without any verification. This is a known vector for " + "spam and abuse. If you would like to allow public registration, please consider adding email, " + "captcha, or token-based verification. Otherwise this check can be removed by setting the " + "`enable_registration_without_verification` config option to `true`." + ) diff --git a/tests/config/test_load.py b/tests/config/test_load.py index b72365b6e3a..c1b787346ef 100644 --- a/tests/config/test_load.py +++ b/tests/config/test_load.py @@ -99,7 +99,14 @@ def test_load_succeeds_if_macaroon_secret_key_missing(self) -> None: def test_disable_registration(self) -> None: self.generate_config() self.add_lines_to_config( - ["enable_registration: true", "disable_registration: true"] + [ + "enable_registration: true", + "disable_registration: true", + # We're not worried about open registration in this test. This test is + # focused on making sure that enable/disable_registration properly + # override each other. + "enable_registration_without_verification: true", + ] ) # Check that disable_registration clobbers enable_registration. config = HomeServerConfig.load_config("", ["-c", self.config_file]) diff --git a/tests/config/test_registration_config.py b/tests/config/test_registration_config.py index 9da0a3f4269..9eb5323a24e 100644 --- a/tests/config/test_registration_config.py +++ b/tests/config/test_registration_config.py @@ -19,6 +19,8 @@ # # +import argparse + import synapse.app.homeserver from synapse.config import ConfigError from synapse.config.homeserver import HomeServerConfig @@ -99,6 +101,10 @@ def test_session_lifetime_must_not_be_exceeded_by_smaller_lifetimes(self) -> Non ) def test_refuse_to_start_if_open_registration_and_no_verification(self) -> None: + """ + Test that our utilities to start the main Synapse homeserver process refuses + to start if we detect open registration. + """ self.generate_config() self.add_lines_to_config( [ @@ -111,7 +117,7 @@ def test_refuse_to_start_if_open_registration_and_no_verification(self) -> None: ) # Test that allowing open registration without verification raises an error - with self.assertRaises(ConfigError): + with self.assertRaises(SystemExit): # Do a normal homeserver creation and setup homeserver_config = synapse.app.homeserver.load_or_generate_config( ["-c", self.config_file] @@ -122,3 +128,76 @@ def test_refuse_to_start_if_open_registration_and_no_verification(self) -> None: # earlier, but in the future, the code could be refactored to raise the # error in a different place. synapse.app.homeserver.setup(hs) + + def test_load_config_error_if_open_registration_and_no_verification(self) -> None: + """ + Test that `HomeServerConfig.load_config(...)` raises an exception when we detect open + registration. + """ + self.generate_config() + self.add_lines_to_config( + [ + " ", + "enable_registration: true", + "registrations_require_3pid: []", + "enable_registration_captcha: false", + "registration_requires_token: false", + ] + ) + + # Test that allowing open registration without verification raises an error + with self.assertRaises(ConfigError): + _homeserver_config = HomeServerConfig.load_config( + description="test", argv_options=["-c", self.config_file] + ) + + def test_load_or_generate_config_error_if_open_registration_and_no_verification( + self, + ) -> None: + """ + Test that `HomeServerConfig.load_or_generate_config(...)` raises an exception when we detect open + registration. + """ + self.generate_config() + self.add_lines_to_config( + [ + " ", + "enable_registration: true", + "registrations_require_3pid: []", + "enable_registration_captcha: false", + "registration_requires_token: false", + ] + ) + + # Test that allowing open registration without verification raises an error + with self.assertRaises(ConfigError): + _homeserver_config = HomeServerConfig.load_or_generate_config( + description="test", argv_options=["-c", self.config_file] + ) + + def test_load_config_with_parser_error_if_open_registration_and_no_verification( + self, + ) -> None: + """ + Test that `HomeServerConfig.load_config_with_parser(...)` raises an exception when we detect open + registration. + """ + self.generate_config() + self.add_lines_to_config( + [ + " ", + "enable_registration: true", + "registrations_require_3pid: []", + "enable_registration_captcha: false", + "registration_requires_token: false", + ] + ) + + # Test that allowing open registration without verification raises an error + with self.assertRaises(ConfigError): + config_parser = argparse.ArgumentParser(description="test") + HomeServerConfig.add_arguments_to_parser(config_parser) + + _homeserver_config = HomeServerConfig.load_config_with_parser( + parser=config_parser, argv_options=["-c", self.config_file] + )