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.