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

Fix user delete when the user has pushed a gem with an associated API key #4130

Merged
merged 1 commit into from
Oct 12, 2023
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
13 changes: 8 additions & 5 deletions app/jobs/delete_user_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,13 @@ class DeleteUserJob < ApplicationJob

def perform(user:)
email = user.email
if user.destroy
Mailer.deletion_complete(email).deliver_later
else
Mailer.deletion_failed(email).deliver_later
end
user.destroy!
rescue ActiveRecord::RecordNotDestroyed, ActiveRecord::NotNullViolation, ActiveRecord::DeleteRestrictionError => 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)
Mailer.deletion_failed(email).deliver_later
else
Mailer.deletion_complete(email).deliver_later
end
end
2 changes: 1 addition & 1 deletion app/models/api_key.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ class ApiKey < ApplicationRecord
has_one :ownership, through: :api_key_rubygem_scope
has_one :oidc_id_token, class_name: "OIDC::IdToken", dependent: :restrict_with_error
has_one :oidc_api_key_role, through: :oidc_id_token, inverse_of: :api_key
has_many :pushed_versions, class_name: "Version", inverse_of: :pusher_api_key, foreign_key: :pusher_api_key_id, dependent: :restrict_with_error
has_many :pushed_versions, class_name: "Version", inverse_of: :pusher_api_key, foreign_key: :pusher_api_key_id, dependent: :nullify

validates :user, :name, :hashed_key, presence: true
validate :exclusive_show_dashboard_scope, if: :can_show_dashboard?
Expand Down
3 changes: 2 additions & 1 deletion app/models/gem_info.rb
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,8 @@ def compute_compact_index_info
end
end

CompactIndex::GemVersion.new(r[0], r[1], Version._sha256_hex(r[2]), r[3], deps, r[4], r[5])
name, platform, checksum, info_checksum, ruby_version, rubygems_version, = r
CompactIndex::GemVersion.new(name, platform, Version._sha256_hex(checksum), info_checksum, deps, ruby_version, rubygems_version)
end
end

Expand Down
2 changes: 1 addition & 1 deletion config/initializers/good_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,6 @@

ActiveRecord::Base.logger = nil
GoodJob.logger = Rails.logger
StatsD.backend = StatsD::Instrument::Backends::NullBackend.new
StatsD.singleton_client = StatsD::Instrument::Client.new
end
end
23 changes: 20 additions & 3 deletions test/jobs/delete_user_job_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ class DeleteUserJobTest < ActiveJob::TestCase
user = create(:user)
rubygem = create(:ownership, user:).rubygem
version = create(:version, rubygem:)
Mailer.expects(:deletion_complete).with(user.email)
Mailer.expects(:deletion_complete).with(user.email).returns(mock(deliver_later: nil))
DeleteUserJob.perform_now(user:)

assert_predicate user, :destroyed?
Expand All @@ -14,10 +14,27 @@ class DeleteUserJobTest < ActiveJob::TestCase

test "sends deletion failed on failure" do
user = create(:user)
user.expects(:destroy).returns(false)
Mailer.expects(:deletion_failed).with(user.email)
create(:oidc_id_token, user:)
Mailer.expects(:deletion_failed).with(user.email).returns(mock(deliver_later: nil))
DeleteUserJob.perform_now(user:)

refute_predicate user.reload, :destroyed?
end

test "succeeds with api key" do
user = create(:user)
create(:api_key, user:)
Mailer.expects(:deletion_complete).with(user.email).returns(mock(deliver_later: nil))

DeleteUserJob.perform_now(user:)
end

test "succeeds with api key used to push version" do
user = create(:user)
api_key = create(:api_key, user:)
create(:version, pusher_api_key: api_key, pusher: user)
Mailer.expects(:deletion_complete).with(user.email).returns(mock(deliver_later: nil))

DeleteUserJob.perform_now(user:)
end
end