From 145817d1e05a3fe42c941ea0655444f6cf7c9110 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Mon, 23 Oct 2017 11:42:37 -0700 Subject: [PATCH] forms: Pass the realm into authenticate in OurAuthenticationForm. Historically, we'd just use the default Django version of this function. However, since we did the big subdomains migration, it's now the case that we have to pass in the subdomain to authenticate (i.e. there's no longer a fallback to just looking up the user by email). This fixes a problem with user creation in an LDAP realm, because previously, the user creation flow would just pass in the username and password (after validating the subdomain). --- zerver/forms.py | 21 +++++++++++++++++ zerver/tests/test_signup.py | 46 +++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/zerver/forms.py b/zerver/forms.py index 30d69059ad..e4a04c3b08 100644 --- a/zerver/forms.py +++ b/zerver/forms.py @@ -1,6 +1,7 @@ from django import forms from django.conf import settings +from django.contrib.auth import authenticate from django.contrib.auth.forms import SetPasswordForm, AuthenticationForm, \ PasswordResetForm from django.core.exceptions import ValidationError @@ -267,6 +268,26 @@ Please contact %s to reactivate this group.""" % ( raise ValidationError(mark_safe(WRONG_SUBDOMAIN_ERROR)) return email + def clean(self): + # type: () -> Dict[str, Any] + username = self.cleaned_data.get('username') + password = self.cleaned_data.get('password') + + if username is not None and password: + subdomain = get_subdomain(self.request) + self.user_cache = authenticate(self.request, username=username, password=password, + realm_subdomain=subdomain) + if self.user_cache is None: + raise forms.ValidationError( + self.error_messages['invalid_login'], + code='invalid_login', + params={'username': self.username_field.verbose_name}, + ) + else: + self.confirm_login_allowed(self.user_cache) + + return self.cleaned_data + class MultiEmailField(forms.Field): def to_python(self, emails): # type: (Text) -> List[Text] diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index 65095bad6d..c9c1f1c3e2 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -1701,6 +1701,52 @@ class UserSignUpTest(ZulipTestCase): # Name comes from form which was set by LDAP. self.assertEqual(user_profile.full_name, full_name) + @override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend', + 'zproject.backends.ZulipDummyBackend')) + def test_ldap_auto_registration_on_login(self): + # type: () -> None + """The most common way for LDAP authentication to be used is with a + server that doesn't have a terms-of-service required, in which + case we offer a complete single-sign-on experience (where the + user just enters their LDAP username and password, and their + account is created if it doesn't already exist). + + This test verifies that flow. + """ + password = "testing" + 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 + + full_name = 'New LDAP fullname' + mock_ldap.directory = { + 'uid=newuser,ou=users,dc=zulip,dc=com': { + 'userPassword': 'testing', + 'fn': [full_name], + 'sn': ['shortname'], + } + } + + with self.settings( + POPULATE_PROFILE_VIA_LDAP=True, + 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'): + + self.login_with_return(email, password, + HTTP_HOST=subdomain + ".testserver") + + user_profile = UserProfile.objects.get(email=email) + # Name comes from form which was set by LDAP. + self.assertEqual(user_profile.full_name, full_name) + @override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend', 'zproject.backends.ZulipDummyBackend')) def test_ldap_registration_when_names_changes_are_disabled(self):