paulmataruso/zulip
1
0
Fork 0
mirror of https://github.com/zulip/zulip.git synced 2025-11-06 06:53:25 +00:00
Code Issues Packages Projects Releases Wiki Activity

stream: Modify flag to allow access for users with metadata access.

Browse Source
This commit is contained in:
Shubham Padia
2025-02-06 17:33:25 +00:00
committed by Tim Abbott
parent 9725de99e9
commit 35f9305acb
7 changed files with 265 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
analytics/views/stats.py
View File

@@ -186,7 +186,7 @@ def get_chart_data_for_stream(
user_profile, user_profile,
stream_id, stream_id,
require_active=True, require_active=True,
allow_realm_admin=True, require_content_access=False,
) )
return do_get_chart_data( return do_get_chart_data(

2
zerver/actions/streams.py
View File

@@ -915,7 +915,7 @@ def send_subscription_remove_events(
stream stream
for stream in streams_by_user[user_profile.id] for stream in streams_by_user[user_profile.id]
if not check_basic_stream_access( if not check_basic_stream_access(
user_profile, stream, is_subscribed=False, allow_realm_admin=True user_profile, stream, is_subscribed=False, require_content_access=False
) )
] ]

92
zerver/lib/streams.py
View File

@@ -466,9 +466,15 @@ def check_for_exactly_one_stream_arg(stream_id: int | None, stream: str | None)
raise IncompatibleParametersError(["stream_id", "stream"]) raise IncompatibleParametersError(["stream_id", "stream"])
@dataclass
class UserGroupMembershipDetails:
user_recursive_group_ids: set[int] | None
def user_has_content_access( def user_has_content_access(
user_profile: UserProfile, user_profile: UserProfile,
stream: Stream, stream: Stream,
user_group_membership_details: UserGroupMembershipDetails,
*, *,
is_subscribed: bool, is_subscribed: bool,
) -> bool: ) -> bool:
@@ -478,14 +484,17 @@ def user_has_content_access(
if is_subscribed: if is_subscribed:
return True return True
if not stream.invite_only and not user_profile.is_guest: if stream.is_public() and not user_profile.is_guest:
return True return True
user_recursive_group_ids = set( if user_group_membership_details.user_recursive_group_ids is None:
get_recursive_membership_groups(user_profile).values_list("id", flat=True) user_group_membership_details.user_recursive_group_ids = set(
) get_recursive_membership_groups(user_profile).values_list("id", flat=True)
)
if is_user_in_can_add_subscribers_group(stream, user_recursive_group_ids): if is_user_in_can_add_subscribers_group(
stream, user_group_membership_details.user_recursive_group_ids
):
return True return True
return False return False
@@ -514,7 +523,10 @@ def check_stream_access_for_delete_or_update_requiring_metadata_access(
# to the channel for this block, but since we have ruled out # to the channel for this block, but since we have ruled out
# the possibility that the user is a channel admin, checking # the possibility that the user is a channel admin, checking
# for content access will save us valuable DB queries. # for content access will save us valuable DB queries.
if user_has_content_access(user_profile, stream, is_subscribed=sub is not None): user_group_membership_details = UserGroupMembershipDetails(user_recursive_group_ids=None)
if user_has_content_access(
user_profile, stream, user_group_membership_details, is_subscribed=sub is not None
):
raise CannotAdministerChannelError raise CannotAdministerChannelError
raise JsonableError(error) raise JsonableError(error)
@@ -555,38 +567,49 @@ def check_basic_stream_access(
stream: Stream, stream: Stream,
*, *,
is_subscribed: bool, is_subscribed: bool,
allow_realm_admin: bool = False, require_content_access: bool = True,
) -> bool: ) -> bool:
# Any realm user, even guests, can access web_public streams. user_group_membership_details = UserGroupMembershipDetails(user_recursive_group_ids=None)
if stream.is_web_public: if user_has_content_access(
user_profile, stream, user_group_membership_details, is_subscribed=is_subscribed
):
return True return True
# If the stream is in your realm and public, you can access it. if not require_content_access:
if stream.is_public() and not user_profile.is_guest: if user_profile.is_realm_admin:
return True return True
# Or if you are subscribed to the stream, you can access it. # This will not get fired in practice since we will only arrive
if is_subscribed: # at this if block if `user_has_content_access` returns False.
return True # In every case that `user_has_content_access` returns False,
# we already would have calculated `user_recursive_group_ids`.
# For some specific callers (e.g. getting list of subscribers, # It is still good to keep this around in case there are
# removing other users from a stream, and updating stream name and # changes in that function.
# description), we allow realm admins to access stream even if if user_group_membership_details.user_recursive_group_ids is None: # nocoverage
# they are not subscribed to a private stream. user_group_membership_details.user_recursive_group_ids = set( # nocoverage
if user_profile.is_realm_admin and allow_realm_admin: get_recursive_membership_groups(user_profile).values_list(
return True "id", flat=True
) # nocoverage
) # nocoverage
if has_metadata_access_to_channel_via_groups(
user_group_membership_details.user_recursive_group_ids,
stream.can_administer_channel_group_id,
stream.can_add_subscribers_group_id,
):
return True
return False return False
# Only set allow_realm_admin flag to True when you want to allow realm admin to # Only set require_content_access flag to False when you want
# access unsubscribed private stream content. # to allow users with metadata access to access unsubscribed private
# stream content.
def access_stream_common( def access_stream_common(
user_profile: UserProfile, user_profile: UserProfile,
stream: Stream, stream: Stream,
error: str, error: str,
require_active: bool = True, require_active: bool = True,
allow_realm_admin: bool = False, require_content_access: bool = True,
) -> Subscription | None: ) -> Subscription | None:
"""Common function for backend code where the target use attempts to """Common function for backend code where the target use attempts to
access the target stream, returning all the data fetched along the access the target stream, returning all the data fetched along the
@@ -608,7 +631,10 @@ def access_stream_common(
sub = None sub = None
if not stream.deactivated and check_basic_stream_access( if not stream.deactivated and check_basic_stream_access(
user_profile, stream, is_subscribed=sub is not None, allow_realm_admin=allow_realm_admin user_profile,
stream,
is_subscribed=sub is not None,
require_content_access=require_content_access,
): ):
return sub return sub
@@ -621,7 +647,7 @@ def access_stream_by_id(
user_profile: UserProfile, user_profile: UserProfile,
stream_id: int, stream_id: int,
require_active: bool = True, require_active: bool = True,
allow_realm_admin: bool = False, require_content_access: bool = True,
) -> tuple[Stream, Subscription | None]: ) -> tuple[Stream, Subscription | None]:
error = _("Invalid channel ID") error = _("Invalid channel ID")
try: try:
@@ -634,7 +660,7 @@ def access_stream_by_id(
stream, stream,
error, error,
require_active=require_active, require_active=require_active,
allow_realm_admin=allow_realm_admin, require_content_access=require_content_access,
) )
return (stream, sub) return (stream, sub)
@@ -643,7 +669,7 @@ def access_stream_by_id_for_message(
user_profile: UserProfile, user_profile: UserProfile,
stream_id: int, stream_id: int,
require_active: bool = True, require_active: bool = True,
allow_realm_admin: bool = False, require_content_access: bool = True,
) -> tuple[Stream, Subscription | None]: ) -> tuple[Stream, Subscription | None]:
""" """
Variant of access_stream_by_id that uses get_stream_by_id_for_sending_message Variant of access_stream_by_id that uses get_stream_by_id_for_sending_message
@@ -660,7 +686,7 @@ def access_stream_by_id_for_message(
stream, stream,
error, error,
require_active=require_active, require_active=require_active,
allow_realm_admin=allow_realm_admin, require_content_access=require_content_access,
) )
return (stream, sub) return (stream, sub)
@@ -697,7 +723,7 @@ def check_stream_name_available(realm: Realm, name: str) -> None:
def access_stream_by_name( def access_stream_by_name(
user_profile: UserProfile, stream_name: str, allow_realm_admin: bool = False user_profile: UserProfile, stream_name: str, require_content_access: bool = True
) -> tuple[Stream, Subscription | None]: ) -> tuple[Stream, Subscription | None]:
error = _("Invalid channel name '{channel_name}'").format(channel_name=stream_name) error = _("Invalid channel name '{channel_name}'").format(channel_name=stream_name)
try: try:
@@ -709,7 +735,7 @@ def access_stream_by_name(
user_profile, user_profile,
stream, stream,
error, error,
allow_realm_admin=allow_realm_admin, require_content_access=require_content_access,
) )
return (stream, sub) return (stream, sub)
@@ -883,7 +909,7 @@ def bulk_can_remove_subscribers_from_streams(
assert stream.recipient_id is not None assert stream.recipient_id is not None
is_subscribed = stream.recipient_id in sub_recipient_ids is_subscribed = stream.recipient_id in sub_recipient_ids
if not check_basic_stream_access( if not check_basic_stream_access(
user_profile, stream, is_subscribed=is_subscribed, allow_realm_admin=True user_profile, stream, is_subscribed=is_subscribed, require_content_access=False
): ):
return False return False

195
zerver/tests/test_subs.py
View File

@@ -72,6 +72,7 @@ from zerver.lib.stream_traffic import (
from zerver.lib.streams import ( from zerver.lib.streams import (
StreamDict, StreamDict,
StreamsCategorizedByPermissions, StreamsCategorizedByPermissions,
UserGroupMembershipDetails,
access_stream_by_id, access_stream_by_id,
access_stream_by_name, access_stream_by_name,
can_access_stream_history, can_access_stream_history,
@@ -3247,7 +3248,7 @@ class StreamAdminTest(ZulipTestCase):
are on. are on.
""" """
result = self.attempt_unsubscribe_of_principal( result = self.attempt_unsubscribe_of_principal(
query_count=18, query_count=19,
target_users=[self.example_user("cordelia")], target_users=[self.example_user("cordelia")],
is_realm_admin=True, is_realm_admin=True,
is_subbed=True, is_subbed=True,
@@ -3264,7 +3265,7 @@ class StreamAdminTest(ZulipTestCase):
streams you aren't on. streams you aren't on.
""" """
result = self.attempt_unsubscribe_of_principal( result = self.attempt_unsubscribe_of_principal(
query_count=18, query_count=19,
target_users=[self.example_user("cordelia")], target_users=[self.example_user("cordelia")],
is_realm_admin=True, is_realm_admin=True,
is_subbed=False, is_subbed=False,
@@ -5991,7 +5992,7 @@ class SubscriptionAPITest(ZulipTestCase):
# Sends 3 peer-remove events, 2 unsubscribe events # Sends 3 peer-remove events, 2 unsubscribe events
# and 2 stream delete events for private streams. # and 2 stream delete events for private streams.
with ( with (
self.assert_database_query_count(18), self.assert_database_query_count(20),
self.assert_memcached_count(3), self.assert_memcached_count(3),
self.capture_send_event_calls(expected_num_events=7) as events, self.capture_send_event_calls(expected_num_events=7) as events,
): ):
@@ -7782,6 +7783,80 @@ class AccessStreamTest(ZulipTestCase):
access_stream_by_id(sipbtest, mit_stream.id) access_stream_by_id(sipbtest, mit_stream.id)
access_stream_by_name(sipbtest, mit_stream.name) access_stream_by_name(sipbtest, mit_stream.name)
def test_access_stream_allow_metadata_access_flag(self) -> None:
"""
A comprehensive security test for the access_stream_by_* API functions.
"""
# Create a private stream for which Hamlet is the only subscriber.
hamlet = self.example_user("hamlet")
stream_name = "new_private_stream"
self.login_user(hamlet)
self.subscribe_via_post(hamlet, [stream_name], invite_only=True)
stream = get_stream(stream_name, hamlet.realm)
othello = self.example_user("othello")
iago = self.example_user("iago")
# Realm admin cannot access the private stream
with self.assertRaisesRegex(JsonableError, "Invalid channel ID"):
access_stream_by_id(iago, stream.id)
with self.assertRaisesRegex(JsonableError, "Invalid channel name 'new_private_stream'"):
access_stream_by_name(iago, stream.name)
# Realm admins can access private stream if
# require_content_access set to False
access_stream_by_id(iago, stream.id, require_content_access=False)
access_stream_by_name(iago, stream.name, require_content_access=False)
# Normal unsubscribed user cannot access a private stream
with self.assertRaisesRegex(JsonableError, "Invalid channel ID"):
access_stream_by_id(othello, stream.id)
with self.assertRaisesRegex(JsonableError, "Invalid channel name 'new_private_stream'"):
access_stream_by_name(othello, stream.name)
# Normal unsubscribed user cannot access a private stream with
# require_content_access set to False
with self.assertRaisesRegex(JsonableError, "Invalid channel ID"):
access_stream_by_id(othello, stream.id, require_content_access=False)
with self.assertRaisesRegex(JsonableError, "Invalid channel name 'new_private_stream'"):
access_stream_by_name(othello, stream.name, require_content_access=False)
othello_group = check_add_user_group(
othello.realm, "user_profile_group", [othello], acting_user=othello
)
nobody_group = NamedUserGroup.objects.get(
name="role:nobody", is_system_group=True, realm=othello.realm
)
do_change_stream_group_based_setting(
stream,
"can_administer_channel_group",
othello_group,
acting_user=None,
)
# Channel admins can access private stream if
# require_content_access is set to False
access_stream_by_id(othello, stream.id, require_content_access=False)
access_stream_by_name(othello, stream.name, require_content_access=False)
do_change_stream_group_based_setting(
stream,
"can_administer_channel_group",
nobody_group,
acting_user=None,
)
do_change_stream_group_based_setting(
stream,
"can_add_subscribers_group",
othello_group,
acting_user=None,
)
# Users in `can_add_subscribers_group` can access private
# stream if require_content_access is set to True
access_stream_by_id(othello, stream.id, require_content_access=False)
access_stream_by_name(othello, stream.name, require_content_access=False)
def test_stream_access_by_guest(self) -> None: def test_stream_access_by_guest(self) -> None:
guest_user_profile = self.example_user("polonius") guest_user_profile = self.example_user("polonius")
self.login_user(guest_user_profile) self.login_user(guest_user_profile)
@@ -7830,26 +7905,90 @@ class AccessStreamTest(ZulipTestCase):
# Even guest user should have access to web public channel. # Even guest user should have access to web public channel.
self.assertEqual( self.assertEqual(
user_has_content_access(guest_user, web_public_stream, is_subscribed=False), True user_has_content_access(
guest_user,
web_public_stream,
user_group_membership_details=UserGroupMembershipDetails(
user_recursive_group_ids=None
),
is_subscribed=False,
),
True,
) )
# User should have access to private channel if they are # User should have access to private channel if they are
# subscribed to it # subscribed to it
self.assertEqual(user_has_content_access(aaron, private_stream, is_subscribed=True), True) self.assertEqual(
self.assertEqual(user_has_content_access(aaron, private_stream, is_subscribed=False), False) user_has_content_access(
aaron,
private_stream,
user_group_membership_details=UserGroupMembershipDetails(
user_recursive_group_ids=None
),
is_subscribed=True,
),
True,
)
self.assertEqual(
user_has_content_access(
aaron,
private_stream,
user_group_membership_details=UserGroupMembershipDetails(
user_recursive_group_ids=None
),
is_subscribed=False,
),
False,
)
# Non guest user should have access to public channel # Non guest user should have access to public channel
# regardless of their subscription to the channel. # regardless of their subscription to the channel.
self.assertEqual(user_has_content_access(aaron, public_stream, is_subscribed=True), True) self.assertEqual(
self.assertEqual(user_has_content_access(aaron, public_stream, is_subscribed=False), True) user_has_content_access(
aaron,
public_stream,
user_group_membership_details=UserGroupMembershipDetails(
user_recursive_group_ids=None
),
is_subscribed=True,
),
True,
)
self.assertEqual(
user_has_content_access(
aaron,
public_stream,
user_group_membership_details=UserGroupMembershipDetails(
user_recursive_group_ids=None
),
is_subscribed=False,
),
True,
)
# Guest user should have access to public channel only if they # Guest user should have access to public channel only if they
# are subscribed to it. # are subscribed to it.
self.assertEqual( self.assertEqual(
user_has_content_access(guest_user, public_stream, is_subscribed=False), False user_has_content_access(
guest_user,
public_stream,
user_group_membership_details=UserGroupMembershipDetails(
user_recursive_group_ids=None
),
is_subscribed=False,
),
False,
) )
self.assertEqual( self.assertEqual(
user_has_content_access(guest_user, public_stream, is_subscribed=True), True user_has_content_access(
guest_user,
public_stream,
user_group_membership_details=UserGroupMembershipDetails(
user_recursive_group_ids=None
),
is_subscribed=True,
),
True,
) )
# User should be able to access private channel if they are # User should be able to access private channel if they are
@@ -7862,7 +8001,17 @@ class AccessStreamTest(ZulipTestCase):
aaron_group, aaron_group,
acting_user=None, acting_user=None,
) )
self.assertEqual(user_has_content_access(aaron, private_stream, is_subscribed=False), True) self.assertEqual(
user_has_content_access(
aaron,
private_stream,
user_group_membership_details=UserGroupMembershipDetails(
user_recursive_group_ids=None
),
is_subscribed=False,
),
True,
)
nobody_group = NamedUserGroup.objects.get( nobody_group = NamedUserGroup.objects.get(
name="role:nobody", realm=realm, is_system_group=True name="role:nobody", realm=realm, is_system_group=True
) )
@@ -7883,8 +8032,28 @@ class AccessStreamTest(ZulipTestCase):
aaron_group, aaron_group,
acting_user=None, acting_user=None,
) )
self.assertEqual(user_has_content_access(aaron, private_stream, is_subscribed=False), False) self.assertEqual(
self.assertEqual(user_has_content_access(aaron, private_stream, is_subscribed=True), True) user_has_content_access(
aaron,
private_stream,
user_group_membership_details=UserGroupMembershipDetails(
user_recursive_group_ids=None
),
is_subscribed=False,
),
False,
)
self.assertEqual(
user_has_content_access(
aaron,
private_stream,
user_group_membership_details=UserGroupMembershipDetails(
user_recursive_group_ids=None
),
is_subscribed=True,
),
True,
)
class StreamTrafficTest(ZulipTestCase): class StreamTrafficTest(ZulipTestCase):

12
zerver/views/realm.py
View File

@@ -404,7 +404,7 @@ def update_realm(
new_moderation_request_channel_id = None new_moderation_request_channel_id = None
if moderation_request_channel_id >= 0: if moderation_request_channel_id >= 0:
(new_moderation_request_channel_id, sub) = access_stream_by_id( (new_moderation_request_channel_id, sub) = access_stream_by_id(
user_profile, moderation_request_channel_id, allow_realm_admin=True user_profile, moderation_request_channel_id, require_content_access=False
) )
do_set_realm_moderation_request_channel( do_set_realm_moderation_request_channel(
realm, realm,
@@ -421,7 +421,9 @@ def update_realm(
new_stream_announcements_stream_new = None new_stream_announcements_stream_new = None
if new_stream_announcements_stream_id >= 0: if new_stream_announcements_stream_id >= 0:
(new_stream_announcements_stream_new, sub) = access_stream_by_id( (new_stream_announcements_stream_new, sub) = access_stream_by_id(
user_profile, new_stream_announcements_stream_id, allow_realm_admin=True user_profile,
new_stream_announcements_stream_id,
require_content_access=False,
) )
do_set_realm_new_stream_announcements_stream( do_set_realm_new_stream_announcements_stream(
realm, realm,
@@ -438,7 +440,7 @@ def update_realm(
new_signup_announcements_stream = None new_signup_announcements_stream = None
if signup_announcements_stream_id >= 0: if signup_announcements_stream_id >= 0:
(new_signup_announcements_stream, sub) = access_stream_by_id( (new_signup_announcements_stream, sub) = access_stream_by_id(
user_profile, signup_announcements_stream_id, allow_realm_admin=True user_profile, signup_announcements_stream_id, require_content_access=False
) )
do_set_realm_signup_announcements_stream( do_set_realm_signup_announcements_stream(
realm, realm,
@@ -455,7 +457,9 @@ def update_realm(
new_zulip_update_announcements_stream = None new_zulip_update_announcements_stream = None
if zulip_update_announcements_stream_id >= 0: if zulip_update_announcements_stream_id >= 0:
(new_zulip_update_announcements_stream, sub) = access_stream_by_id( (new_zulip_update_announcements_stream, sub) = access_stream_by_id(
user_profile, zulip_update_announcements_stream_id, allow_realm_admin=True user_profile,
zulip_update_announcements_stream_id,
require_content_access=False,
) )
do_set_realm_zulip_update_announcements_stream( do_set_realm_zulip_update_announcements_stream(
realm, realm,

17
zerver/views/streams.py
View File

@@ -62,6 +62,7 @@ from zerver.lib.retention import parse_message_retention_days
from zerver.lib.stream_traffic import get_streams_traffic from zerver.lib.stream_traffic import get_streams_traffic
from zerver.lib.streams import ( from zerver.lib.streams import (
StreamDict, StreamDict,
UserGroupMembershipDetails,
access_default_stream_group_by_id, access_default_stream_group_by_id,
access_stream_by_id, access_stream_by_id,
access_stream_by_name, access_stream_by_name,
@@ -244,7 +245,7 @@ def remove_default_stream(
(stream, sub) = access_stream_by_id( (stream, sub) = access_stream_by_id(
user_profile, user_profile,
stream_id, stream_id,
allow_realm_admin=True, require_content_access=False,
) )
do_remove_default_stream(stream) do_remove_default_stream(stream)
return json_success(request) return json_success(request)
@@ -411,9 +412,17 @@ def update_stream_backend(
if validate_group_setting_value_change( if validate_group_setting_value_change(
current_setting_api_value, new_setting_value, expected_current_setting_value current_setting_api_value, new_setting_value, expected_current_setting_value
): ):
user_group_membership_details = UserGroupMembershipDetails(
user_recursive_group_ids=None
)
if ( if (
setting_name in Stream.stream_permission_group_settings_requiring_content_access setting_name in Stream.stream_permission_group_settings_requiring_content_access
and not user_has_content_access(user_profile, stream, is_subscribed=sub is not None) and not user_has_content_access(
user_profile,
stream,
user_group_membership_details,
is_subscribed=sub is not None,
)
): ):
raise JsonableError(_("Invalid channel ID")) raise JsonableError(_("Invalid channel ID"))
with transaction.atomic(durable=True): with transaction.atomic(durable=True):
@@ -889,7 +898,7 @@ def get_subscribers_backend(
(stream, sub) = access_stream_by_id( (stream, sub) = access_stream_by_id(
user_profile, user_profile,
stream_id, stream_id,
allow_realm_admin=True, require_content_access=False,
) )
subscribers = get_subscriber_ids(stream, user_profile) subscribers = get_subscriber_ids(stream, user_profile)
@@ -931,7 +940,7 @@ def get_stream_backend(
*, *,
stream_id: PathOnly[int], stream_id: PathOnly[int],
) -> HttpResponse: ) -> HttpResponse:
(stream, sub) = access_stream_by_id(user_profile, stream_id, allow_realm_admin=True) (stream, sub) = access_stream_by_id(user_profile, stream_id, require_content_access=False)
recent_traffic = get_streams_traffic({stream.id}, user_profile.realm) recent_traffic = get_streams_traffic({stream.id}, user_profile.realm)
setting_groups_dict = get_group_setting_value_dict_for_streams([stream]) setting_groups_dict = get_group_setting_value_dict_for_streams([stream])

2
zerver/views/users.py
View File

@@ -859,7 +859,7 @@ def get_subscription_backend(
stream_id: PathOnly[Json[int]], stream_id: PathOnly[Json[int]],
) -> HttpResponse: ) -> HttpResponse:
target_user = access_user_by_id(user_profile, user_id, for_admin=False) target_user = access_user_by_id(user_profile, user_id, for_admin=False)
(stream, sub) = access_stream_by_id(user_profile, stream_id, allow_realm_admin=True) (stream, sub) = access_stream_by_id(user_profile, stream_id, require_content_access=False)
subscription_status = {"is_subscribed": subscribed_to_stream(target_user, stream_id)} subscription_status = {"is_subscribed": subscribed_to_stream(target_user, stream_id)}