realm_emoji: Stop swallowing all exceptions from upload_emoji_image.

Putting all of the logic in a `finally` block is equivalent to a bare
`except` block, which silently consumes all exceptions.

Move only the most-necessary parts into the except; this lets
`BadImageError` exceptions from `zerver/lib/upload.py` to escape,
allowing better the generic "Image file upload failed" to be replaced
with a more specific message.

It also allows unexpected exceptions, as the previous commit resolved,
to escape and 500.  This lets them be detected and resolved, rather
than give users a silently bad experience.

zerver/lib/actions.py

@@ -7908,7 +7908,7 @@ def notify_realm_emoji(realm: Realm) -> None:
 
 def check_add_realm_emoji(
     realm: Realm, name: str, author: UserProfile, image_file: IO[bytes]
-) -> Optional[RealmEmoji]:
+) -> RealmEmoji:
     try:
         realm_emoji = RealmEmoji(realm=realm, name=name, author=author)
         realm_emoji.full_clean()
@@ -7931,12 +7931,10 @@ def check_add_realm_emoji(
     finally:
         if not emoji_uploaded_successfully:
             realm_emoji.delete()
-            return None
-        else:
-            realm_emoji.file_name = emoji_file_name
-            realm_emoji.is_animated = is_animated
-            realm_emoji.save(update_fields=["file_name", "is_animated"])
-            notify_realm_emoji(realm_emoji.realm)
+    realm_emoji.file_name = emoji_file_name
+    realm_emoji.is_animated = is_animated
+    realm_emoji.save(update_fields=["file_name", "is_animated"])
+    notify_realm_emoji(realm_emoji.realm)
     return realm_emoji
 
 

zerver/tests/test_realm_emoji.py

@@ -10,6 +10,7 @@ from zerver.lib.actions import (
 from zerver.lib.exceptions import JsonableError
 from zerver.lib.test_classes import ZulipTestCase
 from zerver.lib.test_helpers import get_test_image_file
+from zerver.lib.upload import BadImageError
 from zerver.models import Realm, RealmEmoji, UserProfile, get_realm
 
 
@@ -320,11 +321,13 @@ class RealmEmojiTest(ZulipTestCase):
 
     def test_failed_file_upload(self) -> None:
         self.login("iago")
-        with mock.patch("zerver.lib.upload.write_local_file", side_effect=Exception()):
+        with mock.patch(
+            "zerver.lib.upload.write_local_file", side_effect=BadImageError(msg="Broken")
+        ):
             with get_test_image_file("img.png") as fp1:
                 emoji_data = {"f1": fp1}
                 result = self.client_post("/json/realm/emoji/my_emoji", info=emoji_data)
-        self.assert_json_error(result, "Image file upload failed.")
+        self.assert_json_error(result, "Broken")
 
     def test_check_admin_realm_emoji(self) -> None:
         # Test that an user A is able to remove a realm emoji uploaded by him

zerver/tests/test_upload.py

@@ -2288,17 +2288,17 @@ class UploadSpaceTests(UploadSerializeMixin, ZulipTestCase):
 class DecompressionBombTests(ZulipTestCase):
     def setUp(self) -> None:
         super().setUp()
-        self.test_urls = {
-            "/json/users/me/avatar": "Image size exceeds limit.",
-            "/json/realm/logo": "Image size exceeds limit.",
-            "/json/realm/icon": "Image size exceeds limit.",
-            "/json/realm/emoji/bomb_emoji": "Image file upload failed.",
-        }
+        self.test_urls = [
+            "/json/users/me/avatar",
+            "/json/realm/logo",
+            "/json/realm/icon",
+            "/json/realm/emoji/bomb_emoji",
+        ]
 
     def test_decompression_bomb(self) -> None:
         self.login("iago")
         with get_test_image_file("bomb.png") as fp:
-            for url, error_string in self.test_urls.items():
+            for url in self.test_urls:
                 fp.seek(0, 0)
                 if url == "/json/realm/logo":
                     result = self.client_post(
@@ -2306,4 +2306,4 @@ class DecompressionBombTests(ZulipTestCase):
                     )
                 else:
                     result = self.client_post(url, {"f1": fp})
-                self.assert_json_error(result, error_string)
+                self.assert_json_error(result, "Image size exceeds limit.")

zerver/views/realm_emoji.py

@@ -47,9 +47,7 @@ def upload_emoji(
             )
         )
 
-    realm_emoji = check_add_realm_emoji(user_profile.realm, emoji_name, user_profile, emoji_file)
-    if realm_emoji is None:
-        raise JsonableError(_("Image file upload failed."))
+    check_add_realm_emoji(user_profile.realm, emoji_name, user_profile, emoji_file)
     return json_success(request)