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

Predicate FileWidget state on File.is_decrypted, not is_downloaded #768

Merged
merged 1 commit into from Feb 11, 2020

Conversation

rmol
Copy link
Contributor

@rmol rmol commented Feb 4, 2020

Description

To export or print, a file must have been decrypted, so present FileWidget affordances based on that, not just whether it's been downloaded.

Also change how FileWidget's download animation happens: now it starts immediately, and the timer is used when stopping instead, to ensure it always runs for at least 300ms. Credit to @eloquence for that idea.

Finally, start a DownloadException hierarchy, adding DownloadDecryptionException to the existing
DownloadChecksumMismatchException, so we can better diagnose retrieval problems.

Fixes #756.

Also fixes #754 as I had to fix the animation to test.

Test Plan

  • Start a SecureDrop development server instance with make dev in a local working copy.

  • Run the client with run.sh

  • In a web browser, upload a file through the dev server's source interface.

  • In the client, find the source and the file and click the download button to retrieve it. The downloading animation should start, run briefly, and stop, resulting in the export/print buttons becoming available.

  • Stop the client, and add a line like raise CryptoError("BOOM!") at the beginning of FileDownloadJob.call_decrypt.

  • Stop and restart the dev server.

  • Upload another file.

  • Restart the client.

  • Try to download the file. The download animation should start, an error should appear in the status bar, the download animation should stop, and you should be able to retry the download. (This is also the test for downloading animation never ends if the file cannot be decrypted #754.)

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

  • I have tested these changes in the appropriate Qubes environment
  • I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
  • These changes should not need testing in Qubes

If these changes add or remove files other than client code, packaging logic (e.g., the AppArmor profile) may need to be updated. Please check as applicable:

  • I have submitted a separate PR to the packaging repo
  • No update to the packaging logic (e.g., AppArmor profile) is required for these changes
  • I don't know and would appreciate guidance

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.

first pass things look pretty good

if a CryptoError is raised during decryption of a file, which I need to be able to test in order to verify that this fixes the 2 named bugs, the client crashes with the following error:

QWidget::setTabOrder: 'first' and 'second' must be in the same window
Traceback (most recent call last):
  File "/home/creviera/workspace/freedomofpress/securedrop-client/securedrop_client/logic.py", line 718, in on_file_download_failure
    self.file_missing.emit(exception.uuid)
AttributeError: 'InterfaceError' object has no attribute 'uuid'

@rmol
Copy link
Contributor Author

rmol commented Feb 4, 2020

That was happening because it didn't receive a DownloadException. A CryptoError raised in any call_decrypt method of a DownloadJob should be chained into a DownloadDecryptionException, preventing this. Not sure how the InterfaceError wound up there, but I've changed Controller.on_file_download_failure to only signal file_missing when the exception makes the file's UUID available.

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 see that we are now always showing the animation in order to avoid a flicker if our operation duration happens to be just a little bit higher than our delay

✔️ This fixes #756

❌ i'm not seeing this fix #754, if there is a crypto error raised in decrypt_submission_or_reply, you'll see the follow animation:
Peek 2020-02-06 14-29

This needs a rebase, most notably you need to make sure to update:

@pyqtSlot(str)
def _on_file_downloaded(self, file_uuid: str) -> None:

to:

    @pyqtSlot(str, str, str)
    def _on_file_downloaded(self, source_uuid: str, file_uuid: str, filename: str) -> None:

which matches what the file_ready signal emits and this is what fixed #774

@eloquence eloquence moved this from Ready for Review to In Development in SecureDrop Team Board Feb 10, 2020
@rmol rmol force-pushed the 756-download-suffices-not branch 2 times, most recently from dbb5459 to 2d020e8 Compare February 10, 2020 17:41
@rmol rmol moved this from In Development to Ready for Review in SecureDrop Team Board Feb 10, 2020
@rmol rmol self-assigned this Feb 10, 2020
sssoleileraaa
sssoleileraaa previously approved these changes Feb 11, 2020
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.

My PR comment was addressed and this issue was fixed: #754

While testing this I noticed a new issue that is on master as well where the the "DOWNLOAD" text doesn't change on hover anymore, only the download icon does. And now that this PR fixed the issue with the never-ending download spinner, we can see that the download icon remains highlighted even when your not on mouseover if the file was deleted and needs to be redownloaded:

Screenshot from 2020-02-11 09-58-27

Screenshot from 2020-02-11 10-01-02

To export or print, a file must have been decrypted, so present
FileWidget affordances based on that, not just whether it's been
downloaded.

Also change how FileWidget's download animation happens: now it starts
immediately, and the timer is used when stopping instead, to ensure it
always runs for at least 300ms. Credit to @eloquence for that idea.

Finally, start a DownloadException hierarchy, adding
DownloadDecryptionException to the existing
DownloadChecksumMismatchException, so we can better diagnose retrieval
problems.
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.

@sssoleileraaa sssoleileraaa merged commit 2988041 into master Feb 11, 2020
SecureDrop Team Board automation moved this from Ready for Review to Done Feb 11, 2020
@sssoleileraaa sssoleileraaa deleted the 756-download-suffices-not branch February 11, 2020 18:33
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
2 participants