diff --git a/web/src/settings_data.ts b/web/src/settings_data.ts index 29457b938a..031c5b86d3 100644 --- a/web/src/settings_data.ts +++ b/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", ); } diff --git a/web/tests/popover_menus_data.test.js b/web/tests/popover_menus_data.test.js index ca107a6980..92d64d27fc 100644 --- a/web/tests/popover_menus_data.test.js +++ b/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 = { diff --git a/web/tests/settings_data.test.js b/web/tests/settings_data.test.js index 81ae6de0d5..cbb81d305d 100644 --- a/web/tests/settings_data.test.js +++ b/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", () => { diff --git a/web/tests/stream_data.test.js b/web/tests/stream_data.test.js index 41f88419fa..6d3ed05808 100644 --- a/web/tests/stream_data.test.js +++ b/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", diff --git a/zerver/lib/message.py b/zerver/lib/message.py index 4393028a02..0e16d69bbc 100644 --- a/zerver/lib/message.py +++ b/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 diff --git a/zerver/lib/streams.py b/zerver/lib/streams.py index 99509d7600..275f596552 100644 --- a/zerver/lib/streams.py +++ b/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( diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index d232ff0523..c37359d075 100644 --- a/zerver/lib/user_groups.py +++ b/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: diff --git a/zerver/lib/users.py b/zerver/lib/users.py index 2c22e945a7..343437403c 100644 --- a/zerver/lib/users.py +++ b/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 diff --git a/zerver/models/users.py b/zerver/models/users.py index 718f960f95..fe5a99d931 100644 --- a/zerver/models/users.py +++ b/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: diff --git a/zerver/tests/test_message_delete.py b/zerver/tests/test_message_delete.py index e96dfd7ea9..982110fb9a 100644 --- a/zerver/tests/test_message_delete.py +++ b/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)