From 6d105b529b63ca0e19dbd4c879b2281d6a8b5992 Mon Sep 17 00:00:00 2001
From: Michael Z <michael@freedom.press>
Date: Thu, 30 Jun 2022 16:35:22 -0400
Subject: [PATCH] Always communicate error messages to clients

Previously, exit_gracefully did not share the type of error message with
the client if the subprocess exception that called the method captured
output. This is counter to what we would expect.

Co-authored-by: Allie Crevier <allie@freedom.press>
---
 securedrop_export/export.py | 21 +++++++++++----------
 tests/test_export.py        | 10 +++++++---
 2 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/securedrop_export/export.py b/securedrop_export/export.py
index fee1537..4ce3485 100755
--- a/securedrop_export/export.py
+++ b/securedrop_export/export.py
@@ -102,18 +102,19 @@ def exit_gracefully(self, msg, e=False):
         solutions for mimetype handling, which we want to avoid.
         """
         logger.info("Exiting with message: {}".format(msg))
-        if not e:
+        if e:
+            logger.error("Captured exception output: {}".format(e.output))
+        try:
+            # If the file archive was extracted, delete before returning
+            if os.path.isdir(self.tmpdir):
+                shutil.rmtree(self.tmpdir)
+            # Do this after deletion to avoid giving the client two error messages in case of the
+            # block above failing
             sys.stderr.write(msg)
             sys.stderr.write("\n")
-        else:
-            try:
-                # If the file archive was extracted, delete before returning
-                if os.path.isdir(self.tmpdir):
-                    shutil.rmtree(self.tmpdir)
-                logger.error("{}:{}".format(msg, e.output))
-            except Exception as ex:
-                logger.error("Unhandled exception: {}".format(ex))
-                sys.stderr.write(ExportStatus.ERROR_GENERIC.value)
+        except Exception as ex:
+            logger.error("Unhandled exception: {}".format(ex))
+            sys.stderr.write(ExportStatus.ERROR_GENERIC.value)
         # exit with 0 return code otherwise the os will attempt to open
         # the file with another application
         sys.exit(0)
diff --git a/tests/test_export.py b/tests/test_export.py
index dfaab86..4c0b81b 100644
--- a/tests/test_export.py
+++ b/tests/test_export.py
@@ -2,6 +2,8 @@
 import subprocess  # noqa: F401
 import tempfile
 
+from unittest import mock
+
 import json
 import pytest
 import tarfile
@@ -426,16 +428,18 @@ def test_exit_gracefully_no_exception(capsys):
 
 def test_exit_gracefully_exception(capsys):
     submission = export.SDExport("testfile", TEST_CONFIG)
-    test_msg = "test"
+    test_msg = "ERROR_GENERIC"
 
     with pytest.raises(SystemExit) as sysexit:
-        submission.exit_gracefully(test_msg, e=Exception("BANG!"))
+        exception = mock.MagicMock()
+        exception.output = "BANG!"
+        submission.exit_gracefully(test_msg, e=exception)
 
     # A graceful exit means a return code of 0
     assert sysexit.value.code == 0
 
     captured = capsys.readouterr()
-    assert captured.err == export.ExportStatus.ERROR_GENERIC.value
+    assert captured.err.rstrip() == export.ExportStatus.ERROR_GENERIC.value
     assert captured.out == ""