paulmataruso/zulip
1
0
Fork 0
mirror of https://github.com/zulip/zulip.git synced 2025-11-03 13:33:24 +00:00
Code Issues Packages Projects Releases Wiki Activity

streams: Use can_add_subscribers_group for permission check.

Browse Source 
The function to check relevant permissions does so for multiple streams
at once to save us database query counts. Doing it one by one for every
stream would become very expensive.
We've also added `insufficient_permission_streams` to the filter
functions return type for streams for which the current user does not
have permission to subscribe other users.
This commit is contained in:
Shubham Padia
2025-01-16 21:17:29 +00:00
committed by Tim Abbott
parent 7df417f8b1
commit 41c74314c0
6 changed files with 339 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files

65
zerver/lib/streams.py
View File

@@ -25,6 +25,7 @@ from zerver.lib.timestamp import datetime_to_timestamp
from zerver.lib.types import AnonymousSettingGroupDict, APIStreamDict
from zerver.lib.user_groups import (
get_group_setting_value_for_api,
get_recursive_membership_groups,
get_role_based_system_groups_dict,
user_has_permission_for_group_setting,
)
@@ -428,6 +429,10 @@ def check_stream_access_for_delete_or_update(
if stream.realm_id != user_profile.realm_id:
raise JsonableError(error)
# Optimization for the organization administrator code path. We
# don't explicitly grant realm admins this permission, but admins
# implicitly have the can_administer_channel_group permission for
# all accessible channels.
if user_profile.is_realm_admin:
return
@@ -758,6 +763,10 @@ def can_remove_subscribers_from_stream(
if not check_basic_stream_access(user_profile, stream, sub, allow_realm_admin=True):
return False
# Optimization for the organization administrator code path. We
# don't explicitly grant realm admins this permission, but admins
# implicitly have the can_administer_channel_group permission for
# all accessible channels.
if user_profile.is_realm_admin:
return True
@@ -770,6 +779,42 @@ def can_remove_subscribers_from_stream(
)
def get_streams_to_which_user_cannot_add_subscribers(
streams: list[Stream], user_profile: UserProfile
) -> list[Stream]:
# IMPORTANT: This function expects its callers to have already
# checked that the user can access the provided channels, and thus
# does not waste database queries re-checking that.
result: list[Stream] = []
if user_profile.can_subscribe_others_to_all_streams():
return []
# Optimization for the organization administrator code path. We
# don't explicitly grant realm admins this permission, but admins
# implicitly have the can_administer_channel_group permission for
# all accessible channels.
if user_profile.is_realm_admin:
return []
user_recursive_group_ids = set(
get_recursive_membership_groups(user_profile).values_list("id", flat=True)
)
for stream in streams:
group_allowed_to_administer_channel_id = stream.can_administer_channel_group_id
assert group_allowed_to_administer_channel_id is not None
if group_allowed_to_administer_channel_id in user_recursive_group_ids:
continue
group_allowed_to_add_subscribers_id = stream.can_add_subscribers_group_id
assert group_allowed_to_add_subscribers_id is not None
if group_allowed_to_add_subscribers_id not in user_recursive_group_ids:
result.append(stream)
return result
def can_administer_channel(channel: Stream, user_profile: UserProfile) -> bool:
group_allowed_to_administer_channel = channel.can_administer_channel_group
assert group_allowed_to_administer_channel is not None
@@ -781,8 +826,11 @@ def can_administer_channel(channel: Stream, user_profile: UserProfile) -> bool:
def filter_stream_authorization(
user_profile: UserProfile, streams: Collection[Stream]
) -> tuple[list[Stream], list[Stream]]:
user_profile: UserProfile, streams: Collection[Stream], is_subscribing_other_users: bool = False
) -> tuple[list[Stream], list[Stream], list[Stream]]:
if len(streams) == 0:
return [], [], []
recipient_ids = [stream.recipient_id for stream in streams]
subscribed_recipient_ids = set(
Subscription.objects.filter(
@@ -791,6 +839,12 @@ def filter_stream_authorization(
)
unauthorized_streams: list[Stream] = []
streams_to_which_user_cannot_add_subscribers: list[Stream] = []
if is_subscribing_other_users:
streams_to_which_user_cannot_add_subscribers = (
get_streams_to_which_user_cannot_add_subscribers(list(streams), user_profile)
)
for stream in streams:
# Deactivated streams are not accessible
if stream.deactivated:
@@ -815,8 +869,13 @@ def filter_stream_authorization(
stream
for stream in streams
if stream.id not in {stream.id for stream in unauthorized_streams}
and stream.id not in {stream.id for stream in streams_to_which_user_cannot_add_subscribers}
]
return authorized_streams, unauthorized_streams
return (
authorized_streams,
unauthorized_streams,
streams_to_which_user_cannot_add_subscribers,
)
def list_to_streams(

2
zerver/models/users.py
View File

@@ -844,7 +844,7 @@ class UserProfile(AbstractBaseUser, PermissionsMixin, UserBaseSettings):
def can_manage_default_streams(self) -> bool:
return self.is_realm_admin
def can_subscribe_other_users(self) -> bool:
def can_subscribe_others_to_all_streams(self) -> bool:
return self.has_permission("can_add_subscribers_group")
def can_invite_users_by_email(self, realm: Optional["Realm"] = None) -> bool:

40
zerver/tests/test_invite.py
View File

@@ -43,6 +43,7 @@ from zerver.actions.realm_settings import (
do_change_realm_plan_type,
do_set_realm_property,
)
from zerver.actions.streams import do_change_stream_group_based_setting
from zerver.actions.user_groups import check_add_user_group, do_change_user_group_permission_setting
from zerver.actions.user_settings import do_change_full_name
from zerver.actions.users import change_user_is_active
@@ -1645,6 +1646,9 @@ so we didn't send them an invitation. We did send invitations to everyone else!"
def test_invite_without_permission_to_subscribe_others(self) -> None:
realm = get_realm("zulip")
members_group = NamedUserGroup.objects.get(
name=SystemGroups.MEMBERS, realm=realm, is_system_group=True
)
admins_group = NamedUserGroup.objects.get(
name=SystemGroups.ADMINISTRATORS, realm=realm, is_system_group=True
)
@@ -1653,13 +1657,40 @@ so we didn't send them an invitation. We did send invitations to everyone else!"
)
invitee = self.nonreg_email("alice")
stream_names = ["Denmark", "Scotland"]
self.login("hamlet")
result = self.invite(invitee, ["Denmark", "Scotland"])
result = self.invite(invitee, stream_names)
self.assert_json_error(
result, "You do not have permission to subscribe other users to channels."
)
# Changing permission of just one of the channel out of two
# should still give an error.
do_change_stream_group_based_setting(
get_stream("Denmark", realm),
"can_add_subscribers_group",
members_group,
acting_user=None,
)
result = self.invite(invitee, stream_names)
self.assert_json_error(
result, "You do not have permission to subscribe other users to channels."
)
# Changing permission of both the channels out of two should
# result in success.
do_change_stream_group_based_setting(
get_stream("Scotland", realm),
"can_add_subscribers_group",
members_group,
acting_user=None,
)
result = self.invite(invitee, stream_names)
self.assert_json_success(result)
self.check_sent_emails([invitee])
mail.outbox.pop()
# User will be subscribed to default streams even when the
# referrer does not have permission to subscribe others.
result = self.invite(invitee, [], include_realm_default_subscriptions=True)
@@ -1688,20 +1719,17 @@ so we didn't send them an invitation. We did send invitations to everyone else!"
self.login("iago")
invitee = self.nonreg_email("bob")
result = self.invite(invitee, ["Denmark", "Scotland"])
result = self.invite(invitee, stream_names)
self.assert_json_success(result)
self.check_sent_emails([invitee])
mail.outbox.pop()
members_group = NamedUserGroup.objects.get(
name=SystemGroups.MEMBERS, realm=realm, is_system_group=True
)
do_change_realm_permission_group_setting(
realm, "can_add_subscribers_group", members_group, acting_user=None
)
self.login("hamlet")
invitee = self.nonreg_email("test")
result = self.invite(invitee, ["Denmark", "Scotland"])
result = self.invite(invitee, stream_names)
self.assert_json_success(result)
self.check_sent_emails([invitee])
mail.outbox.pop()

232
zerver/tests/test_subs.py
View File

@@ -4937,7 +4937,7 @@ class SubscriptionAPITest(ZulipTestCase):
# Other streams that weren't created using the api should have no creator.
self.assertIsNone(stream["creator_id"])
def test_user_settings_for_subscribing_other_users(self) -> None:
def test_realm_settings_for_subscribing_other_users(self) -> None:
"""
You can't subscribe other people to streams if you are a guest or your account is not old
enough.
@@ -4952,15 +4952,48 @@ class SubscriptionAPITest(ZulipTestCase):
do_change_realm_permission_group_setting(
realm, "can_add_subscribers_group", admins_group, acting_user=None
)
# User should be allowed to add subscribers when creating the
# channel even if they don't have realm wide permission to
# add other subscribers to a channel.
do_change_user_role(self.test_user, UserProfile.ROLE_MODERATOR, acting_user=None)
result = self.common_subscribe_to_streams(
self.test_user,
["stream1"],
{"principals": orjson.dumps([invitee_user_id]).decode()},
# Creator will be part of `can_administer_channel_group` by
# default for a new channel. We set it to admin, so that we
# can test for errors in the next piece of this test.
{
"principals": orjson.dumps([invitee_user_id]).decode(),
"can_administer_channel_group": admins_group.id,
},
allow_fail=True,
)
self.assert_json_success(result)
result = self.common_subscribe_to_streams(
self.test_user,
["stream1"],
{"principals": orjson.dumps([self.example_user("aaron").id]).decode()},
allow_fail=True,
)
self.assert_json_error(result, "Insufficient permission")
nobody_group = NamedUserGroup.objects.get(
name=SystemGroups.NOBODY, realm=realm, is_system_group=True
)
do_change_realm_permission_group_setting(
realm, "can_add_subscribers_group", nobody_group, acting_user=None
)
do_change_stream_group_based_setting(
get_stream("stream1", realm),
"can_add_subscribers_group",
nobody_group,
acting_user=None,
)
# Admins have a special permission to administer every channel
# they have access to. This also grants them access to add
# subscribers.
do_change_user_role(self.test_user, UserProfile.ROLE_REALM_ADMINISTRATOR, acting_user=None)
self.common_subscribe_to_streams(
self.test_user, ["stream1"], {"principals": orjson.dumps([invitee_user_id]).decode()}
@@ -4976,6 +5009,11 @@ class SubscriptionAPITest(ZulipTestCase):
# Make sure that we are checking the permission with a full member,
# as full member is the user just below moderator in the role hierarchy.
self.assertFalse(self.test_user.is_provisional_member)
# User will be able to add subscribers to a newly created
# stream without any realm wide permissions. We create this
# stream programmatically so that we can test for errors for an
# existing stream.
self.make_stream("stream2")
result = self.common_subscribe_to_streams(
self.test_user,
["stream2"],
@@ -5064,6 +5102,176 @@ class SubscriptionAPITest(ZulipTestCase):
)
self.unsubscribe(user_profile, "stream2")
def test_stream_settings_for_subscribing_other_users(self) -> None:
user_profile = self.example_user("cordelia")
invitee_user_id = user_profile.id
realm = user_profile.realm
nobody_group = NamedUserGroup.objects.get(
name=SystemGroups.NOBODY, realm=realm, is_system_group=True
)
do_change_realm_permission_group_setting(
realm, "can_add_subscribers_group", nobody_group, acting_user=None
)
# User will be able to add subscribers to a newly created
# stream without any realm wide permissions. We create this
# stream programmatically so that we can test for errors for an
# existing stream.
do_change_stream_group_based_setting(
self.make_stream("stream1"), "can_add_subscribers_group", nobody_group, acting_user=None
)
result = self.common_subscribe_to_streams(
self.test_user,
["stream1"],
{"principals": orjson.dumps([invitee_user_id]).decode()},
allow_fail=True,
)
self.assert_json_error(result, "Insufficient permission")
# Admins have a special permission to administer every channel
# they have access to. This also grants them access to add
# subscribers.
do_change_user_role(self.test_user, UserProfile.ROLE_REALM_ADMINISTRATOR, acting_user=None)
result = self.common_subscribe_to_streams(
self.test_user, ["stream1"], {"principals": orjson.dumps([invitee_user_id]).decode()}
)
self.assert_json_success(result)
do_change_user_role(self.test_user, UserProfile.ROLE_MEMBER, acting_user=None)
# Make sure that we are checking the permission with a full member,
# as full member is the user just below moderator in the role hierarchy.
self.assertFalse(self.test_user.is_provisional_member)
# User will be able to add subscribers to a newly created
# stream without any realm wide permissions. We create this
# stream programmatically so that we can test for errors for an
# existing stream.
stream2 = self.make_stream("stream2")
moderators_group = NamedUserGroup.objects.get(
name=SystemGroups.MODERATORS, realm=realm, is_system_group=True
)
do_change_stream_group_based_setting(
stream2, "can_add_subscribers_group", moderators_group, acting_user=None
)
result = self.common_subscribe_to_streams(
self.test_user,
["stream2"],
{"principals": orjson.dumps([invitee_user_id]).decode()},
allow_fail=True,
)
self.assert_json_error(result, "Insufficient permission")
do_change_user_role(self.test_user, UserProfile.ROLE_MODERATOR, acting_user=None)
self.common_subscribe_to_streams(
self.test_user, ["stream2"], {"principals": orjson.dumps([invitee_user_id]).decode()}
)
self.unsubscribe(user_profile, "stream2")
members_group = NamedUserGroup.objects.get(
name=SystemGroups.MEMBERS, realm=realm, is_system_group=True
)
do_change_stream_group_based_setting(
stream2, "can_add_subscribers_group", members_group, acting_user=None
)
do_change_user_role(self.test_user, UserProfile.ROLE_GUEST, acting_user=None)
result = self.common_subscribe_to_streams(
self.test_user,
["stream2"],
{"principals": orjson.dumps([invitee_user_id]).decode()},
allow_fail=True,
)
self.assert_json_error(result, "Not allowed for guest users")
do_change_user_role(self.test_user, UserProfile.ROLE_MEMBER, acting_user=None)
self.common_subscribe_to_streams(
self.test_user,
["stream2"],
{"principals": orjson.dumps([self.test_user.id, invitee_user_id]).decode()},
)
self.unsubscribe(user_profile, "stream2")
# User should be able to subscribe other users if they have
# permissions to administer the channel.
do_change_stream_group_based_setting(
stream2, "can_add_subscribers_group", nobody_group, acting_user=None
)
do_change_stream_group_based_setting(
stream2, "can_administer_channel_group", members_group, acting_user=None
)
self.common_subscribe_to_streams(
self.test_user,
["stream2"],
{"principals": orjson.dumps([self.test_user.id, invitee_user_id]).decode()},
)
self.unsubscribe(user_profile, "stream2")
do_change_stream_group_based_setting(
stream2, "can_administer_channel_group", nobody_group, acting_user=None
)
full_members_group = NamedUserGroup.objects.get(
name=SystemGroups.FULL_MEMBERS, realm=realm, is_system_group=True
)
do_change_stream_group_based_setting(
stream2, "can_add_subscribers_group", full_members_group, acting_user=None
)
do_set_realm_property(realm, "waiting_period_threshold", 100000, acting_user=None)
result = self.common_subscribe_to_streams(
self.test_user,
["stream2"],
{"principals": orjson.dumps([invitee_user_id]).decode()},
allow_fail=True,
)
self.assert_json_error(result, "Insufficient permission")
do_set_realm_property(realm, "waiting_period_threshold", 0, acting_user=None)
self.common_subscribe_to_streams(
self.test_user, ["stream2"], {"principals": orjson.dumps([invitee_user_id]).decode()}
)
self.unsubscribe(user_profile, "stream2")
named_user_group = check_add_user_group(
realm, "named_user_group", [self.test_user], acting_user=self.test_user
)
do_change_stream_group_based_setting(
stream2,
"can_add_subscribers_group",
named_user_group,
acting_user=None,
)
self.common_subscribe_to_streams(
self.test_user,
["stream2"],
{"principals": orjson.dumps([invitee_user_id]).decode()},
)
self.unsubscribe(user_profile, "stream2")
anonymous_group = self.create_or_update_anonymous_group_for_setting([self.test_user], [])
do_change_stream_group_based_setting(
stream2,
"can_add_subscribers_group",
anonymous_group,
acting_user=None,
)
self.common_subscribe_to_streams(
self.test_user,
["stream2"],
{"principals": orjson.dumps([invitee_user_id]).decode()},
)
self.unsubscribe(user_profile, "stream2")
private_stream = self.make_stream("private_stream", invite_only=True)
do_change_stream_group_based_setting(
private_stream, "can_add_subscribers_group", members_group, acting_user=None
)
result = self.common_subscribe_to_streams(
self.test_user,
["private_stream"],
{"principals": orjson.dumps([invitee_user_id]).decode()},
allow_fail=True,
)
self.assert_json_error(result, "Unable to access channel (private_stream).")
def test_subscriptions_add_invalid_stream(self) -> None:
"""
Calling POST /json/users/me/subscriptions on a stream whose name is invalid (as
@@ -5131,7 +5339,7 @@ class SubscriptionAPITest(ZulipTestCase):
streams_to_sub = ["multi_user_stream"]
with (
self.capture_send_event_calls(expected_num_events=5) as events,
self.assert_database_query_count(44),
self.assert_database_query_count(42),
):
self.common_subscribe_to_streams(
self.test_user,
@@ -5316,12 +5524,18 @@ class SubscriptionAPITest(ZulipTestCase):
# Verify the internal checks also block guest users.
stream = get_stream("Denmark", guest_user.realm)
self.assertEqual(filter_stream_authorization(guest_user, [stream]), ([], [stream]))
self.assertEqual(
filter_stream_authorization(guest_user, [stream]),
([], [stream], []),
)
stream = self.make_stream("private_stream", invite_only=True)
result = self.common_subscribe_to_streams(guest_user, ["private_stream"], allow_fail=True)
self.assert_json_error(result, "Not allowed for guest users")
self.assertEqual(filter_stream_authorization(guest_user, [stream]), ([], [stream]))
self.assertEqual(
filter_stream_authorization(guest_user, [stream]),
([], [stream], []),
)
web_public_stream = self.make_stream("web_public_stream", is_web_public=True)
public_stream = self.make_stream("public_stream", invite_only=False)
@@ -5337,7 +5551,7 @@ class SubscriptionAPITest(ZulipTestCase):
streams_to_sub = [web_public_stream, public_stream, private_stream]
self.assertEqual(
filter_stream_authorization(guest_user, streams_to_sub),
([web_public_stream], [public_stream, private_stream]),
([web_public_stream], [public_stream, private_stream], []),
)
# Guest can be subscribed by other users.
@@ -5957,7 +6171,7 @@ class SubscriptionAPITest(ZulipTestCase):
]
# Test creating a public stream when realm does not have a notification stream.
with self.assert_database_query_count(44):
with self.assert_database_query_count(42):
self.common_subscribe_to_streams(
self.test_user,
[new_streams[0]],
@@ -5965,7 +6179,7 @@ class SubscriptionAPITest(ZulipTestCase):
)
# Test creating private stream.
with self.assert_database_query_count(48):
with self.assert_database_query_count(46):
self.common_subscribe_to_streams(
self.test_user,
[new_streams[1]],
@@ -5977,7 +6191,7 @@ class SubscriptionAPITest(ZulipTestCase):
new_stream_announcements_stream = get_stream(self.streams[0], self.test_realm)
self.test_realm.new_stream_announcements_stream_id = new_stream_announcements_stream.id
self.test_realm.save()
with self.assert_database_query_count(55):
with self.assert_database_query_count(53):
self.common_subscribe_to_streams(
self.test_user,
[new_streams[2]],

8
zerver/views/invite.py
View File

@@ -20,7 +20,7 @@ from zerver.actions.invites import (
from zerver.decorator import require_member_or_admin
from zerver.lib.exceptions import InvitationError, JsonableError, OrganizationOwnerRequiredError
from zerver.lib.response import json_success
from zerver.lib.streams import access_stream_by_id
from zerver.lib.streams import access_stream_by_id, get_streams_to_which_user_cannot_add_subscribers
from zerver.lib.typed_endpoint import ApiParamConfig, PathOnly, typed_endpoint
from zerver.lib.typed_endpoint_validators import check_int_in_validator
from zerver.lib.user_groups import access_user_group_for_update
@@ -81,6 +81,7 @@ def access_multiuse_invite_by_id(user_profile: UserProfile, invite_id: int) -> M
def access_streams_for_invite(stream_ids: list[int], user_profile: UserProfile) -> list[Stream]:
streams: list[Stream] = []
for stream_id in stream_ids:
try:
(stream, sub) = access_stream_by_id(user_profile, stream_id)
@@ -92,7 +93,10 @@ def access_streams_for_invite(stream_ids: list[int], user_profile: UserProfile)
)
streams.append(stream)
if len(streams) and not user_profile.can_subscribe_other_users():
streams_to_which_user_cannot_add_subscribers = get_streams_to_which_user_cannot_add_subscribers(
streams, user_profile
)
if len(streams_to_which_user_cannot_add_subscribers) > 0:
raise JsonableError(_("You do not have permission to subscribe other users to channels."))
return streams

26
zerver/views/streams.py
View File

@@ -657,16 +657,6 @@ def add_subscriptions_backend(
if len(principals) > 0 and not all(user_id == user_profile.id for user_id in principals):
is_subscribing_other_users = True
if is_subscribing_other_users:
if not user_profile.can_subscribe_other_users():
# Guest users case will not be handled here as it will
# be handled by the decorator above.
raise JsonableError(_("Insufficient permission"))
subscribers = bulk_principals_to_user_profiles(principals, user_profile)
else:
subscribers = {user_profile}
# Validation of the streams arguments, including enforcement of
# can_create_streams policy and check_stream_name policy is inside
# list_to_streams.
@@ -677,15 +667,20 @@ def add_subscriptions_backend(
is_default_stream=is_default_stream,
setting_groups_dict=setting_groups_dict,
)
authorized_streams, unauthorized_streams = filter_stream_authorization(
user_profile, existing_streams
)
(
authorized_streams,
unauthorized_streams,
streams_to_which_user_cannot_add_subscribers,
) = filter_stream_authorization(user_profile, existing_streams, is_subscribing_other_users)
if len(unauthorized_streams) > 0 and authorization_errors_fatal:
raise JsonableError(
_("Unable to access channel ({channel_name}).").format(
channel_name=unauthorized_streams[0].name,
)
)
if len(streams_to_which_user_cannot_add_subscribers) > 0:
raise JsonableError(_("Insufficient permission"))
# Newly created streams are also authorized for the creator
streams = authorized_streams + created_streams
@@ -698,6 +693,11 @@ def add_subscriptions_backend(
_("You can only invite other Zephyr mirroring users to private channels.")
)
if is_subscribing_other_users:
subscribers = bulk_principals_to_user_profiles(principals, user_profile)
else:
subscribers = {user_profile}
if is_default_stream:
for stream in created_streams:
do_add_default_stream(stream)