From f24f1bfd1427d919cef774e286f20ee0b3fcb822 Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Wed, 16 Oct 2024 16:47:55 +0530 Subject: [PATCH] user_groups: Refactor code to check permission for updating groups. Users with permission to manage the group have all the permissions including joining/leaving the group, adding others group which also have a separate setting to control them. So, it makes sense to just check managing permissions first in access_user_group_for_update and then check the specific permission. There is no behavioral change in this commit, it only changes the order of checking permissions. --- zerver/lib/user_groups.py | 39 +++++++++++++------------------------ zerver/views/user_groups.py | 13 +++---------- 2 files changed, 17 insertions(+), 35 deletions(-) diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index eb351f04ee..d2b2be1e79 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -154,42 +154,31 @@ def access_user_group_for_update( if user_group.is_system_group: raise JsonableError(_("Insufficient permission")) - assert permission_setting in NamedUserGroup.GROUP_PERMISSION_SETTINGS - if ( - permission_setting - in [ - "can_manage_group", - "can_add_members_group", - ] - and user_profile.can_manage_all_groups() - ): + # Users with permission to manage the group have all the permissions + # including joining/leaving the group and add others to group. + if user_profile.can_manage_all_groups(): return user_group user_has_permission = user_has_permission_for_group_setting( - getattr(user_group, permission_setting), + user_group.can_manage_group, user_profile, - NamedUserGroup.GROUP_PERMISSION_SETTINGS[permission_setting], + NamedUserGroup.GROUP_PERMISSION_SETTINGS["can_manage_group"], ) + if user_has_permission: + return user_group - # Users with permission to manage the group should be able to add members - # to the group. This also takes care of the case where a user creating a group - # with themselves having the permission to manage it don't have to explicitly - # add themselves to can_add_members_group. - if ( - not user_has_permission - and permission_setting == "can_add_members_group" - and permission_setting != "can_manage_group" - ): + if permission_setting != "can_manage_group": + assert permission_setting in NamedUserGroup.GROUP_PERMISSION_SETTINGS user_has_permission = user_has_permission_for_group_setting( - user_group.can_manage_group, + getattr(user_group, permission_setting), user_profile, - NamedUserGroup.GROUP_PERMISSION_SETTINGS["can_manage_group"], + NamedUserGroup.GROUP_PERMISSION_SETTINGS[permission_setting], ) - if not user_has_permission: - raise JsonableError(_("Insufficient permission")) + if user_has_permission: + return user_group - return user_group + raise JsonableError(_("Insufficient permission")) def access_user_group_for_deactivation( diff --git a/zerver/views/user_groups.py b/zerver/views/user_groups.py index 94e3b98a59..e5e8841d1f 100644 --- a/zerver/views/user_groups.py +++ b/zerver/views/user_groups.py @@ -346,16 +346,9 @@ def remove_members_from_group_backend( ) -> HttpResponse: user_profiles = user_ids_to_users(members, user_profile.realm, allow_deactivated=False) if len(members) == 1 and user_profile.id == members[0]: - try: - user_group = access_user_group_for_update( - user_group_id, user_profile, permission_setting="can_leave_group" - ) - except JsonableError: - # User can leave the group if user has the permission to - # manage the group. - user_group = access_user_group_for_update( - user_group_id, user_profile, permission_setting="can_manage_group" - ) + user_group = access_user_group_for_update( + user_group_id, user_profile, permission_setting="can_leave_group" + ) else: user_group = access_user_group_for_update( user_group_id, user_profile, permission_setting="can_manage_group"