ldap: Fix the syncing of user role via AUTH_LDAP_USER_FLAGS_BY_GROUP.

This was broken, due the mechanism simply using our
is_guest/is_realm_admin/etc. role setters, but failing to adjust system
group memberships - resulting in corrupted database state.
We need to ensure that change_user_role is called for setting user role.

There are two relevant codepaths that run the sync based on
AUTH_LDAP_USER_FLAGS_BY_GROUP and thus need to get this right:
1. manage.py sync_ldap_user_data
2. Just-in-time user creation when a user without a Zulip account logs
   in for the first using their ldap credentials. After
   get_or_build_user returns, django-auth-ldap sees that the user
   account has just been created, and proceeds to run ._populate_user().

Now that both user.save() and do_change_user_realm will be getting
called together, we need to ensure this always happens atomically.

This imposes the need to override _get_or_create_user to put it in a
transaction. The troublesome consequence is that this new
`atomic(savepoint=False)` causes the usual type of issue, where tests
testing error get their transaction rolled back and cannot continue
executing.

To get around that, we add a test helper
`artificial_transaction_savepoint` which allows these tests to wrap
their problematic blocks in an artificial transaction which provides a
savepoint, thus preventing the full test transaction rollback derailing
the rest of the test.

zerver/actions/create_user.py

@@ -499,7 +499,7 @@ def notify_created_bot(user_profile: UserProfile) -> None:
     send_event_on_commit(user_profile.realm, event, bot_owner_user_ids(user_profile))
 
 
-@transaction.atomic(durable=True)
+@transaction.atomic(savepoint=False)
 def do_create_user(
     email: str,
     password: str | None,

zerver/lib/test_classes.py

@@ -1485,10 +1485,7 @@ Output:
             "invite_only": orjson.dumps(invite_only).decode(),
         }
         post_data.update(extra_post_data)
-        # We wrap the API call with a 'transaction.atomic' context
-        # manager as it helps us with NOT rolling back the entire
-        # test transaction due to error responses.
-        with transaction.atomic(savepoint=True):
+        with self.artificial_transaction_savepoint():
             result = self.api_post(
                 user,
                 "/api/v1/users/me/subscriptions",
@@ -1965,6 +1962,16 @@ Output:
                 file_path = upload_message_attachment_from_request(UploadedFile(fp), user)[0]
                 return file_path
 
+    @contextmanager
+    def artificial_transaction_savepoint(self) -> Iterator[None]:
+        # Sometimes we need to wrap some test code, such as an API call with a
+        # 'transaction.atomic' context manager as it helps us with NOT rolling
+        # back the entire test transaction due to errors expected by the test.
+        # Otherwise, those errors can prevent the test from continuing, and throw
+        # TransactionManagementError instead.
+        with transaction.atomic(savepoint=True):
+            yield
+
 
 class ZulipTestCase(ZulipTestCaseMixin, TestCase):
     @contextmanager

zerver/tests/test_auth_backends.py

@@ -82,7 +82,11 @@ from zerver.lib.test_helpers import (
 )
 from zerver.lib.thumbnail import DEFAULT_AVATAR_SIZE, MEDIUM_AVATAR_SIZE, resize_avatar
 from zerver.lib.types import Validator
-from zerver.lib.user_groups import is_user_in_group
+from zerver.lib.user_groups import (
+    get_system_user_group_by_name,
+    get_system_user_group_for_user,
+    is_user_in_group,
+)
 from zerver.lib.users import get_users_for_api
 from zerver.lib.utils import assert_is_not_none
 from zerver.lib.validator import (
@@ -105,6 +109,7 @@ from zerver.models import (
     Stream,
     UserProfile,
 )
+from zerver.models.groups import SystemGroups, UserGroupMembership
 from zerver.models.realms import clear_supported_auth_backends_cache, get_realm
 from zerver.models.users import PasswordTooWeakError, get_user_by_delivery_email
 from zerver.signals import JUST_CREATED_THRESHOLD
@@ -452,6 +457,13 @@ class AuthBackendTest(ZulipTestCase):
 
         username = self.get_email()
         backend = ZulipLDAPAuthBackend()
+        orig_authenticate = backend.authenticate
+
+        def wrapped_authenticate(*args: Any, **kwargs: Any) -> UserProfile | None:
+            with self.artificial_transaction_savepoint():
+                return orig_authenticate(*args, **kwargs)
+
+        backend.authenticate = wrapped_authenticate
 
         # Test LDAP auth fails when LDAP server rejects password
         self.assertIsNone(
@@ -5090,17 +5102,19 @@ class FetchAPIKeyTest(ZulipTestCase):
         # We do test two combinations here:
         # The first user has no (department) attribute set
         # The second user has one set, but to a different value
-        result = self.client_post(
-            "/api/v1/fetch_api_key",
-            dict(username="hamlet", password=self.ldap_password("hamlet")),
-        )
+        with self.artificial_transaction_savepoint():
+            result = self.client_post(
+                "/api/v1/fetch_api_key",
+                dict(username="hamlet", password=self.ldap_password("hamlet")),
+            )
         self.assert_json_error(result, "Your username or password is incorrect", 401)
 
         self.change_ldap_user_attr("hamlet", "department", "testWrongRealm")
-        result = self.client_post(
-            "/api/v1/fetch_api_key",
-            dict(username="hamlet", password=self.ldap_password("hamlet")),
-        )
+        with self.artificial_transaction_savepoint():
+            result = self.client_post(
+                "/api/v1/fetch_api_key",
+                dict(username="hamlet", password=self.ldap_password("hamlet")),
+            )
         self.assert_json_error(result, "Your username or password is incorrect", 401)
 
         self.change_ldap_user_attr("hamlet", "department", "zulip")
@@ -5124,18 +5138,20 @@ class FetchAPIKeyTest(ZulipTestCase):
         self.init_default_ldap_database()
 
         # The first user has no attribute set
-        result = self.client_post(
-            "/api/v1/fetch_api_key",
-            dict(username="hamlet", password=self.ldap_password("hamlet")),
-        )
+        with self.artificial_transaction_savepoint():
+            result = self.client_post(
+                "/api/v1/fetch_api_key",
+                dict(username="hamlet", password=self.ldap_password("hamlet")),
+            )
         self.assert_json_error(result, "Your username or password is incorrect", 401)
 
         self.change_ldap_user_attr("hamlet", "test2", "testing")
         # Check with only one set
-        result = self.client_post(
-            "/api/v1/fetch_api_key",
-            dict(username="hamlet", password=self.ldap_password("hamlet")),
-        )
+        with self.artificial_transaction_savepoint():
+            result = self.client_post(
+                "/api/v1/fetch_api_key",
+                dict(username="hamlet", password=self.ldap_password("hamlet")),
+            )
         self.assert_json_error(result, "Your username or password is incorrect", 401)
 
         self.change_ldap_user_attr("hamlet", "test1", "test")
@@ -5167,10 +5183,11 @@ class FetchAPIKeyTest(ZulipTestCase):
 
         # Setting test1 to wrong value
         self.change_ldap_user_attr("hamlet", "test1", "invalid")
-        result = self.client_post(
-            "/api/v1/fetch_api_key",
-            dict(username="hamlet", password=self.ldap_password("hamlet")),
-        )
+        with self.artificial_transaction_savepoint():
+            result = self.client_post(
+                "/api/v1/fetch_api_key",
+                dict(username="hamlet", password=self.ldap_password("hamlet")),
+            )
         self.assert_json_error(result, "Your username or password is incorrect", 401)
 
         # Override access with `org_membership`
@@ -5191,7 +5208,7 @@ class FetchAPIKeyTest(ZulipTestCase):
                 "/api/v1/fetch_api_key",
                 dict(username="hamlet", password=self.ldap_password("hamlet")),
             )
-            self.assert_json_success(result)
+        self.assert_json_success(result)
 
     def test_inactive_user(self) -> None:
         do_deactivate_user(self.user_profile, acting_user=None)
@@ -6500,6 +6517,21 @@ class TestLDAP(ZulipLDAPTestCase):
             )
             self.assertIs(user, None)
 
+        with (
+            self.settings(LDAP_APPEND_DOMAIN="zulip.com"),
+            self.assertLogs("zulip.auth.ldap", level="DEBUG") as log_debug,
+        ):
+            user = self.backend.authenticate(
+                request=mock.MagicMock(),
+                username=self.example_email("hamlet"),
+                password="",
+                realm=get_realm("zulip"),
+            )
+            self.assertIs(user, None)
+            self.assertEqual(
+                log_debug.output[0], "DEBUG:zulip.auth.ldap:Rejecting empty password for hamlet"
+            )
+
     @override_settings(AUTHENTICATION_BACKENDS=("zproject.backends.ZulipLDAPAuthBackend",))
     def test_login_failure_due_to_nonexistent_user(self) -> None:
         with (
@@ -6972,6 +7004,7 @@ class TestZulipLDAPUserPopulator(ZulipLDAPTestCase):
             ),
             self.assertLogs("django_auth_ldap") as ldap_logs,
             self.assertRaises(AssertionError),
+            self.artificial_transaction_savepoint(),
         ):
             self.perform_ldap_sync(self.example_user("hamlet"))
         hamlet.refresh_from_db()
@@ -7818,6 +7851,68 @@ class JWTFetchAPIKeyTest(ZulipTestCase):
 
 
 class LDAPGroupSyncTest(ZulipTestCase):
+    @override_settings(AUTHENTICATION_BACKENDS=("zproject.backends.ZulipLDAPAuthBackend",))
+    def test_ldap_sync_role_from_groups(self) -> None:
+        self.init_default_ldap_database()
+
+        realm = get_realm("zulip")
+        hamlet = self.example_user("hamlet")
+        with (
+            self.settings(
+                LDAP_APPEND_DOMAIN="zulip.com",
+                AUTH_LDAP_GROUP_SEARCH=LDAPSearch(
+                    "ou=groups,dc=zulip,dc=com",
+                    ldap.SCOPE_ONELEVEL,
+                    "(objectClass=groupOfUniqueNames)",
+                ),
+                AUTH_LDAP_USER_FLAGS_BY_GROUP={
+                    "is_realm_admin": "cn=cool_test_group,ou=groups,dc=zulip,dc=com",
+                },
+            ),
+        ):
+            sync_user_from_ldap(hamlet, mock.Mock())
+            hamlet.refresh_from_db()
+            self.assertEqual(hamlet.role, UserProfile.ROLE_REALM_ADMINISTRATOR)
+
+            admin_group = get_system_user_group_by_name(SystemGroups.ADMINISTRATORS, realm.id)
+            self.assertEqual(get_system_user_group_for_user(hamlet), admin_group)
+
+            # Verify UserGroupMembership is set up correct - the user's direct membership should be the admins group.
+            self.assertIn(
+                admin_group.id,
+                set(
+                    UserGroupMembership.objects.filter(user_profile=hamlet).values_list(
+                        "user_group_id", flat=True
+                    )
+                ),
+            )
+
+            # Now test the just-in-time user creation codepath.
+            # A user with no Zulip account logs in for the first time with their LDAP credentials.
+            # The account is created on the fly and should the .role and system groups memberships
+            # set correctly from the start.
+            self.mock_ldap.directory["cn=cool_test_group,ou=groups,dc=zulip,dc=com"][
+                "uniqueMember"
+            ] = ["uid=newuser,ou=users,dc=zulip,dc=com"]
+            password = self.ldap_password("newuser")
+            email = "newuser@zulip.com"
+            self.login_with_return(email, password)
+            user_profile = UserProfile.objects.get(delivery_email=email)
+
+            self.assertEqual(user_profile.role, UserProfile.ROLE_REALM_ADMINISTRATOR)
+
+            admin_group = get_system_user_group_by_name(SystemGroups.ADMINISTRATORS, realm.id)
+            self.assertEqual(get_system_user_group_for_user(user_profile), admin_group)
+
+            self.assertIn(
+                admin_group.id,
+                set(
+                    UserGroupMembership.objects.filter(user_profile=user_profile).values_list(
+                        "user_group_id", flat=True
+                    )
+                ),
+            )
+
     @override_settings(AUTHENTICATION_BACKENDS=("zproject.backends.ZulipLDAPAuthBackend",))
     def test_ldap_group_sync(self) -> None:
         self.init_default_ldap_database()

zerver/tests/test_signup.py

@@ -3627,7 +3627,8 @@ class UserSignUpTest(ZulipTestCase):
             self.assertLogs("zulip.auth.ldap", "WARNING") as mock_log,
         ):
             original_user_count = UserProfile.objects.count()
-            self.login_with_return(username, password, HTTP_HOST=subdomain + ".testserver")
+            with self.artificial_transaction_savepoint():
+                self.login_with_return(username, password, HTTP_HOST=subdomain + ".testserver")
             # Verify that the process failed as intended - no UserProfile is created.
             self.assertEqual(UserProfile.objects.count(), original_user_count)
             self.assertEqual(

zproject/backends.py

@@ -30,6 +30,7 @@ from django.contrib.auth.backends import RemoteUserBackend
 from django.contrib.staticfiles.storage import staticfiles_storage
 from django.core.exceptions import ValidationError
 from django.core.validators import validate_email
+from django.db import transaction
 from django.dispatch import Signal, receiver
 from django.http import HttpRequest, HttpResponse, HttpResponseRedirect
 from django.shortcuts import render
@@ -1208,6 +1209,38 @@ class ZulipLDAPUser(_LDAPUser):
 
         super().__init__(*args, **kwargs)
 
+    @transaction.atomic(savepoint=False)
+    def _get_or_create_user(self, force_populate: bool = False) -> UserProfile:
+        # This function is responsible for the core logic of syncing
+        # a user's data with ldap - run in both populate_user codepath
+        # and just-in-time user creation upon first login via LDAP.
+        #
+        # To ensure we don't end up with corrupted database state,
+        # we need to run these operations atomically.
+        return super()._get_or_create_user(force_populate=force_populate)
+
+    def _populate_user(self) -> None:
+        """
+        Populates our User object with information from the LDAP directory.
+        """
+        assert isinstance(self._user, UserProfile)
+        user_profile = self._user
+        original_role = user_profile.role
+
+        # _populate_user() will make whatever changes to the user's attributes
+        # that it needs - possibly changing the user's role multiple times e.g.
+        # as it cycles through various role setters in AUTH_LDAP_USER_FLAGS_BY_GROUP.
+        #
+        # For that reason, we want to only look at the final role value after
+        # it is executed. This is the actual change (if any) that should take place.
+        # This allows us to call do_change_user_role only once.
+        super()._populate_user()
+        if user_profile.role != original_role:
+            # Change the role properly, updating system groups.
+            updated_role = user_profile.role
+            user_profile.role = original_role
+            do_change_user_role(user_profile, updated_role, acting_user=None)
+
     def _get_groups(self) -> _LDAPUserGroups:
         groups = super()._get_groups()
         if settings.DEVELOPMENT: