From 4cca5652e3d85767d944ee4a99793a9d5e8088f6 Mon Sep 17 00:00:00 2001 From: Aman Agrawal Date: Thu, 12 Jun 2025 11:01:19 +0530 Subject: [PATCH] slack_import: Pipe file processing error message to the user. When the slack import fails due to invalid zip file being uploaded, we take user back to the file upload page with an appropriate error message. --- templates/zerver/slack_import.html | 2 +- zerver/actions/data_import.py | 4 ++++ zerver/data_import/slack.py | 7 +++++-- zerver/lib/exceptions.py | 11 +++++++++++ zerver/tests/test_slack_importer.py | 8 ++++++-- zerver/views/registration.py | 20 +++++++++++++++++++- 6 files changed, 46 insertions(+), 6 deletions(-) diff --git a/templates/zerver/slack_import.html b/templates/zerver/slack_import.html index e3bf5b57e9..f9e7dd2e0e 100644 --- a/templates/zerver/slack_import.html +++ b/templates/zerver/slack_import.html @@ -70,7 +70,7 @@ {% endtrans %}
-

+

{{ invalid_file_error_message }}

{{ csrf_input }} diff --git a/zerver/actions/data_import.py b/zerver/actions/data_import.py index e25577046f..61518db802 100644 --- a/zerver/actions/data_import.py +++ b/zerver/actions/data_import.py @@ -10,6 +10,7 @@ from zerver.actions.realm_settings import do_delete_all_realm_attachments from zerver.actions.users import do_change_user_role from zerver.context_processors import is_realm_import_enabled from zerver.data_import.slack import do_convert_zipfile +from zerver.lib.exceptions import SlackImportInvalidFileError from zerver.lib.import_realm import do_import_realm from zerver.lib.upload import save_attachment_contents from zerver.models.prereg_users import PreregistrationRealm @@ -78,6 +79,9 @@ def import_slack_data(event: dict[str, Any]) -> None: # Clean up the realm if the import failed preregistration_realm.created_realm = None preregistration_realm.data_import_metadata["is_import_work_queued"] = False + if type(e) is SlackImportInvalidFileError: + # Store the error to be displayed to the user. + preregistration_realm.data_import_metadata["invalid_file_error_message"] = str(e) preregistration_realm.save() realm = Realm.objects.get(string_id=string_id) diff --git a/zerver/data_import/slack.py b/zerver/data_import/slack.py index 6f187bb8c5..b4774e0329 100644 --- a/zerver/data_import/slack.py +++ b/zerver/data_import/slack.py @@ -49,6 +49,7 @@ from zerver.data_import.slack_message_conversion import ( process_slack_block_and_attachment, ) from zerver.lib.emoji import codepoint_to_name, get_emoji_file_name +from zerver.lib.exceptions import SlackImportInvalidFileError from zerver.lib.export import MESSAGE_BATCH_CHUNK_SIZE, do_common_export_processes from zerver.lib.mime_types import guess_type from zerver.lib.storage import static_path @@ -1523,7 +1524,9 @@ def do_convert_zipfile( # top-level directories, or as `canvas_in_the_conversation.json` # files in channel directories. We do not parse these currently. if not re.match(r"[^/]+(\.json|/([^/]+\.json)?)$", fileinfo.filename): - raise Exception("This zip file does not look like a Slack archive") + raise SlackImportInvalidFileError( + "Uploaded zip file is not a valid Slack export." + ) # file_size is the uncompressed size of the file total_size += fileinfo.file_size @@ -1532,7 +1535,7 @@ def do_convert_zipfile( # than a 10x size magnification is suspect, particularly # if it results in over 1GB. if total_size > 1024 * 1024 * 1024 and total_size > 10 * os.path.getsize(original_path): - raise Exception("This zip file is possibly malicious") + raise SlackImportInvalidFileError("Uploaded zip file is not a valid Slack export.") zipObj.extractall(slack_data_dir) diff --git a/zerver/lib/exceptions.py b/zerver/lib/exceptions.py index a0a0b654f6..ff019bc3d4 100644 --- a/zerver/lib/exceptions.py +++ b/zerver/lib/exceptions.py @@ -776,3 +776,14 @@ class DeliveryTimeNotInFutureError(JsonableError): @override def msg_format() -> str: return _("Scheduled delivery time must be in the future.") + + +class SlackImportInvalidFileError(Exception): + """ + An error that is raised during the Slack import process + and is intended to be shown to the user. + """ + + def __init__(self, message: str) -> None: + super().__init__(message) + self.message = message diff --git a/zerver/tests/test_slack_importer.py b/zerver/tests/test_slack_importer.py index 35e7e3444b..834387887a 100644 --- a/zerver/tests/test_slack_importer.py +++ b/zerver/tests/test_slack_importer.py @@ -52,6 +52,7 @@ from zerver.data_import.slack import ( slack_workspace_to_realm, users_to_zerver_userprofile, ) +from zerver.lib.exceptions import SlackImportInvalidFileError from zerver.lib.import_realm import do_import_realm from zerver.lib.test_classes import ZulipTestCase from zerver.lib.test_helpers import find_key_by_email, read_test_image_file @@ -2255,7 +2256,7 @@ by Pieter email="test@example.com", data_import_metadata={"import_from": "slack"}, ) - mock_do_import_realm.side_effect = AssertionError("Import failed") + mock_do_import_realm.side_effect = SlackImportInvalidFileError("Invalid file") event = { "preregistration_realm_id": prereg_realm.id, @@ -2264,7 +2265,7 @@ by Pieter } with ( - self.assertRaises(AssertionError), + self.assertRaises(SlackImportInvalidFileError), self.assertLogs("zerver.actions.data_import", "ERROR"), ): import_slack_data(event) @@ -2272,6 +2273,9 @@ by Pieter prereg_realm.refresh_from_db() self.assertIsNone(prereg_realm.created_realm) self.assertFalse(prereg_realm.data_import_metadata["is_import_work_queued"]) + self.assertEqual( + prereg_realm.data_import_metadata["invalid_file_error_message"], "Invalid file" + ) self.assertFalse(Realm.objects.filter(string_id="test-realm").exists()) @mock.patch("zerver.actions.data_import.do_import_realm") diff --git a/zerver/views/registration.py b/zerver/views/registration.py index 66e71027e7..01f64ec292 100644 --- a/zerver/views/registration.py +++ b/zerver/views/registration.py @@ -390,6 +390,9 @@ def registration_helper( context["uploaded_import_file_name"] = prereg_realm.data_import_metadata.get( "uploaded_import_file_name" ) + context["invalid_file_error_message"] = prereg_realm.data_import_metadata.get( + "invalid_file_error_message", "" + ) return TemplateResponse( request, @@ -1055,7 +1058,22 @@ def realm_import_status( # TODO: Either store the path to the temporary conversion directory on # preregistration_realm.data_import_metadata, or have the conversion # process support writing updates to this for a better progress indicator. - return json_success(request, {"status": _("Converting Slack data… This may take a while.")}) + if preregistration_realm.data_import_metadata.get("is_import_work_queued"): + return json_success( + request, {"status": _("Converting Slack data… This may take a while.")} + ) + elif preregistration_realm.data_import_metadata.get("invalid_file_error_message"): + # Redirect user the file upload page if we have an error message to display. + result = { + "status": preregistration_realm.data_import_metadata.get( + "invalid_file_error_message" + ), + "redirect": reverse( + "get_prereg_key_and_redirect", kwargs={"confirmation_key": confirmation_key} + ), + } + return json_success(request, result) + # TODO: If there is something we need to fix for the import, we should notify the user. if realm.deactivated: # These "if" cases are in the inverse order than they're done