Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attempting to delete a reply whose corresponding GPG file is missing generates a 500 in the JI #5543

Closed
zenmonkeykstop opened this issue Sep 30, 2020 · 6 comments · Fixed by #5549

Comments

@zenmonkeykstop
Copy link
Contributor

zenmonkeykstop commented Sep 30, 2020

Description

Attempting to delete a reply whose corresponding GPG file is missing generates a 500 in the JI

Steps to Reproduce

In the JI:

  • reply to a source
    On the app server:
  • delete the corresponding reply file in /var/lib/securedrop/store
    In the JI:
  • delete the reply

Expected Behavior

Good question! I would err on the side of deleting the db entry and calling it done. But it could flag the issue to the user in more detail than that provided by the 500 error page text.

This also has issues for deleting multiple files - if multiples are selected, deletion stops at the missing reply file. Expected behaviour in that case would be to continue with the operation.

Actual Behavior

500 error thrown. If multiple files selected for deletion, all files listed after the buster reply file remain in place.

@emkll
Copy link
Contributor

emkll commented Sep 30, 2020

possibly related to #4787

@rmol
Copy link
Contributor

rmol commented Sep 30, 2020

I was going to agree that the file being gone is the desired state, so we could just purge the Reply record and continue, but a missing file for a reply probably indicates a problem which should be brought to the attention of the admin. Either there's a hardware or filesystem problem, or someone's been mucking about in the store.

This situation is going to cause the same problem as #4787 but right now the cleanup tool we'd tell the admin to run does not address replies, just submissions.

@eloquence
Copy link
Member

Just adding my standard caveat, let's try for a simple fix for 1.6.0 as discussed, but if scope expands significantly we may have to defer. :)

@zenmonkeykstop
Copy link
Contributor Author

I think the recco in #4787 for the related case is the best option overall - delete the entry and flash a message like "the database and file store are in an inconsistent state - this can be manually resolved by an administrator, so go get one."

@zenmonkeykstop
Copy link
Contributor Author

zenmonkeykstop commented Sep 30, 2020

@rmol are you sure the cleanup tool doesn't sort out replies? ISTR you adding that later in.

@rmol
Copy link
Contributor

rmol commented Sep 30, 2020

@zenmonkeykstop Pretty sure it only checks submissions. I think you're remembering that the inverse operation, looking for files without database records, needed improving so that reply files wouldn't be counted as disconnected because they didn't have submission records.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants