diff --git a/docs/production/authentication-methods.md b/docs/production/authentication-methods.md index 2b4e337aad..82b1247be8 100644 --- a/docs/production/authentication-methods.md +++ b/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 -and is reserved to the browser. +Note that this doesn't work when logging out of the mobile application +since the app doesn't use sessions and relies on just having the user's +API key. #### Caveats diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index 616ed9695e..c13d55f9a5 100644 --- a/zerver/tests/test_auth_backends.py +++ b/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") diff --git a/zerver/views/auth.py b/zerver/views/auth.py index d286c9ed67..635ecc4c34 100644 --- a/zerver/views/auth.py +++ b/zerver/views/auth.py @@ -347,8 +347,18 @@ 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(): - request.session[key] = value + 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 if user_profile is None or user_profile.is_mirror_dummy: @@ -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) diff --git a/zproject/backends.py b/zproject/backends.py index d250859f96..722dbd856d 100644 --- a/zproject/backends.py +++ b/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 - # in the desktop flow, but it should. - # Also, the mobile app doesn't actually use a session, so this - # data is not applicable there either. + # The mobile app doesn't actually use a session, so this + # data is not applicable there. params_to_store_in_authenticated_session: Dict[str, str]