From 3d48150c09ce7fbcd2780d05049ed13884f33f27 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 3 Feb 2021 13:31:08 -0500 Subject: [PATCH 1/9] Revert fa50e4bf4ddcb8e98d44700513a28c490f80f02b --- docs/sample_config.yaml | 31 ++++++++++++++----------------- synapse/api/urls.py | 2 ++ synapse/config/_base.py | 11 +++++------ synapse/config/emailconfig.py | 8 ++++++++ synapse/config/oidc_config.py | 5 ++++- synapse/config/registration.py | 21 +++++++++++++++++---- synapse/config/saml2_config.py | 2 ++ synapse/config/server.py | 24 +++++++++--------------- synapse/config/sso.py | 13 ++++++++----- synapse/handlers/identity.py | 2 ++ synapse/rest/well_known.py | 4 ++++ tests/rest/test_well_known.py | 9 +++++++++ tests/utils.py | 1 + 13 files changed, 85 insertions(+), 48 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 236abd9a3f0f..b3d1cef136e7 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -67,16 +67,11 @@ pid_file: DATADIR/homeserver.pid # #web_client_location: https://riot.example.com/ -# The public-facing base URL that clients use to access this Homeserver (not -# including _matrix/...). This is the same URL a user might enter into the -# 'Custom Homeserver URL' field on their client. If you use Synapse with a -# reverse proxy, this should be the URL to reach Synapse via the proxy. -# Otherwise, it should be the URL to reach Synapse's client HTTP listener (see -# 'listeners' below). -# -# If this is left unset, it defaults to 'https:///'. (Note that -# that will not work unless you configure Synapse or a reverse-proxy to listen -# on port 443.) +# The public-facing base URL that clients use to access this HS +# (not including _matrix/...). This is the same URL a user would +# enter into the 'custom HS URL' field on their client. If you +# use synapse with a reverse proxy, this should be the URL to reach +# synapse via the proxy. # #public_baseurl: https://example.com/ @@ -1169,9 +1164,8 @@ account_validity: # send an email to the account's email address with a renewal link. By # default, no such emails are sent. # - # If you enable this setting, you will also need to fill out the 'email' - # configuration section. You should also check that 'public_baseurl' is set - # correctly. + # If you enable this setting, you will also need to fill out the 'email' and + # 'public_baseurl' configuration sections. # #renew_at: 1w @@ -1262,7 +1256,8 @@ account_validity: # The identity server which we suggest that clients should use when users log # in on this server. # -# (By default, no suggestion is made, so it is left up to the client.) +# (By default, no suggestion is made, so it is left up to the client. +# This setting is ignored unless public_baseurl is also set.) # #default_identity_server: https://matrix.org @@ -1287,6 +1282,8 @@ account_validity: # by the Matrix Identity Service API specification: # https://matrix.org/docs/spec/identity_service/latest # +# If a delegate is specified, the config option public_baseurl must also be filled out. +# account_threepid_delegates: #email: https://example.com # Delegate email sending to example.com #msisdn: http://localhost:8090 # Delegate SMS sending to this local process @@ -1938,9 +1935,9 @@ sso: # phishing attacks from evil.site. To avoid this, include a slash after the # hostname: "https://my.client/". # - # The login fallback page (used by clients that don't natively support the - # required login flows) is automatically whitelisted in addition to any URLs - # in this list. + # If public_baseurl is set, then the login fallback page (used by clients + # that don't natively support the required login flows) is whitelisted in + # addition to any URLs in this list. # # By default, this list is empty. # diff --git a/synapse/api/urls.py b/synapse/api/urls.py index e36aeef31f87..6379c86ddea0 100644 --- a/synapse/api/urls.py +++ b/synapse/api/urls.py @@ -42,6 +42,8 @@ def __init__(self, hs_config): """ if hs_config.form_secret is None: raise ConfigError("form_secret not set in config") + if hs_config.public_baseurl is None: + raise ConfigError("public_baseurl not set in config") self._hmac_secret = hs_config.form_secret.encode("utf-8") self._public_baseurl = hs_config.public_baseurl diff --git a/synapse/config/_base.py b/synapse/config/_base.py index a851f8801d7e..b5d43eacc37d 100644 --- a/synapse/config/_base.py +++ b/synapse/config/_base.py @@ -267,12 +267,11 @@ def read_templates( env = jinja2.Environment(loader=loader, autoescape=jinja2.select_autoescape(),) # Update the environment with our custom filters - env.filters.update( - { - "format_ts": _format_ts_filter, - "mxc_to_http": _create_mxc_to_http_filter(self.public_baseurl), - } - ) + env.filters.update({"format_ts": _format_ts_filter}) + if self.public_baseurl: + env.filters.update( + {"mxc_to_http": _create_mxc_to_http_filter(self.public_baseurl)} + ) # Load the templates return [env.get_template(filename) for filename in filenames] diff --git a/synapse/config/emailconfig.py b/synapse/config/emailconfig.py index 6a487afd3495..d4328c46b9b6 100644 --- a/synapse/config/emailconfig.py +++ b/synapse/config/emailconfig.py @@ -166,6 +166,11 @@ def read_config(self, config, **kwargs): if not self.email_notif_from: missing.append("email.notif_from") + # public_baseurl is required to build password reset and validation links that + # will be emailed to users + if config.get("public_baseurl") is None: + missing.append("public_baseurl") + if missing: raise ConfigError( MISSING_PASSWORD_RESET_CONFIG_ERROR % (", ".join(missing),) @@ -264,6 +269,9 @@ def read_config(self, config, **kwargs): if not self.email_notif_from: missing.append("email.notif_from") + if config.get("public_baseurl") is None: + missing.append("public_baseurl") + if missing: raise ConfigError( "email.enable_notifs is True but required keys are missing: %s" diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py index 4c24c50629a5..4d0f24a9d568 100644 --- a/synapse/config/oidc_config.py +++ b/synapse/config/oidc_config.py @@ -53,7 +53,10 @@ def read_config(self, config, **kwargs): "Multiple OIDC providers have the idp_id %r." % idp_id ) - self.oidc_callback_url = self.public_baseurl + "_synapse/client/oidc/callback" + public_baseurl = self.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" @property def oidc_enabled(self) -> bool: diff --git a/synapse/config/registration.py b/synapse/config/registration.py index ac48913a0bd1..eb650af7fb5f 100644 --- a/synapse/config/registration.py +++ b/synapse/config/registration.py @@ -49,6 +49,10 @@ def __init__(self, config, synapse_config): self.startup_job_max_delta = self.period * 10.0 / 100.0 + if self.renew_by_email_enabled: + if "public_baseurl" not in synapse_config: + raise ConfigError("Can't send renewal emails without 'public_baseurl'") + template_dir = config.get("template_dir") if not template_dir: @@ -105,6 +109,13 @@ 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: + raise ConfigError( + "The configuration option `public_baseurl` is required if " + "`account_threepid_delegate.msisdn` is set, such that " + "clients know where to submit validation tokens to. Please " + "configure `public_baseurl`." + ) self.default_identity_server = config.get("default_identity_server") self.allow_guest_access = config.get("allow_guest_access", False) @@ -227,9 +238,8 @@ def generate_config_section(self, generate_secrets=False, **kwargs): # send an email to the account's email address with a renewal link. By # default, no such emails are sent. # - # If you enable this setting, you will also need to fill out the 'email' - # configuration section. You should also check that 'public_baseurl' is set - # correctly. + # If you enable this setting, you will also need to fill out the 'email' and + # 'public_baseurl' configuration sections. # #renew_at: 1w @@ -320,7 +330,8 @@ def generate_config_section(self, generate_secrets=False, **kwargs): # The identity server which we suggest that clients should use when users log # in on this server. # - # (By default, no suggestion is made, so it is left up to the client.) + # (By default, no suggestion is made, so it is left up to the client. + # This setting is ignored unless public_baseurl is also set.) # #default_identity_server: https://matrix.org @@ -345,6 +356,8 @@ def generate_config_section(self, generate_secrets=False, **kwargs): # by the Matrix Identity Service API specification: # https://matrix.org/docs/spec/identity_service/latest # + # If a delegate is specified, the config option public_baseurl must also be filled out. + # account_threepid_delegates: #email: https://example.com # Delegate email sending to example.com #msisdn: http://localhost:8090 # Delegate SMS sending to this local process diff --git a/synapse/config/saml2_config.py b/synapse/config/saml2_config.py index ad865a667fdb..7226abd8296e 100644 --- a/synapse/config/saml2_config.py +++ b/synapse/config/saml2_config.py @@ -189,6 +189,8 @@ def _default_saml_config_dict( import saml2 public_baseurl = self.public_baseurl + if public_baseurl is None: + raise ConfigError("saml2_config requires a public_baseurl to be set") if self.saml2_grandfathered_mxid_source_attribute: optional_attributes.add(self.saml2_grandfathered_mxid_source_attribute) diff --git a/synapse/config/server.py b/synapse/config/server.py index 47a037017332..3d5e1bd4bc66 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -161,11 +161,7 @@ def read_config(self, config, **kwargs): self.print_pidfile = config.get("print_pidfile") self.user_agent_suffix = config.get("user_agent_suffix") self.use_frozen_dicts = config.get("use_frozen_dicts", False) - self.public_baseurl = config.get("public_baseurl") or "https://%s/" % ( - self.server_name, - ) - if self.public_baseurl[-1] != "/": - self.public_baseurl += "/" + self.public_baseurl = config.get("public_baseurl") # Whether to enable user presence. self.use_presence = config.get("use_presence", True) @@ -321,6 +317,9 @@ def read_config(self, config, **kwargs): # Always blacklist 0.0.0.0, :: self.federation_ip_range_blacklist.update(["0.0.0.0", "::"]) + if self.public_baseurl is not None: + if self.public_baseurl[-1] != "/": + self.public_baseurl += "/" self.start_pushers = config.get("start_pushers", True) # (undocumented) option for torturing the worker-mode replication a bit, @@ -741,16 +740,11 @@ def generate_config_section( # #web_client_location: https://riot.example.com/ - # The public-facing base URL that clients use to access this Homeserver (not - # including _matrix/...). This is the same URL a user might enter into the - # 'Custom Homeserver URL' field on their client. If you use Synapse with a - # reverse proxy, this should be the URL to reach Synapse via the proxy. - # Otherwise, it should be the URL to reach Synapse's client HTTP listener (see - # 'listeners' below). - # - # If this is left unset, it defaults to 'https:///'. (Note that - # that will not work unless you configure Synapse or a reverse-proxy to listen - # on port 443.) + # The public-facing base URL that clients use to access this HS + # (not including _matrix/...). This is the same URL a user would + # enter into the 'custom HS URL' field on their client. If you + # use synapse with a reverse proxy, this should be the URL to reach + # synapse via the proxy. # #public_baseurl: https://example.com/ diff --git a/synapse/config/sso.py b/synapse/config/sso.py index 6c60c6fea467..19bdfd462b1b 100644 --- a/synapse/config/sso.py +++ b/synapse/config/sso.py @@ -64,8 +64,11 @@ def read_config(self, config, **kwargs): # gracefully to the client). This would make it pointless to ask the user for # confirmation, since the URL the confirmation page would be showing wouldn't be # the client's. - login_fallback_url = self.public_baseurl + "_matrix/static/client/login" - self.sso_client_whitelist.append(login_fallback_url) + # 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" + self.sso_client_whitelist.append(login_fallback_url) def generate_config_section(self, **kwargs): return """\ @@ -83,9 +86,9 @@ def generate_config_section(self, **kwargs): # phishing attacks from evil.site. To avoid this, include a slash after the # hostname: "https://my.client/". # - # The login fallback page (used by clients that don't natively support the - # required login flows) is automatically whitelisted in addition to any URLs - # in this list. + # If public_baseurl is set, then the login fallback page (used by clients + # that don't natively support the required login flows) is whitelisted in + # addition to any URLs in this list. # # By default, this list is empty. # diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index 4f7137539bec..b45e2e95c25b 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -504,6 +504,8 @@ async def requestMsisdnToken( except RequestTimedOutError: raise SynapseError(500, "Timed out contacting identity server") + assert self.hs.config.public_baseurl + # we need to tell the client to send the token back to us, since it doesn't # otherwise know where to send it, so add submit_url response parameter # (see also MSC2078) diff --git a/synapse/rest/well_known.py b/synapse/rest/well_known.py index 241fe746d996..f591cc6c5c7b 100644 --- a/synapse/rest/well_known.py +++ b/synapse/rest/well_known.py @@ -34,6 +34,10 @@ def __init__(self, hs): self._config = hs.config def get_well_known(self): + # if we don't have a public_baseurl, we can't help much here. + if self._config.public_baseurl is None: + return None + result = {"m.homeserver": {"base_url": self._config.public_baseurl}} if self._config.default_identity_server: diff --git a/tests/rest/test_well_known.py b/tests/rest/test_well_known.py index c5e44af9f714..14de0921be7b 100644 --- a/tests/rest/test_well_known.py +++ b/tests/rest/test_well_known.py @@ -40,3 +40,12 @@ def test_well_known(self): "m.identity_server": {"base_url": "https://testis"}, }, ) + + def test_well_known_no_public_baseurl(self): + self.hs.config.public_baseurl = None + + channel = self.make_request( + "GET", "/.well-known/matrix/client", shorthand=False + ) + + self.assertEqual(channel.code, 404) diff --git a/tests/utils.py b/tests/utils.py index 68033d75356d..840b657f825d 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -159,6 +159,7 @@ def default_config(name, parse=False): }, "rc_3pid_validation": {"per_second": 10000, "burst_count": 10000}, "saml2_enabled": False, + "public_baseurl": None, "default_identity_server": None, "key_refresh_interval": 24 * 60 * 60 * 1000, "old_signing_keys": {}, From 0bfc63e3bdd4a09d844782ddefaa5371451154b8 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 3 Feb 2021 13:36:39 -0500 Subject: [PATCH 2/9] Keep the improved sample config. --- docs/sample_config.yaml | 11 ++++++----- synapse/config/server.py | 11 ++++++----- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index b3d1cef136e7..d395da11b479 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -67,11 +67,12 @@ pid_file: DATADIR/homeserver.pid # #web_client_location: https://riot.example.com/ -# The public-facing base URL that clients use to access this HS -# (not including _matrix/...). This is the same URL a user would -# enter into the 'custom HS URL' field on their client. If you -# use synapse with a reverse proxy, this should be the URL to reach -# synapse via the proxy. +# The public-facing base URL that clients use to access this Homeserver (not +# including _matrix/...). This is the same URL a user might enter into the +# 'Custom Homeserver URL' field on their client. If you use Synapse with a +# reverse proxy, this should be the URL to reach Synapse via the proxy. +# Otherwise, it should be the URL to reach Synapse's client HTTP listener (see +# 'listeners' below). # #public_baseurl: https://example.com/ diff --git a/synapse/config/server.py b/synapse/config/server.py index 3d5e1bd4bc66..5d72cf2d82cd 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -740,11 +740,12 @@ def generate_config_section( # #web_client_location: https://riot.example.com/ - # The public-facing base URL that clients use to access this HS - # (not including _matrix/...). This is the same URL a user would - # enter into the 'custom HS URL' field on their client. If you - # use synapse with a reverse proxy, this should be the URL to reach - # synapse via the proxy. + # The public-facing base URL that clients use to access this Homeserver (not + # including _matrix/...). This is the same URL a user might enter into the + # 'Custom Homeserver URL' field on their client. If you use Synapse with a + # reverse proxy, this should be the URL to reach Synapse via the proxy. + # Otherwise, it should be the URL to reach Synapse's client HTTP listener (see + # 'listeners' below). # #public_baseurl: https://example.com/ From 4efab71ad1cf8c2a1131e3cd4a5610b90e85800c Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 3 Feb 2021 13:40:34 -0500 Subject: [PATCH 3/9] Add a config check for the CAS service_url. --- synapse/config/cas.py | 23 ++++++++++++++++------- tests/rest/client/v1/test_login.py | 4 +++- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/synapse/config/cas.py b/synapse/config/cas.py index b226890c2a20..64646904d685 100644 --- a/synapse/config/cas.py +++ b/synapse/config/cas.py @@ -13,7 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -from ._base import Config +from ._base import Config, ConfigError class CasConfig(Config): @@ -30,13 +30,22 @@ def read_config(self, config, **kwargs): if self.cas_enabled: self.cas_server_url = cas_config["server_url"] - public_base_url = cas_config.get("service_url") or self.public_baseurl - if public_base_url[-1] != "/": - public_base_url += "/" + + # The public baseurl is required because it is used by the redirect + # template. + public_baseurl = self.public_baseurl + if not public_baseurl: + raise ConfigError("cas_config requires a public_baseurl to be set") + + # If the service_url is given, use it for backwards compatibility. + # Otherwise, use the public_baseurl. + if cas_config.get("service_url"): + public_baseurl = cas_config["service_url"] + if public_baseurl[-1] != "/": + public_baseurl += "/" + # TODO Update this to a _synapse URL. - self.cas_service_url = ( - public_base_url + "_matrix/client/r0/login/cas/ticket" - ) + self.cas_service_url = public_baseurl + "_matrix/client/r0/login/cas/ticket" self.cas_displayname_attribute = cas_config.get("displayname_attribute") self.cas_required_attributes = cas_config.get("required_attributes") or {} else: diff --git a/tests/rest/client/v1/test_login.py b/tests/rest/client/v1/test_login.py index 66dfdaffbc9a..bfcb786af8be 100644 --- a/tests/rest/client/v1/test_login.py +++ b/tests/rest/client/v1/test_login.py @@ -672,10 +672,12 @@ def make_homeserver(self, reactor, clock): self.redirect_path = "_synapse/client/login/sso/redirect/confirm" config = self.default_config() + config["public_baseurl"] = ( + config.get("public_baseurl") or "https://matrix.goodserver.com:8448" + ) config["cas_config"] = { "enabled": True, "server_url": CAS_SERVER, - "service_url": "https://matrix.goodserver.com:8448", } cas_user_id = "username" From fa4a9cc399a99c575a83bd44e31dd2e843e616ff Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 3 Feb 2021 14:44:16 -0500 Subject: [PATCH 4/9] Do not load templates that use mxc_to_http unless public baseurl exists. --- synapse/config/sso.py | 24 +++++++++++++++++++----- synapse/handlers/auth.py | 2 ++ synapse/rest/synapse/client/pick_idp.py | 3 +++ 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/synapse/config/sso.py b/synapse/config/sso.py index 19bdfd462b1b..694eae8d5962 100644 --- a/synapse/config/sso.py +++ b/synapse/config/sso.py @@ -12,7 +12,9 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -from typing import Any, Dict +from typing import Any, Dict, Optional + +import jinja2 from ._base import Config @@ -31,8 +33,6 @@ def read_config(self, config, **kwargs): # Read templates from disk ( - self.sso_login_idp_picker_template, - self.sso_redirect_confirm_template, self.sso_auth_confirm_template, self.sso_error_template, sso_account_deactivated_template, @@ -40,8 +40,6 @@ def read_config(self, config, **kwargs): self.sso_auth_bad_user_template, ) = self.read_templates( [ - "sso_login_idp_picker.html", - "sso_redirect_confirm.html", "sso_auth_confirm.html", "sso_error.html", "sso_account_deactivated.html", @@ -51,6 +49,22 @@ def read_config(self, config, **kwargs): self.sso_template_dir, ) + # The following templates require a public baseurl since they use the + # mxc_to_http filter. + # + # Note that any situation where these templates get used will check that + # public_baseurl is set already. + self.sso_login_idp_picker_template = None # type: Optional[jinja2.Template] + self.sso_redirect_confirm_template = None # type: Optional[jinja2.Template] + if self.public_baseurl: + ( + self.sso_login_idp_picker_template, + self.sso_redirect_confirm_template, + ) = self.read_templates( + ["sso_login_idp_picker.html", "sso_redirect_confirm.html"], + self.sso_template_dir, + ) + # These templates have no placeholders, so render them here self.sso_account_deactivated_template = ( sso_account_deactivated_template.render() diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index a19c5564377a..b7d209e166de 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -1474,6 +1474,8 @@ def _complete_sso_login( # URL we redirect users to. redirect_url_no_params = client_redirect_url.split("?")[0] + assert self._sso_redirect_confirm_template is not None + html = self._sso_redirect_confirm_template.render( display_url=redirect_url_no_params, redirect_url=redirect_url, diff --git a/synapse/rest/synapse/client/pick_idp.py b/synapse/rest/synapse/client/pick_idp.py index 9550b829980d..c97660e155f1 100644 --- a/synapse/rest/synapse/client/pick_idp.py +++ b/synapse/rest/synapse/client/pick_idp.py @@ -74,6 +74,9 @@ async def _async_render_GET(self, request: SynapseRequest) -> None: async def _serve_id_picker( self, request: SynapseRequest, client_redirect_url: str ) -> None: + # The template should exist, but make sure. + assert self._sso_login_idp_picker_template is not None + # otherwise, serve up the IdP picker providers = self._sso_handler.get_identity_providers() html = self._sso_login_idp_picker_template.render( From 12ec86448066084f7e7abf748ed43294a36716bf Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 3 Feb 2021 15:07:00 -0500 Subject: [PATCH 5/9] Newsfragment --- changelog.d/9313.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/9313.bugfix diff --git a/changelog.d/9313.bugfix b/changelog.d/9313.bugfix new file mode 100644 index 000000000000..f578fd13dd6f --- /dev/null +++ b/changelog.d/9313.bugfix @@ -0,0 +1 @@ +Do not automatically calculate `public_baseurl` since it can be wrong in some situations. From 805276da61524852d5153094dde5d3a7250c3a08 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 8 Feb 2021 08:40:54 -0500 Subject: [PATCH 6/9] Remove the CAS fallback to the service_url. --- synapse/config/cas.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/synapse/config/cas.py b/synapse/config/cas.py index 64646904d685..aaa7eba110d1 100644 --- a/synapse/config/cas.py +++ b/synapse/config/cas.py @@ -37,13 +37,6 @@ def read_config(self, config, **kwargs): if not public_baseurl: raise ConfigError("cas_config requires a public_baseurl to be set") - # If the service_url is given, use it for backwards compatibility. - # Otherwise, use the public_baseurl. - if cas_config.get("service_url"): - public_baseurl = cas_config["service_url"] - if public_baseurl[-1] != "/": - public_baseurl += "/" - # TODO Update this to a _synapse URL. self.cas_service_url = public_baseurl + "_matrix/client/r0/login/cas/ticket" self.cas_displayname_attribute = cas_config.get("displayname_attribute") From 7a1168d3d039c8bb19f25795e2050ba0f6a0fb21 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 8 Feb 2021 08:59:43 -0500 Subject: [PATCH 7/9] Raise a runtime error instead of a config error. --- synapse/config/_base.py | 11 ++++++----- synapse/config/sso.py | 25 +++++-------------------- synapse/handlers/auth.py | 2 -- synapse/rest/synapse/client/pick_idp.py | 3 --- synapse/util/templates.py | 15 ++++++++++++--- 5 files changed, 23 insertions(+), 33 deletions(-) diff --git a/synapse/config/_base.py b/synapse/config/_base.py index b5d43eacc37d..a851f8801d7e 100644 --- a/synapse/config/_base.py +++ b/synapse/config/_base.py @@ -267,11 +267,12 @@ def read_templates( env = jinja2.Environment(loader=loader, autoescape=jinja2.select_autoescape(),) # Update the environment with our custom filters - env.filters.update({"format_ts": _format_ts_filter}) - if self.public_baseurl: - env.filters.update( - {"mxc_to_http": _create_mxc_to_http_filter(self.public_baseurl)} - ) + env.filters.update( + { + "format_ts": _format_ts_filter, + "mxc_to_http": _create_mxc_to_http_filter(self.public_baseurl), + } + ) # Load the templates return [env.get_template(filename) for filename in filenames] diff --git a/synapse/config/sso.py b/synapse/config/sso.py index 694eae8d5962..c04a71369bd3 100644 --- a/synapse/config/sso.py +++ b/synapse/config/sso.py @@ -12,9 +12,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -from typing import Any, Dict, Optional - -import jinja2 +from typing import Any, Dict from ._base import Config @@ -33,6 +31,8 @@ def read_config(self, config, **kwargs): # Read templates from disk ( + self.sso_login_idp_picker_template, + self.sso_redirect_confirm_template, self.sso_auth_confirm_template, self.sso_error_template, sso_account_deactivated_template, @@ -40,7 +40,8 @@ def read_config(self, config, **kwargs): self.sso_auth_bad_user_template, ) = self.read_templates( [ - "sso_auth_confirm.html", + "sso_login_idp_picker.html", + "sso_redirect_confirm.html" "sso_auth_confirm.html", "sso_error.html", "sso_account_deactivated.html", "sso_auth_success.html", @@ -49,22 +50,6 @@ def read_config(self, config, **kwargs): self.sso_template_dir, ) - # The following templates require a public baseurl since they use the - # mxc_to_http filter. - # - # Note that any situation where these templates get used will check that - # public_baseurl is set already. - self.sso_login_idp_picker_template = None # type: Optional[jinja2.Template] - self.sso_redirect_confirm_template = None # type: Optional[jinja2.Template] - if self.public_baseurl: - ( - self.sso_login_idp_picker_template, - self.sso_redirect_confirm_template, - ) = self.read_templates( - ["sso_login_idp_picker.html", "sso_redirect_confirm.html"], - self.sso_template_dir, - ) - # These templates have no placeholders, so render them here self.sso_account_deactivated_template = ( sso_account_deactivated_template.render() diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index b7d209e166de..a19c5564377a 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -1474,8 +1474,6 @@ def _complete_sso_login( # URL we redirect users to. redirect_url_no_params = client_redirect_url.split("?")[0] - assert self._sso_redirect_confirm_template is not None - html = self._sso_redirect_confirm_template.render( display_url=redirect_url_no_params, redirect_url=redirect_url, diff --git a/synapse/rest/synapse/client/pick_idp.py b/synapse/rest/synapse/client/pick_idp.py index c97660e155f1..9550b829980d 100644 --- a/synapse/rest/synapse/client/pick_idp.py +++ b/synapse/rest/synapse/client/pick_idp.py @@ -74,9 +74,6 @@ async def _async_render_GET(self, request: SynapseRequest) -> None: async def _serve_id_picker( self, request: SynapseRequest, client_redirect_url: str ) -> None: - # The template should exist, but make sure. - assert self._sso_login_idp_picker_template is not None - # otherwise, serve up the IdP picker providers = self._sso_handler.get_identity_providers() html = self._sso_login_idp_picker_template.render( diff --git a/synapse/util/templates.py b/synapse/util/templates.py index 7e5109d206e5..392dae4a40f5 100644 --- a/synapse/util/templates.py +++ b/synapse/util/templates.py @@ -17,7 +17,7 @@ import time import urllib.parse -from typing import TYPE_CHECKING, Callable, Iterable, Union +from typing import TYPE_CHECKING, Callable, Iterable, Optional, Union import jinja2 @@ -74,14 +74,23 @@ def build_jinja_env( return env -def _create_mxc_to_http_filter(public_baseurl: str) -> Callable: +def _create_mxc_to_http_filter( + public_baseurl: Optional[str], +) -> Callable[[str, int, int, str], str]: """Create and return a jinja2 filter that converts MXC urls to HTTP Args: public_baseurl: The public, accessible base URL of the homeserver """ - def mxc_to_http_filter(value, width, height, resize_method="crop"): + def mxc_to_http_filter( + value: str, width: int, height: int, resize_method: str = "crop" + ) -> str: + if not public_baseurl: + raise RuntimeError( + "public_baseurl must be set in the homeserver config to convert MXC URLs to HTTP URLs." + ) + if value[0:6] != "mxc://": return "" From d5554d38fecf661c2846ab505b3b1d229a6825ed Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 8 Feb 2021 09:03:51 -0500 Subject: [PATCH 8/9] Add a clarifying comment. --- synapse/handlers/identity.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index b45e2e95c25b..8fc1e8b91c0a 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -504,6 +504,8 @@ async def requestMsisdnToken( except RequestTimedOutError: raise SynapseError(500, "Timed out contacting identity server") + # It is already checked that public_baseurl is configured since this code + # should only be used if account_threepid_delegate_msisdn is true. assert self.hs.config.public_baseurl # we need to tell the client to send the token back to us, since it doesn't From 9dbfec0b15ef52508b0ddcd767b48a6edbc847fd Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 8 Feb 2021 09:37:09 -0500 Subject: [PATCH 9/9] Fix a typo. --- synapse/config/sso.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/config/sso.py b/synapse/config/sso.py index c04a71369bd3..19bdfd462b1b 100644 --- a/synapse/config/sso.py +++ b/synapse/config/sso.py @@ -41,7 +41,8 @@ def read_config(self, config, **kwargs): ) = self.read_templates( [ "sso_login_idp_picker.html", - "sso_redirect_confirm.html" "sso_auth_confirm.html", + "sso_redirect_confirm.html", + "sso_auth_confirm.html", "sso_error.html", "sso_account_deactivated.html", "sso_auth_success.html",