From f40db2de28085e01884a0b0625bce1f943c0b972 Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Thu, 9 Jan 2025 07:01:45 +0000 Subject: [PATCH] stream_data: Use can_add_subscribers_group to check permissions. --- web/src/stream_data.ts | 21 +++++-- web/tests/stream_data.test.cjs | 82 ++++++++++++++++++++++++++- web/tests/stream_settings_ui.test.cjs | 8 +++ 3 files changed, 102 insertions(+), 9 deletions(-) diff --git a/web/src/stream_data.ts b/web/src/stream_data.ts index 337ac6642c..6c27f9b389 100644 --- a/web/src/stream_data.ts +++ b/web/src/stream_data.ts @@ -542,12 +542,21 @@ export function can_view_subscribers(sub: StreamSubscription): boolean { } export function can_subscribe_others(sub: StreamSubscription): boolean { - // User can add other users to stream if stream is public or user is subscribed to stream - // and realm level setting allows user to add subscribers. - return ( - !current_user.is_guest && - (!sub.invite_only || sub.subscribed) && - settings_data.can_subscribe_others_to_all_streams() + if (sub.invite_only && !sub.subscribed) { + return false; + } + + if (settings_data.can_subscribe_others_to_all_streams()) { + return true; + } + + if (can_change_permissions(sub)) { + return true; + } + + return user_groups.is_user_in_setting_group( + sub.can_add_subscribers_group, + people.my_current_user_id(), ); } diff --git a/web/tests/stream_data.test.cjs b/web/tests/stream_data.test.cjs index ccb3548ea6..456fabb983 100644 --- a/web/tests/stream_data.test.cjs +++ b/web/tests/stream_data.test.cjs @@ -322,6 +322,9 @@ test("get_streams_for_user", ({override}) => { is_muted: true, invite_only: true, history_public_to_subscribers: true, + can_remove_subscribers_group: admins_group.id, + can_add_subscribers_group: admins_group.id, + can_administer_channel_group: admins_group.id, }; const social = { color: "red", @@ -330,6 +333,9 @@ test("get_streams_for_user", ({override}) => { is_muted: false, invite_only: false, history_public_to_subscribers: false, + can_remove_subscribers_group: admins_group.id, + can_add_subscribers_group: admins_group.id, + can_administer_channel_group: admins_group.id, }; const test = { color: "yellow", @@ -337,6 +343,9 @@ test("get_streams_for_user", ({override}) => { stream_id: 3, is_muted: true, invite_only: true, + can_remove_subscribers_group: admins_group.id, + can_add_subscribers_group: admins_group.id, + can_administer_channel_group: admins_group.id, }; const world = { color: "blue", @@ -345,6 +354,9 @@ test("get_streams_for_user", ({override}) => { is_muted: false, invite_only: false, history_public_to_subscribers: false, + can_remove_subscribers_group: admins_group.id, + can_add_subscribers_group: admins_group.id, + can_administer_channel_group: admins_group.id, }; const errors = { color: "green", @@ -353,6 +365,9 @@ test("get_streams_for_user", ({override}) => { is_muted: false, invite_only: false, history_public_to_subscribers: false, + can_remove_subscribers_group: admins_group.id, + can_add_subscribers_group: admins_group.id, + can_administer_channel_group: admins_group.id, }; const subs = [denmark, social, test, world, errors]; for (const sub of subs) { @@ -380,11 +395,16 @@ test("get_streams_for_user", ({override}) => { social, ]); assert.deepEqual(stream_data.get_streams_for_user(test_user.user_id).can_subscribe, []); - // Verify that administrator cannot subscribe if they are not part - // of the appropriate group. + // Administrator is not part of the realm_can_add_subscribers_group + // or the stream level can_add_subscribers_group. But users with + // the permission to administer a channel can also subscribe other + // users. Admins can administer all channels they have access to. override(current_user, "is_admin", true); assert.equal(user_groups.is_user_in_group(students.id, current_user.user_id), false); - assert.deepEqual(stream_data.get_streams_for_user(test_user.user_id).can_subscribe, []); + assert.deepEqual(stream_data.get_streams_for_user(test_user.user_id).can_subscribe, [ + world, + errors, + ]); override(realm, "realm_can_add_subscribers_group", everyone_group.id); assert.deepEqual(stream_data.get_streams_for_user(test_user.user_id).can_subscribe, [ @@ -435,6 +455,7 @@ test("admin_options", ({override}) => { invite_only: false, can_remove_subscribers_group: admins_group.id, can_administer_channel_group, + can_add_subscribers_group: admins_group.id, date_created: 1691057093, creator_id: null, }; @@ -519,6 +540,7 @@ test("stream_settings", ({override}) => { invite_only: false, can_remove_subscribers_group: admins_group.id, can_administer_channel_group: nobody_group.id, + can_add_subscribers_group: admins_group.id, date_created: 1691057093, creator_id: null, }; @@ -531,6 +553,7 @@ test("stream_settings", ({override}) => { invite_only: false, can_remove_subscribers_group: admins_group.id, can_administer_channel_group: nobody_group.id, + can_add_subscribers_group: admins_group.id, date_created: 1691057093, creator_id: null, }; @@ -545,6 +568,7 @@ test("stream_settings", ({override}) => { message_retention_days: 10, can_remove_subscribers_group: admins_group.id, can_administer_channel_group: nobody_group.id, + can_add_subscribers_group: admins_group.id, date_created: 1691057093, creator_id: null, }; @@ -1246,6 +1270,58 @@ test("can_unsubscribe_others", ({override}) => { assert.equal(stream_data.can_unsubscribe_others(sub), false); }); +test("can_subscribe_others", ({override}) => { + override(realm, "realm_can_add_subscribers_group", admins_group.id); + const sub = { + name: "Denmark", + subscribed: true, + color: "red", + stream_id: 1, + can_add_subscribers_group: admins_group.id, + can_administer_channel_group: nobody_group.id, + can_remove_subscribers_group: admins_group.id, + }; + stream_data.add_sub(sub); + + people.initialize_current_user(admin_user_id); + assert.equal(stream_data.can_subscribe_others(sub), true); + people.initialize_current_user(moderator_user_id); + assert.equal(stream_data.can_subscribe_others(sub), false); + + sub.can_add_subscribers_group = moderators_group.id; + people.initialize_current_user(admin_user_id); + assert.equal(stream_data.can_subscribe_others(sub), true); + people.initialize_current_user(moderator_user_id); + assert.equal(stream_data.can_subscribe_others(sub), true); + people.initialize_current_user(test_user.user_id); + assert.equal(stream_data.can_subscribe_others(sub), false); + + sub.can_add_subscribers_group = everyone_group.id; + people.initialize_current_user(admin_user_id); + assert.equal(stream_data.can_subscribe_others(sub), true); + people.initialize_current_user(moderator_user_id); + assert.equal(stream_data.can_subscribe_others(sub), true); + people.initialize_current_user(test_user.user_id); + assert.equal(stream_data.can_subscribe_others(sub), true); + + // With the setting set to user defined group not including admin, + // admin can still subscribe others. + sub.can_add_subscribers_group = students.id; + override(current_user, "is_admin", true); + people.initialize_current_user(admin_user_id); + assert.equal(stream_data.can_subscribe_others(sub), true); + override(current_user, "is_admin", false); + people.initialize_current_user(moderator_user_id); + assert.equal(stream_data.can_subscribe_others(sub), false); + people.initialize_current_user(test_user.user_id); + assert.equal(stream_data.can_subscribe_others(sub), true); + + sub.can_remove_subscribers_group = everyone_group.id; + sub.subscribed = false; + sub.invite_only = true; + assert.equal(stream_data.can_subscribe_others(sub), false); +}); + test("options for dropdown widget", () => { const denmark = { subscribed: true, diff --git a/web/tests/stream_settings_ui.test.cjs b/web/tests/stream_settings_ui.test.cjs index 5955c86a73..7135dc9460 100644 --- a/web/tests/stream_settings_ui.test.cjs +++ b/web/tests/stream_settings_ui.test.cjs @@ -79,6 +79,7 @@ run_test("redraw_left_panel", ({override, mock_template}) => { color: "red", can_administer_channel_group: nobody_group.id, can_remove_subscribers_group: admins_group.id, + can_add_subscribers_group: admins_group.id, date_created: 1691057093, creator_id: null, }; @@ -93,6 +94,7 @@ run_test("redraw_left_panel", ({override, mock_template}) => { color: "red", can_administer_channel_group: nobody_group.id, can_remove_subscribers_group: admins_group.id, + can_add_subscribers_group: admins_group.id, date_created: 1691057093, creator_id: null, }; @@ -107,6 +109,7 @@ run_test("redraw_left_panel", ({override, mock_template}) => { color: "red", can_remove_subscribers_group: admins_group.id, can_administer_channel_group: nobody_group.id, + can_add_subscribers_group: admins_group.id, date_created: 1691057093, creator_id: null, }; @@ -121,6 +124,7 @@ run_test("redraw_left_panel", ({override, mock_template}) => { color: "red", can_administer_channel_group: nobody_group.id, can_remove_subscribers_group: admins_group.id, + can_add_subscribers_group: admins_group.id, date_created: 1691057093, creator_id: null, }; @@ -135,6 +139,7 @@ run_test("redraw_left_panel", ({override, mock_template}) => { color: "red", can_administer_channel_group: nobody_group.id, can_remove_subscribers_group: admins_group.id, + can_add_subscribers_group: admins_group.id, date_created: 1691057093, creator_id: null, }; @@ -149,6 +154,7 @@ run_test("redraw_left_panel", ({override, mock_template}) => { color: "red", can_administer_channel_group: nobody_group.id, can_remove_subscribers_group: admins_group.id, + can_add_subscribers_group: admins_group.id, date_created: 1691057093, creator_id: null, }; @@ -163,6 +169,7 @@ run_test("redraw_left_panel", ({override, mock_template}) => { color: "red", can_administer_channel_group: nobody_group.id, can_remove_subscribers_group: admins_group.id, + can_add_subscribers_group: admins_group.id, date_created: 1691057093, creator_id: null, }; @@ -177,6 +184,7 @@ run_test("redraw_left_panel", ({override, mock_template}) => { color: "red", can_administer_channel_group: nobody_group.id, can_remove_subscribers_group: admins_group.id, + can_add_subscribers_group: admins_group.id, date_created: 1691057093, creator_id: null, };