settings: Handle guests separately for group-based settings.

This commit adds code to handle guests separately for group
based settings, where guest will only have permission if
that particular setting can be set to "role:everyone" group
even if the guest user is part of the group which is used
for that setting. This is to make sure that guests do not
get permissions for actions that we generally do not want
guests to have.

Currently the guests do not have permission for most of them
except for "Who can delete any message", where guest could
delete a message if the setting was set to a user defined
group with guest being its member. But this commit still
update the code to use the new function for all the settings
as we want to have a consistent pattern of how to check whether
a user has permission for group-based settings.

web/src/settings_data.ts

@@ -1,3 +1,6 @@
+import assert from "minimalistic-assert";
+
+import * as group_permission_settings from "./group_permission_settings";
 import {page_params} from "./page_params";
 import * as settings_config from "./settings_config";
 import {current_user, realm} from "./state_data";
@@ -105,6 +108,24 @@ function user_has_permission(policy_value: number): boolean {
     return user_join_days >= realm.realm_waiting_period_threshold;
 }
 
+export function user_has_permission_for_group_setting(
+    setting_group_id: number,
+    setting_name: string,
+    setting_type: "realm" | "stream" | "group",
+): boolean {
+    const settings_config = group_permission_settings.get_group_permission_setting_config(
+        setting_name,
+        setting_type,
+    );
+    assert(settings_config !== undefined);
+
+    if (!settings_config.allow_everyone_group && current_user.is_guest) {
+        return false;
+    }
+
+    return user_groups.is_user_in_group(setting_group_id, current_user.user_id);
+}
+
 export function user_can_invite_users_by_email(): boolean {
     return user_has_permission(realm.realm_invite_to_realm_policy);
 }
@@ -113,9 +134,10 @@ export function user_can_create_multiuse_invite(): boolean {
     if (page_params.is_spectator) {
         return false;
     }
-    return user_groups.is_user_in_group(
+    return user_has_permission_for_group_setting(
         realm.realm_create_multiuse_invite_group,
-        current_user.user_id,
+        "create_multiuse_invite_group",
+        "realm",
     );
 }
 
@@ -127,9 +149,10 @@ export function user_can_create_private_streams(): boolean {
     if (page_params.is_spectator) {
         return false;
     }
-    return user_groups.is_user_in_group(
+    return user_has_permission_for_group_setting(
         realm.realm_can_create_private_channel_group,
-        current_user.user_id,
+        "can_create_private_channel_group",
+        "realm",
     );
 }
 
@@ -137,9 +160,10 @@ export function user_can_create_public_streams(): boolean {
     if (page_params.is_spectator) {
         return false;
     }
-    return user_groups.is_user_in_group(
+    return user_has_permission_for_group_setting(
         realm.realm_can_create_public_channel_group,
-        current_user.user_id,
+        "can_create_public_channel_group",
+        "realm",
     );
 }
 
@@ -152,9 +176,10 @@ export function user_can_create_web_public_streams(): boolean {
         return false;
     }
 
-    return user_groups.is_user_in_group(
+    return user_has_permission_for_group_setting(
         realm.realm_can_create_web_public_channel_group,
-        current_user.user_id,
+        "can_create_web_public_channel_group",
+        "realm",
     );
 }
 
@@ -186,7 +211,11 @@ export function can_edit_user_group(group_id: number): boolean {
     }
 
     const group = user_groups.get_user_group_from_id(group_id);
-    return user_groups.is_user_in_group(group.can_manage_group, current_user.user_id);
+    return user_has_permission_for_group_setting(
+        group.can_manage_group,
+        "can_manage_group",
+        "group",
+    );
 }
 
 export function user_can_create_user_groups(): boolean {
@@ -205,9 +234,11 @@ export function user_can_delete_any_message(): boolean {
     if (page_params.is_spectator) {
         return false;
     }
-    return user_groups.is_user_in_group(
+
+    return user_has_permission_for_group_setting(
         realm.realm_can_delete_any_message_group,
-        current_user.user_id,
+        "can_delete_any_message_group",
+        "realm",
     );
 }
 
@@ -215,9 +246,11 @@ export function user_can_delete_own_message(): boolean {
     if (page_params.is_spectator) {
         return false;
     }
-    return user_groups.is_user_in_group(
+
+    return user_has_permission_for_group_setting(
         realm.realm_can_delete_own_message_group,
-        current_user.user_id,
+        "can_delete_own_message_group",
+        "realm",
     );
 }
 
@@ -276,9 +309,10 @@ export function user_can_access_all_other_users(): boolean {
         return true;
     }
 
-    return user_groups.is_user_in_group(
+    return user_has_permission_for_group_setting(
         realm.realm_can_access_all_users_group,
-        current_user.user_id,
+        "can_access_all_users_group",
+        "realm",
     );
 }
 

web/tests/popover_menus_data.test.js

@@ -45,6 +45,13 @@ mock_esm("../src/hash_util", {
 mock_esm("../src/stream_data", {
     is_subscribed: () => true,
 });
+mock_esm("../src/group_permission_settings", {
+    get_group_permission_setting_config() {
+        return {
+            allow_everyone_group: false,
+        };
+    },
+});
 
 // Define test users
 const mike = {

web/tests/settings_data.test.js

@@ -2,7 +2,7 @@
 
 const {strict: assert} = require("assert");
 
-const {zrequire} = require("./lib/namespace");
+const {mock_esm, zrequire} = require("./lib/namespace");
 const {run_test} = require("./lib/test");
 const {current_user, page_params, realm, user_settings} = require("./lib/zpage_params");
 
@@ -24,6 +24,8 @@ const isaac = {
     full_name: "Isaac",
 };
 
+const group_permission_settings = mock_esm("../src/group_permission_settings", {});
+
 run_test("user_can_change_email", () => {
     const can_change_email = settings_data.user_can_change_email;
 
@@ -306,11 +308,16 @@ function test_realm_group_settings(setting_name, validation_func) {
         direct_subgroup_ids: new Set([1]),
     };
 
+    group_permission_settings.get_group_permission_setting_config = () => ({
+        allow_everyone_group: false,
+    });
+
     user_groups.initialize({realm_user_groups: [admins, moderators]});
     page_params.is_spectator = true;
     assert.equal(validation_func(), false);
 
     page_params.is_spectator = false;
+    current_user.is_guest = false;
     realm[setting_name] = 1;
     current_user.user_id = admin_user_id;
     assert.equal(validation_func(), true);
@@ -324,6 +331,15 @@ function test_realm_group_settings(setting_name, validation_func) {
 
     current_user.user_id = member_user_id;
     assert.equal(validation_func(), false);
+
+    current_user.user_id = moderator_user_id;
+    current_user.is_guest = true;
+    assert.equal(validation_func(), false);
+
+    group_permission_settings.get_group_permission_setting_config = () => ({
+        allow_everyone_group: true,
+    });
+    assert.equal(validation_func(), true);
 }
 
 run_test("user_can_create_multiuse_invite", () => {

web/tests/stream_data.test.js

@@ -2,7 +2,7 @@
 
 const {strict: assert} = require("assert");
 
-const {zrequire} = require("./lib/namespace");
+const {mock_esm, zrequire} = require("./lib/namespace");
 const {run_test} = require("./lib/test");
 const blueslip = require("./lib/zblueslip");
 const {current_user, page_params, realm, user_settings} = require("./lib/zpage_params");
@@ -20,6 +20,14 @@ const stream_data = zrequire("stream_data");
 const stream_settings_data = zrequire("stream_settings_data");
 const user_groups = zrequire("user_groups");
 
+mock_esm("../src/group_permission_settings", {
+    get_group_permission_setting_config() {
+        return {
+            allow_everyone_group: false,
+        };
+    },
+});
+
 const me = {
     email: "me@zulip.com",
     full_name: "Current User",

zerver/lib/message.py

@@ -29,7 +29,7 @@ from zerver.lib.stream_subscription import (
 from zerver.lib.streams import can_access_stream_history, get_web_public_streams_queryset
 from zerver.lib.topic import MESSAGE__TOPIC, TOPIC_NAME, messages_for_topic
 from zerver.lib.types import UserDisplayRecipient
-from zerver.lib.user_groups import is_user_in_group
+from zerver.lib.user_groups import user_has_permission_for_group_setting
 from zerver.lib.user_topics import build_get_topic_visibility_policy, get_topic_visibility_policy
 from zerver.lib.users import get_inaccessible_user_ids
 from zerver.models import (
@@ -1223,7 +1223,12 @@ def check_user_group_mention_allowed(sender: UserProfile, user_group_ids: list[i
                 )
             )
 
-        if not is_user_in_group(can_mention_group, sender, direct_member_only=False):
+        if not user_has_permission_for_group_setting(
+            can_mention_group,
+            sender,
+            NamedUserGroup.GROUP_PERMISSION_SETTINGS["can_mention_group"],
+            direct_member_only=False,
+        ):
             raise JsonableError(
                 _("You are not allowed to mention user group '{user_group_name}'.").format(
                     user_group_name=group.name

zerver/lib/streams.py

@@ -22,7 +22,7 @@ from zerver.lib.stream_traffic import get_average_weekly_stream_traffic, get_str
 from zerver.lib.string_validation import check_stream_name
 from zerver.lib.timestamp import datetime_to_timestamp
 from zerver.lib.types import APIStreamDict
-from zerver.lib.user_groups import is_user_in_group
+from zerver.lib.user_groups import user_has_permission_for_group_setting
 from zerver.models import (
     DefaultStreamGroup,
     NamedUserGroup,
@@ -642,7 +642,11 @@ def can_remove_subscribers_from_stream(
 
     group_allowed_to_remove_subscribers = stream.can_remove_subscribers_group
     assert group_allowed_to_remove_subscribers is not None
-    return is_user_in_group(group_allowed_to_remove_subscribers, user_profile)
+    return user_has_permission_for_group_setting(
+        group_allowed_to_remove_subscribers,
+        user_profile,
+        Stream.stream_permission_group_settings["can_remove_subscribers_group"],
+    )
 
 
 def filter_stream_authorization(

zerver/lib/user_groups.py

@@ -116,7 +116,11 @@ def has_user_group_access(
     if can_edit_all_user_groups:
         return True
 
-    return is_user_in_group(user_group.can_manage_group, user_profile)
+    return user_has_permission_for_group_setting(
+        user_group.can_manage_group,
+        user_profile,
+        NamedUserGroup.GROUP_PERMISSION_SETTINGS["can_manage_group"],
+    )
 
 
 def access_user_group_by_id(
@@ -623,6 +627,19 @@ def get_recursive_membership_groups(user_profile: UserProfile) -> QuerySet[UserG
     return cte.join(UserGroup, id=cte.col.group_id).with_cte(cte)
 
 
+def user_has_permission_for_group_setting(
+    user_group: UserGroup,
+    user: UserProfile,
+    setting_config: GroupPermissionSetting,
+    *,
+    direct_member_only: bool = False,
+) -> bool:
+    if not setting_config.allow_everyone_group and user.is_guest:
+        return False
+
+    return is_user_in_group(user_group, user, direct_member_only=direct_member_only)
+
+
 def is_user_in_group(
     user_group: UserGroup, user: UserProfile, *, direct_member_only: bool = False
 ) -> bool:

zerver/lib/users.py

@@ -27,7 +27,7 @@ from zerver.lib.string_validation import check_string_is_printable
 from zerver.lib.timestamp import timestamp_to_datetime
 from zerver.lib.timezone import canonicalize_timezone
 from zerver.lib.types import ProfileDataElementUpdateDict, ProfileDataElementValue, RawUserDict
-from zerver.lib.user_groups import is_user_in_group
+from zerver.lib.user_groups import user_has_permission_for_group_setting
 from zerver.models import (
     CustomProfileField,
     CustomProfileFieldValue,
@@ -656,7 +656,11 @@ def check_user_can_access_all_users(acting_user: UserProfile | None) -> bool:
         return True
 
     realm = acting_user.realm
-    if is_user_in_group(realm.can_access_all_users_group, acting_user):
+    if user_has_permission_for_group_setting(
+        realm.can_access_all_users_group,
+        acting_user,
+        Realm.REALM_PERMISSION_GROUP_SETTINGS["can_access_all_users_group"],
+    ):
         return True
 
     return False

zerver/models/users.py

@@ -770,7 +770,7 @@ class UserProfile(AbstractBaseUser, PermissionsMixin, UserBaseSettings):
         return False
 
     def has_permission(self, policy_name: str, realm: Optional["Realm"] = None) -> bool:
-        from zerver.lib.user_groups import is_user_in_group
+        from zerver.lib.user_groups import user_has_permission_for_group_setting
         from zerver.models import Realm
 
         if policy_name not in [
@@ -798,7 +798,8 @@ class UserProfile(AbstractBaseUser, PermissionsMixin, UserBaseSettings):
                 # setting fields using select_related.
                 realm = self.realm
             allowed_user_group = getattr(realm, policy_name)
-            return is_user_in_group(allowed_user_group, self)
+            setting_config = Realm.REALM_PERMISSION_GROUP_SETTINGS[policy_name]
+            return user_has_permission_for_group_setting(allowed_user_group, self, setting_config)
 
         policy_value = getattr(self.realm, policy_name)
         if policy_value == Realm.POLICY_NOBODY:

zerver/tests/test_message_delete.py

@@ -8,6 +8,7 @@ from django.utils.timezone import now as timezone_now
 
 from zerver.actions.message_delete import do_delete_messages
 from zerver.actions.realm_settings import do_change_realm_permission_group_setting
+from zerver.actions.user_groups import check_add_user_group
 from zerver.lib.test_classes import ZulipTestCase
 from zerver.models import Message, NamedUserGroup, UserProfile
 from zerver.models.groups import SystemGroups
@@ -406,6 +407,22 @@ class DeleteMessageTest(ZulipTestCase):
             "hamlet", "cordelia", "You don't have permission to delete this message"
         )
 
+        # Check that guest cannot delete any message even when they are member
+        # of the group which is allowed to delete any message.
+        polonius = self.example_user("polonius")
+        hamlet = self.example_user("hamlet")
+        user_group = check_add_user_group(realm, "test-group", [hamlet, polonius], acting_user=None)
+        do_change_realm_permission_group_setting(
+            realm,
+            "can_delete_any_message_group",
+            user_group,
+            acting_user=None,
+        )
+        check_delete_message_by_other_user("cordelia", "hamlet")
+        check_delete_message_by_other_user(
+            "cordelia", "polonius", "You don't have permission to delete this message"
+        )
+
     def test_delete_message_according_to_can_delete_own_message_group(self) -> None:
         def check_delete_message_by_sender(sender_name: str, error_msg: str | None = None) -> None:
             sender = self.example_user(sender_name)