From d51eb95ef6538f3fdf47ef745404ec7c2c19b017 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 16 Dec 2020 16:42:20 -0500 Subject: [PATCH 1/7] Refactor how CAS responses are parsed. --- synapse/handlers/cas_handler.py | 165 +++++++++++++++++++------------- 1 file changed, 101 insertions(+), 64 deletions(-) diff --git a/synapse/handlers/cas_handler.py b/synapse/handlers/cas_handler.py index f4ea0a976749..5273858ad902 100644 --- a/synapse/handlers/cas_handler.py +++ b/synapse/handlers/cas_handler.py @@ -13,13 +13,15 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging -import urllib -from typing import TYPE_CHECKING, Dict, Optional, Tuple +import urllib.parse +from typing import TYPE_CHECKING, Dict, Optional from xml.etree import ElementTree as ET +import attr + from twisted.web.client import PartialDownloadError -from synapse.api.errors import Codes, LoginError +from synapse.api.errors import Codes, HttpResponseException, LoginError from synapse.http.site import SynapseRequest from synapse.types import UserID, map_username_to_mxid_localpart @@ -29,6 +31,26 @@ logger = logging.getLogger(__name__) +class CasError(Exception): + """Used to catch errors when validating the CAS ticket. + """ + + def __init__(self, error, error_description=None): + self.error = error + self.error_description = error_description + + def __str__(self): + if self.error_description: + return "{}: {}".format(self.error, self.error_description) + return self.error + + +@attr.s(slots=True, frozen=True) +class CasResponse: + username = attr.ib(type=str) + attributes = attr.ib(type=Dict[str, Optional[str]]) + + class CasHandler: """ Utility class for to handle the response from a CAS SSO service. @@ -69,14 +91,20 @@ def _build_service_param(self, args: Dict[str, str]) -> str: async def _validate_ticket( self, ticket: str, service_args: Dict[str, str] - ) -> Tuple[str, Optional[str]]: + ) -> CasResponse: """ - Validate a CAS ticket with the server, parse the response, and return the user and display name. + Validate a CAS ticket with the server, and return the parsed the response. Args: ticket: The CAS ticket from the client. service_args: Additional arguments to include in the service URL. Should be the same as those passed to `get_redirect_url`. + + Raises: + CasError: If there's an error parsing the CAS response. + + Returns: + The parsed CAS response. """ uri = self._cas_server_url + "/proxyValidate" args = { @@ -89,66 +117,65 @@ async def _validate_ticket( # Twisted raises this error if the connection is closed, # even if that's being used old-http style to signal end-of-data body = pde.response + except HttpResponseException as e: + description = ( + ( + 'Authorization server responded with a "{status}" error ' + "while exchanging the authorization code." + ).format(status=e.code), + ) + raise CasError("server_error", description) - user, attributes = self._parse_cas_response(body) - displayname = attributes.pop(self._cas_displayname_attribute, None) - - for required_attribute, required_value in self._cas_required_attributes.items(): - # If required attribute was not in CAS Response - Forbidden - if required_attribute not in attributes: - raise LoginError(401, "Unauthorized", errcode=Codes.UNAUTHORIZED) - - # Also need to check value - if required_value is not None: - actual_value = attributes[required_attribute] - # If required attribute value does not match expected - Forbidden - if required_value != actual_value: - raise LoginError(401, "Unauthorized", errcode=Codes.UNAUTHORIZED) - - return user, displayname + return self._parse_cas_response(body) - def _parse_cas_response( - self, cas_response_body: bytes - ) -> Tuple[str, Dict[str, Optional[str]]]: + def _parse_cas_response(self, cas_response_body: bytes) -> CasResponse: """ Retrieve the user and other parameters from the CAS response. Args: cas_response_body: The response from the CAS query. + Raises: + CasError: If there's an error parsing the CAS response. + Returns: - A tuple of the user and a mapping of other attributes. + The parsed CAS response. """ + + # Ensure the response is valid. + root = ET.fromstring(cas_response_body) + if not root.tag.endswith("serviceResponse"): + raise CasError( + "missing_service_response", + "root of CAS response is not serviceResponse", + ) + + success = root[0].tag.endswith("authenticationSuccess") + if not success: + raise CasError("unsucessful_response", "Unsuccessful CAS response") + + # Iterate through the nodes and pull out the user and any extra attributes. user = None attributes = {} - try: - root = ET.fromstring(cas_response_body) - if not root.tag.endswith("serviceResponse"): - raise Exception("root of CAS response is not serviceResponse") - success = root[0].tag.endswith("authenticationSuccess") - for child in root[0]: - if child.tag.endswith("user"): - user = child.text - if child.tag.endswith("attributes"): - for attribute in child: - # ElementTree library expands the namespace in - # attribute tags to the full URL of the namespace. - # We don't care about namespace here and it will always - # be encased in curly braces, so we remove them. - tag = attribute.tag - if "}" in tag: - tag = tag.split("}")[1] - attributes[tag] = attribute.text - if user is None: - raise Exception("CAS response does not contain user") - except Exception: - logger.exception("Error parsing CAS response") - raise LoginError(401, "Invalid CAS response", errcode=Codes.UNAUTHORIZED) - if not success: - raise LoginError( - 401, "Unsuccessful CAS response", errcode=Codes.UNAUTHORIZED - ) - return user, attributes + for child in root[0]: + if child.tag.endswith("user"): + user = child.text + if child.tag.endswith("attributes"): + for attribute in child: + # ElementTree library expands the namespace in + # attribute tags to the full URL of the namespace. + # We don't care about namespace here and it will always + # be encased in curly braces, so we remove them. + tag = attribute.tag + if "}" in tag: + tag = tag.split("}")[1] + attributes[tag] = attribute.text + + # Ensure a user was found. + if user is None: + raise CasError("no_user", "CAS response does not contain user") + + return CasResponse(user, attributes) def get_redirect_url(self, service_args: Dict[str, str]) -> str: """ @@ -201,7 +228,20 @@ async def handle_ticket( args["redirectUrl"] = client_redirect_url if session: args["session"] = session - username, user_display_name = await self._validate_ticket(ticket, args) + + cas_response = await self._validate_ticket(ticket, args) + + for required_attribute, required_value in self._cas_required_attributes.items(): + # If required attribute was not in CAS Response - Forbidden + if required_attribute not in cas_response.attributes: + raise LoginError(401, "Unauthorized", errcode=Codes.UNAUTHORIZED) + + # Also need to check value + if required_value is not None: + actual_value = cas_response.attributes[required_attribute] + # If required attribute value does not match expected - Forbidden + if required_value != actual_value: + raise LoginError(401, "Unauthorized", errcode=Codes.UNAUTHORIZED) # Pull out the user-agent and IP from the request. user_agent = request.get_user_agent("") @@ -209,7 +249,7 @@ async def handle_ticket( # Get the matrix ID from the CAS username. user_id = await self._map_cas_user_to_matrix_user( - username, user_display_name, user_agent, ip_address + cas_response, user_agent, ip_address ) if session: @@ -225,18 +265,13 @@ async def handle_ticket( ) async def _map_cas_user_to_matrix_user( - self, - remote_user_id: str, - display_name: Optional[str], - user_agent: str, - ip_address: str, + self, cas_response: CasResponse, user_agent: str, ip_address: str, ) -> str: """ Given a CAS username, retrieve the user ID for it and possibly register the user. Args: - remote_user_id: The username from the CAS response. - display_name: The display name from the CAS response. + cas_response: The parsed CAS response. user_agent: The user agent of the client making the request. ip_address: The IP address of the client making the request. @@ -244,15 +279,17 @@ async def _map_cas_user_to_matrix_user( The user ID associated with this response. """ - localpart = map_username_to_mxid_localpart(remote_user_id) + localpart = map_username_to_mxid_localpart(cas_response.username) user_id = UserID(localpart, self._hostname).to_string() registered_user_id = await self._auth_handler.check_user_exists(user_id) + displayname = cas_response.attributes.get(self._cas_displayname_attribute, None) + # If the user does not exist, register it. if not registered_user_id: registered_user_id = await self._registration_handler.register_user( localpart=localpart, - default_display_name=display_name, + default_display_name=displayname, user_agent_ips=[(user_agent, ip_address)], ) From 62f805c7617712b37651cf63cb5971bef8909585 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 16 Dec 2020 16:47:51 -0500 Subject: [PATCH 2/7] Render errors instead of raising exceptions. --- synapse/handlers/cas_handler.py | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/cas_handler.py b/synapse/handlers/cas_handler.py index 5273858ad902..7062f5fdb719 100644 --- a/synapse/handlers/cas_handler.py +++ b/synapse/handlers/cas_handler.py @@ -21,7 +21,7 @@ from twisted.web.client import PartialDownloadError -from synapse.api.errors import Codes, HttpResponseException, LoginError +from synapse.api.errors import HttpResponseException from synapse.http.site import SynapseRequest from synapse.types import UserID, map_username_to_mxid_localpart @@ -72,6 +72,8 @@ def __init__(self, hs: "HomeServer"): self._http_client = hs.get_proxied_http_client() + self._sso_handler = hs.get_sso_handler() + def _build_service_param(self, args: Dict[str, str]) -> str: """ Generates a value to use as the "service" parameter when redirecting or @@ -229,19 +231,32 @@ async def handle_ticket( if session: args["session"] = session - cas_response = await self._validate_ticket(ticket, args) + try: + cas_response = await self._validate_ticket(ticket, args) + except CasError as e: + logger.exception("Could not validate ticket") + self._sso_handler.render_error(request, e.error, e.error_description) + return for required_attribute, required_value in self._cas_required_attributes.items(): # If required attribute was not in CAS Response - Forbidden if required_attribute not in cas_response.attributes: - raise LoginError(401, "Unauthorized", errcode=Codes.UNAUTHORIZED) + self._sso_handler.render_error( + request, "unauthorised", "You are not authorised to log in here." + ) + return # Also need to check value if required_value is not None: actual_value = cas_response.attributes[required_attribute] # If required attribute value does not match expected - Forbidden if required_value != actual_value: - raise LoginError(401, "Unauthorized", errcode=Codes.UNAUTHORIZED) + self._sso_handler.render_error( + request, + "unauthorised", + "You are not authorised to log in here.", + ) + return # Pull out the user-agent and IP from the request. user_agent = request.get_user_agent("") From 5dfc99a884a2a8803faaf16cb3ac119458425438 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 16 Dec 2020 17:19:23 -0500 Subject: [PATCH 3/7] Refactor CAS handler to be structured more like OIDC/SAML handlers. --- synapse/handlers/cas_handler.py | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/synapse/handlers/cas_handler.py b/synapse/handlers/cas_handler.py index 7062f5fdb719..0ff37e8655a1 100644 --- a/synapse/handlers/cas_handler.py +++ b/synapse/handlers/cas_handler.py @@ -238,6 +238,37 @@ async def handle_ticket( self._sso_handler.render_error(request, e.error, e.error_description) return + await self._handle_cas_response( + request, cas_response, client_redirect_url, session + ) + + async def _handle_cas_response( + self, + request: SynapseRequest, + cas_response: CasResponse, + client_redirect_url: Optional[str], + session: Optional[str], + ) -> None: + """Handle a CAS response to a ticket request. + + Assumes that the response has been validated. Maps the user onto an MXID, + registering them if necessary, and returns a response to the browser. + + Args: + request: the incoming request from the browser. We'll respond to it with an + HTML page or a redirect + + cas_response: The parsed CAS response. + + client_redirect_url: the redirectUrl parameter from the `/cas/ticket` HTTP request, if given. + This should be the same as the redirectUrl from the original `/login/sso/redirect` request. + + session: The session parameter from the `/cas/ticket` HTTP request, if given. + This should be the UI Auth session id. + """ + + # Ensure that the attributes of the logged in user meet the required + # attributes. for required_attribute, required_value in self._cas_required_attributes.items(): # If required attribute was not in CAS Response - Forbidden if required_attribute not in cas_response.attributes: From d15f021e8b99333a9f0b43e08d16654c3c960b46 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 16 Dec 2020 17:32:03 -0500 Subject: [PATCH 4/7] Add a changelog. --- changelog.d/8958.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/8958.misc diff --git a/changelog.d/8958.misc b/changelog.d/8958.misc new file mode 100644 index 000000000000..1507073e4f52 --- /dev/null +++ b/changelog.d/8958.misc @@ -0,0 +1 @@ +Properly store the mapping of external ID to Matrix ID for CAS users. From ed106af100c12ab924b2dd2997d0205251334af9 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 18 Dec 2020 09:27:28 -0500 Subject: [PATCH 5/7] Raise from. --- synapse/handlers/cas_handler.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/cas_handler.py b/synapse/handlers/cas_handler.py index 0ff37e8655a1..e0446a67baa7 100644 --- a/synapse/handlers/cas_handler.py +++ b/synapse/handlers/cas_handler.py @@ -126,7 +126,7 @@ async def _validate_ticket( "while exchanging the authorization code." ).format(status=e.code), ) - raise CasError("server_error", description) + raise CasError("server_error", description) from e return self._parse_cas_response(body) From 9c1eaeb699af63c3a135aa8750e336bc90344ee8 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 18 Dec 2020 10:44:58 -0500 Subject: [PATCH 6/7] Return 401 errors. --- synapse/handlers/cas_handler.py | 8 ++++++-- synapse/handlers/sso.py | 9 +++++++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/cas_handler.py b/synapse/handlers/cas_handler.py index e0446a67baa7..e9891e131613 100644 --- a/synapse/handlers/cas_handler.py +++ b/synapse/handlers/cas_handler.py @@ -235,7 +235,7 @@ async def handle_ticket( cas_response = await self._validate_ticket(ticket, args) except CasError as e: logger.exception("Could not validate ticket") - self._sso_handler.render_error(request, e.error, e.error_description) + self._sso_handler.render_error(request, e.error, e.error_description, 401) return await self._handle_cas_response( @@ -273,7 +273,10 @@ async def _handle_cas_response( # If required attribute was not in CAS Response - Forbidden if required_attribute not in cas_response.attributes: self._sso_handler.render_error( - request, "unauthorised", "You are not authorised to log in here." + request, + "unauthorised", + "You are not authorised to log in here.", + 401, ) return @@ -286,6 +289,7 @@ async def _handle_cas_response( request, "unauthorised", "You are not authorised to log in here.", + 401, ) return diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index f054b66a53da..f9a40bfd8518 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -60,7 +60,11 @@ def __init__(self, hs: "HomeServer"): self._mapping_lock = Linearizer(name="sso_user_mapping", clock=hs.get_clock()) def render_error( - self, request, error: str, error_description: Optional[str] = None + self, + request: Request, + error: str, + error_description: Optional[str] = None, + code: int = 400, ) -> None: """Renders the error template and responds with it. @@ -72,11 +76,12 @@ def render_error( We'll respond with an HTML page describing the error. error: A technical identifier for this error. error_description: A human-readable description of the error. + code: The integer error code (an HTTP response code) """ html = self._error_template.render( error=error, error_description=error_description ) - respond_with_html(request, 400, html) + respond_with_html(request, code, html) async def get_sso_user_by_remote_user_id( self, auth_provider_id: str, remote_user_id: str From 4903547414bac83f93ffef59007abbd4202b8b7b Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 18 Dec 2020 10:54:23 -0500 Subject: [PATCH 7/7] Update docs. --- docs/dev/cas.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/dev/cas.md b/docs/dev/cas.md index f8d02cc82ca9..592b2d8d4fc0 100644 --- a/docs/dev/cas.md +++ b/docs/dev/cas.md @@ -31,7 +31,7 @@ easy to run CAS implementation built on top of Django. You should now have a Django project configured to serve CAS authentication with a single user created. -## Configure Synapse (and Riot) to use CAS +## Configure Synapse (and Element) to use CAS 1. Modify your `homeserver.yaml` to enable CAS and point it to your locally running Django test server: @@ -51,9 +51,9 @@ and that the CAS server is on port 8000, both on localhost. ## Testing the configuration -Then in Riot: +Then in Element: -1. Visit the login page with a Riot pointing at your homeserver. +1. Visit the login page with a Element pointing at your homeserver. 2. Click the Single Sign-On button. 3. Login using the credentials created with `createsuperuser`. 4. You should be logged in.