From ffb7744974de0c36471e95ecf3852da50d91414b Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Wed, 10 Jul 2024 09:26:03 +0530 Subject: [PATCH] settings: Allow settings to be set to system groups only in production. We use the already existing server level setting to only allow settings to be set to system groups, not a named user defined group as well, in production. But we allow to settings to be set to any named or anonymous user group in tests and development server. "can_mention_group" setting can be set to user defined groups because some of the realms already do that in production. The existing server level setting is also renamed to make it clear that both user defined groups and anonymous groups are not allowed if that setting is set to False. This commit also changes the error message to be consistent for the case when a setting cannot be set to user defined groups as per server level and setting and when a particular setting cannot be set to user defined groups due to the configuration of that particular setting. For this we add a new class SystemGroupRequiredError in exceptions.py so that we need not re-write the error message in multiple places. --- zerver/lib/exceptions.py | 14 ++++++++++++++ zerver/lib/user_groups.py | 23 +++++++++++++---------- zerver/tests/test_realm.py | 32 ++++++++++++++------------------ zerver/tests/test_user_groups.py | 18 +++++++----------- zproject/default_settings.py | 2 +- zproject/dev_settings.py | 2 +- zproject/test_extra_settings.py | 2 +- 7 files changed, 51 insertions(+), 42 deletions(-) diff --git a/zerver/lib/exceptions.py b/zerver/lib/exceptions.py index 3b04957e32..d1acc476ce 100644 --- a/zerver/lib/exceptions.py +++ b/zerver/lib/exceptions.py @@ -53,6 +53,7 @@ class ErrorCode(Enum): REMOTE_REALM_SERVER_MISMATCH_ERROR = auto() PUSH_NOTIFICATIONS_DISALLOWED = auto() EXPECTATION_MISMATCH = auto() + SYSTEM_GROUP_REQUIRED = auto() class JsonableError(Exception): @@ -688,3 +689,16 @@ class PreviousSettingValueMismatchedError(JsonableError): @override def msg_format() -> str: return _("'old' value does not match the expected value.") + + +class SystemGroupRequiredError(JsonableError): + code = ErrorCode.SYSTEM_GROUP_REQUIRED + data_fields = ["setting_name"] + + def __init__(self, setting_name: str) -> None: + self.setting_name = setting_name + + @staticmethod + @override + def msg_format() -> str: + return _("'{setting_name}' must be a system user group.") diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index 30a9c996f9..5c2bc4a5ae 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -11,7 +11,11 @@ from django_cte import With from django_stubs_ext import ValuesQuerySet from psycopg2.sql import SQL, Literal -from zerver.lib.exceptions import JsonableError, PreviousSettingValueMismatchedError +from zerver.lib.exceptions import ( + JsonableError, + PreviousSettingValueMismatchedError, + SystemGroupRequiredError, +) from zerver.lib.types import GroupPermissionSetting, ServerSupportedPermissionSettings from zerver.models import ( GroupGroupMembership, @@ -187,10 +191,13 @@ def check_setting_configuration_for_system_groups( setting_name: str, permission_configuration: GroupPermissionSetting, ) -> None: + if setting_name != "can_mention_group" and ( + not settings.ALLOW_GROUP_VALUED_SETTINGS and not setting_group.is_system_group + ): + raise SystemGroupRequiredError(setting_name) + if permission_configuration.require_system_group and not setting_group.is_system_group: - raise JsonableError( - _("'{setting_name}' must be a system user group.").format(setting_name=setting_name) - ) + raise SystemGroupRequiredError(setting_name) if ( not permission_configuration.allow_internet_group @@ -730,12 +737,8 @@ 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 - ) - ) + if not settings.ALLOW_GROUP_VALUED_SETTINGS: + raise SystemGroupRequiredError(setting_name) return setting_value diff --git a/zerver/tests/test_realm.py b/zerver/tests/test_realm.py index 3565719a8c..145ea2913a 100644 --- a/zerver/tests/test_realm.py +++ b/zerver/tests/test_realm.py @@ -1839,8 +1839,8 @@ class RealmAPITest(ZulipTestCase): realm = get_realm("zulip") self.assertEqual(getattr(realm, setting_name), admins_group.usergroup_ptr) - # Test case when ALLOW_ANONYMOUS_GROUP_VALUED_SETTINGS is False. - with self.settings(ALLOW_ANONYMOUS_GROUP_VALUED_SETTINGS=False): + # Test case when ALLOW_GROUP_VALUED_SETTINGS is False. + with self.settings(ALLOW_GROUP_VALUED_SETTINGS=False): result = self.client_patch( "/json/realm", { @@ -1854,9 +1854,19 @@ class RealmAPITest(ZulipTestCase): ).decode() }, ) - self.assert_json_error( - result, f"{setting_name} can only be set to a single named user group." + self.assert_json_error(result, f"'{setting_name}' must be a system user group.") + + result = self.client_patch( + "/json/realm", + { + setting_name: orjson.dumps( + { + "new": leadership_group.id, + } + ).decode() + }, ) + self.assert_json_error(result, f"'{setting_name}' must be a system user group.") result = self.client_patch( "/json/realm", @@ -1875,20 +1885,6 @@ class RealmAPITest(ZulipTestCase): realm = get_realm("zulip") self.assertEqual(getattr(realm, setting_name), moderators_group.usergroup_ptr) - result = self.client_patch( - "/json/realm", - { - setting_name: orjson.dumps( - { - "new": leadership_group.id, - } - ).decode() - }, - ) - self.assert_json_success(result) - realm = get_realm("zulip") - self.assertEqual(getattr(realm, setting_name), leadership_group.usergroup_ptr) - def test_update_realm_properties(self) -> None: for prop in Realm.property_types: # push_notifications_enabled is maintained by the server, not via the API. diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index 2f011d3f16..339d2e0f7c 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -571,7 +571,7 @@ 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): + with self.settings(ALLOW_GROUP_VALUED_SETTINGS=False): params = { "name": "frontend", "members": orjson.dumps([hamlet.id]).decode(), @@ -584,9 +584,7 @@ class UserGroupAPITestCase(UserGroupTestCase): ).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." - ) + self.assert_json_error(result, "'can_mention_group' must be a system user group.") params = { "name": "frontend", @@ -608,12 +606,12 @@ class UserGroupAPITestCase(UserGroupTestCase): "name": "devops", "members": orjson.dumps([hamlet.id]).decode(), "description": "Devops team", - "can_mention_group": orjson.dumps(moderators_group.id).decode(), + "can_mention_group": orjson.dumps(leadership_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) + self.assertEqual(devops_group.can_mention_group_id, leadership_group.id) def test_user_group_get(self) -> None: # Test success @@ -868,8 +866,8 @@ 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): + # Test case when ALLOW_GROUP_VALUED_SETTINGS is False. + with self.settings(ALLOW_GROUP_VALUED_SETTINGS=False): params = { "can_mention_group": orjson.dumps( { @@ -881,9 +879,7 @@ class UserGroupAPITestCase(UserGroupTestCase): ).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." - ) + self.assert_json_error(result, "'can_mention_group' must be a system user group.") params = { "can_mention_group": orjson.dumps( diff --git a/zproject/default_settings.py b/zproject/default_settings.py index eb2d33de14..e46b0f3495 100644 --- a/zproject/default_settings.py +++ b/zproject/default_settings.py @@ -642,7 +642,7 @@ CUSTOM_AUTHENTICATION_WRAPPER_FUNCTION: Optional[Callable[..., Any]] = None # 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 +ALLOW_GROUP_VALUED_SETTINGS = False # Grace period during which we don't send a resolve/unresolve # notification to a stream and also delete the previous counter diff --git a/zproject/dev_settings.py b/zproject/dev_settings.py index 9ae2b0434e..f191f46d72 100644 --- a/zproject/dev_settings.py +++ b/zproject/dev_settings.py @@ -209,7 +209,7 @@ 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 +ALLOW_GROUP_VALUED_SETTINGS = True # This value needs to be lower in development than usual to allow # for quicker testing of the feature. diff --git a/zproject/test_extra_settings.py b/zproject/test_extra_settings.py index be79da7f1d..c44cd7171b 100644 --- a/zproject/test_extra_settings.py +++ b/zproject/test_extra_settings.py @@ -269,7 +269,7 @@ SCIM_CONFIG: Dict[str, SCIMConfigDict] = { } } -ALLOW_ANONYMOUS_GROUP_VALUED_SETTINGS = True +ALLOW_GROUP_VALUED_SETTINGS = True # This override disables the grace period for undoing resolving/unresolving # a topic in tests.