From fe11a2bf4596ea138b77b14540076fccf6d1512e Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Wed, 25 Aug 2021 14:03:18 +0100 Subject: [PATCH 1/6] Remove unstable MSC2858 API and `experimental.msc2858_enabled` option Signed-off-by: Sean Quah --- synapse/config/experimental.py | 3 -- synapse/rest/client/login.py | 50 ++++++++-------------------------- 2 files changed, 11 insertions(+), 42 deletions(-) diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index 95deda11a5d6..7b0381c06a27 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/rest/client/login.py b/synapse/rest/client/login.py index 11d07776b2ff..53b5f548390f 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -24,7 +24,7 @@ from synapse.appservice import ApplicationService from synapse.handlers.sso import SsoIdentityProvider from synapse.http import get_request_uri -from synapse.http.server import HttpServer, finish_request +from synapse.http.server import finish_request from synapse.http.servlet import ( RestServlet, assert_params_in_dict, @@ -79,7 +79,6 @@ def __init__(self, hs: "HomeServer"): 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 @@ def __init__(self, hs: "HomeServer"): _load_sso_handlers(hs) def on_GET(self, request: SynapseRequest): - 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 @@ def on_GET(self, request: SynapseRequest): 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 @@ -507,25 +496,8 @@ def __init__(self, hs: "HomeServer"): # 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[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: From a3d6b800b7fe3f764973871a73cc84a14a87db20 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Wed, 25 Aug 2021 14:05:06 +0100 Subject: [PATCH 2/6] Remove tests for unstable MSC2858 API Also move check for MSC2858 IdP info into the main get_login_flows test. Signed-off-by: Sean Quah --- tests/rest/client/test_login.py | 65 ++++----------------------------- 1 file changed, 7 insertions(+), 58 deletions(-) diff --git a/tests/rest/client/test_login.py b/tests/rest/client/test_login.py index 5b2243fe5205..f5c195a075bd 100644 --- a/tests/rest/client/test_login.py +++ b/tests/rest/client/test_login.py @@ -445,26 +445,9 @@ def test_get_login_flows(self): [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 @@ def test_get_msc2858_login_flows(self): ], ) - # 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 @@ def test_multi_sso_redirect_to_unknown(self): 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 @@ def test_client_msc2858_redirect_to_oidc(self): # 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) From 550498f81cdecaa59f87e958f5ff5d5b2dfbeb59 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Wed, 25 Aug 2021 14:10:25 +0100 Subject: [PATCH 3/6] Remove now unused `unstable_idp_brand` OIDC config option Signed-off-by: Sean Quah --- synapse/config/oidc.py | 4 ---- synapse/handlers/cas.py | 1 - synapse/handlers/oidc.py | 3 --- synapse/handlers/saml.py | 1 - synapse/handlers/sso.py | 5 ----- synapse/rest/client/login.py | 9 +-------- 6 files changed, 1 insertion(+), 22 deletions(-) diff --git a/synapse/config/oidc.py b/synapse/config/oidc.py index ba89d11cf016..7eabf315ae76 100644 --- a/synapse/config/oidc.py +++ b/synapse/config/oidc.py @@ -483,7 +483,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 +530,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 0325f86e20a1..47ddabbe464a 100644 --- a/synapse/handlers/cas.py +++ b/synapse/handlers/cas.py @@ -82,7 +82,6 @@ def __init__(self, hs: "HomeServer"): # 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 eca8f1604001..648fcf76f83f 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -338,9 +338,6 @@ def __init__( # 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 e6e71e972964..0066d570c5fe 100644 --- a/synapse/handlers/saml.py +++ b/synapse/handlers/saml.py @@ -80,7 +80,6 @@ def __init__(self, hs: "HomeServer"): # 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 0e6ebb574ecf..0fdc6dd9e789 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -104,11 +104,6 @@ def idp_brand(self) -> Optional[str]: """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 53b5f548390f..6cebf33f3149 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -422,9 +422,7 @@ async def _do_jwt_login( 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 @@ -432,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 From 3d33da8ce70d585c7279c0ed8d32a2ebb2881ff0 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Wed, 25 Aug 2021 14:56:38 +0100 Subject: [PATCH 4/6] Add newsfragment Signed-off-by: Sean Quah --- changelog.d/10693.removal | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10693.removal diff --git a/changelog.d/10693.removal b/changelog.d/10693.removal new file mode 100644 index 000000000000..c8f270e662dc --- /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 haven't already done so. From 9e4f8979dc9d8bee922fbc3f50d58324e9cdbaed Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Wed, 25 Aug 2021 16:50:04 +0100 Subject: [PATCH 5/6] Update newsfragment Signed-off-by: Sean Quah --- changelog.d/10693.removal | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/10693.removal b/changelog.d/10693.removal index c8f270e662dc..0255a8be742a 100644 --- a/changelog.d/10693.removal +++ b/changelog.d/10693.removal @@ -1 +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 haven't already done so. +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. From b09cbded2486d2eff25746b47456031eda6caba7 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Thu, 26 Aug 2021 17:59:00 +0100 Subject: [PATCH 6/6] Remove misnamed `idp_unstable_brand` from `OIDC_PROVIDER_CONFIG_SCHEMA` Signed-off-by: Sean Quah --- synapse/config/oidc.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/synapse/config/oidc.py b/synapse/config/oidc.py index 7eabf315ae76..7e67fbada18f 100644 --- a/synapse/config/oidc.py +++ b/synapse/config/oidc.py @@ -277,12 +277,6 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): "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"},