Consolidate SSO redirects through /_matrix/client/v3/login/sso/redirect(/{idpId}) (#17972)

Consolidate SSO redirects through
`/_matrix/client/v3/login/sso/redirect(/{idpId})`

Spawning from
https://github.com/element-hq/sbg/pull/421#discussion_r1859497330 where
we have a proxy that intercepts responses to
`/_matrix/client/v3/login/sso/redirect(/{idpId})` in order to upgrade
them to use OAuth 2.0 Pushed Authorization Requests (PAR). Instead of
needing to intercept multiple endpoints that redirect to the
authorization endpoint, it seems better to just have Synapse consolidate
to a single flow.


### Testing strategy

1. Create a new OAuth application. I'll be using GitHub for example but
there are [many
options](be65a8ec01/docs/openid.md).
Visit https://github.com/settings/developers -> **New OAuth App**
    - Application name: `Synapse local testing`
    - Homepage URL: `http://localhost:8008`
- Authorization callback URL:
`http://localhost:8008/_synapse/client/oidc/callback`
 1. Update your Synapse `homeserver.yaml`
    ```yaml
    server_name: "my.synapse.server"
    public_baseurl: http://localhost:8008/
    listeners:
      - port: 8008
        bind_addresses: [
          #'::1',
          '127.0.0.1'
        ]
        tls: false
        type: http
        x_forwarded: true
        resources:
          - names: [client, federation, metrics]
            compress: false
    
    # SSO login testing
    oidc_providers:
      - idp_id: github
        idp_name: Github
        idp_brand: "github"  # optional: styling hint for clients
        discover: false
        issuer: "https://github.com/"
        client_id: "xxx" # TO BE FILLED
        client_secret: "xxx" # TO BE FILLED
authorization_endpoint: "https://github.com/login/oauth/authorize"
        token_endpoint: "https://github.com/login/oauth/access_token"
        userinfo_endpoint: "https://api.github.com/user"
        scopes: ["read:user"]
        user_mapping_provider:
          config:
            subject_claim: "id"
            localpart_template: "{{ user.login }}"
            display_name_template: "{{ user.name }}"
    ```
1. Start Synapse: `poetry run synapse_homeserver --config-path
homeserver.yaml`
1. Visit
`http://localhost:8008/_synapse/client/pick_idp?redirectUrl=http%3A%2F%2Fexample.com`
 1. Choose GitHub
1. Notice that you're redirected to GitHub to sign in
(`https://github.com/login/oauth/authorize?...`)

Tested locally and works:

1.
`http://localhost:8008/_synapse/client/pick_idp?idp=oidc-github&redirectUrl=http%3A//example.com`
->
1.
`http://localhost:8008/_matrix/client/v3/login/sso/redirect/oidc-github?redirectUrl=http://example.com`
->
1.
`https://github.com/login/oauth/authorize?response_type=code&client_id=xxx&redirect_uri=http%3A%2F%2Flocalhost%3A8008%2F_synapse%2Fclient%2Foidc%2Fcallback&scope=read%3Auser&state=xxx&nonce=xxx`

changelog.d/17972.misc

@@ -0,0 +1 @@
+Consolidate SSO redirects through `/_matrix/client/v3/login/sso/redirect(/{idpId})`.

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,42 @@ class ConsentURIBuilder:
             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.
+
+        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: Optional ID of the identity provider
+            client_redirect_url: URL to redirect the user to after login
+
+        Returns
+            The URI to follow when choosing a specific identity provider.
+        """
+        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

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 @@ class CasConfig(Config):
 
             # 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 (

synapse/config/server.py

@@ -332,8 +332,14 @@ class ServerConfig(Config):
             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

synapse/rest/synapse/client/pick_idp.py

@@ -21,6 +21,7 @@
 import logging
 from typing import TYPE_CHECKING
 
+from synapse.api.urls import LoginSSORedirectURIBuilder
 from synapse.http.server import (
     DirectServeHtmlResource,
     finish_request,
@@ -49,6 +50,8 @@ class PickIdpResource(DirectServeHtmlResource):
             hs.config.sso.sso_login_idp_picker_template
         )
         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(
@@ -56,25 +59,23 @@ class PickIdpResource(DirectServeHtmlResource):
         )
         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
+        # Otherwise, redirect to the login SSO redirect endpoint for the given IdP
-        providers = self._sso_handler.get_identity_providers()
+        # (which will in turn take us to the the IdP's redirect URI).
-        auth_provider = providers.get(idp)
+        #
-        if not auth_provider:
+        # We could go directly to the IdP's redirect URI, but this way we ensure that
-            logger.info("Unknown idp %r", idp)
+        # the user goes through the same logic as normal flow. Additionally, if a proxy
-            self._sso_handler.render_error(
+        # needs to intercept the request, it only needs to intercept the one endpoint.
-                request, "unknown_idp", "Unknown identity provider ID"
+        sso_login_redirect_url = (
+            self._login_sso_redirect_url_builder.build_login_sso_redirect_uri(
+                idp_id=idp, client_redirect_url=client_redirect_url
             )
-            return
-
-        sso_url = await auth_provider.handle_redirect_request(
-            request, client_redirect_url.encode("utf8")
         )
-        logger.info("Redirecting to %s", sso_url)
+        logger.info("Redirecting to %s", sso_login_redirect_url)
-        request.redirect(sso_url)
+        request.redirect(sso_login_redirect_url)
         finish_request(request)
 
     async def _serve_id_picker(

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:
+# <https://www.gnu.org/licenses/agpl-3.0.html>.
+#
+
+
+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?<ab c>&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",
+        )

tests/rest/client/test_login.py

@@ -43,6 +43,7 @@ from twisted.web.resource import Resource
 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,6 +70,10 @@ try:
 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"
@@ -77,7 +82,7 @@ SYNAPSE_SERVER_PUBLIC_HOSTNAME = "synapse"
 # 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 +114,23 @@ ADDITIONAL_LOGIN_FLOWS = [
 ]
 
 
+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 = parsed_uri.path
+    if parsed_uri.query:
+        relative_uri += "?" + parsed_uri.query
+
+    return relative_uri
+
+
 class TestSpamChecker:
     def __init__(self, config: None, api: ModuleApi):
         api.register_spam_checker_callbacks(
@@ -614,7 +636,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 +675,9 @@ class MultiSSOTestCase(unittest.HomeserverTestCase):
         ]
         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))
@@ -725,6 +750,32 @@ class MultiSSOTestCase(unittest.HomeserverTestCase):
             + "&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
+        channel = self.make_request(
+            "GET",
+            # 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)
         location_headers = channel.headers.getRawHeaders("Location")
         assert location_headers
@@ -750,6 +801,32 @@ class MultiSSOTestCase(unittest.HomeserverTestCase):
             + 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
+        channel = self.make_request(
+            "GET",
+            # 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)
         location_headers = channel.headers.getRawHeaders("Location")
         assert location_headers
@@ -773,13 +850,38 @@ class MultiSSOTestCase(unittest.HomeserverTestCase):
             # pick the default OIDC provider
             channel = self.make_request(
                 "GET",
-                "/_synapse/client/pick_idp?redirectUrl="
+                f"/_synapse/client/pick_idp?redirectUrl={urllib.parse.quote_plus(TEST_CLIENT_REDIRECT_URL)}&idp=oidc",
-                + 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
+            channel = self.make_request(
+                "GET",
+                # 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)
+        location_headers = channel.headers.getRawHeaders("Location")
+        assert location_headers
         oidc_uri = location_headers[0]
         oidc_uri_path, oidc_uri_query = oidc_uri.split("?", 1)
 
@@ -838,12 +940,38 @@ class MultiSSOTestCase(unittest.HomeserverTestCase):
         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
+        channel = self.make_request(
+            "GET",
+            # 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)
 
     def test_client_idp_redirect_to_unknown(self) -> None:
         """If the client tries to pick an unknown IdP, return a 404"""
@@ -1473,7 +1601,7 @@ class UsernamePickerTestCase(HomeserverTestCase):
 
     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)

tests/rest/client/utils.py

@@ -889,7 +889,7 @@ class RestHelper:
             "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 @@ class RestHelper:
 
         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)