From ea3a7a9e6fb340dbccd1df2006ad590fa9ce45ca Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Mon, 3 Jul 2023 11:31:01 +0530 Subject: [PATCH] user_groups: Add API restrictions for long user group names. Previously we had database level restriction on length of user group names. Now we add the same restriction to API level as well, so we can return a better error response. --- zerver/lib/user_groups.py | 9 +++++++++ zerver/models.py | 4 +++- zerver/tests/test_user_groups.py | 14 ++++++++++++++ zerver/views/user_groups.py | 3 +++ 4 files changed, 29 insertions(+), 1 deletion(-) diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index 09fd604662..3427eb50b6 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -101,6 +101,15 @@ def access_user_group_for_setting( return user_group +def check_user_group_name(group_name: str) -> str: + if len(group_name) > UserGroup.MAX_NAME_LENGTH: + raise JsonableError( + _("User group name cannot exceed {} characters.").format(UserGroup.MAX_NAME_LENGTH) + ) + + return group_name + + def user_groups_in_realm_serialized(realm: Realm) -> List[UserGroupDict]: """This function is used in do_events_register code path so this code should be performant. We need to do 2 database queries because diff --git a/zerver/models.py b/zerver/models.py index 66c5a5386b..7b9e235dd7 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -2234,8 +2234,10 @@ class PasswordTooWeakError(Exception): class UserGroup(models.Model): # type: ignore[django-manager-missing] # django-stubs cannot resolve the custom CTEManager yet https://github.com/typeddjango/django-stubs/issues/1023 + MAX_NAME_LENGTH = 100 + objects: CTEManager = CTEManager() - name = models.CharField(max_length=100) + name = models.CharField(max_length=MAX_NAME_LENGTH) direct_members = models.ManyToManyField( UserProfile, through="UserGroupMembership", related_name="direct_groups" ) diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index 505518d359..52a96f80b6 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -267,6 +267,16 @@ class UserGroupAPITestCase(UserGroupTestCase): self.assert_json_error(result, "User group 'support' already exists.") self.assert_length(UserGroup.objects.filter(realm=hamlet.realm), 10) + # Test we cannot create group with same name again + params = { + "name": "a" * (UserGroup.MAX_NAME_LENGTH + 1), + "members": orjson.dumps([hamlet.id]).decode(), + "description": "Test group", + } + result = self.client_post("/json/user_groups/create", info=params) + self.assert_json_error(result, "User group name cannot exceed 100 characters.") + self.assert_length(UserGroup.objects.filter(realm=hamlet.realm), 10) + def test_can_mention_group_setting_during_user_group_creation(self) -> None: self.login("hamlet") hamlet = self.example_user("hamlet") @@ -411,6 +421,10 @@ class UserGroupAPITestCase(UserGroupTestCase): result = self.client_patch(f"/json/user_groups/{lear_test_group.id}", info=params) self.assert_json_error(result, "Invalid user group") + params = {"name": "a" * (UserGroup.MAX_NAME_LENGTH + 1)} + result = self.client_patch(f"/json/user_groups/{user_group.id}", info=params) + self.assert_json_error(result, "User group name cannot exceed 100 characters.") + def test_update_can_mention_group_setting(self) -> None: hamlet = self.example_user("hamlet") support_group = check_add_user_group(hamlet.realm, "support", [hamlet], acting_user=None) diff --git a/zerver/views/user_groups.py b/zerver/views/user_groups.py index a7cf0bfb88..b37de2d481 100644 --- a/zerver/views/user_groups.py +++ b/zerver/views/user_groups.py @@ -26,6 +26,7 @@ from zerver.lib.user_groups import ( access_user_group_by_id, access_user_group_for_setting, access_user_groups_as_potential_subgroups, + check_user_group_name, get_direct_memberships_of_users, get_recursive_subgroups_for_groups, get_subgroup_ids, @@ -51,6 +52,7 @@ def add_user_group( can_mention_group_id: Optional[int] = REQ(json_validator=check_int, default=None), ) -> HttpResponse: user_profiles = user_ids_to_users(members, user_profile.realm) + name = check_user_group_name(name) group_settings_map = {} request_settings_dict = locals() @@ -107,6 +109,7 @@ def edit_user_group( user_group = access_user_group_by_id(user_group_id, user_profile) if name is not None and name != user_group.name: + name = check_user_group_name(name) do_update_user_group_name(user_group, name, acting_user=user_profile) if description is not None and description != user_group.description: