From 4c290a49d3c29648a504e333e9717d32028fb41e Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Fri, 6 Aug 2021 18:52:08 +0530 Subject: [PATCH] user_groups: Do not allow editing system user groups from API. We do not allow any user to edit the system user groups (including renaming, deleting, adding or removing members, etc.) from the API. These user groups will change only by the code when a new user is added or role of a user is changed. This is implemented by rejecting access_user_group_by_id always except the case when it is use to get the user group for sending email and push notifications, as we would need to send notifications to the mentioned user group. --- zerver/lib/email_notifications.py | 2 +- zerver/lib/push_notifications.py | 4 ++- zerver/lib/user_groups.py | 17 +++++++++-- zerver/tests/test_email_notifications.py | 27 ++++++++++++++++++ zerver/tests/test_user_groups.py | 36 ++++++++++++++++++++++++ 5 files changed, 81 insertions(+), 5 deletions(-) diff --git a/zerver/lib/email_notifications.py b/zerver/lib/email_notifications.py index 8b5ff7d07d..4123ab0919 100644 --- a/zerver/lib/email_notifications.py +++ b/zerver/lib/email_notifications.py @@ -371,7 +371,7 @@ def get_mentioned_user_group_name( smallest_user_group_size = math.inf smallest_user_group_name = None for user_group_id in mentioned_user_group_ids: - current_user_group = access_user_group_by_id(user_group_id, user_profile) + current_user_group = access_user_group_by_id(user_group_id, user_profile, for_mention=True) current_user_group_size = len(get_user_group_members(current_user_group)) if current_user_group_size < smallest_user_group_size: diff --git a/zerver/lib/push_notifications.py b/zerver/lib/push_notifications.py index 58e790a589..addf6134d3 100644 --- a/zerver/lib/push_notifications.py +++ b/zerver/lib/push_notifications.py @@ -937,7 +937,9 @@ def handle_push_notification(user_profile_id: int, missed_message: Dict[str, Any mentioned_user_group_id = missed_message.get("mentioned_user_group_id") if mentioned_user_group_id is not None: - user_group = access_user_group_by_id(mentioned_user_group_id, user_profile) + user_group = access_user_group_by_id( + mentioned_user_group_id, user_profile, for_mention=True + ) mentioned_user_group_name = user_group.name apns_payload = get_message_payload_apns( diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index 1d0906f73a..a41b6f4467 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -7,9 +7,13 @@ from zerver.lib.exceptions import JsonableError from zerver.models import Realm, UserGroup, UserGroupMembership, UserProfile -def access_user_group_by_id(user_group_id: int, user_profile: UserProfile) -> UserGroup: +def access_user_group_by_id( + user_group_id: int, user_profile: UserProfile, for_mention: bool = False +) -> UserGroup: try: user_group = UserGroup.objects.get(id=user_group_id, realm=user_profile.realm) + if not for_mention and user_group.is_system_group: + raise JsonableError(_("Insufficient permission")) group_member_ids = get_user_group_members(user_group) if ( not user_profile.is_realm_admin @@ -61,10 +65,17 @@ def remove_user_from_user_group(user_profile: UserProfile, user_group: UserGroup def create_user_group( - name: str, members: List[UserProfile], realm: Realm, *, description: str = "" + name: str, + members: List[UserProfile], + realm: Realm, + *, + description: str = "", + is_system_group: bool = False, ) -> UserGroup: with transaction.atomic(): - user_group = UserGroup.objects.create(name=name, realm=realm, description=description) + user_group = UserGroup.objects.create( + name=name, realm=realm, description=description, is_system_group=is_system_group + ) UserGroupMembership.objects.bulk_create( UserGroupMembership(user_profile=member, user_group=user_group) for member in members ) diff --git a/zerver/tests/test_email_notifications.py b/zerver/tests/test_email_notifications.py index de07652e58..67f32a677e 100644 --- a/zerver/tests/test_email_notifications.py +++ b/zerver/tests/test_email_notifications.py @@ -824,6 +824,33 @@ class TestMissedMessages(ZulipTestCase): for text in expected_email_include: self.assertIn(text, self.normalize_string(mail.outbox[0].body)) + def test_system_user_group_mention_sends_email(self) -> None: + hamlet = self.example_user("hamlet") + cordelia = self.example_user("cordelia") + othello = self.example_user("othello") + + hamlet_and_cordelia = create_user_group( + "hamlet_and_cordelia", [hamlet, cordelia], get_realm("zulip"), is_system_group=True + ) + user_group_mentioned_message_id = self.send_stream_message( + othello, "Denmark", "@*hamlet_and_cordelia*" + ) + + handle_missedmessage_emails( + hamlet.id, + [ + { + "message_id": user_group_mentioned_message_id, + "trigger": "mentioned", + "mentioned_user_group_id": hamlet_and_cordelia.id, + }, + ], + ) + self.assertIn( + "Othello, the Moor of Venice: @*hamlet_and_cordelia* -- ", + self.normalize_string(mail.outbox[0].body), + ) + def test_realm_name_in_notifications(self) -> None: # Test with realm_name_in_notifications for hamlet disabled. self._realm_name_in_missed_message_email_subject(False) diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index 1a49977c9e..a867c88039 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -577,3 +577,39 @@ class UserGroupAPITestCase(UserGroupTestCase): othello.date_joined = timezone_now() - timedelta(days=11) othello.save() check_removing_members_from_group("othello") + + def test_editing_system_user_groups(self) -> None: + desdemona = self.example_user("desdemona") + iago = self.example_user("iago") + othello = self.example_user("othello") + aaron = self.example_user("aaron") + members = [iago, othello] + + user_group = create_user_group( + "System group", + members, + iago.realm, + description="This is a system group", + is_system_group=True, + ) + + def check_support_group_permission(acting_user: UserProfile) -> None: + self.login_user(acting_user) + params = { + "name": "Test system group", + "description": "This is a test system group.", + } + result = self.client_patch(f"/json/user_groups/{user_group.id}", info=params) + self.assert_json_error(result, "Insufficient permission") + + params = {"add": orjson.dumps([aaron.id]).decode()} + result = self.client_post(f"/json/user_groups/{user_group.id}/members", info=params) + self.assert_json_error(result, "Insufficient permission") + + params = {"delete": orjson.dumps([othello.id]).decode()} + result = self.client_post(f"/json/user_groups/{user_group.id}/members", info=params) + self.assert_json_error(result, "Insufficient permission") + + check_support_group_permission(desdemona) + check_support_group_permission(iago) + check_support_group_permission(othello)