From 892d25faa13de3dd571d1b6c72de938c6a7ca424 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Tue, 22 Oct 2019 18:23:57 +0200 Subject: [PATCH] auth: Change SAML login url scheme, enabling multiple IdP support. The url scheme is now /accounts/login/social/saml/{idp_name} to initiate login using the IdP configured under "idp_name" name. display_name and display_logo (the name and icon to show on the "Log in with" button) can be customized by adding the apprioprate settings in the configured IdP dictionaries. --- zerver/tests/test_auth_backends.py | 70 +++++++++++++++++++++++++----- zerver/views/auth.py | 26 +++++------ zproject/backends.py | 20 ++++++++- zproject/prod_settings_template.py | 5 +++ zproject/test_settings.py | 2 + zproject/urls.py | 5 +++ 6 files changed, 102 insertions(+), 26 deletions(-) diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index d711a67cfb..6fc6f5b2a4 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -70,6 +70,7 @@ from social_django.strategy import DjangoStrategy from social_django.storage import BaseDjangoStorage import base64 +import copy import json import urllib import ujson @@ -958,8 +959,8 @@ class SAMLAuthBackendTest(SocialAuthBase): __unittest_skip__ = False BACKEND_CLASS = SAMLAuthBackend - LOGIN_URL = "/accounts/login/social/saml" - SIGNUP_URL = "/accounts/register/social/saml" + LOGIN_URL = "/accounts/login/social/saml/test_idp" + SIGNUP_URL = "/accounts/register/social/saml/test_idp" AUTHORIZATION_URL = "https://idp.testshib.org/idp/profile/SAML2/Redirect/SSO" AUTH_FINISH_URL = "/complete/saml/" CONFIG_ERROR_URL = "/config-error/saml" @@ -1153,16 +1154,61 @@ class SAMLAuthBackendTest(SocialAuthBase): should lead to misconfiguration page. """ - with self.settings(SOCIAL_AUTH_SAML_ENABLED_IDPS={"test_idp1": {}, "test_idp2": {}}): - # We don't need to put full idp configurations in the mock settings above - # to trigger the error. - with mock.patch("zerver.views.auth.logging.error") as mock_error: - result = self.client_get("/accounts/login/social/saml") - self.assertEqual(result.status_code, 302) - self.assertEqual(result.url, '/config-error/saml') - mock_error.assert_called_once_with( - "SAML misconfigured - you have specified multiple IdPs. Only one IdP is supported." - ) + # Setup a new SOCIAL_AUTH_SAML_ENABLED_IDPS dict with two idps. + # We deepcopy() dictionaries around for the sake of brevity, + # to avoid having to spell them out explicitly here. + # The second idp's configuration is a copy of the first one, + # with name test_idp2 and altered url. + idps_dict = copy.deepcopy(settings.SOCIAL_AUTH_SAML_ENABLED_IDPS) + idps_dict['test_idp2'] = copy.deepcopy(idps_dict['test_idp']) + idps_dict['test_idp2']['url'] = 'https://idp2.example.com/idp/profile/SAML2/Redirect/SSO' + idps_dict['test_idp2']['display_name'] = 'Second Test IdP' + + # Run tests with multiple idps configured: + with self.settings(SOCIAL_AUTH_SAML_ENABLED_IDPS=idps_dict): + # Go to the login page and check that buttons to log in show up for both IdPs: + result = self.client_get('/accounts/login/') + self.assert_in_success_response(["Log in with Test IdP"], result) + self.assert_in_success_response(["/accounts/login/social/saml/test_idp"], result) + self.assert_in_success_response(["Log in with Second Test IdP"], result) + self.assert_in_success_response(["/accounts/login/social/saml/test_idp2"], result) + + # Try succesful authentication with the regular idp from all previous tests: + self.test_social_auth_success() + + # Now test with the second idp: + original_LOGIN_URL = self.LOGIN_URL + original_SIGNUP_URL = self.SIGNUP_URL + original_AUTHORIZATION_URL = self.AUTHORIZATION_URL + self.LOGIN_URL = "/accounts/login/social/saml/test_idp2" + self.SIGNUP_URL = "/accounts/register/social/saml/test_idp2" + self.AUTHORIZATION_URL = idps_dict['test_idp2']['url'] + + try: + self.test_social_auth_success() + finally: + # Restore original values at the end, regardless of what happens + # in the block above, to avoid affecting other tests in unpredictable + # ways. + self.LOGIN_URL = original_LOGIN_URL + self.SIGNUP_URL = original_SIGNUP_URL + self.AUTHORIZATION_URL = original_AUTHORIZATION_URL + + def test_social_auth_saml_login_bad_idp_arg(self) -> None: + for action in ['login', 'register']: + result = self.client_get('/accounts/{}/social/saml'.format(action)) + # Missing idp argument. + self.assertEqual(result.status_code, 302) + self.assertEqual(result.url, '/config-error/saml') + + result = self.client_get('/accounts/{}/social/saml/nonexistent_idp'.format(action)) + # No such IdP is configured. + self.assertEqual(result.status_code, 302) + self.assertEqual(result.url, '/config-error/saml') + + result = self.client_get('/accounts/{}/social/saml/'.format(action)) + # No matching url pattern. + self.assertEqual(result.status_code, 404) class GitHubAuthBackendTest(SocialAuthBase): __unittest_skip__ = False diff --git a/zerver/views/auth.py b/zerver/views/auth.py index c98c7aba12..b44d1c51c9 100644 --- a/zerver/views/auth.py +++ b/zerver/views/auth.py @@ -350,7 +350,8 @@ def oauth_redirect_to_root(request: HttpRequest, url: str, return redirect(main_site_uri + '?' + urllib.parse.urlencode(params)) -def start_social_login(request: HttpRequest, backend: str) -> HttpResponse: +def start_social_login(request: HttpRequest, backend: str, extra_arg: Optional[str]=None + ) -> HttpResponse: backend_url = reverse('social:begin', args=[backend]) extra_url_params = {} # type: Dict[str, str] if backend == "saml": @@ -366,16 +367,11 @@ def start_social_login(request: HttpRequest, backend: str) -> HttpResponse: # This backend requires the name of the IdP (from the list of configured ones) # to be passed as the parameter. - # Currently we support configuring only one IdP. - # TODO: Support multiple IdPs. python-social-auth SAML (which we use here) - # already supports that, so essentially only the UI for it on the login pages - # needs to be figured out. - if len(settings.SOCIAL_AUTH_SAML_ENABLED_IDPS) != 1: - logging.error( - "SAML misconfigured - you have specified multiple IdPs. Only one IdP is supported." - ) + if not extra_arg or extra_arg not in settings.SOCIAL_AUTH_SAML_ENABLED_IDPS: + logging.info("Attempted to initiate SAML authentication with wrong idp argument: {}" + .format(extra_arg)) return redirect_to_config_error("saml") - extra_url_params = {'idp': list(settings.SOCIAL_AUTH_SAML_ENABLED_IDPS.keys())[0]} + extra_url_params = {'idp': extra_arg} if (backend == "github") and not (settings.SOCIAL_AUTH_GITHUB_KEY and settings.SOCIAL_AUTH_GITHUB_SECRET): return redirect_to_config_error("github") @@ -386,12 +382,16 @@ def start_social_login(request: HttpRequest, backend: str) -> HttpResponse: return oauth_redirect_to_root(request, backend_url, 'social', extra_url_params=extra_url_params) -def start_social_signup(request: HttpRequest, backend: str) -> HttpResponse: +def start_social_signup(request: HttpRequest, backend: str, extra_arg: Optional[str]=None + ) -> HttpResponse: backend_url = reverse('social:begin', args=[backend]) extra_url_params = {} # type: Dict[str, str] if backend == "saml": - assert len(settings.SOCIAL_AUTH_SAML_ENABLED_IDPS) == 1 - extra_url_params = {'idp': list(settings.SOCIAL_AUTH_SAML_ENABLED_IDPS.keys())[0]} + if not extra_arg or extra_arg not in settings.SOCIAL_AUTH_SAML_ENABLED_IDPS: + logging.info("Attempted to initiate SAML authentication with wrong idp argument: {}" + .format(extra_arg)) + return redirect_to_config_error("saml") + extra_url_params = {'idp': extra_arg} return oauth_redirect_to_root(request, backend_url, 'social', is_signup=True, extra_url_params=extra_url_params) diff --git a/zproject/backends.py b/zproject/backends.py index 23c2c48c62..3d5e81a8e1 100644 --- a/zproject/backends.py +++ b/zproject/backends.py @@ -1253,11 +1253,29 @@ def create_standard_social_backend_dict(social_backend: SocialAuthMixin) -> Soci sort_order=social_backend.sort_order ) +def list_saml_backend_dicts(realm: Optional[Realm]=None) -> List[SocialBackendDictT]: + result = [] # type: List[SocialBackendDictT] + for idp_name, idp_dict in settings.SOCIAL_AUTH_SAML_ENABLED_IDPS.items(): + saml_dict = dict( + name='saml:{}'.format(idp_name), + display_name=idp_dict.get('display_name', SAMLAuthBackend.auth_backend_name), + display_logo=idp_dict.get('display_logo', SAMLAuthBackend.display_logo), + login_url=reverse('login-social-extra-arg', args=('saml', idp_name)), + signup_url=reverse('signup-social-extra-arg', args=('saml', idp_name)), + sort_order=SAMLAuthBackend.sort_order - len(result) + ) # type: SocialBackendDictT + result.append(saml_dict) + + return result + def get_social_backend_dicts(realm: Optional[Realm]=None) -> List[SocialBackendDictT]: result = [] for backend in SOCIAL_AUTH_BACKENDS: if auth_enabled_helper([backend.auth_backend_name], realm): - result.append(create_standard_social_backend_dict(backend)) + if backend != SAMLAuthBackend: + result.append(create_standard_social_backend_dict(backend)) + else: + result += list_saml_backend_dicts(realm) return sorted(result, key=lambda x: x['sort_order'], reverse=True) diff --git a/zproject/prod_settings_template.py b/zproject/prod_settings_template.py index fd6bf397bc..ae273b89c8 100644 --- a/zproject/prod_settings_template.py +++ b/zproject/prod_settings_template.py @@ -223,6 +223,11 @@ SOCIAL_AUTH_SAML_ENABLED_IDPS = { "attr_email": "email", # The "x509cert" attribute is automatically read from # /etc/zulip/saml/idps/{idp_name}.crt; don't specify it here. + + # Optionally, you can edit display_name and display_logo settings below + # to change the name and icon that will show on the login button. + "display_name": "SAML", + "display_logo": "/static/images/landing-page/logos/saml-icon.png" } } diff --git a/zproject/test_settings.py b/zproject/test_settings.py index 4fad5f4160..f176937823 100644 --- a/zproject/test_settings.py +++ b/zproject/test_settings.py @@ -211,5 +211,7 @@ SOCIAL_AUTH_SAML_ENABLED_IDPS = { "attr_last_name": "last_name", "attr_username": "email", "attr_email": "email", + "display_name": "Test IdP", + "display_logo": "/static/images/landing-page/logos/saml-icon.png", } } diff --git a/zproject/urls.py b/zproject/urls.py index 5dc3db7e36..d5bb804d11 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -423,6 +423,8 @@ i18n_urls = [ url(r'^accounts/login/jwt/$', zerver.views.auth.remote_user_jwt, name='login-jwt'), url(r'^accounts/login/social/([\w,-]+)$', zerver.views.auth.start_social_login, name='login-social'), + url(r'^accounts/login/social/([\w,-]+)/([\w,-]+)$', zerver.views.auth.start_social_login, + name='login-social-extra-arg'), # Backwards-compatibility (legacy) Google auth URL for the mobile # apps; see https://github.com/zulip/zulip/issues/13081 for # background. We can remove this once older versions of the @@ -433,6 +435,9 @@ i18n_urls = [ url(r'^accounts/register/social/([\w,-]+)$', zerver.views.auth.start_social_signup, name='signup-social'), + url(r'^accounts/register/social/([\w,-]+)/([\w,-]+)$', + zerver.views.auth.start_social_signup, + name='signup-social-extra-arg'), url(r'^accounts/login/subdomain/([^/]+)$', zerver.views.auth.log_into_subdomain, name='zerver.views.auth.log_into_subdomain'), url(r'^accounts/login/local/$', zerver.views.auth.dev_direct_login,