Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

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.
This commit is contained in:
Shubham Padia 2024-10-10 11:35:44 +00:00 committed by Tim Abbott
parent 9bbd6a7316
commit c9d5276031
6 changed files with 87 additions and 64 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
help/manage-user-groups.md
View File

@ -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).
<!-- TODO: Remove this after #25942 is resolved and we've removed
the condition that only members can manage the group if they are
not admins or moderators. -->
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}

12
web/src/settings_data.ts
View File

@ -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;
}

4
web/tests/settings_data.test.js
View File

@ -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));

30
zerver/lib/user_groups.py
View File

@ -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(

56
zerver/migrations/0602_remap_can_manage_all_groups.py Normal file
View File

@ -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,
)
]

42
zerver/tests/test_user_groups.py
View File

@ -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)