Introduce RootConfig.validate_config() which can be subclassed in HomeServerConfig to do cross-config class validation (#19027)

This means we
can move the open registration config validation from `setup()` to
`HomeServerConfig.validate_config()` (much more sane).

Spawning from looking at this area of code in
#19015

Author: MadLittleMods

changelog.d/19027.misc

@@ -0,0 +1 @@
+Introduce `RootConfig.validate_config()` which can be subclassed in `HomeServerConfig` to do cross-config class validation.

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,

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.

synapse/config/_base.pyi

@@ -158,19 +158,19 @@ 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(
         cls, config_parser: argparse.ArgumentParser
     ) -> 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

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`."
+                )

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])

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]
+            )