Commit 8f178c42 authored by Gabriel Mazetto's avatar Gabriel Mazetto

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.
parent ce89c425
......@@ -222,6 +222,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 :can_create_repository?, on: [:create, :update], if: ->(project) { !project.persisted? || project.renamed? }
validate :avatar_type,
if: ->(project) { project.avatar.present? && project.avatar_changed? }
validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
......@@ -467,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
......@@ -582,7 +583,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)
......@@ -999,6 +1000,20 @@ class Project < ActiveRecord::Base
end
end
# Check if repository already exists on disk
def can_create_repository?
return false unless repository_storage_path
expires_full_path_cache # we need to clear cache to validate renames correctly
if gitlab_shell.exists?(repository_storage_path, "#{disk_path}.git")
errors.add(:base, 'There is already a repository with that name on disk')
return false
end
true
end
def create_repository(force: false)
# Forked import is handled asynchronously
return if forked? && !force
......@@ -1481,6 +1496,10 @@ class Project < ActiveRecord::Base
self.storage_version.nil?
end
def renamed?
persisted? && path_changed?
end
private
def storage
......
......@@ -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.save && !@project.import?
raise 'Failed to create repository' unless @project.create_repository
end
end
......
......@@ -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
......
......@@ -101,8 +101,6 @@ FactoryGirl.define do
# Test repository - https://gitlab.com/gitlab-org/gitlab-test
trait :repository do
path { 'gitlabhq' }
test_repo
transient do
......
......@@ -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
......
......@@ -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)
......
......@@ -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,
......
......@@ -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
......
......@@ -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)
......
......@@ -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
......
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
......
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
......
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)
......
require 'spec_helper'
describe Projects::UpdateService, '#execute' do
let(:gitlab_shell) { Gitlab::Shell.new }
let(:user) { create(:user) }
let(:admin) { create(:admin) }
......@@ -132,6 +133,28 @@ 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 match('Project could not be updated!')
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
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' })
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment