From 4eea5e102e7cb4326dd33fd20e16e72d9009fd7d Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Tue, 18 Oct 2022 19:14:36 -0700 Subject: [PATCH] message_fetch: Move ok_to_include_history to zerver.lib.narrow. Signed-off-by: Anders Kaseorg --- zerver/lib/narrow.py | 52 ++++++++++++++++++++++++++++++ zerver/tests/test_message_fetch.py | 2 +- zerver/views/message_fetch.py | 52 +----------------------------- 3 files changed, 54 insertions(+), 52 deletions(-) diff --git a/zerver/lib/narrow.py b/zerver/lib/narrow.py index baea99be2c..d598fa2862 100644 --- a/zerver/lib/narrow.py +++ b/zerver/lib/narrow.py @@ -41,6 +41,8 @@ from zerver.lib.addressee import get_user_profiles, get_user_profiles_by_ids from zerver.lib.exceptions import ErrorCode, JsonableError from zerver.lib.recipient_users import recipient_for_user_profiles from zerver.lib.streams import ( + can_access_stream_history_by_id, + can_access_stream_history_by_name, get_public_streams_queryset, get_stream_by_narrow_operand_access_unchecked, get_web_public_streams_queryset, @@ -704,6 +706,56 @@ def narrow_parameter(var_name: str, json: str) -> OptionalNarrowListT: return list(map(convert_term, data)) +def ok_to_include_history( + narrow: OptionalNarrowListT, user_profile: Optional[UserProfile], is_web_public_query: bool +) -> bool: + # There are occasions where we need to find Message rows that + # have no corresponding UserMessage row, because the user is + # reading a public stream that might include messages that + # were sent while the user was not subscribed, but which they are + # allowed to see. We have to be very careful about constructing + # queries in those situations, so this function should return True + # only if we are 100% sure that we're gonna add a clause to the + # query that narrows to a particular public stream on the user's realm. + # If we screw this up, then we can get into a nasty situation of + # polluting our narrow results with messages from other realms. + + # For web-public queries, we are always returning history. The + # analogues of the below stream access checks for whether streams + # have is_web_public set and banning is operators in this code + # path are done directly in NarrowBuilder. + if is_web_public_query: + assert user_profile is None + return True + + assert user_profile is not None + + include_history = False + if narrow is not None: + for term in narrow: + if term["operator"] == "stream" and not term.get("negated", False): + operand: Union[str, int] = term["operand"] + if isinstance(operand, str): + include_history = can_access_stream_history_by_name(user_profile, operand) + else: + include_history = can_access_stream_history_by_id(user_profile, operand) + elif ( + term["operator"] == "streams" + and term["operand"] == "public" + and not term.get("negated", False) + and user_profile.can_access_public_streams() + ): + include_history = True + # Disable historical messages if the user is narrowing on anything + # that's a property on the UserMessage table. There cannot be + # historical messages in these cases anyway. + for term in narrow: + if term["operator"] == "is": + include_history = False + + return include_history + + def get_stream_from_narrow_access_unchecked( narrow: OptionalNarrowListT, realm: Realm ) -> Optional[Stream]: diff --git a/zerver/tests/test_message_fetch.py b/zerver/tests/test_message_fetch.py index 3fff68724b..ac1b1c8cf4 100644 --- a/zerver/tests/test_message_fetch.py +++ b/zerver/tests/test_message_fetch.py @@ -32,6 +32,7 @@ from zerver.lib.narrow import ( build_narrow_filter, exclude_muting_conditions, is_spectator_compatible, + ok_to_include_history, ) from zerver.lib.sqlalchemy_utils import get_sqlalchemy_connection from zerver.lib.streams import StreamDict, create_streams_if_needed, get_public_streams_queryset @@ -57,7 +58,6 @@ from zerver.views.message_fetch import ( LARGER_THAN_MAX_MESSAGE_ID, find_first_unread_anchor, get_messages_backend, - ok_to_include_history, post_process_limited_query, ) diff --git a/zerver/views/message_fetch.py b/zerver/views/message_fetch.py index fe6ada0fda..8cf3f12f7a 100644 --- a/zerver/views/message_fetch.py +++ b/zerver/views/message_fetch.py @@ -32,11 +32,11 @@ from zerver.lib.narrow import ( is_spectator_compatible, is_web_public_narrow, narrow_parameter, + ok_to_include_history, ) from zerver.lib.request import REQ, RequestNotes, has_request_variables from zerver.lib.response import json_success from zerver.lib.sqlalchemy_utils import get_sqlalchemy_connection -from zerver.lib.streams import can_access_stream_history_by_id, can_access_stream_history_by_name from zerver.lib.topic import DB_TOPIC_NAME, MATCH_TOPIC, topic_column_sa from zerver.lib.utils import statsd from zerver.lib.validator import check_bool, check_int, check_list, to_non_negative_int @@ -95,56 +95,6 @@ def get_search_fields( } -def ok_to_include_history( - narrow: OptionalNarrowListT, user_profile: Optional[UserProfile], is_web_public_query: bool -) -> bool: - # There are occasions where we need to find Message rows that - # have no corresponding UserMessage row, because the user is - # reading a public stream that might include messages that - # were sent while the user was not subscribed, but which they are - # allowed to see. We have to be very careful about constructing - # queries in those situations, so this function should return True - # only if we are 100% sure that we're gonna add a clause to the - # query that narrows to a particular public stream on the user's realm. - # If we screw this up, then we can get into a nasty situation of - # polluting our narrow results with messages from other realms. - - # For web-public queries, we are always returning history. The - # analogues of the below stream access checks for whether streams - # have is_web_public set and banning is operators in this code - # path are done directly in NarrowBuilder. - if is_web_public_query: - assert user_profile is None - return True - - assert user_profile is not None - - include_history = False - if narrow is not None: - for term in narrow: - if term["operator"] == "stream" and not term.get("negated", False): - operand: Union[str, int] = term["operand"] - if isinstance(operand, str): - include_history = can_access_stream_history_by_name(user_profile, operand) - else: - include_history = can_access_stream_history_by_id(user_profile, operand) - elif ( - term["operator"] == "streams" - and term["operand"] == "public" - and not term.get("negated", False) - and user_profile.can_access_public_streams() - ): - include_history = True - # Disable historical messages if the user is narrowing on anything - # that's a property on the UserMessage table. There cannot be - # historical messages in these cases anyway. - for term in narrow: - if term["operator"] == "is": - include_history = False - - return include_history - - def find_first_unread_anchor( sa_conn: Connection, user_profile: Optional[UserProfile], narrow: OptionalNarrowListT ) -> int: