From ad698d597ac4032c5438f794d05b18765041f879 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Sun, 11 Dec 2022 21:29:10 -0500 Subject: [PATCH] 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 --- zerver/actions/user_groups.py | 21 ++++++++++ zerver/lib/user_groups.py | 18 ++++++-- zerver/tests/test_audit_log.py | 64 +++++++++++++++++++++++++++++ zerver/tests/test_slack_importer.py | 1 + 4 files changed, 101 insertions(+), 3 deletions(-) diff --git a/zerver/actions/user_groups.py b/zerver/actions/user_groups.py index d6e2083527..93a0f656d4 100644 --- a/zerver/actions/user_groups.py +++ b/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) diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index 53dd604db9..a57c3be83d 100644 --- a/zerver/lib/user_groups.py +++ b/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 diff --git a/zerver/tests/test_audit_log.py b/zerver/tests/test_audit_log.py index 94beb3f2ea..e92f674e2a 100644 --- a/zerver/tests/test_audit_log.py +++ b/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]]}, + ) diff --git a/zerver/tests/test_slack_importer.py b/zerver/tests/test_slack_importer.py index c28da40cc5..c43ea9a927 100644 --- a/zerver/tests/test_slack_importer.py +++ b/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, }, )