Merge branch '34648-restrict-forking-outside-of-gma-part-2' into 'master'

Reduce number of fork targets for group-managed accounts

Closes #34648

See merge request gitlab-org/gitlab!24698

app/controllers/projects/forks_controller.rb

@@ -3,6 +3,7 @@
 class Projects::ForksController < Projects::ApplicationController
   include ContinueParams
   include RendersMemberAccess
+  include Gitlab::Utils::StrongMemoize
 
   # Authorize
   before_action :whitelist_query_limiting, only: [:create]
@@ -10,6 +11,7 @@ class Projects::ForksController < Projects::ApplicationController
   before_action :authorize_download_code!
   before_action :authenticate_user!, only: [:new, :create]
   before_action :authorize_fork_project!, only: [:new, :create]
+  before_action :authorize_fork_namespace!, only: [:create]
 
   # rubocop: disable CodeReuse/ActiveRecord
   def index
@@ -37,18 +39,15 @@ class Projects::ForksController < Projects::ApplicationController
   # rubocop: enable CodeReuse/ActiveRecord
 
   def new
-    @namespaces = current_user.manageable_namespaces
-    @namespaces.delete(@project.namespace)
+    @namespaces = fork_service.valid_fork_targets
   end
 
   # rubocop: disable CodeReuse/ActiveRecord
   def create
-    namespace = Namespace.find(params[:namespace_key])
-
-    @forked_project = namespace.projects.find_by(path: project.path)
+    @forked_project = fork_namespace.projects.find_by(path: project.path)
     @forked_project = nil unless @forked_project && @forked_project.forked_from_project == project
 
-    @forked_project ||= ::Projects::ForkService.new(project, current_user, namespace: namespace).execute
+    @forked_project ||= fork_service.execute
 
     if !@forked_project.saved? || !@forked_project.forked?
       render :error
@@ -64,6 +63,22 @@ class Projects::ForksController < Projects::ApplicationController
 
   private
 
+  def fork_service
+    strong_memoize(:fork_service) do
+      ::Projects::ForkService.new(project, current_user, namespace: fork_namespace)
+    end
+  end
+
+  def fork_namespace
+    strong_memoize(:fork_namespace) do
+      Namespace.find(params[:namespace_key]) if params[:namespace_key].present?
+    end
+  end
+
+  def authorize_fork_namespace!
+    access_denied! unless fork_namespace && fork_service.valid_fork_target?
+  end
+
   def whitelist_query_limiting
     Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42335')
   end

app/finders/fork_targets_finder.rb

+# frozen_string_literal: true
+
+class ForkTargetsFinder
+  def initialize(project, user)
+    @project = project
+    @user = user
+  end
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def execute
+    ::Namespace.where(id: user.manageable_namespaces).where.not(id: project.namespace).sort_by_type
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
+
+  private
+
+  attr_reader :project, :user
+end
+
+ForkTargetsFinder.prepend_if_ee('EE::ForkTargetsFinder')

app/models/namespace.rb

@@ -68,6 +68,7 @@ class Namespace < ApplicationRecord
   after_destroy :rm_dir
 
   scope :for_user, -> { where('type IS NULL') }
+  scope :sort_by_type, -> { order(Gitlab::Database.nulls_first_order(:type)) }
 
   scope :with_statistics, -> do
     joins('LEFT JOIN project_statistics ps ON ps.namespace_id = namespaces.id')

app/services/projects/fork_service.rb

@@ -3,24 +3,25 @@
 module Projects
   class ForkService < BaseService
     def execute(fork_to_project = nil)
-      forked_project =
-        if fork_to_project
-          link_existing_project(fork_to_project)
-        else
-          fork_new_project
-        end
+      forked_project = fork_to_project ? link_existing_project(fork_to_project) : fork_new_project
 
       refresh_forks_count if forked_project&.saved?
 
       forked_project
     end
 
-    private
+    def valid_fork_targets
+      @valid_fork_targets ||= ForkTargetsFinder.new(@project, current_user).execute
+    end
 
-    def allowed_fork?
-      current_user.can?(:fork_project, @project)
+    def valid_fork_target?
+      return true if current_user.admin?
+
+      valid_fork_targets.include?(target_namespace)
     end
 
+    private
+
     def link_existing_project(fork_to_project)
       return if fork_to_project.forked?
 
@@ -30,6 +31,21 @@ module Projects
     end
 
     def fork_new_project
+      new_project = CreateService.new(current_user, new_fork_params).execute
+      return new_project unless new_project.persisted?
+
+      # Set the forked_from_project relation after saving to avoid having to
+      # reload the project to reset the association information and cause an
+      # extra query.
+      new_project.forked_from_project = @project
+
+      builds_access_level = @project.project_feature.builds_access_level
+      new_project.project_feature.update(builds_access_level: builds_access_level)
+
+      new_project
+    end
+
+    def new_fork_params
       new_params = {
         visibility_level:          allowed_visibility_level,
         description:               @project.description,
@@ -57,18 +73,11 @@ module Projects
 
       new_params.merge!(@project.object_pool_params)
 
-      new_project = CreateService.new(current_user, new_params).execute
-      return new_project unless new_project.persisted?
-
-      # Set the forked_from_project relation after saving to avoid having to
-      # reload the project to reset the association information and cause an
-      # extra query.
-      new_project.forked_from_project = @project
-
-      builds_access_level = @project.project_feature.builds_access_level
-      new_project.project_feature.update(builds_access_level: builds_access_level)
+      new_params
+    end
 
-      new_project
+    def allowed_fork?
+      current_user.can?(:fork_project, @project)
     end
 
     def fork_network

db/migrate/20200206141511_change_saml_provider_outer_forks_default.rb

+# frozen_string_literal: true
+
+class ChangeSamlProviderOuterForksDefault < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def up
+    change_column_null :saml_providers, :prohibited_outer_forks, false
+    change_column_default :saml_providers, :prohibited_outer_forks, true
+  end
+
+  def down
+    change_column_default :saml_providers, :prohibited_outer_forks, false
+    change_column_null :saml_providers, :prohibited_outer_forks, true
+  end
+end

db/schema.rb

@@ -3770,7 +3770,7 @@ ActiveRecord::Schema.define(version: 2020_02_13_220211) do
     t.string "sso_url", null: false
     t.boolean "enforced_sso", default: false, null: false
     t.boolean "enforced_group_managed_accounts", default: false, null: false
-    t.boolean "prohibited_outer_forks", default: false
+    t.boolean "prohibited_outer_forks", default: true, null: false
     t.index ["group_id"], name: "index_saml_providers_on_group_id"
   end
 

doc/user/group/saml_sso/index.md

@@ -64,7 +64,7 @@ We intend to add a similar SSO requirement for [Git and API activity](https://gi
 
 #### Group-managed accounts
 
-[Introduced in GitLab 12.1](https://gitlab.com/groups/gitlab-org/-/epics/709).
+> [Introduced in GitLab 12.1](https://gitlab.com/groups/gitlab-org/-/epics/709).
 
 When SSO is being enforced, groups can enable an additional level of protection by enforcing the creation of dedicated user accounts to access the group.
 
@@ -95,6 +95,14 @@ To access the Credentials inventory of a group, navigate to **{shield}** **Secur
 
 This feature is similar to the [Credentials inventory for self-managed instances](../../admin_area/credentials_inventory.md).
 
+##### Outer forks restriction for Group-managed accounts
+
+> [Introduced in GitLab 12.9](https://gitlab.com/gitlab-org/gitlab/issues/34648)
+
+Groups with enabled group-managed accounts can allow or disallow forking of projects outside of root group
+by using separate toggle. If forking is disallowed any project of given root group or its subgroups can be forked to
+a subgroup of the same root group only.
+
 #### Assertions
 
 When using group-managed accounts, the following user details need to be passed to GitLab as SAML

ee/app/finders/ee/fork_targets_finder.rb

+# frozen_string_literal: true
+
+module EE
+  module ForkTargetsFinder
+    extend ActiveSupport::Concern
+    extend ::Gitlab::Utils::Override
+
+    override :execute
+    # rubocop: disable CodeReuse/ActiveRecord
+    def execute
+      targets = super
+
+      root_group = project.group&.root_ancestor
+
+      return targets unless root_group&.saml_provider
+
+      if root_group.saml_provider.prohibited_outer_forks?
+        targets = targets.where(id: root_group.self_and_descendants)
+      end
+
+      targets
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+  end
+end

ee/changelogs/unreleased/34648-restrict-forking-outside-of-gma-part-2.yml

+---
+title: Add restrict outer forks functionality for group SAML
+merge_request: 24698
+author:
+type: added

ee/spec/features/groups/saml_providers_spec.rb

@@ -75,7 +75,7 @@ describe 'SAML provider settings' do
     end
 
     context 'with existing SAML provider' do
-      let!(:saml_provider) { create(:saml_provider, group: group) }
+      let!(:saml_provider) { create(:saml_provider, group: group, prohibited_outer_forks: false) }
 
       it 'allows provider to be disabled', :js do
         visit group_saml_providers_path(group)

ee/spec/finders/ee/fork_targets_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ForkTargetsFinder do
+  subject(:finder) { described_class.new(project, user) }
+
+  let(:project) { create :project, namespace: project_group }
+
+  describe '#execute' do
+    subject(:fork_targets) { finder.execute }
+
+    let(:user) { create :user, :group_managed, managing_group: project_group }
+
+    let(:outer_group) { create :group }
+    let(:inner_subgroup) { create(:group, :nested, parent: project_group) }
+
+    before do
+      project_group.add_reporter(user)
+      outer_group.add_owner(user)
+      inner_subgroup.add_owner(user)
+      stub_licensed_features(group_saml: true)
+    end
+
+    context 'when project root group prohibits outer forks' do
+      let(:project_group) do
+        create(:saml_provider, :enforced_group_managed_accounts, prohibited_outer_forks: true).group
+      end
+
+      it 'returns namespaces with the same root group as project one only' do
+        expect(fork_targets).to be_a(ActiveRecord::Relation)
+        expect(fork_targets).to match_array([inner_subgroup])
+      end
+    end
+
+    context 'when project root does not prohibit outer forks' do
+      let(:project_group) do
+        create(:saml_provider, :enforced_group_managed_accounts, prohibited_outer_forks: false).group
+      end
+
+      it 'returns outer namespaces as well as inner' do
+        expect(fork_targets).to be_a(ActiveRecord::Relation)
+        expect(fork_targets).to match_array([outer_group, inner_subgroup, user.namespace])
+      end
+    end
+  end
+end

ee/spec/requests/api/projects_spec.rb

@@ -759,4 +759,53 @@ describe API::Projects do
       end
     end
   end
+
+  describe 'POST /projects/:id/fork' do
+    subject(:fork_call) { post api("/projects/#{group_project.id}/fork", user), params: { namespace: target_namespace.id } }
+
+    let!(:target_namespace) do
+      create(:group).tap { |g| g.add_owner(user) }
+    end
+    let!(:group_project) { create(:project, namespace: group)}
+    let(:group) { create(:group) }
+
+    before do
+      group.add_reporter(user)
+    end
+
+    context 'when project namespace has prohibit_outer_forks enabled' do
+      let(:group) do
+        create(:saml_provider, :enforced_group_managed_accounts, prohibited_outer_forks: true).group
+      end
+      let(:user) do
+        create(:user, managing_group: group).tap do |u|
+          create(:group_saml_identity, user: u, saml_provider: group.saml_provider)
+        end
+      end
+
+      before do
+        stub_feature_flags(enforced_sso_requires_session: false)
+        stub_licensed_features(group_saml: true)
+      end
+
+      context 'and target namespace is outer' do
+        it 'renders 404' do
+          expect { fork_call }.not_to change { ::Project.count }
+
+          expect(response).to have_gitlab_http_status(:not_found)
+          expect(json_response['message']).to eq "404 Target Namespace Not Found"
+        end
+      end
+
+      context 'and target namespace is inner to project namespace' do
+        let!(:target_namespace) { create(:group, parent: group) }
+
+        it 'forks the project' do
+          target_namespace.add_owner(user)
+
+          expect { fork_call }.to change { ::Project.count }.by(1)
+        end
+      end
+    end
+  end
 end

lib/api/projects.rb

@@ -267,18 +267,16 @@ module API
       post ':id/fork' do
         Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42284')
 
+        not_found! unless can?(current_user, :fork_project, user_project)
+
         fork_params = declared_params(include_missing: false)
-        namespace_id = fork_params[:namespace]
+        fork_params[:namespace] = find_namespace!(fork_params[:namespace]) if fork_params[:namespace].present?
 
-        if namespace_id.present?
-          fork_params[:namespace] = find_namespace(namespace_id)
+        service = ::Projects::ForkService.new(user_project, current_user, fork_params)
 
-          unless fork_params[:namespace] && can?(current_user, :create_projects, fork_params[:namespace])
-            not_found!('Target Namespace')
-          end
-        end
+        not_found!('Target Namespace') unless service.valid_fork_target?
 
-        forked_project = ::Projects::ForkService.new(user_project, current_user, fork_params).execute
+        forked_project = service.execute
 
         if forked_project.errors.any?
           conflict!(forked_project.errors.messages)

lib/gitlab/object_hierarchy.rb

@@ -51,7 +51,7 @@ module Gitlab
     # and all their ancestors (recursively).
     #
     # Passing an `upto` will stop the recursion once the specified parent_id is
-    # reached. So all ancestors *lower* than the specified acestor will be
+    # reached. So all ancestors *lower* than the specified ancestor will be
     # included.
     #
     # Passing a `hierarchy_order` with either `:asc` or `:desc` will cause the

spec/controllers/projects/forks_controller_spec.rb

@@ -209,6 +209,17 @@ describe Projects::ForksController do
         expect(response).to redirect_to(namespace_project_import_path(user.namespace, project))
       end
 
+      context 'when target namespace is not valid for forking' do
+        let(:params) { super().merge(namespace_key: another_group.id) }
+        let(:another_group) { create :group }
+
+        it 'responds with :not_found' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+
       context 'continue params' do
         let(:params) do
           {

spec/finders/fork_targets_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ForkTargetsFinder do
+  subject(:finder) { described_class.new(project, user) }
+
+  let(:project) { create(:project, namespace: create(:group)) }
+  let(:user) { create(:user) }
+  let!(:maintained_group) do
+    create(:group).tap { |g| g.add_maintainer(user) }
+  end
+  let!(:owned_group) do
+    create(:group).tap { |g| g.add_owner(user) }
+  end
+  let!(:developer_group) do
+    create(:group).tap { |g| g.add_developer(user) }
+  end
+  let!(:reporter_group) do
+    create(:group).tap { |g| g.add_reporter(user) }
+  end
+  let!(:guest_group) do
+    create(:group).tap { |g| g.add_guest(user) }
+  end
+
+  before do
+    project.namespace.add_owner(user)
+  end
+
+  describe '#execute' do
+    it 'returns all user manageable namespaces except project namespace' do
+      expect(finder.execute).to match_array([user.namespace, maintained_group, owned_group])
+    end
+  end
+end

spec/requests/api/projects_spec.rb

@@ -2698,20 +2698,14 @@ describe API::Projects do
       create(:project, :repository, creator: user, namespace: user.namespace)
     end
 
-    let(:group) { create(:group) }
-    let(:group2) do
-      group = create(:group, name: 'group2_name')
-      group.add_maintainer(user2)
-      group
-    end
-
-    let(:group3) do
-      group = create(:group, name: 'group3_name', parent: group2)
-      group.add_owner(user2)
-      group
-    end
+    let(:group) { create(:group, :public) }
+    let(:group2) { create(:group, name: 'group2_name') }
+    let(:group3) { create(:group, name: 'group3_name', parent: group2) }
 
     before do
+      group.add_guest(user2)
+      group2.add_maintainer(user2)
+      group3.add_owner(user2)
       project.add_reporter(user2)
       project2.add_reporter(user2)
     end
@@ -2720,7 +2714,7 @@ describe API::Projects do
       it 'forks if user has sufficient access to project' do
         post api("/projects/#{project.id}/fork", user2)
 
-        expect(response).to have_gitlab_http_status(201)
+        expect(response).to have_gitlab_http_status(:created)
         expect(json_response['name']).to eq(project.name)
         expect(json_response['path']).to eq(project.path)
         expect(json_response['owner']['id']).to eq(user2.id)
@@ -2733,7 +2727,7 @@ describe API::Projects do
       it 'forks if user is admin' do
         post api("/projects/#{project.id}/fork", admin)
 
-        expect(response).to have_gitlab_http_status(201)
+        expect(response).to have_gitlab_http_status(:created)
         expect(json_response['name']).to eq(project.name)
         expect(json_response['path']).to eq(project.path)
         expect(json_response['owner']['id']).to eq(admin.id)
@@ -2747,14 +2741,17 @@ describe API::Projects do
         new_user = create(:user)
         post api("/projects/#{project.id}/fork", new_user)
 
-        expect(response).to have_gitlab_http_status(404)
+        expect(response).to have_gitlab_http_status(:not_found)
         expect(json_response['message']).to eq('404 Project Not Found')
       end
 
       it 'fails if forked project exists in the user namespace' do
-        post api("/projects/#{project.id}/fork", user)
+        new_project = create(:project, name: project.name, path: project.path)
+        new_project.add_reporter(user)
+
+        post api("/projects/#{new_project.id}/fork", user)
 
-        expect(response).to have_gitlab_http_status(409)
+        expect(response).to have_gitlab_http_status(:conflict)
         expect(json_response['message']['name']).to eq(['has already been taken'])
         expect(json_response['message']['path']).to eq(['has already been taken'])
       end
@@ -2762,48 +2759,48 @@ describe API::Projects do
       it 'fails if project to fork from does not exist' do
         post api('/projects/424242/fork', user)
 
-        expect(response).to have_gitlab_http_status(404)
+        expect(response).to have_gitlab_http_status(:not_found)
         expect(json_response['message']).to eq('404 Project Not Found')
       end
 
       it 'forks with explicit own user namespace id' do
         post api("/projects/#{project.id}/fork", user2), params: { namespace: user2.namespace.id }
 
-        expect(response).to have_gitlab_http_status(201)
+        expect(response).to have_gitlab_http_status(:created)
         expect(json_response['owner']['id']).to eq(user2.id)
       end
 
       it 'forks with explicit own user name as namespace' do
         post api("/projects/#{project.id}/fork", user2), params: { namespace: user2.username }
 
-        expect(response).to have_gitlab_http_status(201)
+        expect(response).to have_gitlab_http_status(:created)
         expect(json_response['owner']['id']).to eq(user2.id)
       end
 
       it 'forks to another user when admin' do
         post api("/projects/#{project.id}/fork", admin), params: { namespace: user2.username }
 
-        expect(response).to have_gitlab_http_status(201)
+        expect(response).to have_gitlab_http_status(:created)
         expect(json_response['owner']['id']).to eq(user2.id)
       end
 
       it 'fails if trying to fork to another user when not admin' do
         post api("/projects/#{project.id}/fork", user2), params: { namespace: admin.namespace.id }
 
-        expect(response).to have_gitlab_http_status(404)
+        expect(response).to have_gitlab_http_status(:not_found)
       end
 
       it 'fails if trying to fork to non-existent namespace' do
         post api("/projects/#{project.id}/fork", user2), params: { namespace: 42424242 }
 
-        expect(response).to have_gitlab_http_status(404)
-        expect(json_response['message']).to eq('404 Target Namespace Not Found')
+        expect(response).to have_gitlab_http_status(:not_found)
+        expect(json_response['message']).to eq('404 Namespace Not Found')
       end
 
       it 'forks to owned group' do
         post api("/projects/#{project.id}/fork", user2), params: { namespace: group2.name }
 
-        expect(response).to have_gitlab_http_status(201)
+        expect(response).to have_gitlab_http_status(:created)
         expect(json_response['namespace']['name']).to eq(group2.name)
       end
 
@@ -2811,7 +2808,7 @@ describe API::Projects do
         full_path = "#{group2.path}/#{group3.path}"
         post api("/projects/#{project.id}/fork", user2), params: { namespace: full_path }
 
-        expect(response).to have_gitlab_http_status(201)
+        expect(response).to have_gitlab_http_status(:created)
         expect(json_response['namespace']['name']).to eq(group3.name)
         expect(json_response['namespace']['full_path']).to eq(full_path)
       end
@@ -2819,20 +2816,21 @@ describe API::Projects do
       it 'fails to fork to not owned group' do
         post api("/projects/#{project.id}/fork", user2), params: { namespace: group.name }
 
-        expect(response).to have_gitlab_http_status(404)
+        expect(response).to have_gitlab_http_status(:not_found)
+        expect(json_response['message']).to eq("404 Target Namespace Not Found")
       end
 
       it 'forks to not owned group when admin' do
         post api("/projects/#{project.id}/fork", admin), params: { namespace: group.name }
 
-        expect(response).to have_gitlab_http_status(201)
+        expect(response).to have_gitlab_http_status(:created)
         expect(json_response['namespace']['name']).to eq(group.name)
       end
 
       it 'accepts a path for the target project' do
         post api("/projects/#{project.id}/fork", user2), params: { path: 'foobar' }
 
-        expect(response).to have_gitlab_http_status(201)
+        expect(response).to have_gitlab_http_status(:created)
         expect(json_response['name']).to eq(project.name)
         expect(json_response['path']).to eq('foobar')
         expect(json_response['owner']['id']).to eq(user2.id)
@@ -2846,14 +2844,14 @@ describe API::Projects do
         post api("/projects/#{project.id}/fork", user2), params: { path: 'foobar' }
         post api("/projects/#{project2.id}/fork", user2), params: { path: 'foobar' }
 
-        expect(response).to have_gitlab_http_status(409)
+        expect(response).to have_gitlab_http_status(:conflict)
         expect(json_response['message']['path']).to eq(['has already been taken'])
       end
 
       it 'accepts a name for the target project' do
         post api("/projects/#{project.id}/fork", user2), params: { name: 'My Random Project' }
 
-        expect(response).to have_gitlab_http_status(201)
+        expect(response).to have_gitlab_http_status(:created)
         expect(json_response['name']).to eq('My Random Project')
         expect(json_response['path']).to eq(project.path)
         expect(json_response['owner']['id']).to eq(user2.id)
@@ -2867,7 +2865,7 @@ describe API::Projects do
         post api("/projects/#{project.id}/fork", user2), params: { name: 'My Random Project' }
         post api("/projects/#{project2.id}/fork", user2), params: { name: 'My Random Project' }
 
-        expect(response).to have_gitlab_http_status(409)
+        expect(response).to have_gitlab_http_status(:conflict)
         expect(json_response['message']['name']).to eq(['has already been taken'])
       end
     end
@@ -2876,7 +2874,7 @@ describe API::Projects do
       it 'returns authentication error' do
         post api("/projects/#{project.id}/fork")
 
-        expect(response).to have_gitlab_http_status(401)
+        expect(response).to have_gitlab_http_status(:unauthorized)
         expect(json_response['message']).to eq('401 Unauthorized')
       end
     end
@@ -2890,8 +2888,7 @@ describe API::Projects do
       it 'denies project to be forked' do
         post api("/projects/#{project.id}/fork", admin)
 
-        expect(response).to have_gitlab_http_status(409)
-        expect(json_response['message']['forked_from_project_id']).to eq(['is forbidden'])
+        expect(response).to have_gitlab_http_status(:not_found)
       end
     end
   end

spec/services/projects/fork_service_spec.rb

@@ -275,6 +275,7 @@ describe Projects::ForkService do
       context 'fork project for group when user not owner' do
         it 'group developer fails to fork project into the group' do
           to_project = fork_project(@project, @developer, @opts)
+
           expect(to_project.errors[:namespace]).to eq(['is not valid'])
         end
       end
@@ -336,7 +337,9 @@ describe Projects::ForkService do
   context 'when linking fork to an existing project' do
     let(:fork_from_project) { create(:project, :public) }
     let(:fork_to_project) { create(:project, :public) }
-    let(:user) { create(:user) }
+    let(:user) do
+      create(:user).tap { |u| fork_to_project.add_maintainer(u) }
+    end
 
     subject { described_class.new(fork_from_project, user) }
 
@@ -387,4 +390,54 @@ describe Projects::ForkService do
       end
     end
   end
+
+  describe '#valid_fork_targets' do
+    let(:finder_mock) { instance_double('ForkTargetsFinder', execute: ['finder_return_value']) }
+    let(:current_user) { instance_double('User') }
+    let(:project) { instance_double('Project') }
+
+    before do
+      allow(ForkTargetsFinder).to receive(:new).with(project, current_user).and_return(finder_mock)
+    end
+
+    it 'returns whatever finder returns' do
+      expect(described_class.new(project, current_user).valid_fork_targets).to eq ['finder_return_value']
+    end
+  end
+
+  describe '#valid_fork_target?' do
+    subject { described_class.new(project, user, params).valid_fork_target? }
+
+    let(:project) { Project.new }
+    let(:params) { {} }
+
+    context 'when current user is an admin' do
+      let(:user) { build(:user, :admin) }
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when current_user is not an admin' do
+      let(:user) { create(:user) }
+
+      let(:finder_mock) { instance_double('ForkTargetsFinder', execute: [user.namespace]) }
+      let(:project) { create(:project) }
+
+      before do
+        allow(ForkTargetsFinder).to receive(:new).with(project, user).and_return(finder_mock)
+      end
+
+      context 'when target namespace is in valid fork targets' do
+        let(:params) { { namespace: user.namespace } }
+
+        it { is_expected.to be_truthy }
+      end
+
+      context 'when target namespace is not in valid fork targets' do
+        let(:params) { { namespace: create(:group) } }
+
+        it { is_expected.to be_falsey }
+      end
+    end
+  end
 end