diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 13e49e65e6..59ba5bf43b 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -197,6 +197,7 @@ from zerver.lib.user_groups import ( access_user_group_by_id, create_system_user_groups_for_realm, create_user_group, + get_system_user_group_for_user, ) from zerver.lib.user_mutes import add_user_mute, get_muting_users, get_user_mutes from zerver.lib.user_status import update_user_status @@ -716,9 +717,27 @@ def do_create_user( if settings.BILLING_ENABLED: update_license_ledger_if_needed(user_profile.realm, event_time) + system_user_group = get_system_user_group_for_user(user_profile) + UserGroupMembership.objects.create(user_profile=user_profile, user_group=system_user_group) + + if user_profile.role == UserProfile.ROLE_MEMBER and not user_profile.is_provisional_member: + full_members_system_group = UserGroup.objects.get( + name="@role:fullmembers", realm=user_profile.realm, is_system_group=True + ) + UserGroupMembership.objects.create( + user_profile=user_profile, user_group=full_members_system_group + ) + # Note that for bots, the caller will send an additional event # with bot-specific info like services. notify_created_user(user_profile) + + do_send_user_group_members_update_event("add_members", system_user_group, [user_profile.id]) + if user_profile.role == UserProfile.ROLE_MEMBER and not user_profile.is_provisional_member: + do_send_user_group_members_update_event( + "add_members", full_members_system_group, [user_profile.id] + ) + if bot_type is None: process_new_human_user( user_profile, @@ -840,6 +859,59 @@ def active_humans_in_realm(realm: Realm) -> Sequence[UserProfile]: return UserProfile.objects.filter(realm=realm, is_active=True, is_bot=False) +@transaction.atomic(savepoint=False) +def update_users_in_full_members_system_group( + realm: Realm, affected_user_ids: Sequence[int] = [] +) -> None: + full_members_system_group = UserGroup.objects.get( + realm=realm, name="@role:fullmembers", is_system_group=True + ) + members_system_group = UserGroup.objects.get( + realm=realm, name="@role:members", is_system_group=True + ) + + full_member_group_users = list( + full_members_system_group.direct_members.filter(id__in=affected_user_ids).values( + "id", "role", "date_joined" + ) + ) + member_group_users = list( + members_system_group.direct_members.filter(id__in=affected_user_ids).values( + "id", "role", "date_joined" + ) + ) + + def is_provisional_member(user: Dict[str, Union[int, datetime.datetime]]) -> bool: + diff = (timezone_now() - user["date_joined"]).days + if diff < realm.waiting_period_threshold: + return True + return False + + old_full_members = [ + user + for user in full_member_group_users + if is_provisional_member(user) or user["role"] != UserProfile.ROLE_MEMBER + ] + + full_member_group_user_ids = [user["id"] for user in full_member_group_users] + members_excluding_full_members = [ + user for user in member_group_users if user["id"] not in full_member_group_user_ids + ] + + new_full_members = [ + user for user in members_excluding_full_members if not is_provisional_member(user) + ] + + old_full_member_ids = [user["id"] for user in old_full_members] + new_full_member_ids = [user["id"] for user in new_full_members] + + if len(old_full_members) > 0: + remove_members_from_user_group(full_members_system_group, old_full_member_ids) + + if len(new_full_members) > 0: + bulk_add_members_to_user_group(full_members_system_group, new_full_member_ids) + + @transaction.atomic(savepoint=False) def do_set_realm_property( realm: Realm, name: str, value: Any, *, acting_user: Optional[UserProfile] @@ -4958,6 +5030,8 @@ def do_change_user_role( user_profile: UserProfile, value: int, *, acting_user: Optional[UserProfile] ) -> None: old_value = user_profile.role + old_system_group = get_system_user_group_for_user(user_profile) + user_profile.role = value user_profile.save(update_fields=["role"]) RealmAuditLog.objects.create( @@ -4981,6 +5055,20 @@ def do_change_user_role( lambda: send_event(user_profile.realm, event, active_user_ids(user_profile.realm_id)) ) + UserGroupMembership.objects.filter( + user_profile=user_profile, user_group=old_system_group + ).delete() + + system_group = get_system_user_group_for_user(user_profile) + UserGroupMembership.objects.create(user_profile=user_profile, user_group=system_group) + + do_send_user_group_members_update_event("remove_members", old_system_group, [user_profile.id]) + + do_send_user_group_members_update_event("add_members", system_group, [user_profile.id]) + + if UserProfile.ROLE_MEMBER in [old_value, value]: + update_users_in_full_members_system_group(user_profile.realm, [user_profile.id]) + def do_make_user_billing_admin(user_profile: UserProfile) -> None: user_profile.is_billing_admin = True diff --git a/zerver/lib/test_classes.py b/zerver/lib/test_classes.py index ef16983920..6803902000 100644 --- a/zerver/lib/test_classes.py +++ b/zerver/lib/test_classes.py @@ -73,6 +73,7 @@ from zerver.lib.test_console_output import ( tee_stdout_and_find_extra_console_output, ) from zerver.lib.test_helpers import find_key_by_email, instrument_url +from zerver.lib.user_groups import get_system_user_group_for_user from zerver.lib.users import get_api_key from zerver.lib.validator import check_string from zerver.lib.webhooks.common import ( @@ -89,6 +90,7 @@ from zerver.models import ( Recipient, Stream, Subscription, + UserGroupMembership, UserMessage, UserProfile, UserStatus, @@ -1519,6 +1521,12 @@ Output: if count == 0: raise AssertionError("test is meaningless without any pertinent rows") + def check_user_added_in_system_group(self, user: UserProfile) -> None: + user_group = get_system_user_group_for_user(user) + self.assertTrue( + UserGroupMembership.objects.filter(user_profile=user, user_group=user_group).exists() + ) + class WebhookTestCase(ZulipTestCase): """Shared test class for all incoming webhooks tests. diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index 97cd47bd1d..eedf1ea7d2 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -129,6 +129,10 @@ def get_recursive_membership_groups(user_profile: UserProfile) -> "QuerySet[User def create_system_user_groups_for_realm(realm: Realm) -> Dict[int, UserGroup]: + """Any changes to this function likely require a migration to adjust + existing realms. See e.g. migration 0375_create_role_based_system_groups.py, + which is a copy of this function from when we introduced system groups. + """ role_system_groups_dict: Dict[int, UserGroup] = {} for role in UserGroup.SYSTEM_USER_GROUP_ROLE_MAP.keys(): user_group_params = UserGroup.SYSTEM_USER_GROUP_ROLE_MAP[role] @@ -174,3 +178,12 @@ def create_system_user_groups_for_realm(realm: Realm) -> Dict[int, UserGroup]: GroupGroupMembership.objects.bulk_create(subgroup_objects) return role_system_groups_dict + + +def get_system_user_group_for_user(user_profile: UserProfile) -> UserGroup: + system_user_group_name = UserGroup.SYSTEM_USER_GROUP_ROLE_MAP[user_profile.role]["name"] + + system_user_group = UserGroup.objects.get( + name=system_user_group_name, realm=user_profile.realm, is_system_group=True + ) + return system_user_group diff --git a/zerver/migrations/0382_create_role_based_system_groups.py b/zerver/migrations/0382_create_role_based_system_groups.py new file mode 100644 index 0000000000..1120bb7c1c --- /dev/null +++ b/zerver/migrations/0382_create_role_based_system_groups.py @@ -0,0 +1,144 @@ +# Generated by Django 3.2.7 on 2021-10-16 12:41 + +from django.db import migrations, transaction +from django.db.backends.postgresql.schema import DatabaseSchemaEditor +from django.db.migrations.state import StateApps +from django.utils.timezone import now as timezone_now + + +def create_role_based_system_groups(apps: StateApps, schema_editor: DatabaseSchemaEditor) -> None: + Realm = apps.get_model("zerver", "Realm") + UserProfile = apps.get_model("zerver", "UserProfile") + UserGroup = apps.get_model("zerver", "UserGroup") + GroupGroupMembership = apps.get_model("zerver", "GroupGroupMembership") + UserGroupMembership = apps.get_model("zerver", "UserGroupMembership") + + UserProfile.ROLE_REALM_OWNER = 100 + UserProfile.ROLE_REALM_ADMINISTRATOR = 200 + UserProfile.ROLE_MODERATOR = 300 + UserProfile.ROLE_MEMBER = 400 + UserProfile.ROLE_GUEST = 600 + + realms = Realm.objects.all() + + SYSTEM_USER_GROUP_ROLE_MAP = { + UserProfile.ROLE_REALM_OWNER: { + "name": "@role:owners", + "description": "Owners of this organization", + }, + UserProfile.ROLE_REALM_ADMINISTRATOR: { + "name": "@role:administrators", + "description": "Administrators of this organization, including owners", + }, + UserProfile.ROLE_MODERATOR: { + "name": "@role:moderators", + "description": "Moderators of this organization, including administrators", + }, + UserProfile.ROLE_MEMBER: { + "name": "@role:members", + "description": "Members of this organization, not including guests", + }, + UserProfile.ROLE_GUEST: { + "name": "@role:everyone", + "description": "Everyone in this organization, including guests", + }, + } + + for realm in realms: + with transaction.atomic(): + if UserGroup.objects.filter( + realm=realm, name="@role:internet", is_system_group=True + ).exists(): + # Handle the case where we are rerunning after a + # failure, and had already processed this realm. + continue + + role_system_groups_dict = {} + for role in SYSTEM_USER_GROUP_ROLE_MAP.keys(): + user_group_params = SYSTEM_USER_GROUP_ROLE_MAP[role] + user_group = UserGroup( + name=user_group_params["name"], + description=user_group_params["description"], + realm=realm, + is_system_group=True, + ) + role_system_groups_dict[role] = user_group + + full_members_system_group = UserGroup( + name="@role:fullmembers", + description="Members of this organization, not including new accounts and guests", + realm=realm, + is_system_group=True, + ) + everyone_on_internet_system_group = UserGroup( + name="@role:internet", + description="Everyone on the Internet", + realm=realm, + is_system_group=True, + ) + + system_user_groups_list = [ + role_system_groups_dict[UserProfile.ROLE_REALM_OWNER], + role_system_groups_dict[UserProfile.ROLE_REALM_ADMINISTRATOR], + role_system_groups_dict[UserProfile.ROLE_MODERATOR], + full_members_system_group, + role_system_groups_dict[UserProfile.ROLE_MEMBER], + role_system_groups_dict[UserProfile.ROLE_GUEST], + everyone_on_internet_system_group, + ] + + UserGroup.objects.bulk_create(system_user_groups_list) + + subgroup_objects = [] + subgroup, remaining_groups = system_user_groups_list[0], system_user_groups_list[1:] + for supergroup in remaining_groups: + subgroup_objects.append( + GroupGroupMembership(subgroup=subgroup, supergroup=supergroup) + ) + subgroup = supergroup + + GroupGroupMembership.objects.bulk_create(subgroup_objects) + + users = UserProfile.objects.filter(realm=realm).only("id", "role", "date_joined") + group_membership_objects = [] + for user in users: + system_group = role_system_groups_dict[user.role] + group_membership_objects.append( + UserGroupMembership(user_profile=user, user_group=system_group) + ) + + if ( + user.role == UserProfile.ROLE_MEMBER + and (timezone_now() - user.date_joined).days >= realm.waiting_period_threshold + ): + group_membership_objects.append( + UserGroupMembership(user_profile=user, user_group=full_members_system_group) + ) + + UserGroupMembership.objects.bulk_create(group_membership_objects) + + +def delete_role_based_system_groups(apps: StateApps, schema_editor: DatabaseSchemaEditor) -> None: + UserGroup = apps.get_model("zerver", "UserGroup") + GroupGroupMembership = apps.get_model("zerver", "GroupGroupMembership") + UserGroupMembership = apps.get_model("zerver", "UserGroupMembership") + with transaction.atomic(): + GroupGroupMembership.objects.all().delete() + UserGroupMembership.objects.filter(user_group__is_system_group=True).delete() + UserGroup.objects.filter(is_system_group=True).delete() + + +class Migration(migrations.Migration): + atomic = False + + dependencies = [ + ("zerver", "0381_alter_userprofile_uuid"), + ] + + operations = [ + migrations.RunPython( + create_role_based_system_groups, + reverse_code=delete_role_based_system_groups, + elidable=True, + ), + ] diff --git a/zerver/tests/test_bots.py b/zerver/tests/test_bots.py index 3cc78f8674..e998cf1cbf 100644 --- a/zerver/tests/test_bots.py +++ b/zerver/tests/test_bots.py @@ -166,7 +166,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): self.login("hamlet") self.assert_num_bots_equal(0) events: List[Mapping[str, Any]] = [] - with self.tornado_redirected_to_list(events, expected_num_events=2): + with self.tornado_redirected_to_list(events, expected_num_events=4): result = self.create_bot() self.assert_num_bots_equal(1) @@ -332,7 +332,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): self.login_user(user) self.assert_num_bots_equal(0) events: List[Mapping[str, Any]] = [] - with self.tornado_redirected_to_list(events, expected_num_events=2): + with self.tornado_redirected_to_list(events, expected_num_events=4): result = self.create_bot() self.assert_num_bots_equal(1) @@ -424,7 +424,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): self.assert_num_bots_equal(0) events: List[Mapping[str, Any]] = [] - with self.tornado_redirected_to_list(events, expected_num_events=2): + with self.tornado_redirected_to_list(events, expected_num_events=4): result = self.create_bot(default_sending_stream="Denmark") self.assert_num_bots_equal(1) self.assertEqual(result["default_sending_stream"], "Denmark") @@ -498,7 +498,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): self.assert_num_bots_equal(0) events: List[Mapping[str, Any]] = [] - with self.tornado_redirected_to_list(events, expected_num_events=2): + with self.tornado_redirected_to_list(events, expected_num_events=4): result = self.create_bot(default_events_register_stream="Denmark") self.assert_num_bots_equal(1) self.assertEqual(result["default_events_register_stream"], "Denmark") diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 09e559e0bb..a12bc3eeb8 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -841,7 +841,7 @@ class NormalActionsTest(BaseAction): acting_user=None, ), state_change_expected=True, - num_events=5, + num_events=7, ) check_invites_changed("events[3]", events[3]) @@ -1029,8 +1029,8 @@ class NormalActionsTest(BaseAction): ) def test_register_events(self) -> None: - events = self.verify_action(lambda: self.register("test1@zulip.com", "test1"), num_events=3) - self.assert_length(events, 3) + events = self.verify_action(lambda: self.register("test1@zulip.com", "test1"), num_events=5) + self.assert_length(events, 5) check_realm_user_add("events[0]", events[0]) new_user_profile = get_user_by_delivery_email("test1@zulip.com", self.user_profile.realm) @@ -1044,6 +1044,9 @@ class NormalActionsTest(BaseAction): events[2]["message"]["content"], ) + check_user_group_add_members("events[3]", events[3]) + check_user_group_add_members("events[4]", events[4]) + def test_register_events_email_address_visibility(self) -> None: do_set_realm_property( self.user_profile.realm, @@ -1052,8 +1055,8 @@ class NormalActionsTest(BaseAction): acting_user=None, ) - events = self.verify_action(lambda: self.register("test1@zulip.com", "test1"), num_events=3) - self.assert_length(events, 3) + events = self.verify_action(lambda: self.register("test1@zulip.com", "test1"), num_events=5) + self.assert_length(events, 5) check_realm_user_add("events[0]", events[0]) new_user_profile = get_user_by_delivery_email("test1@zulip.com", self.user_profile.realm) self.assertEqual(new_user_profile.email, f"user{new_user_profile.id}@zulip.testserver") @@ -1066,6 +1069,9 @@ class NormalActionsTest(BaseAction): events[2]["message"]["content"], ) + check_user_group_add_members("events[3]", events[3]) + check_user_group_add_members("events[4]", events[4]) + def test_alert_words_events(self) -> None: events = self.verify_action(lambda: do_add_alert_words(self.user_profile, ["alert_word"])) check_alert_words("events[0]", events[0]) @@ -1491,11 +1497,20 @@ class NormalActionsTest(BaseAction): do_change_user_role(self.user_profile, UserProfile.ROLE_MEMBER, acting_user=None) for role in [UserProfile.ROLE_REALM_ADMINISTRATOR, UserProfile.ROLE_MEMBER]: events = self.verify_action( - lambda: do_change_user_role(self.user_profile, role, acting_user=None) + lambda: do_change_user_role(self.user_profile, role, acting_user=None), + num_events=4, ) check_realm_user_update("events[0]", events[0], "role") self.assertEqual(events[0]["person"]["role"], role) + check_user_group_remove_members("events[1]", events[1]) + check_user_group_add_members("events[2]", events[2]) + + if role == UserProfile.ROLE_REALM_ADMINISTRATOR: + check_user_group_remove_members("events[3]", events[3]) + else: + check_user_group_add_members("events[3]", events[3]) + def test_change_is_billing_admin(self) -> None: reset_emails_in_zulip_realm() @@ -1519,11 +1534,20 @@ class NormalActionsTest(BaseAction): do_change_user_role(self.user_profile, UserProfile.ROLE_MEMBER, acting_user=None) for role in [UserProfile.ROLE_REALM_OWNER, UserProfile.ROLE_MEMBER]: events = self.verify_action( - lambda: do_change_user_role(self.user_profile, role, acting_user=None) + lambda: do_change_user_role(self.user_profile, role, acting_user=None), + num_events=4, ) check_realm_user_update("events[0]", events[0], "role") self.assertEqual(events[0]["person"]["role"], role) + check_user_group_remove_members("events[1]", events[1]) + check_user_group_add_members("events[2]", events[2]) + + if role == UserProfile.ROLE_REALM_OWNER: + check_user_group_remove_members("events[3]", events[3]) + else: + check_user_group_add_members("events[3]", events[3]) + def test_change_is_moderator(self) -> None: reset_emails_in_zulip_realm() @@ -1535,11 +1559,20 @@ class NormalActionsTest(BaseAction): do_change_user_role(self.user_profile, UserProfile.ROLE_MEMBER, acting_user=None) for role in [UserProfile.ROLE_MODERATOR, UserProfile.ROLE_MEMBER]: events = self.verify_action( - lambda: do_change_user_role(self.user_profile, role, acting_user=None) + lambda: do_change_user_role(self.user_profile, role, acting_user=None), + num_events=4, ) check_realm_user_update("events[0]", events[0], "role") self.assertEqual(events[0]["person"]["role"], role) + check_user_group_remove_members("events[1]", events[1]) + check_user_group_add_members("events[2]", events[2]) + + if role == UserProfile.ROLE_MODERATOR: + check_user_group_remove_members("events[3]", events[3]) + else: + check_user_group_add_members("events[3]", events[3]) + def test_change_is_guest(self) -> None: stream = Stream.objects.get(name="Denmark") do_add_default_stream(stream) @@ -1554,11 +1587,20 @@ class NormalActionsTest(BaseAction): do_change_user_role(self.user_profile, UserProfile.ROLE_MEMBER, acting_user=None) for role in [UserProfile.ROLE_GUEST, UserProfile.ROLE_MEMBER]: events = self.verify_action( - lambda: do_change_user_role(self.user_profile, role, acting_user=None) + lambda: do_change_user_role(self.user_profile, role, acting_user=None), + num_events=4, ) check_realm_user_update("events[0]", events[0], "role") self.assertEqual(events[0]["person"]["role"], role) + check_user_group_remove_members("events[1]", events[1]) + check_user_group_add_members("events[2]", events[2]) + + if role == UserProfile.ROLE_GUEST: + check_user_group_remove_members("events[3]", events[3]) + else: + check_user_group_add_members("events[3]", events[3]) + def test_change_notification_settings(self) -> None: for notification_setting, v in self.user_profile.notification_setting_types.items(): if notification_setting in ["notification_sound", "desktop_icon_count_display"]: @@ -1734,7 +1776,7 @@ class NormalActionsTest(BaseAction): def test_create_bot(self) -> None: action = lambda: self.create_bot("test") - events = self.verify_action(action, num_events=2) + events = self.verify_action(action, num_events=4) check_realm_bot_add("events[1]", events[1]) action = lambda: self.create_bot( @@ -1744,7 +1786,7 @@ class NormalActionsTest(BaseAction): interface_type=Service.GENERIC, bot_type=UserProfile.OUTGOING_WEBHOOK_BOT, ) - events = self.verify_action(action, num_events=2) + events = self.verify_action(action, num_events=4) # The third event is the second call of notify_created_bot, which contains additional # data for services (in contrast to the first call). check_realm_bot_add("events[1]", events[1]) @@ -1756,7 +1798,7 @@ class NormalActionsTest(BaseAction): config_data=orjson.dumps({"foo": "bar"}).decode(), bot_type=UserProfile.EMBEDDED_BOT, ) - events = self.verify_action(action, num_events=2) + events = self.verify_action(action, num_events=4) check_realm_bot_add("events[1]", events[1]) def test_change_bot_full_name(self) -> None: diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index a141e2b6c7..fc92577bc1 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -867,19 +867,21 @@ class LoginTest(ZulipTestCase): ContentType.objects.clear_cache() with queries_captured() as queries, cache_tries_captured() as cache_tries: - self.register(self.nonreg_email("test"), "test") + with self.captureOnCommitCallbacks(execute=True): + self.register(self.nonreg_email("test"), "test") # Ensure the number of queries we make is not O(streams) - self.assert_length(queries, 89) + self.assert_length(queries, 95) # We can probably avoid a couple cache hits here, but there doesn't # seem to be any O(N) behavior. Some of the cache hits are related # to sending messages, such as getting the welcome bot, looking up # the alert words for a realm, etc. - self.assert_length(cache_tries, 21) + self.assert_length(cache_tries, 23) user_profile = self.nonreg_user("test") self.assert_logged_in_user_id(user_profile.id) self.assertFalse(user_profile.enable_stream_desktop_notifications) + self.check_user_added_in_system_group(user_profile) def test_register_deactivated(self) -> None: """ @@ -1325,6 +1327,7 @@ class InviteUserTest(InviteUserBase): invitee_profile = self.nonreg_user("alice") self.assertTrue(invitee_profile.is_realm_owner) self.assertFalse(invitee_profile.is_guest) + self.check_user_added_in_system_group(invitee_profile) def test_invite_user_as_owner_from_admin_account(self) -> None: self.login("iago") @@ -1348,6 +1351,7 @@ class InviteUserTest(InviteUserBase): self.assertTrue(invitee_profile.is_realm_admin) self.assertFalse(invitee_profile.is_realm_owner) self.assertFalse(invitee_profile.is_guest) + self.check_user_added_in_system_group(invitee_profile) def test_invite_user_as_admin_from_normal_account(self) -> None: self.login("hamlet") @@ -1371,6 +1375,7 @@ class InviteUserTest(InviteUserBase): self.assertFalse(invitee_profile.is_realm_admin) self.assertTrue(invitee_profile.is_moderator) self.assertFalse(invitee_profile.is_guest) + self.check_user_added_in_system_group(invitee_profile) def test_invite_user_as_moderator_from_normal_account(self) -> None: self.login("hamlet") @@ -1410,6 +1415,7 @@ class InviteUserTest(InviteUserBase): invitee_profile = self.nonreg_user("alice") self.assertFalse(invitee_profile.is_realm_admin) self.assertTrue(invitee_profile.is_guest) + self.check_user_added_in_system_group(invitee_profile) def test_successful_invite_user_as_guest_from_admin_account(self) -> None: self.login("iago") @@ -1423,6 +1429,7 @@ class InviteUserTest(InviteUserBase): invitee_profile = self.nonreg_user("alice") self.assertFalse(invitee_profile.is_realm_admin) self.assertTrue(invitee_profile.is_guest) + self.check_user_added_in_system_group(invitee_profile) def test_successful_invite_user_with_name(self) -> None: """ diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index 12a1bb9a35..d5b92169eb 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -57,8 +57,10 @@ class UserGroupTestCase(ZulipTestCase): othello = self.example_user("othello") self.create_user_group_for_test("support") user_groups = get_direct_user_groups(othello) - self.assert_length(user_groups, 1) - self.assertEqual(user_groups[0].name, "support") + self.assert_length(user_groups, 3) + # othello is a direct member of two role-based system groups also. + user_group_names = [group.name for group in user_groups] + self.assertEqual(set(user_group_names), {"support", "@role:members", "@role:fullmembers"}) def test_recursive_queries_for_user_groups(self) -> None: realm = get_realm("zulip") @@ -92,14 +94,14 @@ class UserGroupTestCase(ZulipTestCase): list(get_recursive_group_members(everyone_group)), [desdemona, iago, shiva] ) - self.assertCountEqual( - list(get_recursive_membership_groups(desdemona)), - [leadership_group, staff_group, everyone_group], - ) - self.assertCountEqual( - list(get_recursive_membership_groups(iago)), [staff_group, everyone_group] - ) - self.assertCountEqual(list(get_recursive_membership_groups(shiva)), [everyone_group]) + self.assertIn(leadership_group, list(get_recursive_membership_groups(desdemona))) + self.assertIn(staff_group, list(get_recursive_membership_groups(desdemona))) + self.assertIn(everyone_group, list(get_recursive_membership_groups(desdemona))) + + self.assertIn(staff_group, list(get_recursive_membership_groups(iago))) + self.assertIn(everyone_group, list(get_recursive_membership_groups(iago))) + + self.assertIn(everyone_group, list(get_recursive_membership_groups(shiva))) def test_subgroups_of_role_based_system_groups(self) -> None: realm = get_realm("zulip") @@ -268,11 +270,11 @@ class UserGroupAPITestCase(UserGroupTestCase): user_group = UserGroup.objects.get(name="support") # Test success self.assertEqual(UserGroup.objects.filter(realm=hamlet.realm).count(), 9) - self.assertEqual(UserGroupMembership.objects.count(), 3) + self.assertEqual(UserGroupMembership.objects.count(), 19) result = self.client_delete(f"/json/user_groups/{user_group.id}") self.assert_json_success(result) self.assertEqual(UserGroup.objects.filter(realm=hamlet.realm).count(), 8) - self.assertEqual(UserGroupMembership.objects.count(), 2) + self.assertEqual(UserGroupMembership.objects.count(), 18) # Test when invalid user group is supplied result = self.client_delete("/json/user_groups/1111") self.assert_json_error(result, "Invalid user group") diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index 05fb66f24f..18fa141238 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -46,6 +46,7 @@ from zerver.lib.test_helpers import ( simulated_empty_cache, ) from zerver.lib.upload import upload_avatar_image +from zerver.lib.user_groups import get_system_user_group_for_user from zerver.lib.user_topics import add_topic_mute from zerver.lib.users import Accounts, access_user_by_id, get_accounts_for_email, user_ids_to_users from zerver.lib.utils import assert_is_not_none @@ -61,6 +62,7 @@ from zerver.models import ( ScheduledEmail, Stream, Subscription, + UserGroupMembership, UserHotspot, UserProfile, check_valid_user_ids, @@ -213,7 +215,7 @@ class PermissionTest(ZulipTestCase): req = dict(role=UserProfile.ROLE_REALM_OWNER) events: List[Mapping[str, Any]] = [] - with self.tornado_redirected_to_list(events, expected_num_events=1): + with self.tornado_redirected_to_list(events, expected_num_events=4): result = self.client_patch(f"/json/users/{othello.id}", req) self.assert_json_success(result) owner_users = realm.get_human_owner_users() @@ -223,7 +225,7 @@ class PermissionTest(ZulipTestCase): self.assertEqual(person["role"], UserProfile.ROLE_REALM_OWNER) req = dict(role=UserProfile.ROLE_MEMBER) - with self.tornado_redirected_to_list(events, expected_num_events=1): + with self.tornado_redirected_to_list(events, expected_num_events=4): result = self.client_patch(f"/json/users/{othello.id}", req) self.assert_json_success(result) owner_users = realm.get_human_owner_users() @@ -235,7 +237,7 @@ class PermissionTest(ZulipTestCase): # Cannot take away from last owner self.login("desdemona") req = dict(role=UserProfile.ROLE_MEMBER) - with self.tornado_redirected_to_list(events, expected_num_events=1): + with self.tornado_redirected_to_list(events, expected_num_events=4): result = self.client_patch(f"/json/users/{iago.id}", req) self.assert_json_success(result) owner_users = realm.get_human_owner_users() @@ -276,7 +278,7 @@ class PermissionTest(ZulipTestCase): req = dict(role=orjson.dumps(UserProfile.ROLE_REALM_ADMINISTRATOR).decode()) events: List[Mapping[str, Any]] = [] - with self.tornado_redirected_to_list(events, expected_num_events=1): + with self.tornado_redirected_to_list(events, expected_num_events=4): result = self.client_patch(f"/json/users/{othello.id}", req) self.assert_json_success(result) admin_users = realm.get_human_admin_users() @@ -287,7 +289,7 @@ class PermissionTest(ZulipTestCase): # Taketh away req = dict(role=orjson.dumps(UserProfile.ROLE_MEMBER).decode()) - with self.tornado_redirected_to_list(events, expected_num_events=1): + with self.tornado_redirected_to_list(events, expected_num_events=4): result = self.client_patch(f"/json/users/{othello.id}", req) self.assert_json_success(result) admin_users = realm.get_human_admin_users() @@ -520,17 +522,32 @@ class PermissionTest(ZulipTestCase): user_profile = self.example_user(user_email) old_role = user_profile.role + old_system_group = get_system_user_group_for_user(user_profile) self.assertTrue(self.check_property_for_role(user_profile, old_role)) + self.assertTrue( + UserGroupMembership.objects.filter( + user_profile=user_profile, user_group=old_system_group + ).exists() + ) req = dict(role=orjson.dumps(new_role).decode()) events: List[Mapping[str, Any]] = [] - with self.tornado_redirected_to_list(events, expected_num_events=1): + num_events = 3 + if UserProfile.ROLE_MEMBER in [old_role, new_role]: + num_events = 4 + with self.tornado_redirected_to_list(events, expected_num_events=num_events): result = self.client_patch(f"/json/users/{user_profile.id}", req) self.assert_json_success(result) user_profile = self.example_user(user_email) self.assertTrue(self.check_property_for_role(user_profile, new_role)) + system_group = get_system_user_group_for_user(user_profile) + self.assertTrue( + UserGroupMembership.objects.filter( + user_profile=user_profile, user_group=system_group + ).exists() + ) person = events[0]["event"]["person"] self.assertEqual(person["user_id"], user_profile.id) @@ -793,7 +810,7 @@ class QueryCountTest(ZulipTestCase): with queries_captured() as queries: with cache_tries_captured() as cache_tries: - with self.tornado_redirected_to_list(events, expected_num_events=8): + with self.tornado_redirected_to_list(events, expected_num_events=10): fred = do_create_user( email="fred@zulip.com", password="password", @@ -803,8 +820,8 @@ class QueryCountTest(ZulipTestCase): acting_user=None, ) - self.assert_length(queries, 84) - self.assert_length(cache_tries, 27) + self.assert_length(queries, 90) + self.assert_length(cache_tries, 29) peer_add_events = [event for event in events if event["event"].get("op") == "peer_add"] diff --git a/zilencer/management/commands/populate_db.py b/zilencer/management/commands/populate_db.py index cf7dde2c16..4e28b41602 100644 --- a/zilencer/management/commands/populate_db.py +++ b/zilencer/management/commands/populate_db.py @@ -57,6 +57,8 @@ from zerver.models import ( Service, Stream, Subscription, + UserGroup, + UserGroupMembership, UserMessage, UserPresence, UserProfile, @@ -486,6 +488,25 @@ class Command(BaseCommand): assign_time_zone_by_delivery_email("shiva@zulip.com", "Asia/Kolkata") # India assign_time_zone_by_delivery_email("cordelia@zulip.com", "UTC") + users = UserProfile.objects.filter(realm=zulip_realm) + # All users in development environment are full members initially because + # waiting period threshold is 0. Groups of Iago, Dedemona, Shiva and + # Polonius will be updated according to their role in do_change_user_role. + full_members_user_group = UserGroup.objects.get( + realm=zulip_realm, name="@role:fullmembers", is_system_group=True + ) + members_user_group = UserGroup.objects.get( + realm=zulip_realm, name="@role:members", is_system_group=True + ) + user_group_memberships = [] + for user_profile in list(users): + for group in [full_members_user_group, members_user_group]: + user_group_membership = UserGroupMembership( + user_group=group, user_profile=user_profile + ) + user_group_memberships.append(user_group_membership) + UserGroupMembership.objects.bulk_create(user_group_memberships) + iago = get_user_by_delivery_email("iago@zulip.com", zulip_realm) do_change_user_role(iago, UserProfile.ROLE_REALM_ADMINISTRATOR, acting_user=None) iago.is_staff = True