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

Soft-delete users #4376

Merged
merged 10 commits into from
Jan 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions Gemfile
Expand Up @@ -54,6 +54,7 @@ gem "bcrypt", "~> 3.1"
gem "maintenance_tasks", "~> 2.4"
gem "strong_migrations", "~> 1.7"
gem "phlex-rails", "~> 1.1"
gem "discard", "~> 1.3"

# Admin dashboard
gem "avo", "~> 2.47"
Expand Down
4 changes: 4 additions & 0 deletions Gemfile.lock
Expand Up @@ -209,6 +209,8 @@ GEM
rake (> 10, < 14)
ruby-statistics (>= 2.1)
thor (>= 0.19, < 2)
discard (1.3.0)
activerecord (>= 4.2, < 8)
docile (1.4.0)
dogstatsd-ruby (5.6.1)
domain_name (0.6.20231109)
Expand Down Expand Up @@ -749,6 +751,7 @@ DEPENDENCIES
dartsass-sprockets (~> 3.1)
ddtrace (~> 1.19)
derailed_benchmarks (~> 2.1)
discard (~> 1.3)
dogstatsd-ruby (~> 5.5)
dotenv-rails (~> 2.8)
factory_bot_rails (~> 6.4)
Expand Down Expand Up @@ -898,6 +901,7 @@ CHECKSUMS
dead_end (4.0.0) sha256=695c8438993bb4c5415b1618a1b6e0afcae849ef2812fb8cb3846723904307eb
debase-ruby_core_source (3.3.1) sha256=ed904cae290edf0cf274ad707f8981bf1cefad8081e78d4bb71be2a483bc2c08
derailed_benchmarks (2.1.2) sha256=eaadc6206ceeb5538ff8f5e04a0023d54ebdd95d04f33e8960fb95a5f189a14f
discard (1.3.0) sha256=55218997ca4dc11f0594f1bb2d1196c4a959ceb562f1ab6490130233598dda67
docile (1.4.0) sha256=5f1734bde23721245c20c3d723e76c104208e1aa01277a69901ce770f0ebb8d3
dogstatsd-ruby (5.6.1) sha256=615dd1328c57ab66fb48cbf83b38ce2060d8353707be0bc3deb0ae960a659982
domain_name (0.6.20231109) sha256=5127a1521ecca79d54accefc393f6d19db8600c6224416004414f7eaa28aecbe
Expand Down
2 changes: 1 addition & 1 deletion app/avo/resources/api_key_resource.rb
Expand Up @@ -12,7 +12,7 @@ class ExpiredFilter < ScopeBooleanFilter; end
field :user, as: :belongs_to, visible: ->(_) { false }
field :owner, as: :belongs_to,
polymorphic_as: :owner,
types: [::User, ::OIDC::TrustedPublisher::GitHubAction]
types: ["User", "OIDC::TrustedPublisher::GitHubAction"]
field :last_accessed_at, as: :date_time
field :soft_deleted_at, as: :date_time
field :soft_deleted_rubygem_name, as: :text
Expand Down
2 changes: 1 addition & 1 deletion app/avo/resources/audit_resource.rb
Expand Up @@ -14,7 +14,7 @@ class AuditResource < Avo::BaseResource

field :auditable, as: :belongs_to,
polymorphic_as: :auditable,
types: [::User, ::WebHook],
types: %w[User WebHook],
name: "Edited Record"

heading "Action Details"
Expand Down
2 changes: 1 addition & 1 deletion app/avo/resources/link_verification_resource.rb
Expand Up @@ -9,7 +9,7 @@ class LinkVerificationResource < Avo::BaseResource
# Fields generated from the model
field :linkable, as: :belongs_to,
polymorphic_as: :linkable,
types: [::Rubygem]
types: %w[Rubygem]
field :uri, as: :text
field :verified?, as: :boolean
field :last_verified_at, as: :date_time
Expand Down
6 changes: 6 additions & 0 deletions app/avo/resources/user_resource.rb
Expand Up @@ -4,6 +4,10 @@ class UserResource < Avo::BaseResource
self.search_query = lambda {
scope.where("email LIKE ? OR handle LIKE ?", "%#{params[:q]}%", "%#{params[:q]}%")
}
self.unscoped_queries_on_index = true

class DeletedFilter < ScopeBooleanFilter; end
filter DeletedFilter, arguments: { default: { not_deleted: true, deleted: false } }

action BlockUser
action CreateUser
Expand Down Expand Up @@ -34,6 +38,8 @@ class UserResource < Avo::BaseResource
field :mail_fails, as: :number
field :blocked_email, as: :text

field :deleted_at, as: :date_time

tabs style: :pills do
tab "Auth" do
field :encrypted_password, as: :password, visible: ->(_) { false }
Expand Down
6 changes: 6 additions & 0 deletions app/controllers/concerns/avo_auditable.rb
Expand Up @@ -3,6 +3,8 @@ module AvoAuditable

prepended do
include Auditable

prepend_around_action :unscope_users
end

def perform_action_and_record_errors(&blk)
Expand Down Expand Up @@ -38,4 +40,8 @@ def after_create_path

super
end

def unscope_users(&)
User.unscoped(&)
end
end
20 changes: 0 additions & 20 deletions app/jobs/delete_user.rb

This file was deleted.

9 changes: 4 additions & 5 deletions app/jobs/delete_user_job.rb
Expand Up @@ -4,13 +4,12 @@ class DeleteUserJob < ApplicationJob

def perform(user:)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

email = user.email
user.destroy!
rescue ActiveRecord::RecordNotDestroyed, ActiveRecord::NotNullViolation, ActiveRecord::DeleteRestrictionError => e
return if user.discarded?
user.discard!
rescue ActiveRecord::ActiveRecordError, Discard::RecordNotDiscarded => e
# Catch the exception so we can log it, otherwise using `destroy` would give
# us no hint as to why the deletion failed.
Rails.error.report(e, context: { user:, email: }, handled: true)
Rails.error.report(e, context: { user: user.as_json, email: }, handled: true)
Mailer.deletion_failed(email).deliver_later
else
Mailer.deletion_complete(email).deliver_later
end
end
13 changes: 0 additions & 13 deletions app/jobs/email_confirmation_mailer.rb

This file was deleted.

9 changes: 0 additions & 9 deletions app/jobs/email_reset_mailer.rb

This file was deleted.

51 changes: 51 additions & 0 deletions app/models/user.rb
@@ -1,13 +1,28 @@
class User < ApplicationRecord
include UserMultifactorMethods
include Clearance::User

include Gravtastic
is_gravtastic default: "retro"

include Discard::Model
self.discard_column = :deleted_at

default_scope { not_deleted }

before_save :_generate_confirmation_token_no_reset_unconfirmed_email, if: :will_save_change_to_unconfirmed_email?
before_create :_generate_confirmation_token_no_reset_unconfirmed_email
before_discard :yank_gems
before_discard :expire_all_api_keys
before_discard :destroy_associations_for_discard
before_discard :clear_personal_attributes
after_discard :send_deletion_complete_email
before_destroy :yank_gems

scope :not_deleted, -> { kept }
scope :deleted, -> { with_discarded.discarded }
scope :with_deleted, -> { with_discarded }

has_many :ownerships, -> { confirmed }, dependent: :destroy, inverse_of: :user

has_many :rubygems, through: :ownerships, source: :rubygem
Expand All @@ -25,7 +40,10 @@ class User < ApplicationRecord
has_many :api_keys, dependent: :destroy, inverse_of: :owner, as: :owner

has_many :ownership_calls, -> { opened }, dependent: :destroy, inverse_of: :user
has_many :closed_ownership_calls, -> { closed }, dependent: :destroy, inverse_of: :user, class_name: "OwnershipCall"
has_many :ownership_requests, -> { opened }, dependent: :destroy, inverse_of: :user
has_many :closed_ownership_requests, -> { closed }, dependent: :destroy, inverse_of: :user, class_name: "OwnershipRequest"
has_many :approved_ownership_requests, -> { approved }, dependent: :destroy, inverse_of: :user, class_name: "OwnershipRequest"

has_many :audits, as: :auditable, dependent: :nullify

Expand Down Expand Up @@ -280,4 +298,37 @@ def toxic_email_domain

errors.add(:email, I18n.t("activerecord.errors.messages.blocked", domain: domain)) if toxic
end

def expire_all_api_keys
api_keys.unexpired.expire_all!
end

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
Comment on lines +306 to +315
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

👍


def clear_personal_attributes
@email_before_discard = email
update!(
email: "deleted+#{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")
)
end

def send_deletion_complete_email
Mailer.deletion_complete(@email_before_discard).deliver_later
end
end
6 changes: 3 additions & 3 deletions app/models/version.rb
Expand Up @@ -7,7 +7,7 @@ class Version < ApplicationRecord # rubocop:disable Metrics/ClassLength
has_many :dependencies, -> { order("rubygems.name ASC").includes(:rubygem) }, dependent: :destroy, inverse_of: "version"
has_many :audits, as: :auditable, inverse_of: :auditable, dependent: :nullify
has_one :gem_download, inverse_of: :version, dependent: :destroy
belongs_to :pusher, class_name: "User", inverse_of: false, optional: true
belongs_to :pusher, class_name: "User", inverse_of: :pushed_versions, optional: true
belongs_to :pusher_api_key, class_name: "ApiKey", inverse_of: :pushed_versions, optional: true
has_one :deletion, dependent: :delete, inverse_of: :version, required: false
has_one :yanker, through: :deletion, source: :user, inverse_of: :yanked_versions, required: false
Expand All @@ -27,8 +27,8 @@ class Version < ApplicationRecord # rubocop:disable Metrics/ClassLength
serialize :cert_chain, coder: CertificateChainSerializer

validates :number, length: { maximum: Gemcutter::MAX_FIELD_LENGTH }, format: { with: Patterns::VERSION_PATTERN }
validates :platform, length: { maximum: Gemcutter::MAX_FIELD_LENGTH }, format: { with: Rubygem::NAME_PATTERN }
validates :gem_platform, length: { maximum: Gemcutter::MAX_FIELD_LENGTH }, format: { with: Rubygem::NAME_PATTERN },
validates :platform, length: { maximum: Gemcutter::MAX_FIELD_LENGTH }, format: { with: Patterns::NAME_PATTERN }
validates :gem_platform, length: { maximum: Gemcutter::MAX_FIELD_LENGTH }, format: { with: Patterns::NAME_PATTERN },
if: -> { validation_context == :create || gem_platform_changed? }
validates :full_name, presence: true, uniqueness: { case_sensitive: false },
if: -> { validation_context == :create || full_name_changed? }
Expand Down
4 changes: 2 additions & 2 deletions db/migrate/20090821044418_add_scope_to_dependencies.rb
@@ -1,8 +1,8 @@
class AddScopeToDependencies < ActiveRecord::Migration[4.2]
def self.up
add_column :dependencies, :scope, :string
Dependency.update_all(scope: "runtime")
announce "Please reprocess all gems after this migration"
# Dependency.update_all(scope: "runtime")
# announce "Please reprocess all gems after this migration"
segiddins marked this conversation as resolved.
Show resolved Hide resolved
end

def self.down
Expand Down
2 changes: 1 addition & 1 deletion db/migrate/20090826035230_fix_version_dates.rb
Expand Up @@ -2,7 +2,7 @@ class FixVersionDates < ActiveRecord::Migration[4.2]
def self.up
rename_column :versions, :created_at, :built_at
add_column :versions, :created_at, :datetime
Version.update_all("created_at = updated_at")
# Version.update_all("created_at = updated_at")
end

def self.down
Expand Down
2 changes: 1 addition & 1 deletion db/migrate/20090917035818_add_prerelease_to_versions.rb
@@ -1,7 +1,7 @@
class AddPrereleaseToVersions < ActiveRecord::Migration[4.2]
def self.up
add_column :versions, :prerelease, :boolean
Version.update_all(prerelease: false)
# Version.update_all(prerelease: false)
end

def self.down
Expand Down
2 changes: 1 addition & 1 deletion db/migrate/20090930181320_add_position_to_versions.rb
Expand Up @@ -2,7 +2,7 @@ class AddPositionToVersions < ActiveRecord::Migration[4.2]
def self.up
add_column :versions, :position, :integer

Rubygem.find_each(&:reorder_versions)
# Rubygem.find_each(&:reorder_versions)
end

def self.down
Expand Down
2 changes: 1 addition & 1 deletion db/migrate/20091009213456_reorder_versions_again.rb
@@ -1,6 +1,6 @@
class ReorderVersionsAgain < ActiveRecord::Migration[4.2]
def self.up
Rubygem.find_each(&:reorder_versions)
# Rubygem.find_each(&:reorder_versions)
end

def self.down
Expand Down
2 changes: 1 addition & 1 deletion db/migrate/20091021203534_add_latest_to_version.rb
Expand Up @@ -2,7 +2,7 @@ class AddLatestToVersion < ActiveRecord::Migration[4.2]
def self.up
add_column :versions, :latest, :boolean

Rubygem.find_each(&:reorder_versions)
# Rubygem.find_each(&:reorder_versions)
end

def self.down
Expand Down
26 changes: 13 additions & 13 deletions db/migrate/20091026234707_fix_dependencies.rb
@@ -1,20 +1,20 @@
class FixDependencies < ActiveRecord::Migration[4.2]
def self.up
# fix bad version reqs
Dependency.find_each do |dep|
reqs = dep.requirements
begin
Gem::Requirement.new(reqs)
rescue ArgumentError
list = reqs.split(/(>=)|(<=)|(~>)|(>)|(<)|(=)/).reject(&:empty?)
fixed = "#{list[0]}#{list[1]}, #{list[2]}#{list[3]}"
# # fix bad version reqs
# Dependency.find_each do |dep|
# reqs = dep.requirements
# begin
# Gem::Requirement.new(reqs)
# rescue ArgumentError
# list = reqs.split(/(>=)|(<=)|(~>)|(>)|(<)|(=)/).reject(&:empty?)
# fixed = "#{list[0]}#{list[1]}, #{list[2]}#{list[3]}"

dep.update_attribute(:requirements, fixed)
end
end
# dep.update_attribute(:requirements, fixed)
# end
# end

# kill bad deps too
Dependency.includes(:rubygem).where(rubygems: { id: nil }).destroy_all
# # kill bad deps too
# Dependency.includes(:rubygem).where(rubygems: { id: nil }).destroy_all
end

def self.down
Expand Down
@@ -1,9 +1,9 @@
class RemoveImproperlyEmbeddedYamlData < ActiveRecord::Migration[4.2]
def self.up
Dependency.where("requirements like '%YAML::Syck::DefaultKey%'").find_each do |d|
d.requirements = d.clean_requirements
d.save(validate: false)
end
# Dependency.where("requirements like '%YAML::Syck::DefaultKey%'").find_each do |d|
# d.requirements = d.clean_requirements
# d.save(validate: false)
# end
end

def self.down
Expand Down
6 changes: 3 additions & 3 deletions db/migrate/20200508234114_add_indexed_to_rubygems.rb
Expand Up @@ -3,8 +3,8 @@ def change
add_column :rubygems, :indexed, :boolean, null: false, default: false
add_index :rubygems, :indexed

say_with_time "populating indexed rubygems table column" do
Rubygem.joins(:versions).where(versions: { indexed: true }).update_all(indexed: true)
end
# say_with_time "populating indexed rubygems table column" do
# Rubygem.joins(:versions).where(versions: { indexed: true }).update_all(indexed: true)
# end
end
end
2 changes: 1 addition & 1 deletion db/migrate/20220609221942_remove_orphaned_owners.rb
@@ -1,6 +1,6 @@
class RemoveOrphanedOwners < ActiveRecord::Migration[7.0]
def up
Ownership.where.missing(:user).destroy_all
# Ownership.where.missing(:user).destroy_all
end

def down
Expand Down
2 changes: 1 addition & 1 deletion db/migrate/20230702010946_create_gem_name_reservations.rb
Expand Up @@ -65,7 +65,7 @@ def change

reversible do |change|
change.up do
GemNameReservation.insert_all(ORIGINAL_GEM_NAME_RESERVED_LIST.map { |name| { name: name } })
# GemNameReservation.insert_all(ORIGINAL_GEM_NAME_RESERVED_LIST.map { |name| { name: name } })
end
end
end
Expand Down
5 changes: 5 additions & 0 deletions db/migrate/20240120010424_add_deleted_at_to_user.rb
@@ -0,0 +1,5 @@
class AddDeletedAtToUser < ActiveRecord::Migration[7.1]
def change
add_column :users, :deleted_at, :datetime
end
end