From 91d1b234a31703500ebfc342f9bf3d8cda323760 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 27 Nov 2024 15:25:01 -0600 Subject: [PATCH 01/11] Consolidate SSO redirects through `/_matrix/client/v3/login/sso/redirect(/{idpId})` --- synapse/config/cas.py | 6 +++-- synapse/config/server.py | 6 +++++ synapse/rest/synapse/client/pick_idp.py | 30 ++++++++++++------------- 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/synapse/config/cas.py b/synapse/config/cas.py index fa59c350c15..c32bf36951d 100644 --- a/synapse/config/cas.py +++ b/synapse/config/cas.py @@ -20,7 +20,7 @@ # # -from typing import Any, List +from typing import Any, List, Optional from synapse.config.sso import SsoAttributeRequirement from synapse.types import JsonDict @@ -46,7 +46,9 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: # TODO Update this to a _synapse URL. public_baseurl = self.root.server.public_baseurl - self.cas_service_url = public_baseurl + "_matrix/client/r0/login/cas/ticket" + self.cas_service_url: Optional[str] = ( + public_baseurl + "_matrix/client/r0/login/cas/ticket" + ) self.cas_protocol_version = cas_config.get("protocol_version") if ( diff --git a/synapse/config/server.py b/synapse/config/server.py index ad7331de428..6b299836176 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -332,8 +332,14 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: logger.info("Using default public_baseurl %s", public_baseurl) else: self.serve_client_wellknown = True + # Ensure that public_baseurl ends with a trailing slash if public_baseurl[-1] != "/": public_baseurl += "/" + + # Scrutinize user-provided config + if not isinstance(public_baseurl, str): + raise ConfigError("Must be a string", ("public_baseurl",)) + self.public_baseurl = public_baseurl # check that public_baseurl is valid diff --git a/synapse/rest/synapse/client/pick_idp.py b/synapse/rest/synapse/client/pick_idp.py index f26929bd608..eab8244c452 100644 --- a/synapse/rest/synapse/client/pick_idp.py +++ b/synapse/rest/synapse/client/pick_idp.py @@ -20,7 +20,9 @@ # import logging from typing import TYPE_CHECKING +from urllib.parse import urljoin +from synapse.api.urls import CLIENT_API_PREFIX from synapse.http.server import ( DirectServeHtmlResource, finish_request, @@ -49,6 +51,7 @@ def __init__(self, hs: "HomeServer"): hs.config.sso.sso_login_idp_picker_template ) self._server_name = hs.hostname + self._public_baseurl = hs.config.server.public_baseurl async def _async_render_GET(self, request: SynapseRequest) -> None: client_redirect_url = parse_string( @@ -56,25 +59,22 @@ async def _async_render_GET(self, request: SynapseRequest) -> None: ) idp = parse_string(request, "idp", required=False) - # if we need to pick an IdP, do so + # If we need to pick an IdP, do so if not idp: return await self._serve_id_picker(request, client_redirect_url) - # otherwise, redirect to the IdP's redirect URI - providers = self._sso_handler.get_identity_providers() - auth_provider = providers.get(idp) - if not auth_provider: - logger.info("Unknown idp %r", idp) - self._sso_handler.render_error( - request, "unknown_idp", "Unknown identity provider ID" - ) - return - - sso_url = await auth_provider.handle_redirect_request( - request, client_redirect_url.encode("utf8") + # Otherwise, redirect to the login SSO redirect endpoint for the given IdP + # (which will in turn take us to the the IdP's redirect URI). + # + # We could go directly to the IdP's redirect URI, but this way we ensure that + # the user goes through the same logic as normal flow. Additionally, if a proxy + # needs to intercept the request, it only needs to intercept the one endpoint. + sso_login_redirect_url = urljoin( + self._public_baseurl, + f"{CLIENT_API_PREFIX}/v3/login/sso/redirect/{idp}?redirectUrl={client_redirect_url}", ) - logger.info("Redirecting to %s", sso_url) - request.redirect(sso_url) + logger.info("Redirecting to %s", sso_login_redirect_url) + request.redirect(sso_login_redirect_url) finish_request(request) async def _serve_id_picker( From 6276d9864740eeaae2e6d8d9fbc9fb38b1c7e333 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 27 Nov 2024 15:42:24 -0600 Subject: [PATCH 02/11] Add changelog --- changelog.d/17972.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17972.misc diff --git a/changelog.d/17972.misc b/changelog.d/17972.misc new file mode 100644 index 00000000000..e7f009d20d4 --- /dev/null +++ b/changelog.d/17972.misc @@ -0,0 +1 @@ +Consolidate SSO redirects through `/_matrix/client/v3/login/sso/redirect(/{idpId})`. From d0b1552366473374d76c7c328554f5518c1504d8 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 27 Nov 2024 18:13:30 -0600 Subject: [PATCH 03/11] Fix `tests.rest.client.test_login.MultiSSOTestCase.test_login_via_oidc` --- synapse/api/urls.py | 39 ++++++++++++++++- synapse/rest/synapse/client/pick_idp.py | 11 ++--- tests/api/test_urls.py | 55 ++++++++++++++++++++++++ tests/rest/client/test_login.py | 57 ++++++++++++++++++++++--- tests/utils.py | 5 +++ 5 files changed, 154 insertions(+), 13 deletions(-) create mode 100644 tests/api/test_urls.py diff --git a/synapse/api/urls.py b/synapse/api/urls.py index 03a3e96f289..409089cf574 100644 --- a/synapse/api/urls.py +++ b/synapse/api/urls.py @@ -23,7 +23,8 @@ import hmac from hashlib import sha256 -from urllib.parse import urlencode +from typing import Optional +from urllib.parse import urlencode, urljoin from synapse.config import ConfigError from synapse.config.homeserver import HomeServerConfig @@ -66,3 +67,39 @@ def build_user_consent_uri(self, user_id: str) -> str: urlencode({"u": user_id, "h": mac}), ) return consent_uri + + +class LoginSSORedirectURIBuilder: + def __init__(self, hs_config: HomeServerConfig): + self._public_baseurl = hs_config.server.public_baseurl + + def build_login_sso_redirect_uri( + self, *, idp_id: Optional[str], client_redirect_url: str + ) -> str: + """Build a `/login/sso/redirect` URI for the given identity provider + + Args: + idp_id: ID of the identity provider + client_redirect_url: URL to redirect the user to after login + + Returns + The URI where the user can do consent + """ + base_url = urljoin( + self._public_baseurl, + f"{CLIENT_API_PREFIX}/v3/login/sso/redirect", + ) + + serialized_query_parameters = urlencode({"redirectUrl": client_redirect_url}) + + if idp_id: + resultant_url = urljoin( + # We have to add a trailing slash to the base URL to ensure that the + # last path segment is not stripped away when joining with another path. + f"{base_url}/", + f"{idp_id}?{serialized_query_parameters}", + ) + else: + resultant_url = f"{base_url}?{serialized_query_parameters}" + + return resultant_url diff --git a/synapse/rest/synapse/client/pick_idp.py b/synapse/rest/synapse/client/pick_idp.py index eab8244c452..bdfde2d5e2f 100644 --- a/synapse/rest/synapse/client/pick_idp.py +++ b/synapse/rest/synapse/client/pick_idp.py @@ -20,9 +20,8 @@ # import logging from typing import TYPE_CHECKING -from urllib.parse import urljoin -from synapse.api.urls import CLIENT_API_PREFIX +from synapse.api.urls import LoginSSORedirectURIBuilder from synapse.http.server import ( DirectServeHtmlResource, finish_request, @@ -52,6 +51,7 @@ def __init__(self, hs: "HomeServer"): ) self._server_name = hs.hostname self._public_baseurl = hs.config.server.public_baseurl + self.login_sso_redirect_url_builder = LoginSSORedirectURIBuilder(hs.config) async def _async_render_GET(self, request: SynapseRequest) -> None: client_redirect_url = parse_string( @@ -69,9 +69,10 @@ async def _async_render_GET(self, request: SynapseRequest) -> None: # We could go directly to the IdP's redirect URI, but this way we ensure that # the user goes through the same logic as normal flow. Additionally, if a proxy # needs to intercept the request, it only needs to intercept the one endpoint. - sso_login_redirect_url = urljoin( - self._public_baseurl, - f"{CLIENT_API_PREFIX}/v3/login/sso/redirect/{idp}?redirectUrl={client_redirect_url}", + sso_login_redirect_url = ( + self.login_sso_redirect_url_builder.build_login_sso_redirect_uri( + idp_id=idp, client_redirect_url=client_redirect_url + ) ) logger.info("Redirecting to %s", sso_login_redirect_url) request.redirect(sso_login_redirect_url) diff --git a/tests/api/test_urls.py b/tests/api/test_urls.py new file mode 100644 index 00000000000..ce156a05dc4 --- /dev/null +++ b/tests/api/test_urls.py @@ -0,0 +1,55 @@ +# +# This file is licensed under the Affero General Public License (AGPL) version 3. +# +# Copyright (C) 2024 New Vector, Ltd +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as +# published by the Free Software Foundation, either version 3 of the +# License, or (at your option) any later version. +# +# See the GNU Affero General Public License for more details: +# . +# + + +from twisted.test.proto_helpers import MemoryReactor + +from synapse.api.urls import LoginSSORedirectURIBuilder +from synapse.server import HomeServer +from synapse.util import Clock + +from tests.unittest import HomeserverTestCase + +# a (valid) url with some annoying characters in. %3D is =, %26 is &, %2B is + +TRICKY_TEST_CLIENT_REDIRECT_URL = 'https://x?&q"+%3D%2B"="fö%26=o"' + + +class LoginSSORedirectURIBuilderTestCase(HomeserverTestCase): + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.login_sso_redirect_url_builder = LoginSSORedirectURIBuilder(hs.config) + + def test_no_idp_id(self) -> None: + self.assertEqual( + self.login_sso_redirect_url_builder.build_login_sso_redirect_uri( + idp_id=None, client_redirect_url="http://example.com/redirect" + ), + "https://test/_matrix/client/v3/login/sso/redirect?redirectUrl=http%3A%2F%2Fexample.com%2Fredirect", + ) + + def test_explicit_idp_id(self) -> None: + self.assertEqual( + self.login_sso_redirect_url_builder.build_login_sso_redirect_uri( + idp_id="oidc-github", client_redirect_url="http://example.com/redirect" + ), + "https://test/_matrix/client/v3/login/sso/redirect/oidc-github?redirectUrl=http%3A%2F%2Fexample.com%2Fredirect", + ) + + def test_tricky_redirect_uri(self) -> None: + self.assertEqual( + self.login_sso_redirect_url_builder.build_login_sso_redirect_uri( + idp_id="oidc-github", + client_redirect_url=TRICKY_TEST_CLIENT_REDIRECT_URL, + ), + "https://test/_matrix/client/v3/login/sso/redirect/oidc-github?redirectUrl=https%3A%2F%2Fx%3F%3Cab+c%3E%26q%22%2B%253D%252B%22%3D%22f%C3%B6%2526%3Do%22", + ) diff --git a/tests/rest/client/test_login.py b/tests/rest/client/test_login.py index cbd6d8d4bf8..006fb16c7c6 100644 --- a/tests/rest/client/test_login.py +++ b/tests/rest/client/test_login.py @@ -43,6 +43,7 @@ import synapse.rest.admin from synapse.api.constants import ApprovalNoticeMedium, LoginType from synapse.api.errors import Codes +from synapse.api.urls import LoginSSORedirectURIBuilder from synapse.appservice import ApplicationService from synapse.http.client import RawHeaders from synapse.module_api import ModuleApi @@ -69,15 +70,24 @@ except ImportError: HAS_JWT = False +import logging + +logger = logging.getLogger(__name__) + # synapse server name: used to populate public_baseurl in some tests -SYNAPSE_SERVER_PUBLIC_HOSTNAME = "synapse" +# +# Because we can only specify the URI path when doing a `make_request(...)`, this needs +# to match the hostname/port that's used in the `make_request(...)` calls; because the +# `SsoRedirectServlet` expects us to use the "canonical URL" for the homeserver in order +# for the cookies to be visible. +SYNAPSE_SERVER_PUBLIC_HOSTNAME = "127.0.0.1:8888" # public_baseurl for some tests. It uses an http:// scheme because # FakeChannel.isSecure() returns False, so synapse will see the requested uri as # http://..., so using http in the public_baseurl stops Synapse trying to redirect to # https://.... -BASE_URL = "http://%s/" % (SYNAPSE_SERVER_PUBLIC_HOSTNAME,) +PUBLIC_BASEURL = "http://%s/" % (SYNAPSE_SERVER_PUBLIC_HOSTNAME,) # CAS server used in some tests CAS_SERVER = "https://fake.test" @@ -109,6 +119,13 @@ ] +def get_relative_uri_from_absolute_uri(absolute_uri: str) -> str: + parsed_uri = urllib.parse.urlparse(absolute_uri) + assert parsed_uri.scheme == "http" or parsed_uri.scheme == "https" + relative_uri = "?".join(filter(None, [parsed_uri.path, parsed_uri.query])) + return relative_uri + + class TestSpamChecker: def __init__(self, config: None, api: ModuleApi): api.register_spam_checker_callbacks( @@ -614,7 +631,7 @@ class MultiSSOTestCase(unittest.HomeserverTestCase): def default_config(self) -> Dict[str, Any]: config = super().default_config() - config["public_baseurl"] = BASE_URL + config["public_baseurl"] = PUBLIC_BASEURL config["cas_config"] = { "enabled": True, @@ -653,6 +670,9 @@ def default_config(self) -> Dict[str, Any]: ] return config + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.login_sso_redirect_url_builder = LoginSSORedirectURIBuilder(hs.config) + def create_resource_dict(self) -> Dict[str, Resource]: d = super().create_resource_dict() d.update(build_synapse_client_resource_tree(self.hs)) @@ -773,10 +793,33 @@ def test_login_via_oidc(self) -> None: # pick the default OIDC provider channel = self.make_request( "GET", - "/_synapse/client/pick_idp?redirectUrl=" - + urllib.parse.quote_plus(TEST_CLIENT_REDIRECT_URL) - + "&idp=oidc", + f"/_synapse/client/pick_idp?redirectUrl={urllib.parse.quote_plus(TEST_CLIENT_REDIRECT_URL)}&idp=oidc", + ) + self.assertEqual(channel.code, 302, channel.result) + location_headers = channel.headers.getRawHeaders("Location") + assert location_headers + sso_login_redirect_uri = location_headers[0] + + # it should redirect us to the standard login SSO redirect flow + self.assertEqual( + sso_login_redirect_uri, + self.login_sso_redirect_url_builder.build_login_sso_redirect_uri( + idp_id="oidc", client_redirect_url=TEST_CLIENT_REDIRECT_URL + ), + ) + + with fake_oidc_server.patch_homeserver(hs=self.hs): + # follow the redirect + # + # We have to make this relative to be compatible with `make_request(...)` + relative_sso_login_redirect_uri = get_relative_uri_from_absolute_uri( + sso_login_redirect_uri + ) + channel = self.make_request( + "GET", + relative_sso_login_redirect_uri, ) + self.assertEqual(channel.code, 302, channel.result) location_headers = channel.headers.getRawHeaders("Location") assert location_headers @@ -1473,7 +1516,7 @@ def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: def default_config(self) -> Dict[str, Any]: config = super().default_config() - config["public_baseurl"] = BASE_URL + config["public_baseurl"] = PUBLIC_BASEURL config["oidc_config"] = {} config["oidc_config"].update(TEST_OIDC_CONFIG) diff --git a/tests/utils.py b/tests/utils.py index 9fd26ef348e..e0987e7bda6 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -56,6 +56,11 @@ except ImportError: HAS_AUTHLIB = False + +import logging + +logger = logging.getLogger(__name__) + # set this to True to run the tests against postgres instead of sqlite. # # When running under postgres, we first create a base database with the name From 4ea1a51781acf26c57c1cae2530481256a692132 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 27 Nov 2024 18:19:51 -0600 Subject: [PATCH 04/11] Fix the rest of the `MultiSSOTestCase` tests --- tests/rest/client/test_login.py | 76 ++++++++++++++++++++++++++++++++- 1 file changed, 74 insertions(+), 2 deletions(-) diff --git a/tests/rest/client/test_login.py b/tests/rest/client/test_login.py index 006fb16c7c6..c7a9c1cd27f 100644 --- a/tests/rest/client/test_login.py +++ b/tests/rest/client/test_login.py @@ -745,6 +745,30 @@ def test_multi_sso_redirect_to_cas(self) -> None: + "&idp=cas", shorthand=False, ) + self.assertEqual(channel.code, 302, channel.result) + location_headers = channel.headers.getRawHeaders("Location") + assert location_headers + sso_login_redirect_uri = location_headers[0] + + # it should redirect us to the standard login SSO redirect flow + self.assertEqual( + sso_login_redirect_uri, + self.login_sso_redirect_url_builder.build_login_sso_redirect_uri( + idp_id="cas", client_redirect_url=TEST_CLIENT_REDIRECT_URL + ), + ) + + # follow the redirect + # + # We have to make this relative to be compatible with `make_request(...)` + relative_sso_login_redirect_uri = get_relative_uri_from_absolute_uri( + sso_login_redirect_uri + ) + channel = self.make_request( + "GET", + relative_sso_login_redirect_uri, + ) + self.assertEqual(channel.code, 302, channel.result) location_headers = channel.headers.getRawHeaders("Location") assert location_headers @@ -770,6 +794,30 @@ def test_multi_sso_redirect_to_saml(self) -> None: + urllib.parse.quote_plus(TEST_CLIENT_REDIRECT_URL) + "&idp=saml", ) + self.assertEqual(channel.code, 302, channel.result) + location_headers = channel.headers.getRawHeaders("Location") + assert location_headers + sso_login_redirect_uri = location_headers[0] + + # it should redirect us to the standard login SSO redirect flow + self.assertEqual( + sso_login_redirect_uri, + self.login_sso_redirect_url_builder.build_login_sso_redirect_uri( + idp_id="saml", client_redirect_url=TEST_CLIENT_REDIRECT_URL + ), + ) + + # follow the redirect + # + # We have to make this relative to be compatible with `make_request(...)` + relative_sso_login_redirect_uri = get_relative_uri_from_absolute_uri( + sso_login_redirect_uri + ) + channel = self.make_request( + "GET", + relative_sso_login_redirect_uri, + ) + self.assertEqual(channel.code, 302, channel.result) location_headers = channel.headers.getRawHeaders("Location") assert location_headers @@ -881,12 +929,36 @@ def test_login_via_oidc(self) -> None: self.assertEqual(chan.json_body["user_id"], "@user1:test") def test_multi_sso_redirect_to_unknown(self) -> None: - """An unknown IdP should cause a 400""" + """An unknown IdP should cause a 404""" channel = self.make_request( "GET", "/_synapse/client/pick_idp?redirectUrl=http://x&idp=xyz", ) - self.assertEqual(channel.code, 400, channel.result) + self.assertEqual(channel.code, 302, channel.result) + location_headers = channel.headers.getRawHeaders("Location") + assert location_headers + sso_login_redirect_uri = location_headers[0] + + # it should redirect us to the standard login SSO redirect flow + self.assertEqual( + sso_login_redirect_uri, + self.login_sso_redirect_url_builder.build_login_sso_redirect_uri( + idp_id="xyz", client_redirect_url="http://x" + ), + ) + + # follow the redirect + # + # We have to make this relative to be compatible with `make_request(...)` + relative_sso_login_redirect_uri = get_relative_uri_from_absolute_uri( + sso_login_redirect_uri + ) + channel = self.make_request( + "GET", + relative_sso_login_redirect_uri, + ) + + self.assertEqual(channel.code, 404, channel.result) def test_client_idp_redirect_to_unknown(self) -> None: """If the client tries to pick an unknown IdP, return a 404""" From 439aefb1939b22178da933a2111be7e89a93bbfa Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 27 Nov 2024 18:32:27 -0600 Subject: [PATCH 05/11] Use better fix --- tests/rest/client/test_login.py | 63 +++++++++++++++++---------------- tests/rest/client/utils.py | 7 ++-- 2 files changed, 37 insertions(+), 33 deletions(-) diff --git a/tests/rest/client/test_login.py b/tests/rest/client/test_login.py index c7a9c1cd27f..49d5cb32e3f 100644 --- a/tests/rest/client/test_login.py +++ b/tests/rest/client/test_login.py @@ -76,12 +76,7 @@ # synapse server name: used to populate public_baseurl in some tests -# -# Because we can only specify the URI path when doing a `make_request(...)`, this needs -# to match the hostname/port that's used in the `make_request(...)` calls; because the -# `SsoRedirectServlet` expects us to use the "canonical URL" for the homeserver in order -# for the cookies to be visible. -SYNAPSE_SERVER_PUBLIC_HOSTNAME = "127.0.0.1:8888" +SYNAPSE_SERVER_PUBLIC_HOSTNAME = "synapse" # public_baseurl for some tests. It uses an http:// scheme because # FakeChannel.isSecure() returns False, so synapse will see the requested uri as @@ -759,14 +754,16 @@ def test_multi_sso_redirect_to_cas(self) -> None: ) # follow the redirect - # - # We have to make this relative to be compatible with `make_request(...)` - relative_sso_login_redirect_uri = get_relative_uri_from_absolute_uri( - sso_login_redirect_uri - ) channel = self.make_request( "GET", - relative_sso_login_redirect_uri, + # We have to make this relative to be compatible with `make_request(...)` + get_relative_uri_from_absolute_uri(sso_login_redirect_uri), + # We have to set the Host header to match the `public_baseurl` to avoid + # the extra redirect in the `SsoRedirectServlet` in order for the + # cookies to be visible. + custom_headers=[ + ("Host", SYNAPSE_SERVER_PUBLIC_HOSTNAME), + ], ) self.assertEqual(channel.code, 302, channel.result) @@ -808,14 +805,16 @@ def test_multi_sso_redirect_to_saml(self) -> None: ) # follow the redirect - # - # We have to make this relative to be compatible with `make_request(...)` - relative_sso_login_redirect_uri = get_relative_uri_from_absolute_uri( - sso_login_redirect_uri - ) channel = self.make_request( "GET", - relative_sso_login_redirect_uri, + # We have to make this relative to be compatible with `make_request(...)` + get_relative_uri_from_absolute_uri(sso_login_redirect_uri), + # We have to set the Host header to match the `public_baseurl` to avoid + # the extra redirect in the `SsoRedirectServlet` in order for the + # cookies to be visible. + custom_headers=[ + ("Host", SYNAPSE_SERVER_PUBLIC_HOSTNAME), + ], ) self.assertEqual(channel.code, 302, channel.result) @@ -858,14 +857,16 @@ def test_login_via_oidc(self) -> None: with fake_oidc_server.patch_homeserver(hs=self.hs): # follow the redirect - # - # We have to make this relative to be compatible with `make_request(...)` - relative_sso_login_redirect_uri = get_relative_uri_from_absolute_uri( - sso_login_redirect_uri - ) channel = self.make_request( "GET", - relative_sso_login_redirect_uri, + # We have to make this relative to be compatible with `make_request(...)` + get_relative_uri_from_absolute_uri(sso_login_redirect_uri), + # We have to set the Host header to match the `public_baseurl` to avoid + # the extra redirect in the `SsoRedirectServlet` in order for the + # cookies to be visible. + custom_headers=[ + ("Host", SYNAPSE_SERVER_PUBLIC_HOSTNAME), + ], ) self.assertEqual(channel.code, 302, channel.result) @@ -948,14 +949,16 @@ def test_multi_sso_redirect_to_unknown(self) -> None: ) # follow the redirect - # - # We have to make this relative to be compatible with `make_request(...)` - relative_sso_login_redirect_uri = get_relative_uri_from_absolute_uri( - sso_login_redirect_uri - ) channel = self.make_request( "GET", - relative_sso_login_redirect_uri, + # We have to make this relative to be compatible with `make_request(...)` + get_relative_uri_from_absolute_uri(sso_login_redirect_uri), + # We have to set the Host header to match the `public_baseurl` to avoid + # the extra redirect in the `SsoRedirectServlet` in order for the + # cookies to be visible. + custom_headers=[ + ("Host", SYNAPSE_SERVER_PUBLIC_HOSTNAME), + ], ) self.assertEqual(channel.code, 404, channel.result) diff --git a/tests/rest/client/utils.py b/tests/rest/client/utils.py index a1c284726ad..dbd6049f9fc 100644 --- a/tests/rest/client/utils.py +++ b/tests/rest/client/utils.py @@ -889,7 +889,7 @@ def initiate_sso_login( "GET", uri, ) - assert channel.code == 302 + assert channel.code == 302, f"Expected 302 for {uri}, got {channel.code}" # hit the redirect url again with the right Host header, which should now issue # a cookie and redirect to the SSO provider. @@ -901,17 +901,18 @@ def get_location(channel: FakeChannel) -> str: location = get_location(channel) parts = urllib.parse.urlsplit(location) + next_uri = urllib.parse.urlunsplit(("", "") + parts[2:]) channel = make_request( self.reactor, self.site, "GET", - urllib.parse.urlunsplit(("", "") + parts[2:]), + next_uri, custom_headers=[ ("Host", parts[1]), ], ) - assert channel.code == 302 + assert channel.code == 302, f"Expected 302 for {next_uri}, got {channel.code}" channel.extract_cookies(cookies) return get_location(channel) From f97c2f1dcf2b093d6b290afdc12ff1ec726da863 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 27 Nov 2024 18:36:26 -0600 Subject: [PATCH 06/11] Update docstring --- synapse/api/urls.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/synapse/api/urls.py b/synapse/api/urls.py index 409089cf574..c74d8902024 100644 --- a/synapse/api/urls.py +++ b/synapse/api/urls.py @@ -76,14 +76,17 @@ def __init__(self, hs_config: HomeServerConfig): def build_login_sso_redirect_uri( self, *, idp_id: Optional[str], client_redirect_url: str ) -> str: - """Build a `/login/sso/redirect` URI for the given identity provider + """Build a `/login/sso/redirect` URI for the given identity provider. + + Builds `/_matrix/client/v3/login/sso/redirect/{idpId}?redirectUrl=xxx` when `idp_id` is specified. + Otherwise, builds `/_matrix/client/v3/login/sso/redirect?redirectUrl=xxx` when `idp_id` is `None`. Args: idp_id: ID of the identity provider client_redirect_url: URL to redirect the user to after login Returns - The URI where the user can do consent + The URI to follow when choosing a specific identity provider. """ base_url = urljoin( self._public_baseurl, From eaad89c8740c6a186230d281c922170a41237516 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 27 Nov 2024 18:40:40 -0600 Subject: [PATCH 07/11] Less clever --- tests/rest/client/test_login.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/test_login.py b/tests/rest/client/test_login.py index 49d5cb32e3f..1451fd7c29c 100644 --- a/tests/rest/client/test_login.py +++ b/tests/rest/client/test_login.py @@ -115,9 +115,19 @@ def get_relative_uri_from_absolute_uri(absolute_uri: str) -> str: + """ + Peels off the path and query string from an absolute URI. Useful when interacting + with `make_request(...)` util function which expects a relative path instead of a + full URI. + """ parsed_uri = urllib.parse.urlparse(absolute_uri) + # Sanity check that we're working with an absolute URI assert parsed_uri.scheme == "http" or parsed_uri.scheme == "https" - relative_uri = "?".join(filter(None, [parsed_uri.path, parsed_uri.query])) + + relative_uri = parsed_uri.path + if parsed_uri.query: + relative_uri += "?" + parsed_uri.query + return relative_uri From 3c6c0de572a5a2cff385d882f63c6c264afaf0a9 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 29 Nov 2024 10:49:30 -0600 Subject: [PATCH 08/11] Match the surroundings and doesn't need to be public See https://github.com/element-hq/synapse/pull/17972#discussion_r1862563316 --- synapse/rest/synapse/client/pick_idp.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/rest/synapse/client/pick_idp.py b/synapse/rest/synapse/client/pick_idp.py index bdfde2d5e2f..5e599f85b0b 100644 --- a/synapse/rest/synapse/client/pick_idp.py +++ b/synapse/rest/synapse/client/pick_idp.py @@ -51,7 +51,7 @@ def __init__(self, hs: "HomeServer"): ) self._server_name = hs.hostname self._public_baseurl = hs.config.server.public_baseurl - self.login_sso_redirect_url_builder = LoginSSORedirectURIBuilder(hs.config) + self._login_sso_redirect_url_builder = LoginSSORedirectURIBuilder(hs.config) async def _async_render_GET(self, request: SynapseRequest) -> None: client_redirect_url = parse_string( @@ -70,7 +70,7 @@ async def _async_render_GET(self, request: SynapseRequest) -> None: # the user goes through the same logic as normal flow. Additionally, if a proxy # needs to intercept the request, it only needs to intercept the one endpoint. sso_login_redirect_url = ( - self.login_sso_redirect_url_builder.build_login_sso_redirect_uri( + self._login_sso_redirect_url_builder.build_login_sso_redirect_uri( idp_id=idp, client_redirect_url=client_redirect_url ) ) From feb7e8eef9c0077003209431b1f25c249a60735e Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 29 Nov 2024 10:50:20 -0600 Subject: [PATCH 09/11] Remove dangling log setup See https://github.com/element-hq/synapse/pull/17972#discussion_r1862564576 --- tests/utils.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/utils.py b/tests/utils.py index e0987e7bda6..2fbe26b8a80 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -57,10 +57,6 @@ HAS_AUTHLIB = False -import logging - -logger = logging.getLogger(__name__) - # set this to True to run the tests against postgres instead of sqlite. # # When running under postgres, we first create a base database with the name From be72682441cfb52135eadaff7c13b51e081b67d9 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 29 Nov 2024 10:50:42 -0600 Subject: [PATCH 10/11] Optional detail in docstring See https://github.com/element-hq/synapse/pull/17972#discussion_r1862559104 --- synapse/api/urls.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/api/urls.py b/synapse/api/urls.py index c74d8902024..655b5edd7a2 100644 --- a/synapse/api/urls.py +++ b/synapse/api/urls.py @@ -82,7 +82,7 @@ def build_login_sso_redirect_uri( Otherwise, builds `/_matrix/client/v3/login/sso/redirect?redirectUrl=xxx` when `idp_id` is `None`. Args: - idp_id: ID of the identity provider + idp_id: Optional ID of the identity provider client_redirect_url: URL to redirect the user to after login Returns From a8c8f63c12eb116cd6bf10e91665ccce370b10c0 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 29 Nov 2024 10:52:03 -0600 Subject: [PATCH 11/11] Remove extra newline --- tests/utils.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/utils.py b/tests/utils.py index 2fbe26b8a80..9fd26ef348e 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -56,7 +56,6 @@ except ImportError: HAS_AUTHLIB = False - # set this to True to run the tests against postgres instead of sqlite. # # When running under postgres, we first create a base database with the name