Commit 3ed80a01 authored by Nick Thomas's avatar Nick Thomas

Enforce the fork_project permission in Projects::CreateService

Projects::ForkService delegates to this service almost entirely, but needed
one small change so it would propagate create errors correctly.

CreateService#execute needs significant refactoring; it is now right at the
complexity limit set by Rubocop. I avoided doing so in this commit to keep the
diff as small as possible.

Several tests depend on the insecure behaviour of ForkService, so fi them up at
the same time.
parent fca610e5
...@@ -15,6 +15,11 @@ module Projects ...@@ -15,6 +15,11 @@ module Projects
return @project return @project
end end
unless allowed_fork?(forked_from_project_id)
@project.errors.add(:forked_from_project_id, 'is forbidden')
return @project
end
# Set project name from path # Set project name from path
if @project.name.present? && @project.path.present? if @project.name.present? && @project.path.present?
# if both name and path set - everything is ok # if both name and path set - everything is ok
...@@ -71,6 +76,13 @@ module Projects ...@@ -71,6 +76,13 @@ module Projects
@project.errors.add(:namespace, "is not valid") @project.errors.add(:namespace, "is not valid")
end end
def allowed_fork?(source_project_id)
return true if source_project_id.nil?
source_project = Project.find_by(id: source_project_id)
current_user.can?(:fork_project, source_project)
end
def allowed_namespace?(user, namespace_id) def allowed_namespace?(user, namespace_id)
namespace = Namespace.find_by(id: namespace_id) namespace = Namespace.find_by(id: namespace_id)
current_user.can?(:create_projects, namespace) current_user.can?(:create_projects, namespace)
......
...@@ -16,6 +16,8 @@ module Projects ...@@ -16,6 +16,8 @@ module Projects
end end
new_project = CreateService.new(current_user, new_params).execute new_project = CreateService.new(current_user, new_params).execute
return new_project unless new_project.persisted?
builds_access_level = @project.project_feature.builds_access_level builds_access_level = @project.project_feature.builds_access_level
new_project.project_feature.update_attributes(builds_access_level: builds_access_level) new_project.project_feature.update_attributes(builds_access_level: builds_access_level)
......
...@@ -70,6 +70,7 @@ class Spinach::Features::ProjectFork < Spinach::FeatureSteps ...@@ -70,6 +70,7 @@ class Spinach::Features::ProjectFork < Spinach::FeatureSteps
step 'There is an existent fork of the "Shop" project' do step 'There is an existent fork of the "Shop" project' do
user = create(:user, name: 'Mike') user = create(:user, name: 'Mike')
@project.team << [user, :reporter]
@forked_project = Projects::ForkService.new(@project, user).execute @forked_project = Projects::ForkService.new(@project, user).execute
end end
......
...@@ -73,8 +73,8 @@ describe UsersController do ...@@ -73,8 +73,8 @@ describe UsersController do
end end
context 'forked project' do context 'forked project' do
let!(:project) { create(:project) } let(:project) { create(:project) }
let!(:forked_project) { Projects::ForkService.new(project, user).execute } let(:forked_project) { Projects::ForkService.new(project, user).execute }
before do before do
sign_in(user) sign_in(user)
......
...@@ -11,7 +11,7 @@ describe ProjectsHelper do ...@@ -11,7 +11,7 @@ describe ProjectsHelper do
describe "can_change_visibility_level?" do describe "can_change_visibility_level?" do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:user) { create(:user) } let(:user) { create(:project_member, :reporter, user: create(:user), project: project).user }
let(:fork_project) { Projects::ForkService.new(project, user).execute } let(:fork_project) { Projects::ForkService.new(project, user).execute }
it "returns false if there are no appropriate permissions" do it "returns false if there are no appropriate permissions" do
......
...@@ -6,6 +6,7 @@ describe ForkedProjectLink, "add link on fork" do ...@@ -6,6 +6,7 @@ describe ForkedProjectLink, "add link on fork" do
let(:user) { create(:user, namespace: namespace) } let(:user) { create(:user, namespace: namespace) }
before do before do
create(:project_member, :reporter, user: user, project: project_from)
@project_to = fork_project(project_from, user) @project_to = fork_project(project_from, user)
end end
......
...@@ -18,7 +18,7 @@ describe API::API, api: true do ...@@ -18,7 +18,7 @@ describe API::API, api: true do
end end
let(:project_user2) do let(:project_user2) do
create(:project_member, :guest, user: user2, project: project) create(:project_member, :reporter, user: user2, project: project)
end end
describe 'POST /projects/fork/:id' do describe 'POST /projects/fork/:id' do
......
...@@ -12,12 +12,26 @@ describe Projects::ForkService, services: true do ...@@ -12,12 +12,26 @@ describe Projects::ForkService, services: true do
description: 'wow such project') description: 'wow such project')
@to_namespace = create(:namespace) @to_namespace = create(:namespace)
@to_user = create(:user, namespace: @to_namespace) @to_user = create(:user, namespace: @to_namespace)
@from_project.add_user(@to_user, :developer)
end end
context 'fork project' do context 'fork project' do
context 'when forker is a guest' do
before do
@guest = create(:user)
@from_project.add_user(@guest, :guest)
end
subject { fork_project(@from_project, @guest) }
it { is_expected.not_to be_persisted }
it { expect(subject.errors[:forked_from_project_id]).to eq(['is forbidden']) }
end
describe "successfully creates project in the user namespace" do describe "successfully creates project in the user namespace" do
let(:to_project) { fork_project(@from_project, @to_user) } let(:to_project) { fork_project(@from_project, @to_user) }
it { expect(to_project).to be_persisted }
it { expect(to_project.errors).to be_empty }
it { expect(to_project.owner).to eq(@to_user) } it { expect(to_project.owner).to eq(@to_user) }
it { expect(to_project.namespace).to eq(@to_user.namespace) } it { expect(to_project.namespace).to eq(@to_user.namespace) }
it { expect(to_project.star_count).to be_zero } it { expect(to_project.star_count).to be_zero }
...@@ -29,7 +43,9 @@ describe Projects::ForkService, services: true do ...@@ -29,7 +43,9 @@ describe Projects::ForkService, services: true do
it "fails due to validation, not transaction failure" do it "fails due to validation, not transaction failure" do
@existing_project = create(:project, creator_id: @to_user.id, name: @from_project.name, namespace: @to_namespace) @existing_project = create(:project, creator_id: @to_user.id, name: @from_project.name, namespace: @to_namespace)
@to_project = fork_project(@from_project, @to_user) @to_project = fork_project(@from_project, @to_user)
expect(@existing_project.persisted?).to be_truthy expect(@existing_project).to be_persisted
expect(@to_project).not_to be_persisted
expect(@to_project.errors[:name]).to eq(['has already been taken']) expect(@to_project.errors[:name]).to eq(['has already been taken'])
expect(@to_project.errors[:path]).to eq(['has already been taken']) expect(@to_project.errors[:path]).to eq(['has already been taken'])
end end
...@@ -81,18 +97,23 @@ describe Projects::ForkService, services: true do ...@@ -81,18 +97,23 @@ describe Projects::ForkService, services: true do
@group = create(:group) @group = create(:group)
@group.add_user(@group_owner, GroupMember::OWNER) @group.add_user(@group_owner, GroupMember::OWNER)
@group.add_user(@developer, GroupMember::DEVELOPER) @group.add_user(@developer, GroupMember::DEVELOPER)
@project.add_user(@developer, :developer)
@project.add_user(@group_owner, :developer)
@opts = { namespace: @group } @opts = { namespace: @group }
end end
context 'fork project for group' do context 'fork project for group' do
it 'group owner successfully forks project into the group' do it 'group owner successfully forks project into the group' do
to_project = fork_project(@project, @group_owner, @opts) to_project = fork_project(@project, @group_owner, @opts)
expect(to_project).to be_persisted
expect(to_project.errors).to be_empty
expect(to_project.owner).to eq(@group) expect(to_project.owner).to eq(@group)
expect(to_project.namespace).to eq(@group) expect(to_project.namespace).to eq(@group)
expect(to_project.name).to eq(@project.name) expect(to_project.name).to eq(@project.name)
expect(to_project.path).to eq(@project.path) expect(to_project.path).to eq(@project.path)
expect(to_project.description).to eq(@project.description) expect(to_project.description).to eq(@project.description)
expect(to_project.star_count).to be_zero expect(to_project.star_count).to be_zero
end end
end end
......
...@@ -445,7 +445,7 @@ describe SystemNoteService, services: true do ...@@ -445,7 +445,7 @@ describe SystemNoteService, services: true do
end end
context 'commit with cross-reference from fork' do context 'commit with cross-reference from fork' do
let(:author2) { create(:user) } let(:author2) { create(:project_member, :reporter, user: create(:user), project: project).user }
let(:forked_project) { Projects::ForkService.new(project, author2).execute } let(:forked_project) { Projects::ForkService.new(project, author2).execute }
let(:commit2) { forked_project.commit } let(:commit2) { forked_project.commit }
......
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