user_groups: Audit UserGroup subgroup memberships changes.

It's worth noting that instead of adding another field to the
RealmAuditLog model, we store the modified subgroup ids in extra_data as
a JSON encoded dict with the key "subgroup_ids". We don't create audit
log entries for supergroup changes at this point.

Signed-off-by: Zixuan James Li <p359101898@gmail.com>

zerver/actions/user_groups.py

@@ -2,6 +2,7 @@ import datetime
 from typing import Dict, List, Mapping, Optional, Sequence, TypedDict, Union
 
 import django.db.utils
+import orjson
 from django.db import transaction
 from django.utils.timezone import now as timezone_now
 from django.utils.translation import gettext as _
@@ -298,6 +299,16 @@ def add_subgroups_to_user_group(
     GroupGroupMembership.objects.bulk_create(group_memberships)
 
     subgroup_ids = [subgroup.id for subgroup in subgroups]
+    now = timezone_now()
+    RealmAuditLog.objects.create(
+        realm=user_group.realm,
+        modified_user_group=user_group,
+        event_type=RealmAuditLog.USER_GROUP_DIRECT_SUBGROUP_MEMBERSHIP_ADDED,
+        event_time=now,
+        acting_user=acting_user,
+        extra_data=orjson.dumps({"subgroup_ids": subgroup_ids}).decode(),
+    )
+
     do_send_subgroups_update_event("add_subgroups", user_group, subgroup_ids)
 
 
@@ -308,6 +319,16 @@ def remove_subgroups_from_user_group(
     GroupGroupMembership.objects.filter(supergroup=user_group, subgroup__in=subgroups).delete()
 
     subgroup_ids = [subgroup.id for subgroup in subgroups]
+    now = timezone_now()
+    RealmAuditLog.objects.create(
+        realm=user_group.realm,
+        modified_user_group=user_group,
+        event_type=RealmAuditLog.USER_GROUP_DIRECT_SUBGROUP_MEMBERSHIP_REMOVED,
+        event_time=now,
+        acting_user=acting_user,
+        extra_data=orjson.dumps({"subgroup_ids": subgroup_ids}).decode(),
+    )
+
     do_send_subgroups_update_event("remove_subgroups", user_group, subgroup_ids)
 
 

zerver/lib/user_groups.py

@@ -1,5 +1,6 @@
 from typing import Dict, Iterable, List, Mapping, Sequence, TypedDict
 
+import orjson
 from django.db import transaction
 from django.db.models import F, QuerySet
 from django.utils.timezone import now as timezone_now
@@ -328,7 +329,7 @@ def create_system_user_groups_for_realm(realm: Realm) -> Dict[int, UserGroup]:
 
     creation_time = timezone_now()
     UserGroup.objects.bulk_create(system_user_groups_list)
-    RealmAuditLog.objects.bulk_create(
+    realmauditlog_objects = [
         RealmAuditLog(
             realm=realm,
             acting_user=None,
@@ -337,7 +338,7 @@ def create_system_user_groups_for_realm(realm: Realm) -> Dict[int, UserGroup]:
             modified_user_group=user_group,
         )
         for user_group in system_user_groups_list
-    )
+    ]
 
     groups_with_updated_settings = []
     system_groups_name_dict = get_role_based_system_groups_dict(realm)
@@ -346,14 +347,25 @@ def create_system_user_groups_for_realm(realm: Realm) -> Dict[int, UserGroup]:
         groups_with_updated_settings.append(group)
     UserGroup.objects.bulk_update(groups_with_updated_settings, ["can_mention_group"])
 
-    subgroup_objects = []
+    subgroup_objects: List[GroupGroupMembership] = []
     # "Nobody" system group is not a subgroup of any user group, since it is already empty.
     subgroup, remaining_groups = system_user_groups_list[1], system_user_groups_list[2:]
     for supergroup in remaining_groups:
         subgroup_objects.append(GroupGroupMembership(subgroup=subgroup, supergroup=supergroup))
+        realmauditlog_objects.append(
+            RealmAuditLog(
+                realm=realm,
+                modified_user_group=supergroup,
+                event_type=RealmAuditLog.USER_GROUP_DIRECT_SUBGROUP_MEMBERSHIP_ADDED,
+                event_time=timezone_now(),
+                acting_user=None,
+                extra_data=orjson.dumps({"subgroup_ids": [subgroup.id]}).decode(),
+            )
+        )
         subgroup = supergroup
 
     GroupGroupMembership.objects.bulk_create(subgroup_objects)
+    RealmAuditLog.objects.bulk_create(realmauditlog_objects)
 
     return role_system_groups_dict
 

zerver/tests/test_audit_log.py

@@ -46,9 +46,11 @@ from zerver.actions.streams import (
     do_rename_stream,
 )
 from zerver.actions.user_groups import (
+    add_subgroups_to_user_group,
     bulk_add_members_to_user_group,
     check_add_user_group,
     remove_members_from_user_group,
+    remove_subgroups_from_user_group,
 )
 from zerver.actions.user_settings import (
     do_change_avatar_fields,
@@ -1090,6 +1092,32 @@ class TestRealmAuditLog(ZulipTestCase):
         )
         self.assertListEqual(logged_system_group_ids, system_user_group_ids)
 
+        logged_subgroup_entries = sorted(
+            RealmAuditLog.objects.filter(
+                realm=realm,
+                event_type=RealmAuditLog.USER_GROUP_DIRECT_SUBGROUP_MEMBERSHIP_ADDED,
+                event_time__gte=now,
+                acting_user=None,
+            ).values_list("modified_user_group_id", "extra_data")
+        )
+        # Excluding nobody_system_group, the rest of the user groups should have
+        # a chain of subgroup memberships in between.
+        self.assert_length(logged_subgroup_entries, expected_system_user_group_count - 2)
+        for i in range(len(logged_subgroup_entries)):
+            # The offset of 1 is due to nobody_system_group being skipped as
+            # the first user group in the list.
+            # For supergroup, we add an additional 1 because of the order we
+            # put the chain together.
+            expected_subgroup_id = system_user_group_ids[i + 1]
+            expected_supergroup_id = system_user_group_ids[i + 2]
+
+            supergroup_id, subgroup_extra_data = logged_subgroup_entries[i]
+            assert subgroup_extra_data is not None
+            self.assertEqual(
+                orjson.loads(subgroup_extra_data)["subgroup_ids"][0], expected_subgroup_id
+            )
+            self.assertEqual(supergroup_id, expected_supergroup_id)
+
     def test_user_group_creation(self) -> None:
         hamlet = self.example_user("hamlet")
         cordelia = self.example_user("cordelia")
@@ -1146,3 +1174,39 @@ class TestRealmAuditLog(ZulipTestCase):
         )
         self.assert_length(audit_log_entries, 1)
         self.assertEqual(audit_log_entries[0].modified_user, hamlet)
+
+    def test_change_user_group_subgroups_memberships(self) -> None:
+        hamlet = self.example_user("hamlet")
+        user_group = check_add_user_group(hamlet.realm, "main", [], acting_user=None)
+        subgroups = [
+            check_add_user_group(hamlet.realm, f"subgroup{num}", [], acting_user=hamlet)
+            for num in range(3)
+        ]
+
+        now = timezone_now()
+        add_subgroups_to_user_group(user_group, subgroups, acting_user=hamlet)
+        # Only one audit log entry for the subgroup membership is expected.
+        audit_log_entry = RealmAuditLog.objects.get(
+            realm=hamlet.realm,
+            event_time__gte=now,
+            event_type=RealmAuditLog.USER_GROUP_DIRECT_SUBGROUP_MEMBERSHIP_ADDED,
+        )
+        self.assertEqual(audit_log_entry.modified_user_group, user_group)
+        self.assertEqual(audit_log_entry.acting_user, hamlet)
+        self.assertDictEqual(
+            orjson.loads(assert_is_not_none(audit_log_entry.extra_data)),
+            {"subgroup_ids": [subgroup.id for subgroup in subgroups]},
+        )
+
+        remove_subgroups_from_user_group(user_group, subgroups[:2], acting_user=hamlet)
+        audit_log_entry = RealmAuditLog.objects.get(
+            realm=hamlet.realm,
+            event_time__gte=now,
+            event_type=RealmAuditLog.USER_GROUP_DIRECT_SUBGROUP_MEMBERSHIP_REMOVED,
+        )
+        self.assertEqual(audit_log_entry.modified_user_group, user_group)
+        self.assertEqual(audit_log_entry.acting_user, hamlet)
+        self.assertDictEqual(
+            orjson.loads(assert_is_not_none(audit_log_entry.extra_data)),
+            {"subgroup_ids": [subgroup.id for subgroup in subgroups[:2]]},
+        )

zerver/tests/test_slack_importer.py

@@ -1335,6 +1335,7 @@ class SlackImporter(ZulipTestCase):
                 RealmAuditLog.REALM_PLAN_TYPE_CHANGED,
                 RealmAuditLog.REALM_CREATED,
                 RealmAuditLog.USER_GROUP_CREATED,
+                RealmAuditLog.USER_GROUP_DIRECT_SUBGROUP_MEMBERSHIP_ADDED,
             },
         )