Merge branch '33257-prevent-accidental-deletions-via-soft-delete-for-groups-api' into 'master'

Resolve "Prevent accidental deletions via soft delete for groups" - API changes  (MR: 4/n)

See merge request gitlab-org/gitlab!19430

changelogs/unreleased/33257-prevent-accidental-deletions-via-soft-delete-for-groups-api.yml

+---
+title: Add API endpoints for 'soft-delete for groups' feature
+merge_request: 19430
+author:
+type: added

doc/api/groups.md

@@ -628,7 +628,12 @@ Feature.disable(:limit_projects_in_groups_api)
 
 ## Remove group
 
-Removes group with all projects inside. Only available to group owners and administrators.
+Only available to group owners and administrators.
+
+This endpoint either:
+
+- Removes group, and queues a background job to delete all projects in the group as well.
+- Since GitLab 12.8, on [Premium](https://about.gitlab.com/pricing/premium/) or higher tiers, marks a group for deletion. The deletion will happen 7 days later by default, but this can be changed in the [instance settings](../user/admin_area/settings/visibility_and_access_controls.md#default-deletion-adjourned-period-premium-only).
 
 ```
 DELETE /groups/:id
@@ -636,10 +641,27 @@ DELETE /groups/:id
 
 Parameters:
 
-- `id` (required) - The ID or path of a user group
+| Attribute       | Type           | Required | Description |
+| --------------- | -------------- | -------- | ----------- |
+| `id`            | integer/string | yes      | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) |
+
+The response will be `202 Accepted` if the user has authorization.
+
+## Restore group marked for deletion **(PREMIUM)**
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/33257) in GitLab 12.8.
+
+Restores a group marked for deletion.
+
+```plaintext
+POST /groups/:id/restore
+```
+
+Parameters:
 
-This will queue a background job to delete all projects in the group. The
-response will be a 202 Accepted if the user has authorization.
+| Attribute       | Type           | Required | Description |
+| --------------- | -------------- | -------- | ----------- |
+| `id`            | integer/string | yes      | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) |
 
 ## Search for group
 

doc/api/projects.md

@@ -1769,7 +1769,7 @@ This endpoint either:
 - Removes a project including all associated resources (issues, merge requests etc).
 - From GitLab 12.6 on Premium or higher tiers, marks a project for deletion. Actual
   deletion happens after number of days specified in
-  [instance settings](../user/admin_area/settings/visibility_and_access_controls.md#project-deletion-adjourned-period-premium-only).
+  [instance settings](../user/admin_area/settings/visibility_and_access_controls.md#default-deletion-adjourned-period-premium-only).
 
 ```
 DELETE /projects/:id
@@ -1781,6 +1781,8 @@ DELETE /projects/:id
 
 ## Restore project marked for deletion **(PREMIUM)**
 
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/32935) in GitLab 12.6.
+
 Restores project marked for deletion.
 
 ```

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 ONLY)** How many days after marking project for deletion it is actually removed. Value between 0 and 90.
+| `deletion_adjourned_period`      | integer          | no                                   | **(PREMIUM ONLY)** The number of days to wait before removing a project or group that is marked for deletion. Value must be 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. |

doc/user/admin_area/settings/visibility_and_access_controls.md

@@ -47,11 +47,13 @@ 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 ONLY)**
+## Default deletion adjourned period **(PREMIUM ONLY)**
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/32935) in GitLab 12.6.
 
-By default, project marked for deletion will be permanently removed after 7 days. This period may be changed.
+By default, a project or group marked for removal will be permanently removed after 7 days.
+This period may be changed, and setting this period to 0 will enable immediate removal
+of projects or groups.
 
 To change this period:
 

ee/app/models/ee/group.rb

@@ -279,6 +279,12 @@ module EE
       marked_for_deletion_on.present?
     end
 
+    def adjourned_deletion?
+      return false unless feature_available?(:adjourned_deletion_for_projects_and_groups)
+
+      ::Gitlab::CurrentSettings.deletion_adjourned_period > 0
+    end
+
     private
 
     def custom_project_templates_group_allowed

ee/lib/ee/api/entities.rb

@@ -66,6 +66,7 @@ module EE
           expose :checked_file_template_project_id,
                  as: :file_template_project_id,
                  if: ->(group, options) { group.feature_available?(:custom_file_templates_for_namespace) }
+          expose :marked_for_deletion_on, if: ->(group, _) { group.feature_available?(:adjourned_deletion_for_projects_and_groups) }
         end
       end
 

ee/lib/ee/api/groups.rb

@@ -12,7 +12,7 @@ module EE
           override :find_groups
           # rubocop: disable CodeReuse/ActiveRecord
           def find_groups(params, parent_id = nil)
-            super.preload(:ldap_group_links)
+            super.preload(:ldap_group_links, :deletion_schedule)
           end
           # rubocop: enable CodeReuse/ActiveRecord
 
@@ -54,6 +54,21 @@ module EE
             audit_log_finder_params = params.slice(:created_after, :created_before)
             audit_log_finder_params.merge(entity_type: group.class.name, entity_id: group.id)
           end
+
+          override :delete_group
+          def delete_group(group)
+            return super unless group.adjourned_deletion?
+
+            result = destroy_conditionally!(group) do |group|
+              ::Groups::MarkForDeletionService.new(group, current_user).execute
+            end
+
+            if result[:status] == :success
+              accepted!
+            else
+              render_api_error!(result[:message], 400)
+            end
+          end
         end
 
         resource :groups, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
@@ -104,6 +119,20 @@ module EE
               present audit_event, with: EE::API::Entities::AuditEvent
             end
           end
+
+          desc 'Restore a group.'
+          post ':id/restore' do
+            authorize! :admin_group, user_group
+            break not_found! unless user_group.feature_available?(:adjourned_deletion_for_projects_and_groups)
+
+            result = ::Groups::RestoreService.new(user_group, current_user).execute
+
+            if result[:status] == :success
+              present user_group, with: ::API::Entities::GroupDetail, current_user: current_user
+            else
+              render_api_error!(result[:message], 400)
+            end
+          end
         end
       end
     end

ee/spec/models/group_spec.rb

@@ -639,6 +639,46 @@ describe Group do
   describe '#marked_for_deletion?' do
     subject { group.marked_for_deletion? }
 
+    context 'adjourned deletion feature is available' do
+      before do
+        stub_licensed_features(adjourned_deletion_for_projects_and_groups: true)
+      end
+
+      context 'when the group is marked for adjourned deletion' do
+        before do
+          create(:group_deletion_schedule, group: group, marked_for_deletion_on: 1.day.ago)
+        end
+
+        it { is_expected.to be_truthy }
+      end
+
+      context 'when the group is not marked for adjourned deletion' do
+        it { is_expected.to be_falsey }
+      end
+    end
+
+    context 'adjourned deletion feature is not available' do
+      before do
+        stub_licensed_features(adjourned_deletion_for_projects_and_groups: false)
+      end
+
+      context 'when the group is marked for adjourned deletion' do
+        before do
+          create(:group_deletion_schedule, group: group, marked_for_deletion_on: 1.day.ago)
+        end
+
+        it { is_expected.to be_falsey }
+      end
+
+      context 'when the group is not marked for adjourned deletion' do
+        it { is_expected.to be_falsey }
+      end
+    end
+  end
+
+  describe '#adjourned_deletion?' do
+    subject { group.adjourned_deletion? }
+
     shared_examples_for 'returns false' do
       it { is_expected.to be_falsey }
     end
@@ -652,15 +692,19 @@ describe Group do
         stub_licensed_features(adjourned_deletion_for_projects_and_groups: true)
       end
 
-      context 'when the group is marked for adjourned deletion' do
+      context 'when adjourned deletion period is set to more than 0' do
         before do
-          create(:group_deletion_schedule, group: group, marked_for_deletion_on: 1.day.ago)
+          stub_application_setting(deletion_adjourned_period: 1)
         end
 
         it_behaves_like 'returns true'
       end
 
-      context 'when the group is not marked for adjourned deletion' do
+      context 'when adjourned deletion period is set to 0' do
+        before do
+          stub_application_setting(deletion_adjourned_period: 0)
+        end
+
         it_behaves_like 'returns false'
       end
     end
@@ -670,15 +714,19 @@ describe Group do
         stub_licensed_features(adjourned_deletion_for_projects_and_groups: false)
       end
 
-      context 'when the group is marked for adjourned deletion' do
+      context 'when adjourned deletion period is set to more than 0' do
         before do
-          create(:group_deletion_schedule, group: group, marked_for_deletion_on: 1.day.ago)
+          stub_application_setting(deletion_adjourned_period: 1)
         end
 
         it_behaves_like 'returns false'
       end
 
-      context 'when the group is not marked for adjourned deletion' do
+      context 'when adjourned deletion period is set to 0' do
+        before do
+          stub_application_setting(deletion_adjourned_period: 0)
+        end
+
         it_behaves_like 'returns false'
       end
     end

ee/spec/requests/api/groups_spec.rb

@@ -44,38 +44,66 @@ describe API::Groups do
   end
 
   describe 'GET /groups/:id' do
-    before do
-      create(:ip_restriction, group: private_group)
-      private_group.add_maintainer(user)
-    end
-
-    context 'when the group_ip_restriction feature is not available' do
+    context 'group_ip_restriction' do
       before do
-        stub_licensed_features(group_ip_restriction: false)
+        create(:ip_restriction, group: private_group)
+        private_group.add_maintainer(user)
       end
 
-      it 'returns 200' do
-        get api("/groups/#{private_group.id}", user)
+      context 'when the group_ip_restriction feature is not available' do
+        before do
+          stub_licensed_features(group_ip_restriction: false)
+        end
 
-        expect(response).to have_gitlab_http_status(200)
+        it 'returns 200' do
+          get api("/groups/#{private_group.id}", user)
+
+          expect(response).to have_gitlab_http_status(200)
+        end
       end
-    end
 
-    context 'when the group_ip_restriction feature is available' do
-      before do
-        stub_licensed_features(group_ip_restriction: true)
+      context 'when the group_ip_restriction feature is available' do
+        before do
+          stub_licensed_features(group_ip_restriction: true)
+        end
+
+        it 'returns 404 for request from ip not in the range' do
+          get api("/groups/#{private_group.id}", user)
+
+          expect(response).to have_gitlab_http_status(404)
+        end
+
+        it 'returns 200 for request from ip in the range' do
+          get api("/groups/#{private_group.id}", user), headers: { 'REMOTE_ADDR' => '192.168.0.0' }
+
+          expect(response).to have_gitlab_http_status(200)
+        end
       end
+    end
+
+    context 'marked_for_deletion_on attribute' do
+      context 'when feature is available' do
+        before do
+          stub_licensed_features(adjourned_deletion_for_projects_and_groups: true)
+        end
 
-      it 'returns 404 for request from ip not in the range' do
-        get api("/groups/#{private_group.id}", user)
+        it 'is exposed' do
+          get api("/groups/#{group.id}", user)
 
-        expect(response).to have_gitlab_http_status(404)
+          expect(json_response).to have_key 'marked_for_deletion_on'
+        end
       end
 
-      it 'returns 200 for request from ip in the range' do
-        get api("/groups/#{private_group.id}", user), headers: { 'REMOTE_ADDR' => '192.168.0.0' }
+      context 'when feature is not available' do
+        before do
+          stub_licensed_features(adjourned_deletion_for_projects_and_groups: false)
+        end
 
-        expect(response).to have_gitlab_http_status(200)
+        it 'is not exposed' do
+          get api("/groups/#{group.id}", user)
+
+          expect(json_response).not_to have_key 'marked_for_deletion_on'
+        end
       end
     end
   end
@@ -460,6 +488,138 @@ describe API::Groups do
     end
   end
 
+  describe "DELETE /groups/:id" do
+    subject { delete api("/groups/#{group.id}", user) }
+
+    shared_examples_for 'immediately enqueues the job to delete the group' do
+      it do
+        Sidekiq::Testing.fake! do
+          expect { subject }.to change(GroupDestroyWorker.jobs, :size).by(1)
+        end
+
+        expect(response).to have_gitlab_http_status(202)
+      end
+    end
+
+    context 'feature is available' do
+      before do
+        stub_licensed_features(adjourned_deletion_for_projects_and_groups: true)
+      end
+
+      context 'period for adjourned deletion is greater than 0' do
+        before do
+          stub_application_setting(deletion_adjourned_period: 1)
+        end
+
+        context 'success' do
+          it 'marks the group for adjourned deletion' do
+            subject
+            group.reload
+
+            expect(response).to have_gitlab_http_status(202)
+            expect(group.marked_for_deletion_on).to eq(Date.today)
+            expect(group.deleting_user).to eq(user)
+          end
+
+          it 'does not immediately enqueue the job to delete the group' do
+            expect { subject }.not_to change(GroupDestroyWorker.jobs, :size)
+          end
+        end
+
+        context 'failure' do
+          before do
+            allow(::Groups::MarkForDeletionService).to receive_message_chain(:new, :execute).and_return({ status: :error, message: 'error' })
+          end
+
+          it 'returns error' do
+            subject
+
+            expect(response).to have_gitlab_http_status(400)
+            expect(json_response['message']).to eq('error')
+          end
+        end
+      end
+
+      context 'period of adjourned deletion is set to 0' do
+        before do
+          stub_application_setting(deletion_adjourned_period: 0)
+        end
+
+        it_behaves_like 'immediately enqueues the job to delete the group'
+      end
+    end
+
+    context 'feature is not available' do
+      before do
+        stub_licensed_features(adjourned_deletion_for_projects_and_groups: false)
+      end
+
+      it_behaves_like 'immediately enqueues the job to delete the group'
+    end
+  end
+
+  describe "POST /groups/:id/restore" do
+    let(:group) do
+      create(:group_with_deletion_schedule,
+      marked_for_deletion_on: 1.day.ago,
+      deleting_user: user)
+    end
+
+    subject { post api("/groups/#{group.id}/restore", user) }
+
+    context 'feature is available' do
+      before do
+        stub_licensed_features(adjourned_deletion_for_projects_and_groups: true)
+      end
+
+      context 'authenticated as owner' do
+        context 'restoring is successful' do
+          it 'restores the group to original state' do
+            subject
+
+            expect(response).to have_gitlab_http_status(201)
+            expect(json_response['marked_for_deletion_on']).to be_falsey
+          end
+        end
+
+        context 'restoring fails' do
+          before do
+            allow(::Groups::RestoreService).to receive_message_chain(:new, :execute).and_return({ status: :error, message: 'error' })
+          end
+
+          it 'returns error' do
+            subject
+
+            expect(response).to have_gitlab_http_status(400)
+            expect(json_response['message']).to eq('error')
+          end
+        end
+      end
+
+      context 'authenticated as user without access to the group' do
+        subject { post api("/groups/#{group.id}/restore", another_user) }
+
+        it 'returns 403' do
+          subject
+
+          expect(response).to have_gitlab_http_status(403)
+        end
+      end
+    end
+
+    context 'feature is not available' do
+      before do
+        stub_licensed_features(adjourned_deletion_for_projects_and_groups: false)
+      end
+
+      it 'returns 404' do
+        subject
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
+  end
+
   def ldap_sync(group_id, user, sidekiq_testing_method)
     Sidekiq::Testing.send(sidekiq_testing_method) do
       post api("/groups/#{group_id}/ldap_sync", user)

lib/api/groups.rb

@@ -92,6 +92,15 @@ module API
 
         present paginate(groups), options
       end
+
+      def delete_group(group)
+        Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/46285')
+        destroy_conditionally!(group) do |group|
+          ::Groups::DestroyService.new(group, current_user).async_execute
+        end
+
+        accepted!
+      end
     end
 
     resource :groups do
@@ -187,12 +196,7 @@ module API
         group = find_group!(params[:id])
         authorize! :admin_group, group
 
-        Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/46285')
-        destroy_conditionally!(group) do |group|
-          ::Groups::DestroyService.new(group, current_user).async_execute
-        end
-
-        accepted!
+        delete_group(group)
       end
 
       desc 'Get a list of projects in this group.' do