ldap: Fix attempting to sync data for deactivated users.

The order of operations for our LDAP synchronization code wasn't
correct: We would run the code to sync avatars (etc.) even for
deactivated users.

Thanks to niels for the report.

Co-authored-by: mateuszmandera <mateusz.mandera@protonmail.com>

zerver/tests/test_auth_backends.py

@@ -2678,6 +2678,20 @@ class TestZulipLDAPUserPopulator(ZulipLDAPTestCase):
         hamlet = self.example_user('hamlet')
         self.assertFalse(hamlet.is_active)
 
+    @mock.patch("zproject.backends.ZulipLDAPAuthBackendBase.sync_full_name_from_ldap")
+    def test_dont_sync_disabled_ldap_user(self, fake_sync: mock.MagicMock) -> None:
+        self.mock_ldap.directory = {
+            'uid=hamlet,ou=users,dc=zulip,dc=com': {
+                'cn': ['King Hamlet', ],
+                'userAccountControl': ['2', ],
+            }
+        }
+
+        with self.settings(AUTH_LDAP_USER_ATTR_MAP={'full_name': 'cn',
+                                                    'userAccountControl': 'userAccountControl'}):
+            self.perform_ldap_sync(self.example_user('hamlet'))
+            fake_sync.assert_not_called()
+
     def test_reactivate_user(self) -> None:
         self.mock_ldap.directory = {
             'uid=hamlet,ou=users,dc=zulip,dc=com': {

zproject/backends.py

@@ -426,20 +426,23 @@ class ZulipLDAPAuthBackendBase(ZulipAuthMixin, LDAPBackend):
            In authentication contexts, this is overriden in ZulipLDAPAuthBackend.
         """
         (user, built) = super().get_or_build_user(username, ldap_user)
-        self.sync_avatar_from_ldap(user, ldap_user)
-        self.sync_full_name_from_ldap(user, ldap_user)
-        self.sync_custom_profile_fields_from_ldap(user, ldap_user)
         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 and user.is_active:
-                logging.info("Deactivating user %s because they are disabled in LDAP." %
-                             (user.email,))
-                do_deactivate_user(user)
+            if user_disabled_in_ldap:
+                if user.is_active:
+                    logging.info("Deactivating user %s because they are disabled in LDAP." %
+                                 (user.email,))
+                    do_deactivate_user(user)
+                # Do an early return to avoid trying to sync additional data.
                 return (user, built)
-            if not user_disabled_in_ldap and not user.is_active:
+            elif not user.is_active:
                 logging.info("Reactivating user %s because they are not disabled in LDAP." %
                              (user.email,))
                 do_reactivate_user(user)
+
+        self.sync_avatar_from_ldap(user, ldap_user)
+        self.sync_full_name_from_ldap(user, ldap_user)
+        self.sync_custom_profile_fields_from_ldap(user, ldap_user)
         return (user, built)
 
 class ZulipLDAPAuthBackend(ZulipLDAPAuthBackendBase):