saml: Save SessionIndex in session and use when making a LogoutRequest.

This is a useful improvement in general for making correct
LogoutRequests to Idps and a necessary one to make SP-initiated logout
fully work properly in the desktop application. During desktop auth
flow, the user goes through the browser, where they log in through their
IdP. This gives them a logged in  browser session at the IdP. However,
SAML SP-initiated logout is fully conducted within the desktop
application. This means that proper information needs to be given to the
the IdP in the LogoutRequest to let it associate the LogoutRequest with
that logged in session that was established in the browser. SessionIndex
is exactly the tool for that in the SAML spec.

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.
 

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("<samlp:LogoutRequest", saml_request)
         self.assertIn(f"saml:NameID>{hamlet.delivery_email}</saml:NameID>", saml_request)
+        self.assertIn(f"<samlp:SessionIndex>{session_index}</samlp:SessionIndex>", 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:

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)