diff --git a/.rubocop_todo/rspec/be_eq.yml b/.rubocop_todo/rspec/be_eq.yml index 3d060a09a27752282611ed2ae63cfc7148d6ee21..fd742987aa8fc7594c9c0388b970232235357f73 100644 --- a/.rubocop_todo/rspec/be_eq.yml +++ b/.rubocop_todo/rspec/be_eq.yml @@ -955,7 +955,6 @@ RSpec/BeEq: - 'spec/migrations/re_add_tags_name_unique_index_spec.rb' - 'spec/models/analytics/cycle_analytics/aggregation_spec.rb' - 'spec/models/anti_abuse/reports/note_spec.rb' - - 'spec/models/anti_abuse/user_trust_score_spec.rb' - 'spec/models/appearance_spec.rb' - 'spec/models/authentication_event_spec.rb' - 'spec/models/batched_git_ref_updates/deletion_spec.rb' diff --git a/.rubocop_todo/sidekiq/enforce_database_health_signal_deferral.yml b/.rubocop_todo/sidekiq/enforce_database_health_signal_deferral.yml index 74e8b783a25c40a509f189bd58daefa0345e4a02..1bcec37b532b0284092bd9fbf78158329e8545fe 100644 --- a/.rubocop_todo/sidekiq/enforce_database_health_signal_deferral.yml +++ b/.rubocop_todo/sidekiq/enforce_database_health_signal_deferral.yml @@ -5,8 +5,6 @@ Sidekiq/EnforceDatabaseHealthSignalDeferral: - 'app/workers/analytics/usage_trends/counter_job_worker.rb' - 'app/workers/anti_abuse/ban_duplicate_users_worker.rb' - 'app/workers/anti_abuse/spam_abuse_events_worker.rb' - - 'app/workers/anti_abuse/trust_score_cleanup_worker.rb' - - 'app/workers/anti_abuse/trust_score_worker.rb' - 'app/workers/authorized_project_update/enqueue_group_members_refresh_authorized_projects_worker.rb' - 'app/workers/authorized_project_update/periodic_recalculate_worker.rb' - 'app/workers/authorized_project_update/user_refresh_from_replica_worker.rb' diff --git a/app/models/anti_abuse/trust_score.rb b/app/models/anti_abuse/trust_score.rb deleted file mode 100644 index b9f7790194082e2ab6ab5563cd784bcdca08d7cd..0000000000000000000000000000000000000000 --- a/app/models/anti_abuse/trust_score.rb +++ /dev/null @@ -1,26 +0,0 @@ -# frozen_string_literal: true - -module AntiAbuse - class TrustScore < ApplicationRecord - self.table_name = 'abuse_trust_scores' - - enum :source, Enums::Abuse::Source.sources - - belongs_to :user - - validates :user, presence: true - validates :score, presence: true - validates :source, presence: true - - scope :order_created_at_asc, -> { order(created_at: :asc) } - scope :order_created_at_desc, -> { order(created_at: :desc) } - - before_create :assign_correlation_id - - private - - def assign_correlation_id - self.correlation_id_value ||= Labkit::Correlation::CorrelationId.current_id || '' - end - end -end diff --git a/app/models/user.rb b/app/models/user.rb index ccd7fc0402472a9d4444a3c2c1d183e9a677f5c3..f13a1e986cb7a3e1f7b6a42fda6ef6d1bda13bf2 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -248,10 +248,6 @@ def update_tracked_fields!(request) has_many :resolved_abuse_reports, foreign_key: :resolved_by_id, class_name: "AbuseReport", inverse_of: :resolved_by has_many :abuse_events, foreign_key: :user_id, class_name: 'AntiAbuse::Event', inverse_of: :user has_many :spam_logs, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - has_many :abuse_trust_scores, - class_name: 'AntiAbuse::TrustScore', - foreign_key: :user_id, - dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - required by https://gitlab.com/groups/gitlab-org/-/epics/19085 has_many :builds, class_name: 'Ci::Build' has_many :pipelines, class_name: 'Ci::Pipeline' diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 88cb09d9fa6c3ca75d3745e1b443a13445755f2d..989215b092fc0df5f995ce38a130b2feef1e4f33 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -3007,26 +3007,6 @@ :idempotent: true :tags: [] :queue_namespace: -- :name: anti_abuse_trust_score - :worker_name: AntiAbuse::TrustScoreWorker - :feature_category: :instance_resiliency - :has_external_dependencies: false - :urgency: :low - :resource_boundary: :unknown - :weight: 1 - :idempotent: true - :tags: [] - :queue_namespace: -- :name: anti_abuse_trust_score_cleanup - :worker_name: AntiAbuse::TrustScoreCleanupWorker - :feature_category: :instance_resiliency - :has_external_dependencies: false - :urgency: :low - :resource_boundary: :unknown - :weight: 1 - :idempotent: true - :tags: [] - :queue_namespace: - :name: approve_blocked_pending_approval_users :worker_name: ApproveBlockedPendingApprovalUsersWorker :feature_category: :user_profile diff --git a/app/workers/anti_abuse/trust_score_cleanup_worker.rb b/app/workers/anti_abuse/trust_score_cleanup_worker.rb deleted file mode 100644 index 6568fe03886b10ae5c9860c9d3f4c2037a49390c..0000000000000000000000000000000000000000 --- a/app/workers/anti_abuse/trust_score_cleanup_worker.rb +++ /dev/null @@ -1,25 +0,0 @@ -# frozen_string_literal: true - -module AntiAbuse - class TrustScoreCleanupWorker - include ApplicationWorker - - idempotent! - data_consistency :delayed - deduplicate :until_executed - feature_category :instance_resiliency - urgency :low - - def perform(user_id, source) - user = User.find_by_id(user_id) - return unless user - return if Feature.enabled?(:remove_trust_scores, user) - - cache_key = "abuse:trust_score_cleanup_worker:#{user.id}:#{source}" - return if Rails.cache.exist?(cache_key) - - AntiAbuse::UserTrustScore.new(user).remove_old_scores(source) - Rails.cache.write(cache_key, true, expires_in: 5.minutes) - end - end -end diff --git a/app/workers/anti_abuse/trust_score_worker.rb b/app/workers/anti_abuse/trust_score_worker.rb deleted file mode 100644 index 55f37406dcb6b30c732ae0860e032ff8872fd19d..0000000000000000000000000000000000000000 --- a/app/workers/anti_abuse/trust_score_worker.rb +++ /dev/null @@ -1,26 +0,0 @@ -# frozen_string_literal: true - -module AntiAbuse - class TrustScoreWorker - include ApplicationWorker - - data_consistency :delayed - - idempotent! - feature_category :instance_resiliency - urgency :low - - def perform(user_id, source, score, correlation_id = '') - user = User.find_by_id(user_id) - unless user - logger.info(structured_payload(message: "User not found.", user_id: user_id)) - return - end - - return if Feature.enabled?(:remove_trust_scores, user) - - AntiAbuse::TrustScore.create!(user: user, source: source, score: score.to_f, correlation_id_value: correlation_id) - AntiAbuse::TrustScoreCleanupWorker.perform_async(user.id, source) - end - end -end diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 23f015dd53a59ca8557d98bd5c302ecafd700017..8709f9ffae0a09676d90ed148d9d593a49ae89fc 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -63,10 +63,6 @@ - 1 - - anti_abuse_spam_abuse_events - 1 -- - anti_abuse_trust_score - - 1 -- - anti_abuse_trust_score_cleanup - - 1 - - app_config_cascade_duo_features_enabled - 1 - - app_config_cascade_duo_settings diff --git a/db/docs/abuse_trust_scores.yml b/db/docs/deleted_tables/abuse_trust_scores.yml similarity index 77% rename from db/docs/abuse_trust_scores.yml rename to db/docs/deleted_tables/abuse_trust_scores.yml index 7bb1676cabde9347cf549ce842f249148c71e203..1cf88ad6ae999a7f68bd4c5ce61a4181f35bda80 100644 --- a/db/docs/abuse_trust_scores.yml +++ b/db/docs/deleted_tables/abuse_trust_scores.yml @@ -11,3 +11,5 @@ gitlab_schema: gitlab_main_user table_size: small sharding_key: user_id: users +removed_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/209696 +removed_in_milestone: '18.6' diff --git a/db/post_migrate/20251021191955_drop_table_abuse_trust_scores.rb b/db/post_migrate/20251021191955_drop_table_abuse_trust_scores.rb new file mode 100644 index 0000000000000000000000000000000000000000..67edbea87d9bb7d891a78d386dfc6893ebba3a2a --- /dev/null +++ b/db/post_migrate/20251021191955_drop_table_abuse_trust_scores.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +class DropTableAbuseTrustScores < Gitlab::Database::Migration[2.3] + milestone '18.6' + + disable_ddl_transaction! + + def up + drop_table :abuse_trust_scores, if_exists: true + end + + def down + create_table :abuse_trust_scores, if_not_exists: true do |t| + t.references :user, null: true, index: false + t.float :score, null: false + t.timestamps_with_timezone null: false + t.integer :source, limit: 2, null: false + t.text :correlation_id_value, limit: 255 + + t.index [:user_id, :source, :created_at] + end + + add_concurrent_foreign_key :abuse_trust_scores, :users, column: :user_id, on_delete: :cascade, + name: 'fk_rails_b903079eb4' + end +end diff --git a/db/schema_migrations/20251021191955 b/db/schema_migrations/20251021191955 new file mode 100644 index 0000000000000000000000000000000000000000..6154e8452ce8ac63645a3bbffa4a0948f70757e7 --- /dev/null +++ b/db/schema_migrations/20251021191955 @@ -0,0 +1 @@ +11b3185a021317f319a3b2912b034ad8b14237419894e93641d8acaf52a3191e \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 25b51d61c9b4ac8382de66024097c00049cd3a1a..667e8fe26743b33d051ec18e076318296aed1f0b 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -9657,26 +9657,6 @@ CREATE SEQUENCE abuse_reports_id_seq ALTER SEQUENCE abuse_reports_id_seq OWNED BY abuse_reports.id; -CREATE TABLE abuse_trust_scores ( - id bigint NOT NULL, - user_id bigint, - score double precision NOT NULL, - created_at timestamp with time zone NOT NULL, - updated_at timestamp with time zone NOT NULL, - source smallint NOT NULL, - correlation_id_value text, - CONSTRAINT check_77ca9551db CHECK ((char_length(correlation_id_value) <= 255)) -); - -CREATE SEQUENCE abuse_trust_scores_id_seq - START WITH 1 - INCREMENT BY 1 - NO MINVALUE - NO MAXVALUE - CACHE 1; - -ALTER SEQUENCE abuse_trust_scores_id_seq OWNED BY abuse_trust_scores.id; - CREATE TABLE achievement_uploads ( id bigint NOT NULL, size bigint NOT NULL, @@ -30388,8 +30368,6 @@ ALTER TABLE ONLY abuse_report_user_mentions ALTER COLUMN id SET DEFAULT nextval( ALTER TABLE ONLY abuse_reports ALTER COLUMN id SET DEFAULT nextval('abuse_reports_id_seq'::regclass); -ALTER TABLE ONLY abuse_trust_scores ALTER COLUMN id SET DEFAULT nextval('abuse_trust_scores_id_seq'::regclass); - ALTER TABLE ONLY achievements ALTER COLUMN id SET DEFAULT nextval('achievements_id_seq'::regclass); ALTER TABLE ONLY activity_pub_releases_subscriptions ALTER COLUMN id SET DEFAULT nextval('activity_pub_releases_subscriptions_id_seq'::regclass); @@ -32714,9 +32692,6 @@ ALTER TABLE ONLY abuse_report_user_mentions ALTER TABLE ONLY abuse_reports ADD CONSTRAINT abuse_reports_pkey PRIMARY KEY (id); -ALTER TABLE ONLY abuse_trust_scores - ADD CONSTRAINT abuse_trust_scores_pkey PRIMARY KEY (id); - ALTER TABLE ONLY achievement_uploads ADD CONSTRAINT achievement_uploads_pkey PRIMARY KEY (id, model_type); @@ -38438,8 +38413,6 @@ CREATE INDEX index_abuse_reports_on_status_category_and_id ON abuse_reports USIN CREATE INDEX index_abuse_reports_on_status_reporter_id_and_id ON abuse_reports USING btree (status, reporter_id, id); -CREATE INDEX index_abuse_trust_scores_on_user_id_and_source_and_created_at ON abuse_trust_scores USING btree (user_id, source, created_at); - CREATE UNIQUE INDEX "index_achievements_on_namespace_id_LOWER_name" ON achievements USING btree (namespace_id, lower(name)); CREATE UNIQUE INDEX index_active_context_connections_single_active ON ai_active_context_connections USING btree (active) WHERE (active = true); @@ -51560,9 +51533,6 @@ ALTER TABLE batched_background_migration_job_transition_logs ALTER TABLE ONLY approval_project_rules_protected_branches ADD CONSTRAINT fk_rails_b7567b031b FOREIGN KEY (protected_branch_id) REFERENCES protected_branches(id) ON DELETE CASCADE; -ALTER TABLE ONLY abuse_trust_scores - ADD CONSTRAINT fk_rails_b903079eb4 FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE; - ALTER TABLE ONLY project_authorizations_for_migration ADD CONSTRAINT fk_rails_b91fc9995e FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; diff --git a/spec/factories/anti_abuse/trust_score.rb b/spec/factories/anti_abuse/trust_score.rb deleted file mode 100644 index fb93d1134413b196fe68dad7815ac47a0959661d..0000000000000000000000000000000000000000 --- a/spec/factories/anti_abuse/trust_score.rb +++ /dev/null @@ -1,10 +0,0 @@ -# frozen_string_literal: true - -FactoryBot.define do - factory :abuse_trust_score, class: 'AntiAbuse::TrustScore' do - user - score { 0.1 } - source { :spamcheck } - correlation_id_value { 'abcdefg' } - end -end diff --git a/spec/lib/gitlab/database/sharding_key_spec.rb b/spec/lib/gitlab/database/sharding_key_spec.rb index b847073edacd8c8f9e6ac50c484234840eb02a79..69032578768ac2632c604904411486a7ac86a49a 100644 --- a/spec/lib/gitlab/database/sharding_key_spec.rb +++ b/spec/lib/gitlab/database/sharding_key_spec.rb @@ -24,7 +24,6 @@ let(:allowed_to_be_missing_not_null) do [ 'member_roles.namespace_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/444161 - 'abuse_trust_scores.user_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/553436 *['todos.project_id', 'todos.group_id'], # https://gitlab.com/gitlab-org/gitlab/-/issues/562437 *[ 'bulk_import_trackers.organization_id', diff --git a/spec/models/anti_abuse/trust_score_spec.rb b/spec/models/anti_abuse/trust_score_spec.rb deleted file mode 100644 index 90c3f90893987f0b68b985288e7484d1d9d3b779..0000000000000000000000000000000000000000 --- a/spec/models/anti_abuse/trust_score_spec.rb +++ /dev/null @@ -1,45 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe AntiAbuse::TrustScore, feature_category: :instance_resiliency do - let_it_be(:user) { create(:user) } - - let(:correlation_id) { nil } - - let(:abuse_trust_score) do - create(:abuse_trust_score, user: user, correlation_id_value: correlation_id) - end - - describe 'associations' do - it { is_expected.to belong_to(:user) } - end - - describe 'validations' do - it { is_expected.to validate_presence_of(:user) } - it { is_expected.to validate_presence_of(:score) } - it { is_expected.to validate_presence_of(:source) } - end - - describe 'create' do - subject { abuse_trust_score } - - before do - allow(Labkit::Correlation::CorrelationId).to receive(:current_id).and_return('123abc') - end - - context 'if correlation ID is nil' do - it 'adds the correlation id' do - expect(abuse_trust_score.correlation_id_value).to eq('123abc') - end - end - - context 'if correlation ID is set' do - let(:correlation_id) { 'already-set' } - - it 'does not change the correlation id' do - expect(abuse_trust_score.correlation_id_value).to eq('already-set') - end - end - end -end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 5ca4f40736dd473cda11564d8f37d0d8e8f094d2..67a50e8cf6a941c08feb16f29fd27ce213fc97b7 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -278,7 +278,6 @@ it { is_expected.to have_many(:achievements).through(:user_achievements).class_name('Achievements::Achievement').inverse_of(:users) } it { is_expected.to have_many(:namespace_commit_emails).class_name('Users::NamespaceCommitEmail') } it { is_expected.to have_many(:audit_events).with_foreign_key(:author_id).inverse_of(:user) } - it { is_expected.to have_many(:abuse_trust_scores).class_name('AntiAbuse::TrustScore').dependent(:destroy) } it { is_expected.to have_many(:issue_assignment_events).class_name('ResourceEvents::IssueAssignmentEvent') } it { is_expected.to have_many(:merge_request_assignment_events).class_name('ResourceEvents::MergeRequestAssignmentEvent') } it { is_expected.to have_many(:early_access_program_tracking_events).class_name('EarlyAccessProgram::TrackingEvent') } diff --git a/spec/workers/anti_abuse/trust_score_cleanup_worker_spec.rb b/spec/workers/anti_abuse/trust_score_cleanup_worker_spec.rb deleted file mode 100644 index ce7a566aa7f7d7ca2a16ab3e6133727d7c8ed289..0000000000000000000000000000000000000000 --- a/spec/workers/anti_abuse/trust_score_cleanup_worker_spec.rb +++ /dev/null @@ -1,61 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe AntiAbuse::TrustScoreCleanupWorker, :clean_gitlab_redis_shared_state, feature_category: :instance_resiliency do - let(:worker) { described_class.new } - let(:source) { Enums::Abuse::Source.sources[:telesign] } - let_it_be(:user) { create(:user) } - - subject(:perform) { worker.perform(user.id, source) } - - it_behaves_like 'an idempotent worker' do - let(:job_args) { [user.id, source] } - end - - context "when the user does not exist" do - before do - allow(User).to receive(:find_by_id).with(user.id).and_return(nil) - end - - it 'returns early' do - expect(Rails.cache).not_to receive(:exist?) - expect(AntiAbuse::UserTrustScore).not_to receive(:new) - - perform - end - end - - context "when the user exists" do - before do - stub_feature_flags(remove_trust_scores: false) - end - - context "when the cache key exists" do - it 'returns early' do - expect(Rails.cache).to receive(:exist?).and_return(true) - expect(AntiAbuse::UserTrustScore).not_to receive(:new) - - perform - end - end - - context "when the cache key does not exist" do - it 'removes old scores for the user' do - expect(Rails.cache).to receive(:exist?).and_return(false) - expect_next_instance_of(AntiAbuse::UserTrustScore, user) do |instance| - expect(instance).to receive(:remove_old_scores).with(source) - end - - perform - end - - it 'sets the cache_key' do - cache_key = "abuse:trust_score_cleanup_worker:#{user.id}:#{source}" - expect(Rails.cache).to receive(:write).with(cache_key, true, expires_in: 5.minutes) - - perform - end - end - end -end diff --git a/spec/workers/anti_abuse/trust_score_worker_spec.rb b/spec/workers/anti_abuse/trust_score_worker_spec.rb deleted file mode 100644 index a2aa74a037799f42f4b56c5309ca14d806f54756..0000000000000000000000000000000000000000 --- a/spec/workers/anti_abuse/trust_score_worker_spec.rb +++ /dev/null @@ -1,48 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe AntiAbuse::TrustScoreWorker, :clean_gitlab_redis_shared_state, feature_category: :instance_resiliency do - let(:worker) { described_class.new } - let_it_be(:user) { create(:user) } - - subject(:perform) { worker.perform(user.id, :telesign, 0.85, 'foo') } - - it_behaves_like 'an idempotent worker' do - let(:job_args) { [user.id, :telesign, 0.5] } - end - - context "when the user does not exist" do - let(:log_payload) { { 'message' => 'User not found.', 'user_id' => user.id } } - - before do - allow(User).to receive(:find_by_id).with(user.id).and_return(nil) - end - - it 'logs an error' do - expect(Sidekiq.logger).to receive(:info).with(hash_including(log_payload)) - - expect { perform }.not_to raise_exception - end - - it 'does not attempt to create the trust score' do - expect(AntiAbuse::TrustScore).not_to receive(:create!) - - perform - end - end - - context "when the user exists" do - it 'creates an abuse trust score with the correct data' do - stub_feature_flags(remove_trust_scores: false) - - expect { perform }.to change { AntiAbuse::TrustScore.count }.from(0).to(1) - expect(AntiAbuse::TrustScore.last.attributes).to include({ - user_id: user.id, - source: "telesign", - score: 0.85, - correlation_id_value: 'foo' - }.stringify_keys) - end - end -end