Commit fe93a9b4 authored by Rémy Coutable's avatar Rémy Coutable Committed by Ruben Davila

Merge branch '18028-respect-fork-project' into 'security'

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.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/18028

See merge request !1996
parent 26b47b24
...@@ -20,6 +20,7 @@ v 8.12.2 (unreleased) ...@@ -20,6 +20,7 @@ v 8.12.2 (unreleased)
- Make JWT messages Docker-compatible - Make JWT messages Docker-compatible
- Fix an issue with the "Commits" section of the cycle analytics summary. !6513 - Fix an issue with the "Commits" section of the cycle analytics summary. !6513
- Fix duplicate branch entry in the merge request version compare dropdown - Fix duplicate branch entry in the merge request version compare dropdown
- Respect the fork_project permission when forking projects
v 8.12.1 v 8.12.1
- Fix a memory leak in HTML::Pipeline::SanitizationFilter::WHITELIST - Fix a memory leak in HTML::Pipeline::SanitizationFilter::WHITELIST
......
...@@ -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