Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Require direct references to configuration variables. #10985

Merged
merged 8 commits into from
Oct 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/10985.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Use direct references to config flags.
4 changes: 2 additions & 2 deletions scripts/synapse_port_db
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ class MockHomeserver:
def __init__(self, config):
self.clock = Clock(reactor)
self.config = config
self.hostname = config.server_name
self.hostname = config.server.server_name
self.version_string = "Synapse/" + get_version_string(synapse)

def get_clock(self):
Expand Down Expand Up @@ -583,7 +583,7 @@ class Porter(object):
return

self.postgres_store = self.build_db_store(
self.hs_config.get_single_database()
self.hs_config.database.get_single_database()
)

await self.run_background_updates_on_postgres()
Expand Down
2 changes: 1 addition & 1 deletion scripts/update_synapse_database
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class MockHomeserver(HomeServer):

def __init__(self, config, **kwargs):
super(MockHomeserver, self).__init__(
config.server_name, reactor=reactor, config=config, **kwargs
config.server.server_name, reactor=reactor, config=config, **kwargs
)

self.version_string = "Synapse/" + get_version_string(synapse)
Expand Down
2 changes: 1 addition & 1 deletion synapse/app/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ def refresh_certificate(hs):
if not hs.config.server.has_tls_listener():
return

hs.config.read_certificate_from_disk()
hs.config.tls.read_certificate_from_disk()
hs.tls_server_context_factory = context_factory.ServerContextFactory(hs.config)

if hs._listening_services:
Expand Down
4 changes: 2 additions & 2 deletions synapse/app/admin_cmd.py
Original file line number Diff line number Diff line change
Expand Up @@ -197,9 +197,9 @@ def start(config_options):
# Explicitly disable background processes
config.server.update_user_directory = False
config.worker.run_background_tasks = False
config.start_pushers = False
config.worker.start_pushers = False
config.pusher_shard_config.instances = []
config.send_federation = False
config.worker.send_federation = False
config.federation_shard_config.instances = []

synapse.events.USE_FROZEN_DICTS = config.server.use_frozen_dicts
Expand Down
2 changes: 1 addition & 1 deletion synapse/app/homeserver.py
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ def _configure_named_resource(self, name, compress=False):
)

if name in ["media", "federation", "client"]:
if self.config.media.enable_media_repo:
if self.config.server.enable_media_repo:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing context here, but why was this line (wrongly) trying to get the setting from the media config? Ditto for the account_validity vs email config.

Also, and not something that needs to be fixed now, I find it a bit worrying if we have been looking for this setting in the wrong config class for some time and CI didn't pick it up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing context here, but why was this line (wrongly) trying to get the setting from the media config? Ditto for the account_validity vs email config.

In one of the previous PRs I updated it to look at the wrong configuration.

Also, and not something that needs to be fixed now, I find it a bit worrying if we have been looking for this setting in the wrong config class for some time and CI didn't pick it up.

It still worked fine before this PR, which is why CI didn't pick it up. It is due to this code:

def __getattr__(self, item: str) -> Any:
"""
Try and fetch a configuration option that does not exist on this class.
This is so that existing configs that rely on `self.value`, where value
is actually from a different config section, continue to work.
"""
if item in ["generate_config_section", "read_config"]:
raise AttributeError(item)
if self.root is None:
raise AttributeError(item)
else:
return self.root._get_unclassed_config(self.section, item)

(Which I need to double check is removed by this PR actually...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I removed that! 🎉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a bit to figure out how this could possibly have worked, it's quite confusing! Glad we're getting rid of it....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah definitely...

media_repo = self.get_media_repository_resource()
resources.update(
{MEDIA_PREFIX: media_repo, LEGACY_MEDIA_PREFIX: media_repo}
Expand Down
64 changes: 8 additions & 56 deletions synapse/config/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,21 +118,6 @@ def __init__(self, root_config=None):
"synapse", "res/templates"
)

def __getattr__(self, item: str) -> Any:
"""
Try and fetch a configuration option that does not exist on this class.

This is so that existing configs that rely on `self.value`, where value
is actually from a different config section, continue to work.
"""
if item in ["generate_config_section", "read_config"]:
raise AttributeError(item)

if self.root is None:
raise AttributeError(item)
else:
return self.root._get_unclassed_config(self.section, item)

@staticmethod
def parse_size(value):
if isinstance(value, int):
Expand Down Expand Up @@ -289,7 +274,9 @@ def read_templates(
env.filters.update(
{
"format_ts": _format_ts_filter,
"mxc_to_http": _create_mxc_to_http_filter(self.public_baseurl),
"mxc_to_http": _create_mxc_to_http_filter(
self.root.server.public_baseurl
),
}
)

Expand All @@ -311,8 +298,6 @@ class RootConfig:
config_classes = []

def __init__(self):
self._configs = OrderedDict()

for config_class in self.config_classes:
if config_class.section is None:
raise ValueError("%r requires a section name" % (config_class,))
Expand All @@ -321,42 +306,7 @@ def __init__(self):
conf = config_class(self)
except Exception as e:
raise Exception("Failed making %s: %r" % (config_class.section, e))
self._configs[config_class.section] = conf

def __getattr__(self, item: str) -> Any:
"""
Redirect lookups on this object either to config objects, or values on
config objects, so that `config.tls.blah` works, as well as legacy uses
of things like `config.server.server_name`. It will first look up the config
section name, and then values on those config classes.
"""
if item in self._configs.keys():
return self._configs[item]

return self._get_unclassed_config(None, item)

def _get_unclassed_config(self, asking_section: Optional[str], item: str):
"""
Fetch a config value from one of the instantiated config classes that
has not been fetched directly.

Args:
asking_section: If this check is coming from a Config child, which
one? This section will not be asked if it has the value.
item: The configuration value key.

Raises:
AttributeError if no config classes have the config key. The body
will contain what sections were checked.
"""
for key, val in self._configs.items():
if key == asking_section:
continue

if item in dir(val):
return getattr(val, item)

raise AttributeError(item, "not found in %s" % (list(self._configs.keys()),))
setattr(self, config_class.section, conf)

def invoke_all(self, func_name: str, *args, **kwargs) -> MutableMapping[str, Any]:
"""
Expand All @@ -373,9 +323,11 @@ def invoke_all(self, func_name: str, *args, **kwargs) -> MutableMapping[str, Any
"""
res = OrderedDict()

for name, config in self._configs.items():
for config_class in self.config_classes:
config = getattr(self, config_class.section)

if hasattr(config, func_name):
res[name] = getattr(config, func_name)(*args, **kwargs)
res[config_class.section] = getattr(config, func_name)(*args, **kwargs)

return res

Expand Down
2 changes: 1 addition & 1 deletion synapse/config/account_validity.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ def read_config(self, config, **kwargs):
)

if self.account_validity_renew_by_email_enabled:
if not self.public_baseurl:
if not self.root.server.public_baseurl:
raise ConfigError("Can't send renewal emails without 'public_baseurl'")

# Load account validity templates.
Expand Down
2 changes: 1 addition & 1 deletion synapse/config/cas.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ def read_config(self, config, **kwargs):

# The public baseurl is required because it is used by the redirect
# template.
public_baseurl = self.public_baseurl
public_baseurl = self.root.server.public_baseurl
if not public_baseurl:
raise ConfigError("cas_config requires a public_baseurl to be set")

Expand Down
9 changes: 4 additions & 5 deletions synapse/config/emailconfig.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import logging
import os
from enum import Enum
from typing import Optional

import attr

Expand Down Expand Up @@ -135,7 +134,7 @@ def read_config(self, config, **kwargs):
# msisdn is currently always remote while Synapse does not support any method of
# sending SMS messages
ThreepidBehaviour.REMOTE
if self.account_threepid_delegate_email
if self.root.registration.account_threepid_delegate_email
else ThreepidBehaviour.LOCAL
)
# Prior to Synapse v1.4.0, there was another option that defined whether Synapse would
Expand All @@ -144,7 +143,7 @@ def read_config(self, config, **kwargs):
# identity server in the process.
self.using_identity_server_from_trusted_list = False
if (
not self.account_threepid_delegate_email
not self.root.registration.account_threepid_delegate_email
and config.get("trust_identity_server_for_password_resets", False) is True
):
# Use the first entry in self.trusted_third_party_id_servers instead
Expand All @@ -156,7 +155,7 @@ def read_config(self, config, **kwargs):

# trusted_third_party_id_servers does not contain a scheme whereas
# account_threepid_delegate_email is expected to. Presume https
self.account_threepid_delegate_email: Optional[str] = (
self.root.registration.account_threepid_delegate_email = (
"https://" + first_trusted_identity_server
)
self.using_identity_server_from_trusted_list = True
Expand Down Expand Up @@ -335,7 +334,7 @@ def read_config(self, config, **kwargs):
"client_base_url", email_config.get("riot_base_url", None)
)

if self.account_validity_renew_by_email_enabled:
if self.root.account_validity.account_validity_renew_by_email_enabled:
expiry_template_html = email_config.get(
"expiry_template_html", "notice_expiry.html"
)
Expand Down
6 changes: 4 additions & 2 deletions synapse/config/key.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,11 +145,13 @@ def read_config(self, config, config_dir_path, **kwargs):

# list of TrustedKeyServer objects
self.key_servers = list(
_parse_key_servers(key_servers, self.federation_verify_certificates)
_parse_key_servers(
key_servers, self.root.tls.federation_verify_certificates
)
)

self.macaroon_secret_key = config.get(
"macaroon_secret_key", self.registration_shared_secret
"macaroon_secret_key", self.root.registration.registration_shared_secret
)

if not self.macaroon_secret_key:
Expand Down
2 changes: 1 addition & 1 deletion synapse/config/oidc.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ def read_config(self, config, **kwargs):
"Multiple OIDC providers have the idp_id %r." % idp_id
)

public_baseurl = self.public_baseurl
public_baseurl = self.root.server.public_baseurl
if public_baseurl is None:
raise ConfigError("oidc_config requires a public_baseurl to be set")
self.oidc_callback_url = public_baseurl + "_synapse/client/oidc/callback"
Expand Down
7 changes: 5 additions & 2 deletions synapse/config/registration.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,10 @@ def read_config(self, config, **kwargs):
account_threepid_delegates = config.get("account_threepid_delegates") or {}
self.account_threepid_delegate_email = account_threepid_delegates.get("email")
self.account_threepid_delegate_msisdn = account_threepid_delegates.get("msisdn")
if self.account_threepid_delegate_msisdn and not self.public_baseurl:
if (
self.account_threepid_delegate_msisdn
and not self.root.server.public_baseurl
):
raise ConfigError(
"The configuration option `public_baseurl` is required if "
"`account_threepid_delegate.msisdn` is set, such that "
Expand Down Expand Up @@ -85,7 +88,7 @@ def read_config(self, config, **kwargs):
if mxid_localpart:
# Convert the localpart to a full mxid.
self.auto_join_user_id = UserID(
mxid_localpart, self.server_name
mxid_localpart, self.root.server.server_name
).to_string()

if self.autocreate_auto_join_rooms:
Expand Down
2 changes: 1 addition & 1 deletion synapse/config/repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ def read_config(self, config, **kwargs):
# Only enable the media repo if either the media repo is enabled or the
# current worker app is the media repo.
if (
self.enable_media_repo is False
self.root.server.enable_media_repo is False
and config.get("worker_app") != "synapse.app.media_repository"
):
self.can_load_media_repo = False
Expand Down
2 changes: 1 addition & 1 deletion synapse/config/saml2.py
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ def _default_saml_config_dict(
"""
import saml2

public_baseurl = self.public_baseurl
public_baseurl = self.root.server.public_baseurl
if public_baseurl is None:
raise ConfigError("saml2_config requires a public_baseurl to be set")

Expand Down
4 changes: 3 additions & 1 deletion synapse/config/server_notices.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,9 @@ def read_config(self, config, **kwargs):
return

mxid_localpart = c["system_mxid_localpart"]
self.server_notices_mxid = UserID(mxid_localpart, self.server_name).to_string()
self.server_notices_mxid = UserID(
mxid_localpart, self.root.server.server_name
).to_string()
self.server_notices_mxid_display_name = c.get("system_mxid_display_name", None)
self.server_notices_mxid_avatar_url = c.get("system_mxid_avatar_url", None)
# todo: i18n
Expand Down
6 changes: 4 additions & 2 deletions synapse/config/sso.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,10 @@ def read_config(self, config, **kwargs):
# the client's.
# public_baseurl is an optional setting, so we only add the fallback's URL to the
# list if it's provided (because we can't figure out what that URL is otherwise).
if self.public_baseurl:
login_fallback_url = self.public_baseurl + "_matrix/static/client/login"
if self.root.server.public_baseurl:
login_fallback_url = (
self.root.server.public_baseurl + "_matrix/static/client/login"
)
self.sso_client_whitelist.append(login_fallback_url)

def generate_config_section(self, **kwargs):
Expand Down
8 changes: 2 additions & 6 deletions synapse/handlers/account_validity.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,8 @@ def __init__(self, hs: "HomeServer"):
and self._account_validity_renew_by_email_enabled
):
# Don't do email-specific configuration if renewal by email is disabled.
self._template_html = (
hs.config.account_validity.account_validity_template_html
)
self._template_text = (
hs.config.account_validity.account_validity_template_text
)
self._template_html = hs.config.email.account_validity_template_html
self._template_text = hs.config.email.account_validity_template_text
self._renew_email_subject = (
hs.config.account_validity.account_validity_renew_email_subject
)
Expand Down
7 changes: 5 additions & 2 deletions synapse/handlers/room_member.py
Original file line number Diff line number Diff line change
Expand Up @@ -1468,8 +1468,11 @@ async def _remote_join(
if len(remote_room_hosts) == 0:
raise SynapseError(404, "No known servers")

check_complexity = self.hs.config.limit_remote_rooms.enabled
if check_complexity and self.hs.config.limit_remote_rooms.admins_can_join:
check_complexity = self.hs.config.server.limit_remote_rooms.enabled
if (
check_complexity
and self.hs.config.server.limit_remote_rooms.admins_can_join
):
check_complexity = not await self.auth.is_server_admin(user)

if check_complexity:
Expand Down
2 changes: 1 addition & 1 deletion synapse/replication/tcp/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ def __init__(self, hs: "HomeServer"):
self._instance_name = hs.get_instance_name()
self._typing_handler = hs.get_typing_handler()

self._notify_pushers = hs.config.start_pushers
self._notify_pushers = hs.config.worker.start_pushers
self._pusher_pool = hs.get_pusherpool()
self._presence_handler = hs.get_presence_handler()

Expand Down
7 changes: 5 additions & 2 deletions synapse/replication/tcp/handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,10 @@ def __init__(self, hs: "HomeServer"):
if hs.config.worker.worker_app is not None:
continue

if stream.NAME == FederationStream.NAME and hs.config.send_federation:
if (
stream.NAME == FederationStream.NAME
and hs.config.worker.send_federation
):
# We only support federation stream if federation sending
# has been disabled on the master.
continue
Expand Down Expand Up @@ -225,7 +228,7 @@ def __init__(self, hs: "HomeServer"):
self._is_master = hs.config.worker.worker_app is None

self._federation_sender = None
if self._is_master and not hs.config.send_federation:
if self._is_master and not hs.config.worker.send_federation:
self._federation_sender = hs.get_federation_sender()

self._server_notices_sender = None
Expand Down
2 changes: 1 addition & 1 deletion synapse/rest/client/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ def __init__(self, hs: "HomeServer"):
self.auth_handler = hs.get_auth_handler()
self.registration_handler = hs.get_registration_handler()
self.recaptcha_template = hs.config.captcha.recaptcha_template
self.terms_template = hs.config.terms_template
self.terms_template = hs.config.consent.terms_template
self.registration_token_template = (
hs.config.registration.registration_token_template
)
Expand Down
4 changes: 3 additions & 1 deletion synapse/rest/client/push_rule.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ def __init__(self, hs: "HomeServer"):
self.notifier = hs.get_notifier()
self._is_worker = hs.config.worker.worker_app is not None

self._users_new_default_push_rules = hs.config.users_new_default_push_rules
self._users_new_default_push_rules = (
hs.config.server.users_new_default_push_rules
)

async def on_PUT(self, request: SynapseRequest, path: str) -> Tuple[int, JsonDict]:
if self._is_worker:
Expand Down
4 changes: 3 additions & 1 deletion synapse/storage/databases/main/push_rule.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,9 @@ def __init__(self, database: DatabasePool, db_conn, hs):
prefilled_cache=push_rules_prefill,
)

self._users_new_default_push_rules = hs.config.users_new_default_push_rules
self._users_new_default_push_rules = (
hs.config.server.users_new_default_push_rules
)

@abc.abstractmethod
def get_max_push_rules_stream_id(self):
Expand Down
Loading