From 6773454e9af9d7d60eeb353a1919233db97620a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Fri, 18 Oct 2019 09:17:01 +0200 Subject: [PATCH 01/12] Add application settings Add new columns to projects and application settings Add attributes to import specs Import should ignore marked for deletion column Remove unneeded differences Remove unneeded differences Add specs to model Refactor specs to use specs helper Add cr remarks Add cr remarks Add db changes Add new columns to projects and application settings Add attributes to import specs Import should ignore marked for deletion column Add application setting With api entry and ui changes Fix validations name Remove unneeded differences Add strings to the file Add changelog entry Change name in migration to the proper one Remove unneeded differences Remove parts of code that by error was brought back to old version Add constant to application settings Add constant to migration Correct wording for more universal approach Update changelog entry Remove merge leftovers Remove rubocop offence --- doc/api/settings.md | 7 +++++-- .../settings/visibility_and_access_controls.md | 9 +++++++++ .../ee/admin/application_settings_controller.rb | 4 ++++ ee/app/helpers/ee/application_settings_helper.rb | 1 + ee/app/models/ee/application_setting.rb | 6 ++++++ ...t_project_deletion_adjourned_period_setting.html.haml | 9 +++++++++ ...-accidental-project-deletion-application-settings.yml | 5 +++++ ee/lib/ee/api/entities.rb | 1 + ee/lib/ee/api/helpers/settings_helpers.rb | 1 + ee/lib/ee/api/settings.rb | 4 ++++ .../admin/application_settings_controller_spec.rb | 7 +++++++ ee/spec/models/project_spec.rb | 2 ++ ee/spec/requests/api/settings_spec.rb | 7 +++++++ locale/gitlab.pot | 6 ++++++ 14 files changed, 67 insertions(+), 2 deletions(-) create mode 100644 ee/app/views/admin/application_settings/_default_project_deletion_adjourned_period_setting.html.haml create mode 100644 ee/changelogs/unreleased/32935-preventing-accidental-project-deletion-application-settings.yml diff --git a/doc/api/settings.md b/doc/api/settings.md index 185cce6353ee6d..33dd588f9efe66 100644 --- a/doc/api/settings.md +++ b/doc/api/settings.md @@ -72,14 +72,15 @@ Example response: ``` Users on GitLab [Premium or Ultimate](https://about.gitlab.com/pricing/) may also see -the `file_template_project_id` or the `geo_node_allowed_ips` parameters: +the `file_template_project_id`, `deletion_adjourned_period` or the `geo_node_allowed_ips` parameters: ```json { "id" : 1, "signup_enabled" : true, "file_template_project_id": 1, - "geo_node_allowed_ips": "0.0.0.0/0, ::/0" + "geo_node_allowed_ips": "0.0.0.0/0, ::/0", + "deletion_adjourned_period": 7, ... } ``` @@ -162,6 +163,7 @@ these parameters: - `file_template_project_id` - `geo_node_allowed_ips` - `geo_status_timeout` +- `deletion_adjourned_period` Example responses: **(PREMIUM ONLY)** @@ -292,6 +294,7 @@ are listed in the descriptions of the relevant settings. | `plantuml_enabled` | boolean | no | (**If enabled, requires:** `plantuml_url`) Enable PlantUML integration. Default is `false`. | | `plantuml_url` | string | required by: `plantuml_enabled` | The PlantUML instance URL for integration. | | `polling_interval_multiplier` | decimal | no | Interval multiplier used by endpoints that perform polling. Set to `0` to disable polling. | +| `deletion_adjourned_period` | integer | no | **(PREMIUM)** How many days after marking project for deletion it is actually removed. Value between 0 and 7. | `project_export_enabled` | boolean | no | Enable project export. | | `prometheus_metrics_enabled` | boolean | no | Enable Prometheus metrics. | | `protected_ci_variables` | boolean | no | Environment variables are protected by default. | diff --git a/doc/user/admin_area/settings/visibility_and_access_controls.md b/doc/user/admin_area/settings/visibility_and_access_controls.md index 95e4c45e56c3b6..490f14241e17af 100644 --- a/doc/user/admin_area/settings/visibility_and_access_controls.md +++ b/doc/user/admin_area/settings/visibility_and_access_controls.md @@ -48,6 +48,15 @@ To ensure only admin users can delete projects: 1. Check the **Default project deletion protection** checkbox. 1. Click **Save changes**. +## Project deletion adjourned period **(PREMIUM)** + +By default, project marked for deletion will be permanently removed after 7 days. This period may be changed. + +To change this period: + +1. Select the desired option. +1. Click **Save changes**. + ## Default project visibility To set the default visibility levels for new projects: diff --git a/ee/app/controllers/ee/admin/application_settings_controller.rb b/ee/app/controllers/ee/admin/application_settings_controller.rb index 5246fa77973f13..899a007c032635 100644 --- a/ee/app/controllers/ee/admin/application_settings_controller.rb +++ b/ee/app/controllers/ee/admin/application_settings_controller.rb @@ -38,6 +38,10 @@ def visible_application_setting_attributes attrs << :default_project_deletion_protection end + if License.feature_available?(:marking_project_for_deletion) + attrs << :deletion_adjourned_period + end + if License.feature_available?(:required_ci_templates) attrs << :required_instance_ci_template end diff --git a/ee/app/helpers/ee/application_settings_helper.rb b/ee/app/helpers/ee/application_settings_helper.rb index 4e270f7354693b..de8a42714e89d9 100644 --- a/ee/app/helpers/ee/application_settings_helper.rb +++ b/ee/app/helpers/ee/application_settings_helper.rb @@ -76,6 +76,7 @@ def self.possible_licensed_attributes email_additional_text file_template_project_id default_project_deletion_protection + deletion_adjourned_period ] end end diff --git a/ee/app/models/ee/application_setting.rb b/ee/app/models/ee/application_setting.rb index 29a901211d0007..13977b100fd259 100644 --- a/ee/app/models/ee/application_setting.rb +++ b/ee/app/models/ee/application_setting.rb @@ -11,6 +11,7 @@ module ApplicationSetting prepended do EMAIL_ADDITIONAL_TEXT_CHARACTER_LIMIT = 10_000 INSTANCE_REVIEW_MIN_USERS = 100 + DEFAULT_NUMBER_OF_DAYS_BEFORE_REMOVAL = 7 belongs_to :file_template_project, class_name: "Project" @@ -39,6 +40,10 @@ module ApplicationSetting presence: true, numericality: { only_integer: true, greater_than: 0 } + validates :deletion_adjourned_period, + presence: true, + numericality: { only_integer: true, greater_than_or_equal_to: 0, less_than_or_equal_to: 7 } + validates :elasticsearch_replicas, presence: true, numericality: { only_integer: true, greater_than: 0 } @@ -87,6 +92,7 @@ def defaults mirror_capacity_threshold: Settings.gitlab['mirror_capacity_threshold'], mirror_max_capacity: Settings.gitlab['mirror_max_capacity'], mirror_max_delay: Settings.gitlab['mirror_max_delay'], + deletion_adjourned_period: DEFAULT_NUMBER_OF_DAYS_BEFORE_REMOVAL, pseudonymizer_enabled: false, repository_size_limit: 0, slack_app_enabled: false, diff --git a/ee/app/views/admin/application_settings/_default_project_deletion_adjourned_period_setting.html.haml b/ee/app/views/admin/application_settings/_default_project_deletion_adjourned_period_setting.html.haml new file mode 100644 index 00000000000000..95719bacc1a4fe --- /dev/null +++ b/ee/app/views/admin/application_settings/_default_project_deletion_adjourned_period_setting.html.haml @@ -0,0 +1,9 @@ +- return unless License.feature_available?(:marking_project_for_deletion) + +- f = local_assigns.fetch(:form) + +.form-group + = f.label s_('Default deletion adjourned period'), class: 'label-bold' + = f.select :deletion_adjourned_period, options_for_select(0..7, @application_setting.deletion_adjourned_period), {}, class: 'form-control' + = f.label :deletion_adjourned_period, class: 'form-check-label' do + = _('How many days need to pass between marking entity for deletion and actual removing it.') diff --git a/ee/changelogs/unreleased/32935-preventing-accidental-project-deletion-application-settings.yml b/ee/changelogs/unreleased/32935-preventing-accidental-project-deletion-application-settings.yml new file mode 100644 index 00000000000000..3cd8e0d785e972 --- /dev/null +++ b/ee/changelogs/unreleased/32935-preventing-accidental-project-deletion-application-settings.yml @@ -0,0 +1,5 @@ +--- +title: Add application settings needed for soft-deletion +merge_request: 18790 +author: +type: added diff --git a/ee/lib/ee/api/entities.rb b/ee/lib/ee/api/entities.rb index 7b085495b3e5e1..fcdffbcda72748 100644 --- a/ee/lib/ee/api/entities.rb +++ b/ee/lib/ee/api/entities.rb @@ -202,6 +202,7 @@ module ApplicationSetting expose :email_additional_text, if: ->(_instance, _opts) { ::License.feature_available?(:email_additional_text) } expose :file_template_project_id, if: ->(_instance, _opts) { ::License.feature_available?(:custom_file_templates) } expose :default_project_deletion_protection, if: ->(_instance, _opts) { ::License.feature_available?(:default_project_deletion_protection) } + expose :deletion_adjourned_period, if: ->(_instance, _opts) { ::License.feature_available?(:marking_project_for_deletion) } end end diff --git a/ee/lib/ee/api/helpers/settings_helpers.rb b/ee/lib/ee/api/helpers/settings_helpers.rb index 5c128993de7c62..9be42c6818e407 100644 --- a/ee/lib/ee/api/helpers/settings_helpers.rb +++ b/ee/lib/ee/api/helpers/settings_helpers.rb @@ -31,6 +31,7 @@ module SettingsHelpers optional :email_additional_text, type: String, desc: 'Additional text added to the bottom of every email for legal/auditing/compliance reasons' optional :default_project_deletion_protection, type: Grape::API::Boolean, desc: 'Disable project owners ability to delete project' + optional :deletion_adjourned_period, type: Integer, desc: 'Number of days between marking project as deleted and actual removal' optional :help_text, type: String, desc: 'GitLab server administrator information' optional :repository_size_limit, type: Integer, desc: 'Size limit per repository (MB)' optional :file_template_project_id, type: Integer, desc: 'ID of project where instance-level file templates are stored.' diff --git a/ee/lib/ee/api/settings.rb b/ee/lib/ee/api/settings.rb index 12a4ee5cfa44d2..96e0d88de3c17a 100644 --- a/ee/lib/ee/api/settings.rb +++ b/ee/lib/ee/api/settings.rb @@ -27,6 +27,10 @@ def filter_attributes_using_license(attrs) attrs = attrs.except(:default_project_deletion_protection) end + unless License.feature_available?(:marking_project_for_deletion) + attrs = attrs.except(:deletion_adjourned_period) + end + attrs end end diff --git a/ee/spec/controllers/admin/application_settings_controller_spec.rb b/ee/spec/controllers/admin/application_settings_controller_spec.rb index c5c82af3c21415..b2a16add23ace2 100644 --- a/ee/spec/controllers/admin/application_settings_controller_spec.rb +++ b/ee/spec/controllers/admin/application_settings_controller_spec.rb @@ -104,6 +104,13 @@ it_behaves_like 'settings for licensed features' end + context 'project deletion adjourned period' do + let(:settings) { { deletion_adjourned_period: 6 } } + let(:feature) { :marking_project_for_deletion } + + it_behaves_like 'settings for licensed features' + end + context 'additional email footer' do let(:settings) { { email_additional_text: 'scary legal footer' } } let(:feature) { :email_additional_text } diff --git a/ee/spec/models/project_spec.rb b/ee/spec/models/project_spec.rb index 2fff009f2582e7..4d1e3428182456 100644 --- a/ee/spec/models/project_spec.rb +++ b/ee/spec/models/project_spec.rb @@ -2255,6 +2255,7 @@ def stub_default_url_options(host) context 'when number of days is set to more than 0' do it 'returns true' do stub_application_setting(deletion_adjourned_period: 1) + expect(project.adjourned_deletion?).to eq(true) end end @@ -2262,6 +2263,7 @@ def stub_default_url_options(host) context 'when number of days is set to 0' do it 'returns false' do stub_application_setting(deletion_adjourned_period: 0) + expect(project.adjourned_deletion?).to eq(false) end end diff --git a/ee/spec/requests/api/settings_spec.rb b/ee/spec/requests/api/settings_spec.rb index f5384c63b31d8a..d283990d0d2434 100644 --- a/ee/spec/requests/api/settings_spec.rb +++ b/ee/spec/requests/api/settings_spec.rb @@ -143,6 +143,13 @@ it_behaves_like 'settings for licensed features' end + context 'deletion adjourned period' do + let(:settings) { { deletion_adjourned_period: 5 } } + let(:feature) { :marking_project_for_deletion } + + it_behaves_like 'settings for licensed features' + end + context 'custom file template project' do let(:settings) { { file_template_project_id: project.id } } let(:feature) { :custom_file_templates } diff --git a/locale/gitlab.pot b/locale/gitlab.pot index d6e15fba348123..ed6c03b4563f3c 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -5509,6 +5509,9 @@ msgstr "" msgid "Default classification label" msgstr "" +msgid "Default deletion adjourned period" +msgstr "" + msgid "Default description template for issues" msgstr "" @@ -9291,6 +9294,9 @@ msgstr "" msgid "How it works" msgstr "" +msgid "How many days need to pass between marking entity for deletion and actual removing it." +msgstr "" + msgid "How many replicas each Elasticsearch shard has." msgstr "" -- GitLab From 5069075af5a9014d2d60b2494dccbfb66d07f52a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Thu, 7 Nov 2019 22:06:39 +0100 Subject: [PATCH 02/12] Add longer adjourned period possibility --- doc/api/settings.md | 2 +- ee/app/models/ee/application_setting.rb | 2 +- ..._default_project_deletion_adjourned_period_setting.html.haml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/api/settings.md b/doc/api/settings.md index 33dd588f9efe66..ad31edb37b1040 100644 --- a/doc/api/settings.md +++ b/doc/api/settings.md @@ -294,7 +294,7 @@ are listed in the descriptions of the relevant settings. | `plantuml_enabled` | boolean | no | (**If enabled, requires:** `plantuml_url`) Enable PlantUML integration. Default is `false`. | | `plantuml_url` | string | required by: `plantuml_enabled` | The PlantUML instance URL for integration. | | `polling_interval_multiplier` | decimal | no | Interval multiplier used by endpoints that perform polling. Set to `0` to disable polling. | -| `deletion_adjourned_period` | integer | no | **(PREMIUM)** How many days after marking project for deletion it is actually removed. Value between 0 and 7. +| `deletion_adjourned_period` | integer | no | **(PREMIUM)** How many days after marking project for deletion it is actually removed. Value between 0 and 90. | `project_export_enabled` | boolean | no | Enable project export. | | `prometheus_metrics_enabled` | boolean | no | Enable Prometheus metrics. | | `protected_ci_variables` | boolean | no | Environment variables are protected by default. | diff --git a/ee/app/models/ee/application_setting.rb b/ee/app/models/ee/application_setting.rb index 13977b100fd259..c926ea84fa908e 100644 --- a/ee/app/models/ee/application_setting.rb +++ b/ee/app/models/ee/application_setting.rb @@ -42,7 +42,7 @@ module ApplicationSetting validates :deletion_adjourned_period, presence: true, - numericality: { only_integer: true, greater_than_or_equal_to: 0, less_than_or_equal_to: 7 } + numericality: { only_integer: true, greater_than_or_equal_to: 0, less_than_or_equal_to: 90 } validates :elasticsearch_replicas, presence: true, diff --git a/ee/app/views/admin/application_settings/_default_project_deletion_adjourned_period_setting.html.haml b/ee/app/views/admin/application_settings/_default_project_deletion_adjourned_period_setting.html.haml index 95719bacc1a4fe..3c9e7c335edbe7 100644 --- a/ee/app/views/admin/application_settings/_default_project_deletion_adjourned_period_setting.html.haml +++ b/ee/app/views/admin/application_settings/_default_project_deletion_adjourned_period_setting.html.haml @@ -4,6 +4,6 @@ .form-group = f.label s_('Default deletion adjourned period'), class: 'label-bold' - = f.select :deletion_adjourned_period, options_for_select(0..7, @application_setting.deletion_adjourned_period), {}, class: 'form-control' + = f.select :deletion_adjourned_period, options_for_select(0..90, @application_setting.deletion_adjourned_period), {}, class: 'form-control' = f.label :deletion_adjourned_period, class: 'form-check-label' do = _('How many days need to pass between marking entity for deletion and actual removing it.') -- GitLab From b1271076e87ecfa618198294608b7756c21062ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Fri, 8 Nov 2019 11:37:29 +0100 Subject: [PATCH 03/12] Add call to partial --- .../admin/application_settings/_visibility_and_access.html.haml | 1 + 1 file changed, 1 insertion(+) diff --git a/app/views/admin/application_settings/_visibility_and_access.html.haml b/app/views/admin/application_settings/_visibility_and_access.html.haml index be5f1f4f9a828a..50a375dabe0fcf 100644 --- a/app/views/admin/application_settings/_visibility_and_access.html.haml +++ b/app/views/admin/application_settings/_visibility_and_access.html.haml @@ -9,6 +9,7 @@ = f.label s_('ProjectCreationLevel|Default project creation protection'), class: 'label-bold' = f.select :default_project_creation, options_for_select(Gitlab::Access.project_creation_options, @application_setting.default_project_creation), {}, class: 'form-control' = render_if_exists 'admin/application_settings/default_project_deletion_protection_setting', form: f + = render_if_exists 'admin/application_settings/default_project_deletion_adjourned_period_setting', form: f .form-group.visibility-level-setting = f.label :default_project_visibility, class: 'label-bold' = render('shared/visibility_radios', model_method: :default_project_visibility, form: f, selected_level: @application_setting.default_project_visibility, form_model: Project.new) -- GitLab From e1f2e329ae9c25b2294382d125021b2ccd01eadd Mon Sep 17 00:00:00 2001 From: Gosia Ksionek Date: Mon, 18 Nov 2019 10:58:57 +0000 Subject: [PATCH 04/12] Add services for marking projects for deletion and restoring --- .../projects/mark_for_deletion_service.rb | 36 ++++++++++ ee/app/services/projects/restore_service.rb | 34 +++++++++ .../mark_for_deletion_service_spec.rb | 69 +++++++++++++++++++ .../services/projects/restore_service_spec.rb | 47 +++++++++++++ locale/gitlab.pot | 3 + 5 files changed, 189 insertions(+) create mode 100644 ee/app/services/projects/mark_for_deletion_service.rb create mode 100644 ee/app/services/projects/restore_service.rb create mode 100644 ee/spec/services/projects/mark_for_deletion_service_spec.rb create mode 100644 ee/spec/services/projects/restore_service_spec.rb diff --git a/ee/app/services/projects/mark_for_deletion_service.rb b/ee/app/services/projects/mark_for_deletion_service.rb new file mode 100644 index 00000000000000..18164cfe41c6f7 --- /dev/null +++ b/ee/app/services/projects/mark_for_deletion_service.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module Projects + class MarkForDeletionService < BaseService + def execute + return if project.marked_for_deletion_at? + return unless project.feature_available?(:marking_project_for_deletion) + + result = ::Projects::UpdateService.new( + project, + current_user, + { archived: true, + marked_for_deletion_at: Time.now.utc, + deleting_user: current_user } + ).execute + log_event if result[:status] == :success + log_error(result[:message]) if result[:status] == :error + + result + end + + def log_event + log_audit_event + log_info("User #{current_user.id} marked project #{project.full_path} for deletion") + end + + def log_audit_event + ::AuditEventService.new( + current_user, + project, + action: :custom, + custom_message: "Project marked for deletion" + ).for_project.security_event + end + end +end diff --git a/ee/app/services/projects/restore_service.rb b/ee/app/services/projects/restore_service.rb new file mode 100644 index 00000000000000..96dd738aa77271 --- /dev/null +++ b/ee/app/services/projects/restore_service.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module Projects + class RestoreService < BaseService + def execute + return error(_('Project already deleted')) if project.pending_delete? + + result = ::Projects::UpdateService.new( + project, + current_user, + { archived: false, + marked_for_deletion_at: nil, + deleting_user: nil } + ).execute + log_event if result[:status] == :success + + result + end + + def log_event + log_audit_event + log_info("User #{current_user.id} restored project #{project.full_path}") + end + + def log_audit_event + ::AuditEventService.new( + current_user, + project, + action: :custom, + custom_message: "Project restored" + ).for_project.security_event + end + end +end diff --git a/ee/spec/services/projects/mark_for_deletion_service_spec.rb b/ee/spec/services/projects/mark_for_deletion_service_spec.rb new file mode 100644 index 00000000000000..34283e3167f886 --- /dev/null +++ b/ee/spec/services/projects/mark_for_deletion_service_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Projects::MarkForDeletionService do + let(:user) { create(:user) } + let(:marked_for_deletion_at) { nil } + let(:project) do + create(:project, + :repository, + namespace: user.namespace, + marked_for_deletion_at: marked_for_deletion_at) + end + + context 'with soft-delete feature turned on' do + before do + stub_licensed_features(marking_project_for_deletion: true) + end + + context 'marking project for deletion' do + before do + described_class.new(project, user).execute + end + + it 'marks project as archived and marked for deletion' do + expect(Project.unscoped.all).to include(project) + + expect(project.archived).to eq(true) + expect(project.marked_for_deletion_at).not_to be_nil + expect(project.deleting_user).to eq(user) + end + end + + context 'marking project for deletion once again' do + let(:marked_for_deletion_at) { 2.days.ago } + + before do + described_class.new(project, user).execute + end + + it 'does not change original date' do + expect(project.marked_for_deletion_at).to eq(marked_for_deletion_at.to_date) + end + end + + context 'audit events' do + it 'saves audit event' do + expect { described_class.new(project, user).execute } + .to change { AuditEvent.count }.by(1) + end + end + end + + context 'with soft-delete feature turned off' do + context 'marking project for deletion' do + before do + described_class.new(project, user).execute + end + + it 'does not change project attributes' do + expect(Project.all).to include(project) + + expect(project.archived).to eq(false) + expect(project.marked_for_deletion_at).to be_nil + expect(project.deleting_user).to be_nil + end + end + end +end diff --git a/ee/spec/services/projects/restore_service_spec.rb b/ee/spec/services/projects/restore_service_spec.rb new file mode 100644 index 00000000000000..acc85f32ecc9f3 --- /dev/null +++ b/ee/spec/services/projects/restore_service_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Projects::RestoreService do + let(:user) { create(:user) } + let(:pending_delete) { nil } + let(:project) do + create(:project, + :repository, + namespace: user.namespace, + marked_for_deletion_at: 1.day.ago, + deleting_user: user, + archived: true, + pending_delete: pending_delete) + end + + context 'restoring project' do + before do + described_class.new(project, user).execute + end + + it 'marks project as unarchived and not marked for deletion' do + expect(Project.unscoped.all).to include(project) + + expect(project.archived).to eq(false) + expect(project.marked_for_deletion_at).to be_nil + expect(project.deleting_user).to eq(nil) + end + end + + context 'restoring project already in process of removal' do + let(:deletion_date) { 2.days.ago } + let(:pending_delete) { true } + + it 'does not allow to restore' do + expect(described_class.new(project, user).execute).to include(status: :error) + end + end + + context 'audit events' do + it 'saves audit event' do + expect { described_class.new(project, user).execute } + .to change { AuditEvent.count }.by(1) + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index ed6c03b4563f3c..7e03a2c9f0d437 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -13613,6 +13613,9 @@ msgstr "" msgid "Project already created" msgstr "" +msgid "Project already deleted" +msgstr "" + msgid "Project and wiki repositories" msgstr "" -- GitLab From 3cd061e50a1a8ea7b63f52c19cf5a1e807b60614 Mon Sep 17 00:00:00 2001 From: Gosia Ksionek Date: Tue, 10 Dec 2019 12:45:20 +0000 Subject: [PATCH 05/12] Add workers for deleting projects Introduces workers used in soft-deletion of a project. --- config/initializers/1_settings.rb | 3 ++ config/sidekiq_queues.yml | 2 +- .../adjourned_project_deletion_worker.rb | 19 ++++++++ ...adjourned_projects_deletion_cron_worker.rb | 20 ++++++++ ee/app/workers/all_queues.yml | 2 + .../adjourned_project_deletion_worker_spec.rb | 28 +++++++++++ ...rned_projects_deletion_cron_worker_spec.rb | 47 +++++++++++++++++++ 7 files changed, 120 insertions(+), 1 deletion(-) create mode 100644 ee/app/workers/adjourned_project_deletion_worker.rb create mode 100644 ee/app/workers/adjourned_projects_deletion_cron_worker.rb create mode 100644 ee/spec/workers/adjourned_project_deletion_worker_spec.rb create mode 100644 ee/spec/workers/adjourned_projects_deletion_cron_worker_spec.rb diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index bb0c4696effd41..8e204c7a145e94 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -475,6 +475,9 @@ Settings.cron_jobs['clear_shared_runners_minutes_worker'] ||= Settingslogic.new({}) Settings.cron_jobs['clear_shared_runners_minutes_worker']['cron'] ||= '0 0 1 * *' Settings.cron_jobs['clear_shared_runners_minutes_worker']['job_class'] = 'ClearSharedRunnersMinutesWorker' + Settings.cron_jobs['adjourned_projects_deletion_cron_worker'] ||= Settingslogic.new({}) + Settings.cron_jobs['adjourned_projects_deletion_cron_worker']['cron'] ||= '0 4 * * *' + Settings.cron_jobs['adjourned_projects_deletion_cron_worker']['job_class'] = 'AdjournedProjectsDeletionCronWorker' Settings.cron_jobs['geo_file_download_dispatch_worker'] ||= Settingslogic.new({}) Settings.cron_jobs['geo_file_download_dispatch_worker']['cron'] ||= '*/1 * * * *' Settings.cron_jobs['geo_file_download_dispatch_worker']['job_class'] ||= 'Geo::FileDownloadDispatchWorker' diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 063a3cc2b5b484..68ad819d48bf8f 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -124,4 +124,4 @@ - [design_management_new_version, 1] - [epics, 2] - [personal_access_tokens, 1] - + - [adjourned_project_deletion, 1] diff --git a/ee/app/workers/adjourned_project_deletion_worker.rb b/ee/app/workers/adjourned_project_deletion_worker.rb new file mode 100644 index 00000000000000..9513b95001a393 --- /dev/null +++ b/ee/app/workers/adjourned_project_deletion_worker.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class AdjournedProjectDeletionWorker + include ApplicationWorker + include ExceptionBacktrace + + feature_category :authentication_and_authorization + + def perform(project_id) + project = Project.find(project_id) + user = project.deleting_user + + return unless user + + ::Projects::DestroyService.new(project, user).async_execute + rescue ActiveRecord::RecordNotFound => error + logger.error("Failed to delete project (#{project_id}): #{error.message}") + end +end diff --git a/ee/app/workers/adjourned_projects_deletion_cron_worker.rb b/ee/app/workers/adjourned_projects_deletion_cron_worker.rb new file mode 100644 index 00000000000000..c82a35ae37c2ca --- /dev/null +++ b/ee/app/workers/adjourned_projects_deletion_cron_worker.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class AdjournedProjectsDeletionCronWorker + include ApplicationWorker + include CronjobQueue + + INTERVAL = 5.minutes.to_i + + feature_category :authentication_and_authorization + + def perform + deletion_cutoff = Gitlab::CurrentSettings.deletion_adjourned_period.days.ago.to_date + + Project.aimed_for_deletion(deletion_cutoff).find_each(batch_size: 100).with_index do |project, index| # rubocop: disable CodeReuse/ActiveRecord + delay = index * INTERVAL + + AdjournedProjectDeletionWorker.perform_in(delay, project.id) + end + end +end diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index 5869ccf9eee93e..136d7cfc5dc34b 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -21,6 +21,7 @@ - cronjob:update_all_mirrors - cronjob:pseudonymizer - cronjob:update_max_seats_used_for_gitlab_com_subscriptions +- cronjob:adjourned_projects_deletion_cron - gcp_cluster:cluster_update_app - gcp_cluster:cluster_wait_for_app_update @@ -74,6 +75,7 @@ - new_epic - project_import_schedule - project_update_repository_storage +- adjourned_project_deletion - rebase - refresh_license_compliance_checks - repository_update_mirror diff --git a/ee/spec/workers/adjourned_project_deletion_worker_spec.rb b/ee/spec/workers/adjourned_project_deletion_worker_spec.rb new file mode 100644 index 00000000000000..cbe6f6b3d4b099 --- /dev/null +++ b/ee/spec/workers/adjourned_project_deletion_worker_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe AdjournedProjectDeletionWorker do + describe "#perform" do + subject(:worker) { described_class.new } + + let(:user) { create(:user)} + let(:project) { create(:project, deleting_user: user) } + let(:service) { instance_double(Projects::DestroyService) } + + it 'executes destroying project' do + expect(service).to receive(:async_execute) + expect(Projects::DestroyService).to receive(:new).with(project, user).and_return(service) + + worker.perform(project.id) + end + + it 'stops execution if user was deleted' do + project.update(deleting_user: nil) + + expect(Projects::DestroyService).not_to receive(:new) + + worker.perform(project.id) + end + end +end diff --git a/ee/spec/workers/adjourned_projects_deletion_cron_worker_spec.rb b/ee/spec/workers/adjourned_projects_deletion_cron_worker_spec.rb new file mode 100644 index 00000000000000..dddbcaf6ec80a0 --- /dev/null +++ b/ee/spec/workers/adjourned_projects_deletion_cron_worker_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe AdjournedProjectsDeletionCronWorker do + describe "#perform" do + subject(:worker) { described_class.new } + + let(:user) { create(:user)} + let(:marked_for_deletion_at) { 14.days.ago } + let!(:project_marked_for_deletion) { create(:project, marked_for_deletion_at: marked_for_deletion_at, deleting_user: user) } + + before do + create(:project) + create(:project, marked_for_deletion_at: 3.days.ago) + end + + it 'only schedules to delete projects marked for deletion before number of days from settings' do + expect(AdjournedProjectDeletionWorker).to receive(:perform_in).with(0, project_marked_for_deletion.id) + + worker.perform + end + + context 'marked for deletion exectly before number of days from settings' do + let(:marked_for_deletion_at) { 7.days.ago } + + it 'schedules to delete project ' do + expect(AdjournedProjectDeletionWorker).to receive(:perform_in).with(0, project_marked_for_deletion.id) + + worker.perform + end + end + + context 'when settings are set to not-default number of days' do + before do + create(:project, marked_for_deletion_at: 5.days.ago) + allow(Gitlab::CurrentSettings).to receive(:deletion_adjourned_period).and_return(4) + end + + it 'only schedules to delete projects marked for deletion before number of days from settings' do + expect(AdjournedProjectDeletionWorker).to receive(:perform_in).twice + + worker.perform + end + end + end +end -- GitLab From 153f7ef1bd7a7efc5f6d38612b0aec67033d1205 Mon Sep 17 00:00:00 2001 From: Gosia Ksionek Date: Tue, 10 Dec 2019 13:58:13 +0000 Subject: [PATCH 06/12] Add changes to api Add all methods in api to use new services responsible for marking for deletion and restoring --- doc/api/projects.md | 15 ++++ ee/lib/ee/api/entities.rb | 1 + ee/lib/ee/api/projects.rb | 32 ++++++++ ee/spec/requests/api/projects_spec.rb | 106 ++++++++++++++++++++++++++ lib/api/projects.rb | 14 ++-- 5 files changed, 163 insertions(+), 5 deletions(-) diff --git a/doc/api/projects.md b/doc/api/projects.md index f2b5ed65cdad9d..eb87bd54c63183 100644 --- a/doc/api/projects.md +++ b/doc/api/projects.md @@ -1722,6 +1722,21 @@ DELETE /projects/:id | --------- | ---- | -------- | ----------- | | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) | +On premium or higher tiers this endpoint marks project for deletion - actual removal happens +after number of days specified in instance settings, default number of days is 7. + +## Restore project marked for deletion **(PREMIUM)** + +Restores project marked for deletion. + +``` +POST /projects/:id/restore +``` + +| Attribute | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) | + ## Upload a file Uploads a file to the specified project to be used in an issue or merge request description, or a comment. diff --git a/ee/lib/ee/api/entities.rb b/ee/lib/ee/api/entities.rb index fcdffbcda72748..be089c1fdd5a1b 100644 --- a/ee/lib/ee/api/entities.rb +++ b/ee/lib/ee/api/entities.rb @@ -50,6 +50,7 @@ module Project expose :packages_enabled, if: ->(project, _) { project.feature_available?(:packages) } expose :service_desk_enabled, if: ->(project, _) { project.feature_available?(:service_desk) } expose :service_desk_address, if: ->(project, _) { project.feature_available?(:service_desk) } + expose :marked_for_deletion_at, if: ->(project, _) { project.feature_available?(:marking_project_for_deletion) } end end diff --git a/ee/lib/ee/api/projects.rb b/ee/lib/ee/api/projects.rb index 91f900c8f3f6e4..c7a80006ec81c5 100644 --- a/ee/lib/ee/api/projects.rb +++ b/ee/lib/ee/api/projects.rb @@ -6,6 +6,23 @@ module Projects extend ActiveSupport::Concern prepended do + resource :projects do + desc 'Restore a project' do + success Entities::Project + end + post ':id/restore' do + authorize!(:remove_project, user_project) + break not_found! unless user_project.feature_available?(:marking_project_for_deletion) + + result = ::Projects::RestoreService.new(user_project, current_user).execute + if result[:status] == :success + present user_project, with: ::API::Entities::Project, current_user: current_user + else + render_api_error!(result[:message], 400) + end + end + end + helpers do extend ::Gitlab::Utils::Override @@ -41,6 +58,21 @@ def verify_mirror_attrs!(project, attrs) attrs.delete(:import_data_attributes) end end + + override :delete_project + def delete_project(user_project) + return super unless user_project.adjourned_deletion? + + result = destroy_conditionally!(user_project) do + ::Projects::MarkForDeletionService.new(user_project, current_user, {}).execute + end + + if result[:status] == :success + accepted! + else + render_api_error!(result[:message], 400) + end + end end end end diff --git a/ee/spec/requests/api/projects_spec.rb b/ee/spec/requests/api/projects_spec.rb index 378ecd0c49ff16..0a6457da5b9e43 100644 --- a/ee/spec/requests/api/projects_spec.rb +++ b/ee/spec/requests/api/projects_spec.rb @@ -180,6 +180,24 @@ end end + describe 'marked_for_deletion attribute' do + it 'exposed when the feature is available' do + stub_licensed_features(marking_project_for_deletion: true) + + get api("/projects/#{project.id}", user) + + expect(json_response).to have_key 'marked_for_deletion_at' + end + + it 'not exposed when the feature is not available' do + stub_licensed_features(marking_project_for_deletion: false) + + get api("/projects/#{project.id}", user) + + expect(json_response).not_to have_key 'marked_for_deletion_at' + end + end + describe 'repository_storage attribute' do context 'when authenticated as an admin' do let(:admin) { create(:admin) } @@ -632,4 +650,92 @@ end end end + + describe 'POST /projects/:id/restore' do + context 'feature is available' do + before do + stub_licensed_features(marking_project_for_deletion: true) + end + + it 'restores project' do + project.update(archived: true, marked_for_deletion_at: 1.day.ago, deleting_user: user) + + post api("/projects/#{project.id}/restore", user) + + expect(response).to have_gitlab_http_status(201) + expect(json_response['archived']).to be_falsey + expect(json_response['marked_for_deletion_at']).to be_falsey + end + + it 'returns error if project is already being deleted' do + message = 'Error' + expect(::Projects::RestoreService).to receive_message_chain(:new, :execute).and_return({ status: :error, message: message }) + + post api("/projects/#{project.id}/restore", user) + + expect(response).to have_gitlab_http_status(400) + expect(json_response["message"]).to eq(message) + end + end + + context 'feature is not available' do + before do + stub_licensed_features(marking_project_for_deletion: false) + end + + it 'returns error' do + post api("/projects/#{project.id}/restore", user) + + expect(response).to have_gitlab_http_status(404) + end + end + end + + describe 'DELETE /projects/:id' do + context 'when feature is available' do + before do + stub_licensed_features(marking_project_for_deletion: true) + end + + it 'marks project for deletion' do + delete api("/projects/#{project.id}", user) + + expect(response).to have_gitlab_http_status(202) + expect(project.reload.marked_for_deletion?).to be_truthy + end + + it 'returns error if project cannot be marked for deletion' do + message = 'Error' + expect(::Projects::MarkForDeletionService).to receive_message_chain(:new, :execute).and_return({ status: :error, message: message }) + + delete api("/projects/#{project.id}", user) + + expect(response).to have_gitlab_http_status(400) + expect(json_response["message"]).to eq(message) + end + + context 'when instance setting is set to 0 days' do + it 'deletes project right away' do + allow(Gitlab::CurrentSettings).to receive(:deletion_adjourned_period).and_return(0) + delete api("/projects/#{project.id}", user) + + expect(response).to have_gitlab_http_status(202) + expect(project.reload.pending_delete).to eq(true) + end + end + end + + context 'when feature is not available' do + before do + stub_licensed_features(marking_project_for_deletion: false) + end + + it 'deletes project' do + delete api("/projects/#{project.id}", user) + + expect(response).to have_gitlab_http_status(202) + expect(project.reload.pending_delete).to eq(true) + end + end + end end diff --git a/lib/api/projects.rb b/lib/api/projects.rb index a1fce9e8b20fc6..d1f99ea49ce1d1 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -26,6 +26,14 @@ def apply_filters(projects) def verify_update_project_attrs!(project, attrs) end + + def delete_project(user_project) + destroy_conditionally!(user_project) do + ::Projects::DestroyService.new(user_project, current_user, {}).async_execute + end + + accepted! + end end helpers do @@ -404,11 +412,7 @@ def translate_params_for_compatibility(params) delete ":id" do authorize! :remove_project, user_project - destroy_conditionally!(user_project) do - ::Projects::DestroyService.new(user_project, current_user, {}).async_execute - end - - accepted! + delete_project(user_project) end desc 'Mark this project as forked from another' -- GitLab From 4d40b8ad1167583baa1a3f393571c6cc3ca1ba4c Mon Sep 17 00:00:00 2001 From: Gosia Ksionek Date: Tue, 10 Dec 2019 14:47:07 +0000 Subject: [PATCH 07/12] Add ui changes In routes, controllers and views Fix method names Add changelog entry Move EE specific code Move EE specific code Add cr remarks Remove obsolete variable Add services for marking projects for deletion and restoring With specs Fix specs Fix service Fix rubocop Add strings Add cr remarks Add cr remarks Regarding specs and logging --- .../groups/components/item_stats.vue | 7 ++ .../mixins/is_project_pending_removal.js | 7 ++ .../javascripts/groups/store/groups_store.js | 1 + app/serializers/group_child_entity.rb | 2 + .../_visibility_and_access.html.haml | 1 + app/views/admin/projects/_archived.html.haml | 3 + app/views/admin/projects/_projects.html.haml | 3 +- app/views/projects/_archived_notice.html.haml | 5 + app/views/projects/_remove.html.haml | 10 ++ app/views/projects/edit.html.haml | 27 +----- .../projects/settings/_archive.html.haml | 18 ++++ app/views/projects/show.html.haml | 7 +- app/views/shared/projects/_archived.html.haml | 3 + app/views/shared/projects/_project.html.haml | 3 +- .../mixins/is_project_pending_removal.js | 9 ++ ee/app/controllers/ee/projects_controller.rb | 34 +++++++ ee/app/helpers/ee/projects_helper.rb | 13 +++ ee/app/serializers/ee/group_child_entity.rb | 13 +++ .../views/admin/projects/_archived.html.haml | 6 ++ .../views/projects/_archived_notice.html.haml | 7 ++ .../projects/_marked_for_deletion_notice.haml | 5 + ee/app/views/projects/_remove.html.haml | 14 +++ .../projects/settings/_archive.html.haml | 2 + .../settings/_marked_for_removal.html.haml | 8 ++ .../projects/settings/_restore.html.haml | 13 +++ .../views/shared/projects/_archived.html.haml | 6 ++ ee/config/routes/project.rb | 2 + .../controllers/projects_controller_spec.rb | 92 +++++++++++++++++++ locale/gitlab.pot | 43 ++++++++- qa/qa/page/project/settings/advanced.rb | 3 + 30 files changed, 331 insertions(+), 36 deletions(-) create mode 100644 app/assets/javascripts/groups/mixins/is_project_pending_removal.js create mode 100644 app/views/admin/projects/_archived.html.haml create mode 100644 app/views/projects/_archived_notice.html.haml create mode 100644 app/views/projects/_remove.html.haml create mode 100644 app/views/projects/settings/_archive.html.haml create mode 100644 app/views/shared/projects/_archived.html.haml create mode 100644 ee/app/assets/javascripts/groups/mixins/is_project_pending_removal.js create mode 100644 ee/app/serializers/ee/group_child_entity.rb create mode 100644 ee/app/views/admin/projects/_archived.html.haml create mode 100644 ee/app/views/projects/_archived_notice.html.haml create mode 100644 ee/app/views/projects/_marked_for_deletion_notice.haml create mode 100644 ee/app/views/projects/_remove.html.haml create mode 100644 ee/app/views/projects/settings/_archive.html.haml create mode 100644 ee/app/views/projects/settings/_marked_for_removal.html.haml create mode 100644 ee/app/views/projects/settings/_restore.html.haml create mode 100644 ee/app/views/shared/projects/_archived.html.haml diff --git a/app/assets/javascripts/groups/components/item_stats.vue b/app/assets/javascripts/groups/components/item_stats.vue index 734a9a89c722b6..675552e6c2bb67 100644 --- a/app/assets/javascripts/groups/components/item_stats.vue +++ b/app/assets/javascripts/groups/components/item_stats.vue @@ -1,5 +1,6 @@