-
Notifications
You must be signed in to change notification settings - Fork 42
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
Conversation
There was a problem hiding this 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'
That was happening because it didn't receive a |
b8a74c9
to
f402760
Compare
There was a problem hiding this 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:
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
dbb5459
to
2d020e8
Compare
There was a problem hiding this 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:
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.
2d020e8
to
adea1c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, addingDownloadDecryptionException
to the existingDownloadChecksumMismatchException
, 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 ofFileDownloadJob.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:
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: