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

@@ -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.")

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(
+        raise SystemGroupRequiredError(setting_name)
-            _("'{setting_name}' must be a system user group.").format(setting_name=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:
+    if not settings.ALLOW_GROUP_VALUED_SETTINGS:
-        raise JsonableError(
+        raise SystemGroupRequiredError(setting_name)
-            _("{setting_name} can only be set to a single named user group.").format(
-                setting_name=setting_name
-            )
-        )
 
     return setting_value
 

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.
+        # Test case when ALLOW_GROUP_VALUED_SETTINGS is False.
-        with self.settings(ALLOW_ANONYMOUS_GROUP_VALUED_SETTINGS=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(
+            self.assert_json_error(result, f"'{setting_name}' must be a system user group.")
-                result, f"{setting_name} can only be set to a single named 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.

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(
+            self.assert_json_error(result, "'can_mention_group' must be a system user group.")
-                result, "can_mention_group can only be set to a single named 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.
+        # Test case when ALLOW_GROUP_VALUED_SETTINGS is False.
-        with self.settings(ALLOW_ANONYMOUS_GROUP_VALUED_SETTINGS=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(
+            self.assert_json_error(result, "'can_mention_group' must be a system user group.")
-                result, "can_mention_group can only be set to a single named user group."
-            )
 
             params = {
                 "can_mention_group": orjson.dumps(

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

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.

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.