saml: Make SP-initiated SLO work in the desktop application.

docs/production/authentication-methods.md

@@ -712,8 +712,9 @@ Logout endpoint with a `LogoutRequest`. If a successful
 `LogoutResponse` is received back, their current Zulip session will be
 terminated.
 
-Note that this doesn't work in the case of desktop and mobile application
+Note that this doesn't work when logging out of the mobile application
-and is reserved to the browser.
+since the app doesn't use sessions and relies on just having the user's
+API key.
 
 #### Caveats
 

zerver/tests/test_auth_backends.py

@@ -2125,6 +2125,34 @@ class SAMLAuthBackendTest(SocialAuthBase):
         self.client_get(result["Location"])
         self.assert_logged_in_user_id(None)
 
+    def test_saml_sp_initiated_logout_desktop_flow(self) -> None:
+        desktop_flow_otp = "1234abcd" * 8
+        account_data_dict = self.get_account_data_dict(email=self.email, name=self.name)
+        with self.assertLogs(self.logger_string, level="INFO"):
+            result = self.social_auth_test(
+                account_data_dict,
+                subdomain="zulip",
+                expect_choose_email_screen=False,
+                desktop_flow_otp=desktop_flow_otp,
+            )
+            # Flush the session to have a clean slate - since up till now
+            # we were simulating the part of the flow that happens in the browser.
+            # Now we will simulating the last part of the flow, which gets executed
+            # in the desktop application - thus with a separate session.
+            self.client.session.flush()
+            self.verify_desktop_flow_end_page(result, self.email, desktop_flow_otp)
+
+        # 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
+        # session in the app.
+        result = self.client_post("/accounts/logout/")
+        # A redirect to the IdP is returned.
+        self.assertEqual(result.status_code, 302)
+        self.assertIn(
+            settings.SOCIAL_AUTH_SAML_ENABLED_IDPS["test_idp"]["slo_url"], result["Location"]
+        )
+
     def test_saml_sp_initiated_logout_invalid_logoutresponse(self) -> None:
         hamlet = self.example_user("hamlet")
         self.login("hamlet")

zerver/views/auth.py

@@ -347,7 +347,17 @@ def login_or_register_remote_user(request: HttpRequest, result: ExternalAuthResu
       are doing authentication using the mobile_flow_otp or desktop_flow_otp flow.
     """
 
-    for key, value in result.data_dict.get("params_to_store_in_authenticated_session", {}).items():
+    params_to_store_in_authenticated_session = result.data_dict.get(
+        "params_to_store_in_authenticated_session", {}
+    )
+    mobile_flow_otp = result.data_dict.get("mobile_flow_otp")
+    desktop_flow_otp = result.data_dict.get("desktop_flow_otp")
+    if not mobile_flow_otp and not desktop_flow_otp:
+        # We don't want to store anything in the browser session if we're doing
+        # mobile or desktop flows, since that's just an intermediary step and the
+        # browser session is not to be used any further. Storing extra data in
+        # it just risks bugs or leaking the data.
+        for key, value in params_to_store_in_authenticated_session.items():
             request.session[key] = value
 
     user_profile = result.user_profile
@@ -357,12 +367,12 @@ def login_or_register_remote_user(request: HttpRequest, result: ExternalAuthResu
     # account, and we need to do the right thing depending whether
     # or not they're using the mobile OTP flow or want a browser session.
     is_realm_creation = result.data_dict.get("is_realm_creation")
-    mobile_flow_otp = result.data_dict.get("mobile_flow_otp")
-    desktop_flow_otp = result.data_dict.get("desktop_flow_otp")
     if mobile_flow_otp is not None:
         return finish_mobile_flow(request, user_profile, mobile_flow_otp)
     elif desktop_flow_otp is not None:
-        return finish_desktop_flow(request, user_profile, desktop_flow_otp)
+        return finish_desktop_flow(
+            request, user_profile, desktop_flow_otp, params_to_store_in_authenticated_session
+        )
 
     do_login(request, user_profile)
 
@@ -377,7 +387,12 @@ def login_or_register_remote_user(request: HttpRequest, result: ExternalAuthResu
     return HttpResponseRedirect(redirect_to)
 
 
-def finish_desktop_flow(request: HttpRequest, user_profile: UserProfile, otp: str) -> HttpResponse:
+def finish_desktop_flow(
+    request: HttpRequest,
+    user_profile: UserProfile,
+    otp: str,
+    params_to_store_in_authenticated_session: Optional[Dict[str, str]] = None,
+) -> HttpResponse:
     """
     The desktop otp flow returns to the app (through the clipboard)
     a token that allows obtaining (through log_into_subdomain) a logged in session
@@ -386,7 +401,14 @@ def finish_desktop_flow(request: HttpRequest, user_profile: UserProfile, otp: st
     of being created, as nothing more powerful is needed for the desktop flow
     and this ensures the key can only be used for completing this authentication attempt.
     """
-    result = ExternalAuthResult(user_profile=user_profile)
+    data_dict = None
+    if params_to_store_in_authenticated_session:
+        data_dict = ExternalAuthDataDict(
+            params_to_store_in_authenticated_session=params_to_store_in_authenticated_session
+        )
+
+    result = ExternalAuthResult(user_profile=user_profile, data_dict=data_dict)
+
     token = result.store_data()
     key = bytes.fromhex(otp)
     iv = secrets.token_bytes(12)

zproject/backends.py

@@ -1306,10 +1306,8 @@ class ExternalAuthDataDict(TypedDict, total=False):
     desktop_flow_otp: Optional[str]
     multiuse_object_key: str
     full_name_validated: bool
-    # TODO: This currently does not get plumbed through to the final session
+    # The mobile app doesn't actually use a session, so this
-    # in the desktop flow, but it should.
+    # data is not applicable there.
-    # Also, the mobile app doesn't actually use a session, so this
-    # data is not applicable there either.
     params_to_store_in_authenticated_session: Dict[str, str]