diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index c1ba64fd24647d9f0736307ba011354e2fbc4370..22ed8321e84764330268301b861bf0cd33ff119d 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -173,6 +173,9 @@ def edit_group_origin_location def destroy if group.self_deletion_scheduled? && ::Gitlab::Utils.to_boolean(params.permit(:permanently_remove)[:permanently_remove]) + + return access_denied! if Feature.enabled?(:disallow_immediate_deletion, current_user) + return destroy_immediately end diff --git a/app/controllers/organizations/groups_controller.rb b/app/controllers/organizations/groups_controller.rb index 5fa1e8f8102a13979f705e2018c4bb7b95c0599c..701a3e9644db503e2adb4074d14e3a784419216e 100644 --- a/app/controllers/organizations/groups_controller.rb +++ b/app/controllers/organizations/groups_controller.rb @@ -30,6 +30,9 @@ def create def destroy if group.self_deletion_scheduled? && ::Gitlab::Utils.to_boolean(params.permit(:permanently_remove)[:permanently_remove]) + + return access_denied! if Feature.enabled?(:disallow_immediate_deletion, current_user) + return destroy_immediately end diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 65b87bee8426306297d56e72aa927c6bd134129c..15778d7f793c53efd00155884f64617fb4d37dc8 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -192,7 +192,13 @@ def show def destroy return access_denied! unless can?(current_user, :remove_project, @project) - return destroy_immediately if @project.self_deletion_scheduled? && params[:permanently_delete].present? + if @project.self_deletion_scheduled? && + ::Gitlab::Utils.to_boolean(params.permit(:permanently_delete)[:permanently_delete]) + + return access_denied! if Feature.enabled?(:disallow_immediate_deletion, current_user) + + return destroy_immediately + end result = ::Projects::MarkForDeletionService.new(@project, current_user).execute diff --git a/app/views/groups/settings/_advanced.html.haml b/app/views/groups/settings/_advanced.html.haml index 1615979e8dcd3b996c85e7e8ff93a8463fbc683f..fecaad94d90ab58ad2e5b067cf48b5f33b420093 100644 --- a/app/views/groups/settings/_advanced.html.haml +++ b/app/views/groups/settings/_advanced.html.haml @@ -33,5 +33,5 @@ = render 'groups/settings/transfer', group: @group = render 'groups/settings/remove', group: @group, remove_form_id: remove_form_id - = render_if_exists 'shared/groups_projects/settings/restore', context: @group - = render_if_exists 'groups/settings/immediately_remove', group: @group, remove_form_id: remove_form_id + = render 'shared/groups_projects/settings/restore', context: @group + = render 'groups/settings/immediately_remove', group: @group, remove_form_id: remove_form_id diff --git a/app/views/groups/settings/_immediately_remove.html.haml b/app/views/groups/settings/_immediately_remove.html.haml index cf90f78ed95070fe84da25dbca5c454098ada87f..91ef408aff1bce2208125e078fd21bc2756f253f 100644 --- a/app/views/groups/settings/_immediately_remove.html.haml +++ b/app/views/groups/settings/_immediately_remove.html.haml @@ -1,3 +1,4 @@ +- return if Feature.enabled?(:disallow_immediate_deletion, current_user) - return unless group.self_deletion_scheduled? - remove_form_id = local_assigns.fetch(:remove_form_id, nil) diff --git a/app/views/projects/_delete.html.haml b/app/views/projects/_delete.html.haml index 3017d27813de00ffab2bd05df85d2d07b5c4c60f..a3a15fd3612ed212bb2e90ea1394033844f3bbff 100644 --- a/app/views/projects/_delete.html.haml +++ b/app/views/projects/_delete.html.haml @@ -1,4 +1,6 @@ - return unless can?(current_user, :remove_project, @project) +- return if Feature.enabled?(:disallow_immediate_deletion, current_user) && @project.self_deletion_scheduled? + - title = @project.self_deletion_scheduled? ? _('Delete project immediately') : _('Delete project') = render Pajamas::CardComponent.new(body_options: { class: 'gl-bg-feedback-danger' }) do |c| @@ -7,4 +9,8 @@ %h4.gl-text-base.gl-leading-24.gl-m-0.gl-text-feedback-danger= title - c.with_body do - = render 'projects/delete_card_body' + - if @project.self_deletion_scheduled? + %p= delete_immediately_namespace_scheduled_for_deletion_message(@project) + #js-project-delete-button{ data: project_delete_immediately_button_data(@project, _('Delete project immediately')) } + - else + = render 'delete_delayed' diff --git a/app/views/projects/_delete_card_body.html.haml b/app/views/projects/_delete_card_body.html.haml deleted file mode 100644 index e1404bc6f89c4c483c6f93fc67ec3f457882ce7a..0000000000000000000000000000000000000000 --- a/app/views/projects/_delete_card_body.html.haml +++ /dev/null @@ -1,5 +0,0 @@ -- if @project.self_deletion_scheduled? - %p= delete_immediately_namespace_scheduled_for_deletion_message(@project) - #js-project-delete-button{ data: project_delete_immediately_button_data(@project, _('Delete project immediately')) } -- else - = render 'delete_delayed' diff --git a/config/feature_flags/gitlab_com_derisk/disallow_immediate_deletion.yml b/config/feature_flags/gitlab_com_derisk/disallow_immediate_deletion.yml new file mode 100644 index 0000000000000000000000000000000000000000..9a11c78ca5c13621acfc9d50bf96e09113cffdf9 --- /dev/null +++ b/config/feature_flags/gitlab_com_derisk/disallow_immediate_deletion.yml @@ -0,0 +1,10 @@ +--- +name: disallow_immediate_deletion +description: +feature_issue_url: +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/201957 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/562308 +milestone: '18.4' +group: group::organizations +type: gitlab_com_derisk +default_enabled: false diff --git a/lib/api/groups.rb b/lib/api/groups.rb index 7bcaeb6b1c210b0caaf61884cf88cdb03e6fdd49..d6d7582896f33754719535cb8392d6535c70fa6c 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -173,7 +173,9 @@ def present_groups_with_pagination_strategies(params, groups) end def immediately_delete_subgroup_error(group) - if !group.subgroup? + if Feature.enabled?(:disallow_immediate_deletion, current_user) + '`permanently_remove` option is not available anymore (behind the :disallow_immediate_deletion feature flag).' + elsif !group.subgroup? '`permanently_remove` option is only available for subgroups.' elsif !group.self_deletion_scheduled? 'Group must be marked for deletion first.' diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 73d92a0e435acb31158ecc06d6dfca7d6b311303..1614c8eb53131c96be02934effdcf609e95500a2 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -86,7 +86,9 @@ def support_order_by_similarity!(attrs) end def immediately_delete_project_error(project) - if !project.marked_for_deletion_at? + if Feature.enabled?(:disallow_immediate_deletion, current_user) + '`permanently_remove` option is not available anymore (behind the :disallow_immediate_deletion feature flag).' + elsif !project.self_deletion_scheduled? 'Project must be marked for deletion first.' elsif project.full_path != params[:full_path] '`full_path` is incorrect. You must enter the complete path for the project.' diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index b47e7988ae097d1fc6395150dcbb2d9f5e7054c7..95e03dc5d274495d1027eb848ac0920c77737262 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -633,26 +633,42 @@ context 'when permanently_remove param is set' do let(:params) { { permanently_remove: true } } - context 'for a html request' do - it 'deletes the group immediately and redirects to root path' do - expect(GroupDestroyWorker).to receive(:perform_async) - - subject + describe 'forbidden by the :disallow_immediate_deletion feature flag' do + it 'returns error' do + Sidekiq::Testing.fake! do + expect { subject }.not_to change { GroupDestroyWorker.jobs.size } + end - expect(response).to redirect_to(root_path) - expect(flash[:toast]).to include "Group '#{group.name}' is being deleted." + expect(response).to have_gitlab_http_status(:not_found) end end - context 'for a json request' do - let(:format) { :json } + context 'when the :disallow_immediate_deletion feature flag is disabled' do + before do + stub_feature_flags(disallow_immediate_deletion: false) + end - it 'deletes the group immediately and returns json with message' do - expect(GroupDestroyWorker).to receive(:perform_async) + context 'for a html request' do + it 'deletes the group immediately and redirects to root path' do + expect(GroupDestroyWorker).to receive(:perform_async) - subject + subject - expect(json_response['message']).to eq("Group '#{group.name}' is being deleted.") + expect(response).to redirect_to(root_path) + expect(flash[:toast]).to include "Group '#{group.name}' is being deleted." + end + end + + context 'for a json request' do + let(:format) { :json } + + it 'deletes the group immediately and returns json with message' do + expect(GroupDestroyWorker).to receive(:perform_async) + + subject + + expect(json_response['message']).to eq("Group '#{group.name}' is being deleted.") + end end end end diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index d75534bb15f70b0eb90014a704e42b89029a4ec4..45659311dcba8e6686eeeb6eee0ee4cf1ab6a2db 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -1226,16 +1226,6 @@ def update_project_feature sign_in(user) end - shared_examples 'deletes project right away' do - specify :aggregate_failures do - delete :destroy, params: { namespace_id: project.namespace, id: project } - - expect(project.self_deletion_scheduled?).to be_falsey - expect(response).to have_gitlab_http_status(:found) - expect(response).to redirect_to(dashboard_projects_path) - end - end - shared_examples 'marks project for deletion' do specify :aggregate_failures do delete :destroy, params: { namespace_id: project.namespace, id: project } @@ -1265,77 +1255,22 @@ def update_project_feature context 'when project is already marked for deletion' do let_it_be(:project) { create(:project, group: group, marked_for_deletion_at: Date.current) } - context 'when permanently_delete param is set' do - it 'deletes project right away' do - expect(ProjectDestroyWorker).to receive(:perform_async) - - delete :destroy, params: { namespace_id: project.namespace, id: project, permanently_delete: true } - - expect(project.reload.pending_delete).to eq(true) - expect(response).to have_gitlab_http_status(:found) - expect(response).to redirect_to(dashboard_projects_path) - end - end - - context 'when permanently_delete param is not set' do - it 'redirects to edit page' do - expect(ProjectDestroyWorker).not_to receive(:perform_async) - - delete :destroy, params: { namespace_id: project.namespace, id: project } - - expect(project.reload.pending_delete).to eq(false) - expect(response).to have_gitlab_http_status(:ok) - expect(response).to render_template(:edit) - expect(flash[:alert]).to include('Project has been already marked for deletion') - end - end - end - - context 'for projects in user namespace' do - let_it_be_with_reload(:project) { create(:project, namespace: user.namespace) } - - before do - sign_in(user) - end + describe 'forbidden by the :disallow_immediate_deletion feature flag' do + subject(:request) { delete :destroy, params: { namespace_id: project.namespace, id: project, permanently_delete: true } } - shared_examples 'deletes project right away' do - specify :aggregate_failures do - delete :destroy, params: { namespace_id: project.namespace, id: project } + it 'returns error' do + Sidekiq::Testing.fake! do + expect { request }.not_to change { ProjectDestroyWorker.jobs.size } + end - expect(project.self_deletion_scheduled?).to be_falsey - expect(response).to have_gitlab_http_status(:found) - expect(response).to redirect_to(dashboard_projects_path) + expect(response).to have_gitlab_http_status(:not_found) end end - shared_examples 'marks project for deletion' do - specify :aggregate_failures do - delete :destroy, params: { namespace_id: project.namespace, id: project } - - expect(project.reload.self_deletion_scheduled?).to be_truthy - expect(project.reload.hidden?).to be_falsey - expect(response).to have_gitlab_http_status(:found) - expect(response).to redirect_to(project_path(project)) - expect(flash[:toast]).to be_nil + context 'when the :disallow_immediate_deletion feature flag is disabled' do + before do + stub_feature_flags(disallow_immediate_deletion: false) end - end - - it_behaves_like 'marks project for deletion' - - it 'does not mark project for deletion because of error' do - message = 'Error' - - expect(::Projects::MarkForDeletionService).to receive_message_chain(:new, :execute).and_return(ServiceResponse.error(message: message)) - - delete :destroy, params: { namespace_id: project.namespace, id: project } - - expect(response).to have_gitlab_http_status(:ok) - expect(response).to render_template(:edit) - expect(flash[:alert]).to include(message) - end - - context 'when project is already marked for deletion' do - let_it_be(:project) { create(:project, group: group, marked_for_deletion_at: Date.current) } context 'when permanently_delete param is set' do it 'deletes project right away' do @@ -1348,26 +1283,20 @@ def update_project_feature expect(response).to redirect_to(dashboard_projects_path) end end + end - context 'when permanently_delete param is not set' do - it 'redirects to edit page' do - expect(ProjectDestroyWorker).not_to receive(:perform_async) + context 'when permanently_delete param is not set' do + it 'redirects to edit page' do + expect(ProjectDestroyWorker).not_to receive(:perform_async) - delete :destroy, params: { namespace_id: project.namespace, id: project } + delete :destroy, params: { namespace_id: project.namespace, id: project } - expect(project.reload.pending_delete).to eq(false) - expect(response).to have_gitlab_http_status(:ok) - expect(response).to render_template(:edit) - expect(flash[:alert]).to include('Project has been already marked for deletion') - end + expect(project.reload.pending_delete).to eq(false) + expect(response).to have_gitlab_http_status(:ok) + expect(response).to render_template(:edit) + expect(flash[:alert]).to include('Project has been already marked for deletion') end end - - context 'for projects in user namespace' do - let(:project) { create(:project, namespace: user.namespace) } - - it_behaves_like 'marks project for deletion' - end end end diff --git a/spec/features/groups/group_settings_spec.rb b/spec/features/groups/group_settings_spec.rb index 6b1dfff81da47de54b0c947e1edc334047ce7bd5..1ba2315e1f86d854a2eb889c8e98ed57f19c1895 100644 --- a/spec/features/groups/group_settings_spec.rb +++ b/spec/features/groups/group_settings_spec.rb @@ -395,32 +395,30 @@ end describe 'delayed project deletion' do - let(:form_group_selector) { '[data-testid="delayed-project-removal-form-group"]' } - def remove_with_confirm(button_text, confirm_with, confirm_button_text = 'Confirm') click_button button_text fill_in 'confirm_name_input', with: confirm_with click_button confirm_button_text end - context 'when immediately deleting a project marked for deletion', :js do + context 'when group is marked for deletion', :js do before do - create(:group_deletion_schedule, group: group, marked_for_deletion_on: 2.days.from_now) - - visit edit_group_path(group) + create(:group_deletion_schedule, group: group) end - it 'deletes the project immediately', :sidekiq_inline do - expect { remove_with_confirm('Delete group', group.path) }.to change { Group.count }.by(-1) + context 'when the :disallow_immediate_deletion feature flag is disabled' do + before do + stub_feature_flags(disallow_immediate_deletion: false) - expect(page).to have_content "is being deleted" - end - end + visit edit_group_path(group) + end - it 'does not display delayed project removal field at group level', :js do - visit edit_group_path(group) + it 'deletes the project immediately', :sidekiq_inline do + expect { remove_with_confirm('Delete group immediately', group.path) }.to change { Group.count }.by(-1) - expect(page).not_to have_css(form_group_selector) + expect(page).to have_content "is being deleted" + end + end end end diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index 36dc4ddab0a37daaa38dcc667cf199bc0ee91569..4f7e1660c9f39ccf12465b779b8ddb3d6f01ea31 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -3230,39 +3230,61 @@ def make_upload_request subject { delete api("/groups/#{subgroup.id}", user), params: params } - context 'when group is already marked for deletion' do + context 'forbidden by the :disallow_immediate_deletion feature flag' do + it_behaves_like 'does not immediately enqueues the job to delete the group', + '`permanently_remove` option is not available anymore (behind the :disallow_immediate_deletion feature flag).' + end + + context 'when the :disallow_immediate_deletion feature flag is disabled' do before do - create(:group_deletion_schedule, group: subgroup, marked_for_deletion_on: Date.current) + stub_feature_flags(disallow_immediate_deletion: false) end - context 'when full_path param is not passed' do - it_behaves_like 'does not immediately enqueues the job to delete the group', - '`full_path` is incorrect. You must enter the complete path for the subgroup.' + context 'when group is not marked for deletion' do + it_behaves_like 'does not immediately enqueues the job to delete the group', 'Group must be marked for deletion first.' end - context 'when full_path param is not equal to full_path' do - let(:params) { { permanently_remove: true, full_path: subgroup.path } } + context 'when group is already marked for deletion' do + before do + create(:group_deletion_schedule, group: subgroup, marked_for_deletion_on: Date.current) + end - it_behaves_like 'does not immediately enqueues the job to delete the group', - '`full_path` is incorrect. You must enter the complete path for the subgroup.' - end + context 'when full_path param is not passed' do + it_behaves_like 'does not immediately enqueues the job to delete the group', + '`full_path` is incorrect. You must enter the complete path for the subgroup.' + end - context 'when the full_path param is passed and it matches the full path of subgroup' do - let(:params) { { permanently_remove: true, full_path: subgroup.full_path } } + context 'when full_path param is not equal to full_path' do + let(:params) { { permanently_remove: true, full_path: subgroup.path } } - it_behaves_like 'immediately enqueues the job to delete the group' - end - end + it_behaves_like 'does not immediately enqueues the job to delete the group', + '`full_path` is incorrect. You must enter the complete path for the subgroup.' + end + + context 'when the full_path param is passed and it matches the full path of subgroup' do + let(:params) { { permanently_remove: true, full_path: subgroup.full_path } } - context 'when group is not marked for deletion' do - it_behaves_like 'does not immediately enqueues the job to delete the group', 'Group must be marked for deletion first.' + it_behaves_like 'immediately enqueues the job to delete the group' + end + end end end context 'if group is not a subgroup' do subject { delete api("/groups/#{group.id}", user), params: params } - it_behaves_like 'does not immediately enqueues the job to delete the group', '`permanently_remove` option is only available for subgroups.' + context 'forbidden by the :disallow_immediate_deletion feature flag' do + it_behaves_like 'does not immediately enqueues the job to delete the group', + '`permanently_remove` option is not available anymore (behind the :disallow_immediate_deletion feature flag).' + end + + context 'when the :disallow_immediate_deletion feature flag is disabled' do + before do + stub_feature_flags(disallow_immediate_deletion: false) + end + + it_behaves_like 'does not immediately enqueues the job to delete the group', '`permanently_remove` option is only available for subgroups.' + end end end diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index d262f862822ce1c9679db0a8155598053a0f1172..68490826cf62954e1f8698af6fa17971abfa4d76 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -5602,34 +5602,46 @@ def request params.merge!(permanently_remove: true) end - context 'when project is already marked for deletion' do + context 'forbidden by the :disallow_immediate_deletion feature flag' do + let(:error_message) { '`permanently_remove` option is not available anymore (behind the :disallow_immediate_deletion feature flag).' } + + it_behaves_like 'immediately delete project error' + end + + context 'when the :disallow_immediate_deletion feature flag is disabled' do before do - project.update!(archived: true, marked_for_deletion_at: 1.day.ago, deleting_user: user) + stub_feature_flags(disallow_immediate_deletion: false) end - context 'with correct project full path' do - before do - params.merge!(full_path: project.full_path) - end + context 'when project is not marked for deletion' do + let(:error_message) { 'Project must be marked for deletion first.' } - it_behaves_like 'deletes project immediately' + it_behaves_like 'immediately delete project error' end - context 'with incorrect project full path' do - let(:error_message) { '`full_path` is incorrect. You must enter the complete path for the project.' } - + context 'when project is already marked for deletion' do before do - params.merge!(full_path: "#{project.full_path}-wrong-path") + project.update!(archived: true, marked_for_deletion_at: 1.day.ago, deleting_user: user) end - it_behaves_like 'immediately delete project error' - end - end + context 'with correct project full path' do + before do + params.merge!(full_path: project.full_path) + end - context 'when project is not marked for deletion' do - let(:error_message) { 'Project must be marked for deletion first.' } + it_behaves_like 'deletes project immediately' + end - it_behaves_like 'immediately delete project error' + context 'with incorrect project full path' do + let(:error_message) { '`full_path` is incorrect. You must enter the complete path for the project.' } + + before do + params.merge!(full_path: "#{project.full_path}-wrong-path") + end + + it_behaves_like 'immediately delete project error' + end + end end end end diff --git a/spec/requests/organizations/groups_controller_spec.rb b/spec/requests/organizations/groups_controller_spec.rb index 9cf30c799752e936e78b9fc522c1558144e910c0..1dd4974169a6080c216e50b669eaf181720f5cce 100644 --- a/spec/requests/organizations/groups_controller_spec.rb +++ b/spec/requests/organizations/groups_controller_spec.rb @@ -298,17 +298,39 @@ context 'when group is already marked for deletion' do before do - create(:group_deletion_schedule, group: group, marked_for_deletion_on: Date.current) + create(:group_deletion_schedule, group: group) end context 'when permanently_remove param is set' do - it 'deletes the group immediately' do - expect(GroupDestroyWorker).to receive(:perform_async) + let(:params) { { permanently_remove: true } } - delete groups_organization_path(organization, id: group.to_param, permanently_remove: true) + subject(:gitlab_request) do + delete groups_organization_path(organization, id: group.to_param), params: params, as: :json + end + + describe 'forbidden by the :disallow_immediate_deletion feature flag' do + it 'returns error' do + Sidekiq::Testing.fake! do + expect { gitlab_request }.not_to change { GroupDestroyWorker.jobs.size } + end + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when the :disallow_immediate_deletion feature flag is disabled' do + before do + stub_feature_flags(disallow_immediate_deletion: false) + end + + it 'deletes the group immediately' do + expect(GroupDestroyWorker).to receive(:perform_async) + + gitlab_request - expect(response).to have_gitlab_http_status(:ok) - expect(json_response['message']).to include "Group '#{group.name}' is being deleted." + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['message']).to include "Group '#{group.name}' is being deleted." + end end end