Unverified Commit f006462d authored by Rémy Coutable's avatar Rémy Coutable Committed by Rémy Coutable

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
Signed-off-by: 's avatarRémy Coutable <remy@rymai.me>
parent 62ead92e
Please view this file on the master branch, on stable branches it's out of date.
v 8.11.8
- Respect the fork_project permission when forking projects
v 8.11.7
- Avoid conflict with admin labels when importing GitHub labels. !6158
- Restores `fieldName` to allow only string values in `gl_dropdown.js`. !6234
......@@ -16,6 +16,11 @@ module Projects
return @project
unless allowed_fork?(forked_from_project_id)
@project.errors.add(:forked_from_project_id, 'is forbidden')
return @project
# Set project name from path
if @project.name.present? && @project.path.present?
# if both name and path set - everything is ok
......@@ -72,6 +77,13 @@ module Projects
@project.errors.add(:namespace, "is not valid")
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)
def allowed_namespace?(user, namespace_id)
namespace = Namespace.find_by(id: namespace_id)
current_user.can?(:create_projects, namespace)
......@@ -17,6 +17,8 @@ module Projects
new_project = CreateService.new(current_user, new_params).execute
return new_project unless new_project.persisted?
......@@ -70,6 +70,7 @@ class Spinach::Features::ProjectFork < Spinach::FeatureSteps
step 'There is an existent fork of the "Shop" project' do
user = create(:user, name: 'Mike')
@project.team << [user, :reporter]
@forked_project = Projects::ForkService.new(@project, user).execute
......@@ -73,8 +73,8 @@ describe UsersController do
context 'forked project' do
let!(:project) { create(:project) }
let!(:forked_project) { Projects::ForkService.new(project, user).execute }
let(:project) { create(:project) }
let(:forked_project) { Projects::ForkService.new(project, user).execute }
before do
......@@ -11,7 +11,7 @@ describe ProjectsHelper do
describe "can_change_visibility_level?" do
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 }
it "returns false if there are no appropriate permissions" do
......@@ -6,6 +6,7 @@ describe ForkedProjectLink, "add link on fork" do
let(:user) { create(:user, namespace: namespace) }
before do
create(:project_member, :reporter, user: user, project: project_from)
@project_to = fork_project(project_from, user)
......@@ -12,7 +12,7 @@ describe API::API, api: true do
let(:project_user2) do
create(:project_member, :guest, user: user2, project: project)
create(:project_member, :reporter, user: user2, project: project)
describe 'POST /projects/fork/:id' do
......@@ -12,12 +12,26 @@ describe Projects::ForkService, services: true do
description: 'wow such project')
@to_namespace = create(:namespace)
@to_user = create(:user, namespace: @to_namespace)
@from_project.add_user(@to_user, :developer)
context 'fork project' do
context 'when forker is a guest' do
before do
@guest = create(:user)
@from_project.add_user(@guest, :guest)
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']) }
describe "successfully creates project in the user namespace" do
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.namespace).to eq(@to_user.namespace) }
it { expect(to_project.star_count).to be_zero }
......@@ -29,7 +43,9 @@ describe Projects::ForkService, services: true 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)
@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[:path]).to eq(['has already been taken'])
......@@ -81,18 +97,23 @@ describe Projects::ForkService, services: true do
@group = create(:group)
@group.add_user(@group_owner, GroupMember::OWNER)
@group.add_user(@developer, GroupMember::DEVELOPER)
@project.add_user(@developer, :developer)
@project.add_user(@group_owner, :developer)
@opts = { namespace: @group }
context 'fork project for group' do
it 'group owner successfully forks project into the group' do
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.namespace).to eq(@group)
expect(to_project.name).to eq(@project.name)
expect(to_project.path).to eq(@project.path)
expect(to_project.description).to eq(@project.description)
expect(to_project.star_count).to be_zero
expect(to_project.star_count).to be_zero
......@@ -445,7 +445,7 @@ describe SystemNoteService, services: true 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(: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