From 3f55c1068506ca1f244fe65fdec56b7c3fe3b858 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Sat, 12 Nov 2022 21:44:02 +0100 Subject: [PATCH] saml: Rework SP-initiated logout config to support IdP-level config. This gives more flexibility on a server with multiple organizations and SAML IdPs. Such a server can have some organizations handled by IdPs with SLO set up, and some without it set up. In such a scenario, having a generic True/False server-wide setting is insufficient and instead being able to specify the IdPs/orgs for SLO is needed. --- docs/production/authentication-methods.md | 3 ++- zerver/tests/test_auth_backends.py | 8 ++----- zerver/views/auth.py | 29 ++++++++++++++++------- zproject/backends.py | 8 +++---- zproject/default_settings.py | 2 -- zproject/prod_settings_template.py | 6 ++--- zproject/settings_types.py | 1 + zproject/test_extra_settings.py | 1 + 8 files changed, 33 insertions(+), 25 deletions(-) diff --git a/docs/production/authentication-methods.md b/docs/production/authentication-methods.md index 1adcb21fc2..2b4e337aad 100644 --- a/docs/production/authentication-methods.md +++ b/docs/production/authentication-methods.md @@ -703,7 +703,8 @@ this with another IdP. #### SP-initiated Single Logout After configuring IdP-initiated Logout, you only need to set -`SAML_ENABLE_SP_INITIATED_SINGLE_LOGOUT = True` in +`"sp_initiated_logout_enabled": True` in the appropriate IdP +configuration dict in `SOCIAL_AUTH_SAML_ENABLED_IDPS` in `/etc/zulip/settings.py` to also enable SP-initiated Logout. When this is active, a user who logged in to Zulip via SAML, upon clicking "Logout" in the Zulip web app will be redirected to the IdP's Single diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index 9eea010f19..616ed9695e 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -2067,7 +2067,6 @@ class SAMLAuthBackendTest(SocialAuthBase): def get_account_data_dict(self, email: str, name: str) -> Dict[str, Any]: return dict(email=email, name=name) - @override_settings(SAML_ENABLE_SP_INITIATED_SINGLE_LOGOUT=True) def test_saml_sp_initiated_logout_success(self) -> None: hamlet = self.example_user("hamlet") @@ -2126,7 +2125,6 @@ class SAMLAuthBackendTest(SocialAuthBase): self.client_get(result["Location"]) self.assert_logged_in_user_id(None) - @override_settings(SAML_ENABLE_SP_INITIATED_SINGLE_LOGOUT=True) def test_saml_sp_initiated_logout_invalid_logoutresponse(self) -> None: hamlet = self.example_user("hamlet") self.login("hamlet") @@ -2148,14 +2146,13 @@ class SAMLAuthBackendTest(SocialAuthBase): ) self.assert_logged_in_user_id(hamlet.id) - @override_settings(SAML_ENABLE_SP_INITIATED_SINGLE_LOGOUT=True) def test_saml_sp_initiated_logout_endpoint_when_not_logged_in(self) -> None: self.assert_logged_in_user_id(None) result = self.client_post("/accounts/logout/") - self.assert_json_error(result, "Not logged in.") + self.assertEqual(result.status_code, 302) + self.assertEqual(result["Location"], "/accounts/login/") - @override_settings(SAML_ENABLE_SP_INITIATED_SINGLE_LOGOUT=True) def test_saml_sp_initiated_logout_logged_in_not_via_saml(self) -> None: """ If the user is logged in, but not via SAML, the normal logout flow @@ -2171,7 +2168,6 @@ class SAMLAuthBackendTest(SocialAuthBase): self.client_get(result["Location"]) self.assert_logged_in_user_id(None) - @override_settings(SAML_ENABLE_SP_INITIATED_SINGLE_LOGOUT=True) def test_saml_sp_initiated_logout_when_saml_not_enabled(self) -> None: """ If SAML is not enabled, the normal logout flow should be correctly executed. diff --git a/zerver/views/auth.py b/zerver/views/auth.py index 51518258bb..d286c9ed67 100644 --- a/zerver/views/auth.py +++ b/zerver/views/auth.py @@ -1118,24 +1118,35 @@ def json_fetch_api_key( logout_then_login = require_post(django_logout_then_login) -@require_post -def logout_view(request: HttpRequest, /, **kwargs: Any) -> HttpResponse: +def should_do_saml_sp_initiated_logout(request: HttpRequest) -> bool: realm = RequestNotes.get_notes(request).realm assert realm is not None - if not settings.SAML_ENABLE_SP_INITIATED_SINGLE_LOGOUT or not saml_auth_enabled(realm): - return logout_then_login(request, **kwargs) - if not request.user.is_authenticated: - raise JsonableError(_("Not logged in.")) + return False + + if not saml_auth_enabled(realm): + return False + + idp_name = SAMLSPInitiatedLogout.get_logged_in_user_idp(request) + if idp_name is None: + # This session wasn't authenticated via SAML, so proceed with normal logout process. + return False + + return settings.SOCIAL_AUTH_SAML_ENABLED_IDPS[idp_name].get( + "sp_initiated_logout_enabled", False + ) + + +@require_post +def logout_view(request: HttpRequest, /, **kwargs: Any) -> HttpResponse: + if not should_do_saml_sp_initiated_logout(request): + return logout_then_login(request, **kwargs) # This will first redirect to the IdP with a LogoutRequest and if successful on the IdP side, # the user will be redirected to our SAMLResponse-handling endpoint with a success LogoutResponse, # where we will finally terminate their session. result = SAMLSPInitiatedLogout.slo_request_to_idp(request, return_to=None) - if result is None: - # This session wasn't authenticated via SAML, so proceed with normal logout process. - return logout_then_login(request, **kwargs) return result diff --git a/zproject/backends.py b/zproject/backends.py index 2d83875aa4..d250859f96 100644 --- a/zproject/backends.py +++ b/zproject/backends.py @@ -2882,11 +2882,11 @@ class SAMLSPInitiatedLogout: @classmethod def slo_request_to_idp( cls, request: HttpRequest, return_to: Optional[str] = None - ) -> Optional[HttpResponse]: + ) -> HttpResponse: """ Generates the redirect to the IdP's SLO endpoint with - the appropriately generated LogoutRequest or None if the session - wasn't authenticated via SAML. + the appropriately generated LogoutRequest. This should only be called + on requests with a session that was indeed obtained via SAML. """ user_profile = request.user @@ -2900,7 +2900,7 @@ class SAMLSPInitiatedLogout: idp_name = cls.get_logged_in_user_idp(request) if idp_name is None: - return None + raise AssertionError("User not logged in via SAML") idp = saml_backend.get_idp(idp_name) auth = saml_backend._create_saml_auth(idp) diff --git a/zproject/default_settings.py b/zproject/default_settings.py index 2e591f30e2..170c6e83f0 100644 --- a/zproject/default_settings.py +++ b/zproject/default_settings.py @@ -92,8 +92,6 @@ SOCIAL_AUTH_SAML_SECURITY_CONFIG: Dict[str, Any] = {} # the limit_to_subdomains setting to be considered valid: SAML_REQUIRE_LIMIT_TO_SUBDOMAINS = False -SAML_ENABLE_SP_INITIATED_SINGLE_LOGOUT = False - # Historical name for SOCIAL_AUTH_GITHUB_KEY; still allowed in production. GOOGLE_OAUTH2_CLIENT_ID: Optional[str] = None diff --git a/zproject/prod_settings_template.py b/zproject/prod_settings_template.py index bc732eadd3..b4522f3dca 100644 --- a/zproject/prod_settings_template.py +++ b/zproject/prod_settings_template.py @@ -450,6 +450,9 @@ SOCIAL_AUTH_SAML_ENABLED_IDPS: Dict[str, Any] = { ## default, Zulip asks the user whether they want to create an ## account or try to log in again using another method. # "auto_signup": False, + ## Determines whether Service Provider initiated SAML Single Logout should be enabled. + ## Note that IdP-initiated Single Logout must be configured before enabling this. + # "sp_initiated_logout_enabled": False, }, } @@ -494,9 +497,6 @@ SOCIAL_AUTH_SAML_SUPPORT_CONTACT = { # } # } -# This setting allows enabling of SP-initiated logout with SAML. -# SAML_ENABLE_SP_INITIATED_SINGLE_LOGOUT = True - ######## ## Apple authentication ("Sign in with Apple"). ## diff --git a/zproject/settings_types.py b/zproject/settings_types.py index cfd4314669..39cc271a6b 100644 --- a/zproject/settings_types.py +++ b/zproject/settings_types.py @@ -12,6 +12,7 @@ class SAMLIdPConfigDict(TypedDict, total=False): entity_id: str url: str slo_url: str + sp_initiated_logout_enabled: bool attr_user_permanent_id: str attr_first_name: str attr_last_name: str diff --git a/zproject/test_extra_settings.py b/zproject/test_extra_settings.py index cbea88b7d6..14e8e97f65 100644 --- a/zproject/test_extra_settings.py +++ b/zproject/test_extra_settings.py @@ -245,6 +245,7 @@ SOCIAL_AUTH_SAML_ENABLED_IDPS: Dict[str, SAMLIdPConfigDict] = { "entity_id": "https://idp.testshib.org/idp/shibboleth", "url": "https://idp.testshib.org/idp/profile/SAML2/Redirect/SSO", "slo_url": "https://idp.testshib.org/idp/profile/SAML2/Redirect/Logout", + "sp_initiated_logout_enabled": True, "x509cert": get_from_file_if_exists("zerver/tests/fixtures/saml/idp.crt"), "attr_user_permanent_id": "email", "attr_first_name": "first_name",