attachments: Allow seeing attachments to users with content access.

Fixes https://chat.zulip.org/#narrow/channel/9-issues/topic/Can't.20view.20images.20in.20private.20channel.2E

(cherry picked from commit 700da670cf)

zerver/lib/attachments.py

@@ -8,7 +8,9 @@ from django.utils.timezone import now as timezone_now
 from django.utils.translation import gettext as _
 
 from zerver.lib.exceptions import JsonableError, RateLimitedError
+from zerver.lib.streams import is_user_in_groups_granting_content_access
 from zerver.lib.upload import delete_message_attachment
+from zerver.lib.user_groups import get_recursive_membership_groups
 from zerver.models import (
     ArchivedAttachment,
     Attachment,
@@ -131,7 +133,7 @@ def validate_attachment_request(
         # Any user in the realm can access realm-public files
         return True, attachment
 
-    messages = attachment.messages.all()
+    messages = attachment.messages.all().select_related("recipient")
 
     usermessages_channel_ids = set()
     usermessage_rows = UserMessage.objects.filter(
@@ -146,30 +148,50 @@ def validate_attachment_request(
             usermessages_channel_ids.add(um.message.recipient.type_id)
 
     # These are subscriptions to a channel one of the messages was sent to
-    relevant_channel_ids = Subscription.objects.filter(
+    subscribed_channel_ids = Subscription.objects.filter(
         user_profile=user_profile,
         active=True,
         recipient__type=Recipient.STREAM,
         recipient__in=[m.recipient_id for m in messages],
     ).values_list("recipient__type_id", flat=True)
 
-    if usermessages_channel_ids & set(relevant_channel_ids):
+    if usermessages_channel_ids.intersection(subscribed_channel_ids):
         # If the attachment was sent in a channel with public
         # or protected history and the user is still subscribed
         # to the channel then anyone who received that message
         # can access it.
         return True, attachment
 
-    # The user didn't receive any of the messages that included this
+    message_channel_ids = set()
-    # attachment. But they might still have access to it, if it was
+    for message in messages:
-    # sent to a stream they are on where history is public to
+        if message.is_stream_message():
-    # subscribers.
+            message_channel_ids.add(message.recipient.type_id)
-    if len(relevant_channel_ids) == 0:
-        return False, attachment
 
-    return Stream.objects.filter(
+    if len(message_channel_ids) > 0:
-        id__in=relevant_channel_ids, history_public_to_subscribers=True
+        message_channels = Stream.objects.filter(id__in=message_channel_ids)
-    ).exists(), attachment
+        for channel in message_channels:
+            # The user didn't receive any of the messages that included
+            # this attachment. But they might still have access to it,
+            # if it was sent to a stream they are subscribed to where
+            # history is public to subscribers.
+            if channel.id in subscribed_channel_ids and channel.is_history_public_to_subscribers():
+                return True, attachment
+
+        user_recursive_group_ids = set(
+            get_recursive_membership_groups(user_profile).values_list("id", flat=True)
+        )
+        for channel in message_channels:
+            if is_user_in_groups_granting_content_access(channel, user_recursive_group_ids):
+                if channel.is_history_public_to_subscribers():
+                    return True, attachment
+                # If the user had received the message at one point of
+                # time, but they are no longer subscribed to a stream
+                # with protected history. They can still access that
+                # message and it's attachment
+                elif channel.id in usermessages_channel_ids:
+                    return True, attachment
+
+    return False, attachment
 
 
 def get_old_unclaimed_attachments(

zerver/tests/test_subs.py

@@ -1658,7 +1658,9 @@ class StreamAdminTest(ZulipTestCase):
         self.assertTrue(attachment.is_realm_public)
 
         # Verify moving a message to a private stream
-        private_stream = self.make_stream("private_stream", realm=realm, invite_only=True)
+        private_stream = self.make_stream(
+            "private_stream", realm=realm, invite_only=True, history_public_to_subscribers=True
+        )
         self.subscribe(owner, "private_stream")
         result = self.client_patch(
             "/json/messages/" + str(msg_id),
@@ -1695,6 +1697,135 @@ class StreamAdminTest(ZulipTestCase):
         attachment.refresh_from_db()
         self.assertTrue(attachment.is_web_public)
 
+    def test_stream_group_permission_changes_updates_updates_attachments(self) -> None:
+        self.login("desdemona")
+        fp = StringIO("zulip!")
+        fp.name = "zulip.txt"
+
+        result = self.client_post("/json/user_uploads", {"file": fp})
+        url = self.assert_json_success(result)["url"]
+
+        owner = self.example_user("desdemona")
+        realm = owner.realm
+        cordelia = self.example_user("cordelia")
+        private_stream_public_history = self.make_stream(
+            "private_stream_public_history",
+            realm=realm,
+            invite_only=True,
+            history_public_to_subscribers=True,
+        )
+        self.subscribe(owner, "private_stream_public_history")
+        body = f"First message ...[zulip.txt](http://{realm.host}" + url + ")"
+        msg_id = self.send_stream_message(owner, "private_stream_public_history", body, "test")
+        attachment = Attachment.objects.get(messages__id=msg_id)
+
+        self.assertFalse(private_stream_public_history.is_web_public)
+        self.assertFalse(attachment.is_web_public)
+        self.assertTrue(private_stream_public_history.invite_only)
+        self.assertFalse(attachment.is_realm_public)
+
+        # User should be able to see the attachment if they have
+        # content access to a channel with public history.
+        for setting_name in Stream.stream_permission_group_settings_requiring_content_access:
+            self.assertFalse(validate_attachment_request(cordelia, attachment.path_id)[0])
+            self.assertTrue(validate_attachment_request(owner, attachment.path_id)[0])
+            do_change_stream_group_based_setting(
+                private_stream_public_history,
+                setting_name,
+                UserGroupMembersData(direct_members=[cordelia.id], direct_subgroups=[]),
+                acting_user=cordelia,
+            )
+            self.assertFalse(check_subscriptions_exists(cordelia, private_stream_public_history))
+            self.assertTrue(validate_attachment_request(cordelia, attachment.path_id)[0])
+            self.assertTrue(validate_attachment_request(owner, attachment.path_id)[0])
+            attachment.refresh_from_db()
+            self.assertFalse(attachment.is_realm_public)
+            nobody_group = NamedUserGroup.objects.get(
+                name="role:nobody", is_system_group=True, realm=realm
+            )
+            do_change_stream_group_based_setting(
+                private_stream_public_history, setting_name, nobody_group, acting_user=cordelia
+            )
+
+        fp = StringIO("zulip2!")
+        fp.name = "zulip2.txt"
+
+        result = self.client_post("/json/user_uploads", {"file": fp})
+        url = self.assert_json_success(result)["url"]
+
+        private_stream_protected_history = self.make_stream(
+            "private_stream_protected_history",
+            realm=realm,
+            invite_only=True,
+            history_public_to_subscribers=False,
+        )
+        self.subscribe(owner, "private_stream_protected_history")
+        body = f"First message ...[zulip2.txt](http://{realm.host}" + url + ")"
+        msg_id = self.send_stream_message(owner, "private_stream_protected_history", body, "test")
+        attachment = Attachment.objects.get(messages__id=msg_id)
+
+        # User should not be able to see the attachment if they have
+        # content access to a private channel with protected history
+        # but were not subscribed to the channel when the upload was
+        # sent.
+        for setting_name in Stream.stream_permission_group_settings_requiring_content_access:
+            self.assertFalse(validate_attachment_request(cordelia, attachment.path_id)[0])
+            self.assertTrue(validate_attachment_request(owner, attachment.path_id)[0])
+            do_change_stream_group_based_setting(
+                private_stream_protected_history,
+                setting_name,
+                UserGroupMembersData(direct_members=[cordelia.id], direct_subgroups=[]),
+                acting_user=cordelia,
+            )
+            self.assertFalse(check_subscriptions_exists(cordelia, private_stream_protected_history))
+            self.assertFalse(validate_attachment_request(cordelia, attachment.path_id)[0])
+            self.assertTrue(validate_attachment_request(owner, attachment.path_id)[0])
+
+            # They should not have access to the upload if they are
+            # subscribed to the channel but they were not subscribed at
+            # the time of upload.
+            self.subscribe(cordelia, "private_stream_protected_history")
+            self.assertFalse(validate_attachment_request(cordelia, attachment.path_id)[0])
+            self.assertTrue(validate_attachment_request(owner, attachment.path_id)[0])
+            self.unsubscribe(cordelia, "private_stream_protected_history")
+            attachment.refresh_from_db()
+
+            nobody_group = NamedUserGroup.objects.get(
+                name="role:nobody", is_system_group=True, realm=realm
+            )
+            do_change_stream_group_based_setting(
+                private_stream_protected_history, setting_name, nobody_group, acting_user=cordelia
+            )
+
+        # User should be able to see the attachment if they have
+        # content access to a private channel with protected history
+        # and were subscribed to the channel when the upload was sent.
+        self.subscribe(cordelia, "private_stream_protected_history")
+        body = f"Second message ...[zulip2.txt](http://{realm.host}" + url + ")"
+        msg_id = self.send_stream_message(owner, "private_stream_protected_history", body, "test")
+        attachment = Attachment.objects.get(messages__id=msg_id)
+        self.unsubscribe(cordelia, "private_stream_protected_history")
+        for setting_name in Stream.stream_permission_group_settings_requiring_content_access:
+            self.assertFalse(validate_attachment_request(cordelia, attachment.path_id)[0])
+            self.assertTrue(validate_attachment_request(owner, attachment.path_id)[0])
+            do_change_stream_group_based_setting(
+                private_stream_protected_history,
+                setting_name,
+                UserGroupMembersData(direct_members=[cordelia.id], direct_subgroups=[]),
+                acting_user=cordelia,
+            )
+            self.assertFalse(check_subscriptions_exists(cordelia, private_stream_protected_history))
+            self.assertTrue(validate_attachment_request(cordelia, attachment.path_id)[0])
+            self.assertTrue(validate_attachment_request(owner, attachment.path_id)[0])
+            attachment.refresh_from_db()
+            self.assertFalse(attachment.is_realm_public)
+            nobody_group = NamedUserGroup.objects.get(
+                name="role:nobody", is_system_group=True, realm=realm
+            )
+            do_change_stream_group_based_setting(
+                private_stream_protected_history, setting_name, nobody_group, acting_user=cordelia
+            )
+
     def test_try_make_stream_public_with_private_history(self) -> None:
         # We only support public streams with private history if
         # is_zephyr_mirror_realm, and don't allow changing stream

zerver/tests/test_upload.py

@@ -890,7 +890,7 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase):
         def assert_cannot_access_file(user: UserProfile) -> None:
             self.login_user(user)
             # It takes a few extra queries to verify lack of access with shared history.
-            with self.assert_database_query_count(8):
+            with self.assert_database_query_count(10):
                 response = self.client_get(url)
             self.assertEqual(response.status_code, 403)
             self.assert_in_response("You are not authorized to view this file.", response)
@@ -931,7 +931,7 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase):
 
         user = self.example_user("aaron")
         self.login_user(user)
-        with self.assert_database_query_count(8):
+        with self.assert_database_query_count(10):
             response = self.client_get(url)
             self.assertEqual(response.status_code, 403)
             self.assert_in_response("You are not authorized to view this file.", response)