From 91ec0aba09389b97e1cda886df1b87795d1f9e37 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Mon, 28 May 2018 21:52:06 -0700 Subject: [PATCH] auth: Improve interactions between LDAPAuthBackend and EmailAuthBackend. Previously, if you had LDAPAuthBackend enabled, we basically blocked any other auth backends from working at all, by requiring the user's login flow include verifying the user's LDAP password. We still want to enforce that in the case that the account email matches LDAP_APPEND_DOMAIN, but there's a reasonable corner case: Having effectively guest users from outside the LDAP domain. We don't want to allow creating a Zulip-level password for a user inside the LDAP domain, so we still verify the LDAP password in that flow, but if the email is allowed to register (due to invite or whatever) but is outside the LDAP domain for the organization, we allow it to create an account and set a password. For the moment, this solution only covers EmailAuthBackend. It's likely that just extending the list of other backends we check for in the new conditional on `email_auth_backend` would be correct, but we haven't done any testing for those cases, and with auth code paths, it's better to disallow than allow untested code paths. Fixes #9422. --- zerver/tests/test_signup.py | 92 +++++++++++++++++++++++++++++++++++- zerver/views/registration.py | 34 ++++++++++--- zproject/backends.py | 11 +++-- 3 files changed, 127 insertions(+), 10 deletions(-) diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index fe8af48de1..cba013f55d 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -363,7 +363,6 @@ class PasswordResetTest(ZulipTestCase): "/accounts/password/reset/done/")) result = self.client_get(result["Location"]) - from django.core.mail import outbox self.assertEqual(len(outbox), 1) message = outbox.pop() self.assertIn(FromAddress.NOREPLY, message.from_email) @@ -2428,6 +2427,97 @@ class UserSignUpTest(ZulipTestCase): # Name comes from LDAP session. self.assertEqual(user_profile.full_name, 'New LDAP fullname') + @override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend', + 'zproject.backends.EmailAuthBackend', + 'zproject.backends.ZulipDummyBackend')) + def test_signup_with_ldap_and_email_enabled_using_email(self) -> None: + password = "mynewpassword" + email = "newuser@zulip.com" + subdomain = "zulip" + + ldap_user_attr_map = {'full_name': 'fn', 'short_name': 'sn'} + + ldap_patcher = patch('django_auth_ldap.config.ldap.initialize') + mock_initialize = ldap_patcher.start() + mock_ldap = MockLDAP() + mock_initialize.return_value = mock_ldap + + mock_ldap.directory = { + 'uid=newuser,ou=users,dc=zulip,dc=com': { + 'userPassword': 'testing', + 'fn': ['New LDAP fullname'], + 'sn': ['New LDAP shortname'], + } + } + with patch('zerver.views.registration.get_subdomain', return_value=subdomain): + result = self.client_post('/register/', {'email': email}) + + self.assertEqual(result.status_code, 302) + self.assertTrue(result["Location"].endswith( + "/accounts/send_confirm/%s" % (email,))) + result = self.client_get(result["Location"]) + self.assert_in_response("Check your email so we can get started.", result) + + # If the user's email is inside the LDAP domain and we just + # have a wrong password, then we refuse to create an account + with self.settings( + POPULATE_PROFILE_VIA_LDAP=True, + # Important: This doesn't match the new user + LDAP_APPEND_DOMAIN='zulip.com', + AUTH_LDAP_BIND_PASSWORD='', + AUTH_LDAP_USER_ATTR_MAP=ldap_user_attr_map, + AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com'): + + result = self.submit_reg_form_for_user( + email, + password, + from_confirmation="1", + # Pass HTTP_HOST for the target subdomain + HTTP_HOST=subdomain + ".testserver") + self.assertEqual(result.status_code, 200) + + result = self.submit_reg_form_for_user(email, + password, + full_name="Non-LDAP Full Name", + # Pass HTTP_HOST for the target subdomain + HTTP_HOST=subdomain + ".testserver") + self.assertEqual(result.status_code, 302) + # We get redirected back to the login page because password was wrong + self.assertEqual(result.url, "/accounts/login/?email=newuser%40zulip.com") + self.assertFalse(UserProfile.objects.filter(email=email).exists()) + + # If the user's email is outside the LDAP domain, though, we + # successfully create an account with a password in the Zulip + # database. + with self.settings( + POPULATE_PROFILE_VIA_LDAP=True, + # Important: This doesn't match the new user + LDAP_APPEND_DOMAIN='example.com', + AUTH_LDAP_BIND_PASSWORD='', + AUTH_LDAP_USER_ATTR_MAP=ldap_user_attr_map, + AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com'): + + with patch('zerver.views.registration.logging.warning') as mock_warning: + result = self.submit_reg_form_for_user( + email, + password, + from_confirmation="1", + # Pass HTTP_HOST for the target subdomain + HTTP_HOST=subdomain + ".testserver") + self.assertEqual(result.status_code, 200) + mock_warning.assert_called_once_with("New account email newuser@zulip.com could not be found in LDAP") + + result = self.submit_reg_form_for_user(email, + password, + full_name="Non-LDAP Full Name", + # Pass HTTP_HOST for the target subdomain + HTTP_HOST=subdomain + ".testserver") + self.assertEqual(result.status_code, 302) + self.assertEqual(result.url, "http://zulip.testserver/") + user_profile = UserProfile.objects.get(email=email) + # Name comes from the POST request, not LDAP + self.assertEqual(user_profile.full_name, 'Non-LDAP Full Name') + def test_registration_when_name_changes_are_disabled(self) -> None: """ Test `name_changes_disabled` when we are not running under LDAP. diff --git a/zerver/views/registration.py b/zerver/views/registration.py index 15de826664..5c34d46862 100644 --- a/zerver/views/registration.py +++ b/zerver/views/registration.py @@ -35,7 +35,8 @@ from zerver.lib.timezone import get_all_timezones from zerver.views.auth import create_preregistration_user, \ redirect_and_log_into_subdomain, \ redirect_to_deactivation_notice -from zproject.backends import ldap_auth_enabled, password_auth_enabled, ZulipLDAPAuthBackend +from zproject.backends import ldap_auth_enabled, password_auth_enabled, ZulipLDAPAuthBackend, \ + ZulipLDAPException, email_auth_enabled from confirmation.models import Confirmation, RealmCreationKey, ConfirmationKeyException, \ validate_key, create_confirmation_link, get_object_from_key, \ @@ -133,7 +134,13 @@ def accounts_register(request: HttpRequest) -> HttpResponse: elif settings.POPULATE_PROFILE_VIA_LDAP: for backend in get_backends(): if isinstance(backend, LDAPBackend): - ldap_attrs = _LDAPUser(backend, backend.django_to_ldap_username(email)).attrs + try: + ldap_attrs = _LDAPUser(backend, backend.django_to_ldap_username(email)).attrs + except ZulipLDAPException: + logging.warning("New account email %s could not be found in LDAP" % (email,)) + form = RegistrationForm(realm_creation=realm_creation) + break + try: ldap_full_name = ldap_attrs[settings.AUTH_LDAP_USER_ATTR_MAP['full_name']][0] request.session['authenticated_full_name'] = ldap_full_name @@ -230,7 +237,24 @@ def accounts_register(request: HttpRequest) -> HttpResponse: password=password, realm=realm, return_data=return_data) - if auth_result is None: + if auth_result is not None: + # Since we'll have created a user, we now just log them in. + return login_and_go_to_home(request, auth_result) + + if return_data.get("outside_ldap_domain") and email_auth_enabled(realm): + # If both the LDAP and Email auth backends are + # enabled, and the user's email is outside the LDAP + # domain, then the intent is to create a user in the + # realm with their email outside the LDAP organization + # (with e.g. a password stored in the Zulip database, + # not LDAP). So we fall through and create the new + # account. + # + # It's likely that we can extend this block to the + # Google and GitHub auth backends with no code changes + # other than here. + pass + else: # TODO: This probably isn't going to give a # user-friendly error message, but it doesn't # particularly matter, because the registration form @@ -238,9 +262,7 @@ def accounts_register(request: HttpRequest) -> HttpResponse: return HttpResponseRedirect(reverse('django.contrib.auth.views.login') + '?email=' + urllib.parse.quote_plus(email)) - # Since we'll have created a user, we now just log them in. - return login_and_go_to_home(request, auth_result) - elif existing_user_profile is not None and existing_user_profile.is_mirror_dummy: + if existing_user_profile is not None and existing_user_profile.is_mirror_dummy: user_profile = existing_user_profile do_activate_user(user_profile) do_change_password(user_profile, password) diff --git a/zproject/backends.py b/zproject/backends.py index a3c67979a3..3d64abd1ad 100644 --- a/zproject/backends.py +++ b/zproject/backends.py @@ -382,6 +382,9 @@ def email_belongs_to_ldap(realm: Realm, email: str) -> bool: class ZulipLDAPException(_LDAPUser.AuthenticationFailed): pass +class ZulipLDAPExceptionOutsideDomain(ZulipLDAPException): + pass + class ZulipLDAPConfigurationError(Exception): pass @@ -408,7 +411,7 @@ class ZulipLDAPAuthBackendBase(ZulipAuthMixin, LDAPBackend): def django_to_ldap_username(self, username: str) -> str: if settings.LDAP_APPEND_DOMAIN: if not username.endswith("@" + settings.LDAP_APPEND_DOMAIN): - raise ZulipLDAPException("Username does not match LDAP domain.") + raise ZulipLDAPExceptionOutsideDomain("Username does not match LDAP domain.") return email_to_username(username) return username @@ -433,8 +436,10 @@ class ZulipLDAPAuthBackend(ZulipLDAPAuthBackendBase): return ZulipLDAPAuthBackendBase.authenticate(self, username=username, password=password) - except ZulipLDAPException: - return None # nocoverage # TODO: this may no longer be possible + except ZulipLDAPException as e: + if isinstance(e, ZulipLDAPExceptionOutsideDomain): + return_data['outside_ldap_domain'] = True + return None def get_or_build_user(self, username: str, ldap_user: _LDAPUser) -> Tuple[UserProfile, bool]: