Merge branch '36743-existing-repo-9-4' into 'security-9-4'

[9.4] Prevent project creation (blank, import or fork) when repository already exists on disk

See merge request gitlab/gitlabhq!2171

17 files changed, 143 insertions, 25 deletions

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index aec9feffd84..2a8235ffd35 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -400,12 +400,9 @@ db:rollback-mysql:
   <<: *except-docs
   variables:
     SIZE: "1"
-    SETUP_DB: "false"
-    RAILS_ENV: "development"
   script:
-    - git clone https://gitlab.com/gitlab-org/gitlab-test.git
-       /home/git/repositories/gitlab-org/gitlab-test.git
-    - bundle exec rake db:setup db:seed_fu
+    - cp -R db/fixtures/development db/fixtures/test
+    - bundle exec rake db:seed_fu
   artifacts:
     when: on_failure
     expire_in: 1d
diff --git a/app/models/project.rb b/app/models/project.rb
index f722d959b04..fce66d48d95 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -468,7 +468,7 @@ class Project < ActiveRecord::Base
   end
 
   def repository_storage_path
-    Gitlab.config.repositories.storages[repository_storage]['path']
+    Gitlab.config.repositories.storages[repository_storage].try(:[], 'path')
   end
 
   def team
@@ -569,7 +569,7 @@ class Project < ActiveRecord::Base
   end
 
   def valid_import_url?
-    valid? || errors.messages[:import_url].nil?
+    valid?(:import_url) || errors.messages[:import_url].nil?
   end
 
   def create_or_update_import_data(data: nil, credentials: nil)
@@ -1036,6 +1036,22 @@ class Project < ActiveRecord::Base
     end
   end
 
+  # Check if repository already exists on disk
+  def can_create_repository?
+    return false unless repository_storage_path
+
+    if gitlab_shell.exists?(repository_storage_path, "#{build_full_path}.git")
+      errors.add(:base, 'There is already a repository with that name on disk')
+      return false
+    end
+
+    true
+  end
+
+  def renamed?
+    persisted? && path_changed?
+  end
+
   def hook_attrs(backward: true)
     attrs = {
       name: name,
diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb
index e874a2d8789..2a7653320f1 100644
--- a/app/services/projects/create_service.rb
+++ b/app/services/projects/create_service.rb
@@ -111,7 +111,7 @@ module Projects
       Project.transaction do
         @project.create_or_update_import_data(data: import_data[:data], credentials: import_data[:credentials]) if import_data
 
-        if @project.save && !@project.import?
+        if @project.valid? && @project.can_create_repository? && @project.save && !@project.import?
           raise 'Failed to create repository' unless @project.create_repository
         end
       end
diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb
index d81035e4eba..ca02b456ae5 100644
--- a/app/services/projects/update_service.rb
+++ b/app/services/projects/update_service.rb
@@ -13,7 +13,13 @@ module Projects
         project.change_head(params[:default_branch])
       end
 
-      if project.update_attributes(params.except(:default_branch))
+      project.assign_attributes(params.except(:default_branch))
+
+      if project.renamed? && !project.can_create_repository?
+        return error('Cannot rename project because there is already a repository with that new name on disk')
+      end
+
+      if project.save
         if project.previous_changes.include?('path')
           project.rename_repo
         else
diff --git a/db/fixtures/development/04_project.rb b/db/fixtures/development/04_project.rb
index 6553c5d457a..b35797502df 100644
--- a/db/fixtures/development/04_project.rb
+++ b/db/fixtures/development/04_project.rb
@@ -75,7 +75,7 @@ Sidekiq::Testing.inline! do
         project.send(:_run_after_commit_queue)
       end
 
-      if project.valid? && project.valid_repo?
+      if project.errors.messages.empty? && project.valid_repo?
         print '.'
       else
         puts project.errors.full_messages
diff --git a/features/steps/shared/group.rb b/features/steps/shared/group.rb
index de119f2c6c0..03bc7e798e0 100644
--- a/features/steps/shared/group.rb
+++ b/features/steps/shared/group.rb
@@ -36,14 +36,12 @@ module SharedGroup
   protected
 
   def is_member_of(username, groupname, role)
-    @project_count ||= 0
     user = User.find_by(name: username) || create(:user, name: username)
     group = Group.find_by(name: groupname) || create(:group, name: groupname)
     group.add_user(user, role)
-    project ||= create(:project, :repository, namespace: group, path: "project#{@project_count}")
+    project ||= create(:project, :repository, namespace: group)
     create(:closed_issue_event, project: project)
     project.team << [user, :master]
-    @project_count += 1
   end
 
   def owned_group
diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb
index 1bb2db11e7f..c48936e1f53 100644
--- a/spec/factories/projects.rb
+++ b/spec/factories/projects.rb
@@ -162,8 +162,6 @@ FactoryGirl.define do
   # Test repository source can be found at
   # https://gitlab.com/gitlab-org/gitlab-test
   factory :project, parent: :empty_project do
-    path { 'gitlabhq' }
-
     test_repo
 
     transient do
diff --git a/spec/features/projects/import_export/import_file_spec.rb b/spec/features/projects/import_export/import_file_spec.rb
index 3c94272b605..a3f612aa888 100644
--- a/spec/features/projects/import_export/import_file_spec.rb
+++ b/spec/features/projects/import_export/import_file_spec.rb
@@ -3,6 +3,8 @@ require 'spec_helper'
 feature 'Import/Export - project import integration test', feature: true, js: true do
   include Select2Helper
 
+  let(:gitlab_shell) { Gitlab::Shell.new }
+  let(:repository_storage_path) { Gitlab.config.repositories.storages['default']['path'] }
   let(:file) { File.join(Rails.root, 'spec', 'features', 'projects', 'import_export', 'test_project_export.tar.gz') }
   let(:export_path) { "#{Dir.tmpdir}/import_file_spec" }
 
@@ -12,6 +14,8 @@ feature 'Import/Export - project import integration test', feature: true, js: tr
 
   after(:each) do
     FileUtils.rm_rf(export_path, secure: true)
+    gitlab_shell.remove_repository(repository_storage_path, 'asd/test-project-path')
+    gitlab_shell.remove_repository(repository_storage_path, 'asd/test-project-path.wiki')
   end
 
   context 'when selecting the namespace' do
diff --git a/spec/helpers/blob_helper_spec.rb b/spec/helpers/blob_helper_spec.rb
index bd3a3d24b84..77969bd0932 100644
--- a/spec/helpers/blob_helper_spec.rb
+++ b/spec/helpers/blob_helper_spec.rb
@@ -95,13 +95,13 @@ describe BlobHelper do
     it 'returns a link with the proper route' do
       link = edit_blob_link(project, 'master', 'README.md')
 
-      expect(Capybara.string(link).find_link('Edit')[:href]).to eq('/gitlab/gitlabhq/edit/master/README.md')
+      expect(Capybara.string(link).find_link('Edit')[:href]).to eq("/#{project.full_path}/edit/master/README.md")
     end
 
     it 'returns a link with the passed link_opts on the expected route' do
       link = edit_blob_link(project, 'master', 'README.md', link_opts: { mr_id: 10 })
 
-      expect(Capybara.string(link).find_link('Edit')[:href]).to eq('/gitlab/gitlabhq/edit/master/README.md?mr_id=10')
+      expect(Capybara.string(link).find_link('Edit')[:href]).to eq("/#{project.full_path}/edit/master/README.md?mr_id=10")
     end
   end
 
diff --git a/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb b/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb
index 4a9c9a7fe34..ec58a2a3c93 100644
--- a/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb
+++ b/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb
@@ -13,7 +13,7 @@ describe Gitlab::Email::Handler::CreateIssueHandler, lib: true do
   let(:email_raw) { fixture_file('emails/valid_new_issue.eml') }
   let(:namespace) { create(:namespace, path: 'gitlabhq') }
 
-  let!(:project)  { create(:project, :public, :repository, namespace: namespace) }
+  let!(:project)  { create(:project, :public, namespace: namespace, path: 'gitlabhq') }
   let!(:user) do
     create(
       :user,
diff --git a/spec/lib/gitlab/email/message/repository_push_spec.rb b/spec/lib/gitlab/email/message/repository_push_spec.rb
index 7b3291b8315..ad2ae2d7514 100644
--- a/spec/lib/gitlab/email/message/repository_push_spec.rb
+++ b/spec/lib/gitlab/email/message/repository_push_spec.rb
@@ -4,7 +4,7 @@ describe Gitlab::Email::Message::RepositoryPush do
   include RepoHelpers
 
   let!(:group) { create(:group, name: 'my_group') }
-  let!(:project) { create(:project, :repository, name: 'my_project', namespace: group) }
+  let!(:project) { create(:project, :repository, namespace: group) }
   let!(:author) { create(:author, name: 'Author') }
 
   let(:message) do
@@ -38,7 +38,7 @@ describe Gitlab::Email::Message::RepositoryPush do
 
     describe '#project_name_with_namespace' do
       subject { message.project_name_with_namespace }
-      it { is_expected.to eq 'my_group / my_project' }
+      it { is_expected.to eq "#{group.name} / #{project.name}" }
     end
 
     describe '#author' do
diff --git a/spec/lib/gitlab/github_import/wiki_formatter_spec.rb b/spec/lib/gitlab/github_import/wiki_formatter_spec.rb
index 1bd29b8a563..875c1a57906 100644
--- a/spec/lib/gitlab/github_import/wiki_formatter_spec.rb
+++ b/spec/lib/gitlab/github_import/wiki_formatter_spec.rb
@@ -3,7 +3,7 @@ require 'spec_helper'
 describe Gitlab::GithubImport::WikiFormatter, lib: true do
   let(:project) do
     create(:project,
-           namespace: create(:namespace, path: 'gitlabhq'),
+           namespace: create(:namespace),
            import_url: 'https://xxx@github.com/gitlabhq/sample.gitlabhq.git')
   end
 
@@ -11,7 +11,7 @@ describe Gitlab::GithubImport::WikiFormatter, lib: true do
 
   describe '#path_with_namespace' do
     it 'appends .wiki to project path' do
-      expect(wiki.path_with_namespace).to eq 'gitlabhq/gitlabhq.wiki'
+      expect(wiki.path_with_namespace).to eq "#{project.full_path}.wiki"
     end
   end
 
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index ecff2ea77e8..49421cfc1d0 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -181,7 +181,7 @@ describe Project, models: true do
       end
     end
 
-    context 'repository storages inclussion' do
+    context 'repository storages inclusion' do
       let(:project2) { build(:empty_project, repository_storage: 'missing') }
 
       before do
diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb
index 40298dcb723..6e30bccb68f 100644
--- a/spec/services/projects/create_service_spec.rb
+++ b/spec/services/projects/create_service_spec.rb
@@ -1,11 +1,12 @@
 require 'spec_helper'
 
 describe Projects::CreateService, '#execute', services: true do
+  let(:gitlab_shell) { Gitlab::Shell.new }
   let(:user) { create :user }
   let(:opts) do
     {
-      name: "GitLab",
-      namespace: user.namespace
+      name: 'GitLab',
+      namespace_id: user.namespace.id
     }
   end
 
@@ -146,6 +147,41 @@ describe Projects::CreateService, '#execute', services: true do
       expect(project.owner).to eq(user)
       expect(project.namespace).to eq(user.namespace)
     end
+
+    context 'when another repository already exists on disk' do
+      let(:opts) do
+        {
+          name: 'Existing',
+          namespace_id: user.namespace.id
+        }
+      end
+
+      let(:repository_storage_path) { Gitlab.config.repositories.storages['default']['path'] }
+
+      before do
+        gitlab_shell.add_repository(repository_storage_path, "#{user.namespace.full_path}/existing")
+      end
+
+      after do
+        gitlab_shell.remove_repository(repository_storage_path, "#{user.namespace.full_path}/existing")
+      end
+
+      it 'does not allow to create project with same path' do
+        project = create_project(user, opts)
+
+        expect(project).to respond_to(:errors)
+        expect(project.errors.messages).to have_key(:base)
+        expect(project.errors.messages[:base].first).to match('There is already a repository with that name on disk')
+      end
+
+      it 'does not allow to import a project with the same path' do
+        project = create_project(user, opts.merge({ import_url: 'https://gitlab.com/gitlab-org/gitlab-test.git' }))
+
+        expect(project).to respond_to(:errors)
+        expect(project.errors.messages).to have_key(:base)
+        expect(project.errors.messages[:base].first).to match('There is already a repository with that name on disk')
+      end
+    end
   end
 
   context 'when there is an active service template' do
diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb
index 0df81f3abcb..68999e8daf4 100644
--- a/spec/services/projects/fork_service_spec.rb
+++ b/spec/services/projects/fork_service_spec.rb
@@ -1,6 +1,7 @@
 require 'spec_helper'
 
 describe Projects::ForkService, services: true do
+  let(:gitlab_shell) { Gitlab::Shell.new }
   describe 'fork by user' do
     before do
       @from_user = create(:user)
@@ -65,6 +66,26 @@ describe Projects::ForkService, services: true do
       end
     end
 
+    context 'repository already exists' do
+      let(:repository_storage_path) { Gitlab.config.repositories.storages['default']['path'] }
+
+      before do
+        gitlab_shell.add_repository(repository_storage_path, "#{@to_user.namespace.full_path}/#{@from_project.path}")
+      end
+
+      after do
+        gitlab_shell.remove_repository(repository_storage_path, "#{@to_user.namespace.full_path}/#{@from_project.path}")
+      end
+
+      it 'does not allow creation' do
+        to_project = fork_project(@from_project, @to_user)
+
+        expect(to_project).not_to be_persisted
+        expect(to_project.errors.messages).to have_key(:base)
+        expect(to_project.errors.messages[:base].first).to match('There is already a repository with that name on disk')
+      end
+    end
+
     context 'GitLab CI is enabled' do
       it "forks and enables CI for fork" do
         @from_project.enable_ci
diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb
index 441a5276c56..142ed7ddf01 100644
--- a/spec/services/projects/transfer_service_spec.rb
+++ b/spec/services/projects/transfer_service_spec.rb
@@ -1,6 +1,7 @@
 require 'spec_helper'
 
 describe Projects::TransferService, services: true do
+  let(:gitlab_shell) { Gitlab::Shell.new }
   let(:user) { create(:user) }
   let(:group) { create(:group) }
   let(:project) { create(:project, :repository, namespace: user.namespace) }
@@ -119,6 +120,25 @@ describe Projects::TransferService, services: true do
     it { expect(project.namespace).to eq(user.namespace) }
   end
 
+  context 'namespace which contains orphan repository with same projects path name' do
+    let(:repository_storage_path) { Gitlab.config.repositories.storages['default']['path'] }
+
+    before do
+      group.add_owner(user)
+      gitlab_shell.add_repository(repository_storage_path, "#{group.full_path}/#{project.path}")
+
+      @result = transfer_project(project, user, group)
+    end
+
+    after do
+      gitlab_shell.remove_repository(repository_storage_path, "#{group.full_path}/#{project.path}")
+    end
+
+    it { expect(@result).to eq false }
+    it { expect(project.namespace).to eq(user.namespace) }
+    it { expect(project.errors[:new_namespace]).to include('Cannot move project') }
+  end
+
   def transfer_project(project, user, new_namespace)
     Projects::TransferService.new(project, user).execute(new_namespace)
   end
diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb
index 3ee834748df..aab70ed745a 100644
--- a/spec/services/projects/update_service_spec.rb
+++ b/spec/services/projects/update_service_spec.rb
@@ -1,6 +1,7 @@
 require 'spec_helper'
 
 describe Projects::UpdateService, '#execute', :services do
+  let(:gitlab_shell) { Gitlab::Shell.new }
   let(:user) { create(:user) }
   let(:admin) { create(:admin) }
 
@@ -125,6 +126,27 @@ describe Projects::UpdateService, '#execute', :services do
     end
   end
 
+  context 'when renaming a project' do
+    let(:repository_storage_path) { Gitlab.config.repositories.storages['default']['path'] }
+
+    before do
+      gitlab_shell.add_repository(repository_storage_path, "#{user.namespace.full_path}/existing")
+    end
+
+    after do
+      gitlab_shell.remove_repository(repository_storage_path, "#{user.namespace.full_path}/existing")
+    end
+
+    it 'does not allow renaming when new path matches existing repository on disk' do
+      result = update_project(project, admin, path: 'existing')
+
+      expect(result).to include(status: :error)
+      expect(result[:message]).to include('Cannot rename project')
+      expect(project.errors.messages).to have_key(:base)
+      expect(project.errors.messages[:base]).to include('There is already a repository with that name on disk')
+    end
+  end
+
   context 'when passing invalid parameters' do
     it 'returns an error result when record cannot be updated' do
       result = update_project(project, admin, { name: 'foo&bar' })