Prevent new / renamed project from using a repository path that already exists on disk

There are some redundancies in the validation steps, and that is to
preserve current error messages behavior

Also few specs have to be changed in order to fix madness in validation
logic.

14 files changed, 138 insertions, 16 deletions

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..c0b7ee5e935 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.any? && 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/features/projects/import_export/import_file_spec.rb b/spec/features/projects/import_export/import_file_spec.rb
index 533ff4612ff..80c3999dc49 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,7 @@ 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')
   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..d2b5268ea29 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.path}" }
     end
 
     describe '#author' do
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' })