ldap: Extract ldap user -> django username mapping logic to a function.

Fixes #11878

Instead of a confusing mix of django_auth_backed applying
ldap_to_django_username in its internals for one part of the
translation, and then custom logic for grabbing it from the email
attribute of the ldapuser in ZulipLDAPAuthBackend.get_or_build_user
for the second part of the translation,
we put all the logic in a single function user_email_from_ldapuser
which will be used by get_or_build of both ZulipLDAPUserPopulator and
ZulipLDAPAuthBackend.

This, building on the previous commits with the email search feature,
fixes the ldap sync bug from issue #11878.

If we can get upstream django-auth-ldap to merge
https://github.com/django-auth-ldap/django-auth-ldap/pull/154, we'll
be able to go back to using the version of ldap_to_django_username
that accepts a _LDAPUser object.

zerver/tests/test_auth_backends.py

@@ -3,7 +3,7 @@ from django.conf import settings
 from django.core import mail
 from django.http import HttpResponse
 from django.test import override_settings
-from django_auth_ldap.backend import LDAPBackend, LDAPSearch
+from django_auth_ldap.backend import LDAPBackend, LDAPSearch, _LDAPUser
 from django.test.client import RequestFactory
 from django.utils.timezone import now as timezone_now
 from typing import Any, Callable, Dict, List, Optional, Set, Tuple
@@ -2546,10 +2546,11 @@ class TestLDAP(ZulipLDAPTestCase):
             backend.django_to_ldap_username('"hamlet@test"@test.com')
 
     @override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',))
-    def test_ldap_to_django_username(self) -> None:
+    def test_user_email_from_ldapuser_with_append_domain(self) -> None:
         backend = self.backend
         with self.settings(LDAP_APPEND_DOMAIN='zulip.com'):
-            username = backend.ldap_to_django_username('"hamlet@test"')
+            username = backend.user_email_from_ldapuser('this_argument_is_ignored',
+                                                        _LDAPUser(self.backend, username='"hamlet@test"'))
             self.assertEqual(username, '"hamlet@test"@zulip.com')
 
     @override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',))

zerver/tests/test_ldap.py

@@ -4,7 +4,7 @@ from django.test.utils import override_settings
 
 from zerver.lib.test_classes import ZulipTestCase
 from zerver.models import get_realm
-from zproject.backends import ZulipLDAPAuthBackend, ZulipLDAPExceptionOutsideDomain
+from zproject.backends import ZulipLDAPAuthBackend, ZulipLDAPExceptionOutsideDomain, sync_user_from_ldap
 
 from django_auth_ldap.config import LDAPSearch
 
@@ -89,3 +89,27 @@ class DjangoToLDAPUsernameTests(ZulipTestCase):
             # Ldap password works now:
             self.assertEqual(authenticate(username=user_profile.email, password="testing", realm=realm),
                              user_profile)
+
+    @override_settings(LDAP_EMAIL_ATTR='mail', LDAP_DEACTIVATE_NON_MATCHING_USERS=True)
+    def test_sync_user_from_ldap_with_email_attr(self) -> None:
+        """In LDAP configurations with LDAP_EMAIL_ATTR configured and
+        LDAP_DEACTIVATE_NON_MATCHING_USERS set, a possible failure
+        mode if django_to_ldap_username isn't configured correctly is
+        all LDAP users having their accounts deactivated.  Before the
+        introduction of AUTH_LDAP_REVERSE_EMAIL_SEARCH, this would happen
+        even in valid LDAP configurations using LDAP_EMAIL_ATTR.
+
+        This test confirms that such a failure mode doesn't happen with
+        a valid LDAP configuration.
+        """
+
+        user_profile = self.example_user("hamlet")
+        with self.settings():
+            sync_user_from_ldap(user_profile, mock.Mock())
+            # Syncing didn't deactivate the user:
+            self.assertTrue(user_profile.is_active)
+
+        with self.settings(AUTH_LDAP_REVERSE_EMAIL_SEARCH=None):
+            # Without email search, the user will get deactivated.
+            sync_user_from_ldap(user_profile, mock.Mock())
+            self.assertFalse(user_profile.is_active)

zproject/backends.py

@@ -348,9 +348,35 @@ class ZulipLDAPAuthBackendBase(ZulipAuthMixin, LDAPBackend):
 
         return username
 
-    def ldap_to_django_username(self, username: str) -> str:
+    def user_email_from_ldapuser(self, username: str, ldap_user: _LDAPUser) -> str:
+        if hasattr(ldap_user, '_username'):
+            # In tests, we sometimes pass a simplified _LDAPUser without _username attr,
+            # and with the intended username in the username argument.
+            username = ldap_user._username
+
         if settings.LDAP_APPEND_DOMAIN:
             return "@".join((username, settings.LDAP_APPEND_DOMAIN))
+
+        if settings.LDAP_EMAIL_ATTR is not None:
+            # Get email from ldap attributes.
+            if settings.LDAP_EMAIL_ATTR not in ldap_user.attrs:
+                raise ZulipLDAPException("LDAP user doesn't have the needed %s attribute" % (
+                    settings.LDAP_EMAIL_ATTR,))
+            else:
+                return ldap_user.attrs[settings.LDAP_EMAIL_ATTR][0]
+
+        return username
+
+    def ldap_to_django_username(self, username: str) -> str:
+        """
+        This is called inside django_auth_ldap with only one role:
+        to convert _LDAPUser._username to django username (so in Zulip, the email)
+        and pass that as "username" argument to get_or_build_user(username, ldapuser).
+        In many cases, the email is stored in the _LDAPUser's attributes, so it can't be
+        constructed just from the username. We choose to do nothing in this function,
+        and our overrides of get_or_build_user() obtain that username from the _LDAPUser
+        object on their own, through our user_email_from_ldapuser function.
+        """
         return username
 
     def sync_avatar_from_ldap(self, user: UserProfile, ldap_user: _LDAPUser) -> None:
@@ -478,7 +504,13 @@ class ZulipLDAPAuthBackendBase(ZulipAuthMixin, LDAPBackend):
              ./manage.py sync_ldap_user_data
            In authentication contexts, this is overriden in ZulipLDAPAuthBackend.
         """
+        # Obtain the django username from the ldap_user object:
+        username = self.user_email_from_ldapuser(username, ldap_user)
+
+        # Call the library get_or_build_user for building the UserProfile
+        # with the username we obtained:
         (user, built) = super().get_or_build_user(username, ldap_user)
+        # Synchronise the UserProfile with its LDAP attributes:
         if 'userAccountControl' in settings.AUTH_LDAP_USER_ATTR_MAP:
             user_disabled_in_ldap = self.is_account_control_disabled_user(ldap_user)
             if user_disabled_in_ldap:
@@ -549,14 +581,7 @@ class ZulipLDAPAuthBackend(ZulipLDAPAuthBackendBase):
         """
         return_data = {}  # type: Dict[str, Any]
 
-        if settings.LDAP_EMAIL_ATTR is not None:
-            # Get email from ldap attributes.
-            if settings.LDAP_EMAIL_ATTR not in ldap_user.attrs:
-                return_data["ldap_missing_attribute"] = settings.LDAP_EMAIL_ATTR
-                raise ZulipLDAPException("LDAP user doesn't have the needed %s attribute" % (
-                    settings.LDAP_EMAIL_ATTR,))
-
-            username = ldap_user.attrs[settings.LDAP_EMAIL_ATTR][0]
+        username = self.user_email_from_ldapuser(username, ldap_user)
 
         if 'userAccountControl' in settings.AUTH_LDAP_USER_ATTR_MAP:  # nocoverage
             ldap_disabled = self.is_account_control_disabled_user(ldap_user)