Merge branch '228618-attributes-permitter-in-import-first-phase' into 'master'

Use allowlist of allowed attributes for imported models

See merge request gitlab-org/gitlab!70168

config/feature_flags/development/permitted_attributes_for_import_export.yml

+---
+name: permitted_attributes_for_import_export
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/70168
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/340789
+milestone: '14.4'
+type: development
+group: group::import
+default_enabled: true

lib/gitlab/import_export/attributes_permitter.rb

@@ -42,6 +42,10 @@ module Gitlab
     class AttributesPermitter
       attr_reader :permitted_attributes
 
+      # We want to use AttributesCleaner for these relations instead, in the future this should be removed to make sure
+      # we are using AttributesPermitter for every imported relation.
+      DISABLED_RELATION_NAMES = %i[user author ci_cd_settings issuable_sla push_rule].freeze
+
       def initialize(config: ImportExport::Config.new.to_h)
         @config = config
         @attributes_finder = Gitlab::ImportExport::AttributesFinder.new(config: @config)
@@ -50,16 +54,20 @@ module Gitlab
         build_permitted_attributes
       end
 
-      def permit(relation_name, relation_hash)
-        permitted_attributes = permitted_attributes_for(relation_name)
+      def permit(relation_sym, relation_hash)
+        permitted_attributes = permitted_attributes_for(relation_sym)
 
         relation_hash.select do |key, _|
-          permitted_attributes.include?(key)
+          permitted_attributes.include?(key.to_sym)
         end
       end
 
-      def permitted_attributes_for(relation_name)
-        @permitted_attributes[relation_name] || []
+      def permitted_attributes_for(relation_sym)
+        @permitted_attributes[relation_sym] || []
+      end
+
+      def permitted_attributes_defined?(relation_sym)
+        !DISABLED_RELATION_NAMES.include?(relation_sym) && @attributes_finder.included_attributes.key?(relation_sym)
       end
 
       private

lib/gitlab/import_export/base/relation_factory.rb

@@ -29,7 +29,7 @@ module Gitlab
            owner_id
          ].freeze
 
-        TOKEN_RESET_MODELS = %i[Project Namespace Group Ci::Trigger Ci::Build Ci::Runner ProjectHook].freeze
+        TOKEN_RESET_MODELS = %i[Project Namespace Group Ci::Trigger Ci::Build Ci::Runner ProjectHook ErrorTracking::ProjectErrorTrackingSetting].freeze
 
         def self.create(*args, **kwargs)
           new(*args, **kwargs).create
@@ -45,6 +45,7 @@ module Gitlab
         end
 
         def initialize(relation_sym:, relation_index:, relation_hash:, members_mapper:, object_builder:, user:, importable:, excluded_keys: [])
+          @relation_sym = relation_sym
           @relation_name = self.class.overrides[relation_sym]&.to_sym || relation_sym
           @relation_index = relation_index
           @relation_hash = relation_hash.except('noteable_id')
@@ -181,8 +182,17 @@ module Gitlab
         end
 
         def parsed_relation_hash
-          @parsed_relation_hash ||= Gitlab::ImportExport::AttributeCleaner.clean(relation_hash: @relation_hash,
-                                                                                 relation_class: relation_class)
+          strong_memoize(:parsed_relation_hash) do
+            if Feature.enabled?(:permitted_attributes_for_import_export, default_enabled: :yaml) && attributes_permitter.permitted_attributes_defined?(@relation_sym)
+              attributes_permitter.permit(@relation_sym, @relation_hash)
+            else
+              Gitlab::ImportExport::AttributeCleaner.clean(relation_hash: @relation_hash, relation_class: relation_class)
+            end
+          end
+        end
+
+        def attributes_permitter
+          @attributes_permitter ||= Gitlab::ImportExport::AttributesPermitter.new
         end
 
         def existing_or_new_object

lib/gitlab/import_export/project/import_export.yml

@@ -121,6 +121,41 @@ included_attributes:
     - :name
   ci_cd_settings:
     - :group_runners_enabled
+  metrics_setting:
+    - :dashboard_timezone
+    - :external_dashboard_url
+    - :project_id
+  project_badges:
+    - :created_at
+    - :group_id
+    - :image_url
+    - :link_url
+    - :name
+    - :project_id
+    - :type
+    - :updated_at
+  pipeline_schedules:
+    - :active
+    - :created_at
+    - :cron
+    - :cron_timezone
+    - :description
+    - :next_run_at
+    - :owner_id
+    - :project_id
+    - :ref
+    - :updated_at
+  error_tracking_setting:
+    - :api_url
+    - :organization_name
+    - :project_id
+    - :project_name
+  auto_devops:
+    - :created_at
+    - :deploy_strategy
+    - :enabled
+    - :project_id
+    - :updated_at
 
 # Do not include the following attributes for the models specified.
 excluded_attributes:

spec/lib/gitlab/import_export/attributes_permitter_spec.rb

@@ -74,4 +74,73 @@ RSpec.describe Gitlab::ImportExport::AttributesPermitter do
       expect(subject.permitted_attributes_for(:labels)).to contain_exactly(:title, :description, :type, :priorities)
     end
   end
+
+  describe '#permitted_attributes_defined?' do
+    using RSpec::Parameterized::TableSyntax
+
+    let(:attributes_permitter) { described_class.new }
+
+    where(:relation_name, :permitted_attributes_defined) do
+      :user                   | false
+      :author                 | false
+      :ci_cd_settings         | false
+      :issuable_sla           | false
+      :push_rule              | false
+      :metrics_setting        | true
+      :project_badges         | true
+      :pipeline_schedules     | true
+      :error_tracking_setting | true
+      :auto_devops            | true
+    end
+
+    with_them do
+      it { expect(attributes_permitter.permitted_attributes_defined?(relation_name)).to eq(permitted_attributes_defined) }
+    end
+  end
+
+  describe 'included_attributes for Project' do
+    let(:prohibited_attributes) { %i[remote_url my_attributes my_ids token my_id test] }
+
+    subject { described_class.new }
+
+    Gitlab::ImportExport::Config.new.to_h[:included_attributes].each do |relation_sym, permitted_attributes|
+      context "for #{relation_sym}" do
+        let(:import_export_config) { Gitlab::ImportExport::Config.new.to_h }
+        let(:project_relation_factory) { Gitlab::ImportExport::Project::RelationFactory }
+
+        let(:relation_hash) { (permitted_attributes + prohibited_attributes).map(&:to_s).zip([]).to_h }
+        let(:relation_name) { project_relation_factory.overrides[relation_sym]&.to_sym || relation_sym }
+        let(:relation_class) { project_relation_factory.relation_class(relation_name) }
+        let(:excluded_keys) { import_export_config.dig(:excluded_keys, relation_sym) || [] }
+
+        let(:cleaned_hash) do
+          Gitlab::ImportExport::AttributeCleaner.new(
+            relation_hash: relation_hash,
+            relation_class: relation_class,
+            excluded_keys: excluded_keys
+          ).clean
+        end
+
+        let(:permitted_hash) { subject.permit(relation_sym, relation_hash) }
+
+        if described_class.new.permitted_attributes_defined?(relation_sym)
+          it 'contains only attributes that are defined as permitted in the import/export config' do
+            expect(permitted_hash.keys).to contain_exactly(*permitted_attributes.map(&:to_s))
+          end
+
+          it 'does not contain attributes that would be cleaned with AttributeCleaner' do
+            expect(cleaned_hash.keys).to include(*permitted_hash.keys)
+          end
+
+          it 'does not contain prohibited attributes that are not related to given relation' do
+            expect(permitted_hash.keys).not_to include(*prohibited_attributes.map(&:to_s))
+          end
+        else
+          it 'is disabled' do
+            expect(subject).not_to be_permitted_attributes_defined(relation_sym)
+          end
+        end
+      end
+    end
+  end
 end