narrow: Update with operator behaviour when message not accessible.

Previously, when the message of the "with" operator can not be
accessed by the user, it used to return messages in combined feed
or stream feed.

This commit updates it to rather raise a BadNarrowOperatorError
if the message of "with" operand is not accessible to user, and the
narrow terms present can not define a conversation. However, if
the narrow terms can define a conversation, then the narrow falls
back to that of without the "with" operator.

api_docs/construct-narrow.md

@@ -106,14 +106,21 @@ number or a string.
 The `id` operator returns the message with the specified ID if it exists,
 and if it can be accessed by the user.
 
-The `with` operator is designed to be used for permanent links to topics,
+The `with` operator is designed to be used for permanent links to
-which means they should continue to work when the topic is
+topics, which means they should continue to work when the topic is
 [moved](/help/move-content-to-another-topic) or
-[resolved](/help/resolve-a-topic). If the message with the specified ID
+[resolved](/help/resolve-a-topic). If the message with the specified
-exists, and can be accessed by the user, then it will return messages
+ID exists, and can be accessed by the user, then it will return
-with the `channel`/`topic`/`dm` operators corresponding to the current
+messages with the `channel`/`topic`/`dm` operators corresponding to
-conversation containing that message, and replacing any such filters
+the current conversation containing that message, replacing any such
-included in the narrow.
+operators included in the original narrow query.
+
+If no such message exists, or the message ID represents a message that
+is inaccessible to the user, this operator will be ignored (rather
+than throwing an error) if the remaining operators uniquely identify a
+conversation (i.e., they contain `channel` and `topic` terms or `dm`
+term). This behavior is intended to provide the best possible
+experience for links to private channels with protected history.
 
 The [help center](/help/search-for-messages#search-by-message-id) also
 documents the `near` operator for searching for messages by ID, but

zerver/lib/narrow.py

@@ -878,13 +878,38 @@ def get_channel_from_narrow_access_unchecked(
     return None
 
 
+# This function verifies if the current narrow has the necessary
+# terms to point to a channel or a direct message conversation.
+def can_narrow_define_conversation(narrow: list[NarrowParameter]) -> bool:
+    contains_channel_term = False
+    contains_topic_term = False
+
+    for term in narrow:
+        if term.operator in ["dm", "pm-with"]:
+            return True
+
+        elif term.operator in ["stream", "channel"]:
+            contains_channel_term = True
+
+        elif term.operator == "topic":
+            contains_topic_term = True
+
+        if contains_channel_term and contains_topic_term:
+            return True
+
+    return False
+
+
 # This function implements the core logic of the `with` operator,
 # which is designed to support permanent links to a topic that
 # robustly function if the topic is moved.
 #
 # The with operator accepts a message ID as an operand. If the
 # message ID does not exist or is otherwise not accessible to the
-# current user, then it has no effect.
+# current user, then if the remaining narrow terms can point to
+# a conversation then the narrow corresponding to it is returned.
+# If the remaining terms can not point to a particular conversation,
+# then a BadNarrowOperatorError is raised.
 #
 # Otherwise, the narrow terms are mutated to remove any
 # channel/topic/dm operators, replacing them with the appropriate
@@ -898,6 +923,7 @@ def update_narrow_terms_containing_with_operator(
         return narrow
 
     with_operator_terms = list(filter(lambda term: term.operator == "with", narrow))
+    can_user_access_target_message = True
 
     if len(with_operator_terms) > 1:
         raise InvalidOperatorCombinationError(_("Duplicate 'with' operators."))
@@ -916,12 +942,22 @@ def update_narrow_terms_containing_with_operator(
         try:
             message = access_message(maybe_user_profile, message_id)
         except JsonableError:
-            return narrow
+            can_user_access_target_message = False
     else:
         try:
             message = access_web_public_message(realm, message_id)
         except MissingAuthenticationError:
+            can_user_access_target_message = False
+
+    # If the user can not access the target message we fall back to the
+    # conversation specified by those other operators if they're enough
+    # to specify a single conversation.
+    # Else, we raise a BadNarrowOperatorError.
+    if not can_user_access_target_message:
+        if can_narrow_define_conversation(narrow):
             return narrow
+        else:
+            raise BadNarrowOperatorError(_("Invalid 'with' operator"))
 
     # TODO: It would be better if the legacy names here are canonicalized
     # while building a NarrowParameter.

zerver/tests/test_message_fetch.py

@@ -2800,9 +2800,14 @@ class GetOldMessagesTest(ZulipTestCase):
         self.make_stream("dev team", invite_only=True, history_public_to_subscribers=False)
         self.subscribe(iago, "dev team")
 
+        self.make_stream("public")
+        self.subscribe(hamlet, "public")
+
         # Test `with` operator effective when targeting a topic with
         # message which can be accessed by the user.
         msg_id = self.send_stream_message(iago, "dev team", topic_name="test")
+        msg_id_2 = self.send_stream_message(hamlet, "public", topic_name="test")
+        dm_msg_id = self.send_personal_message(hamlet, iago, "direct message")
 
         narrow = [
             dict(operator="channel", operand="dev team"),
@@ -2823,7 +2828,10 @@ class GetOldMessagesTest(ZulipTestCase):
 
         # Test `with` operator ineffective when targeting a topic with
         # message that can not be accessed by the user.
+        #
         # Since !history_public_to_subscribers, hamlet cannot view.
+        # Hence, it falls back to the narrow without the `with`
+        # operator since it can alone define a conversation.
         self.subscribe(hamlet, "dev team")
         self.login("hamlet")
 
@@ -2835,22 +2843,59 @@ class GetOldMessagesTest(ZulipTestCase):
         results = self.get_and_check_messages(dict(narrow=orjson.dumps(narrow).decode()))
         self.assert_length(results["messages"], 0)
 
-        # Same result with topic specified incorrectly
+        narrow = [
+            dict(operator="channel", operand="public"),
+            dict(operator="topic", operand="test"),
+            dict(operator="with", operand=msg_id),
+        ]
+        results = self.get_and_check_messages(dict(narrow=orjson.dumps(narrow).decode()))
+        self.assert_length(results["messages"], 1)
+        self.assertEqual(results["messages"][0]["id"], msg_id_2)
+
+        # Since `dm` operator alone can also define conversation,
+        # narrow falls back to `dm` since hamlet can't access
+        # msg_id.
+        narrow = [
+            dict(operator="dm", operand=iago.email),
+            dict(operator="with", operand=msg_id),
+        ]
+        results = self.get_and_check_messages(dict(narrow=orjson.dumps(narrow).decode()))
+        self.assert_length(results["messages"], 1)
+        self.assertEqual(results["messages"][0]["id"], dm_msg_id)
+
+        # However, if the narrow can not define conversation,
+        # and the target message is not accessible to user,
+        # then BadNarrowOperatorError is raised.
+        #
+        # narrow can't define conversation due to missing topic term.
         narrow = [
             dict(operator="channel", operand="dev team"),
-            dict(operator="topic", operand="wrong_guess"),
             dict(operator="with", operand=msg_id),
         ]
-        results = self.get_and_check_messages(dict(narrow=orjson.dumps(narrow).decode()))
+        post_params = {
-        self.assert_length(results["messages"], 0)
+            "anchor": msg_id,
+            "num_before": 0,
+            "num_after": 5,
+            "narrow": orjson.dumps(narrow).decode(),
+        }
+        result = self.client_get("/json/messages", dict(post_params))
+        self.assert_json_error(result, "Invalid narrow operator: Invalid 'with' operator")
 
-        # If just with is specified, we get messages a la combined feed,
+        # narrow can't define conversation due to missing channel term.
-        # but not the target message.
+        narrow = [
+            dict(operator="topic", operand="test"),
+            dict(operator="with", operand=msg_id),
+        ]
+        result = self.client_get("/json/messages", dict(post_params))
+        self.assert_json_error(result, "Invalid narrow operator: Invalid 'with' operator")
+
+        # narrow can't define conversation due to missing channel-topic
+        # terms or dm terms.
         narrow = [
             dict(operator="with", operand=msg_id),
         ]
-        results = self.get_and_check_messages(dict(narrow=orjson.dumps(narrow).decode()))
+        result = self.client_get("/json/messages", dict(post_params))
-        self.assertNotIn(msg_id, [message["id"] for message in results["messages"]])
+        self.assert_json_error(result, "Invalid narrow operator: Invalid 'with' operator")
 
         # Test `with` operator is effective when targeting personal
         # messages with message id, and returns messages of that narrow.
@@ -2867,7 +2912,7 @@ class GetOldMessagesTest(ZulipTestCase):
         results = self.get_and_check_messages(dict(narrow=orjson.dumps(with_narrow).decode()))
         self.assertNotIn(msg_id, [message["id"] for message in results["messages"]])
 
-        # Now switch to a user how does have access.
+        # Now switch to a user who does have access.
         self.login("iago")
         with_narrow = [
             # Important: We pass the wrong conversation.