Merge branch '38202-cannot-rename-a-hashed-project' into 'master'

app/models/project.rb

@@ -64,6 +64,7 @@ class Project < ActiveRecord::Base
 
   # Storage specific hooks
   after_initialize :use_hashed_storage
+  after_create :check_repository_absence!
   after_create :ensure_storage_path_exists
   after_save :ensure_storage_path_exists, if: :namespace_id_changed?
 
@@ -228,7 +229,7 @@ class Project < ActiveRecord::Base
   validates :import_url, importable_url: true, if: [:external_import?, :import_url_changed?]
   validates :star_count, numericality: { greater_than_or_equal_to: 0 }
   validate :check_limit, on: :create
-  validate :check_repository_path_availability, on: [:create, :update], if: ->(project) { !project.persisted? || project.renamed? }
+  validate :check_repository_path_availability, on: :update, if: ->(project) { project.renamed? }
   validate :avatar_type,
     if: ->(project) { project.avatar.present? && project.avatar_changed? }
   validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
@@ -1025,7 +1026,9 @@ class Project < ActiveRecord::Base
 
     expires_full_path_cache # we need to clear cache to validate renames correctly
 
-    if gitlab_shell.exists?(repository_storage_path, "#{disk_path}.git")
+    # Check if repository with same path already exists on disk we can
+    # skip this for the hashed storage because the path does not change
+    if legacy_storage? && repository_with_same_path_already_exists?
       errors.add(:base, 'There is already a repository with that name on disk')
       return false
     end
@@ -1641,6 +1644,19 @@ class Project < ActiveRecord::Base
     Gitlab::ReferenceCounter.new(gl_repository(is_wiki: true)).value
   end
 
+  def check_repository_absence!
+    return if skip_disk_validation
+
+    if repository_storage_path.blank? || repository_with_same_path_already_exists?
+      errors.add(:base, 'There is already a repository with that name on disk')
+      throw :abort
+    end
+  end
+
+  def repository_with_same_path_already_exists?
+    gitlab_shell.exists?(repository_storage_path, "#{disk_path}.git")
+  end
+
   # set last_activity_at to the same as created_at
   def set_last_activity_at
     update_column(:last_activity_at, self.created_at)

changelogs/unreleased/38202-cannot-rename-a-hashed-project.yml

+---
+title: Does not check if an invariant hashed storage path exists on disk when renaming
+  projects.
+merge_request: 14428
+author:
+type: fixed

spec/services/projects/create_service_spec.rb

@@ -149,6 +149,9 @@ describe Projects::CreateService, '#execute' do
     end
 
     context 'when another repository already exists on disk' do
+      let(:repository_storage) { 'default' }
+      let(:repository_storage_path) { Gitlab.config.repositories.storages[repository_storage]['path'] }
+
       let(:opts) do
         {
           name: 'Existing',
@@ -156,31 +159,59 @@ describe Projects::CreateService, '#execute' do
         }
       end
 
-      let(:repository_storage) { 'default' }
-      let(:repository_storage_path) { Gitlab.config.repositories.storages[repository_storage]['path'] }
+      context 'with legacy storage' do
+        before do
+          gitlab_shell.add_repository(repository_storage, "#{user.namespace.full_path}/existing")
+        end
 
-      before do
-        gitlab_shell.add_repository(repository_storage, "#{user.namespace.full_path}/existing")
-      end
+        after do
+          gitlab_shell.remove_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 a project when path matches existing repository on disk' do
+          project = create_project(user, opts)
 
-      it 'does not allow to create project with same path' do
-        project = create_project(user, opts)
+          expect(project).not_to be_persisted
+          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
 
-        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')
+        it 'does not allow to import project when path matches existing repository on disk' do
+          project = create_project(user, opts.merge({ import_url: 'https://gitlab.com/gitlab-org/gitlab-test.git' }))
+
+          expect(project).not_to be_persisted
+          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
 
-      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' }))
+      context 'with hashed storage' do
+        let(:hash) { '6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b' }
+        let(:hashed_path) { '@hashed/6b/86/6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b' }
+
+        before do
+          stub_application_setting(hashed_storage_enabled: true)
+          allow(Digest::SHA2).to receive(:hexdigest) { hash }
+        end
+
+        before do
+          gitlab_shell.add_repository(repository_storage, hashed_path)
+        end
+
+        after do
+          gitlab_shell.remove_repository(repository_storage_path, hashed_path)
+        end
+
+        it 'does not allow to create a project when path matches existing repository on disk' 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')
+          expect(project).not_to be_persisted
+          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
   end

spec/services/projects/update_service_spec.rb

@@ -152,22 +152,40 @@ describe Projects::UpdateService, '#execute' do
     let(:repository_storage) { 'default' }
     let(:repository_storage_path) { Gitlab.config.repositories.storages[repository_storage]['path'] }
 
-    before do
-      gitlab_shell.add_repository(repository_storage, "#{user.namespace.full_path}/existing")
-    end
+    context 'with legacy storage' do
+      before do
+        gitlab_shell.add_repository(repository_storage, "#{user.namespace.full_path}/existing")
+      end
 
-    after do
-      gitlab_shell.remove_repository(repository_storage_path, "#{user.namespace.full_path}/existing")
+      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 match('There is already a repository with that name on disk')
+        expect(project).not_to be_valid
+        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
 
-    it 'does not allow renaming when new path matches existing repository on disk' do
-      result = update_project(project, admin, path: 'existing')
+    context 'with hashed storage' do
+      let(:project) { create(:project, :repository, creator: user, namespace: user.namespace) }
 
-      expect(result).to include(status: :error)
-      expect(result[:message]).to match('There is already a repository with that name on disk')
-      expect(project).not_to be_valid
-      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')
+      before do
+        stub_application_setting(hashed_storage_enabled: true)
+      end
+
+      it 'does not check if new path matches existing repository on disk' do
+        expect(project).not_to receive(:repository_with_same_path_already_exists?)
+
+        result = update_project(project, admin, path: 'existing')
+
+        expect(result).to include(status: :success)
+      end
     end
   end