diff --git a/app/services/groups/destroy_service.rb b/app/services/groups/destroy_service.rb index 8edef9a5348bd5b4132906a559d8d56be69bcf0b..4b2755b97c0019d69529088d4861eb9e1f0a1631 100644 --- a/app/services/groups/destroy_service.rb +++ b/app/services/groups/destroy_service.rb @@ -4,6 +4,13 @@ module Groups class DestroyService < Groups::BaseService DestroyError = Class.new(StandardError) + DEFAULT_USER_PROJECT_ACCESS_CHANGE_PRIORITY = UserProjectAccessChangedService::HIGH_PRIORITY + VALID_USER_PROJECT_ACCESS_CHANGE_PRIORITIES = [ + UserProjectAccessChangedService::HIGH_PRIORITY, + UserProjectAccessChangedService::MEDIUM_PRIORITY, + UserProjectAccessChangedService::LOW_PRIORITY + ].freeze + def async_execute mark_deleted @@ -12,10 +19,14 @@ def async_execute end # rubocop: disable CodeReuse/ActiveRecord - def execute + def execute(user_project_access_change_priority: DEFAULT_USER_PROJECT_ACCESS_CHANGE_PRIORITY) # TODO - add a policy check here https://gitlab.com/gitlab-org/gitlab/-/issues/353082 raise DestroyError, "You can't delete this group because you're blocked." if current_user.blocked? + unless VALID_USER_PROJECT_ACCESS_CHANGE_PRIORITIES.include?(user_project_access_change_priority) + user_project_access_change_priority = DEFAULT_USER_PROJECT_ACCESS_CHANGE_PRIORITY + end + mark_deleted group.projects.includes(:project_feature).find_each do |project| @@ -42,9 +53,13 @@ def execute group.destroy if user_ids_for_project_authorizations_refresh.present? - UserProjectAccessChangedService - .new(user_ids_for_project_authorizations_refresh) - .execute + service = UserProjectAccessChangedService.new(user_ids_for_project_authorizations_refresh) + + if Feature.enabled?(:resource_tokens_lower_user_access_change_priority, group) + service.execute(priority: user_project_access_change_priority) + else + service.execute + end end publish_event diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb index 775774baad93f900bf3363f67f2b76b1ec724dc5..4bdc16c397806e3914515acd4a8e9f040d771f6a 100644 --- a/app/services/users/destroy_service.rb +++ b/app/services/users/destroy_service.rb @@ -64,7 +64,9 @@ def execute(user, options = {}) user.members.each_batch { |batch| batch.destroy_all } # rubocop:disable Cop/DestroyAll solo_owned_groups.each do |group| - Groups::DestroyService.new(group, current_user).execute + Groups::DestroyService.new(group, current_user).execute( + user_project_access_change_priority: options[:user_project_access_change_priority] + ) end user.personal_projects.each do |project| diff --git a/app/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker.rb b/app/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker.rb index 7e6a4e4b955ecd78d3af648c786b0d38afb0d077..f1b1f37f83021306dbcb584c8cb37d1ef6521501 100644 --- a/app/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker.rb +++ b/app/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker.rb @@ -55,7 +55,11 @@ def initiate_deletion_for(users) arguments_proc: ->(user) { [ admin_bot_id, user.id, - { skip_authorization: true, reason_for_deletion: "No active token assigned" } + { + skip_authorization: true, + reason_for_deletion: "No active token assigned", + user_project_access_change_priority: :medium + } ] }, context_proc: ->(user) { { user: user } } diff --git a/config/feature_flags/gitlab_com_derisk/resource_tokens_lower_user_access_change_priority.yml b/config/feature_flags/gitlab_com_derisk/resource_tokens_lower_user_access_change_priority.yml new file mode 100644 index 0000000000000000000000000000000000000000..80968b14589ade2f779735c1f05883ad0564a783 --- /dev/null +++ b/config/feature_flags/gitlab_com_derisk/resource_tokens_lower_user_access_change_priority.yml @@ -0,0 +1,10 @@ +--- +name: resource_tokens_lower_user_access_change_priority +description: Lower user access change priority for inactive_tokens_deletion_cron_worker +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/541445 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/198610 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/556934 +milestone: '18.3' +group: group::authorization +type: gitlab_com_derisk +default_enabled: false diff --git a/ee/app/services/ee/groups/destroy_service.rb b/ee/app/services/ee/groups/destroy_service.rb index e34c76acd21c277bfbb400450a3ae87884164a7d..7ee228675dd0e45cace3f3201c3732c4e8e73f95 100644 --- a/ee/app/services/ee/groups/destroy_service.rb +++ b/ee/app/services/ee/groups/destroy_service.rb @@ -6,9 +6,9 @@ module DestroyService extend ::Gitlab::Utils::Override override :execute - def execute + def execute(user_project_access_change_priority: :high) with_scheduling_epic_cache_update do - group = super + group = super(user_project_access_change_priority: user_project_access_change_priority) after_destroy(group) group diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb index 8649023432be1cb7ec76f34b4a99ee989032b9de..c5015e368d2ef97b924b9ecab9270ca489e69b6a 100644 --- a/spec/services/groups/destroy_service_spec.rb +++ b/spec/services/groups/destroy_service_spec.rb @@ -15,11 +15,12 @@ group.add_member(user, Gitlab::Access::OWNER) end - def destroy_group(group, user, async) + def destroy_group(group, user, async, user_project_access_change_priority: :high) if async Groups::DestroyService.new(group, user).async_execute else - Groups::DestroyService.new(group, user).execute + Groups::DestroyService.new(group, + user).execute(user_project_access_change_priority: user_project_access_change_priority) end end @@ -134,6 +135,20 @@ def destroy_group(group, user, async) expect(group.deleted_at).to be_nil end end + + context 'when destroying a group with lower user project access change priority' do + let(:user_project_access_change_priority) { :low } + + it 'destroys the group with the specified priority' do + expect_next_instance_of(Groups::DestroyService) do |service| + expect(service).to receive(:execute).with( + user_project_access_change_priority: user_project_access_change_priority + ) + end + + destroy_group(group, user, false, user_project_access_change_priority: user_project_access_change_priority) + end + end end context 'projects in pending_delete' do @@ -242,6 +257,67 @@ def destroy_group(group, user, async) destroy_group(group2, group2_user, false) end + + context 'when feature flag :resource_tokens_lower_user_access_change_priority is enabled' do + before do + stub_feature_flags(resource_tokens_lower_user_access_change_priority: true) + end + + context 'when user_project_access_change_priority is specified' do + let(:user_project_access_change_priority) { :medium } + + it 'calls the service with the correct user access change priority' do + expect_next_instance_of(UserProjectAccessChangedService) do |service| + expect(service).to receive(:execute).with(priority: user_project_access_change_priority) + end + + destroy_group(group2, group2_user, false, + user_project_access_change_priority: user_project_access_change_priority) + end + + context 'when user_project_access_change_priority is not specified' do + it 'calls the service with the default priority' do + expect_next_instance_of(UserProjectAccessChangedService) do |service| + expect(service).to receive(:execute).with( + priority: Groups::DestroyService::DEFAULT_USER_PROJECT_ACCESS_CHANGE_PRIORITY + ) + end + destroy_group(group2, group2_user, false) + end + end + + context 'when user_project_access_change_priority is invalid' do + let(:user_project_access_change_priority) { :invalid } + + it 'calls the service with the default priority' do + expect_next_instance_of(UserProjectAccessChangedService) do |service| + expect(service).to receive(:execute).with( + priority: Groups::DestroyService::DEFAULT_USER_PROJECT_ACCESS_CHANGE_PRIORITY) + end + + destroy_group(group2, group2_user, false, + user_project_access_change_priority: user_project_access_change_priority) + end + end + end + + context 'when feature flag :resource_tokens_lower_user_access_change_priority is disabled' do + let(:user_project_access_change_priority) { :medium } + + before do + stub_feature_flags(resource_tokens_lower_user_access_change_priority: false) + end + + it 'calls the service without the priority argument' do + expect_next_instance_of(UserProjectAccessChangedService) do |service| + expect(service).to receive(:execute).with(no_args) + end + + destroy_group(group2, group2_user, false, + user_project_access_change_priority: user_project_access_change_priority) + end + end + end end context 'and the group is shared with another group' do