From b66dc9de5069ef1ebf2fae4e1bbb7e9f25d76bb2 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Sat, 23 May 2020 15:21:19 +0200 Subject: [PATCH] saml: Support IdP-initiated SSO. --- docs/production/authentication-methods.md | 18 +++ zerver/tests/test_auth_backends.py | 154 ++++++++++++++++++---- zproject/backends.py | 80 +++++++---- 3 files changed, 205 insertions(+), 47 deletions(-) diff --git a/docs/production/authentication-methods.md b/docs/production/authentication-methods.md index 1e6af7f995..a53e0d0de9 100644 --- a/docs/production/authentication-methods.md +++ b/docs/production/authentication-methods.md @@ -135,6 +135,24 @@ found at `https://yourzulipdomain.example.com/saml/metadata.xml`. You can use this for verifying your configuration or provide it to your IdP. +### IdP-initiated SSO + +The above configuration is sufficient for Service Provider initialized +SSO, i.e. you can visit the Zulip webapp and click "Sign in with +{IdP}" and it'll correctly start the authentication flow. If you are +not hosting multiple organizations, with Zulip 2.2+, the above +configuration is also sufficient for Identity Provider initiated SSO, +i.e. clicking a "Sign in to Zulip" button on the IdP's website can +correctly authenticate the user to Zulip. + +If you're hosting multiple organizations and thus using the +`SOCIAL_AUTH_SUBDOMAIN` setting, you'll need to configure a custom +`RelayState` in your IdP of the form `{"subdomain": +"yourzuliporganization"}` to let Zulip know which organization to +authenticate the user to when they visit your SSO URL from the IdP. +(If the organization is on the root domain, use the empty string: +`{"subdomain": ""}`.). + ```eval_rst .. _ldap: ``` diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index 91b74249b9..6d638192c4 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -1424,7 +1424,7 @@ class SAMLAuthBackendTest(SocialAuthBase): parsed_url = urllib.parse.urlparse(result.url) relay_state = urllib.parse.parse_qs(parsed_url.query)['RelayState'][0] # Make sure params are getting encoded into RelayState: - data = SAMLAuthBackend.get_data_from_redis(relay_state) + data = SAMLAuthBackend.get_data_from_redis(ujson.loads(relay_state)['state_token']) if next: self.assertEqual(data['next'], next) if is_signup: @@ -1512,7 +1512,9 @@ class SAMLAuthBackendTest(SocialAuthBase): mock.patch('zproject.backends.logging.info') as m: # This mock causes AuthFailed to be raised. saml_response = self.generate_saml_response(self.email, self.name) - relay_state = SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"}) + relay_state = ujson.dumps(dict( + state_token=SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"}) + )) post_params = {"SAMLResponse": saml_response, "RelayState": relay_state} result = self.client_post('/complete/saml/', post_params) self.assertEqual(result.status_code, 302) @@ -1525,7 +1527,9 @@ class SAMLAuthBackendTest(SocialAuthBase): side_effect=AuthStateForbidden('State forbidden')), \ mock.patch('zproject.backends.logging.warning') as m: saml_response = self.generate_saml_response(self.email, self.name) - relay_state = SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"}) + relay_state = ujson.dumps(dict( + state_token=SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"}) + )) post_params = {"SAMLResponse": saml_response, "RelayState": relay_state} result = self.client_post('/complete/saml/', post_params) self.assertEqual(result.status_code, 302) @@ -1540,12 +1544,14 @@ class SAMLAuthBackendTest(SocialAuthBase): result = self.client_get('/complete/saml/') self.assertEqual(result.status_code, 302) self.assertIn('login', result.url) - m.assert_called_with("SAML authentication failed: missing RelayState.") + m.assert_called_with("/complete/saml/: No SAMLResponse in request.") # Check that POSTing the RelayState, but with missing SAMLResponse, # doesn't cause errors either: with mock.patch('zproject.backends.logging.info') as m: - relay_state = SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"}) + relay_state = ujson.dumps(dict( + state_token=SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"}) + )) post_params = {"RelayState": relay_state} result = self.client_post('/complete/saml/', post_params) self.assertEqual(result.status_code, 302) @@ -1554,7 +1560,9 @@ class SAMLAuthBackendTest(SocialAuthBase): # Now test bad SAMLResponses. with mock.patch('zproject.backends.logging.info') as m: - relay_state = SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"}) + relay_state = ujson.dumps(dict( + state_token=SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"}) + )) post_params = {"RelayState": relay_state, 'SAMLResponse': ''} result = self.client_post('/complete/saml/', post_params) self.assertEqual(result.status_code, 302) @@ -1562,7 +1570,9 @@ class SAMLAuthBackendTest(SocialAuthBase): m.assert_called() with mock.patch('zproject.backends.logging.info') as m: - relay_state = SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"}) + relay_state = ujson.dumps(dict( + state_token=SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"}) + )) post_params = {"RelayState": relay_state, 'SAMLResponse': 'b'} result = self.client_post('/complete/saml/', post_params) self.assertEqual(result.status_code, 302) @@ -1570,36 +1580,31 @@ class SAMLAuthBackendTest(SocialAuthBase): m.assert_called() with mock.patch('zproject.backends.logging.info') as m: - relay_state = SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"}) + relay_state = ujson.dumps(dict( + state_token=SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"}) + )) post_params = {"RelayState": relay_state, 'SAMLResponse': 'dGVzdA=='} # base64 encoded 'test' result = self.client_post('/complete/saml/', post_params) self.assertEqual(result.status_code, 302) self.assertIn('login', result.url) m.assert_called() - with mock.patch('zproject.backends.logging.info') as m: - relay_state = SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"}) - relay_state = relay_state[:-1] # Break the token by removing the last character - post_params = {"RelayState": relay_state} - result = self.client_post('/complete/saml/', post_params) - self.assertEqual(result.status_code, 302) - self.assertIn('login', result.url) - m.assert_called_with("SAML authentication failed: bad RelayState token.") - def test_social_auth_complete_no_subdomain(self) -> None: with mock.patch('zproject.backends.logging.info') as m: - relay_state = SAMLAuthBackend.put_data_in_redis({}) - post_params = {"RelayState": relay_state, + post_params = {"RelayState": '', 'SAMLResponse': self.generate_saml_response(email=self.example_email("hamlet"), name="King Hamlet")} - result = self.client_post('/complete/saml/', post_params) + with mock.patch.object(SAMLAuthBackend, 'choose_subdomain', return_value=None): + result = self.client_post('/complete/saml/', post_params) self.assertEqual(result.status_code, 302) self.assertEqual('/login/', result.url) - self.assertIn("/complete/saml/: Missing subdomain value in relayed_params.", + self.assertIn("/complete/saml/: Can't figure out subdomain for this authentication request", m.call_args.args[0]) def test_social_auth_complete_wrong_issuing_idp(self) -> None: - relay_state = SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"}) + relay_state = ujson.dumps(dict( + state_token=SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"}) + )) saml_response = self.generate_saml_response(email=self.example_email("hamlet"), name="King Hamlet") @@ -1626,7 +1631,9 @@ class SAMLAuthBackendTest(SocialAuthBase): with mock.patch('zproject.backends.logging.info') as m, \ mock.patch.object(SAMLAuthBackend, 'get_issuing_idp', return_value='test_idp'): - relay_state = SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"}) + relay_state = ujson.dumps(dict( + state_token=SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"}) + )) post_params = {"RelayState": relay_state, 'SAMLResponse': 'dGVzdA=='} result = self.client_post('/complete/saml/', post_params) self.assertEqual(result.status_code, 302) @@ -1720,7 +1727,8 @@ class SAMLAuthBackendTest(SocialAuthBase): self.assertEqual(result.status_code, 302) self.assertEqual('/login/', result.url) m.assert_called_with( - "User authenticated with IdP %s but this provider is not enabled for this realm %s.", + '/complete/saml/: Authentication request with IdP %s but this provider is not enabled ' + + 'for this subdomain %s.', "test_idp", "zephyr", ) @@ -1761,6 +1769,104 @@ class SAMLAuthBackendTest(SocialAuthBase): "IdPs don't have limit_to_subdomains specified and will be ignored: " + "['test_idp']") + def test_idp_initiated_signin_subdomain_specified(self) -> None: + post_params = { + "RelayState": '{"subdomain": "zulip"}', + "SAMLResponse": self.generate_saml_response(email=self.email, name=self.name) + } + + with mock.patch.object(OneLogin_Saml2_Response, 'is_valid', return_value=True): + # We're not able to generate valid signatures in tests, so we need the mock. + result = self.client_post('/complete/saml/', post_params) + + data = load_subdomain_token(result) + self.assertEqual(data['email'], self.example_email("hamlet")) + self.assertEqual(data['full_name'], self.name) + self.assertEqual(data['subdomain'], 'zulip') + self.assertEqual(result.status_code, 302) + parsed_url = urllib.parse.urlparse(result.url) + uri = "{}://{}{}".format(parsed_url.scheme, parsed_url.netloc, + parsed_url.path) + self.assertTrue(uri.startswith('http://zulip.testserver/accounts/login/subdomain/')) + + self.client_get(uri) + self.assert_logged_in_user_id(self.example_user("hamlet").id) + + def test_choose_subdomain_invalid_subdomain_specified(self) -> None: + post_params = { + "RelayState": '{"subdomain": "invalid"}', + "SAMLResponse": self.generate_saml_response(email=self.email, name=self.name) + } + + with mock.patch.object(OneLogin_Saml2_Response, 'is_valid', return_value=True): + # We're not able to generate valid signatures in tests, so we need the mock. + result = self.client_post('/complete/saml/', post_params) + + self.assertEqual(result.status_code, 302) + self.assertEqual(result.url, "/accounts/find/") + + def test_idp_initiated_signin_subdomain_implicit(self) -> None: + post_params = { + "RelayState": '', + "SAMLResponse": self.generate_saml_response(email=self.email, name=self.name) + } + + with mock.patch.object(OneLogin_Saml2_Response, 'is_valid', return_value=True): + # We're not able to generate valid signatures in tests, so we need the mock. + result = self.client_post('http://zulip.testserver/complete/saml/', post_params) + + data = load_subdomain_token(result) + self.assertEqual(data['email'], self.example_email("hamlet")) + self.assertEqual(data['full_name'], self.name) + self.assertEqual(data['subdomain'], 'zulip') + self.assertEqual(result.status_code, 302) + parsed_url = urllib.parse.urlparse(result.url) + uri = "{}://{}{}".format(parsed_url.scheme, parsed_url.netloc, + parsed_url.path) + self.assertTrue(uri.startswith('http://zulip.testserver/accounts/login/subdomain/')) + + self.client_get(uri) + self.assert_logged_in_user_id(self.example_user("hamlet").id) + + def test_idp_initiated_signin_subdomain_implicit_no_relaystate_param(self) -> None: + post_params = { + "SAMLResponse": self.generate_saml_response(email=self.email, name=self.name) + } + + with mock.patch.object(OneLogin_Saml2_Response, 'is_valid', return_value=True): + # We're not able to generate valid signatures in tests, so we need the mock. + result = self.client_post('http://zulip.testserver/complete/saml/', post_params) + + data = load_subdomain_token(result) + self.assertEqual(data['email'], self.example_email("hamlet")) + self.assertEqual(data['full_name'], self.name) + self.assertEqual(data['subdomain'], 'zulip') + self.assertEqual(result.status_code, 302) + parsed_url = urllib.parse.urlparse(result.url) + uri = "{}://{}{}".format(parsed_url.scheme, parsed_url.netloc, + parsed_url.path) + self.assertTrue(uri.startswith('http://zulip.testserver/accounts/login/subdomain/')) + + self.client_get(uri) + self.assert_logged_in_user_id(self.example_user("hamlet").id) + + def test_idp_initiated_signin_subdomain_implicit_invalid(self) -> None: + post_params = { + "RelayState": '', + "SAMLResponse": self.generate_saml_response(email=self.email, name=self.name) + } + + with mock.patch("logging.info") as m: + with mock.patch('zproject.backends.get_subdomain', return_value='invalid'): + # Due to the quirks of our test setup, get_subdomain on all these `some_subdomain.testserver` + # requests returns 'zulip', so we need to mock it here. + result = self.client_post('http://invalid.testserver/complete/saml/', post_params) + + self.assertEqual(result.status_code, 302) + self.assertEqual('/login/', result.url) + self.assertIn("/complete/saml/: Can't figure out subdomain for this authentication request", + m.call_args.args[0]) + class GitHubAuthBackendTest(SocialAuthBase): __unittest_skip__ = False diff --git a/zproject/backends.py b/zproject/backends.py index 5aed5e8cb8..d77e3654c5 100644 --- a/zproject/backends.py +++ b/zproject/backends.py @@ -16,6 +16,7 @@ import binascii import copy import logging import magic +import ujson from abc import ABC, abstractmethod from typing import Any, Callable, Dict, List, Optional, Set, Tuple, Type, TypeVar, Union, \ cast @@ -64,6 +65,7 @@ from zerver.lib.rate_limiter import RateLimitedObject from zerver.lib.request import JsonableError from zerver.lib.users import check_full_name, validate_user_custom_profile_field from zerver.lib.redis_utils import get_redis_client, get_dict_from_redis, put_dict_in_redis +from zerver.lib.subdomains import get_subdomain from zerver.models import CustomProfileField, DisposableEmailError, DomainNotAllowedForRealmError, \ EmailContainsPlusError, PreregistrationUser, UserProfile, Realm, custom_profile_fields_for_realm, \ get_user_profile_by_id, remote_user_to_email, \ @@ -1549,7 +1551,7 @@ class SAMLAuthBackend(SocialAuthMixin, SAMLAuth): data_to_relay = { key: request_data[key] for key in params_to_relay if key in request_data } - relay_state = self.put_data_in_redis(data_to_relay) + relay_state = ujson.dumps({"state_token": self.put_data_in_redis(data_to_relay)}) return auth.login(return_to=relay_state) @@ -1565,10 +1567,6 @@ class SAMLAuthBackend(SocialAuthMixin, SAMLAuth): if key.startswith('saml_token_'): # Safety if statement, to not allow someone to poke around arbitrary redis keys here. data = get_dict_from_redis(redis_client, "saml_token_{token}", key) - if data is None: - # TODO: We will need some sort of user-facing message - # about the authentication session having expired here. - logging.info("SAML authentication failed: bad RelayState token.") return data @@ -1596,6 +1594,46 @@ class SAMLAuthBackend(SocialAuthMixin, SAMLAuth): return None + def get_relayed_params(self) -> Dict[str, Any]: + request_data = self.strategy.request_data() + if 'RelayState' not in request_data: + return {} + + relay_state = request_data['RelayState'] + try: + data = ujson.loads(relay_state) + if 'state_token' in data: + # SP-initiated sign in. We stored relevant information in the first + # step of the flow + return self.get_data_from_redis(data['state_token']) or {} + else: + # IdP-initiated sign in. Right now we only support transporting subdomain through json in + # RelayState, but this format is nice in that it allows easy extensibility here. + return {'subdomain': data.get('subdomain')} + except (ValueError, TypeError): + return {} + + def choose_subdomain(self, relayed_params: Dict[str, Any]) -> Optional[str]: + subdomain = relayed_params.get("subdomain") + if subdomain is not None: + return subdomain + + # If not specified otherwise, the intended subdomain for this + # authentication attempt is the subdomain of the request. + request_subdomain = get_subdomain(self.strategy.request) + try: + # We only want to do a basic sanity-check here for whether + # this subdomain has a realm one could try to authenticate + # to. True validation of whether the realm is active, the + # IdP is appropriate for the subdomain, etc. happens + # elsewhere in the flow and we shouldn't duplicate such + # logic here. + get_realm(request_subdomain) + except Realm.DoesNotExist: + return None + else: + return request_subdomain + def auth_complete(self, *args: Any, **kwargs: Any) -> Optional[HttpResponse]: """ Additional ugly wrapping on top of auth_complete in SocialAuthMixin. @@ -1610,41 +1648,37 @@ class SAMLAuthBackend(SocialAuthMixin, SAMLAuth): and then change the RelayState param to the idp_name, because that's what SAMLAuth.auth_complete() expects. """ - if 'RelayState' not in self.strategy.request_data(): - logging.info("SAML authentication failed: missing RelayState.") - return None - - # Set the relevant params that we transported in the RelayState: - redis_key = self.strategy.request_data()['RelayState'] - relayed_params = self.get_data_from_redis(redis_key) - if relayed_params is None: - return None - SAMLResponse = self.strategy.request_data().get('SAMLResponse') if SAMLResponse is None: logging.info("/complete/saml/: No SAMLResponse in request.") return None + relayed_params = self.get_relayed_params() + + subdomain = self.choose_subdomain(relayed_params) + if subdomain is None: + error_msg = "/complete/saml/: Can't figure out subdomain for this authentication request. " + \ + "relayed_params: %s" + logging.info(error_msg, relayed_params) + return None + idp_name = self.get_issuing_idp(SAMLResponse) if idp_name is None: logging.info("/complete/saml/: No valid IdP as issuer of the SAMLResponse.") return None - subdomain = relayed_params.get("subdomain") - if subdomain is None: - logging.info("/complete/saml/: Missing subdomain value in relayed_params.") - return None - idp_valid = self.validate_idp_for_subdomain(idp_name, subdomain) if not idp_valid: - error_msg = "User authenticated with IdP %s but this provider is not " + \ - "enabled for this realm %s." + error_msg = "/complete/saml/: Authentication request with IdP %s but this provider is not " + \ + "enabled for this subdomain %s." logging.info(error_msg, idp_name, subdomain) return None result = None try: - for param, value in relayed_params.items(): + params = relayed_params.copy() + params['subdomain'] = subdomain + for param, value in params.items(): if param in self.standard_relay_params: self.strategy.session_set(param, value)