diff --git a/app/models/ci/pipeline_schedule.rb b/app/models/ci/pipeline_schedule.rb index 49d270537452cd5dc99533863ba60d1e54f14056..eaef5608ad9d6b0439281096941ed171ad8d6b55 100644 --- a/app/models/ci/pipeline_schedule.rb +++ b/app/models/ci/pipeline_schedule.rb @@ -31,6 +31,7 @@ class PipelineSchedule < Ci::ApplicationRecord scope :inactive, -> { where(active: false) } scope :preloaded, -> { preload(:owner, project: [:route]) } scope :owned_by, ->(user) { where(owner: user) } + scope :for_project, ->(project_id) { where(project_id: project_id) } accepts_nested_attributes_for :variables, allow_destroy: true diff --git a/app/models/user.rb b/app/models/user.rb index dca6570661e5d10ee241dd58e538f5ad2b96d969..3e62b755f332dbc4164d4a86de096482260bd0e5 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -500,8 +500,11 @@ def blocked? # rubocop: disable CodeReuse/ServiceClass after_transition any => :blocked do |user| user.run_after_commit do - Ci::DropPipelineService.new.execute_async_for_all(user.pipelines, :user_blocked, user) - Ci::DisableUserPipelineSchedulesService.new.execute(user) + Ci::DropPipelinesAndDisableSchedulesForUserService.new.execute( + user, + reason: :user_blocked, + include_owned_projects_and_groups: false + ) end end @@ -512,11 +515,23 @@ def blocked? NotificationService.new.user_deactivated(user.name, user.notification_email_or_default) end end - # rubocop: enable CodeReuse/ServiceClass after_transition active: :banned do |user| user.create_banned_user + + if Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks -- this is always necessary on GitLab.com + user.run_after_commit do + deep_clean_ci = user.custom_attributes.by_key(UserCustomAttribute::DEEP_CLEAN_CI_USAGE_WHEN_BANNED).exists? + + Ci::DropPipelinesAndDisableSchedulesForUserService.new.execute( + user, + reason: :user_banned, + include_owned_projects_and_groups: deep_clean_ci + ) + end + end end + # rubocop: enable CodeReuse/ServiceClass after_transition banned: :active do |user| user.banned_user&.destroy diff --git a/app/models/user_custom_attribute.rb b/app/models/user_custom_attribute.rb index d294ea49352dc546897ae23f05e84c0296e30beb..8d82910d29a50cc5948d699d711f0145bf8b8d25 100644 --- a/app/models/user_custom_attribute.rb +++ b/app/models/user_custom_attribute.rb @@ -24,6 +24,7 @@ class UserCustomAttribute < ApplicationRecord DELETED_OWN_ACCOUNT_AT = 'deleted_own_account_at' SKIPPED_ACCOUNT_DELETION_AT = 'skipped_account_deletion_at' ASSUMED_HIGH_RISK_REASON = 'assumed_high_risk_reason' + DEEP_CLEAN_CI_USAGE_WHEN_BANNED = 'deep_clean_ci_usage_when_banned' class << self def upsert_custom_attributes(custom_attributes) diff --git a/app/services/ci/disable_user_pipeline_schedules_service.rb b/app/services/ci/disable_user_pipeline_schedules_service.rb deleted file mode 100644 index 6499fbba0ecb0bc2c13f6d896427d0b6301b2be0..0000000000000000000000000000000000000000 --- a/app/services/ci/disable_user_pipeline_schedules_service.rb +++ /dev/null @@ -1,11 +0,0 @@ -# frozen_string_literal: true - -module Ci - class DisableUserPipelineSchedulesService - def execute(user) - Ci::PipelineSchedule.active.owned_by(user).each_batch do |relation| - relation.update_all(active: false) - end - end - end -end diff --git a/app/services/ci/drop_pipelines_and_disable_schedules_for_user_service.rb b/app/services/ci/drop_pipelines_and_disable_schedules_for_user_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..e6c18b2f27b4d49f7af9bb9846c8ecbeec7bfa4a --- /dev/null +++ b/app/services/ci/drop_pipelines_and_disable_schedules_for_user_service.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +module Ci + class DropPipelinesAndDisableSchedulesForUserService + def execute(user, reason: :user_blocked, include_owned_projects_and_groups: false) + if include_owned_projects_and_groups + # Projects in the user namespace + Project.personal(user).each_batch do |relation| + project_ids = relation.pluck_primary_key + + drop_pipelines_for_projects(user, project_ids, reason) + disable_schedules_for_projects(project_ids) + end + + # Projects in group and descendant namespaces owned by the user + user.owned_groups.select(:id).each_batch do |owned_groups_relation| + owned_groups_relation.each do |owned_group| + Project.in_namespace(owned_group.self_and_descendant_ids).each_batch do |project_relation| + project_ids = project_relation.pluck_primary_key + + drop_pipelines_for_projects(user, project_ids, reason) + disable_schedules_for_projects(project_ids) + end + end + end + end + + drop_pipelines_for_user(user, reason) + disable_schedules_for_user(user) + end + + private + + def drop_pipelines_for_user(user, reason) + Ci::DropPipelineService.new.execute_async_for_all( + Ci::Pipeline.for_user(user), + reason, + user + ) + end + + def drop_pipelines_for_projects(user, project_ids, reason) + Ci::DropPipelineService.new.execute_async_for_all( + Ci::Pipeline.for_project(project_ids), + reason, + user + ) + end + + def disable_schedules_for_user(user) + Ci::PipelineSchedule.owned_by(user).active.each_batch do |relation| + relation.update_all(active: false) + end + end + + def disable_schedules_for_projects(project_ids) + Ci::PipelineSchedule.for_project(project_ids).active.each_batch do |relation| + relation.update_all(active: false) + end + end + end +end diff --git a/spec/models/ci/pipeline_schedule_spec.rb b/spec/models/ci/pipeline_schedule_spec.rb index c441be58edfa9dac1f7d55ef0ce7461245c82089..941ccb2dd31ddd05bafb4fce1c59f3e6a15954f3 100644 --- a/spec/models/ci/pipeline_schedule_spec.rb +++ b/spec/models/ci/pipeline_schedule_spec.rb @@ -115,6 +115,18 @@ end end + describe '.for_project' do + let(:project) { create(:project) } + let!(:project_pipeline_schedule) { create(:ci_pipeline_schedule, project: project) } + let!(:other_pipeline_schedule) { create(:ci_pipeline_schedule) } + + subject { described_class.for_project(project) } + + it 'returns pipeline schedule only for project' do + is_expected.to eq([project_pipeline_schedule]) + end + end + describe '#set_next_run_at' do let(:now) { Time.zone.local(2021, 3, 2, 1, 0) } let(:pipeline_schedule) { create(:ci_pipeline_schedule, cron: "0 1 * * *") } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 7014c9e685f159b4dc9c7a61fb1015a57eb0a8c3..c40c43db72702b1d3fdc179fd5178ccc887ef979 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2718,6 +2718,25 @@ it { expect(@user.namespaces).to eq([@user.namespace]) } end + shared_examples 'Ci::DropPipelinesAndDisableSchedulesForUserService called with correct arguments' do + let(:reason) { :user_blocked } + let(:include_owned_projects_and_groups) { false } + subject(:action) { user.block! } + + it 'calls Ci::DropPipelinesAndDisableSchedules service with correct arguments' do + drop_disable_service = double + + expect(Ci::DropPipelinesAndDisableSchedulesForUserService).to receive(:new).and_return(drop_disable_service) + expect(drop_disable_service).to receive(:execute).with( + user, + reason: reason, + include_owned_projects_and_groups: include_owned_projects_and_groups + ) + + action + end + end + describe 'blocking user' do let_it_be_with_refind(:user) { create(:user, name: 'John Smith') } @@ -2727,33 +2746,10 @@ expect(user.blocked?).to be_truthy end - context 'when user has running CI pipelines' do - let(:pipelines) { build_list(:ci_pipeline, 3, :running) } - - it 'drops all running pipelines and related jobs' do - drop_service = double - disable_service = double - - expect(user).to receive(:pipelines).and_return(pipelines) - expect(Ci::DropPipelineService).to receive(:new).and_return(drop_service) - expect(drop_service).to receive(:execute_async_for_all).with(pipelines, :user_blocked, user) - - expect(Ci::DisableUserPipelineSchedulesService).to receive(:new).and_return(disable_service) - expect(disable_service).to receive(:execute).with(user) - - user.block! - end - - it 'does not drop running pipelines if the transaction rolls back' do - expect(Ci::DropPipelineService).not_to receive(:new) - expect(Ci::DisableUserPipelineSchedulesService).not_to receive(:new) - - User.transaction do - user.block - - raise ActiveRecord::Rollback - end - end + it_behaves_like 'Ci::DropPipelinesAndDisableSchedulesForUserService called with correct arguments' do + let(:reason) { :user_blocked } + let(:include_owned_projects_and_groups) { false } + subject(:action) { user.block! } end context 'when user has active CI pipeline schedules' do @@ -2865,7 +2861,7 @@ describe 'banning and unbanning a user', :aggregate_failures do let(:user) { create(:user) } - context 'banning a user' do + context 'when banning a user' do it 'bans and blocks the user' do user.ban @@ -2877,6 +2873,35 @@ expect { user.ban }.to change { Users::BannedUser.count }.by(1) expect(Users::BannedUser.last.user_id).to eq(user.id) end + + context 'when GitLab.com' do + before do + allow(::Gitlab).to receive(:com?).and_return(true) + end + + it_behaves_like 'Ci::DropPipelinesAndDisableSchedulesForUserService called with correct arguments' do + let(:reason) { :user_banned } + let(:include_owned_projects_and_groups) { false } + subject(:action) { user.ban! } + end + + context 'when user has "deep_clean_ci_usage_when_banned" custom attribute set' do + before do + create( + :user_custom_attribute, + key: UserCustomAttribute::DEEP_CLEAN_CI_USAGE_WHEN_BANNED, value: true.to_s, + user_id: user.id + ) + user.reload + end + + it_behaves_like 'Ci::DropPipelinesAndDisableSchedulesForUserService called with correct arguments' do + let(:reason) { :user_banned } + let(:include_owned_projects_and_groups) { true } + subject(:action) { user.ban! } + end + end + end end context 'unbanning a user' do diff --git a/spec/services/ci/disable_user_pipeline_schedules_service_spec.rb b/spec/services/ci/disable_user_pipeline_schedules_service_spec.rb deleted file mode 100644 index d422cf0dab9ee491cab17ba93441c3928d22d737..0000000000000000000000000000000000000000 --- a/spec/services/ci/disable_user_pipeline_schedules_service_spec.rb +++ /dev/null @@ -1,19 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Ci::DisableUserPipelineSchedulesService, feature_category: :continuous_integration do - describe '#execute' do - let(:user) { create(:user) } - - subject(:service) { described_class.new.execute(user) } - - context 'when user has active pipeline schedules' do - let(:owned_pipeline_schedule) { create(:ci_pipeline_schedule, active: true, owner: user) } - - it 'disables all active pipeline schedules', :aggregate_failures do - expect { service }.to change { owned_pipeline_schedule.reload.active? } - end - end - end -end diff --git a/spec/services/ci/drop_pipelines_and_disable_schedules_for_user_service_spec.rb b/spec/services/ci/drop_pipelines_and_disable_schedules_for_user_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..da3852eed47285ddbf9245c41040d4967d5f0d16 --- /dev/null +++ b/spec/services/ci/drop_pipelines_and_disable_schedules_for_user_service_spec.rb @@ -0,0 +1,207 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::DropPipelinesAndDisableSchedulesForUserService, feature_category: :continuous_integration do + describe '#execute' do + let_it_be(:user) { create(:user) } + + let_it_be(:user_personal_projects) { create_list(:project, 2, :repository, namespace: user.namespace) } + + let_it_be(:group) do + create(:group) do |group| + group.add_owner(user) + end + end + + let_it_be(:subgroup) { create(:group, parent: group) } + + let_it_be(:other_user) do + create(:user) do |new_user| + create(:group_member, :maintainer, user: new_user, group: group) + create(:group_member, :maintainer, user: new_user, group: subgroup) + + user_personal_projects.each do |project| + create(:project_member, :maintainer, user: new_user, project: project) + end + end + end + + let_it_be(:group_projects) { create_list(:project, 2, :repository, namespace: group) } + let_it_be(:subgroup_projects) do + create_list(:project, 2, :repository, namespace: subgroup) + end + + let_it_be(:other_user_personal_projects) { create_list(:project, 2, :repository, namespace: other_user.namespace) } + + subject(:service) { described_class.new.execute(user) } + + context 'when user owns pipelines/schedules and groups with other users also owning pipelines/schedules' do + # Pipelines/pipeline schedules owned by the user in their personal, group and descendent group projects + let_it_be_with_reload(:user_owned_pipelines) do + [user_personal_projects, group_projects, subgroup_projects].flat_map do |projects| + projects.flat_map do |project| + create_list(:ci_pipeline, 2, :running, project: project, user: user) do |pipeline| + create(:ci_build, :running, pipeline: pipeline) + create(:commit_status, :running, pipeline: pipeline) + end + end + end + end + + let_it_be_with_reload(:user_owned_schedules) do + [user_personal_projects, group_projects, subgroup_projects].flat_map do |projects| + projects.flat_map do |project| + create_list(:ci_pipeline_schedule, 2, active: true, project: project, owner: user) + end + end + end + + # Pipelines/pipeline schedules owned by another user in user personal projects and group and descendant group + # projects owned by the user + let_it_be_with_reload(:other_user_owned_group_project_pipelines) do + [user_personal_projects, group_projects, subgroup_projects].flat_map do |projects| + projects.flat_map do |project| + create_list(:ci_pipeline, 2, :running, project: project, user: other_user) do |pipeline| + create(:ci_build, :running, pipeline: pipeline) + create(:commit_status, :running, pipeline: pipeline) + end + end + end + end + + let_it_be_with_reload(:other_user_owned_group_project_schedules) do + [user_personal_projects, group_projects, subgroup_projects].flat_map do |projects| + projects.flat_map do |project| + create_list(:ci_pipeline_schedule, 2, active: true, project: project, owner: other_user) + end + end + end + + # Pipelines/pipeline schedules owned by another user in their personal projects (should never be changed) + let_it_be_with_reload(:other_user_owned_personal_pipelines) do + other_user_personal_projects.flat_map do |project| + create_list(:ci_pipeline, 2, :running, project: project, user: other_user) do |pipeline| + create(:ci_build, :running, pipeline: pipeline) + create(:commit_status, :running, pipeline: pipeline) + end + end + end + + let_it_be_with_reload(:other_user_owned_personal_schedules) do + other_user_personal_projects.flat_map do |project| + create_list(:ci_pipeline_schedule, 2, active: true, project: project, owner: other_user) + end + end + + it 'drops running pipelines/disabled active schedules owned by user', :sidekiq_inline do + expect { service }.to change { + user_owned_pipelines.map(&:reload).map(&:status).uniq + } + .from(["running"]) + .to(["failed"]) + .and change { + user_owned_schedules.map(&:reload).map(&:active?).uniq + } + .from([true]) + .to([false]) + .and not_change { + [ + other_user_owned_group_project_pipelines, + other_user_owned_personal_pipelines + ].flatten.map(&:reload).map(&:status).uniq + } + .and not_change { + [ + other_user_owned_group_project_schedules, + other_user_owned_personal_schedules + ].flatten.map(&:reload).map(&:active?).uniq + } + end + + it 'avoids N+1 queries when reading data' do + control_count = ActiveRecord::QueryRecorder.new do + described_class.new.execute(user) + end.count + + extra_projects = create_list(:project, 2, :repository, namespace: group) + + [extra_projects, user_personal_projects, group_projects, subgroup_projects].flat_map do |projects| + projects.flat_map do |project| + create_list(:ci_pipeline, 2, :running, project: project, user: user) do |pipeline| + create(:ci_build, :running, pipeline: pipeline) + create(:commit_status, :running, pipeline: pipeline) + end + create_list(:ci_pipeline_schedule, 2, active: true, project: project, owner: user) + end + end + + expect do + described_class.new.execute(user) + end.not_to exceed_query_limit(control_count) + end + + context 'when include_owned_projects_and_groups is true' do + subject(:service) { described_class.new.execute(user, include_owned_projects_and_groups: true) } + + it 'drops running pipelines/disabled active schedules owned by user and/or in their owned groups/descendants', + :sidekiq_inline do + expect { service }.to change { + [ + user_owned_pipelines, + other_user_owned_group_project_pipelines + ].flatten.map(&:reload).map(&:status).uniq + } + .from(["running"]) + .to(["failed"]) + .and change { + [ + user_owned_schedules, + other_user_owned_group_project_schedules + ].flatten.map(&:reload).map(&:active?).uniq + } + .from([true]) + .to([false]) + .and not_change { + other_user_owned_personal_pipelines.map(&:reload).map(&:active?).uniq + } + .and not_change { + other_user_owned_personal_schedules.map(&:reload).map(&:active?).uniq + } + end + + it 'avoids N+1 queries when reading data' do + control_count = ActiveRecord::QueryRecorder.new do + described_class.new.execute(user, include_owned_projects_and_groups: true) + end.count + + extra_projects = create_list(:project, 2, :repository, namespace: group) + + [user_personal_projects, extra_projects, group_projects, subgroup_projects].flat_map do |projects| + projects.flat_map do |project| + create_list(:ci_pipeline, 2, :running, project: project, user: user) do |pipeline| + create(:ci_build, :running, pipeline: pipeline) + create(:commit_status, :running, pipeline: pipeline) + end + create_list(:ci_pipeline_schedule, 2, active: true, project: project, owner: user) + end + end + + [extra_projects, group_projects, subgroup_projects].flat_map do |projects| + projects.flat_map do |project| + create_list(:ci_pipeline, 2, :running, project: project, user: other_user) do |pipeline| + create(:ci_build, :running, pipeline: pipeline) + create(:commit_status, :running, pipeline: pipeline) + end + create_list(:ci_pipeline_schedule, 2, active: true, project: project, owner: other_user) + end + end + + expect do + described_class.new.execute(user, include_owned_projects_and_groups: true) + end.not_to exceed_query_limit(control_count) + end + end + end + end +end diff --git a/spec/support/helpers/user_with_namespace_shim.yml b/spec/support/helpers/user_with_namespace_shim.yml index 7e7a25aa023946f4bc483e41af12579fea7314b7..7f683e3f2206ff10b982f6c452fb3e4fc7f84036 100644 --- a/spec/support/helpers/user_with_namespace_shim.yml +++ b/spec/support/helpers/user_with_namespace_shim.yml @@ -934,6 +934,7 @@ - spec/services/auth/container_registry_authentication_service_spec.rb - spec/services/award_emojis/add_service_spec.rb - spec/services/ci/abort_pipelines_service_spec.rb +- spec/services/ci/drop_pipelines_and_disable_schedules_for_user_service_spec.rb - spec/services/draft_notes/publish_service_spec.rb - spec/services/environments/schedule_to_delete_review_apps_service_spec.rb - spec/services/import/bitbucket_server_service_spec.rb diff --git a/spec/support/rspec_order_todo.yml b/spec/support/rspec_order_todo.yml index 3a0e563f7f87bbdbe0b80e73b242037f1e683175..d67eaf6920ce9ff48bf36590b4a5d8f7a2c7bbfd 100644 --- a/spec/support/rspec_order_todo.yml +++ b/spec/support/rspec_order_todo.yml @@ -8510,7 +8510,6 @@ - './spec/services/ci/deployments/destroy_service_spec.rb' - './spec/services/ci/destroy_pipeline_service_spec.rb' - './spec/services/ci/destroy_secure_file_service_spec.rb' -- './spec/services/ci/disable_user_pipeline_schedules_service_spec.rb' - './spec/services/ci/drop_pipeline_service_spec.rb' - './spec/services/ci/ensure_stage_service_spec.rb' - './spec/services/ci/expire_pipeline_cache_service_spec.rb'