Refactor: CLI wrapper, export and print services, single entrypoint #105
Conversation
rocodes
force-pushed
the
refactor-cli
branch
2 times, most recently
from
58aaa2f to
a19e37d
Compare
|
(mild puzzlement since all tests pass locally, but will figure it out) |
|
(Blocked by/dependent on freedomofpress/securedrop-client#1594) |
rocodes
force-pushed
the
refactor-cli
branch
from
47dc4b7 to
a7790bc
Compare
rocodes
force-pushed
the
refactor-cli
branch
from
a7790bc to
50a917d
Compare
|
Everything's passing locally, but the tests that are failing on CI are the ones where |
|
There's a problem with our Edit: will be fixed in #115 |
rocodes
force-pushed
the
refactor-cli
branch
4 times, most recently
from
2738085 to
19d18d7
Compare
rocodes
force-pushed
the
refactor-cli
branch
from
19d18d7 to
a8fe78d
Compare
|
The following zip file contains a test artifact signed with my public key. However, I recommend skipping the trust exercise and building your own .deb per the method in the PR ;) securedrop_export_test_deb_and_build_logs.tar.gz
|
There was a problem hiding this comment.
As discussed offline, I added some nitpicks, identified by "nit".
I've been up to and stopped before revieweing the tests directory.
Otherwise, in reading order:
- I find the docstrings of the
CLImethods very useful and well calibrated. ⭐ CLI.mount_volumehandles abstracts nicely the fact that volumes might or not have been previously mounted!- The entire
CLIclass uses a very consistent pattern of returning a usable object or raising an exception, that helps a lot forming expectations, I love it. - I marked a few places to think about sanitizing input. The device name doesn't obviously look like user input, but given the devices are user-provided, and their name is used in subsequent commands, I think it might be prudent to treat it as such? (@L3th3 @lsd-cat)
- The
importblocks thorough the change set are super clean/shallow! At first glance, I take at that as a sign that the separation of concerns is very likely sound. (The names make sense too, so I think it is indeed sound.) - The entire
disk.Volumeis very neat. - The
mainmodule is pretty clean too, I like it.
Not having run anything yet, I feel really good about this PR. To be continued!
.semgrep/custom-rules.yaml
Outdated
| @@ -47,7 +47,7 @@ rules: | |||
| languages: | |||
| - python | |||
| severity: ERROR | |||
| message: Possible path traversal or insecure directory and file permissions through os.mkdir(). Use securedrop_export.utils.safe_mkdir instead. | |||
| message: Possible path traversal or insecure directory and file permissions through os.mkdir(). Use securedrop_export.directory_util.safe_mkdir instead. | |||
There was a problem hiding this comment.
Naming nit: could it be securedrop_export.directory.safe_mkdir?
Picking on util as a smell, and thinking that directory seems perfectly legit both in the context of the other modules and with respect to what the module contains. What does _util bring to the name?
There was a problem hiding this comment.
The reason I called it this way, which may or may not be valid, is that it's not a class - it's a series of functions (utilities) that are imported individually to classes that need them, but are unfortunately used in multiple places so I didn't want to make them class methods since that wouldn't be DRY. Up to you if you want it changed or not.
There was a problem hiding this comment.
I get that. I'd say that the logging module defines functions, it is not called logging_util for that. Nor is os called os_utils.
I see a convention, but I think it is not a two-way convention: when defining a class (e.g. Dog) it is nice for the module to be called dog (file: dog.py), but the fact that a module is called cat doesn't imply that a Cat class must be defined in it.
In any case, I was suggesting renaming the file only, not changing anything inside.
There was a problem hiding this comment.
Addressed in ed6ea1e
| @@ -0,0 +1,101 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Is this needed? I'd think it is if part of the script is executed directly, but I don't see any __main__ function that suggests that. (I may also not be aware of other uses!)
There was a problem hiding this comment.
A: was there, isn't new. May or not be needed, out of scope.
| is_removable = False | ||
| try: | ||
| removable = subprocess.check_output( | ||
| ["cat", f"/sys/class/block/{device}/removable"], |
There was a problem hiding this comment.
Is there an opportunity to sanitize device here?
There was a problem hiding this comment.
device here is the device identifier assigned by the OS. (cat /sys/class/block/xvdc/removable, for example). I do not think sanitization is required.
There was a problem hiding this comment.
I'm on the fence because that string originates outside of the scope of the function. But I'm not overly concerned by it either.
cc: @L3th3 @lsd-cat in case you've got any advice to give us. (I'd otherwise second @rocodes' suggestion that we keep this as it is, with no sanitization.)
Edit: note to selves :P if we decide to take action on this, there are a few occurrences to follow up on.
securedrop_export/directory_util.py
Outdated
| import logging | ||
|
|
||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
Nope, my bad -removed 🤦
securedrop_export/disk/cli.py
Outdated
| from securedrop_export.exceptions import ExportException | ||
|
|
||
| from .volume import EncryptionScheme, Volume | ||
| from .new_status import Status |
There was a problem hiding this comment.
Naming nit: If the new_*.py modules were instead called status.py and service.py, and the other ones called legacy_*.py:
- while both exist, it would be clear that the legacy code shouldn't be built upon
- when time comes to remove the legacy code, there would be no need to touch the files that define or import the code that won't be removed
There was a problem hiding this comment.
Addressed in 5e12476
securedrop_export/main.py
Outdated
| from securedrop_export.disk.service import Service as ExportService | ||
| from securedrop_export.print.service import Service as PrintService |
There was a problem hiding this comment.
Since the services are intended to be used outside the disk and print modules, I would export them in disk/__init__.py (resp. print/__init__.py) as follows:
# __init__.py
from .service import Service # noqa: F401That would allow to import them without reaching into the internals of the modules:
| from securedrop_export.disk.service import Service as ExportService | |
| from securedrop_export.print.service import Service as PrintService | |
| from securedrop_export.disk import Service as ExportService | |
| from securedrop_export.print import Service as PrintService |
I see that as documenting which elements of a module are meant to be used / constitute its API. Accordingly, an import path with more than one dot is a smell to me.
There was a problem hiding this comment.
Thank you! addressed in eaef241
|
USB Acceptance tests
Additional testing
The warning line is optional but will show up if the user manually mounts a drive somewhere we weren't expecting (eg via Nautilus, like in this test plan). Printer acceptance tests
|
|
Heads up @rocodes I've got no printer to test the three conditions I skipped at the end of the list above. (I'm working on all the other ones.) |
There was a problem hiding this comment.
Test plan check out 🚀
Two comments:
- I haven't tested the scenarios involving a printer. (Marked as SKIP in my report.)
- I've noted that two items of Improve error handling for unsupported drive configurations #70 are not handled (and handling them wasn't the intention in this PR), and that the issue wasn't marked to be closed, so all is great 🙂 I'll edit the issue to take note of the progress.
- I have a few suggestions for changes in the test suite, but I think those can perfectly be addressed as follow ups, as they don't affect the efficacy of the test suite. Given we've tested the branch as-is, I'd merge it and refactor what we want later.
Base on the above, I'll approve the PR. @rocodes I let you either update the status of the printer items, or we can talk about how to get them covered 🙂
P.S.: Thank you for the thorough test plan, and the script to build the package 😍 those made review a breeze.
|
Thanks for your review :) I'll add printer tests shortly so let's hold off on merging. I'm also going to address some of your review feedback, then squash commits once you've had the chance to take a peek. |
There was a problem hiding this comment.
since you'll take some of the review comments in consideration now, I'll add the few I mentioned yesterday. Again, I wouldn't mind taking that to follow up time, I let you judge when is a good time.
Can't be attached to the changes:
- `tests/disk/test_status.py is an empty file?
tests/disk/test_service.py
Outdated
| self.mock_cli.get_connected_devices.side_effect = ExportException( | ||
| sdstatus=NewStatus.ERROR_MOUNT | ||
| ) |
There was a problem hiding this comment.
Naming nit: In general terms, I think that naming all mocks mock_* is unnecessary. The readability improvement is debatable, and the mock_* prefix doesn't add any new information. From the moment you assign to it or assert on invocations, the thing is being treated as controlled entity, a mock, anyway.
I would say that this block is meant to be understood as:
"Given self.cli.get_connected_devices's side effect is ExportException(...)"
And that 1. is closer to that than 2.:
1. self.cli.get_connected_devices.side_effect = ExportException(...)
2. self.mock_cli.get_connected_devices.side_effect = ExportException(...)
Isn't it?
There was a problem hiding this comment.
I see value in indicating which components are stubs/mocks and which ones are 'live', personally, just to make it explicit that we are using a 'fake' cli and not the real one.
There was a problem hiding this comment.
Fair. That's something we can talk about if we'd prefer having a consistent convention. I'd usually not prefix variables, and go for shorter names the closer usage is from the declaration. (In Python I don't go all the way to the one letter convention in Go, but in my experience that scope-size-to-name-length heuristic does improve readability, so I follow that spirit.)
|
Just stepped through printer acceptance tests on Brother HL-L2360DW ✔️ Will address @gonzalo-bulnes' review comments, file naming etc, and clean up commits |
rocodes
force-pushed
the
refactor-cli
branch
2 times, most recently
from
a73cec5 to
282a9a0
Compare
|
(Sorry, had to rebase. The last 5 commits address review feedback, the rest is unchanged. Once you've seen these I will squash/clean up.) |
Exceptions. Reorganize Print actions into methods in service class. Use commands to separate different types of export and print actions. Rename SDExport to Archive. Use methods instead of classes for each export routine. Move ExportException to common directory and add Command enum for supported export commands.
Refactor test suite to match new export and print services and CLI wrapper.
improve CLI test coverage.
semgrep rules. i#add directory_util.py test coverage, show new name in semgrep rules
test_archive.py
…nger inherits from ExportException. Small fix to export metadata and get_partitioned_devices.
…ferred. Log exceptions when they occur instead of during graceful exit. Rename old service and status files to legacy_*; address review feedback.
There was a problem hiding this comment.
I'll approve and merge this PR, based on:
- reviewing the changes individually
- the diff since my last full-checklist review
Refactor export PR.
main.pyandentrypoint.pyexportStatusvalues; the existing (legacy) export service wraps & returns backward-compatible old statuses that the current client is expecting. (When we migrate, we will get rid of the files now called service.py and status.py)main.pyThis will address the following open issues:
Closes #107, closes #84, closes #44, closes #114, closes #9, closes #25
Towards #70, freedomofpress/securedrop-workstation#265
freedomofpress/securedrop-client#1734 will be implemented but will need corresponding UI changes in the client in order for the change to be user-visible
Test Plan
Setup
From an up-to-date SDW machine:
sd-large-bullseye-template, let's call ittest-sd-large-bullseye-template. Then, in dom0,qvm-tags sd-large-bullseye-template del sd-workstation.test-sd-large-bullseye-templateusing dpkg. To do this, use either the prebuilt .deb file that I will upload and sign when ready for review, or better yet, build your own .deb from the tip of this branch. (If you have a clean dispvm builder environment as described here, put this script in the builder vm, then run./builder.sh securedrop-export 105and you'll get a test .deb built and you'll be prompted to copy it into the vm of your choice :) ).test-sd-large-bullseye-template. Then use either the cli or dom0 Template Manager, and set thetest-sd-large-bullseye-templateas the templateVM forsd-devices-dvm.sd-devices(andsd-devices-dvm) are powered down.USB Acceptance tests
Pro tip: Export works in offline mode once a file is already downloaded. If the Tor weather is bad, you will thank me for mentioning this ;)
Additional testing
metadata.jsonis not retained on sd-devices at end of successful or failed export routine. (Note: this does not resolve a comment in the issue about avoiding writingmetadata.jsonto disk entirely. At reviewer's discretion whether or not to close the issue or re-title it).securedrop-export/disk/cli.pyandstatus.pyto observe the improved error handling for unsupported drives, although client changes are required to make this user-visibleThe warning line is optional but will show up if the user manually mounts a drive somewhere we weren't expecting (eg via Nautilus, like in this test plan).
Printer acceptance tests