Soft-delete users #4376

segiddins · 2024-01-22T22:25:01Z

So we can retain a record of actions taken by users even if they are deleted, for security auditing purposes

Part of #4360

Additional changes to avoid issues with the migrations & seeds by avoiding loading model classes before the migrations have finished

codecov · 2024-01-22T22:32:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b9b0edf) 96.97% compared to head (5ccbf1b) 96.99%.
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4376      +/-   ##
==========================================
+ Coverage   96.97%   96.99%   +0.01%     
==========================================
  Files         336      336              
  Lines        7477     7514      +37     
==========================================
+ Hits         7251     7288      +37     
  Misses        226      226

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

indirect

This looks good! I'm a little bit worried about the possible security issue of a future feature where the developer doesn't know they have to use User.active, and just writes User.find(id).do_important_thing!. Is there some way we can communicate to future contributors (and future ourselves) that is needed?

Ideas that come to mind for me are:

a default scope of deleted_at IS NULL so you can't accidentally find deleted users
a default scope with a syntax error in it, so you are forced to choose active or inactive anytime you query the users table
some sort of rubocop rule, that rejects queries without a scope for deletion

Open to other ideas!

simi · 2024-01-22T23:14:32Z

@indirect what about some kind of inheritance to make User behave as is until this PR? It would potentially help also with distributing methods in this long model across multiple places. Since for example login related code could be part of "active" User model only and vice versa. There is already User::WithPrivateFields model introducing this pattern. It would be just tricky to find out good name for "raw" user without this default scope.

class User < Raw::User
  default_scope ... # with deleted_at IS NULL
end

db/schema.rb

simi · 2024-01-22T23:21:22Z

@segiddins was there any blocker to use gem like https://github.com/jhawthorn/discard?

indirect · 2024-01-23T01:05:29Z

30c239b fully addresses my worry, if that works for the rest of you.

@simi I think in this case the diff is so small I would prefer this over a gem? but I guess I will let @segiddins answer with his own reasoning.

segiddins · 2024-01-23T02:26:12Z

I think not using a gem here is clearer, as we can control which associations get their dependent action executed

martinemde

I support this change. I left some comments one a few particular pieces of code but nothing should block merge if you feel otherwise.

martinemde · 2024-01-23T18:13:28Z

app/jobs/delete_user_job.rb

+        user.class.reflect_on_all_associations.each do |association|
+          next if association.name == :api_keys
+          case association.options[:dependent]
+          when :destroy, :destroy_async
+            user.association(association.name).handle_dependency
+          end
+        end


I find it a little strange to manage the associations ourselves like this instead of explicitly deleting what should be deleted here.

An argument in favor of the approach in this PR is that you could delete a user for real and it would cascade the delete correctly because you have all the dependent options set. But, do we ever do that?

I'm wondering if leaving the path open for a hard delete makes this unnecessarily complex. Reaching into active record reflections and handling it ourselves doesn't seem very forward compatible, and it seems to split the logic about what is actually happening here.

let me know if you prefer this?

db/migrate/20090821044418_add_scope_to_dependencies.rb

test/integration/sign_in_test.rb

martinemde · 2024-01-24T05:27:00Z

app/jobs/delete_user_job.rb

+        user.update!(
+          deleted_at: Time.current, email: "deleted+#{user.id}@rubygems.org",
+          handle: nil, email_confirmed: false,
+          unconfirmed_email: nil, blocked_email: nil,
+          api_key: nil, confirmation_token: nil, remember_token: nil,
+          twitter_username: nil, webauthn_id: nil, full_name: nil,
+          totp_seed: nil, mfa_hashed_recovery_codes: nil,
+          mfa_level: :disabled,
+          password: SecureRandom.hex(20).encode("UTF-8")
+        )


I like clearing the user. This removes any potential personal data while keeping whatever important associations we need to preserve.

simi · 2024-01-25T06:26:47Z

app/jobs/delete_user_job.rb

+    user.class.reflect_on_all_associations.each do |reflection|
+      next if reflection.through_reflection?
+
+      action = ASSOCIATIONS.fetch(reflection.name)


What's the benefit of using these AR internals instead of calling each relation and call destroy_all/destroy one by one? There are entries in ASSOCIATIONS with nil value, those are skipped if I understand it well. What's point of keeping those in that list? Wouldn't be enough to just do explicit calls like following?

user.approved_ownership_requests.destroy_all # ... webauthn_verification.destroy

Well put. This is my preference also.

The benefit is making sure we make a decision about each association, so when new associations are added they will be handled explicitly

I don't think it's a given that the dependent options (which protect foreign keys constraints) should trigger a hard delete on user soft delete.

simi · 2024-01-25T06:27:49Z

app/jobs/delete_user_job.rb

+    oidc_api_key_roles: nil,
+    pushed_versions: nil
+  }.freeze
+
  def perform(user:)


I would expect this logic to be part of model itself and in background job just got called something like user.soft_delete. Btw. would it make sense to rename this job to SoftDeleteUserJob?

I agree, this solves the concern about a split canonical information about deletes.

simi · 2024-01-25T06:28:32Z

app/jobs/delete_user_job.rb

+  end
+
+  def destroy_ActiveRecord__Reflection__HasOneReflection(association, _reflection) # rubocop:disable Naming/MethodName
+    association.delete


Why delete here instead of destroy? 🤔

that's how HasOneReflection#handle_dependency works

simi · 2024-01-25T06:32:21Z

I think not using a gem here is clearer, as we can control which associations get their dependent action executed

@segiddins Discard gem doesn't handle associations, but provides "discard" callback you can handle your strategy on your own in model. That seems friendlier to developer than separate logic in job class (easy to miss). By doing so, it would be enough to create DiscardUserJob with user.discard body.

simi · 2024-01-25T06:47:49Z

Ad migrations: if they doesn't work anymore as they are, I can squash them together.

So we can retain a record of actions taken by users even if they are deleted, for security auditing purposes

segiddins · 2024-01-27T00:54:43Z

@simi @martinemde is this more what you were envisioning?

martinemde

Nice. I really like this now. The discard gem reduced boilerplate too. Thanks for letting me pick on this PR that was already approved 🙇

app/models/user.rb

martinemde · 2024-01-27T01:01:25Z

app/models/user.rb

+  def destroy_associations_for_discard
+    ownerships.unscope(where: :confirmed_at).destroy_all
+    ownership_requests.update_all(status: :closed)
+    ownership_calls.unscope(where: :status).destroy_all
+    oidc_pending_trusted_publishers.destroy_all
+    subscriptions.destroy_all
+    web_hooks.destroy_all
+    webauthn_credentials.destroy_all
+    webauthn_verification&.destroy!
+  end


This is way better in my opinion. Absolutely easier to understand what will happen.

simi

Thanks for your patience with my review @segiddins. This looks fantastic now!

simi · 2024-01-27T16:49:12Z

app/models/user.rb

+  def destroy_associations_for_discard
+    ownerships.unscope(where: :confirmed_at).destroy_all
+    ownership_requests.update_all(status: :closed)
+    ownership_calls.unscope(where: :status).destroy_all
+    oidc_pending_trusted_publishers.destroy_all
+    subscriptions.destroy_all
+    web_hooks.destroy_all
+    webauthn_credentials.destroy_all
+    webauthn_verification&.destroy!
+  end


segiddins force-pushed the segiddins/soft-delete-users branch from ed737f5 to c7af718 Compare January 22, 2024 22:58

indirect approved these changes Jan 22, 2024

View reviewed changes

simi reviewed Jan 22, 2024

View reviewed changes

db/schema.rb Outdated Show resolved Hide resolved

segiddins force-pushed the segiddins/soft-delete-users branch from c7af718 to 30c239b Compare January 23, 2024 00:45

segiddins force-pushed the segiddins/soft-delete-users branch from 30c239b to add3dff Compare January 23, 2024 02:19

segiddins marked this pull request as ready for review January 23, 2024 02:28

martinemde approved these changes Jan 23, 2024

View reviewed changes

segiddins force-pushed the segiddins/soft-delete-users branch from add3dff to e504ded Compare January 24, 2024 01:35

martinemde reviewed Jan 24, 2024

View reviewed changes

segiddins force-pushed the segiddins/soft-delete-users branch from 18c3d90 to 71c824f Compare January 25, 2024 01:00

simi reviewed Jan 25, 2024

View reviewed changes

simi mentioned this pull request Jan 25, 2024

Stop deleting pusher_id on versions when the parent user is deleted #3766

Closed

segiddins added 8 commits January 26, 2024 16:52

Soft-delete users

8e1463d

So we can retain a record of actions taken by users even if they are deleted, for security auditing purposes

Use a default scope for not_deleted users

7811892

Remove class references from migrations

f7a3554

Refer to avo polymorphic types by class name

ff9a57a

Avoid loading Rubygem when loading Version

531f7d7

Fix seed url

d07e105

Rewrite deleting again

fcdd533

Remove dead code

5611221

segiddins force-pushed the segiddins/soft-delete-users branch from 71c824f to d9c430a Compare January 27, 2024 00:53

Use discard for soft-deleting users

34ac8aa

segiddins force-pushed the segiddins/soft-delete-users branch from d9c430a to 34ac8aa Compare January 27, 2024 00:57

martinemde approved these changes Jan 27, 2024

View reviewed changes

Flip callback ordering

5ccbf1b

martinemde approved these changes Jan 27, 2024

View reviewed changes

simi approved these changes Jan 27, 2024

View reviewed changes

segiddins merged commit 91fc748 into master Jan 27, 2024
16 checks passed

segiddins deleted the segiddins/soft-delete-users branch January 27, 2024 21:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Soft-delete users #4376

Soft-delete users #4376

segiddins commented Jan 22, 2024 •

edited

codecov bot commented Jan 22, 2024 •

edited

indirect left a comment

simi commented Jan 22, 2024

simi commented Jan 22, 2024

indirect commented Jan 23, 2024

segiddins commented Jan 23, 2024

martinemde left a comment

martinemde Jan 23, 2024

segiddins Jan 24, 2024

martinemde Jan 24, 2024

simi Jan 25, 2024

martinemde Jan 25, 2024

segiddins Jan 26, 2024

martinemde Jan 26, 2024

simi Jan 25, 2024

martinemde Jan 25, 2024

simi Jan 25, 2024

segiddins Jan 25, 2024

simi commented Jan 25, 2024

simi commented Jan 25, 2024

segiddins commented Jan 27, 2024

martinemde left a comment •

edited

martinemde Jan 27, 2024

simi Jan 27, 2024

simi left a comment

simi Jan 27, 2024

Soft-delete users #4376

Soft-delete users #4376

Conversation

segiddins commented Jan 22, 2024 • edited

codecov bot commented Jan 22, 2024 • edited

Codecov Report

indirect left a comment

Choose a reason for hiding this comment

simi commented Jan 22, 2024

simi commented Jan 22, 2024

indirect commented Jan 23, 2024

segiddins commented Jan 23, 2024

martinemde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simi commented Jan 25, 2024

simi commented Jan 25, 2024

segiddins commented Jan 27, 2024

martinemde left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

segiddins commented Jan 22, 2024 •

edited

codecov bot commented Jan 22, 2024 •

edited

martinemde left a comment •

edited