From c9d527603164b580c3ba96cbb6eea32b7c61991f Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Thu, 10 Oct 2024 11:35:44 +0000 Subject: [PATCH] user_groups: Set can_manage_all_groups to administrator group. Earlier we use to restrict admins, moderators or members of a group to manage that group if they were part of the realm wide `can_manage_all_groups`. We will not do that anymore and even non-members of a group regardless of role can manage a group if they are part of `can_manage_all_groups`. See https://chat.zulip.org/#narrow/stream/101-design/topic/Group.20add.20members.20dropdown/near/1952902 to check more about the migration plan for which this is the last step. --- help/manage-user-groups.md | 7 +-- web/src/settings_data.ts | 12 +--- web/tests/settings_data.test.js | 4 +- zerver/lib/user_groups.py | 30 +++------- .../0602_remap_can_manage_all_groups.py | 56 +++++++++++++++++++ zerver/tests/test_user_groups.py | 42 ++++++-------- 6 files changed, 87 insertions(+), 64 deletions(-) create mode 100644 zerver/migrations/0602_remap_can_manage_all_groups.py diff --git a/help/manage-user-groups.md b/help/manage-user-groups.md index a1a62f9248..62e1bc4673 100644 --- a/help/manage-user-groups.md +++ b/help/manage-user-groups.md @@ -142,12 +142,7 @@ By default, [owners](/help/roles-and-permissions) in a Zulip organization can manage user groups. However, you can expand that ability to specific [roles](/help/roles-and-permissions). - -Note that administrators and moderators can modify any user group, -while other organization members can only modify user groups to which -they belong. Guests cannot modify user groups. +Guests cannot modify user groups. {start_tabs} diff --git a/web/src/settings_data.ts b/web/src/settings_data.ts index b4bbfdfd97..e338236c1f 100644 --- a/web/src/settings_data.ts +++ b/web/src/settings_data.ts @@ -192,19 +192,9 @@ export function can_manage_user_group(group_id: number): boolean { return false; } - let can_manage_all_groups = user_can_manage_all_groups(); - const group = user_groups.get_user_group_from_id(group_id); - if ( - !current_user.is_admin && - !current_user.is_moderator && - !user_groups.is_direct_member_of(current_user.user_id, group_id) - ) { - can_manage_all_groups = false; - } - - if (can_manage_all_groups) { + if (user_can_manage_all_groups()) { return true; } diff --git a/web/tests/settings_data.test.js b/web/tests/settings_data.test.js index b1613bdfc6..5f986efbaf 100644 --- a/web/tests/settings_data.test.js +++ b/web/tests/settings_data.test.js @@ -443,10 +443,12 @@ run_test("can_manage_user_group", ({override}) => { override(current_user, "user_id", 2); assert.ok(!settings_data.can_manage_user_group(students.id)); + // User with role member and not part of the group. override(realm, "realm_can_manage_all_groups", members.id); override(current_user, "user_id", 3); - assert.ok(!settings_data.can_manage_user_group(students.id)); + assert.ok(settings_data.can_manage_user_group(students.id)); + // User with role member and part of the group. override(current_user, "user_id", 2); assert.ok(settings_data.can_manage_user_group(students.id)); diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index ffadfa0166..0ef7defd1b 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -132,24 +132,6 @@ def access_user_group_to_read_membership(user_group_id: int, realm: Realm) -> Na return get_user_group_by_id_in_realm(user_group_id, realm, for_read=True) -def check_permission_for_managing_all_groups( - user_group: UserGroup, user_profile: UserProfile -) -> bool: - """ - Given a user and a group in the same realm, checks if the user - can manage the group through the legacy can_manage_all_groups - permission, which is a permission that requires either certain roles - or membership in the group itself to be used. - """ - can_manage_all_groups = user_profile.can_manage_all_groups() - if can_manage_all_groups: - if user_profile.is_realm_admin or user_profile.is_moderator: - return True - - return is_user_in_group(user_group, user_profile) - return False - - def access_user_group_for_update( user_group_id: int, user_profile: UserProfile, @@ -173,10 +155,14 @@ def access_user_group_for_update( raise JsonableError(_("Insufficient permission")) assert permission_setting in NamedUserGroup.GROUP_PERMISSION_SETTINGS - if permission_setting in [ - "can_manage_group", - "can_add_members_group", - ] and check_permission_for_managing_all_groups(user_group, user_profile): + if ( + permission_setting + in [ + "can_manage_group", + "can_add_members_group", + ] + and user_profile.can_manage_all_groups() + ): return user_group user_has_permission = user_has_permission_for_group_setting( diff --git a/zerver/migrations/0602_remap_can_manage_all_groups.py b/zerver/migrations/0602_remap_can_manage_all_groups.py new file mode 100644 index 0000000000..a9a6a87410 --- /dev/null +++ b/zerver/migrations/0602_remap_can_manage_all_groups.py @@ -0,0 +1,56 @@ +# Generated by Django 5.0.9 on 2024-10-10 10:41 + +from django.db import migrations, transaction +from django.db.backends.base.schema import BaseDatabaseSchemaEditor +from django.db.migrations.state import StateApps +from django.db.models import Max, Min, OuterRef + + +def remap_can_manage_all_groups_for_existing_realms( + apps: StateApps, schema_editor: BaseDatabaseSchemaEditor +) -> None: + Realm = apps.get_model("zerver", "Realm") + NamedUserGroup = apps.get_model("zerver", "NamedUserGroup") + BATCH_SIZE = 1000 + max_id = NamedUserGroup.objects.aggregate(Max("id"))["id__max"] + + if max_id is None: + # Do nothing if there are no user groups on the server. + return + + lower_bound = NamedUserGroup.objects.aggregate(Min("id"))["id__min"] + + while lower_bound <= max_id: + upper_bound = lower_bound + BATCH_SIZE - 1 + print(f"Processing batch {lower_bound} to {upper_bound} for NamedUserGroup") + + with transaction.atomic(): + # Since can_manage_group, can_add_members_group, etc. have + # migrated to the nearest possible value from + # user_group_edit_policy, we want to set + # can_manage_all_groups to the most restrictive setting + # previously possible. We've chosen administrators as the + # value here since the highest possible + # user_group_edit_policy was with role administrators. + Realm.objects.update( + can_manage_all_groups=NamedUserGroup.objects.filter( + name="role:administrators", realm=OuterRef("id"), is_system_group=True + ).values("pk") + ) + + lower_bound += BATCH_SIZE + + +class Migration(migrations.Migration): + atomic = False + dependencies = [ + ("zerver", "0601_alter_namedusergroup_can_add_members_group"), + ] + + operations = [ + migrations.RunPython( + remap_can_manage_all_groups_for_existing_realms, + elidable=True, + reverse_code=migrations.RunPython.noop, + ) + ] diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index b26e84f601..89d9004d83 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -1291,10 +1291,6 @@ class UserGroupAPITestCase(UserGroupTestCase): acting_user=None, ) - self.login("hamlet") - result = self.client_post(f"/json/user_groups/{support_group.id}/deactivate") - self.assert_json_error(result, "Insufficient permission") - self.login("othello") result = self.client_post(f"/json/user_groups/{support_group.id}/deactivate") self.assert_json_success(result) @@ -1997,8 +1993,7 @@ class UserGroupAPITestCase(UserGroupTestCase): check_update_user_group("support1", "Support team - test", "iago") check_update_user_group("support", "Support team", "othello") - # Check only members are allowed to update the user group and only if belong to the - # user group. + # Check only members are allowed to update the user group. members_group = NamedUserGroup.objects.get(name=SystemGroups.MEMBERS, realm=realm) do_change_realm_permission_group_setting( realm, @@ -2009,13 +2004,7 @@ class UserGroupAPITestCase(UserGroupTestCase): check_update_user_group( "help", "Troubleshooting team", "polonius", "Not allowed for guest users" ) - check_update_user_group( - "help", - "Troubleshooting team", - "cordelia", - "Insufficient permission", - ) - check_update_user_group("help", "Troubleshooting team", "othello") + check_update_user_group("help", "Troubleshooting team", "cordelia") # Check user who is member of a subgroup of the group being updated # can also update the group. @@ -2181,8 +2170,7 @@ class UserGroupAPITestCase(UserGroupTestCase): self.assert_json_error(result, error_msg) realm = get_realm("zulip") - # Check only admins are allowed to add/remove users from the group. Admins are allowed even if - # they are not a member of the group. + # Check only admins are allowed to add/remove users from the group. admins_group = NamedUserGroup.objects.get(name=SystemGroups.ADMINISTRATORS, realm=realm) do_change_realm_permission_group_setting( realm, @@ -2196,8 +2184,7 @@ class UserGroupAPITestCase(UserGroupTestCase): check_removing_members_from_group("shiva", "Insufficient permission") check_removing_members_from_group("iago") - # Check moderators are allowed to add/remove users from the group but not members. Moderators are - # allowed even if they are not a member of the group. + # Check moderators are allowed to add/remove users from the group but not members. moderators_group = NamedUserGroup.objects.get(name=SystemGroups.MODERATORS, realm=realm) do_change_realm_permission_group_setting( realm, @@ -2258,15 +2245,18 @@ class UserGroupAPITestCase(UserGroupTestCase): acting_user=None, ) check_adding_members_to_group("polonius", "Not allowed for guest users") - check_adding_members_to_group("cordelia", "Insufficient permission") - check_adding_members_to_group("othello") + # User with role member but not part of the target group should + # be allowed to add members to the group if they are part of + # `can_manage_all_groups`. + check_adding_members_to_group("cordelia") + check_removing_members_from_group("cordelia") + + check_adding_members_to_group("othello") check_removing_members_from_group("polonius", "Not allowed for guest users") - check_removing_members_from_group("cordelia", "Insufficient permission") check_removing_members_from_group("othello") - # Check only full members are allowed to add/remove users in the group and only if belong to the - # user group. + # Check only full members are allowed to add/remove users in the group. full_members_group = NamedUserGroup.objects.get(name=SystemGroups.FULL_MEMBERS, realm=realm) do_change_realm_permission_group_setting( realm, @@ -2284,7 +2274,12 @@ class UserGroupAPITestCase(UserGroupTestCase): cordelia.date_joined = timezone_now() - timedelta(days=11) cordelia.save() promote_new_full_members() - check_adding_members_to_group("cordelia", "Insufficient permission") + + # Full members who are not part of the target group should + # be allowed to add members to the group if they are part of + # `can_manage_all_groups`. + check_adding_members_to_group("cordelia") + check_removing_members_from_group("cordelia") othello.date_joined = timezone_now() - timedelta(days=11) othello.save() @@ -2295,7 +2290,6 @@ class UserGroupAPITestCase(UserGroupTestCase): othello.save() promote_new_full_members() - check_removing_members_from_group("cordelia", "Insufficient permission") check_removing_members_from_group("othello", "Insufficient permission") othello.date_joined = timezone_now() - timedelta(days=11)