Merge branch 'fix/import-projectmember-security' into 'security'

Fix Import/Export foreign key issue to do with project members

Cleans-up any foreign keys in `ProjectMember` - same as we do with the rest of the models when importing.

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/23837 and https://gitlab.com/gitlab-org/gitlab-ce/issues/23739



See merge request !2020

Signed-off-by: Rémy Coutable <remy@rymai.me>

lib/gitlab/import_export/attribute_cleaner.rb

@@ -3,10 +3,25 @@ module Gitlab
     class AttributeCleaner
       ALLOWED_REFERENCES = RelationFactory::PROJECT_REFERENCES + RelationFactory::USER_REFERENCES + ['group_id']
-      def self.clean!(relation_hash:)
-        relation_hash.reject! do |key, _value|
-          key.end_with?('_id') && !ALLOWED_REFERENCES.include?(key)
-        end
+      def self.clean(*args)
+        new(*args).clean
+      end
+
+      def initialize(relation_hash:, relation_class:)
+        @relation_hash = relation_hash
+        @relation_class = relation_class
+      end
+
+      def clean
+        @relation_hash.reject do |key, _value|
+          prohibited_key?(key) || !@relation_class.attribute_method?(key)
+        end.except('id')
+      end
+
+      private
+
+      def prohibited_key?(key)
+        key.end_with?('_id') && !ALLOWED_REFERENCES.include?(key)
       end
     end
   end

lib/gitlab/import_export/members_mapper.rb

@@ -55,7 +55,12 @@ module Gitlab
       end
       def member_hash(member)
-        member.except('id').merge(source_id: @project.id, importing: true)
+        parsed_hash(member).merge('source_id' => @project.id, 'importing' => true)
+      end
+
+      def parsed_hash(member)
+        Gitlab::ImportExport::AttributeCleaner.clean(relation_hash: member.deep_stringify_keys,
+                                                     relation_class: ProjectMember)
       end
       def find_project_user_query(member)

lib/gitlab/import_export/relation_factory.rb

@@ -14,7 +14,7 @@ module Gitlab
                     priorities: :label_priorities,
                     label: :project_label }.freeze
-      USER_REFERENCES = %w[author_id assignee_id updated_by_id user_id].freeze
+      USER_REFERENCES = %w[author_id assignee_id updated_by_id user_id created_by_id].freeze
       PROJECT_REFERENCES = %w[project_id source_project_id gl_project_id target_project_id].freeze
@@ -30,7 +30,7 @@ module Gitlab
       def initialize(relation_sym:, relation_hash:, members_mapper:, user:, project_id:)
         @relation_name = OVERRIDES[relation_sym] || relation_sym
-        @relation_hash = relation_hash.except('id', 'noteable_id').merge('project_id' => project_id)
+        @relation_hash = relation_hash.except('noteable_id').merge('project_id' => project_id)
         @members_mapper = members_mapper
         @user = user
         @imported_object_retries = 0
@@ -172,11 +172,8 @@ module Gitlab
       end
       def parsed_relation_hash
-        @parsed_relation_hash ||= begin
-          Gitlab::ImportExport::AttributeCleaner.clean!(relation_hash: @relation_hash)
-
-          @relation_hash.reject { |k, _v| !relation_class.attribute_method?(k) }
-        end
+        @parsed_relation_hash ||= Gitlab::ImportExport::AttributeCleaner.clean(relation_hash: @relation_hash,
+                                                                               relation_class: relation_class)
       end
       def set_st_diffs

spec/lib/gitlab/import_export/attribute_cleaner_spec.rb

 require 'spec_helper'
 describe Gitlab::ImportExport::AttributeCleaner, lib: true do
+  let(:relation_class){ double('relation_class').as_null_object }
   let(:unsafe_hash) do
     {
+      'id' => 101,
       'service_id' => 99,
       'moved_to_id' => 99,
       'namespace_id' => 99,
@@ -27,8 +29,9 @@ describe Gitlab::ImportExport::AttributeCleaner, lib: true do
   end
   it 'removes unwanted attributes from the hash' do
-    described_class.clean!(relation_hash: unsafe_hash)
-    expect(unsafe_hash).to eq(post_safe_hash)
+    # allow(relation_class).to receive(:attribute_method?).and_return(true)
+    parsed_hash = described_class.clean(relation_hash: unsafe_hash, relation_class: relation_class)
+    expect(parsed_hash).to eq(post_safe_hash)
   end
 end