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

Basic export integration with placeholder messaging #547

Merged
merged 9 commits into from Sep 23, 2019
Merged

Conversation

sssoleileraaa
Copy link
Contributor

@sssoleileraaa sssoleileraaa commented Sep 13, 2019

Description

Adds export functionality to the client.

Test Plan

From Qubes:

  1. Start the client and make sure the Export VM is running and configured correctly
  2. Plug in your luks-encrypted transfer device and click Export next to the file you want to export
  3. Type in your passphrase and click Submit
  4. Once the transfer is successful, you will see a SecureDrop Qubes notification saying so
  5. Check you transfer device for the file

Test this with the wrong password and without a transfer device plugged in. If you see any strange behavior, first check the known issues in the securedrop-export repo to see if it's already documented.

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

@eloquence eloquence changed the title add export Basic export integration with placeholder messaging Sep 17, 2019
@eloquence eloquence moved this from In Development to Ready for review in SecureDrop Team Board Sep 17, 2019
@sssoleileraaa
Copy link
Contributor Author

Closes #526
UI and messaging will be addressed here: #549

Also, the recent commits to this PR add the following behavior, which we discussed yesterday:

1. "Preparing export" and run preflight check
2. "Please insert USB drive" if VM could not start
3. "Please contact your administrator" if VM could not start a second time

To test this behavior:

  1. Power down sd-export-usb if it's not already
  2. Making sure your thumb drive is not plugged in, click Export
  3. You probably won't see the "Preparing export" message because the code executes fast and before you know it you'll see the "Please insert USB drive" message
  4. Do not insert USB drive and click "Continue" to see the "Please contact your administrator" message
  5. Run through steps 1-3 again, but instead of not inserting your USB drive on step 4, insert it and see the next screen to enter passphrase.

For more information on why we do this, see my code documentation:
780d021#diff-16a175b4662541f3889b432ac9e7e0e8R1945-R1952

@eloquence eloquence moved this from Ready for review to Under Review in SecureDrop Team Board Sep 18, 2019
Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

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

Tested the PR as follows:

  • Provisioned workstation on lastest master
  • Built a deb on this branch, installed it in sd-svs-template
  • Started the securedrop-client , connected to a SecureDrop instance
  • Attempted to export a submission

Export works as expected ! 🎉 . This looks great, very well documented and clear. Thanks @creviera !

Two behaviors I have observed (but couldn't pinpoint the root cause in the diff):

  1. When the client refreshes and polls the server (and the client redrawn) the export windows disappear (I cannot find them anymore), which could be problematic depending on how those are timed. Also wondering in that case it it would kill the export operation?

  2. When the export popup comes up, clicking anywhere outside the box dismisses and hides it, making it impossible to reappear (other than clicking export again). If we are to make the export action blocking, we should consider making that window sticky, and perhaps preventing focus/clicks outside of that panel? What do you think?

I also have a couple of comments/suggestion inline, for discussion.

securedrop_client/export.py Outdated Show resolved Hide resolved
securedrop_client/export.py Outdated Show resolved Hide resolved
securedrop_client/gui/widgets.py Show resolved Hide resolved
securedrop_client/gui/widgets.py Show resolved Hide resolved
tests/test_export.py Show resolved Hide resolved
securedrop_client/export.py Outdated Show resolved Hide resolved
securedrop_client/export.py Outdated Show resolved Hide resolved
securedrop_client/gui/widgets.py Outdated Show resolved Hide resolved
# in case the issue is that the Export VM cannot start due to a USB device being
# unavailable for attachment. According to the Qubes docs:
#
# "If the device is unavailable (physically missing or sourceVM not started), booting
Copy link
Contributor

Choose a reason for hiding this comment

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

also an edge case (documenting here for posterity and not necessary to include in the documentation) is the case where the machine can't start for other reasons as well (maybe memory pressure or other unknown error)

securedrop_client/gui/widgets.py Show resolved Hide resolved
@sssoleileraaa
Copy link
Contributor Author

review comments addressed

@emkll
Copy link
Contributor

emkll commented Sep 23, 2019

Thanks for the changes @creviera ! Tested this again on a deb built on 05e9ca3

Still experiencing the disappearing export popup when the client redraws/is refreshed, as well as the export window disappearing when you click outside of the export popup dialog.

Steps to reproduce:

  • log in
  • export a file
  • wait 5 minutes
  • observe the window closing

Do you think the setModal property for QDialog/ExportDialog be helpful here? https://doc.qt.io/qt-5/qdialog.html#modal-prop ?

Other than the issue describe above, all other comments were addressed (this is feature complete, and we can further improve UX in #549 )

@sssoleileraaa
Copy link
Contributor Author

sssoleileraaa commented Sep 23, 2019

the export window disappearing when you click outside of the export popup dialog.

this is by design: https://projects.invisionapp.com/share/WKTCW3J7B34#/screens/377942793
this is by design, although I'm realizing now that it's been word-of-mouth

Still experiencing the disappearing export popup when the client redraws/is refreshed

this is also by design: https://projects.invisionapp.com/share/WKTCW3J7B34#/screens/377942793
this is by design, although I'm realizing now that it's been word-of-mouth

there is an issue that we need to address where the export dialog does NOT close during blocking calls to the Export VM, e.g. when the VM needs to boot up. in order to make it so the export dialog closes when the user clicks outside the dialog during VM boot up, transferring files, etc. we need to put our check_output call on a separate thread.

Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

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

Thanks for the changes and clarification @creviera.
All functionality in this PR works as expected, is blocking when a file is actually transferring and performs all preflight check in the required order for integration.
From my perspective, this is good to merge as-is, and will be very helpful to test and iterate on the export functionality in the future, where #549 will track the broader UI changes, and #556 the successful export messaging.

@emkll emkll merged commit ed1b40e into master Sep 23, 2019
SecureDrop Team Board automation moved this from Under Review to Done Sep 23, 2019
@emkll emkll deleted the issue-526-export branch September 23, 2019 20:30
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