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

@@ -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.

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)

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]: