From d3ea6520dcde63568f5fee8f302754703a836ac2 Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Thu, 30 May 2024 09:15:38 +0530 Subject: [PATCH] user_groups: Add server level setting disallow anonymous groups for settings. This commit adds a server level setting which controls whether the setting can be set to anonymous user groups. We only allow it in the tests for now because the UI can only handle named user groups. --- api_docs/group-setting-values.md | 7 +++ zerver/lib/user_groups.py | 9 ++++ zerver/tests/test_user_groups.py | 88 ++++++++++++++++++++++++++++++++ zerver/views/user_groups.py | 10 ++-- zproject/default_settings.py | 6 +++ zproject/dev_settings.py | 3 ++ zproject/test_extra_settings.py | 2 + 7 files changed, 122 insertions(+), 3 deletions(-) diff --git a/api_docs/group-setting-values.md b/api_docs/group-setting-values.md index aceaf4f7c2..62a20b8aac 100644 --- a/api_docs/group-setting-values.md +++ b/api_docs/group-setting-values.md @@ -4,6 +4,13 @@ Settings defining permissions in Zulip are increasingly represented using [user groups](/help/user-groups), which offer much more flexible configuration than the older [roles](/api/roles-and-permissions) system. +!!! warn "" + + This API feature is under development, and currently only values that + correspond to a single named user group are permitted in + production environments, pending the web application UI supporting + displaying more complex values correctly. + In the API, these settings are represented using a **group-setting value**, which can take two forms: diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index 8559cd2239..a7dcce9e5e 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -2,6 +2,7 @@ from contextlib import contextmanager from dataclasses import dataclass from typing import Collection, Dict, Iterable, Iterator, List, Mapping, Optional, TypedDict, Union +from django.conf import settings from django.db import connection, transaction from django.db.models import F, Prefetch, QuerySet from django.utils.timezone import now as timezone_now @@ -712,6 +713,7 @@ def get_server_supported_permission_settings() -> ServerSupportedPermissionSetti def parse_group_setting_value( setting_value: Union[int, AnonymousSettingGroupDict], + setting_name: str, ) -> Union[int, AnonymousSettingGroupDict]: if isinstance(setting_value, int): return setting_value @@ -719,6 +721,13 @@ def parse_group_setting_value( if len(setting_value.direct_members) == 0 and len(setting_value.direct_subgroups) == 1: return setting_value.direct_subgroups[0] + if not settings.ALLOW_ANONYMOUS_GROUP_VALUED_SETTINGS: + raise JsonableError( + _("{setting_name} can only be set to a single named user group.").format( + setting_name=setting_name + ) + ) + return setting_value diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index 653f178935..621766e463 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -538,6 +538,50 @@ class UserGroupAPITestCase(UserGroupTestCase): result = self.client_post("/json/user_groups/create", info=params) self.assert_json_error(result, "Invalid user group ID: 1111") + with self.settings(ALLOW_ANONYMOUS_GROUP_VALUED_SETTINGS=False): + params = { + "name": "frontend", + "members": orjson.dumps([hamlet.id]).decode(), + "description": "Frontend team", + "can_mention_group": orjson.dumps( + { + "direct_members": [othello.id], + "direct_subgroups": [moderators_group.id], + } + ).decode(), + } + result = self.client_post("/json/user_groups/create", info=params) + self.assert_json_error( + result, "can_mention_group can only be set to a single named user group." + ) + + params = { + "name": "frontend", + "members": orjson.dumps([hamlet.id]).decode(), + "description": "Frontend team", + "can_mention_group": orjson.dumps( + { + "direct_members": [], + "direct_subgroups": [moderators_group.id], + } + ).decode(), + } + result = self.client_post("/json/user_groups/create", info=params) + self.assert_json_success(result) + frontend_group = NamedUserGroup.objects.get(name="frontend", realm=hamlet.realm) + self.assertEqual(frontend_group.can_mention_group_id, moderators_group.id) + + params = { + "name": "devops", + "members": orjson.dumps([hamlet.id]).decode(), + "description": "Devops team", + "can_mention_group": orjson.dumps(moderators_group.id).decode(), + } + result = self.client_post("/json/user_groups/create", info=params) + self.assert_json_success(result) + devops_group = NamedUserGroup.objects.get(name="devops", realm=hamlet.realm) + self.assertEqual(devops_group.can_mention_group_id, moderators_group.id) + def test_user_group_get(self) -> None: # Test success user_profile = self.example_user("hamlet") @@ -791,6 +835,50 @@ class UserGroupAPITestCase(UserGroupTestCase): result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params) self.assert_json_error(result, "Invalid user group ID: 1111") + # Test case when ALLOW_ANONYMOUS_GROUP_VALUED_SETTINGS is False. + with self.settings(ALLOW_ANONYMOUS_GROUP_VALUED_SETTINGS=False): + params = { + "can_mention_group": orjson.dumps( + { + "new": { + "direct_members": [othello.id], + "direct_subgroups": [moderators_group.id, marketing_group.id], + } + } + ).decode() + } + result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params) + self.assert_json_error( + result, "can_mention_group can only be set to a single named user group." + ) + + params = { + "can_mention_group": orjson.dumps( + { + "new": { + "direct_members": [], + "direct_subgroups": [moderators_group.id], + } + } + ).decode() + } + result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params) + self.assert_json_success(result) + support_group = NamedUserGroup.objects.get(name="support", realm=hamlet.realm) + self.assertEqual(support_group.can_mention_group_id, moderators_group.id) + + params = { + "can_mention_group": orjson.dumps( + { + "new": marketing_group.id, + } + ).decode() + } + result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params) + self.assert_json_success(result) + support_group = NamedUserGroup.objects.get(name="support", realm=hamlet.realm) + self.assertEqual(support_group.can_mention_group_id, marketing_group.id) + def test_user_group_update_to_already_existing_name(self) -> None: hamlet = self.example_user("hamlet") self.login_user(hamlet) diff --git a/zerver/views/user_groups.py b/zerver/views/user_groups.py index 52d4a8d1cf..216c449727 100644 --- a/zerver/views/user_groups.py +++ b/zerver/views/user_groups.py @@ -70,7 +70,9 @@ def add_user_group( continue if request_settings_dict[setting_name] is not None: - setting_value = parse_group_setting_value(request_settings_dict[setting_name]) + setting_value = parse_group_setting_value( + request_settings_dict[setting_name], setting_name + ) setting_value_group = access_user_group_for_setting( setting_value, user_profile, @@ -130,11 +132,13 @@ def edit_user_group( continue setting_value = request_settings_dict[setting_name] - new_setting_value = parse_group_setting_value(setting_value.new) + new_setting_value = parse_group_setting_value(setting_value.new, setting_name) expected_current_setting_value = None if setting_value.old is not None: - expected_current_setting_value = parse_group_setting_value(setting_value.old) + expected_current_setting_value = parse_group_setting_value( + setting_value.old, setting_name + ) current_value = getattr(user_group, setting_name) current_setting_api_value = get_group_setting_value_for_api(current_value) diff --git a/zproject/default_settings.py b/zproject/default_settings.py index 303e3c8550..acadf16a5b 100644 --- a/zproject/default_settings.py +++ b/zproject/default_settings.py @@ -635,3 +635,9 @@ CAN_ACCESS_ALL_USERS_GROUP_LIMITS_PRESENCE = False SIGNED_ACCESS_TOKEN_VALIDITY_IN_SECONDS = 60 CUSTOM_AUTHENTICATION_WRAPPER_FUNCTION: Optional[Callable[..., Any]] = None + +# Whether we allow settings to be set to a collection of users and +# groups as described in api_docs/group-setting-values.md. Set to +# False in production, as we can only handle named user groups in the +# web app settings UI. +ALLOW_ANONYMOUS_GROUP_VALUED_SETTINGS = False diff --git a/zproject/dev_settings.py b/zproject/dev_settings.py index 5c132426ca..c35015b60b 100644 --- a/zproject/dev_settings.py +++ b/zproject/dev_settings.py @@ -207,3 +207,6 @@ SCIM_CONFIG: Dict[str, SCIMConfigDict] = { SELF_HOSTING_MANAGEMENT_SUBDOMAIN = "selfhosting" DEVELOPMENT_DISABLE_PUSH_BOUNCER_DOMAIN_CHECK = True PUSH_NOTIFICATION_BOUNCER_URL = f"http://push.{EXTERNAL_HOST}" + +# Breaks the UI if used, but enabled for development environment testing. +ALLOW_ANONYMOUS_GROUP_VALUED_SETTINGS = True diff --git a/zproject/test_extra_settings.py b/zproject/test_extra_settings.py index c60ac3fa76..81936e13bd 100644 --- a/zproject/test_extra_settings.py +++ b/zproject/test_extra_settings.py @@ -268,3 +268,5 @@ SCIM_CONFIG: Dict[str, SCIMConfigDict] = { "name_formatted_included": True, } } + +ALLOW_ANONYMOUS_GROUP_VALUED_SETTINGS = True