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

Handle messages and files that fail to decrypt #287

Merged
merged 7 commits into from Apr 5, 2019

Conversation

redshiftzero
Copy link
Contributor

Fix #276

Previously:

Now:

  • Files that fail to decrypt will get decrypted 5 seconds after a key becomes available
  • If decryption is failing, the file will only be downloaded once and then saved on disk. The message sync / reply sync threads will resume and decrypt

Test with STR in #276 and ensure that the download endpoints are only hit once per file

@eloquence eloquence added this to Ready for review in SecureDrop Team Board Mar 29, 2019
@heartsucker heartsucker self-assigned this Apr 1, 2019
Copy link
Contributor

@heartsucker heartsucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as advertised. The nits about noqa feel like they should be addressed since we are disabling too many rules with that syntax and it may prevent us from catching bugs in the future. However, it is minor, so I'm approving. I'll let you decide if this is worth correcting or not.

securedrop_client/storage.py Outdated Show resolved Hide resolved
securedrop_client/storage.py Outdated Show resolved Hide resolved
securedrop_client/storage.py Outdated Show resolved Hide resolved
securedrop_client/storage.py Outdated Show resolved Hide resolved
tests/test_storage.py Show resolved Hide resolved
tests/test_storage.py Show resolved Hide resolved
@heartsucker
Copy link
Contributor

During stand up we decided to make these changes so we could get this in while @redshiftzero is OOO. @creviera Can you review these two extra patches?

Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran through the steps to reproduce in #276 on master and on download-then-decrypt and confirm that the fix works as documented with download endpoints only being hit once per file.

@@ -706,6 +707,7 @@ def test_find_new_replies(mocker, session):
session.add(reply_not_downloaded)
session.add(reply_decrypt_failed)
session.add(reply_decrypt_success)
session.commit()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -661,6 +661,7 @@ def test_find_new_messages(mocker, session):
session.add(message_not_downloaded)
session.add(message_decrypt_failed)
session.add(message_decrypt_success)
session.commit()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@heartsucker heartsucker merged commit 2e425ce into master Apr 5, 2019
SecureDrop Team Board automation moved this from Ready for review to Done Apr 5, 2019
@heartsucker heartsucker deleted the download-then-decrypt branch April 5, 2019 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants