diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 4a1ed07b8c..a6866f2c22 100644 --- a/zerver/lib/actions.py +++ b/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) diff --git a/zerver/migrations/0385_attachment_flags_cache.py b/zerver/migrations/0385_attachment_flags_cache.py new file mode 100644 index 0000000000..fe35cd3633 --- /dev/null +++ b/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), + ), + ] diff --git a/zerver/models.py b/zerver/models.py index a92cabe8a1..525059f1b1 100644 --- a/zerver/models.py +++ b/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 diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index 2f2a82cee9..c1daea3874 100644 --- a/zerver/tests/test_message_edit.py +++ b/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 diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 0b8861c132..07f7ce695c 100644 --- a/zerver/tests/test_subs.py +++ b/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)