Handle FileNotFoundError on download of messages #5573

DrGFreeman · 2020-10-10T15:06:53Z

Status

Ready for review.

Description of Changes

Changes proposed in this pull request:

Handle the FileNotFoundError on download of message in the JI where the corresponding file is missing on the filesystem.

Display a flash message to the journalist.
Add a log entry reporting the error.
Redirect to the page from which the download was attempted.

Also added functional test for flash message and redirect and tests for logging of the error.

Testing

How should the reviewer test this PR?
Write out any special testing steps here.

Setup

Run make dev.
Log in the Source Interface and submit two messages.
Log in the app server Docker container with docker exec -it securedrop-dev-0 bash and delete the first message (filename 1-something_something-msg.gpg) from the source's directory in /var/lib/securedrop/store.

Scenario 1 - Download via "n unread" button on /

Log in the Journalist Interface.
Click the "2 unread" button of the first source.
- Verify that a flash error message is displayed
- Verify that the page remains unchanged (journalist homepage)
- Verify that the source's number of unread message is unchanged (2)

Scenario 2 - Download via "Download Unread" button on /

Log in the Journalist Interface.
Check the first source's checkbox.
Click the "Download Unread" button.
- Verify that a flash error message is displayed
- Verify that the page remains unchanged (journalist homepage)
- Verify that the source's number of unread message is unchanged (2)

Scenario 3 - Download via "Download" button on /

Log in the Journalist Interface.
Check the first source's checkbox.
Click the "Download" button.
- Verify that a flash error message is displayed
- Verify that the page remains unchanged (journalist homepage)
- Verify that the source's number of unread message is unchanged (2)

Scenario 4 - Download via "Download Selected" button on /col/<filesystem_id>

Log in the Journalist Interface.
Click the first source's code name.
Click the "Select All" button
Click the "Download Selected" button.
- Verify that a flash error message is displayed
- Verify that the page remains unchanged (source collection page)
- Verify that the first message remains unread

Scenario 5 - Direct message download from /col/<filesystem_id>

Log in the Journalist Interface.
Click the first source's code name.
Click first message.
- Verify that a flash error message is displayed
- Verify that the page remains unchanged (source collection page)
- Verify that the first message remains unread

Deployment

Any special considerations for deployment? Consider both:

Upgrading existing production instances.
New installs.

Checklist

If you made changes to the server application code:

Linting (make lint) and tests (make test) pass in the development container

If you made changes to `securedrop-admin`:

Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

Configuration tests pass

If you made non-trivial code changes:

I have written a test plan and validated it for this PR
Test plan above + added unit/functional tests.

If you made changes to documentation:

Doc linting (make docs-lint) passed locally

If you added or updated a code dependency:

Choose one of the following:

I have performed a diff review and pasted the contents to the packaging wiki
I would like someone else to do the diff review

eloquence · 2020-10-13T19:45:00Z

Thanks for picking this up @DrGFreeman. Don't hesitate to comment if you're looking for feedback even while it's still in draft stage.

DrGFreeman · 2020-10-14T04:07:03Z

Thanks for the follow-up @eloquence. I was having difficulty setting up the missing file for the functional tests but figured out a way today (ref 995eed0).

I'm not yet done with the PR as I also want to check for proper logging of the error. UX feedback would however be welcome when you or someone has time. I added testing steps to the PR.

DrGFreeman · 2020-10-15T04:11:52Z

As discussed in #4787 (comment), I used a catch-all flash message for now. I'm open for modifications of the message as well as the error message being logged. Thx.

eloquence · 2020-10-27T23:28:52Z

Kicked the error message around a bit with @creviera and @ninavizz.

Currently:

"An unexpected error occurred! Please inform your admin."

(Also used in other contexts.)

Proposed option A replacement:

"An unexpected error has occurred. Your admin can find more information in the system logs."

Proposed option B replacement:

"An unexpected error has occurred. More information has been recorded in the system logs."

Rationale:

Exclamation mark can be anxiety-provoking
Present perfect tense connects the error more clearly with the user's action
Pointing to logs (if it is true, which judging by the code changes is the case here) gives more actionable information

Thoughts @DrGFreeman, others?

DrGFreeman · 2020-10-27T23:47:49Z

@eloquence, thanks for the feedback.

Proposed option A replacement:

"An unexpected error has occurred. Your admin can find more information in the system logs."

Proposed option B replacement:

"An unexpected error has occurred. More information has been recorded in the system logs."

I would personally go with option A as it refers the user to their admin while hinting at the additional information in the logs. With option B, as a journalist I may not know how to access the system logs and/or think about contacting the admin.

Rationale:

* Exclamation mark can be anxiety-provoking

* Present perfect tense connects the error more clearly with the user's action

I fully agree.

Side note: Instead of creating a new message, I used one that was already in the codebase:

securedrop/securedrop/journalist_app/main.py

Lines 130 to 132 in 6475ae8

    
           flash(gettext( 
        
               "An unexpected error occurred! Please " 
        
               "inform your admin."), "error")

securedrop/securedrop/journalist_app/utils.py

Lines 58 to 60 in 44a2e52

    
           flash(gettext( 
        
               "An unexpected error occurred! Please " 
        
                 "inform your admin."), "error")

securedrop/securedrop/journalist_app/utils.py

Lines 157 to 159 in 44a2e52

    
           flash(gettext( 
        
               "An unexpected error occurred! " 
        
               "Please inform your admin."), "error")

Maybe we should consider improving these messages as well? I could include this as an additional change in this PR.

eloquence · 2020-10-27T23:49:26Z

Thanks @DrGFreeman. Yes, I noticed that, and think we should standardize the message where it is used, assuming of course that errors are indeed logged in all those cases. I suggest we wait another day or so, in case other folks want to weigh in before we do that.

sssoleileraaa

lgtm: we no longer display a FileNotFoundError with traceback; instead a more user-friendly error is shown on the same page. one improvement would be to keep the checkboxes checked after an error rather than clearing the selection. this will help the user remember which files they tried to download. i think it's fine to create a follow-up issue if this takes too much time, especially since we might want to catch the missing files early on and handle it differently in the future. sounds like we're going to let this pr sit a bit longer to discuss error messaging, which gives you time to address my comment about logging and leaving the checkboxes checked (but again more than happy to open some follow-up issues for this just like i did for the bug found while testing your pr: freedomofpress/securedrop-client#1173)

securedrop/journalist_app/utils.py

securedrop/journalist_app/col.py

sssoleileraaa · 2020-10-28T00:19:41Z

Forgot to share this lovely test plan:

Scenario 1 - Download via "n unread" button on /

Log in the Journalist Interface.
Click the "2 unread" button of the first source.
✔️ Verify that a flash error message is displayed
✔️ Verify that the page remains unchanged (journalist homepage)
✔️ Verify that the source's number of unread message is unchanged (2)

Scenario 2 - Download via "Download Unread" button on /

Log in the Journalist Interface.
Check the first source's checkbox.
Click the "Download Unread" button.
✔️ Verify that a flash error message is displayed
✔️ Verify that the page remains unchanged (journalist homepage)
✔️ Verify that the source's number of unread message is unchanged (2)

Scenario 3 - Download via "Download" button on /

Log in the Journalist Interface.
Check the first source's checkbox.
Click the "Download" button.
✔️ Verify that a flash error message is displayed
✔️ Verify that the page remains unchanged (journalist homepage)
✔️ Verify that the source's number of unread message is unchanged (2)

Scenario 4 - Download via "Download Selected" button on /col/<filesystem_id>

Log in the Journalist Interface.
Click the first source's code name.
Click the "Select All" button
Click the "Download Selected" button.
✔️ Verify that a flash error message is displayed
✔️ Verify that the page remains unchanged (source collection page)
✔️ Verify that the first message remains unread

Scenario 5 - Direct message download from /col/<filesystem_id>

Log in the Journalist Interface.
Click the first source's code name.
Click first message.
✔️ Verify that a flash error message is displayed
✔️ Verify that the page remains unchanged (source collection page)
✔️ Verify that the first message remains unread

sssoleileraaa · 2020-10-28T20:09:58Z

re proposed error message replacement options - couple of suggestions:

i think "please contact an administrator" instead of saying "your administrator" sounds better - people might not think of people with securedrop admin privileges as their admins
the error is that a file is missing, so we could be better about catching that error specifically and communicating about it specifically
overall, all language suggestions seem fine to me so far, especially since a) this is such an edge case and b) it would probably be better to focus on creating clear logs of which files are missing and not show database entries without files as options for download (also might be better to alert an admin ahead of time rather than waiting until user action)

DrGFreeman · 2020-10-30T00:45:24Z

@creviera, thanks for the feedback!

I'll look into updating the error messages per your suggestions over the weekend and see how I can proceed to leave the messages selected in the case where the user used "Download Selected" (test scenario 4).

sssoleileraaa

this looks great! tests pass and there are clear logs of which files are missing. left one review comment.

securedrop/journalist_app/col.py

nabla-c0d3 · 2020-11-05T02:50:13Z

securedrop/store.py

-                        os.path.basename(filename)
-                    ))
-        return zip_file
+                        missing_files = True


I might be missing something but why not raise FileNotFoundError here directly?

To allow logging all the missing files if more than one are missing (see commit message ee18a23)

securedrop/store.py

DrGFreeman · 2020-11-05T04:15:51Z

@creviera, message updated as per your proposal in 38b0aa4.

Looks like the type linting failure in CI is related to #5616 so I suppose it will be resolved when #5616 is merged.

DrGFreeman · 2020-11-05T04:24:30Z

@creviera & @eloquence, returning on this discussion:

Thanks @DrGFreeman. Yes, I noticed that, and think we should standardize the message where it is used, assuming of course that errors are indeed logged in all those cases. I suggest we wait another day or so, in case other folks want to weigh in before we do that.

Initially we were to use a single generic message for all these errors. Now that the message is specific to missing files, I did not update the other instances. I could update these other messages in this PR but I'd have to review their context and propose appropriate messages.

Or would you prefer to raise a follow-up issue for these other messages?

Please advise.

eloquence · 2020-11-05T17:40:53Z

Now that the message is specific to missing files, I did not update the other instances.

I think that's the best course of action here, we can revisit that at a later time if warranted. You are a bit behind develop, so I think next step is to rebase and ensure we no longer see the type annotation related linting issues.

Handle the FileNotFoundError on download of message in the JI where the corresponding file is missing on the filesystem. - Display a flash message to the journalist. - Add a log entry reporting the error. - Redirect to the page from which the download was attempted.

Implement functional tests for the case where a message file is missing in the store. One test for each of the five ways to download a message from the journalist interface. Each test implements the following: -Creates a message from the source interface -Delete the message file from the store -Attempts to download the message from the journalist interface -Check that a flash message is displayed in the journalist interface -Check that the journalist remains on the page from which they attempted to download the message

Check error logging for bulk download via /bulk and single message download via /col/<filesystem_id>/<fn>.

- If more than one file is missing in bulk download, add a log entry for each missing file before returning the FileNotFound exception. - Update unit test to verify logging of multiple missing files. - Improve flashed error message. - Rename tests, fixture and journalist navigation steps to align with changes. - Rename file_ variable to file.

Change the return type to the more generic werkzeug.Response which represents both the return types of flask.redirect (werkzeug.Response) and flask.send_file (flask.Response which is derived from wekrzeuf.Response).

Improve flash message text based on feedback from PR review process.

DrGFreeman · 2020-11-06T02:33:40Z

I think next step is to rebase and ensure we no longer see the type annotation related linting issues.

@eloquence & @creviera, here you go, rebased and all tests passing!

sssoleileraaa

ran through the test plan again and lgtm

eloquence changed the title ~~Handle FileNotFoudError on download of messages~~ Handle FileNotFoundError on download of messages Oct 13, 2020

DrGFreeman marked this pull request as ready for review October 15, 2020 04:07

DrGFreeman requested review from kushaldas and redshiftzero as code owners October 15, 2020 04:07

sssoleileraaa reviewed Oct 28, 2020

View reviewed changes

securedrop/journalist_app/utils.py Show resolved Hide resolved

securedrop/journalist_app/col.py Outdated Show resolved Hide resolved

DrGFreeman force-pushed the 4787-error-on-download-deleted-files branch from 581e91f to ee18a23 Compare November 2, 2020 05:16

sssoleileraaa reviewed Nov 3, 2020

View reviewed changes

securedrop/journalist_app/col.py Outdated Show resolved Hide resolved

nabla-c0d3 reviewed Nov 5, 2020

View reviewed changes

securedrop/store.py Show resolved Hide resolved

DrGFreeman added 7 commits November 5, 2020 20:32

Cast request.referrer to str to please mypy

f975e8d

Add tests to check error logging on download of missing files

1fa8997

Check error logging for bulk download via /bulk and single message download via /col/<filesystem_id>/<fn>.

Change return type hint of download function

55f8cf2

Change the return type to the more generic werkzeug.Response which represents both the return types of flask.redirect (werkzeug.Response) and flask.send_file (flask.Response which is derived from wekrzeuf.Response).

Improve flash message text

592410f

Improve flash message text based on feedback from PR review process.

DrGFreeman force-pushed the 4787-error-on-download-deleted-files branch from 38b0aa4 to 592410f Compare November 6, 2020 01:33

sssoleileraaa approved these changes Nov 6, 2020

View reviewed changes

sssoleileraaa merged commit ce22827 into freedomofpress:develop Nov 6, 2020

eloquence added this to the 1.7.0 milestone Jan 5, 2021

kushaldas mentioned this pull request Jan 18, 2021

Release SecureDrop 1.7.0 #5689

Closed

22 tasks

This was referenced Jan 19, 2021

500 error on attempt to download missing file submissions via "N unread" #5721

Closed

404 errors when attempting to download missing files in JI #5722

Closed

rmol mentioned this pull request Jan 20, 2021

fix download error handling when files are missing #5733

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle FileNotFoundError on download of messages #5573

Handle FileNotFoundError on download of messages #5573

DrGFreeman commented Oct 10, 2020 •

edited

Loading

eloquence commented Oct 13, 2020

DrGFreeman commented Oct 14, 2020

DrGFreeman commented Oct 15, 2020

eloquence commented Oct 27, 2020

DrGFreeman commented Oct 27, 2020

eloquence commented Oct 27, 2020

sssoleileraaa left a comment

sssoleileraaa commented Oct 28, 2020

sssoleileraaa commented Oct 28, 2020 •

edited

Loading

DrGFreeman commented Oct 30, 2020

sssoleileraaa left a comment

nabla-c0d3 Nov 5, 2020

DrGFreeman Nov 5, 2020

DrGFreeman commented Nov 5, 2020

DrGFreeman commented Nov 5, 2020

eloquence commented Nov 5, 2020 •

edited

Loading

DrGFreeman commented Nov 6, 2020

sssoleileraaa left a comment

Handle FileNotFoundError on download of messages #5573

Handle FileNotFoundError on download of messages #5573

Conversation

DrGFreeman commented Oct 10, 2020 • edited Loading

Status

Description of Changes

Testing

Deployment

Checklist

If you made changes to the server application code:

If you made changes to securedrop-admin:

If you made changes to the system configuration:

If you made non-trivial code changes:

If you made changes to documentation:

If you added or updated a code dependency:

eloquence commented Oct 13, 2020

DrGFreeman commented Oct 14, 2020

DrGFreeman commented Oct 15, 2020

eloquence commented Oct 27, 2020

DrGFreeman commented Oct 27, 2020

eloquence commented Oct 27, 2020

sssoleileraaa left a comment

Choose a reason for hiding this comment

sssoleileraaa commented Oct 28, 2020

sssoleileraaa commented Oct 28, 2020 • edited Loading

DrGFreeman commented Oct 30, 2020

sssoleileraaa left a comment

Choose a reason for hiding this comment

nabla-c0d3 Nov 5, 2020

Choose a reason for hiding this comment

DrGFreeman Nov 5, 2020

Choose a reason for hiding this comment

DrGFreeman commented Nov 5, 2020

DrGFreeman commented Nov 5, 2020

eloquence commented Nov 5, 2020 • edited Loading

DrGFreeman commented Nov 6, 2020

sssoleileraaa left a comment

Choose a reason for hiding this comment

DrGFreeman commented Oct 10, 2020 •

edited

Loading

If you made changes to `securedrop-admin`:

sssoleileraaa commented Oct 28, 2020 •

edited

Loading

eloquence commented Nov 5, 2020 •

edited

Loading