From 7a6135371ec38db16d290a4e0de1f3aed1eec933 Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Fri, 6 Sep 2024 20:11:41 +0530 Subject: [PATCH] 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 | 64 +++++++++++++++++++++------- web/tests/popover_menus_data.test.js | 7 +++ web/tests/settings_data.test.js | 18 +++++++- web/tests/stream_data.test.js | 10 ++++- zerver/lib/message.py | 9 +++- zerver/lib/streams.py | 8 +++- zerver/lib/user_groups.py | 19 ++++++++- zerver/lib/users.py | 8 +++- zerver/models/users.py | 5 ++- zerver/tests/test_message_delete.py | 17 ++++++++ 10 files changed, 139 insertions(+), 26 deletions(-) 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)