From 463ecb375faeec6c5cd160e3e203df885b846089 Mon Sep 17 00:00:00 2001 From: Priyank Patel Date: Sun, 11 Aug 2019 16:57:54 +0000 Subject: [PATCH] refactor: Add get_stream_by_narrow_operand_access_unchecked. This let's us clean up the linter that excludes the use of get_stream and by adding the access_unchecked in the name we make it clear that it should be used with caution. Refactoring idea by Tim Abbott. --- tools/linter_lib/custom_check.py | 4 ---- zerver/lib/streams.py | 13 +++++++++++-- zerver/views/messages.py | 21 +++++++++++---------- 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/tools/linter_lib/custom_check.py b/tools/linter_lib/custom_check.py index 49f3f06a6c..31e6f2ebe2 100644 --- a/tools/linter_lib/custom_check.py +++ b/tools/linter_lib/custom_check.py @@ -395,10 +395,6 @@ python_rules = RuleList( # how most instances are written, but better to exclude something than nothing ('zerver/lib/actions.py', 'stream = get_stream(stream_name, realm)'), ('zerver/lib/actions.py', 'get_stream(admin_realm_signup_notifications_stream, admin_realm)'), - # Here we need get_stream to access streams you've since unsubscribed from. - ('zerver/views/messages.py', 'stream = get_stream(operand, self.user_profile.realm)'), - # Use stream_id to exclude mutes. - ('zerver/views/messages.py', 'stream_id = get_stream(stream_name, user_profile.realm).id'), ]), 'description': 'Please use access_stream_by_*() to fetch Stream objects', }, diff --git a/zerver/lib/streams.py b/zerver/lib/streams.py index 4577d74f00..93ab7b00ae 100644 --- a/zerver/lib/streams.py +++ b/zerver/lib/streams.py @@ -1,4 +1,4 @@ -from typing import Any, Iterable, List, Mapping, Set, Tuple, Optional +from typing import Any, Iterable, List, Mapping, Set, Tuple, Optional, Union from django.utils.translation import ugettext as _ @@ -6,7 +6,7 @@ from zerver.lib.actions import check_stream_name, create_streams_if_needed from zerver.lib.request import JsonableError from zerver.models import UserProfile, Stream, Subscription, \ Realm, Recipient, bulk_get_recipients, get_stream_recipient, get_stream, \ - bulk_get_streams, get_realm_stream, DefaultStreamGroup + bulk_get_streams, get_realm_stream, DefaultStreamGroup, get_stream_by_id_in_realm def check_for_exactly_one_stream_arg(stream_id: Optional[int], stream: Optional[str]) -> None: if stream_id is None and stream is None: @@ -282,3 +282,12 @@ def access_default_stream_group_by_id(realm: Realm, group_id: int) -> DefaultStr return DefaultStreamGroup.objects.get(realm=realm, id=group_id) except DefaultStreamGroup.DoesNotExist: raise JsonableError(_("Default stream group with id '%s' does not exist.") % (group_id,)) + +def get_stream_by_narrow_operand_access_unchecked(operand: Union[str, int], realm: Realm) -> Stream: + """This is required over access_stream_* in certain cases where + we need the stream data only to prepare a response that user can access + and not send it out to unauthorized recipients. + """ + if isinstance(operand, str): + return get_stream(operand, realm) + return get_stream_by_id_in_realm(operand, realm) diff --git a/zerver/views/messages.py b/zerver/views/messages.py index 7e35a6967b..9fa39c9507 100644 --- a/zerver/views/messages.py +++ b/zerver/views/messages.py @@ -30,7 +30,8 @@ from zerver.lib.message import ( ) from zerver.lib.response import json_success, json_error from zerver.lib.sqlalchemy_utils import get_sqlalchemy_connection -from zerver.lib.streams import access_stream_by_id, can_access_stream_history_by_name +from zerver.lib.streams import access_stream_by_id, can_access_stream_history_by_name, \ + get_stream_by_narrow_operand_access_unchecked from zerver.lib.timestamp import datetime_to_timestamp, convert_to_UTC from zerver.lib.timezone import get_timezone from zerver.lib.topic import ( @@ -50,8 +51,8 @@ from zerver.lib.validator import \ from zerver.lib.zephyr import compute_mit_user_fullname from zerver.models import Message, UserProfile, Stream, Subscription, Client,\ Realm, RealmDomain, Recipient, UserMessage, bulk_get_recipients, get_personal_recipient, \ - get_stream, email_to_domain, get_realm, get_active_streams, \ - get_user_including_cross_realm, get_user_by_id_in_realm_including_cross_realm, get_stream_recipient + email_to_domain, get_realm, get_active_streams, get_user_including_cross_realm, \ + get_user_by_id_in_realm_including_cross_realm, get_stream_recipient from sqlalchemy import func from sqlalchemy.sql import select, join, column, literal_column, literal, and_, \ @@ -207,8 +208,8 @@ class NarrowBuilder: try: # Because you can see your own message history for # private streams you are no longer subscribed to, we - # need get_stream, not access_stream, here. - stream = get_stream(operand, self.user_profile.realm) + # need get_stream_by_narrow_operand_access_unchecked here. + stream = get_stream_by_narrow_operand_access_unchecked(operand, self.user_profile.realm) except Stream.DoesNotExist: raise BadNarrowOperator('unknown stream ' + operand) @@ -593,11 +594,11 @@ def exclude_muting_conditions(user_profile: UserProfile, stream_id = None if stream_name is not None: try: - # Note that this code works around a lint rule that - # says we should use access_stream_by_name to get the - # stream. It is okay here, because we are only using - # the stream id to exclude data, not to include results. - stream_id = get_stream(stream_name, user_profile.realm).id + # Note: It is okay here to not check access to stream + # because we are only using the stream id to exclude data, + # not to include results. + stream = get_stream_by_narrow_operand_access_unchecked(stream_name, user_profile.realm) + stream_id = stream.id except Stream.DoesNotExist: pass