diff options
author | Rémy Coutable <remy@gitlab.com> | 2016-09-28 09:42:33 +0000 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2016-09-28 17:47:27 +0200 |
commit | f006462d7b41381f83ba90463fc1b291034ee908 (patch) | |
tree | ac055c2cf501c2950815108dad84f8f527842c28 | |
parent | 62ead92ee808d32965b19128b529dce68d188f51 (diff) | |
download | gitlab-ce-f006462d7b41381f83ba90463fc1b291034ee908.tar.gz |
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: Rémy Coutable <remy@rymai.me>
-rw-r--r-- | CHANGELOG | 3 | ||||
-rw-r--r-- | app/services/projects/create_service.rb | 12 | ||||
-rw-r--r-- | app/services/projects/fork_service.rb | 2 | ||||
-rw-r--r-- | features/steps/project/fork.rb | 1 | ||||
-rw-r--r-- | spec/controllers/users_controller_spec.rb | 4 | ||||
-rw-r--r-- | spec/helpers/projects_helper_spec.rb | 2 | ||||
-rw-r--r-- | spec/models/forked_project_link_spec.rb | 1 | ||||
-rw-r--r-- | spec/requests/api/fork_spec.rb | 2 | ||||
-rw-r--r-- | spec/services/projects/fork_service_spec.rb | 25 | ||||
-rw-r--r-- | spec/services/system_note_service_spec.rb | 2 |
10 files changed, 47 insertions, 7 deletions
diff --git a/CHANGELOG b/CHANGELOG index c5bfa05508c..de08c7a8587 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,5 +1,8 @@ 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 diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb index 55956be2844..4ec975cf96c 100644 --- a/app/services/projects/create_service.rb +++ b/app/services/projects/create_service.rb @@ -16,6 +16,11 @@ module Projects return @project 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 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") 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) namespace = Namespace.find_by(id: namespace_id) current_user.can?(:create_projects, namespace) diff --git a/app/services/projects/fork_service.rb b/app/services/projects/fork_service.rb index de6dc38cc8e..90c72c4ceb1 100644 --- a/app/services/projects/fork_service.rb +++ b/app/services/projects/fork_service.rb @@ -17,6 +17,8 @@ module Projects end new_project = CreateService.new(current_user, new_params).execute + return new_project unless new_project.persisted? + new_project end diff --git a/features/steps/project/fork.rb b/features/steps/project/fork.rb index 8abeb5ee242..70dbd030003 100644 --- a/features/steps/project/fork.rb +++ b/features/steps/project/fork.rb @@ -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 end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 54a2d3d9460..19a8b1fe524 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -73,8 +73,8 @@ describe UsersController do end 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 sign_in(user) diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index 284b58d8d5c..5a202e11d0b 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -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 diff --git a/spec/models/forked_project_link_spec.rb b/spec/models/forked_project_link_spec.rb index 9c81d159cdf..1863581f57b 100644 --- a/spec/models/forked_project_link_spec.rb +++ b/spec/models/forked_project_link_spec.rb @@ -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) end diff --git a/spec/requests/api/fork_spec.rb b/spec/requests/api/fork_spec.rb index f802fcd2d2e..0378e3bda73 100644 --- a/spec/requests/api/fork_spec.rb +++ b/spec/requests/api/fork_spec.rb @@ -12,7 +12,7 @@ describe API::API, api: true do end let(:project_user2) do - create(:project_member, :guest, user: user2, project: project) + create(:project_member, :reporter, user: user2, project: project) end describe 'POST /projects/fork/:id' do diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index ef2036c78b1..64d15c0523c 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -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) end 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 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']) end @@ -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 } end 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 end end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 00427d6db2a..9f909c6224c 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -445,7 +445,7 @@ describe SystemNoteService, services: true do end 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 } |