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

Delete conversation files and records as soon as server confirms scheduled deletion #1432

Merged
merged 6 commits into from Mar 14, 2022

Conversation

rocodes
Copy link
Contributor

@rocodes rocodes commented Feb 24, 2022

Description

Delete local files and relevant source records immediately as soon as conversation is scheduled for deletion on the server (do not wait for sync to delete).

Fixes #1429
Replaces #1424
Will be relevant for #1344

High-level approach:

  • When the server has successfully scheduled files for deletion, files and records are deleted locally, and the source's UUID (not id) is inserted into a deletedconversation table (which is basically a flag). (We avoid storing source id and other PII/attributes.)
  • For the first sync after the deletion request is made, if a source's UUID is in the deletedconversation table, we do not update their record. This is to avoid a situation where a stale sync re-populates source records that have been deleted (network race condition). (Note: a sync happens every 15 seconds)
  • At the end of the first sync, the source's UUID is dropped from the deletedconversation table, and their records will be updated as normal for all subsequent syncs.
  • This approach will also work for user deletion and avoiding stale syncs repopulating a deleted user's records.

Test Plan

Test local deletion: happy path

  • Log into the client and select a source with replies, messages, and files. Download some files and observe them in a subdirectory of ~/.securedrop_client/data/
  • Safe-delete ("delete files and messages") for the source.
  • Inspect the ~/.securedrop_client/data/ directory; the source's subdirectory should be removed and all relevant files deleted.
  • Inspect the sources table and note the source's id (not UUID). Query the messages, replies, draftreplies, and files tables for rows with the source id and ensure that no matching rows are present.

Test local deletion with network failures

  • Log into the client and select a source with replies, messages, and files. Download some files and observe them in a subdirectory of ~/.securedrop_client/data/
  • Open the Qubes Settings -> network settings for sd-whonix, or open a terminal in sd-whonix.
  • Safe-delete ("delete files and messages") for the source, then as soon as the UI confirms deletion, quickly kill the network connection by setting the nevtm to none or issuing sudo service tor stop in the sd-whonix terminal.
  • Inspect the ~/.securedrop_client/data/ directory; the source's subdirectory should be removed and all relevant files deleted.
  • Inspect the sources table and note the source's id and UUID. Query the messages, replies, draftreplies, and files tables for rows with the source id and ensure that no matching rows are present.
  • Inspect the deletedconversation table and ensure that a row with the source uuid is present.
  • Re-connect the network and wait for sync.
  • Again, query the messages, replies, draftreplies, and files tables for rows with the source id and ensure that no matching rows are present.
  • Ensure the entry with source UUID has been removed from the deletedconversation table.

Ensure no regression when deletion is done on the server

  • Download files for a source with files and messages.
  • Delete the source's files and messages via the Journalist Interface.
  • Wait for sync.
  • Ensure files and messages have been deleted from the client as in the steps above.

Test upgrade of existing client

  • Ensure database upgrade works and local deletion works as above

(Added) No ghost replies

  • Safe delete a conversation, then send reply A, then safe delete again.
  • Send reply B
  • Ensure reply A is not temporarily visible in the GUI (note: this is a GUI bug; it's not added to the database)

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, the AppArmor profile may need to be updated. Please check as applicable:

  • I have updated the AppArmor profile
  • No update to the AppArmor profile is required for these changes
  • I don't know and would appreciate guidance

If these changes modify the database schema, you should include a database migration. Please check as applicable:

  • I have written a migration and upgraded a test database based on main and confirmed that the migration applies cleanly
  • I have written a migration but have not upgraded a test database based on main and would like the reviewer to do so
  • I need help writing a database migration
  • No database schema changes are needed

# til next sync to update it. The list of skip_uuids is managed by the
# parent method and purged after each sync.
if skip_uuids and source.uuid in skip_uuids:
logger.debug("Skip source update for {} (this sync only)")
Copy link
Contributor Author

@rocodes rocodes Feb 28, 2022

Choose a reason for hiding this comment

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

We don't want to update a source's file count, document count, etc., on the first sync if their files and messages have been deleted locally, since those values will be incorrect.

securedrop_client/storage.py Outdated Show resolved Hide resolved
@rocodes rocodes force-pushed the 1429-delete-conversation branch 2 times, most recently from 90c6216 to befd8b9 Compare February 28, 2022 17:26
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.

Since this is a draft PR, I'm going to provide my partial review. I still need to step through the test plan, but as far as the solution goes, it looks good to me, and the code is clean and easy to follow. Tracking the source UUID instead of the conversation item UUIDs seems to work well the way it's been implemented. I can see how, similar to DeletedConversation, you'll be able to use similar logic for speeding up source account deletion.

securedrop_client/storage.py Outdated Show resolved Hide resolved
securedrop_client/storage.py Outdated Show resolved Hide resolved
securedrop_client/storage.py Outdated Show resolved Hide resolved
securedrop_client/storage.py Outdated Show resolved Hide resolved
securedrop_client/storage.py Outdated Show resolved Hide resolved
securedrop_client/storage.py Outdated Show resolved Hide resolved
securedrop_client/storage.py Outdated Show resolved Hide resolved
securedrop_client/storage.py Outdated Show resolved Hide resolved
securedrop_client/storage.py Show resolved Hide resolved
securedrop_client/storage.py Outdated Show resolved Hide resolved
@rocodes rocodes marked this pull request as ready for review March 7, 2022 15:54
@rocodes rocodes requested a review from a team as a code owner March 7, 2022 15:54
@rocodes rocodes added this to Ready for Review in SecureDrop Team Board Mar 7, 2022
@rocodes rocodes moved this from Ready for Review to In Development in SecureDrop Team Board Mar 7, 2022
@rocodes
Copy link
Contributor Author

rocodes commented Mar 7, 2022

Blocked on #1433

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.

Since this is no longer blocked (I rebased locally on main for the test plan), I went ahead and gave it another review. The changes look great and everything worked so smoothly. The test plan still says that it's a WIP, so if you're planning to add more tests let me know and I can give it another spint tomorrow.

Test Plan

(WIP)

Test local deletion: happy path

  • Log into the client and select a source with replies, messages, and files. Download some files and observe them in a subdirectory of ~/.securedrop_client/data/
  • Safe-delete ("delete files and messages") for the source.
  • Inspect the ~/.securedrop_client/data/ directory; the source's subdirectory should be removed and all relevant files deleted.
  • Inspect the sources table and note the source's id (not UUID). Query the messages, replies, draftreplies, and files tables for rows with the source id and ensure that no matching rows are present.

Test local deletion with network failures

  • Log into the client and select a source with replies, messages, and files. Download some files and observe them in a subdirectory of ~/.securedrop_client/data/
  • Open the Qubes Settings -> network settings for sd-whonix, or open a terminal in sd-whonix.
  • Safe-delete ("delete files and messages") for the source, then as soon as the UI confirms deletion, quickly kill the network connection by setting the nevtm to none or issuing sudo service tor stop in the sd-whonix terminal.
  • Inspect the ~/.securedrop_client/data/ directory; the source's subdirectory should be removed and all relevant files deleted.
  • Inspect the sources table and note the source's id and UUID. Query the messages, replies, draftreplies, and files tables for rows with the source id and ensure that no matching rows are present.
  • Inspect the deletedconversation table and ensure that a row with the source uuid is present.
  • Re-connect the network and wait for sync.
  • Again, query the messages, replies, draftreplies, and files tables for rows with the source id and ensure that no matching rows are present.
  • Ensure the entry with source UUID has been removed from the deletedconversation table.

Ensure no regression when deletion is done on the server

  • Download files for a source with files and messages.
  • Delete the source's files and messages via the Journalist Interface.
  • Wait for sync.
  • Ensure files and messages have been deleted from the client as in the steps above.

Test upgrade of existing client

  • Ensure database upgrade works and local deletion works as above

…source records. (Skip source for one sync).

Add local delete of files, messages, and replies for source by uuid. Add error-handling in case of race condition between sync-delete and local delete.
@rocodes
Copy link
Contributor Author

rocodes commented Mar 8, 2022

Thanks @creviera! I think the test plan covers everything, so I've removed the WIP. Such dangerous words! Going to update the test plan to include the "ghost reply" issue and work on a fix tomorrow, good find @creviera.

@rocodes
Copy link
Contributor Author

rocodes commented Mar 14, 2022

@creviera thank you for the work in #834, just helped resolve those last few UI issues! Unfortunately the commit signatures didn't make it into this branch. If you'd like me to sign those last two on your behalf, I can, or if you want to sign and force-push that works too.

@rocodes rocodes moved this from In Development to Ready for Review in SecureDrop Team Board Mar 14, 2022
Allie Crevier added 2 commits March 14, 2022 15:24
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, works great, and once you apply the same high-level solution for source account deletion, we can do some code clean up around sync_started_timestamp and deletion_scheduled_timestamp and check for any remaining edge cases after both changes are applied.

The GUI

Test Plan

Test local deletion: happy path

  • Log into the client and select a source with replies, messages, and files. Download some files and observe them in a subdirectory of ~/.securedrop_client/data/
  • Safe-delete ("delete files and messages") for the source.
  • Inspect the ~/.securedrop_client/data/ directory; the source's subdirectory should be removed and all relevant files deleted.
  • Inspect the sources table and note the source's id (not UUID). Query the messages, replies, draftreplies, and files tables for rows with the source id and ensure that no matching rows are present.

Test local deletion with network failures

  • Log into the client and select a source with replies, messages, and files. Download some files and observe them in a subdirectory of ~/.securedrop_client/data/
  • Open the Qubes Settings -> network settings for sd-whonix, or open a terminal in sd-whonix.
  • Safe-delete ("delete files and messages") for the source, then as soon as the UI confirms deletion, quickly kill the network connection by setting the nevtm to none or issuing sudo service tor stop in the sd-whonix terminal.
  • Inspect the ~/.securedrop_client/data/ directory; the source's subdirectory should be removed and all relevant files deleted.
  • Inspect the sources table and note the source's id and UUID. Query the messages, replies, draftreplies, and files tables for rows with the source id and ensure that no matching rows are present.
  • Inspect the deletedconversation table and ensure that a row with the source uuid is present.
  • Re-connect the network and wait for sync.
  • Again, query the messages, replies, draftreplies, and files tables for rows with the source id and ensure that no matching rows are present.
  • Ensure the entry with source UUID has been removed from the deletedconversation table.

Ensure no regression when deletion is done on the server

  • Download files for a source with files and messages.
  • Delete the source's files and messages via the Journalist Interface.
  • Wait for sync.
  • Ensure files and messages have been deleted from the client as in the steps above.

Test upgrade of existing client

  • Ensure database upgrade works and local deletion works as above

(Added) No ghost replies

  • Safe delete a conversation, then send reply A, then safe delete again.
  • Send reply B
  • Ensure reply A is not temporarily visible in the GUI (note: this is a GUI bug; it's not added to the database)

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.

meant to select "approve"

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.

User notification on deletion successful is triggered too early for conversation deletion
2 participants