From b81bde3a909f9708bc4f5d2830d270ddcbdf7140 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Tue, 24 Oct 2017 10:38:31 -0700 Subject: [PATCH] ldap: Prevent creating accounts with Zulip/EmailAuthBackend passwords. While our recent changing to hide /register means we don't need a nice pretty error message here, eventually we'll want to clean up the error message. Fixes #7047. --- zerver/tests/test_signup.py | 30 ++++++++++++++++++++-------- zerver/views/registration.py | 38 +++++++++++++++++++++++++++++++++--- 2 files changed, 57 insertions(+), 11 deletions(-) diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index c9c1f1c3e2..275e010e66 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -1657,7 +1657,8 @@ class UserSignUpTest(ZulipTestCase): mock_ldap.directory = { 'uid=newuser,ou=users,dc=zulip,dc=com': { 'userPassword': 'testing', - 'fn': [full_name] + 'fn': [full_name], + 'sn': ['shortname'], } } @@ -1691,7 +1692,19 @@ class UserSignUpTest(ZulipTestCase): "newuser@zulip.com"], result) - # Submit the final form. + # Submit the final form with the wrong password. + result = self.submit_reg_form_for_user(email, + 'wrongpassword', + full_name=full_name, + # Pass HTTP_HOST for the target subdomain + HTTP_HOST=subdomain + ".testserver") + # Didn't create an account + with self.assertRaises(UserProfile.DoesNotExist): + user_profile = UserProfile.objects.get(email=email) + self.assertEqual(result.status_code, 302) + self.assertEqual(result.url, "/accounts/login/?email=newuser%40zulip.com") + + # Submit the final form with the wrong password. result = self.submit_reg_form_for_user(email, password, full_name=full_name, @@ -1765,7 +1778,8 @@ class UserSignUpTest(ZulipTestCase): mock_ldap.directory = { 'uid=newuser,ou=users,dc=zulip,dc=com': { 'userPassword': 'testing', - 'fn': ['New LDAP fullname'] + 'fn': ['New LDAP fullname'], + 'sn': ['New LDAP shortname'], } } @@ -1795,11 +1809,11 @@ class UserSignUpTest(ZulipTestCase): # Pass HTTP_HOST for the target subdomain HTTP_HOST=subdomain + ".testserver") - with patch('zerver.views.registration.name_changes_disabled', return_value=True): - result = self.submit_reg_form_for_user(email, - password, - # Pass HTTP_HOST for the target subdomain - HTTP_HOST=subdomain + ".testserver") + with patch('zerver.views.registration.name_changes_disabled', return_value=True): + result = self.submit_reg_form_for_user(email, + password, + # Pass HTTP_HOST for the target subdomain + HTTP_HOST=subdomain + ".testserver") user_profile = UserProfile.objects.get(email=email) # Name comes from LDAP session. self.assertEqual(user_profile.full_name, 'New LDAP fullname') diff --git a/zerver/views/registration.py b/zerver/views/registration.py index e8b38df268..fa1b7a412d 100644 --- a/zerver/views/registration.py +++ b/zerver/views/registration.py @@ -31,7 +31,7 @@ from zerver.lib.onboarding import send_initial_pms, setup_initial_streams, \ from zerver.lib.response import json_success from zerver.lib.subdomains import get_subdomain, is_root_domain_available from zerver.lib.timezone import get_all_timezones -from zproject.backends import password_auth_enabled +from zproject.backends import ldap_auth_enabled, password_auth_enabled, ZulipLDAPAuthBackend from confirmation.models import Confirmation, RealmCreationKey, ConfirmationKeyException, \ check_key_is_valid, create_confirmation_link, get_object_from_key, \ @@ -190,7 +190,36 @@ def accounts_register(request): except UserProfile.DoesNotExist: existing_user_profile = None - if existing_user_profile is not None and existing_user_profile.is_mirror_dummy: + return_data = {} # type: Dict[str, bool] + if ldap_auth_enabled(realm): + # If the user was authenticated using an external SSO + # mechanism like Google or GitHub auth, then authentication + # will have already been done before creating the + # PreregistrationUser object with password_required=False, and + # so we don't need to worry about passwords. + # + # If instead the realm is using EmailAuthBackend, we will + # set their password above. + # + # But if the realm is using LDAPAuthBackend, we need to verify + # their LDAP password (which will, as a side effect, create + # the user account) here using authenticate. + auth_result = authenticate(request, + username=email, + password=password, + realm_subdomain=realm.subdomain, + return_data=return_data) + if auth_result is None: + # TODO: This probably isn't going to give a + # user-friendly error message, but it doesn't + # particularly matter, because the registration form + # is hidden for most users. + 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: user_profile = existing_user_profile do_activate_user(user_profile) do_change_password(user_profile, password) @@ -203,6 +232,10 @@ def accounts_register(request): timezone=timezone, newsletter_data={"IP": request.META['REMOTE_ADDR']}) + # Note: Any logic like this must also be replicated in + # ZulipLDAPAuthBackend and zerver/views/users.py. This is + # ripe for a refactoring, though care is required to avoid + # import loops with zerver/lib/actions.py and zerver/lib/onboarding.py. send_initial_pms(user_profile) if realm_creation: @@ -217,7 +250,6 @@ def accounts_register(request): # This dummy_backend check below confirms the user is # authenticating to the correct subdomain. - return_data = {} # type: Dict[str, bool] auth_result = authenticate(username=user_profile.email, realm_subdomain=realm.subdomain, return_data=return_data,