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

Hard delete auditing #24172

Merged
merged 8 commits into from Aug 10, 2018
Merged

Hard delete auditing #24172

merged 8 commits into from Aug 10, 2018

Conversation

islemaster
Copy link
Contributor

@islemaster islemaster commented Aug 8, 2018

Adds two types of log output to the hard-delete cronjob. See the "Auditing" section of the Hard-deleting accounts tech spec.

We're uploading to the cdo-audit-logs bucket which is versioned to make it harder to accidentally delete/overwrite anything.

Purged account logs

cdo-audit-logs/purged-users/<environment>/YYYY-MM-DD/<user-id>

Goals:

  • Allows us to re-delete user accounts should we need to restore a recent database backup.
  • Keep ids tied to external services so we can follow up on their hard-delete processes.
  • Gives us some ability to answer questions if someone comes to us with a complaint that their account is gone, and we can’t find it in our database.

We upload one of these logs for every account we purge. These are created and uploaded even when an engineer calls the AccountPurger directly, so we have records of manually-performed account purges too. The logs are a JSON blob, formatted something like this:

    {
      "user_id": 1234567,
      "hashed_email": "abcdef",
      "pardot_ids": [],
      "poste_contact_ids":[],
      "reason": "soft delete 28 days ago",
      "purged_at": "2018-08-08 15:23",
      "confirmed_at": null
    }

ExpiredDeletedAccountPurger activity logs

cdo-audit-logs/expired-deleted-account-purger-activity/<environment>/YYYYMMDDTHHMMSS-0700

Goals:

  • Verbose description of the automated delete job's activity to make it easier to diagnose issues with the automation.

We upload one of these each time the task runs (normally, nightly).

Starting purge_expired_deleted_accounts!
deleted_after: 2018-08-06 23:15:13 UTC
deleted_before: 2018-08-08 16:15:13 -0700
max_accounts_to_purge: 100
(dry-run)
Purging user_id 1 (dry-run)
Would have purged 1 accounts
Custom/DeletedAccountPurger/SoftDeletedAccounts: 1
Custom/DeletedAccountPurger/AccountsPurged: 1
Custom/DeletedAccountPurger/ManualReviewQueueDepth: 0
Done in 0.016986554 seconds

Eventually this should contain more detail generated by AccountPurger while it's purging the account.

@islemaster islemaster changed the base branch from review-queue to staging August 8, 2018 23:27
@islemaster islemaster changed the title [WIP] Hard delete auditing Hard delete auditing Aug 8, 2018
deleted_after: 4.days.ago,
deleted_before: 2.days.ago
ExpiredDeletedAccountPurger.any_instance.stubs(:upload_activity_log).at_least(0)
PurgedAccountLog.any_instance.stubs(:upload).at_least(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need the at_least(0) part with stubs, only with expects.

From http://gofreerange.com/mocha/docs/#Expectation_matching___invocation_order

Stubs and expectations are basically the same thing. A stub is just an expectation of zero or more invocations. The Expectation#stubs method is syntactic sugar to make the intent of the test more explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, awesome! Thank you for citing the docs - I've always had a hard time finding things in mocha docs, and looking for features that aren't there like sinon's callsFake. I guess I should read this main page in more detail!

@@ -186,6 +204,16 @@ def setup
assert_includes purged, student_b
assert_includes purged, student_c
refute_includes purged, student_d

assert_equal <<~LOG, edap.log.string
Copy link
Contributor

Choose a reason for hiding this comment

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

neat! I didn't realize heredocs start their content on the next line. TIL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's this weird shorthand that actually reads pretty nice.

@islemaster islemaster merged commit d016447 into staging Aug 10, 2018
@islemaster islemaster deleted the hard-delete-auditing branch August 10, 2018 00:46
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.

None yet

5 participants