Clean up orphaned submissions/replies no longer associated with sources #4672

redshiftzero · 2019-08-15T02:17:17Z

Status

Ready for review

Description of Changes

Fixes #1189

Changes proposed in this pull request:

Remove submissions and replies that are associated with null sources or sources that no longer exist

Testing

Other than reviewing the diff (see commit messages for details):

Provision staging on develop
SSH in and do the following as the www-data user (sudo su www-data -s /bin/bash)
Don’t submit anything via the source interface, so the database will be empty.
Create a submission associated with a source that doesn’t exist:

www-data@app-staging:/home/vagrant$ cd /var/lib/securedrop/
www-data@app-staging:/home/vagrant$ sqlite3 db.sqlite
sqlite> insert into submissions values (1, "uuid-1", 9000, "1-test-potato-doc.gz.gpg", 1, 0, "checksum”);

Insert one more submission - we won’t create a file for this one to simulate a scenario where the admin has gone ahead and manually deleted some of the orphaned files (for example if they were running low on disk utilization):

sqlite> insert into submissions values (2, "uuid-2", 9000, "2-test-potato-doc.gz.gpg", 1, 0, "checksum");

Make a test file such that we can assert that the procedure cleans up the files also:

www-data@app-staging:/home/vagrant$ mkdir /var/lib/securedrop/store/filesystem_id
www-data@app-staging:/home/vagrant$ touch /var/lib/securedrop/store/filesystem_id/1-test-potato-doc.gz.gpg

Now build debs on this branch and provision staging again (e.g. make build-debs and vagrant provision /staging/). The migration should run as the app code package is installed.
Verify the submission we added is now gone, as are the files themselves:

www-data@app-staging:/home/vagrant$ ls /var/lib/securedrop/store/filesystem_id/
www-data@app-staging:/home/vagrant$

sqlite> select * from submissions;
sqlite>

Deployment

This will run in postinst of securedrop-app-code package

Checklist

If you made changes to the server application code:

Linting (make lint) and tests (make -C securedrop test) pass in the development container

If you made non-trivial code changes:

I have written a test plan and validated it for this PR

This is accomplished by an alembic migration that removes any orphaned submissions from the database.

if a source has been deleted, we no longer have its filesystem_id in the database. as such, we will need to find the file, being careful to guard against potential duplicates due to the journalist_designations not necessarily being unique (this is a very rare case)

… files

two situations _could_ potentially arise trying to think of the edge cases here: - Admin has been deleting files manually by digging around for files that consume a lot of disk in the securedrop data directory - A very rare situation can occur where there is a colliding journalist_designation

invalid objects being those which do not have a matching source_id in the sources table

easiest to do with a custom exception, but for files that were manually deleted by a curious admin who e.g. removed large files that were not properly deleted due to this bug, we should also remove the corresponding row in the database.

bandit flags this because the table name is passed in as a variable. It's not user controlled so this does not introduce a security problem (function is in the migration only). I could suppress the alert by removing the function but in the spirit of not making this alembic migration even more repetitive I'm adding `# nosec`.

one of the quirks of SecureDrop is that config.py might not yet exist until the app Ansible role has ran this means for fresh installs, the database must be created via alembic upgrade head without any errors raising, else the securedrop-app-code package install will fail

redshiftzero · 2019-08-29T15:01:55Z

OK this one I just rebased on top of develop, updates from the latest deletion code changes you can see in 08cfcd8. Ready for review!

redshiftzero · 2019-08-29T18:04:33Z

the staging CI failure here is legit, investigating

so it won't import on existing installs

redshiftzero · 2019-08-29T20:22:32Z

[still failing so re-provisioning staging on this branch so I can reproduce the fresh install scenario from CI]

rmol · 2019-08-29T20:58:14Z

Looks good to me. Got through the test plan, and problems were cleaned up. So as soon as we appease CI....

redshiftzero · 2019-08-29T21:00:39Z

OK kewl, pretty sure I have a fix, testing locally. It's just fixing more import paths due to the changes introduced on develop. Basically (this is useful for future maintainers) the issue here is that when the migrations run, config.py may not yet exist - this is the scenario when the securedrop-app-code package is installed on a fresh system. In this case, any imports that then import config.py will fail and the securedrop-app-code package will not be installed, failing the entire install.

redshiftzero · 2019-08-29T21:52:26Z

OK latest commit should fix the staging CI job (did for me locally - just moving imports under that try/except) - if it does, @conorsch please stamp this one (heads up that we will also test this scenario during QA for 1.0.0).

codecov-io · 2019-08-29T22:29:13Z

Codecov Report

Merging #4672 into develop will increase coverage by 13.52%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           develop    #4672       +/-   ##
============================================
+ Coverage    68.08%   81.61%   +13.52%     
============================================
  Files           48       49        +1     
  Lines         3350     3416       +66     
  Branches       380      391       +11     
============================================
+ Hits          2281     2788      +507     
+ Misses         966      535      -431     
+ Partials       103       93       -10

Impacted Files	Coverage Δ
...rsions/3da3fcab826a_delete_orphaned_submissions.py	`34.69% <0%> (ø)`
securedrop/securedrop/journalist_app/__init__.py	`88.23% <0%> (+1.96%)`	⬆️
securedrop/securedrop/journalist_app/utils.py	`89% <0%> (+3.66%)`	⬆️
securedrop/securedrop/store.py	`93.79% <0%> (+5.51%)`	⬆️
securedrop/securedrop/models.py	`89.5% <0%> (+6.35%)`	⬆️
securedrop/securedrop/crypto_util.py	`88.61% <0%> (+8.94%)`	⬆️
securedrop/securedrop/source_app/utils.py	`89.47% <0%> (+12.28%)`	⬆️
securedrop/securedrop/source_app/decorators.py	`86.66% <0%> (+13.33%)`	⬆️
securedrop/securedrop/template_filters.py	`96.15% <0%> (+19.23%)`	⬆️
securedrop/securedrop/source_app/__init__.py	`92.39% <0%> (+19.56%)`	⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2d222c...b515226. Read the comment docs.

codecov-io · 2019-08-29T22:29:13Z

Codecov Report

Merging #4672 into develop will increase coverage by 13.52%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           develop    #4672       +/-   ##
============================================
+ Coverage    68.08%   81.61%   +13.52%     
============================================
  Files           48       49        +1     
  Lines         3350     3416       +66     
  Branches       380      391       +11     
============================================
+ Hits          2281     2788      +507     
+ Misses         966      535      -431     
+ Partials       103       93       -10

Impacted Files	Coverage Δ
...rsions/3da3fcab826a_delete_orphaned_submissions.py	`34.69% <0%> (ø)`
securedrop/securedrop/journalist_app/__init__.py	`88.23% <0%> (+1.96%)`	⬆️
securedrop/securedrop/journalist_app/utils.py	`89% <0%> (+3.66%)`	⬆️
securedrop/securedrop/store.py	`93.79% <0%> (+5.51%)`	⬆️
securedrop/securedrop/models.py	`89.5% <0%> (+6.35%)`	⬆️
securedrop/securedrop/crypto_util.py	`88.61% <0%> (+8.94%)`	⬆️
securedrop/securedrop/source_app/utils.py	`89.47% <0%> (+12.28%)`	⬆️
securedrop/securedrop/source_app/decorators.py	`86.66% <0%> (+13.33%)`	⬆️
securedrop/securedrop/template_filters.py	`96.15% <0%> (+19.23%)`	⬆️
securedrop/securedrop/source_app/__init__.py	`92.39% <0%> (+19.56%)`	⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2d222c...b515226. Read the comment docs.

conorsch

Approving based on visual review.

redshiftzero requested review from heartsucker and kushaldas as code owners August 15, 2019 02:17

eloquence changed the title ~~clean up orphaned submissions/replies no longer associated with sources~~ Clean up orphaned submissions/replies no longer associated with sources Aug 15, 2019

redshiftzero force-pushed the bug-1189-contrib branch from da9e3c0 to f052f02 Compare August 20, 2019 14:07

ehashman and others added 9 commits August 29, 2019 10:15

Delete Submissions with Sources that were deleted

2ea900c

This is accomplished by an alembic migration that removes any orphaned submissions from the database.

alembic: add replies to the migration and enqueue jobs to cleanup the…

1aa5c07

… files

alembic: we should also delete objects that have invalid source_ids

b4ef839

invalid objects being those which do not have a matching source_id in the sources table

deletion cleanup: srm -> secure_delete, new session_nonce

08cfcd8

redshiftzero force-pushed the bug-1189-contrib branch from f052f02 to 08cfcd8 Compare August 29, 2019 14:51

deletion cleanup: worker.py now imports config.py

679aa5c

so it won't import on existing installs

deletion cleanup: mv two imports that import worker.py into try/except

b515226

conorsch approved these changes Aug 29, 2019

View reviewed changes

conorsch merged commit d3d3ab7 into develop Aug 29, 2019

conorsch deleted the bug-1189-contrib branch August 29, 2019 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up orphaned submissions/replies no longer associated with sources #4672

Clean up orphaned submissions/replies no longer associated with sources #4672

redshiftzero commented Aug 15, 2019 •

edited

Loading

redshiftzero commented Aug 29, 2019

redshiftzero commented Aug 29, 2019

redshiftzero commented Aug 29, 2019

rmol commented Aug 29, 2019

redshiftzero commented Aug 29, 2019

redshiftzero commented Aug 29, 2019

codecov-io commented Aug 29, 2019

codecov-io commented Aug 29, 2019

conorsch left a comment

Clean up orphaned submissions/replies no longer associated with sources #4672

Clean up orphaned submissions/replies no longer associated with sources #4672

Conversation

redshiftzero commented Aug 15, 2019 • edited Loading

Status

Description of Changes

Testing

Deployment

Checklist

If you made changes to the server application code:

If you made non-trivial code changes:

redshiftzero commented Aug 29, 2019

redshiftzero commented Aug 29, 2019

redshiftzero commented Aug 29, 2019

rmol commented Aug 29, 2019

redshiftzero commented Aug 29, 2019

redshiftzero commented Aug 29, 2019

codecov-io commented Aug 29, 2019

Codecov Report

codecov-io commented Aug 29, 2019

Codecov Report

conorsch left a comment

Choose a reason for hiding this comment

redshiftzero commented Aug 15, 2019 •

edited

Loading