Skip to content
This repository has been archived by the owner on Jan 5, 2024. It is now read-only.

Separate print, USB export related actions into their own classes #41

Merged
merged 4 commits into from Dec 19, 2019

Conversation

redshiftzero
Copy link
Contributor

Closes #33, breaks up the SDExport class, we now have:

  • SDExport (same name) which contains submission archive related information and helper methods, like the nice Metadata object that was already on master
  • *Action objects, one for each type of export we support, which contain methods/attributes related to the export action in question. Each export action must implement run().

Two things that could be followups would be:

  • see if we can make SDExport.exit_gracefully more standalone: I ended up not making a utils.py as the exit_gracefully method is used almost everywhere, and part of its cleanup is removing the submission's tmpdir. So I'd either need to pass tmpdir around everywhere, which is messy, or just leave it as a method on SDExport, which is what I elected to do for now. The fact this is used so widely is also what complicated removing the submission from the test actions.
  • combining usb-test and disk-test: I didn't do that here as I wanted to keep the inter-VM interface the same (i.e. no changes are needed in securedrop-client to merge this) but I think this would make sense to do.

Since I'm moving around the furniture, feel free to let me know if you don't like where things are @emkll & @creviera :-)

Test plan

I have tested the following with my HP LaserJet and a USB drive:

  • Ensure you can still USB export successfully
  • Ensure you can print a document successfully

@redshiftzero redshiftzero added this to Ready for Review in SecureDrop Team Board Dec 18, 2019
securedrop_export/exceptions.py Outdated Show resolved Hide resolved
securedrop_export/usb/actions.py Outdated Show resolved Hide resolved
securedrop_export/usb/actions.py Outdated Show resolved Hide resolved
securedrop_export/main.py Show resolved Hide resolved
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.

Two things that could be followups would be:

  • see if we can make SDExport.exit_gracefully more standalone: I ended up not making a utils.py as the exit_gracefully method is used almost everywhere, and part of its cleanup is removing the submission's tmpdir. So I'd either need to pass tmpdir around everywhere, which is messy, or just leave it as a method on SDExport, which is what I elected to do for now. The fact this is used so widely is also what complicated removing the submission from the test actions.

I agree that the preflight check Actions and PrintTestPageAction should not require a submission object. It seems like it makes more sense for there to be a Printer object with printer_uri etc that you can pass to printer Actions, and a DiskDrive object that you pass to disk transfer actions. I think it would make sense to create a followup discussion issue.

  • combining usb-test and disk-test: I didn't do that here as I wanted to keep the inter-VM interface the same (i.e. no changes are needed in securedrop-client to merge this) but I think this would make sense to do.

I agree with this followup.

✔️ I made sure I could still print a file from a LaserJet Pro M404n printer and transfer a file to a usb disk drive.

this is more accurate since there are no _strict_ USB
requirements in the current export code: removable block devices
of other kinds can be exported to
@redshiftzero
Copy link
Contributor Author

redshiftzero commented Dec 19, 2019

thanks for the comments! agreed all around, added two commits (and did a quick retest of the USB export... i mean Disk export 😇)

@sssoleileraaa sssoleileraaa moved this from Ready for Review to Under Review in SecureDrop Team Board Dec 19, 2019
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.

lgtm

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

breaking up SDExport
2 participants