From dbe08ae8aa9343536e1ab7418929b7c48d6490d7 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Tue, 3 Oct 2017 08:49:43 +0000 Subject: Merge branch '38202-cannot-rename-a-hashed-project' into 'master' Resolve "Cannot rename a hashed project" Closes #38202 See merge request gitlab-org/gitlab-ce!14428 # Conflicts: # app/models/project.rb # spec/services/projects/create_service_spec.rb # spec/services/projects/update_service_spec.rb --- app/models/project.rb | 20 ++++++- .../38202-cannot-rename-a-hashed-project.yml | 6 ++ spec/services/projects/create_service_spec.rb | 66 ++++++++++++++++------ spec/services/projects/update_service_spec.rb | 42 ++++++++++---- 4 files changed, 103 insertions(+), 31 deletions(-) create mode 100644 changelogs/unreleased/38202-cannot-rename-a-hashed-project.yml diff --git a/app/models/project.rb b/app/models/project.rb index 9bec7963130..7800054f410 100644 --- a/app/models/project.rb +++ b/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 } @@ -1022,7 +1023,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 @@ -1576,6 +1579,19 @@ class Project < ActiveRecord::Base end 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) diff --git a/changelogs/unreleased/38202-cannot-rename-a-hashed-project.yml b/changelogs/unreleased/38202-cannot-rename-a-hashed-project.yml new file mode 100644 index 00000000000..768e296fcd7 --- /dev/null +++ b/changelogs/unreleased/38202-cannot-rename-a-hashed-project.yml @@ -0,0 +1,6 @@ +--- +title: Does not check if an invariant hashed storage path exists on disk when renaming + projects. +merge_request: 14428 +author: +type: fixed diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index 7af7e170046..2cc4643777e 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/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,30 +159,59 @@ describe Projects::CreateService, '#execute' do } end - let(:repository_storage_path) { Gitlab.config.repositories.storages['default']['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_path, "#{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 diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 5920e79852b..3af9f63b434 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -151,22 +151,40 @@ describe Projects::UpdateService, '#execute' do 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 + 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 -- cgit v1.2.3