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.

9 files changed, 44 insertions, 7 deletions

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 }