Do not allow project member import when membership is locked

Merge branch 'security-project-import-members-fix-14-10' into '14-10-stable-ee'

See merge request gitlab-org/security/gitlab!2447

Changelog: security

app/policies/project_policy.rb

@@ -748,6 +748,10 @@ class ProjectPolicy < BasePolicy
     prevent :register_project_runners
   end
 
+  rule { can?(:admin_project_member) }.policy do
+    enable :import_project_members_from_another_project
+  end
+
   private
 
   def user_is_user?

app/services/members/import_project_team_service.rb

@@ -29,7 +29,7 @@ module Members
     def import_project_team
       return false unless target_project.present? && source_project.present? && current_user.present?
       return false unless can?(current_user, :read_project_member, source_project)
-      return false unless can?(current_user, :admin_project_member, target_project)
+      return false unless can?(current_user, :import_project_members_from_another_project, target_project)
 
       target_project.team.import(source_project, current_user)
     end

ee/app/policies/ee/project_policy.rb

@@ -145,6 +145,15 @@ module EE
         ::Gitlab::IncidentManagement.escalation_policies_available?(@subject)
       end
 
+      with_scope :subject
+      condition(:membership_locked_via_parent_group) do
+        @subject.group && (@subject.group.membership_lock? || ::Gitlab::CurrentSettings.lock_memberships_to_ldap?)
+      end
+
+      rule { membership_locked_via_parent_group }.policy do
+        prevent :import_project_members_from_another_project
+      end
+
       rule { visual_review_bot }.policy do
         prevent :read_note
         enable :create_note

ee/spec/policies/project_policy_spec.rb

@@ -2052,4 +2052,34 @@ RSpec.describe ProjectPolicy do
       expect_disallowed(*owner_permissions)
     end
   end
+
+  context 'importing members from another project' do
+    let(:current_user) { owner }
+
+    context 'for a personal project' do
+      it { is_expected.to be_allowed(:import_project_members_from_another_project) }
+    end
+
+    context 'for a project in a group' do
+      let(:project) { create(:project, group: create(:group)) }
+
+      context 'when the project has locked their membership' do
+        context 'via the parent group' do
+          before do
+            project.group.update!(membership_lock: true)
+          end
+
+          it { is_expected.to be_disallowed(:import_project_members_from_another_project) }
+        end
+
+        context 'via LDAP' do
+          before do
+            stub_application_setting(lock_memberships_to_ldap: true)
+          end
+
+          it { is_expected.to be_disallowed(:import_project_members_from_another_project) }
+        end
+      end
+    end
+  end
 end

ee/spec/requests/api/projects_spec.rb

@@ -1433,4 +1433,46 @@ RSpec.describe API::Projects do
       end
     end
   end
+
+  describe 'POST /projects/:id/import_project_members/:project_id' do
+    let_it_be(:project) { create(:project) }
+    let_it_be(:target_project) { create(:project, group: create(:group)) }
+
+    before_all do
+      project.add_maintainer(another_user)
+      target_project.add_maintainer(another_user)
+    end
+
+    context 'when the target project has locked their membership' do
+      context 'via the parent group' do
+        before do
+          target_project.group.update!(membership_lock: true)
+        end
+
+        it 'returns 403' do
+          expect do
+            post api("/projects/#{target_project.id}/import_project_members/#{project.id}", another_user)
+          end.not_to change { target_project.members.count }
+
+          expect(response).to have_gitlab_http_status(:unprocessable_entity)
+          expect(json_response['message']).to eq('Import failed')
+        end
+      end
+
+      context 'via LDAP' do
+        before do
+          stub_application_setting(lock_memberships_to_ldap: true)
+        end
+
+        it 'returns 403' do
+          expect do
+            post api("/projects/#{target_project.id}/import_project_members/#{project.id}", another_user)
+          end.not_to change { target_project.members.count }
+
+          expect(response).to have_gitlab_http_status(:unprocessable_entity)
+          expect(json_response['message']).to eq('Import failed')
+        end
+      end
+    end
+  end
 end

ee/spec/services/ee/members/import_project_team_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Members::ImportProjectTeamService do
+  describe '#execute' do
+    let_it_be(:source_project) { create(:project) }
+    let_it_be(:target_project) { create(:project, group: create(:group)) }
+    let_it_be(:user) { create(:user) }
+
+    let(:source_project_id) { source_project.id }
+    let(:target_project_id) { target_project.id }
+
+    subject { described_class.new(user, { id: target_project_id, project_id: source_project_id }) }
+
+    before_all do
+      source_project.add_guest(user)
+      target_project.add_maintainer(user)
+    end
+
+    context 'when the project team import fails' do
+      context 'when the target project has locked their membership' do
+        context 'via the parent group' do
+          before do
+            target_project.group.update!(membership_lock: true)
+          end
+
+          it 'returns false' do
+            expect(subject.execute).to be(false)
+          end
+        end
+
+        context 'via LDAP' do
+          before do
+            stub_application_setting(lock_memberships_to_ldap: true)
+          end
+
+          it 'returns false' do
+            expect(subject.execute).to be(false)
+          end
+        end
+      end
+    end
+  end
+end

spec/policies/project_policy_spec.rb

@@ -396,6 +396,36 @@ RSpec.describe ProjectPolicy do
     end
   end
 
+  context 'importing members from another project' do
+    %w(maintainer owner).each do |role|
+      context "with #{role}" do
+        let(:current_user) { send(role) }
+
+        it { is_expected.to be_allowed(:import_project_members_from_another_project) }
+      end
+    end
+
+    %w(guest reporter developer anonymous).each do |role|
+      context "with #{role}" do
+        let(:current_user) { send(role) }
+
+        it { is_expected.to be_disallowed(:import_project_members_from_another_project) }
+      end
+    end
+
+    context 'with an admin' do
+      let(:current_user) { admin }
+
+      context 'when admin mode is enabled', :enable_admin_mode do
+        it { expect_allowed(:import_project_members_from_another_project) }
+      end
+
+      context 'when admin mode is disabled' do
+        it { expect_disallowed(:import_project_members_from_another_project) }
+      end
+    end
+  end
+
   context 'reading usage quotas' do
     %w(maintainer owner).each do |role|
       context "with #{role}" do