diff options
author | Gabriel Mazetto <brodock@gmail.com> | 2017-08-23 02:19:35 +0300 |
---|---|---|
committer | Gabriel Mazetto <brodock@gmail.com> | 2017-08-28 22:08:22 +0300 |
commit | 29edaa1aa6f77333492dc366c9265d31cf88b699 (patch) | |
tree | 679aeefe5676af69242b228e7bf111760e8de83d | |
parent | ab974152dac919d5f9febb6399951bc15a3c1ddd (diff) |
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.
-rw-r--r-- | app/models/project.rb | 24 | ||||
-rw-r--r-- | app/services/projects/create_service.rb | 2 | ||||
-rw-r--r-- | app/services/projects/update_service.rb | 8 | ||||
-rw-r--r-- | db/fixtures/development/04_project.rb | 2 | ||||
-rw-r--r-- | features/steps/shared/group.rb | 4 | ||||
-rw-r--r-- | spec/factories/projects.rb | 2 | ||||
-rw-r--r-- | spec/features/projects/import_export/import_file_spec.rb | 3 | ||||
-rw-r--r-- | spec/helpers/blob_helper_spec.rb | 4 | ||||
-rw-r--r-- | spec/lib/container_registry/tag_spec.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/email/handler/create_issue_handler_spec.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/email/message/repository_push_spec.rb | 4 | ||||
-rw-r--r-- | spec/models/container_repository_spec.rb | 2 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 2 | ||||
-rw-r--r-- | spec/services/projects/create_service_spec.rb | 40 | ||||
-rw-r--r-- | spec/services/projects/fork_service_spec.rb | 22 | ||||
-rw-r--r-- | spec/services/projects/transfer_service_spec.rb | 20 | ||||
-rw-r--r-- | spec/services/projects/update_service_spec.rb | 22 |
17 files changed, 145 insertions, 20 deletions
diff --git a/app/models/project.rb b/app/models/project.rb index 9652a2a1c4d..2666bcb4438 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -473,7 +473,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 @@ -574,7 +574,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) @@ -991,6 +991,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, @@ -1399,6 +1415,10 @@ class Project < ActiveRecord::Base Projects::ForksCountService.new(self).count end + def legacy_storage? + self.storage_version.nil? + end + private def cross_namespace_reference?(from) diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb index 48578b6d9e5..4b2c3073a03 100644 --- a/app/services/projects/create_service.rb +++ b/app/services/projects/create_service.rb @@ -115,7 +115,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/factories/projects.rb b/spec/factories/projects.rb index 3f8e7030b1c..45462441c6f 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -62,8 +62,6 @@ FactoryGirl.define do # Test repository - https://gitlab.com/gitlab-org/gitlab-test trait :repository 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 a61c132799b..01a108327bb 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', js: true do include Select2Helper + let(:gitlab_shell) { Gitlab::Shell.new } + let(:repository_storage_path) { Gitlab.config.repositories.storages['default']['path'] } let(:user) { create(:user) } let(:file) { File.join(Rails.root, 'spec', 'features', 'projects', 'import_export', 'test_project_export.tar.gz') } let(:export_path) { "#{Dir.tmpdir}/import_file_spec" } @@ -14,6 +16,7 @@ feature 'Import/Export - project import integration test', js: true do 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 c654151564e..04620f6d88c 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/container_registry/tag_spec.rb b/spec/lib/container_registry/tag_spec.rb index e76463b5e7c..cb4ae3be525 100644 --- a/spec/lib/container_registry/tag_spec.rb +++ b/spec/lib/container_registry/tag_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe ContainerRegistry::Tag do let(:group) { create(:group, name: 'group') } - let(:project) { create(:project, :repository, path: 'test', group: group) } + let(:project) { create(:project, path: 'test', group: group) } let(:repository) do create(:container_repository, name: '', project: project) 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 bd36d1d309d..6568a0b1bb0 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 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 83c4d177cae..0ec1f931037 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/container_repository_spec.rb b/spec/models/container_repository_spec.rb index bae88cb1d24..e46945e301e 100644 --- a/spec/models/container_repository_spec.rb +++ b/spec/models/container_repository_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe ContainerRepository do let(:group) { create(:group, name: 'group') } - let(:project) { create(:project, :repository, path: 'test', group: group) } + let(:project) { create(:project, path: 'test', group: group) } let(:repository) do create(:container_repository, name: 'my_image', project: project) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index fb905f4aa62..98861c8c81a 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -181,7 +181,7 @@ describe Project do end end - context 'repository storages inclussion' do + context 'repository storages inclusion' do let(:project2) { build(: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 b0dc7488b5f..dbf3c49c67d 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' 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' 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 21c4b30734c..a6e0364d44c 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe Projects::ForkService do + let(:gitlab_shell) { Gitlab::Shell.new } + describe 'fork by user' do before do @from_user = create(:user) @@ -73,6 +75,26 @@ describe Projects::ForkService 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 2cb60cbcfc4..a14ed526f68 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 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 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) service = Projects::TransferService.new(project, user) diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index d945e0aa1f0..3036ced3ef8 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' do + let(:gitlab_shell) { Gitlab::Shell.new } let(:user) { create(:user) } let(:admin) { create(:admin) } @@ -125,6 +126,27 @@ describe Projects::UpdateService, '#execute' 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' }) |