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

Signed-off-by: Sean Quah <seanq@element.io>
This commit is contained in:
Sean 2021-09-09 17:59:59 +01:00 committed by GitHub
parent a621ba0259
commit 273b6861f2
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 19 additions and 127 deletions

View File

@ -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.

View File

@ -24,9 +24,6 @@ class ExperimentalConfig(Config):
def read_config(self, config: JsonDict, **kwargs): def read_config(self, config: JsonDict, **kwargs):
experimental = config.get("experimental_features") or {} experimental = config.get("experimental_features") or {}
# MSC2858 (multiple SSO identity providers)
self.msc2858_enabled: bool = experimental.get("msc2858_enabled", False)
# MSC3026 (busy presence state) # MSC3026 (busy presence state)
self.msc3026_enabled: bool = experimental.get("msc3026_enabled", False) self.msc3026_enabled: bool = experimental.get("msc3026_enabled", False)

View File

@ -277,12 +277,6 @@ OIDC_PROVIDER_CONFIG_SCHEMA = {
"maxLength": 255, "maxLength": 255,
"pattern": "^[a-z][a-z0-9_.-]*$", "pattern": "^[a-z][a-z0-9_.-]*$",
}, },
"idp_unstable_brand": {
"type": "string",
"minLength": 1,
"maxLength": 255,
"pattern": "^[a-z][a-z0-9_.-]*$",
},
"discover": {"type": "boolean"}, "discover": {"type": "boolean"},
"issuer": {"type": "string"}, "issuer": {"type": "string"},
"client_id": {"type": "string"}, "client_id": {"type": "string"},
@ -483,7 +477,6 @@ def _parse_oidc_config_dict(
idp_name=oidc_config.get("idp_name", "OIDC"), idp_name=oidc_config.get("idp_name", "OIDC"),
idp_icon=idp_icon, idp_icon=idp_icon,
idp_brand=oidc_config.get("idp_brand"), idp_brand=oidc_config.get("idp_brand"),
unstable_idp_brand=oidc_config.get("unstable_idp_brand"),
discover=oidc_config.get("discover", True), discover=oidc_config.get("discover", True),
issuer=oidc_config["issuer"], issuer=oidc_config["issuer"],
client_id=oidc_config["client_id"], client_id=oidc_config["client_id"],
@ -531,9 +524,6 @@ class OidcProviderConfig:
# Optional brand identifier for this IdP. # Optional brand identifier for this IdP.
idp_brand = attr.ib(type=Optional[str]) 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 # whether the OIDC discovery mechanism is used to discover endpoints
discover = attr.ib(type=bool) discover = attr.ib(type=bool)

View File

@ -82,7 +82,6 @@ class CasHandler:
# the SsoIdentityProvider protocol type. # the SsoIdentityProvider protocol type.
self.idp_icon = None self.idp_icon = None
self.idp_brand = None self.idp_brand = None
self.unstable_idp_brand = None
self._sso_handler = hs.get_sso_handler() self._sso_handler = hs.get_sso_handler()

View File

@ -338,9 +338,6 @@ class OidcProvider:
# optional brand identifier for this auth provider # optional brand identifier for this auth provider
self.idp_brand = provider.idp_brand 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 = hs.get_sso_handler()
self._sso_handler.register_identity_provider(self) self._sso_handler.register_identity_provider(self)

View File

@ -80,7 +80,6 @@ class SamlHandler(BaseHandler):
# the SsoIdentityProvider protocol type. # the SsoIdentityProvider protocol type.
self.idp_icon = None self.idp_icon = None
self.idp_brand = None self.idp_brand = None
self.unstable_idp_brand = None
# a map from saml session id to Saml2SessionData object # a map from saml session id to Saml2SessionData object
self._outstanding_requests_dict: Dict[str, Saml2SessionData] = {} self._outstanding_requests_dict: Dict[str, Saml2SessionData] = {}

View File

@ -104,11 +104,6 @@ class SsoIdentityProvider(Protocol):
"""Optional branding identifier""" """Optional branding identifier"""
return None return None
@property
def unstable_idp_brand(self) -> Optional[str]:
"""Optional brand identifier for the unstable API (see MSC2858)."""
return None
@abc.abstractmethod @abc.abstractmethod
async def handle_redirect_request( async def handle_redirect_request(
self, self,

View File

@ -79,7 +79,6 @@ class LoginRestServlet(RestServlet):
self.saml2_enabled = hs.config.saml2_enabled self.saml2_enabled = hs.config.saml2_enabled
self.cas_enabled = hs.config.cas_enabled self.cas_enabled = hs.config.cas_enabled
self.oidc_enabled = hs.config.oidc_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._msc2918_enabled = hs.config.access_token_lifetime is not None
self.auth = hs.get_auth() self.auth = hs.get_auth()
@ -111,7 +110,7 @@ class LoginRestServlet(RestServlet):
_load_sso_handlers(hs) _load_sso_handlers(hs)
def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
flows = [] flows: List[JsonDict] = []
if self.jwt_enabled: if self.jwt_enabled:
flows.append({"type": LoginRestServlet.JWT_TYPE}) flows.append({"type": LoginRestServlet.JWT_TYPE})
flows.append({"type": LoginRestServlet.JWT_TYPE_DEPRECATED}) flows.append({"type": LoginRestServlet.JWT_TYPE_DEPRECATED})
@ -122,25 +121,15 @@ class LoginRestServlet(RestServlet):
flows.append({"type": LoginRestServlet.CAS_TYPE}) flows.append({"type": LoginRestServlet.CAS_TYPE})
if self.cas_enabled or self.saml2_enabled or self.oidc_enabled: if self.cas_enabled or self.saml2_enabled or self.oidc_enabled:
sso_flow: JsonDict = { flows.append(
"type": LoginRestServlet.SSO_TYPE, {
"identity_providers": [ "type": LoginRestServlet.SSO_TYPE,
_get_auth_flow_dict_for_idp( "identity_providers": [
idp, _get_auth_flow_dict_for_idp(idp)
) for idp in self._sso_handler.get_identity_providers().values()
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)
# While it's valid for us to advertise this login type generally, # While it's valid for us to advertise this login type generally,
# synapse currently only gives out these tokens as part of the # synapse currently only gives out these tokens as part of the
@ -433,9 +422,7 @@ class LoginRestServlet(RestServlet):
return result return result
def _get_auth_flow_dict_for_idp( def _get_auth_flow_dict_for_idp(idp: SsoIdentityProvider) -> JsonDict:
idp: SsoIdentityProvider, use_unstable_brands: bool = False
) -> JsonDict:
"""Return an entry for the login flow dict """Return an entry for the login flow dict
Returns an entry suitable for inclusion in "identity_providers" in the Returns an entry suitable for inclusion in "identity_providers" in the
@ -443,17 +430,12 @@ def _get_auth_flow_dict_for_idp(
Args: Args:
idp: the identity provider to describe 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} e: JsonDict = {"id": idp.idp_id, "name": idp.idp_name}
if idp.idp_icon: if idp.idp_icon:
e["icon"] = idp.idp_icon e["icon"] = idp.idp_icon
if idp.idp_brand: if idp.idp_brand:
e["brand"] = 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 return e
@ -504,25 +486,8 @@ class SsoRedirectServlet(RestServlet):
# register themselves with the main SSOHandler. # register themselves with the main SSOHandler.
_load_sso_handlers(hs) _load_sso_handlers(hs)
self._sso_handler = hs.get_sso_handler() self._sso_handler = hs.get_sso_handler()
self._msc2858_enabled = hs.config.experimental.msc2858_enabled
self._public_baseurl = hs.config.public_baseurl 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( async def on_GET(
self, request: SynapseRequest, idp_id: Optional[str] = None self, request: SynapseRequest, idp_id: Optional[str] = None
) -> None: ) -> None:

View File

@ -445,26 +445,9 @@ class MultiSSOTestCase(unittest.HomeserverTestCase):
[f["type"] for f in channel.json_body["flows"]], expected_flow_types [f["type"] for f in channel.json_body["flows"]], expected_flow_types
) )
@override_config({"experimental_features": {"msc2858_enabled": True}}) flows = {flow["type"]: flow for flow in channel.json_body["flows"]}
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
self.assertCountEqual( self.assertCountEqual(
sso_flow["org.matrix.msc2858.identity_providers"], flows["m.login.sso"]["identity_providers"],
[ [
{"id": "cas", "name": "CAS"}, {"id": "cas", "name": "CAS"},
{"id": "saml", "name": "SAML"}, {"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): def test_multi_sso_redirect(self):
"""/login/sso/redirect should redirect to an identity picker""" """/login/sso/redirect should redirect to an identity picker"""
# first hit the redirect url, which should redirect to our idp 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) self.assertEqual(channel.code, 302, channel.result)
uri = channel.headers.getRawHeaders("Location")[0] uri = channel.headers.getRawHeaders("Location")[0]
@ -637,13 +611,13 @@ class MultiSSOTestCase(unittest.HomeserverTestCase):
def test_client_idp_redirect_to_unknown(self): def test_client_idp_redirect_to_unknown(self):
"""If the client tries to pick an unknown IdP, return a 404""" """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.code, 404, channel.result)
self.assertEqual(channel.json_body["errcode"], "M_NOT_FOUND") self.assertEqual(channel.json_body["errcode"], "M_NOT_FOUND")
def test_client_idp_redirect_to_oidc(self): def test_client_idp_redirect_to_oidc(self):
"""If the client pick a known IdP, redirect to it""" """If the client pick a known IdP, redirect to it"""
channel = self._make_sso_redirect_request(False, "oidc") channel = self._make_sso_redirect_request("oidc")
self.assertEqual(channel.code, 302, channel.result) self.assertEqual(channel.code, 302, channel.result)
oidc_uri = channel.headers.getRawHeaders("Location")[0] oidc_uri = channel.headers.getRawHeaders("Location")[0]
oidc_uri_path, oidc_uri_query = oidc_uri.split("?", 1) oidc_uri_path, oidc_uri_query = oidc_uri.split("?", 1)
@ -651,37 +625,12 @@ class MultiSSOTestCase(unittest.HomeserverTestCase):
# it should redirect us to the auth page of the OIDC server # it should redirect us to the auth page of the OIDC server
self.assertEqual(oidc_uri_path, TEST_OIDC_AUTH_ENDPOINT) self.assertEqual(oidc_uri_path, TEST_OIDC_AUTH_ENDPOINT)
@override_config({"experimental_features": {"msc2858_enabled": True}}) def _make_sso_redirect_request(self, idp_prov: Optional[str] = None):
def test_client_msc2858_redirect_to_oidc(self):
"""Test the unstable API"""
channel = self._make_sso_redirect_request(True, "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)
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
):
"""Send a request to /_matrix/client/r0/login/sso/redirect """Send a request to /_matrix/client/r0/login/sso/redirect
... or the unstable equivalent
... possibly specifying an IDP provider ... possibly specifying an IDP provider
""" """
endpoint = ( endpoint = "/_matrix/client/r0/login/sso/redirect"
"/_matrix/client/unstable/org.matrix.msc2858/login/sso/redirect"
if unstable_endpoint
else "/_matrix/client/r0/login/sso/redirect"
)
if idp_prov is not None: if idp_prov is not None:
endpoint += "/" + idp_prov endpoint += "/" + idp_prov
endpoint += "?redirectUrl=" + urllib.parse.quote_plus(TEST_CLIENT_REDIRECT_URL) endpoint += "?redirectUrl=" + urllib.parse.quote_plus(TEST_CLIENT_REDIRECT_URL)