Add SecureDeletionService with crypto-shredding for patient PII#3537
Open
Rolling2405 wants to merge 8 commits intoDARIAEngineering:mainfrom
Open
Add SecureDeletionService with crypto-shredding for patient PII#3537Rolling2405 wants to merge 8 commits intoDARIAEngineering:mainfrom
Rolling2405 wants to merge 8 commits intoDARIAEngineering:mainfrom
Conversation
Implements adaptive secure deletion per NIST SP 800-88 guidelines:
Level 5 (Heroku/managed DB):
1. Crypto-shred: Overwrite 9 PII fields with random data (name, phones,
address, contacts)
2. Shred associated note text (encrypted field overwritten)
3. SQL DELETE via destroy!
4. VACUUM to reclaim disk pages
Level 6 (Self-hosted, auto-detected via absence of DYNO env var):
All of Level 5 plus WAL CHECKPOINT to flush write-ahead log
The service replaces plain destroy! in ArchivedPatient.archive_eligible_patients!
so that PII overwrite happens before row deletion, preventing recovery from
DB page reuse, WAL segments, or point-in-time backups.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Move VACUUM/CHECKPOINT to run after outer transaction commits using after_transaction_commit callback when nested inside another transaction (e.g., archive_eligible_patients!) - Scope VACUUM to patients table only instead of full database VACUUM - Add comprehensive SecureDeletionService tests: - Patient record deletion - PII field overwriting before deletion - Associated notes shredding - Patients with no notes handled gracefully - Patients with multiple notes - Atomicity (transaction rollback on failure) - VACUUM failure handling (no raise) - Tenant isolation - PII field completeness Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Test VACUUM on patients table specifically - Test graceful handling of nested transactions - Test self-hosted vs managed environment detection Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Explicitly destroy all polymorphic associations (notes, calls, external_pledges, practical_supports, fulfillment) before patient deletion since they lack dependent: :destroy - Scrub PaperTrail version history for the patient - Simplify VACUUM scheduling (no more after_transaction_commit) - Add tests for association cleanup and PaperTrail scrubbing Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ords Previously only scrubbed versions for Patient records, leaving audit history for Notes, Calls, ExternalPledges, PracticalSupports, and Fulfillments intact. This is a data leak since those versions can contain sensitive patient information. Changes: - scrub_paper_trail_versions! now deletes versions for all associated record types before deleting patient versions - Uses delete_all instead of destroy_all for efficiency (no callbacks needed on PaperTrailVersion) - Extracted scrub_versions_for helper to DRY the per-type deletion - Fixed misleading comment about after_commit VACUUM scheduling - Added test for associated record version scrubbing Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Three issues fixed: 1. ORDERING BUG: scrub_paper_trail_versions! was called AFTER destroy_associations!, meaning associated record IDs were already gone. Now captures all IDs via capture_associated_ids BEFORE destruction, then uses cached IDs for version scrubbing. 2. MISSING RECORDS: PracticalSupport has_many :notes (polymorphic), which were not being shredded or version-scrubbed. Now handles notes attached to practical_supports in both shred and scrub steps. 3. WEAK TESTS: Expanded version scrubbing test to verify ALL associated record types (Note, Call, ExternalPledge, PracticalSupport) not just Note. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Update associated records inside with_versioning to ensure PaperTrail versions actually exist before asserting they're scrubbed - Add PracticalSupport-owned note (nested polymorphic) to version test - Add dedicated test for PS note shredding and destruction - All 6 tracked types now verified: Patient, Note (direct + PS-owned), Call, ExternalPledge, PracticalSupport Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add tests for: - Patient with Fulfillment records and their PaperTrail version scrubbing - Shredded PII values are actually random hex (not empty strings), unique per field - Idempotency: calling delete on already-deleted patient raises RecordNotFound - Service works correctly when PaperTrail is disabled (records still destroyed) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I rule and have completed some work on Case Manager that's ready for review!
This adds a secure deletion service that thoroughly overwrites patient data before deleting it, ensuring that sensitive records can't be recovered from the database after removal.
This pull request makes the following changes:
SecureDeletionServicewith 3-pass overwrite before deletionafter_commit) to reclaim storageFor reviewer:
featureif it contains a feature, fix, or similar. This is anything that contains a user-facing fix in some way, such as frontend changes, alterations to backend behavior, or bug fixes.dependenciesif it contains library upgrades or similar. This is anything that upgrades any dependency, such as a Gemfile update or npm package upgrade.