diff --git a/docs/production/authentication-methods.md b/docs/production/authentication-methods.md index 82b1247be8..420204b4e2 100644 --- a/docs/production/authentication-methods.md +++ b/docs/production/authentication-methods.md @@ -721,9 +721,6 @@ API key. - This implementation doesn't support using `SessionIndex` to limit which sessions are affected; in IdP-initiated Logout it always terminates all logged-in sessions for the user identified in the `NameID`. - In SP-initiated Logout this simply means that Zulip does not include - `SessionIndex` in the `LogoutRequest` to the IdP - however, that doesn't - seem to cause any undesired behavior with Keycloak. - SAML Logout in a configuration where your IdP handles authentication for multiple organizations is not yet supported. diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index c13d55f9a5..faf4e8d6cb 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -1990,7 +1990,11 @@ class SAMLAuthBackendTest(SocialAuthBase): return result def generate_saml_response( - self, email: str, name: str, extra_attributes: Mapping[str, List[str]] = {} + self, + email: str, + name: str, + extra_attributes: Mapping[str, List[str]] = {}, + include_session_index: bool = True, ) -> str: """ The samlresponse.txt fixture has a pre-generated SAMLResponse, @@ -2025,6 +2029,14 @@ class SAMLAuthBackendTest(SocialAuthBase): last_name=last_name, extra_attrs=extra_attrs, ) + + if not include_session_index: + # The SessionIndex value is hard-coded in the fixture, so we can remove it + # in a very hacky, but easy way, without needing to write regexes or parsing XML. + unencoded_saml_response = unencoded_saml_response.replace( + ' SessionIndex="ONELOGIN_a5fde8b09598814d7af2537f865d31c2f7aea831"', "" + ) + # SAMLResponse needs to be base64-encoded. saml_response: str = base64.b64encode(unencoded_saml_response.encode()).decode() @@ -2084,6 +2096,11 @@ class SAMLAuthBackendTest(SocialAuthBase): self.assert_logged_in_user_id(hamlet.id) + # Obtain the SessionIndex value saved in the session, for ensuring it's sent in + # the LogoutRequest later. + session_index = self.client.session["saml_session_index"] + self.assertNotEqual(session_index, None) + result = self.client_post("/accounts/logout/") # A redirect to the IdP is returned. self.assertEqual(result.status_code, 302) @@ -2100,6 +2117,7 @@ class SAMLAuthBackendTest(SocialAuthBase): saml_request = OneLogin_Saml2_Utils.decode_base64_and_inflate(saml_request_encoded).decode() self.assertIn("{hamlet.delivery_email}", saml_request) + self.assertIn(f"{session_index}", saml_request) unencoded_logout_response = self.fixture_data("logoutresponse.txt", type="saml") logout_response: str = base64.b64encode(unencoded_logout_response.encode()).decode() @@ -2142,6 +2160,10 @@ class SAMLAuthBackendTest(SocialAuthBase): self.client.session.flush() self.verify_desktop_flow_end_page(result, self.email, desktop_flow_otp) + # Ensure the SessionIndex gets plumbed through to the final session in the app. + session_index = self.client.session["saml_session_index"] + self.assertNotEqual(session_index, None) + # Now we have a desktop app logged in session. Verify that the logout # request will trigger the SAML SLO flow - it should if we correctly # plumbed through the information about the SAML authentication to the @@ -2584,6 +2606,39 @@ class SAMLAuthBackendTest(SocialAuthBase): ], ) + def test_social_auth_complete_samlresponse_without_session_index_logging(self) -> None: + """ + Tests submitting a valid SAMLResponse to /complete/saml/ without the SessionIndex attribute, + which is optional - to verify we log that unusual situation appropriately. + """ + + # We don't need to test full flow, so just generate the data to POST to /complete/saml/ + # as the last step of the SAML flow. + relay_state = orjson.dumps( + dict( + state_token=SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"}), + ) + ).decode() + saml_response = self.generate_saml_response( + email=self.example_email("hamlet"), name="King Hamlet", include_session_index=False + ) + + with self.assertLogs(self.logger_string, level="INFO") as m: + post_params = {"RelayState": relay_state, "SAMLResponse": saml_response} + with mock.patch.object(OneLogin_Saml2_Response, "is_valid", return_value=True): + result = self.client_post("/complete/saml/", post_params) + # Redirect to /accounts/login/subdomain/ indicating auth success. + self.assertEqual(result.status_code, 302) + self.assertIn("/accounts/login/subdomain/", result["Location"]) + + # Verify the expected logline exists. + self.assertIn( + self.logger_output( + "/complete/saml/: IdP did not provide SessionIndex in the SAMLResponse.", "info" + ), + m.output, + ) + def test_social_auth_complete_wrong_issuing_idp(self) -> None: relay_state = orjson.dumps( dict( @@ -2636,7 +2691,7 @@ class SAMLAuthBackendTest(SocialAuthBase): self.assertEqual(result.status_code, 302) self.assertIn("login", result["Location"]) - self.assert_length(m.output, 1) + self.assert_length(m.output, 3) def test_social_auth_saml_bad_idp_param_on_login_page(self) -> None: with self.assertLogs(self.logger_string, level="INFO") as m: diff --git a/zproject/backends.py b/zproject/backends.py index 722dbd856d..35ca613600 100644 --- a/zproject/backends.py +++ b/zproject/backends.py @@ -2358,6 +2358,22 @@ class SAMLResponse(SAMLDocument): self.logger.error("Error parsing SAMLResponse: %s", str(e)) return [] + def get_session_index(self) -> Optional[str]: + """ + Returns the SessionIndex from the SAMLResponse. + """ + config = self.backend.generate_saml_config() + saml_settings = OneLogin_Saml2_Settings(config, sp_validation_only=True) + + try: + resp = OneLogin_Saml2_Response( + settings=saml_settings, response=self.encoded_saml_message + ) + return resp.get_session_index() + except self.SAML_PARSING_EXCEPTIONS as e: + self.logger.error("Error parsing SAMLResponse: %s", str(e)) + return None + def is_logout_response(self) -> bool: """ Checks whether the SAMLResponse is a LogoutResponse based on some @@ -2676,6 +2692,8 @@ class SAMLAuthBackend(SocialAuthMixin, SAMLAuth): # or an authentication response. return SAMLSPInitiatedLogout.process_logout_response(saml_document, idp_name) + assert isinstance(saml_document, SAMLResponse) + result = None try: params = relayed_params.copy() @@ -2686,6 +2704,14 @@ class SAMLAuthBackend(SocialAuthMixin, SAMLAuth): # We want the IdP name to be accessible from the social pipeline. self.strategy.session_set("saml_idp_name", idp_name) + session_index = saml_document.get_session_index() + if session_index is None: + # In general IdPs will always provide a SessionIndex, but we can't know + # if some providers might not send it, so we allow it but log the event. + self.logger.info( + "/complete/saml/: IdP did not provide SessionIndex in the SAMLResponse." + ) + self.strategy.session_set("saml_session_index", session_index) # super().auth_complete expects to have RelayState set to the idp_name, # so we need to replace this param. @@ -2769,8 +2795,12 @@ class SAMLAuthBackend(SocialAuthMixin, SAMLAuth): def get_params_to_store_in_authenticated_session(self) -> Dict[str, str]: idp_name = self.strategy.session_get("saml_idp_name") + saml_session_index = self.strategy.session_get("saml_session_index") - return {"social_auth_backend": f"saml:{idp_name}"} + return { + "social_auth_backend": f"saml:{idp_name}", + "saml_session_index": saml_session_index, + } def patch_saml_auth_require_messages_signed(auth: OneLogin_Saml2_Auth) -> None: @@ -2877,6 +2907,20 @@ class SAMLSPInitiatedLogout: return authentication_method.split("saml:")[1] + @classmethod + def get_logged_in_user_session_index(cls, request: HttpRequest) -> Optional[str]: + """ + During SAML authentication, we obtain the SessionIndex value provided + by the IdP and save it in the session. This function can be used + to retrieve it. + """ + # Some asserts to ensure this doesn't get called incorrectly: + assert hasattr(request, "user") + assert isinstance(request.user, UserProfile) + + session_index = request.session.get("saml_session_index") + return session_index + @classmethod def slo_request_to_idp( cls, request: HttpRequest, return_to: Optional[str] = None @@ -2899,10 +2943,13 @@ class SAMLSPInitiatedLogout: idp_name = cls.get_logged_in_user_idp(request) if idp_name is None: raise AssertionError("User not logged in via SAML") + session_index = cls.get_logged_in_user_session_index(request) idp = saml_backend.get_idp(idp_name) auth = saml_backend._create_saml_auth(idp) - slo_url = auth.logout(name_id=user_profile.delivery_email, return_to=return_to) + slo_url = auth.logout( + name_id=user_profile.delivery_email, return_to=return_to, session_index=session_index + ) return HttpResponseRedirect(slo_url)