From b4ae76be65421d3305274f477aeaa1e059611bb3 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Tue, 15 Oct 2024 12:02:13 -0700 Subject: [PATCH] upload: Improve error messages when uploads limited by plan. To make the tests work, we ensure that MAX_FILE_UPLOAD_SIZE is enforced even in the plans case. --- zerver/models/realms.py | 4 ++-- zerver/tests/test_tusd.py | 35 +++++++++++++++++++++++++++++++++-- zerver/tests/test_upload.py | 23 ++++++++++++++++++++++- zerver/views/tusd.py | 27 ++++++++++++++++++++------- zerver/views/upload.py | 21 ++++++++++++++++----- 5 files changed, 93 insertions(+), 17 deletions(-) diff --git a/zerver/models/realms.py b/zerver/models/realms.py index 1ba3103291..fb37a7b4cb 100644 --- a/zerver/models/realms.py +++ b/zerver/models/realms.py @@ -1025,13 +1025,13 @@ class Realm(models.Model): # type: ignore[django-manager-missing] # django-stub if plan_type == Realm.PLAN_TYPE_SELF_HOSTED: return settings.MAX_FILE_UPLOAD_SIZE elif plan_type == Realm.PLAN_TYPE_LIMITED: - return 10 + return min(10, settings.MAX_FILE_UPLOAD_SIZE) elif plan_type in [ Realm.PLAN_TYPE_STANDARD, Realm.PLAN_TYPE_STANDARD_FREE, Realm.PLAN_TYPE_PLUS, ]: - return 1024 + return min(1024, settings.MAX_FILE_UPLOAD_SIZE) else: raise AssertionError("Invalid plan type") diff --git a/zerver/tests/test_tusd.py b/zerver/tests/test_tusd.py index b11d8d494a..5dc86e27ca 100644 --- a/zerver/tests/test_tusd.py +++ b/zerver/tests/test_tusd.py @@ -10,7 +10,8 @@ from zerver.lib.test_helpers import create_s3_buckets, use_s3_backend from zerver.lib.upload import sanitize_name, upload_backend, upload_message_attachment from zerver.lib.upload.s3 import S3UploadBackend from zerver.lib.utils import assert_is_not_none -from zerver.models import Attachment +from zerver.models import Attachment, Realm +from zerver.models.realms import get_realm from zerver.views.tusd import TusEvent, TusHook, TusHTTPRequest, TusUpload @@ -210,7 +211,37 @@ class TusdPreCreateTest(ZulipTestCase): self.assertEqual(result_json["HttpResponse"]["StatusCode"], 413) self.assertEqual( orjson.loads(result_json["HttpResponse"]["Body"]), - {"message": "Uploaded file is larger than the allowed limit of 1 MiB"}, + { + "message": "File is larger than this server's configured maximum upload size (1 MiB)." + }, + ) + self.assertEqual(result_json["RejectUpload"], True) + + @override_settings(MAX_FILE_UPLOAD_SIZE=1) # In MB + def test_file_too_big_failure_limited(self) -> None: + self.login("hamlet") + request = self.request() + request.event.upload.size = 1024 * 1024 * 5 # 5MB + + realm = get_realm("zulip") + realm.plan_type = Realm.PLAN_TYPE_LIMITED + realm.save() + + result = self.client_post( + "/api/internal/tusd", + request.model_dump(), + content_type="application/json", + ) + + self.assertEqual(result.status_code, 200) + result_json = result.json() + self.assertEqual(result_json["HttpResponse"]["StatusCode"], 413) + self.assertEqual( + orjson.loads(result_json["HttpResponse"]["Body"]), + { + "message": "File is larger than the maximum upload size (1 MiB) allowed by " + "your organization's plan." + }, ) self.assertEqual(result_json["RejectUpload"], True) diff --git a/zerver/tests/test_upload.py b/zerver/tests/test_upload.py index 9f8231ad81..264ee46174 100644 --- a/zerver/tests/test_upload.py +++ b/zerver/tests/test_upload.py @@ -121,7 +121,28 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): # would be 1MB. with self.settings(MAX_FILE_UPLOAD_SIZE=0): result = self.client_post("/json/user_uploads", {"f1": fp}) - self.assert_json_error(result, "Uploaded file is larger than the allowed limit of 0 MiB") + self.assert_json_error( + result, + "File is larger than this server's configured maximum upload size (0 MiB).", + ) + + def test_file_too_big_failure_standard_plan(self) -> None: + """ + Verify error message where a plan is involved. + """ + self.login("hamlet") + fp = StringIO("bah!") + fp.name = "a.txt" + + realm = get_realm("zulip") + realm.plan_type = Realm.PLAN_TYPE_LIMITED + realm.save() + with self.settings(MAX_FILE_UPLOAD_SIZE=0): + result = self.client_post("/json/user_uploads", {"f1": fp}) + self.assert_json_error( + result, + "File is larger than the maximum upload size (0 MiB) allowed by your organization's plan.", + ) def test_multiple_upload_failure(self) -> None: """ diff --git a/zerver/views/tusd.py b/zerver/views/tusd.py index cef36a85e0..cb13416a21 100644 --- a/zerver/views/tusd.py +++ b/zerver/views/tusd.py @@ -25,7 +25,7 @@ from zerver.lib.upload import ( upload_backend, ) from zerver.lib.upload.base import INLINE_MIME_TYPES -from zerver.models import UserProfile +from zerver.models import Realm, UserProfile # See https://tus.github.io/tusd/advanced-topics/hooks/ for the spec @@ -101,12 +101,25 @@ def handle_upload_pre_create_hook( max_file_upload_size_mebibytes = user_profile.realm.get_max_file_upload_size_mebibytes() if data.size > max_file_upload_size_mebibytes * 1024 * 1024: - return reject_upload( - _("Uploaded file is larger than the allowed limit of {max_file_size} MiB").format( - max_file_size=max_file_upload_size_mebibytes - ), - 413, - ) + if user_profile.realm.plan_type != Realm.PLAN_TYPE_SELF_HOSTED: + return reject_upload( + _( + "File is larger than the maximum upload size ({max_size} MiB) allowed by your organization's plan." + ).format( + max_size=max_file_upload_size_mebibytes, + ), + 413, + ) + else: + return reject_upload( + _( + "File is larger than this server's configured maximum upload size ({max_size} MiB)." + ).format( + max_size=max_file_upload_size_mebibytes, + ), + 413, + ) + try: check_upload_within_quota(user_profile.realm, data.size) except RealmUploadQuotaError as e: diff --git a/zerver/views/upload.py b/zerver/views/upload.py index 3675c7d966..ec3fb1caad 100644 --- a/zerver/views/upload.py +++ b/zerver/views/upload.py @@ -45,7 +45,7 @@ from zerver.lib.upload import ( from zerver.lib.upload.base import INLINE_MIME_TYPES from zerver.lib.upload.local import assert_is_local_storage_path from zerver.lib.upload.s3 import get_signed_upload_url -from zerver.models import Attachment, ImageAttachment, UserProfile +from zerver.models import Attachment, ImageAttachment, Realm, UserProfile from zerver.worker.thumbnail import ensure_thumbnails @@ -450,11 +450,22 @@ def upload_file_backend(request: HttpRequest, user_profile: UserProfile) -> Http assert file_size is not None max_file_upload_size_mebibytes = user_profile.realm.get_max_file_upload_size_mebibytes() if file_size > max_file_upload_size_mebibytes * 1024 * 1024: - raise JsonableError( - _("Uploaded file is larger than the allowed limit of {max_size} MiB").format( - max_size=max_file_upload_size_mebibytes, + if user_profile.realm.plan_type != Realm.PLAN_TYPE_SELF_HOSTED: + raise JsonableError( + _( + "File is larger than the maximum upload size ({max_size} MiB) allowed by your organization's plan." + ).format( + max_size=max_file_upload_size_mebibytes, + ) + ) + else: + raise JsonableError( + _( + "File is larger than this server's configured maximum upload size ({max_size} MiB)." + ).format( + max_size=max_file_upload_size_mebibytes, + ) ) - ) check_upload_within_quota(user_profile.realm, file_size) url, filename = upload_message_attachment_from_request(user_file, user_profile)