Added error handling for file deletion operations.

* added exception handling around the delete_file_object function, to catch
  errors caused by missing files in instance storage
* disabled shellcheck for SC1090 errors, which were being triggered by a shell
  script in the admin venv

devops/scripts/shellcheck.sh

@@ -4,7 +4,7 @@ set -e
 
 
 function run_native_or_in_docker () {
-    EXCLUDE_RULES="SC1091,SC2001,SC2064,SC2181,SC1117"
+    EXCLUDE_RULES="SC1090,SC1091,SC2001,SC2064,SC2181,SC1117"
     if [ "$(command -v shellcheck)" ]; then
         shellcheck -x --exclude="$EXCLUDE_RULES" "$1"
     else

securedrop/journalist_app/utils.py

@@ -218,22 +218,36 @@ def download(zip_basename: str, submissions: List[Union[Submission, Reply]]) ->
 
 def delete_file_object(file_object: Union[Submission, Reply]) -> None:
     path = current_app.storage.path(file_object.source.filesystem_id, file_object.filename)
-    current_app.storage.move_to_shredder(path)
-    db.session.delete(file_object)
-    db.session.commit()
+    try:
+        current_app.storage.move_to_shredder(path)
+    except ValueError as e:
+        current_app.logger.error("could not queue file for deletion: %s", e)
+        raise
+    finally:
+        db.session.delete(file_object)
+        db.session.commit()
 
 
 def bulk_delete(
     filesystem_id: str,
     items_selected: List[Union[Submission, Reply]]
 ) -> werkzeug.Response:
+    deletion_errors = 0
     for item in items_selected:
-        delete_file_object(item)
+        try:
+            delete_file_object(item)
+        except ValueError:
+            deletion_errors += 1
 
     flash(ngettext("Submission deleted.",
                    "{num} submissions deleted.".format(
                        num=len(items_selected)),
                    len(items_selected)), "notification")
+    if deletion_errors > 0:
+        flash(ngettext("An error occured during deletion - contact an administrator",
+                       "{num} errors occured during deletion - contact an administrator.".format(
+                           num=deletion_errors),
+                       deletion_errors), "error")
     return redirect(url_for('col.col', filesystem_id=filesystem_id))
 
 

securedrop/tests/test_journalist.py

@@ -1940,6 +1940,94 @@ def assertion():
         utils.asynchronous.wait_for_assertion(assertion)
 
 
+def test_bulk_delete_deletes_db_entries(journalist_app,
+                                        test_source,
+                                        test_journo,
+                                        config):
+    """
+    Verify that when files are deleted, the corresponding db entries are
+    also deleted.
+    """
+
+    with journalist_app.app_context():
+        source = Source.query.get(test_source['id'])
+        journo = Journalist.query.get(test_journo['id'])
+
+        utils.db_helper.submit(source, 2)
+        utils.db_helper.reply(journo, source, 2)
+
+        dir_source_docs = os.path.join(config.STORE_DIR, test_source['filesystem_id'])
+        assert os.path.exists(dir_source_docs)
+
+        subs = Submission.query.filter_by(source_id=source.id).all()
+        assert subs
+
+        replies = Reply.query.filter_by(source_id=source.id).all()
+        assert replies
+
+        file_list = []
+        file_list.extend(subs)
+        file_list.extend(replies)
+
+        with journalist_app.test_request_context('/'):
+            journalist_app_module.utils.bulk_delete(test_source['filesystem_id'],
+                                                    file_list)
+
+        def db_assertion():
+            subs = Submission.query.filter_by(source_id=source.id).all()
+            assert not subs
+
+            replies = Reply.query.filter_by(source_id=source.id).all()
+            assert not replies
+
+        utils.asynchronous.wait_for_assertion(db_assertion)
+
+
+def test_bulk_delete_works_when_files_absent(journalist_app,
+                                             test_source,
+                                             test_journo,
+                                             config):
+    """
+    Verify that when files are deleted but are already missing,
+    the corresponding db entries are still
+    """
+
+    with journalist_app.app_context():
+        source = Source.query.get(test_source['id'])
+        journo = Journalist.query.get(test_journo['id'])
+
+        utils.db_helper.submit(source, 2)
+        utils.db_helper.reply(journo, source, 2)
+
+        dir_source_docs = os.path.join(config.STORE_DIR, test_source['filesystem_id'])
+        assert os.path.exists(dir_source_docs)
+
+        subs = Submission.query.filter_by(source_id=source.id).all()
+        assert subs
+
+        replies = Reply.query.filter_by(source_id=source.id).all()
+        assert replies
+
+        file_list = []
+        file_list.extend(subs)
+        file_list.extend(replies)
+
+        with journalist_app.test_request_context('/'):
+            with patch("store.Storage.move_to_shredder") as delMock:
+                delMock.side_effect = ValueError
+                journalist_app_module.utils.bulk_delete(test_source['filesystem_id'],
+                                                        file_list)
+
+        def db_assertion():
+            subs = Submission.query.filter_by(source_id=source.id).all()
+            assert not subs
+
+            replies = Reply.query.filter_by(source_id=source.id).all()
+            assert not replies
+
+        utils.asynchronous.wait_for_assertion(db_assertion)
+
+
 def test_login_with_invalid_password_doesnt_call_argon2(mocker, test_journo):
     mock_argon2 = mocker.patch('models.argon2.verify')
     invalid_pw = 'a'*(Journalist.MAX_PASSWORD_LEN + 1)