Merge branch 'fix-namespace-deletion' into 'master'

Fix bug where destroying a namespace would not always destroy projects
    
There is a race condition in DestroyGroupService now that projects are deleted asynchronously:
    
1. User attempts to delete group
2. DestroyGroupService iterates through all projects and schedules a Sidekiq job to delete each Project
3. DestroyGroupService destroys the Group, leaving all its projects without a namespace
4. Projects::DestroyService runs later but the can?(current_user,
    :remove_project) is `false` because the user no longer has permission to
    destroy projects with no namespace.
5. This leaves the project in pending_delete state with no namespace/group.
 
Projects without a namespace or group also adds another problem: it's not possible to destroy the container registry tags, since `container_registry_path_with_namespace` is the wrong value.

The fix is to destroy the group asynchronously and run `execute` directly on Projects::DestroyService.
 
Closes #17893

See merge request !4341

6 files changed, 114 insertions, 24 deletions

diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb
new file mode 100644
index 00000000000..0239aea47fb
--- /dev/null
+++ b/spec/controllers/admin/groups_controller_spec.rb
@@ -0,0 +1,24 @@
+require 'spec_helper'
+
+describe Admin::GroupsController do
+  let(:group) { create(:group) }
+  let(:project) { create(:project, namespace: group) }
+  let(:admin) { create(:admin) }
+
+  before do
+    sign_in(admin)
+    Sidekiq::Testing.fake!
+  end
+
+  describe 'DELETE #destroy' do
+    it 'schedules a group destroy' do
+      expect { delete :destroy, id: project.group.path }.to change(GroupDestroyWorker.jobs, :size).by(1)
+    end
+
+    it 'redirects to the admin group path' do
+      delete :destroy, id: project.group.path
+
+      expect(response).to redirect_to(admin_groups_path)
+    end
+  end
+end
diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb
index cd98fecd0c7..4ae6364207b 100644
--- a/spec/controllers/groups_controller_spec.rb
+++ b/spec/controllers/groups_controller_spec.rb
@@ -75,4 +75,33 @@ describe GroupsController do
       end
     end
   end
+
+  describe 'DELETE #destroy' do
+    context 'as another user' do
+      it 'returns 404' do
+        sign_in(create(:user))
+
+        delete :destroy, id: group.path
+
+        expect(response.status).to eq(404)
+      end
+    end
+
+    context 'as the group owner' do
+      before do
+        Sidekiq::Testing.fake!
+        sign_in(user)
+      end
+
+      it 'schedules a group destroy' do
+        expect { delete :destroy, id: group.path }.to change(GroupDestroyWorker.jobs, :size).by(1)
+      end
+
+      it 'redirects to the root path' do
+        delete :destroy, id: group.path
+
+        expect(response).to redirect_to(root_path)
+      end
+    end
+  end
 end
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index e0e041b4e15..0bbba64a6d5 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -564,12 +564,14 @@ describe API::API, api: true  do
   end
 
   describe "DELETE /users/:id" do
+    let!(:namespace) { user.namespace }
     before { admin }
 
     it "deletes user" do
       delete api("/users/#{user.id}", admin)
       expect(response).to have_http_status(200)
       expect { User.find(user.id) }.to raise_error ActiveRecord::RecordNotFound
+      expect { Namespace.find(namespace.id) }.to raise_error ActiveRecord::RecordNotFound
       expect(json_response['email']).to eq(user.email)
     end
 
diff --git a/spec/services/delete_user_service_spec.rb b/spec/services/delete_user_service_spec.rb
index 630458f9efc..418a12a83a9 100644
--- a/spec/services/delete_user_service_spec.rb
+++ b/spec/services/delete_user_service_spec.rb
@@ -9,9 +9,11 @@ describe DeleteUserService, services: true do
 
     context 'no options are given' do
       it 'deletes the user' do
-        DeleteUserService.new(current_user).execute(user)
+        user_data = DeleteUserService.new(current_user).execute(user)
 
-        expect { User.find(user.id)       }.to  raise_error(ActiveRecord::RecordNotFound)
+        expect { user_data['email'].to eq(user.email) }
+        expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound)
+        expect { Namespace.with_deleted.find(user.namespace.id) }.to raise_error(ActiveRecord::RecordNotFound)
       end
 
       it 'will delete the project in the near future' do
diff --git a/spec/services/destroy_group_service_spec.rb b/spec/services/destroy_group_service_spec.rb
index eca8ddd8ea4..da724643604 100644
--- a/spec/services/destroy_group_service_spec.rb
+++ b/spec/services/destroy_group_service_spec.rb
@@ -7,38 +7,52 @@ describe DestroyGroupService, services: true do
   let!(:gitlab_shell) { Gitlab::Shell.new }
   let!(:remove_path) { group.path + "+#{group.id}+deleted" }
 
-  context 'database records' do
-    before do
-      destroy_group(group, user)
+  shared_examples 'group destruction' do |async|
+    context 'database records' do
+      before do
+        destroy_group(group, user, async)
+      end
+
+      it { expect(Group.all).not_to include(group) }
+      it { expect(Project.all).not_to include(project) }
     end
 
-    it { expect(Group.all).not_to include(group) }
-    it { expect(Project.all).not_to include(project) }
-  end
+    context 'file system' do
+      context 'Sidekiq inline' do
+        before do
+          # Run sidekiq immediatly to check that renamed dir will be removed
+          Sidekiq::Testing.inline! { destroy_group(group, user, async) }
+        end
 
-  context 'file system' do
-    context 'Sidekiq inline' do
-      before do
-        # Run sidekiq immediatly to check that renamed dir will be removed
-        Sidekiq::Testing.inline! { destroy_group(group, user) }
+        it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey }
+        it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey }
       end
 
-      it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey }
-      it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey }
-    end
+      context 'Sidekiq fake' do
+        before do
+          # Dont run sidekiq to check if renamed repository exists
+          Sidekiq::Testing.fake! { destroy_group(group, user, async) }
+        end
 
-    context 'Sidekiq fake' do
-      before do
-        # Dont run sidekiq to check if renamed repository exists
-        Sidekiq::Testing.fake! { destroy_group(group, user) }
+        it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey }
+        it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_truthy }
       end
+    end
 
-      it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey }
-      it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_truthy }
+    def destroy_group(group, user, async)
+      if async
+        DestroyGroupService.new(group, user).async_execute
+      else
+        DestroyGroupService.new(group, user).execute
+      end
     end
   end
 
-  def destroy_group(group, user)
-    DestroyGroupService.new(group, user).execute
+  describe 'asynchronous delete' do
+    it_behaves_like 'group destruction', true
+  end
+
+  describe 'synchronous delete' do
+    it_behaves_like 'group destruction', false
   end
 end
diff --git a/spec/workers/group_destroy_worker_spec.rb b/spec/workers/group_destroy_worker_spec.rb
new file mode 100644
index 00000000000..4e4eaf9b2f7
--- /dev/null
+++ b/spec/workers/group_destroy_worker_spec.rb
@@ -0,0 +1,19 @@
+require 'spec_helper'
+
+describe GroupDestroyWorker do
+  let(:group) { create(:group) }
+  let(:user) { create(:admin) }
+  let!(:project) { create(:project, namespace: group) }
+
+  subject { GroupDestroyWorker.new }
+
+  describe "#perform" do
+    it "deletes the project" do
+      subject.perform(group.id, user.id)
+
+      expect(Group.all).not_to include(group)
+      expect(Project.all).not_to include(project)
+      expect(Dir.exist?(project.path)).to be_falsey
+    end
+  end
+end