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

10 files changed, 45 insertions, 7 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 4cc047c77eb..02eb9406476 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -20,6 +20,7 @@ v 8.12.2 (unreleased)
   - Make JWT messages Docker-compatible
   - Fix an issue with the "Commits" section of the cycle analytics summary. !6513
   - Fix duplicate branch entry in the merge request version compare dropdown
+  - Respect the fork_project permission when forking projects
 
 v 8.12.1
   - Fix a memory leak in HTML::Pipeline::SanitizationFilter::WHITELIST
diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb
index be749ba4a1c..696fe3efe8f 100644
--- a/app/services/projects/create_service.rb
+++ b/app/services/projects/create_service.rb
@@ -15,6 +15,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
@@ -71,6 +76,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 a2de4dccece..a2b23ea6171 100644
--- a/app/services/projects/fork_service.rb
+++ b/app/services/projects/fork_service.rb
@@ -16,6 +16,8 @@ module Projects
       end
 
       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
       new_project.project_feature.update_attributes(builds_access_level: builds_access_level)
 
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 70032e7df94..bcd53440cb4 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 34f84f78952..e38d5745d44 100644
--- a/spec/requests/api/fork_spec.rb
+++ b/spec/requests/api/fork_spec.rb
@@ -18,7 +18,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 3d854a959f3..a3f16e2d945 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 }