diff --git a/api_docs/changelog.md b/api_docs/changelog.md index eb4623f11f..3889ff3a0b 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,18 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 10.0 +**Feature level 305** + +* [`POST /register`](/api/register-queue), [`GET /events`](/api/get-events), + [`GET /user_groups`](/api/get-user-groups): Add `can_add_members_group` to + user group objects. +* [`POST /user_groups/create`](/api/create-user-group): Added `can_add_members_group` + parameter to support setting the user group which can add members to the user + group. +* [`PATCH /user_groups/{user_group_id}`](/api/update-user-group): Added + `can_add_members_group` parameter to support changing the user group which + can add members to the specified user group. + **Feature level 304** * [`GET /export/realm`](/api/get-realm-exports), diff --git a/zerver/actions/user_groups.py b/zerver/actions/user_groups.py index 6e0e3729d6..4bcca3437a 100644 --- a/zerver/actions/user_groups.py +++ b/zerver/actions/user_groups.py @@ -188,6 +188,7 @@ def do_send_create_user_group_event( id=user_group.id, is_system_group=user_group.is_system_group, direct_subgroup_ids=[direct_subgroup.id for direct_subgroup in direct_subgroups], + can_add_members_group=get_group_setting_value_for_api(user_group.can_add_members_group), can_join_group=get_group_setting_value_for_api(user_group.can_join_group), can_manage_group=get_group_setting_value_for_api(user_group.can_manage_group), can_mention_group=get_group_setting_value_for_api(user_group.can_mention_group), diff --git a/zerver/lib/event_schema.py b/zerver/lib/event_schema.py index e793651bbb..f6af8445e3 100644 --- a/zerver/lib/event_schema.py +++ b/zerver/lib/event_schema.py @@ -1848,6 +1848,7 @@ group_type = DictType( ("direct_subgroup_ids", ListType(int)), ("description", str), ("is_system_group", bool), + ("can_add_members_group", group_setting_type), ("can_join_group", group_setting_type), ("can_manage_group", group_setting_type), ("can_mention_group", group_setting_type), @@ -1898,6 +1899,7 @@ user_group_data_type = DictType( optional_keys=[ ("name", str), ("description", str), + ("can_add_members_group", group_setting_type), ("can_join_group", group_setting_type), ("can_manage_group", group_setting_type), ("can_mention_group", group_setting_type), diff --git a/zerver/lib/import_realm.py b/zerver/lib/import_realm.py index 92f1297a5a..83064ca041 100644 --- a/zerver/lib/import_realm.py +++ b/zerver/lib/import_realm.py @@ -757,6 +757,7 @@ def bulk_import_named_user_groups(data: TableData) -> None: group["name"], group["description"], group["is_system_group"], + group["can_add_members_group_id"], group["can_join_group_id"], group["can_manage_group_id"], group["can_mention_group_id"], @@ -768,7 +769,7 @@ def bulk_import_named_user_groups(data: TableData) -> None: query = SQL( """ - INSERT INTO zerver_namedusergroup (usergroup_ptr_id, realm_id, name, description, is_system_group, can_join_group_id, can_manage_group_id, can_mention_group_id, deactivated, date_created) + INSERT INTO zerver_namedusergroup (usergroup_ptr_id, realm_id, name, description, is_system_group, can_add_members_group_id, can_join_group_id, can_manage_group_id, can_mention_group_id, deactivated, date_created) VALUES %s """ ) diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index 2ef2a04e13..2dbc2cc840 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -55,6 +55,7 @@ class UserGroupDict(TypedDict): creator_id: int | None date_created: int | None is_system_group: bool + can_add_members_group: int | AnonymousSettingGroupDict can_join_group: int | AnonymousSettingGroupDict can_manage_group: int | AnonymousSettingGroupDict can_mention_group: int | AnonymousSettingGroupDict @@ -554,6 +555,8 @@ def user_groups_in_realm_serialized( UserGroup and UserGroupMembership that we need. """ realm_groups = NamedUserGroup.objects.select_related( + "can_add_members_group", + "can_add_members_group__named_user_group", "can_join_group", "can_join_group__named_user_group", "can_manage_group", @@ -610,6 +613,9 @@ def user_groups_in_realm_serialized( members=direct_member_ids, direct_subgroup_ids=direct_subgroup_ids, is_system_group=user_group.is_system_group, + can_add_members_group=get_setting_value_for_user_group_object( + user_group.can_add_members_group, group_members, group_subgroups + ), can_join_group=get_setting_value_for_user_group_object( user_group.can_join_group, group_members, group_subgroups ), @@ -834,7 +840,7 @@ def bulk_create_system_user_groups(groups: list[dict[str, str]], realm: Realm) - user_group_ids = [id for (id,) in cursor.fetchall()] rows = [ - SQL("({},{},{},{},{},{},{},{},{})").format( + SQL("({},{},{},{},{},{},{},{},{},{})").format( Literal(user_group_ids[idx]), Literal(realm.id), Literal(group["name"]), @@ -843,13 +849,14 @@ def bulk_create_system_user_groups(groups: list[dict[str, str]], realm: Realm) - Literal(initial_group_setting_value), Literal(initial_group_setting_value), Literal(initial_group_setting_value), + Literal(initial_group_setting_value), Literal(False), ) for idx, group in enumerate(groups) ] query = SQL( """ - INSERT INTO zerver_namedusergroup (usergroup_ptr_id, realm_id, name, description, is_system_group, can_join_group_id, can_manage_group_id, can_mention_group_id, deactivated) + INSERT INTO zerver_namedusergroup (usergroup_ptr_id, realm_id, name, description, is_system_group, can_add_members_group_id, can_join_group_id, can_manage_group_id, can_mention_group_id, deactivated) VALUES {rows} """ ).format(rows=SQL(", ").join(rows)) @@ -931,7 +938,8 @@ def create_system_user_groups_for_realm(realm: Realm) -> dict[int, NamedUserGrou user_group = set_defaults_for_group_settings(group, {}, system_groups_name_dict) groups_with_updated_settings.append(user_group) NamedUserGroup.objects.bulk_update( - groups_with_updated_settings, ["can_join_group", "can_manage_group", "can_mention_group"] + groups_with_updated_settings, + ["can_add_members_group", "can_join_group", "can_manage_group", "can_mention_group"], ) subgroup_objects: list[GroupGroupMembership] = [] diff --git a/zerver/migrations/0599_namedusergroup_add_can_add_members_group.py b/zerver/migrations/0599_namedusergroup_add_can_add_members_group.py new file mode 100644 index 0000000000..938fbde05b --- /dev/null +++ b/zerver/migrations/0599_namedusergroup_add_can_add_members_group.py @@ -0,0 +1,23 @@ +# Generated by Django 5.0.9 on 2024-10-07 07:23 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("zerver", "0598_alter_namedusergroup_can_join_group"), + ] + + operations = [ + migrations.AddField( + model_name="namedusergroup", + name="can_add_members_group", + field=models.ForeignKey( + null=True, + on_delete=django.db.models.deletion.RESTRICT, + related_name="+", + to="zerver.usergroup", + ), + ), + ] diff --git a/zerver/migrations/0600_set_default_for_can_add_members_group.py b/zerver/migrations/0600_set_default_for_can_add_members_group.py new file mode 100644 index 0000000000..b98ab93f6f --- /dev/null +++ b/zerver/migrations/0600_set_default_for_can_add_members_group.py @@ -0,0 +1,144 @@ +# Generated by Django 5.0.9 on 2024-10-07 17:05 + +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 F, Max, Min, OuterRef + + +def set_default_value_for_can_add_members_group( + apps: StateApps, schema_editor: BaseDatabaseSchemaEditor +) -> None: + NamedUserGroup = apps.get_model("zerver", "NamedUserGroup") + Realm = apps.get_model("zerver", "Realm") + + BATCH_SIZE = 1000 + max_id = NamedUserGroup.objects.filter(can_add_members_group=None).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.filter(can_add_members_group=None).aggregate(Min("id"))[ + "id__min" + ] + + owners_system_group_ids = NamedUserGroup.objects.filter(name="role:owners").values_list( + "id", flat=True + ) + realm_ids_allowing_owners_to_manage_all_groups = Realm.objects.filter( + can_manage_all_groups__in=owners_system_group_ids + ).values_list("id", flat=True) + + admins_system_group_ids = NamedUserGroup.objects.filter(name="role:administrators").values_list( + "id", flat=True + ) + realm_ids_allowing_admins_to_manage_all_groups = Realm.objects.filter( + can_manage_all_groups__in=admins_system_group_ids + ).values_list("id", flat=True) + + moderators_system_group_ids = NamedUserGroup.objects.filter(name="role:moderators").values_list( + "id", flat=True + ) + realm_ids_allowing_moderators_to_manage_all_groups = Realm.objects.filter( + can_manage_all_groups__in=moderators_system_group_ids + ).values_list("id", flat=True) + + members_system_group_ids = NamedUserGroup.objects.filter(name="role:members").values_list( + "id", flat=True + ) + realm_ids_allowing_members_to_manage_all_groups = Realm.objects.filter( + can_manage_all_groups__in=members_system_group_ids + ).values_list("id", flat=True) + + 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(): + # Initialize to the corresponding system group, i.e. just + # copy from can_manage_all_groups. Previously, either + # owners/administrators/moderators or group members were + # allowed to manage the group if they were part of + # can_manage_all_groups. Non members were not allowed to + # manage the group unless for the roles mentioned above. + # That is why copying from can_manage_all_groups should + # give the identical set of users permission. + NamedUserGroup.objects.filter( + id__range=(lower_bound, upper_bound), + can_add_members_group=None, + realm_id__in=realm_ids_allowing_owners_to_manage_all_groups, + ).update( + can_add_members_group=NamedUserGroup.objects.filter( + name="role:owners", + realm_for_sharding=OuterRef("realm_for_sharding"), + is_system_group=True, + ).values("pk") + ) + + NamedUserGroup.objects.filter( + id__range=(lower_bound, upper_bound), + can_add_members_group=None, + realm_id__in=realm_ids_allowing_admins_to_manage_all_groups, + ).update( + can_add_members_group=NamedUserGroup.objects.filter( + name="role:administrators", + realm_for_sharding=OuterRef("realm_for_sharding"), + is_system_group=True, + ).values("pk") + ) + + NamedUserGroup.objects.filter( + id__range=(lower_bound, upper_bound), + can_add_members_group=None, + realm_id__in=realm_ids_allowing_moderators_to_manage_all_groups, + ).update( + can_add_members_group=NamedUserGroup.objects.filter( + name="role:moderators", + realm_for_sharding=OuterRef("realm_for_sharding"), + is_system_group=True, + ).values("pk") + ) + + # Initialize can_add_members_group to the group itself. + # This should give the identical set of users permission, + # since users could only exercise the permission previously + # if they were a member of the group. + NamedUserGroup.objects.filter( + id__range=(lower_bound, upper_bound), + can_add_members_group=None, + realm_id__in=realm_ids_allowing_members_to_manage_all_groups, + ).update(can_add_members_group=F("id")) + + # For the remaining group options, there is no direct + # translation to can_add_members_group. We'll pick the + # closest safe choice i.e. moderators for this. + NamedUserGroup.objects.filter( + id__range=(lower_bound, upper_bound), + can_add_members_group=None, + ).update( + can_add_members_group=NamedUserGroup.objects.filter( + name="role:moderators", + realm_for_sharding=OuterRef("realm_for_sharding"), + is_system_group=True, + ).values("pk") + ) + + lower_bound += BATCH_SIZE + + +class Migration(migrations.Migration): + atomic = False + dependencies = [ + ("zerver", "0599_namedusergroup_add_can_add_members_group"), + ] + + operations = [ + migrations.RunPython( + set_default_value_for_can_add_members_group, + elidable=True, + reverse_code=migrations.RunPython.noop, + ) + ] diff --git a/zerver/migrations/0601_alter_namedusergroup_can_add_members_group.py b/zerver/migrations/0601_alter_namedusergroup_can_add_members_group.py new file mode 100644 index 0000000000..7c951dabe5 --- /dev/null +++ b/zerver/migrations/0601_alter_namedusergroup_can_add_members_group.py @@ -0,0 +1,22 @@ +# Generated by Django 5.0.9 on 2024-10-09 08:25 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("zerver", "0600_set_default_for_can_add_members_group"), + ] + + operations = [ + migrations.AlterField( + model_name="namedusergroup", + name="can_add_members_group", + field=models.ForeignKey( + on_delete=django.db.models.deletion.RESTRICT, + related_name="+", + to="zerver.usergroup", + ), + ), + ] diff --git a/zerver/models/groups.py b/zerver/models/groups.py index c37f4ac7ef..80373ba09f 100644 --- a/zerver/models/groups.py +++ b/zerver/models/groups.py @@ -59,6 +59,9 @@ class NamedUserGroup(UserGroup): # type: ignore[django-manager-missing] # djang ) is_system_group = models.BooleanField(default=False, db_column="is_system_group") + can_add_members_group = models.ForeignKey( + UserGroup, on_delete=models.RESTRICT, related_name="+" + ) can_join_group = models.ForeignKey(UserGroup, on_delete=models.RESTRICT, related_name="+") can_manage_group = models.ForeignKey(UserGroup, on_delete=models.RESTRICT, related_name="+") can_mention_group = models.ForeignKey( @@ -95,6 +98,16 @@ class NamedUserGroup(UserGroup): # type: ignore[django-manager-missing] # djang } GROUP_PERMISSION_SETTINGS = { + "can_add_members_group": GroupPermissionSetting( + require_system_group=False, + allow_internet_group=False, + allow_owners_group=True, + allow_nobody_group=True, + allow_everyone_group=False, + default_group_name="group_creator", + default_for_system_groups=SystemGroups.NOBODY, + id_field_name="can_add_members_group_id", + ), "can_join_group": GroupPermissionSetting( require_system_group=False, allow_internet_group=False, diff --git a/zerver/openapi/curl_param_value_generators.py b/zerver/openapi/curl_param_value_generators.py index f356da4798..52eca0c01b 100644 --- a/zerver/openapi/curl_param_value_generators.py +++ b/zerver/openapi/curl_param_value_generators.py @@ -264,6 +264,7 @@ def get_temp_user_group_id() -> dict[str, object]: user_group, _ = NamedUserGroup.objects.get_or_create( name="temp", realm=get_realm("zulip"), + can_add_members_group_id=11, can_join_group_id=11, can_manage_group_id=11, can_mention_group_id=11, @@ -279,6 +280,7 @@ def get_temp_user_group_id_for_deactivation() -> dict[str, object]: user_group, _ = NamedUserGroup.objects.get_or_create( name="temp-deactivation", realm=get_realm("zulip"), + can_add_members_group_id=11, can_join_group_id=11, can_manage_group_id=11, can_mention_group_id=11, diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 2ce2711406..deb76d6b0f 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -3175,6 +3175,7 @@ paths: "description": "Backend team", "id": 2, "is_system_group": false, + "can_add_members_group": 16, "can_join_group": 16, "can_manage_group": 16, "can_mention_group": 11, @@ -3232,6 +3233,20 @@ paths: description: | The new description of the group. Only present if the description changed. + can_add_members_group: + allOf: + - $ref: "#/components/schemas/GroupSettingValue" + - description: | + A [group-setting value][setting-values] defining the set of users who + have permission to add members to this group. Only present if this user + group permission setting changed. + + **Changes**: New in Zulip 10.0 (feature level 305). Previously, this + permission was controlled by the `can_manage_group` setting. + + Will be one of the following: + + [setting-values]: /api/group-setting-values can_join_group: allOf: - $ref: "#/components/schemas/GroupSettingValue" @@ -20248,6 +20263,19 @@ paths: items: type: integer example: [1, 2, 3, 4] + can_add_members_group: + allOf: + - description: | + A [group-setting value][setting-values] defining the set of users who + have permission to add members to this user group. + + **Changes**: New in Zulip 10.0 (feature level 305). Previously, this + permission was controlled by the `can_manage_group` setting. + + [setting-values]: /api/group-setting-values + [system-groups]: /api/group-setting-values#system-groups + - $ref: "#/components/schemas/GroupSettingValue" + example: 11 can_join_group: allOf: - description: | @@ -20306,6 +20334,8 @@ paths: encoding: members: contentType: application/json + can_add_members_group: + contentType: application/json can_join_group: contentType: application/json can_manage_group: @@ -20442,6 +20472,38 @@ paths: a required field. type: string example: The marketing team. + can_add_members_group: + description: | + The set of users who have permission to add members to this user group + expressed as an [update to a group-setting value][update-group-setting]. + + **Changes**: New in Zulip 10.0 (feature level 305). Previously, this + permission was controlled by the `can_manage_group` setting. + + [update-group-setting]: /api/group-setting-values#updating-group-setting-values + [system-groups]: /api/group-setting-values#system-groups + type: object + additionalProperties: false + properties: + new: + allOf: + - description: | + The new [group-setting value](/api/group-setting-values) for the set of users + who would have the permission to add members to the group. + - $ref: "#/components/schemas/GroupSettingValue" + old: + allOf: + - description: | + The expected current [group-setting value](/api/group-setting-values) + for the set of users who have the permission to add members to the group. + - $ref: "#/components/schemas/GroupSettingValue" + required: + - new + example: + { + "new": {"direct_members": [10], "direct_subgroups": [11]}, + "old": 11, + } can_join_group: description: | The set of users who have permission to join this user group @@ -20557,6 +20619,8 @@ paths: "old": 11, } encoding: + can_add_members_group: + contentType: application/json can_join_group: contentType: application/json can_manage_group: @@ -20706,6 +20770,19 @@ paths: modified by users. **Changes**: New in Zulip 5.0 (feature level 93). + can_add_members_group: + allOf: + - $ref: "#/components/schemas/GroupSettingValue" + - description: | + A [group-setting value][setting-values] defining the set of users who + have permission to add members to this user group. + + **Changes**: New in Zulip 10.0 (feature level 305). Previously, this + permission was controlled by the `can_manage_group` setting. + + Will be one of the following: + + [setting-values]: /api/group-setting-values can_join_group: allOf: - $ref: "#/components/schemas/GroupSettingValue" @@ -20777,6 +20854,7 @@ paths: "members": [1], "direct_subgroup_ids": [], "is_system_group": true, + "can_add_members_group": 16, "can_join_group": 16, "can_manage_group": 16, "can_mention_group": 11, @@ -20790,6 +20868,7 @@ paths: "members": [2], "direct_subgroup_ids": [1], "is_system_group": true, + "can_add_members_group": 17, "can_join_group": 17, "can_manage_group": 17, "can_mention_group": 12, @@ -20803,6 +20882,7 @@ paths: "members": [3, 4], "direct_subgroup_ids": [], "is_system_group": false, + "can_add_members_group": 20, "can_join_group": 20, "can_manage_group": 20, "can_mention_group": 13, @@ -22069,6 +22149,19 @@ components: directly modified by users. **Changes**: New in Zulip 5.0 (feature level 93). + can_add_members_group: + allOf: + - $ref: "#/components/schemas/GroupSettingValue" + - description: | + A [group-setting value][setting-values] defining the set of users who + have permission to add members to this user group. + + **Changes**: New in Zulip 10.0 (feature level 305). Previously, this + permission was controlled by the `can_manage_group` setting. + + Will be one of the following: + + [setting-values]: /api/group-setting-values can_join_group: allOf: - $ref: "#/components/schemas/GroupSettingValue" @@ -24222,7 +24315,7 @@ components: The ID of the target user group. schema: type: integer - example: 34 + example: 35 required: true QueueId: name: queue_id diff --git a/zerver/tests/test_import_export.py b/zerver/tests/test_import_export.py index cc955f2e38..5b38e877e4 100644 --- a/zerver/tests/test_import_export.py +++ b/zerver/tests/test_import_export.py @@ -499,7 +499,7 @@ class RealmImportExportTest(ExportFile): self.assertEqual(exported_realm_user_default[0]["default_language"], "de") exported_usergroups = data["zerver_usergroup"] - self.assert_length(exported_usergroups, 10) + self.assert_length(exported_usergroups, 11) self.assertFalse("direct_members" in exported_usergroups[2]) self.assertFalse("direct_subgroups" in exported_usergroups[2]) diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index dc63310886..4343b8ec65 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -1615,7 +1615,7 @@ class UserGroupAPITestCase(UserGroupTestCase): for i in range(50) ] - with self.assert_database_query_count(7): + with self.assert_database_query_count(9): user_group = create_user_group_in_database( name="support", members=[hamlet, cordelia, *original_users], diff --git a/zerver/transaction_tests/test_user_groups.py b/zerver/transaction_tests/test_user_groups.py index b68eb7e08e..3b5f6be325 100644 --- a/zerver/transaction_tests/test_user_groups.py +++ b/zerver/transaction_tests/test_user_groups.py @@ -78,8 +78,10 @@ class UserGroupRaceConditionTestCase(ZulipTransactionTestCase): # can_manage_group in this test, deleting that group # should be reconsidered. can_manage_group = group.can_manage_group + can_add_members_group = group.can_add_members_group group.delete() can_manage_group.delete() + can_add_members_group.delete() transaction.on_commit(self.created_user_groups.clear) super().tearDown() diff --git a/zerver/views/user_groups.py b/zerver/views/user_groups.py index 494261be38..223d396e95 100644 --- a/zerver/views/user_groups.py +++ b/zerver/views/user_groups.py @@ -57,6 +57,7 @@ def add_user_group( name: str, members: Json[list[int]], description: str, + can_add_members_group: Json[int | AnonymousSettingGroupDict] | None = None, can_join_group: Json[int | AnonymousSettingGroupDict] | None = None, can_manage_group: Json[int | AnonymousSettingGroupDict] | None = None, can_mention_group: Json[int | AnonymousSettingGroupDict] | None = None, @@ -117,6 +118,7 @@ def edit_user_group( user_group_id: PathOnly[int], name: str | None = None, description: str | None = None, + can_add_members_group: Json[GroupSettingChangeRequest] | None = None, can_join_group: Json[GroupSettingChangeRequest] | None = None, can_manage_group: Json[GroupSettingChangeRequest] | None = None, can_mention_group: Json[GroupSettingChangeRequest] | None = None, @@ -124,6 +126,7 @@ def edit_user_group( if ( name is None and description is None + and can_add_members_group is None and can_join_group is None and can_manage_group is None and can_mention_group is None @@ -136,6 +139,7 @@ def edit_user_group( if user_group.deactivated and ( description is not None + or can_add_members_group is not None or can_join_group is not None or can_mention_group is not None or can_manage_group is not None