models: Rework Attachment.is_*_public to be a cache.

Previously, Attachment.is_realm_public and its cousin,
Attachment.is_web_public, were properties that began as False and
transitioned to True only when a message containing a link to the
attachment was sent to the appropriate class of stream, or such a link
was added as part of editing a message.

This pattern meant that neither field was updated in situations where
the access permissions for a message changed:

* Moving the message to a different stream.
* Changing the permissions for a stream containing links to the message.

This correctness issue has limited security impact, because uploaded
files are secured both by a random URL and by these access checks.

To fix this, we reformulate these fields as a cache, with code paths
that change the permissions affecting an attachment responsible for
setting these values to the `None` (uncached) state. We prefer setting
this `None` state over computing the correct permissions, because the
correct post-edit permissions are a function of all messages
containing the attachment, and we don't want to be responsible for
fetching all of those messages in the edit code paths.

zerver/lib/actions.py

@@ -221,6 +221,7 @@ from zerver.lib.utils import generate_api_key, log_statsd_event
 from zerver.lib.validator import check_widget_content
 from zerver.lib.widget import do_widget_post_save_actions, is_widget_message
 from zerver.models import (
+    ArchivedAttachment,
     Attachment,
     Client,
     CustomProfileField,
@@ -5195,6 +5196,17 @@ def do_change_stream_permission(
 
         event_time = timezone_now()
         if old_invite_only_value != stream.invite_only:
+            # Reset the Attachment.is_realm_public cache for all
+            # messages in the stream whose permissions were changed.
+            Attachment.objects.filter(messages__recipient_id=stream.recipient_id).update(
+                is_realm_public=None
+            )
+            # We need to do the same for ArchivedAttachment to avoid
+            # bugs if deleted attachments are later restored.
+            ArchivedAttachment.objects.filter(messages__recipient_id=stream.recipient_id).update(
+                is_realm_public=None
+            )
+
             RealmAuditLog.objects.create(
                 realm=stream.realm,
                 acting_user=acting_user,
@@ -5227,6 +5239,17 @@ def do_change_stream_permission(
             )
 
         if old_is_web_public_value != stream.is_web_public:
+            # Reset the Attachment.is_realm_public cache for all
+            # messages in the stream whose permissions were changed.
+            Attachment.objects.filter(messages__recipient_id=stream.recipient_id).update(
+                is_web_public=None
+            )
+            # We need to do the same for ArchivedAttachment to avoid
+            # bugs if deleted attachments are later restored.
+            ArchivedAttachment.objects.filter(messages__recipient_id=stream.recipient_id).update(
+                is_web_public=None
+            )
+
             RealmAuditLog.objects.create(
                 realm=stream.realm,
                 acting_user=acting_user,
@@ -7046,6 +7069,24 @@ def do_update_message(
             delete_event_notify_user_ids = [sub.user_profile_id for sub in subs_losing_access]
             send_event(user_profile.realm, delete_event, delete_event_notify_user_ids)
 
+            # Reset the Attachment.is_*_public caches for all messages
+            # moved to another stream with different access permissions.
+            if new_stream.invite_only != stream_being_edited.invite_only:
+                Attachment.objects.filter(messages__in=changed_message_ids).update(
+                    is_realm_public=None,
+                )
+                ArchivedAttachment.objects.filter(messages__in=changed_message_ids).update(
+                    is_realm_public=None,
+                )
+
+            if new_stream.is_web_public != stream_being_edited.is_web_public:
+                Attachment.objects.filter(messages__in=changed_message_ids).update(
+                    is_web_public=None,
+                )
+                ArchivedAttachment.objects.filter(messages__in=changed_message_ids).update(
+                    is_web_public=None,
+                )
+
     # This does message.save(update_fields=[...])
     save_message_for_edit_use_case(message=target_message)
 

zerver/migrations/0385_attachment_flags_cache.py

@@ -0,0 +1,33 @@
+# Generated by Django 3.2.12 on 2022-03-23 03:49
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("zerver", "0384_alter_realm_not_null"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="archivedattachment",
+            name="is_realm_public",
+            field=models.BooleanField(default=False, null=True),
+        ),
+        migrations.AlterField(
+            model_name="archivedattachment",
+            name="is_web_public",
+            field=models.BooleanField(default=False, null=True),
+        ),
+        migrations.AlterField(
+            model_name="attachment",
+            name="is_realm_public",
+            field=models.BooleanField(default=False, null=True),
+        ),
+        migrations.AlterField(
+            model_name="attachment",
+            name="is_web_public",
+            field=models.BooleanField(default=False, null=True),
+        ),
+    ]

zerver/models.py

@@ -33,7 +33,7 @@ from django.core.exceptions import ValidationError
 from django.core.validators import MinLengthValidator, RegexValidator, URLValidator, validate_email
 from django.db import models, transaction
 from django.db.backends.base.base import BaseDatabaseWrapper
-from django.db.models import CASCADE, F, Manager, Q, Sum
+from django.db.models import CASCADE, Exists, F, Manager, OuterRef, Q, Sum
 from django.db.models.functions import Upper
 from django.db.models.query import QuerySet
 from django.db.models.signals import post_delete, post_save, pre_delete
@@ -3291,18 +3291,23 @@ class AbstractAttachment(models.Model):
     # Size of the uploaded file, in bytes
     size: int = models.IntegerField()
 
-    # The two fields below lets us avoid looking up the corresponding
-    # messages/streams to check permissions before serving these files.
+    # The two fields below serve as caches to let us avoid looking up
+    # the corresponding messages/streams to check permissions before
+    # serving these files.
+    #
+    # For both fields, the `null` state is used when a change in
+    # message permissions mean that we need to determine their proper
+    # value.
 
     # Whether this attachment has been posted to a public stream, and
     # thus should be available to all non-guest users in the
     # organization (even if they weren't a recipient of a message
     # linking to it).
-    is_realm_public: bool = models.BooleanField(default=False)
+    is_realm_public: Optional[bool] = models.BooleanField(default=False, null=True)
     # Whether this attachment has been posted to a web-public stream,
     # and thus should be available to everyone on the internet, even
     # if the person isn't logged in.
-    is_web_public: bool = models.BooleanField(default=False)
+    is_web_public: Optional[bool] = models.BooleanField(default=False, null=True)
 
     class Meta:
         abstract = True
@@ -3351,12 +3356,52 @@ post_save.connect(flush_used_upload_space_cache, sender=Attachment)
 post_delete.connect(flush_used_upload_space_cache, sender=Attachment)
 
 
+def validate_attachment_request_for_spectator_access(
+    realm: Realm, attachment: Attachment
+) -> Optional[bool]:
+    if attachment.realm != realm:
+        return False
+
+    # Update cached is_web_public property, if necessary.
+    if attachment.is_web_public is None:
+        # Fill the cache in a single query. This is important to avoid
+        # a potential race condition between checking and setting,
+        # where the attachment could have been moved again.
+        Attachment.objects.filter(id=attachment.id, is_web_public__isnull=True).update(
+            is_web_public=Exists(
+                Message.objects.filter(
+                    attachment=OuterRef("id"),
+                    recipient__stream__invite_only=False,
+                    recipient__stream__is_web_public=True,
+                ),
+            ),
+        )
+        attachment.refresh_from_db()
+
+    return attachment.is_web_public
+
+
 def validate_attachment_request(user_profile: UserProfile, path_id: str) -> Optional[bool]:
     try:
         attachment = Attachment.objects.get(path_id=path_id)
     except Attachment.DoesNotExist:
         return None
 
+    # Update cached is_realm_public property, if necessary.
+    if attachment.is_realm_public is None:
+        # Fill the cache in a single query. This is important to avoid
+        # a potential race condition between checking and setting,
+        # where the attachment could have been moved again.
+        Attachment.objects.filter(id=attachment.id, is_realm_public__isnull=True).update(
+            is_realm_public=Exists(
+                Message.objects.filter(
+                    attachment=OuterRef("id"),
+                    recipient__stream__invite_only=False,
+                ),
+            ),
+        )
+        attachment.refresh_from_db()
+
     if user_profile == attachment.owner:
         # If you own the file, you can access it.
         return True

zerver/tests/test_message_edit.py

@@ -1401,7 +1401,7 @@ class EditMessageTest(EditMessageTestCase):
                 send_notification_to_new_thread=False,
                 content=None,
             )
-            self.assert_length(queries, 31)
+            self.assert_length(queries, 33)
 
         # Cordelia is not subscribed to the private stream, so
         # Cordelia should have had the topic unmuted, while Desdemona

zerver/tests/test_subs.py

@@ -1,6 +1,7 @@
 import hashlib
 import random
 from datetime import timedelta
+from io import StringIO
 from typing import Any, Dict, List, Mapping, Optional, Sequence, Set, Union
 from unittest import mock
 
@@ -75,6 +76,7 @@ from zerver.lib.test_helpers import (
 )
 from zerver.lib.types import NeverSubscribedStreamDict, SubscriptionInfo
 from zerver.models import (
+    Attachment,
     DefaultStream,
     DefaultStreamGroup,
     Message,
@@ -92,6 +94,8 @@ from zerver.models import (
     get_stream,
     get_user,
     get_user_profile_by_id_in_realm,
+    validate_attachment_request,
+    validate_attachment_request_for_spectator_access,
 )
 from zerver.views.streams import compose_views
 
@@ -951,6 +955,148 @@ class StreamAdminTest(ZulipTestCase):
         ).decode()
         self.assertEqual(realm_audit_log.extra_data, expected_extra_data)
 
+    def test_stream_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})
+        uri = result.json()["uri"]
+        self.assert_json_success(result)
+
+        owner = self.example_user("desdemona")
+        realm = owner.realm
+        stream = self.make_stream("test_stream", realm=realm)
+        self.subscribe(owner, "test_stream")
+        body = f"First message ...[zulip.txt](http://{realm.host}" + uri + ")"
+        msg_id = self.send_stream_message(owner, "test_stream", body, "test")
+        attachment = Attachment.objects.get(messages__id=msg_id)
+
+        self.assertFalse(stream.is_web_public)
+        self.assertFalse(attachment.is_web_public)
+        self.assertFalse(stream.invite_only)
+        self.assertTrue(attachment.is_realm_public)
+
+        params = {
+            "stream_name": orjson.dumps("test_stream").decode(),
+            "is_private": orjson.dumps(True).decode(),
+            "history_public_to_subscribers": orjson.dumps(True).decode(),
+        }
+        result = self.client_patch(f"/json/streams/{stream.id}", params)
+        self.assert_json_success(result)
+
+        attachment.refresh_from_db()
+        stream.refresh_from_db()
+        self.assertFalse(stream.is_web_public)
+        self.assertFalse(attachment.is_web_public)
+        self.assertTrue(stream.invite_only)
+        self.assertIsNone(attachment.is_realm_public)
+
+        cordelia = self.example_user("cordelia")
+        self.assertFalse(validate_attachment_request(cordelia, attachment.path_id))
+        self.assertTrue(validate_attachment_request(owner, attachment.path_id))
+        attachment.refresh_from_db()
+        self.assertFalse(attachment.is_realm_public)
+        self.assertFalse(validate_attachment_request_for_spectator_access(realm, attachment))
+
+        params = {
+            "stream_name": orjson.dumps("test_stream").decode(),
+            "is_private": orjson.dumps(False).decode(),
+            "is_web_public": orjson.dumps(True).decode(),
+            "history_public_to_subscribers": orjson.dumps(True).decode(),
+        }
+        result = self.client_patch(f"/json/streams/{stream.id}", params)
+        self.assert_json_success(result)
+
+        attachment.refresh_from_db()
+        stream.refresh_from_db()
+        self.assertFalse(stream.invite_only)
+        self.assertTrue(stream.is_web_public)
+        self.assertIsNone(attachment.is_realm_public)
+        self.assertIsNone(attachment.is_web_public)
+
+        self.assertTrue(validate_attachment_request_for_spectator_access(realm, attachment))
+        attachment.refresh_from_db()
+        self.assertTrue(attachment.is_web_public)
+        self.assertIsNone(attachment.is_realm_public)
+
+        self.assertTrue(validate_attachment_request(cordelia, attachment.path_id))
+        attachment.refresh_from_db()
+        self.assertTrue(attachment.is_realm_public)
+
+        params = {
+            "stream_name": orjson.dumps("test_stream").decode(),
+            "is_private": orjson.dumps(False).decode(),
+            "is_web_public": orjson.dumps(False).decode(),
+            "history_public_to_subscribers": orjson.dumps(True).decode(),
+        }
+        result = self.client_patch(f"/json/streams/{stream.id}", params)
+        self.assert_json_success(result)
+
+        attachment.refresh_from_db()
+        stream.refresh_from_db()
+        self.assertIsNone(attachment.is_web_public)
+        self.assertFalse(stream.invite_only)
+        self.assertTrue(attachment.is_realm_public)
+
+        self.assertFalse(validate_attachment_request_for_spectator_access(realm, attachment))
+        attachment.refresh_from_db()
+        stream.refresh_from_db()
+        self.assertFalse(attachment.is_web_public)
+
+        # Verify moving a message to another public stream doesn't reset cache.
+        new_stream = self.make_stream("new_stream", realm=realm)
+        self.subscribe(owner, "new_stream")
+        result = self.client_patch(
+            "/json/messages/" + str(msg_id),
+            {
+                "stream_id": new_stream.id,
+                "propagate_mode": "change_all",
+            },
+        )
+        self.assert_json_success(result)
+        attachment.refresh_from_db()
+        self.assertFalse(attachment.is_web_public)
+        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)
+        self.subscribe(owner, "private_stream")
+        result = self.client_patch(
+            "/json/messages/" + str(msg_id),
+            {
+                "stream_id": private_stream.id,
+                "propagate_mode": "change_all",
+            },
+        )
+        self.assert_json_success(result)
+        attachment.refresh_from_db()
+        self.assertFalse(attachment.is_web_public)
+        self.assertIsNone(attachment.is_realm_public)
+
+        self.assertFalse(validate_attachment_request(cordelia, attachment.path_id))
+        self.assertTrue(validate_attachment_request(owner, attachment.path_id))
+        attachment.refresh_from_db()
+        self.assertFalse(attachment.is_realm_public)
+
+        # Verify moving a message to a web-public stream
+        web_public_stream = self.make_stream("web_public_stream", realm=realm, is_web_public=True)
+        result = self.client_patch(
+            "/json/messages/" + str(msg_id),
+            {
+                "stream_id": web_public_stream.id,
+                "propagate_mode": "change_all",
+            },
+        )
+        self.assert_json_success(result)
+        attachment.refresh_from_db()
+        self.assertIsNone(attachment.is_web_public)
+        self.assertIsNone(attachment.is_realm_public)
+
+        self.assertTrue(validate_attachment_request_for_spectator_access(realm, attachment))
+        attachment.refresh_from_db()
+        self.assertTrue(attachment.is_web_public)
+
     def test_try_make_stream_public_with_private_history(self) -> None:
         user_profile = self.example_user("hamlet")
         self.login_user(user_profile)