attachment: Fix handling of stream history.

This fixes two issues:

* Our guest users feature gave guest users access to public stream
  attachments even if they couldn't access the public stream.

* After a user joins a private stream with our new shared history
  feature, they couldn't see images uploaded before they joined.

The tests need to check for a few types of issues:
* The actual access control permissions.
* How many database queries are used in the various
  cases for that second model, especially with multiple messages
  referencing an attachment.  This function gets called a lot, and we
  want to keep it fast.

Fixes #9372.

zerver/models.py

@@ -1470,7 +1470,8 @@ def validate_attachment_request(user_profile: UserProfile, path_id: str) -> Opti
     if user_profile == attachment.owner:
         # If you own the file, you can access it.
         return True
-    if attachment.is_realm_public and attachment.realm == user_profile.realm:
+    if (attachment.is_realm_public and attachment.realm == user_profile.realm and
+            user_profile.can_access_public_streams()):
         # Any user in the realm can access realm-public files
         return True
 
@@ -1480,7 +1481,22 @@ def validate_attachment_request(user_profile: UserProfile, path_id: str) -> Opti
         # message, then anyone who received that message can access it.
         return True
 
-    return False
+    # 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 on where history is public to
+    # subscribers.
+
+    # These are subscriptions to a stream one of the messages was sent to
+    relevant_stream_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 len(relevant_stream_ids) == 0:
+        return False
+
+    return Stream.objects.filter(id__in=relevant_stream_ids,
+                                 history_public_to_subscribers=True).exists()
 
 def get_old_unclaimed_attachments(weeks_ago: int) -> Sequence[Attachment]:
     # TODO: Change return type to QuerySet[Attachment]

zerver/tests/test_upload.py

@@ -16,6 +16,7 @@ from zerver.lib.test_helpers import (
     get_test_image_file,
     POSTRequestMock,
     use_s3_backend,
+    queries_captured,
 )
 from zerver.lib.test_runner import slow
 from zerver.lib.upload import sanitize_name, S3UploadBackend, \
@@ -27,7 +28,8 @@ from zerver.lib.upload import sanitize_name, S3UploadBackend, \
 import zerver.lib.upload
 from zerver.models import Attachment, get_user, \
     get_old_unclaimed_attachments, Message, UserProfile, Stream, Realm, \
-    RealmDomain, RealmEmoji, get_realm, get_system_bot
+    RealmDomain, RealmEmoji, get_realm, get_system_bot, \
+    validate_attachment_request
 from zerver.lib.actions import (
     do_delete_old_unclaimed_attachments,
     internal_send_private_message,
@@ -205,12 +207,6 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase):
         result = self.client_post("/json/user_uploads")
         self.assert_json_error(result, "You must specify a file to upload")
 
-    def test_download_non_existent_file(self) -> None:
-        self.login(self.example_email("hamlet"))
-        response = self.client_get('/user_uploads/unk/nonexistent_file')
-        self.assertEqual(response.status_code, 404)
-        self.assertIn('File not found', str(response.content))
-
     # This test will go through the code path for uploading files onto LOCAL storage
     # when zulip is in DEVELOPMENT mode.
     def test_file_upload_authed(self) -> None:
@@ -527,43 +523,170 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase):
         self.assert_in_response("You are not authorized to view this file.", response)
 
     def test_file_download_authorization_invite_only(self) -> None:
-        subscribed_users = [self.example_email("hamlet"), self.example_email("iago")]
+        user = self.example_user("hamlet")
-        unsubscribed_users = [self.example_email("othello"), self.example_email("prospero")]
+        subscribed_emails = [user.email, self.example_email("cordelia")]
-        realm = get_realm("zulip")
+        unsubscribed_emails = [self.example_email("othello"), self.example_email("prospero")]
-        for email in subscribed_users:
+        stream_name = "test-subscribe"
-            self.subscribe(get_user(email, realm), "test-subscribe")
+        self.make_stream(stream_name, realm=user.realm, invite_only=True, history_public_to_subscribers=False)
 
-        # Make the stream private
+        for email in subscribed_emails:
-        stream = Stream.objects.get(name='test-subscribe')
+            self.subscribe(get_user(email, user.realm), stream_name)
-        stream.invite_only = True
-        stream.save()
 
-        self.login(self.example_email("hamlet"))
+        self.login(user.email)
         fp = StringIO("zulip!")
         fp.name = "zulip.txt"
         result = self.client_post("/json/user_uploads", {'file': fp})
         uri = result.json()['uri']
         fp_path_id = re.sub('/user_uploads/', '', uri)
         body = "First message ...[zulip.txt](http://localhost:9991/user_uploads/" + fp_path_id + ")"
-        self.send_stream_message(self.example_email("hamlet"), "test-subscribe", body, "test")
+        self.send_stream_message(user.email, stream_name, body, "test")
         self.logout()
 
-        # Subscribed user should be able to view file
+        # Owner user should be able to view file
-        for user in subscribed_users:
+        self.login(user.email)
-            self.login(user)
+        with queries_captured() as queries:
             response = self.client_get(uri)
             self.assertEqual(response.status_code, 200)
             data = b"".join(response.streaming_content)
             self.assertEqual(b"zulip!", data)
+        self.logout()
+        self.assertEqual(len(queries), 5)
+
+        # Subscribed user who recieved the message should be able to view file
+        self.login(subscribed_emails[1])
+        with queries_captured() as queries:
+            response = self.client_get(uri)
+            self.assertEqual(response.status_code, 200)
+            data = b"".join(response.streaming_content)
+            self.assertEqual(b"zulip!", data)
+        self.logout()
+        self.assertEqual(len(queries), 6)
+
+        def assert_cannot_access_file(user_email: str) -> None:
+            response = self.api_get(user_email, uri)
+            self.assertEqual(response.status_code, 403)
+            self.assert_in_response("You are not authorized to view this file.", response)
+
+        late_subscribed_user = self.example_user("aaron")
+        self.subscribe(late_subscribed_user, stream_name)
+        assert_cannot_access_file(late_subscribed_user.email)
+
+        # Unsubscribed user should not be able to view file
+        for unsubscribed_user in unsubscribed_emails:
+            assert_cannot_access_file(unsubscribed_user)
+
+    def test_file_download_authorization_invite_only_with_shared_history(self) -> None:
+        user = self.example_user("hamlet")
+        subscribed_emails = [user.email, self.example_email("polonius")]
+        unsubscribed_emails = [self.example_email("othello"), self.example_email("prospero")]
+        stream_name = "test-subscribe"
+        self.make_stream(stream_name, realm=user.realm, invite_only=True, history_public_to_subscribers=True)
+
+        for email in subscribed_emails:
+            self.subscribe(get_user(email, user.realm), stream_name)
+
+        self.login(user.email)
+        fp = StringIO("zulip!")
+        fp.name = "zulip.txt"
+        result = self.client_post("/json/user_uploads", {'file': fp})
+        uri = result.json()['uri']
+        fp_path_id = re.sub('/user_uploads/', '', uri)
+        body = "First message ...[zulip.txt](http://localhost:9991/user_uploads/" + fp_path_id + ")"
+        self.send_stream_message(user.email, stream_name, body, "test")
+        self.logout()
+
+        # Add aaron as a subscribed after the message was sent
+        late_subscribed_user = self.example_user("aaron")
+        self.subscribe(late_subscribed_user, stream_name)
+        subscribed_emails.append(late_subscribed_user.email)
+
+        # Owner user should be able to view file
+        self.login(user.email)
+        with queries_captured() as queries:
+            response = self.client_get(uri)
+            self.assertEqual(response.status_code, 200)
+            data = b"".join(response.streaming_content)
+            self.assertEqual(b"zulip!", data)
+        self.logout()
+        self.assertEqual(len(queries), 5)
+
+        # Originally subscribed user should be able to view file
+        self.login(subscribed_emails[1])
+        with queries_captured() as queries:
+            response = self.client_get(uri)
+            self.assertEqual(response.status_code, 200)
+            data = b"".join(response.streaming_content)
+            self.assertEqual(b"zulip!", data)
+        self.logout()
+        self.assertEqual(len(queries), 6)
+
+        # Subscribed user who did not receive the message should also be able to view file
+        self.login(late_subscribed_user.email)
+        with queries_captured() as queries:
+            response = self.client_get(uri)
+            self.assertEqual(response.status_code, 200)
+            data = b"".join(response.streaming_content)
+            self.assertEqual(b"zulip!", data)
+        self.logout()
+        # It takes a few extra queries to verify access because of shared history.
+        self.assertEqual(len(queries), 9)
+
+        def assert_cannot_access_file(user_email: str) -> None:
+            self.login(user_email)
+            with queries_captured() as queries:
+                response = self.client_get(uri)
+            self.assertEqual(response.status_code, 403)
+            # It takes a few extra queries to verify lack of access with shared history.
+            self.assertEqual(len(queries), 8)
+            self.assert_in_response("You are not authorized to view this file.", response)
             self.logout()
 
         # Unsubscribed user should not be able to view file
-        for user in unsubscribed_users:
+        for unsubscribed_user in unsubscribed_emails:
-            self.login(user)
+            assert_cannot_access_file(unsubscribed_user)
+
+    def test_multiple_message_attachment_file_download(self) -> None:
+        hamlet = self.example_user("hamlet")
+        for i in range(0, 5):
+            stream_name = "test-subscribe %s" % (i,)
+            self.make_stream(stream_name, realm=hamlet.realm, invite_only=True, history_public_to_subscribers=True)
+            self.subscribe(hamlet, stream_name)
+
+        self.login(hamlet.email)
+        fp = StringIO("zulip!")
+        fp.name = "zulip.txt"
+        result = self.client_post("/json/user_uploads", {'file': fp})
+        uri = result.json()['uri']
+        fp_path_id = re.sub('/user_uploads/', '', uri)
+        for i in range(20):
+            body = "First message ...[zulip.txt](http://localhost:9991/user_uploads/" + fp_path_id + ")"
+            self.send_stream_message(self.example_email("hamlet"), "test-subscribe %s" % (i % 5,), body, "test")
+        self.logout()
+
+        user = self.example_user("aaron")
+        self.login(user.email)
+        with queries_captured() as queries:
             response = self.client_get(uri)
             self.assertEqual(response.status_code, 403)
             self.assert_in_response("You are not authorized to view this file.", response)
-            self.logout()
+        self.assertEqual(len(queries), 8)
+
+        self.subscribe(user, "test-subscribe 1")
+        self.subscribe(user, "test-subscribe 2")
+
+        with queries_captured() as queries:
+            response = self.client_get(uri)
+            self.assertEqual(response.status_code, 200)
+            data = b"".join(response.streaming_content)
+            self.assertEqual(b"zulip!", data)
+        # If we were accidentally one query per message, this would be 20+
+        self.assertEqual(len(queries), 9)
+
+        with queries_captured() as queries:
+            self.assertTrue(validate_attachment_request(user, fp_path_id))
+        self.assertEqual(len(queries), 6)
+
+        self.logout()
 
     def test_file_download_authorization_public(self) -> None:
         subscribed_users = [self.example_email("hamlet"), self.example_email("iago")]