Merge branch 'fix/rename-group-export-vuln' into 'security'

Fix export files not removed when a user takes over a namespace

See merge request !2051

app/models/namespace.rb

@@ -130,6 +130,8 @@ class Namespace < ActiveRecord::Base
     Gitlab::UploadsTransfer.new.rename_namespace(path_was, path)
+    remove_exports!
+
     # If repositories moved successfully we need to
     # send update instructions to users.
     # However we cannot allow rollback since we moved namespace dir
@@ -214,6 +216,8 @@ class Namespace < ActiveRecord::Base
         GitlabShellWorker.perform_in(5.minutes, :rm_namespace, repository_storage_path, new_path)
       end
     end
+
+    remove_exports!
   end
   def refresh_access_of_projects_invited_groups
@@ -226,4 +230,20 @@ class Namespace < ActiveRecord::Base
   def full_path_changed?
     path_changed? || parent_id_changed?
   end
+
+  def remove_exports!
+    Gitlab::Popen.popen(%W(find #{export_path} -not -path #{export_path} -delete))
+  end
+
+  def export_path
+    File.join(Gitlab::ImportExport.storage_path, full_path_was)
+  end
+
+  def full_path_was
+    if parent
+      parent.full_path + '/' + path_was
+    else
+      path_was
+    end
+  end
 end

spec/features/projects/import_export/namespace_export_file_spec.rb

+require 'spec_helper'
+
+feature 'Import/Export - Namespace export file cleanup', feature: true, js: true do
+  let(:export_path) { "#{Dir::tmpdir}/import_file_spec" }
+  let(:config_hash) { YAML.load_file(Gitlab::ImportExport.config_file).deep_stringify_keys }
+
+  let(:project) { create(:empty_project) }
+
+  background do
+    allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path)
+  end
+
+  after do
+    FileUtils.rm_rf(export_path, secure: true)
+  end
+
+  context 'admin user' do
+    before do
+      login_as(:admin)
+    end
+
+    context 'moving the namespace' do
+      scenario 'removes the export file' do
+        setup_export_project
+
+        old_export_path = project.export_path.dup
+
+        expect(File).to exist(old_export_path)
+
+        project.namespace.update(path: 'new_path')
+
+        expect(File).not_to exist(old_export_path)
+      end
+    end
+
+    context 'deleting the namespace' do
+      scenario 'removes the export file' do
+        setup_export_project
+
+        old_export_path = project.export_path.dup
+
+        expect(File).to exist(old_export_path)
+
+        project.namespace.destroy
+
+        expect(File).not_to exist(old_export_path)
+      end
+    end
+
+    def setup_export_project
+      visit edit_namespace_project_path(project.namespace, project)
+
+      expect(page).to have_content('Export project')
+
+      click_link 'Export project'
+
+      visit edit_namespace_project_path(project.namespace, project)
+
+      expect(page).to have_content('Download export')
+    end
+  end
+end

spec/models/namespace_spec.rb

@@ -117,6 +117,7 @@ describe Namespace, models: true do
       new_path = @namespace.path + "_new"
       allow(@namespace).to receive(:path_was).and_return(@namespace.path)
       allow(@namespace).to receive(:path).and_return(new_path)
+      expect(@namespace).to receive(:remove_exports!)
       expect(@namespace.move_dir).to be_truthy
     end
@@ -139,11 +140,17 @@ describe Namespace, models: true do
     let!(:project) { create(:project, namespace: namespace) }
     let!(:path) { File.join(Gitlab.config.repositories.storages.default, namespace.path) }
-    before { namespace.destroy }
-
     it "removes its dirs when deleted" do
+      namespace.destroy
+
       expect(File.exist?(path)).to be(false)
     end
+
+    it 'removes the exports folder' do
+      expect(namespace).to receive(:remove_exports!)
+
+      namespace.destroy
+    end
   end
   describe '.find_by_path_or_name' do