Remove unstable MSC2858 API, including `experimental.msc2858_enabled` config option (#10693)

Signed-off-by: Sean Quah <seanq@element.io>

9 files changed, 19 insertions, 127 deletions

diff --git a/changelog.d/10693.removal b/changelog.d/10693.removal
new file mode 100644
index 0000000000..0255a8be74
--- /dev/null
+++ b/changelog.d/10693.removal
@@ -0,0 +1 @@
+Remove [unstable MSC2858 API](https://github.com/matrix-org/matrix-doc/blob/master/proposals/2858-Multiple-SSO-Identity-Providers.md#unstable-prefix), including the undocumented `experimental.msc2858_enabled` config option. The unstable API has been deprecated since Synapse 1.35. Client authors should update their clients to use the stable API introduced in Synapse 1.30 if they have not already done so.
diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py
index 95deda11a5..7b0381c06a 100644
--- a/synapse/config/experimental.py
+++ b/synapse/config/experimental.py
@@ -24,9 +24,6 @@ class ExperimentalConfig(Config):
     def read_config(self, config: JsonDict, **kwargs):
         experimental = config.get("experimental_features") or {}
 
-        # MSC2858 (multiple SSO identity providers)
-        self.msc2858_enabled: bool = experimental.get("msc2858_enabled", False)
-
         # MSC3026 (busy presence state)
         self.msc3026_enabled: bool = experimental.get("msc3026_enabled", False)
 
diff --git a/synapse/config/oidc.py b/synapse/config/oidc.py
index ba89d11cf0..7e67fbada1 100644
--- a/synapse/config/oidc.py
+++ b/synapse/config/oidc.py
@@ -277,12 +277,6 @@ OIDC_PROVIDER_CONFIG_SCHEMA = {
             "maxLength": 255,
             "pattern": "^[a-z][a-z0-9_.-]*$",
         },
-        "idp_unstable_brand": {
-            "type": "string",
-            "minLength": 1,
-            "maxLength": 255,
-            "pattern": "^[a-z][a-z0-9_.-]*$",
-        },
         "discover": {"type": "boolean"},
         "issuer": {"type": "string"},
         "client_id": {"type": "string"},
@@ -483,7 +477,6 @@ def _parse_oidc_config_dict(
         idp_name=oidc_config.get("idp_name", "OIDC"),
         idp_icon=idp_icon,
         idp_brand=oidc_config.get("idp_brand"),
-        unstable_idp_brand=oidc_config.get("unstable_idp_brand"),
         discover=oidc_config.get("discover", True),
         issuer=oidc_config["issuer"],
         client_id=oidc_config["client_id"],
@@ -531,9 +524,6 @@ class OidcProviderConfig:
     # Optional brand identifier for this IdP.
     idp_brand = attr.ib(type=Optional[str])
 
-    # Optional brand identifier for the unstable API (see MSC2858).
-    unstable_idp_brand = attr.ib(type=Optional[str])
-
     # whether the OIDC discovery mechanism is used to discover endpoints
     discover = attr.ib(type=bool)
 
diff --git a/synapse/handlers/cas.py b/synapse/handlers/cas.py
index 0325f86e20..47ddabbe46 100644
--- a/synapse/handlers/cas.py
+++ b/synapse/handlers/cas.py
@@ -82,7 +82,6 @@ class CasHandler:
         # the SsoIdentityProvider protocol type.
         self.idp_icon = None
         self.idp_brand = None
-        self.unstable_idp_brand = None
 
         self._sso_handler = hs.get_sso_handler()
 
diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py
index eca8f16040..648fcf76f8 100644
--- a/synapse/handlers/oidc.py
+++ b/synapse/handlers/oidc.py
@@ -338,9 +338,6 @@ class OidcProvider:
         # optional brand identifier for this auth provider
         self.idp_brand = provider.idp_brand
 
-        # Optional brand identifier for the unstable API (see MSC2858).
-        self.unstable_idp_brand = provider.unstable_idp_brand
-
         self._sso_handler = hs.get_sso_handler()
 
         self._sso_handler.register_identity_provider(self)
diff --git a/synapse/handlers/saml.py b/synapse/handlers/saml.py
index e6e71e9729..0066d570c5 100644
--- a/synapse/handlers/saml.py
+++ b/synapse/handlers/saml.py
@@ -80,7 +80,6 @@ class SamlHandler(BaseHandler):
         # the SsoIdentityProvider protocol type.
         self.idp_icon = None
         self.idp_brand = None
-        self.unstable_idp_brand = None
 
         # a map from saml session id to Saml2SessionData object
         self._outstanding_requests_dict: Dict[str, Saml2SessionData] = {}
diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py
index 0e6ebb574e..0fdc6dd9e7 100644
--- a/synapse/handlers/sso.py
+++ b/synapse/handlers/sso.py
@@ -104,11 +104,6 @@ class SsoIdentityProvider(Protocol):
         """Optional branding identifier"""
         return None
 
-    @property
-    def unstable_idp_brand(self) -> Optional[str]:
-        """Optional brand identifier for the unstable API (see MSC2858)."""
-        return None
-
     @abc.abstractmethod
     async def handle_redirect_request(
         self,
diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py
index 4be502a77b..bcba106bdd 100644
--- a/synapse/rest/client/login.py
+++ b/synapse/rest/client/login.py
@@ -79,7 +79,6 @@ class LoginRestServlet(RestServlet):
         self.saml2_enabled = hs.config.saml2_enabled
         self.cas_enabled = hs.config.cas_enabled
         self.oidc_enabled = hs.config.oidc_enabled
-        self._msc2858_enabled = hs.config.experimental.msc2858_enabled
         self._msc2918_enabled = hs.config.access_token_lifetime is not None
 
         self.auth = hs.get_auth()
@@ -111,7 +110,7 @@ class LoginRestServlet(RestServlet):
         _load_sso_handlers(hs)
 
     def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
-        flows = []
+        flows: List[JsonDict] = []
         if self.jwt_enabled:
             flows.append({"type": LoginRestServlet.JWT_TYPE})
             flows.append({"type": LoginRestServlet.JWT_TYPE_DEPRECATED})
@@ -122,25 +121,15 @@ class LoginRestServlet(RestServlet):
             flows.append({"type": LoginRestServlet.CAS_TYPE})
 
         if self.cas_enabled or self.saml2_enabled or self.oidc_enabled:
-            sso_flow: JsonDict = {
-                "type": LoginRestServlet.SSO_TYPE,
-                "identity_providers": [
-                    _get_auth_flow_dict_for_idp(
-                        idp,
-                    )
-                    for idp in self._sso_handler.get_identity_providers().values()
-                ],
-            }
-
-            if self._msc2858_enabled:
-                # backwards-compatibility support for clients which don't
-                # support the stable API yet
-                sso_flow["org.matrix.msc2858.identity_providers"] = [
-                    _get_auth_flow_dict_for_idp(idp, use_unstable_brands=True)
-                    for idp in self._sso_handler.get_identity_providers().values()
-                ]
-
-            flows.append(sso_flow)
+            flows.append(
+                {
+                    "type": LoginRestServlet.SSO_TYPE,
+                    "identity_providers": [
+                        _get_auth_flow_dict_for_idp(idp)
+                        for idp in self._sso_handler.get_identity_providers().values()
+                    ],
+                }
+            )
 
             # While it's valid for us to advertise this login type generally,
             # synapse currently only gives out these tokens as part of the
@@ -433,9 +422,7 @@ class LoginRestServlet(RestServlet):
         return result
 
 
-def _get_auth_flow_dict_for_idp(
-    idp: SsoIdentityProvider, use_unstable_brands: bool = False
-) -> JsonDict:
+def _get_auth_flow_dict_for_idp(idp: SsoIdentityProvider) -> JsonDict:
     """Return an entry for the login flow dict
 
     Returns an entry suitable for inclusion in "identity_providers" in the
@@ -443,17 +430,12 @@ def _get_auth_flow_dict_for_idp(
 
     Args:
         idp: the identity provider to describe
-        use_unstable_brands: whether we should use brand identifiers suitable
-           for the unstable API
     """
     e: JsonDict = {"id": idp.idp_id, "name": idp.idp_name}
     if idp.idp_icon:
         e["icon"] = idp.idp_icon
     if idp.idp_brand:
         e["brand"] = idp.idp_brand
-    # use the stable brand identifier if the unstable identifier isn't defined.
-    if use_unstable_brands and idp.unstable_idp_brand:
-        e["brand"] = idp.unstable_idp_brand
     return e
 
 
@@ -504,25 +486,8 @@ class SsoRedirectServlet(RestServlet):
         # register themselves with the main SSOHandler.
         _load_sso_handlers(hs)
         self._sso_handler = hs.get_sso_handler()
-        self._msc2858_enabled = hs.config.experimental.msc2858_enabled
         self._public_baseurl = hs.config.public_baseurl
 
-    def register(self, http_server: HttpServer) -> None:
-        super().register(http_server)
-        if self._msc2858_enabled:
-            # expose additional endpoint for MSC2858 support: backwards-compat support
-            # for clients which don't yet support the stable endpoints.
-            http_server.register_paths(
-                "GET",
-                client_patterns(
-                    "/org.matrix.msc2858/login/sso/redirect/(?P<idp_id>[A-Za-z0-9_.~-]+)$",
-                    releases=(),
-                    unstable=True,
-                ),
-                self.on_GET,
-                self.__class__.__name__,
-            )
-
     async def on_GET(
         self, request: SynapseRequest, idp_id: Optional[str] = None
     ) -> None:
diff --git a/tests/rest/client/test_login.py b/tests/rest/client/test_login.py
index 5b2243fe52..f5c195a075 100644
--- a/tests/rest/client/test_login.py
+++ b/tests/rest/client/test_login.py
@@ -445,26 +445,9 @@ class MultiSSOTestCase(unittest.HomeserverTestCase):
             [f["type"] for f in channel.json_body["flows"]], expected_flow_types
         )
 
-    @override_config({"experimental_features": {"msc2858_enabled": True}})
-    def test_get_msc2858_login_flows(self):
-        """The SSO flow should include IdP info if MSC2858 is enabled"""
-        channel = self.make_request("GET", "/_matrix/client/r0/login")
-        self.assertEqual(channel.code, 200, channel.result)
-
-        # stick the flows results in a dict by type
-        flow_results: Dict[str, Any] = {}
-        for f in channel.json_body["flows"]:
-            flow_type = f["type"]
-            self.assertNotIn(
-                flow_type, flow_results, "duplicate flow type %s" % (flow_type,)
-            )
-            flow_results[flow_type] = f
-
-        self.assertIn("m.login.sso", flow_results, "m.login.sso was not returned")
-        sso_flow = flow_results.pop("m.login.sso")
-        # we should have a set of IdPs
+        flows = {flow["type"]: flow for flow in channel.json_body["flows"]}
         self.assertCountEqual(
-            sso_flow["org.matrix.msc2858.identity_providers"],
+            flows["m.login.sso"]["identity_providers"],
             [
                 {"id": "cas", "name": "CAS"},
                 {"id": "saml", "name": "SAML"},
@@ -473,19 +456,10 @@ class MultiSSOTestCase(unittest.HomeserverTestCase):
             ],
         )
 
-        # the rest of the flows are simple
-        expected_flows = [
-            {"type": "m.login.cas"},
-            {"type": "m.login.token"},
-            {"type": "m.login.password"},
-        ] + ADDITIONAL_LOGIN_FLOWS
-
-        self.assertCountEqual(flow_results.values(), expected_flows)
-
     def test_multi_sso_redirect(self):
         """/login/sso/redirect should redirect to an identity picker"""
         # first hit the redirect url, which should redirect to our idp picker
-        channel = self._make_sso_redirect_request(False, None)
+        channel = self._make_sso_redirect_request(None)
         self.assertEqual(channel.code, 302, channel.result)
         uri = channel.headers.getRawHeaders("Location")[0]
 
@@ -637,24 +611,13 @@ class MultiSSOTestCase(unittest.HomeserverTestCase):
 
     def test_client_idp_redirect_to_unknown(self):
         """If the client tries to pick an unknown IdP, return a 404"""
-        channel = self._make_sso_redirect_request(False, "xxx")
+        channel = self._make_sso_redirect_request("xxx")
         self.assertEqual(channel.code, 404, channel.result)
         self.assertEqual(channel.json_body["errcode"], "M_NOT_FOUND")
 
     def test_client_idp_redirect_to_oidc(self):
         """If the client pick a known IdP, redirect to it"""
-        channel = self._make_sso_redirect_request(False, "oidc")
-        self.assertEqual(channel.code, 302, channel.result)
-        oidc_uri = channel.headers.getRawHeaders("Location")[0]
-        oidc_uri_path, oidc_uri_query = oidc_uri.split("?", 1)
-
-        # it should redirect us to the auth page of the OIDC server
-        self.assertEqual(oidc_uri_path, TEST_OIDC_AUTH_ENDPOINT)
-
-    @override_config({"experimental_features": {"msc2858_enabled": True}})
-    def test_client_msc2858_redirect_to_oidc(self):
-        """Test the unstable API"""
-        channel = self._make_sso_redirect_request(True, "oidc")
+        channel = self._make_sso_redirect_request("oidc")
         self.assertEqual(channel.code, 302, channel.result)
         oidc_uri = channel.headers.getRawHeaders("Location")[0]
         oidc_uri_path, oidc_uri_query = oidc_uri.split("?", 1)
@@ -662,26 +625,12 @@ class MultiSSOTestCase(unittest.HomeserverTestCase):
         # it should redirect us to the auth page of the OIDC server
         self.assertEqual(oidc_uri_path, TEST_OIDC_AUTH_ENDPOINT)
 
-    def test_client_idp_redirect_msc2858_disabled(self):
-        """If the client tries to use the MSC2858 endpoint but MSC2858 is disabled, return a 400"""
-        channel = self._make_sso_redirect_request(True, "oidc")
-        self.assertEqual(channel.code, 400, channel.result)
-        self.assertEqual(channel.json_body["errcode"], "M_UNRECOGNIZED")
-
-    def _make_sso_redirect_request(
-        self, unstable_endpoint: bool = False, idp_prov: Optional[str] = None
-    ):
+    def _make_sso_redirect_request(self, idp_prov: Optional[str] = None):
         """Send a request to /_matrix/client/r0/login/sso/redirect
 
-        ... or the unstable equivalent
-
         ... possibly specifying an IDP provider
         """
-        endpoint = (
-            "/_matrix/client/unstable/org.matrix.msc2858/login/sso/redirect"
-            if unstable_endpoint
-            else "/_matrix/client/r0/login/sso/redirect"
-        )
+        endpoint = "/_matrix/client/r0/login/sso/redirect"
         if idp_prov is not None:
             endpoint += "/" + idp_prov
         endpoint += "?redirectUrl=" + urllib.parse.quote_plus(TEST_CLIENT_REDIRECT_URL)