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

app: delete source collection when source is deleted locally #866

Merged
merged 3 commits into from
Mar 4, 2020

Conversation

redshiftzero
Copy link
Contributor

Description

Fixes #856

There are two ways sources can be deleted: via the metadata sync (since sources and submissions can be deleted by other users) and via the source deletion job (if the client user deletes a source). In the latter place, we were previously only deleting the source database row, the documents would remain on disk.

Test Plan

  1. Apply the AppArmor profile change and this code diff. You can do either by building the deb on this branch and installing it in sd-app, OR by copying the AppArmor profile into place (in /etc/apparmor.d/), running sudo service apparmor restart, and copying the code on top of the existing installed code (e.g. in the root of the git repository sudo cp -r securedrop_client /opt/venvs/securedrop-client/lib/python3.7/site-packages/).
  2. Start client via securedrop-client.
  3. Log in.
  4. Submit a file as a source. Download it and wait for it to finish.
  5. ls -la ~/.securedrop_client/data. You should see a folder in here with the designation of the source, and the file inside that folder.
  6. Delete the source.
  7. Check syslog and verify no AppArmor denials.
  8. ls -la ~/.securedrop_client/data. You should see the file and the entire folder is now gone.

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

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:

  • I have submitted a separate PR to the packaging repo
  • No update to the packaging logic is required for these changes - the AppArmor change is included here
  • I don't know and would appreciate guidance

"""
Delete the source with the referenced UUID.
"""
source = session.query(Source).filter_by(uuid=uuid).one_or_none()
if source:
delete_source_collection(source.journalist_filename, data_dir)
Copy link
Contributor

@sssoleileraaa sssoleileraaa Mar 4, 2020

Choose a reason for hiding this comment

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

checking my understanding: before we were only deleting the source locally from the db after we successfully sent a source-deletion job to the server, and then we'd rely on the metadata sync to tell us to delete a sources files on disk:

# The uuids remaining in local_uuids do not exist on the remote server, so
# delete the related records.
for deleted_source in local_sources_by_uuid.values():
for document in deleted_source.collection:
if isinstance(document, (Message, File, Reply)):
delete_single_submission_or_reply_on_disk(document, data_dir)
session.delete(deleted_source)
logger.debug('Deleted source {}'.format(deleted_source.uuid))
session.commit()

this change makes it so we will delete a source's files ahead of time, before a sync? or was there a bug where the files were never getting deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Back when we were calling sync_api after a deletion in the deletion success callback: the sync would delete the files from a deleted source. The deletion job itself would not delete either the database row or the corresponding files, relying on the sync to do so. After we removed the sync_api call, it was replaced with a function in the deletion success callback that only deleted the database row. I don't think the intention was to rely on the sync in that case, since that would not have worked - the corresponding database rows (which the sync uses to determine which items to delete) are now gone.

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.

so i have a comment that is fine to respond to after merge

code lgtm and works

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[0.2.1-deb] AppArmor denial causing deletion to not take effect
2 participants