From 89e595eead45a1bccb2a4869edcdf40a6a980bd9 Mon Sep 17 00:00:00 2001 From: "Alan (Maciej) Paruszewski" Date: Thu, 7 Oct 2021 00:12:35 +0200 Subject: [PATCH 1/5] Use allowlist of allowed attributes for imported models This change changes the behavior that is reponsible for filtering out attributes when importing new project. We were cleaning attributes based on list of excluded patterns and attributes. For Merge Request, Project Member, Snippet and related models we are changing that behavior. Changelog: changed --- .../import_export/attributes_permitter.rb | 2 +- .../import_export/project/import_export.yml | 226 +++++++++++++++++- .../attributes_permitter_spec.rb | 25 +- 3 files changed, 250 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/import_export/attributes_permitter.rb b/lib/gitlab/import_export/attributes_permitter.rb index 2d8e25a9f70a3e..f6f65f855990e5 100644 --- a/lib/gitlab/import_export/attributes_permitter.rb +++ b/lib/gitlab/import_export/attributes_permitter.rb @@ -44,7 +44,7 @@ class AttributesPermitter # 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 issuable_sla].freeze + DISABLED_RELATION_NAMES = %i[author issuable_sla].freeze def initialize(config: ImportExport::Config.new.to_h) @config = config diff --git a/lib/gitlab/import_export/project/import_export.yml b/lib/gitlab/import_export/project/import_export.yml index 5962355a8abd67..a800146e99e864 100644 --- a/lib/gitlab/import_export/project/import_export.yml +++ b/lib/gitlab/import_export/project/import_export.yml @@ -113,7 +113,6 @@ tree: # Only include the following attributes for the models specified. included_attributes: user: - - :id - :public_email - :username author: @@ -315,6 +314,231 @@ included_attributes: - :project_id - :issue_template_key - :project_key + snippets: + - :title + - :content + - :author_id + - :project_id + - :created_at + - :updated_at + - :file_name + - :visibility_level + - :description + project_members: + - :access_level + - :source_type + - :user_id + - :notification_level + - :created_at + - :updated_at + - :created_by_id + - :invite_email + - :invite_token + - :invite_accepted_at + - :requested_at + - :expires_at + - :ldap + - :override + merge_request: + - :target_branch + - :source_branch + - :source_project_id + - :author_id + - :assignee_id + - :title + - :created_at + - :updated_at + - :state + - :merge_status + - :target_project_id + - :iid + - :description + - :updated_by_id + - :merge_error + - :merge_params + - :merge_when_pipeline_succeeds + - :merge_user_id + - :merge_commit_sha + - :squash_commit_sha + - :in_progress_merge_commit_sha + - :lock_version + - :approvals_before_merge + - :rebase_commit_sha + - :time_estimate + - :squash + - :last_edited_at + - :last_edited_by_id + - :discussion_locked + - :allow_maintainer_to_push + - :merge_ref_sha + - :draft + merge_requests: + - :target_branch + - :source_branch + - :source_project_id + - :author_id + - :assignee_id + - :title + - :created_at + - :updated_at + - :state + - :merge_status + - :target_project_id + - :iid + - :description + - :updated_by_id + - :merge_error + - :merge_params + - :merge_when_pipeline_succeeds + - :merge_user_id + - :merge_commit_sha + - :squash_commit_sha + - :in_progress_merge_commit_sha + - :lock_version + - :approvals_before_merge + - :rebase_commit_sha + - :time_estimate + - :squash + - :last_edited_at + - :last_edited_by_id + - :discussion_locked + - :allow_maintainer_to_push + - :merge_ref_sha + - :draft + award_emoji: + - :user_id + - :name + - :awardable_type + - :created_at + - :updated_at + commit_author: + - :name + - :email + committer: + - :name + - :email + events: + - :target_type + - :action + - :author_id + - :fingerprint + label_links: + - :target_type + - :created_at + - :updated_at + merge_request_diff: + - :state + - :created_at + - :updated_at + - :base_commit_sha + - :real_size + - :head_commit_sha + - :start_commit_sha + - :commits_count + - :files_count + - :sorted + - :diff_type + merge_request_diff_commits: + - :relative_order + - :sha + - :authored_date + - :committed_date + - :author_name + - :author_email + - :committer_name + - :committer_email + - :message + - :trailers + merge_request_diff_files: + - :relative_order + - :new_file + - :renamed_file + - :deleted_file + - :new_path + - :old_path + - :a_mode + - :b_mode + - :too_large + - :binary + metrics: + - :created_at + - :updated_at + - :latest_closed_by_id + - :latest_closed_at + - :merged_by_id + - :merged_at + - :latest_build_started_at + - :latest_build_finished_at + - :first_deployed_to_production_at + - :first_comment_at + - :first_commit_at + - :last_commit_at + - :diff_size + - :modified_paths_size + - :commits_count + - :first_approved_at + - :first_reassigned_at + - :added_lines + - :target_project_id + - :removed_lines + notes: + - :note + - :noteable_type + - :author_id + - :created_at + - :updated_at + - :project_id + - :attachment + - :line_code + - :commit_id + - :system + - :st_diff + - :updated_by_id + - :type + - :position + - :original_position + - :change_position + - :resolved_at + - :resolved_by_id + - :resolved_by_push + - :discussion_id + - :confidential + - :last_edited_at + push_event_payload: + - :commit_count + - :action + - :ref_type + - :commit_from + - :commit_to + - :ref + - :commit_title + - :ref_count + resource_label_events: + - :action + - :user_id + - :created_at + suggestions: + - :relative_order + - :applied + - :commit_id + - :from_content + - :to_content + - :outdated + - :lines_above + - :lines_below + system_note_metadata: + - :commit_count + - :action + - :created_at + - :updated_at + timelogs: + - :time_spent + - :user_id + - :project_id + - :spent_at + - :created_at + - :updated_at + - :summary # Do not include the following attributes for the models specified. excluded_attributes: diff --git a/spec/lib/gitlab/import_export/attributes_permitter_spec.rb b/spec/lib/gitlab/import_export/attributes_permitter_spec.rb index 55422ec3ef2608..125341d58cdcdc 100644 --- a/spec/lib/gitlab/import_export/attributes_permitter_spec.rb +++ b/spec/lib/gitlab/import_export/attributes_permitter_spec.rb @@ -81,7 +81,7 @@ let(:attributes_permitter) { described_class.new } where(:relation_name, :permitted_attributes_defined) do - :user | false + :user | true :author | false :ci_cd_settings | true :metrics_setting | true @@ -91,6 +91,7 @@ :auto_devops | true :boards | true :custom_attributes | true + :label | true :labels | true :protected_branches | true :protected_tags | true @@ -99,6 +100,28 @@ :push_access_levels | true :releases | true :links | true + :priorities | true + :milestone | true + :milestones | true + :snippets | true + :project_members | true + :merge_request | true + :merge_requests | true + :award_emoji | true + :commit_author | true + :committer | true + :events | true + :label_links | true + :merge_request_diff | true + :merge_request_diff_commits | true + :merge_request_diff_files | true + :metrics | true + :notes | true + :push_event_payload | true + :resource_label_events | true + :suggestions | true + :system_note_metadata | true + :timelogs | true :container_expiration_policy | true :project_feature | true :prometheus_metrics | true -- GitLab From 52888d1975a28bb07a77df62d49507aed6669d80 Mon Sep 17 00:00:00 2001 From: "Alan (Maciej) Paruszewski" Date: Tue, 12 Oct 2021 17:11:29 +0200 Subject: [PATCH 2/5] Fix failing spec --- lib/gitlab/import_export/project/import_export.yml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lib/gitlab/import_export/project/import_export.yml b/lib/gitlab/import_export/project/import_export.yml index a800146e99e864..2a6901f5acb8b3 100644 --- a/lib/gitlab/import_export/project/import_export.yml +++ b/lib/gitlab/import_export/project/import_export.yml @@ -422,6 +422,8 @@ included_attributes: - :action - :author_id - :fingerprint + - :created_at + - :updated_at label_links: - :target_type - :created_at @@ -460,6 +462,7 @@ included_attributes: - :b_mode - :too_large - :binary + - :diff metrics: - :created_at - :updated_at @@ -661,6 +664,9 @@ excluded_attributes: - :latest_merge_request_diff_id - :head_pipeline_id - :state_id + - :diff_head_sha + - :source_branch_sha + - :target_branch_sha merge_requests: - :milestone_id - :sprint_id @@ -670,6 +676,9 @@ excluded_attributes: - :latest_merge_request_diff_id - :head_pipeline_id - :state_id + - :diff_head_sha + - :source_branch_sha + - :target_branch_sha award_emoji: - :awardable_id statuses: -- GitLab From a51bcd3ed67690b40d12b229a93adb7668aad3cc Mon Sep 17 00:00:00 2001 From: "Alan (Maciej) Paruszewski" Date: Wed, 13 Oct 2021 08:53:34 +0200 Subject: [PATCH 3/5] Fix failing spec --- lib/gitlab/import_export/project/import_export.yml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/gitlab/import_export/project/import_export.yml b/lib/gitlab/import_export/project/import_export.yml index 2a6901f5acb8b3..27e073e15a1188 100644 --- a/lib/gitlab/import_export/project/import_export.yml +++ b/lib/gitlab/import_export/project/import_export.yml @@ -372,6 +372,9 @@ included_attributes: - :allow_maintainer_to_push - :merge_ref_sha - :draft + - :diff_head_sha + - :source_branch_sha + - :target_branch_sha merge_requests: - :target_branch - :source_branch @@ -405,6 +408,9 @@ included_attributes: - :allow_maintainer_to_push - :merge_ref_sha - :draft + - :diff_head_sha + - :source_branch_sha + - :target_branch_sha award_emoji: - :user_id - :name @@ -664,9 +670,6 @@ excluded_attributes: - :latest_merge_request_diff_id - :head_pipeline_id - :state_id - - :diff_head_sha - - :source_branch_sha - - :target_branch_sha merge_requests: - :milestone_id - :sprint_id @@ -676,9 +679,6 @@ excluded_attributes: - :latest_merge_request_diff_id - :head_pipeline_id - :state_id - - :diff_head_sha - - :source_branch_sha - - :target_branch_sha award_emoji: - :awardable_id statuses: -- GitLab From 59d8a95184bda62abf4a362e93747e60c6bfd46d Mon Sep 17 00:00:00 2001 From: "Alan (Maciej) Paruszewski" Date: Sun, 31 Oct 2021 12:23:15 +0100 Subject: [PATCH 4/5] Address MR comments --- .../import_export/project/import_export.yml | 89 +++---------------- .../attributes_permitter_spec.rb | 6 +- .../attributes_permitter_shared_examples.rb | 4 +- 3 files changed, 17 insertions(+), 82 deletions(-) diff --git a/lib/gitlab/import_export/project/import_export.yml b/lib/gitlab/import_export/project/import_export.yml index 27e073e15a1188..22c022aca90bf0 100644 --- a/lib/gitlab/import_export/project/import_export.yml +++ b/lib/gitlab/import_export/project/import_export.yml @@ -113,6 +113,7 @@ tree: # Only include the following attributes for the models specified. included_attributes: user: + - :id - :public_email - :username author: @@ -177,17 +178,7 @@ included_attributes: - :project_id - :key - :value - label: - - :title - - :color - - :project_id - - :group_id - - :created_at - - :updated_at - - :template - - :description - - :priority - labels: + label: &label_definition - :title - :color - :project_id @@ -197,23 +188,13 @@ included_attributes: - :template - :description - :priority + labels: *label_definition priorities: - :project_id - :priority - :created_at - :updated_at - milestone: - - :iid - - :title - - :project_id - - :group_id - - :description - - :due_date - - :created_at - - :updated_at - - :start_date - - :state - milestones: + milestone: &milestone_definition - :iid - :title - :project_id @@ -224,6 +205,7 @@ included_attributes: - :updated_at - :start_date - :state + milestones: *milestone_definition protected_branches: - :project_id - :name @@ -333,49 +315,12 @@ included_attributes: - :updated_at - :created_by_id - :invite_email - - :invite_token - :invite_accepted_at - :requested_at - :expires_at - :ldap - :override - merge_request: - - :target_branch - - :source_branch - - :source_project_id - - :author_id - - :assignee_id - - :title - - :created_at - - :updated_at - - :state - - :merge_status - - :target_project_id - - :iid - - :description - - :updated_by_id - - :merge_error - - :merge_params - - :merge_when_pipeline_succeeds - - :merge_user_id - - :merge_commit_sha - - :squash_commit_sha - - :in_progress_merge_commit_sha - - :lock_version - - :approvals_before_merge - - :rebase_commit_sha - - :time_estimate - - :squash - - :last_edited_at - - :last_edited_by_id - - :discussion_locked - - :allow_maintainer_to_push - - :merge_ref_sha - - :draft - - :diff_head_sha - - :source_branch_sha - - :target_branch_sha - merge_requests: + merge_request: &merge_request_definition - :target_branch - :source_branch - :source_project_id @@ -411,6 +356,7 @@ included_attributes: - :diff_head_sha - :source_branch_sha - :target_branch_sha + merge_requests: *merge_request_definition award_emoji: - :user_id - :name @@ -451,10 +397,6 @@ included_attributes: - :sha - :authored_date - :committed_date - - :author_name - - :author_email - - :committer_name - - :committer_email - :message - :trailers merge_request_diff_files: @@ -661,16 +603,7 @@ excluded_attributes: - :service_desk_reply_to - :upvotes_count - :work_item_type_id - merge_request: - - :milestone_id - - :sprint_id - - :ref_fetched - - :merge_jid - - :rebase_jid - - :latest_merge_request_diff_id - - :head_pipeline_id - - :state_id - merge_requests: + merge_request: &merge_request_excluded_definition - :milestone_id - :sprint_id - :ref_fetched @@ -679,6 +612,7 @@ excluded_attributes: - :latest_merge_request_diff_id - :head_pipeline_id - :state_id + merge_requests: *merge_request_excluded_definition award_emoji: - :awardable_id statuses: @@ -747,10 +681,9 @@ excluded_attributes: - :issue_id zoom_meetings: - :issue_id - design: - - :issue_id - designs: + design: &design_excluded_definition - :issue_id + designs: *design_excluded_definition design_versions: - :issue_id actions: diff --git a/spec/lib/gitlab/import_export/attributes_permitter_spec.rb b/spec/lib/gitlab/import_export/attributes_permitter_spec.rb index 125341d58cdcdc..4fea61ec3a82f6 100644 --- a/spec/lib/gitlab/import_export/attributes_permitter_spec.rb +++ b/spec/lib/gitlab/import_export/attributes_permitter_spec.rb @@ -80,7 +80,7 @@ let(:attributes_permitter) { described_class.new } - where(:relation_name, :permitted_attributes_defined) do + where(:relation_name, :permitted_attributes_defined ) do :user | true :author | false :ci_cd_settings | true @@ -136,9 +136,11 @@ describe 'included_attributes for Project' do subject { described_class.new } + additional_attributes = { user: %w[id] } + Gitlab::ImportExport::Config.new.to_h[:included_attributes].each do |relation_sym, permitted_attributes| context "for #{relation_sym}" do - it_behaves_like 'a permitted attribute', relation_sym, permitted_attributes + it_behaves_like 'a permitted attribute', relation_sym, permitted_attributes, additional_attributes[relation_sym] end end end diff --git a/spec/support/shared_examples/lib/gitlab/import_export/attributes_permitter_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/import_export/attributes_permitter_shared_examples.rb index 5ce698c470182d..41d3d76b66b6b3 100644 --- a/spec/support/shared_examples/lib/gitlab/import_export/attributes_permitter_shared_examples.rb +++ b/spec/support/shared_examples/lib/gitlab/import_export/attributes_permitter_shared_examples.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true -RSpec.shared_examples 'a permitted attribute' do |relation_sym, permitted_attributes| +RSpec.shared_examples 'a permitted attribute' do |relation_sym, permitted_attributes, additional_attributes = []| let(:prohibited_attributes) { %i[remote_url my_attributes my_ids token my_id test] } let(:import_export_config) { Gitlab::ImportExport::Config.new.to_h } @@ -26,7 +26,7 @@ end it 'does not contain attributes that would be cleaned with AttributeCleaner' do - expect(cleaned_hash.keys).to include(*permitted_hash.keys) + expect(cleaned_hash.keys + additional_attributes.to_a).to include(*permitted_hash.keys) end it 'does not contain prohibited attributes that are not related to given relation' do -- GitLab From a817b439ae537163fa8dac4a5a96b72abfaf0164 Mon Sep 17 00:00:00 2001 From: "Alan (Maciej) Paruszewski" Date: Sun, 31 Oct 2021 12:39:37 +0100 Subject: [PATCH 5/5] Fix failing spec --- ee/spec/lib/gitlab/import_export/attributes_permitter_spec.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ee/spec/lib/gitlab/import_export/attributes_permitter_spec.rb b/ee/spec/lib/gitlab/import_export/attributes_permitter_spec.rb index 2ddbfe381c225c..1cb989c051b36a 100644 --- a/ee/spec/lib/gitlab/import_export/attributes_permitter_spec.rb +++ b/ee/spec/lib/gitlab/import_export/attributes_permitter_spec.rb @@ -27,9 +27,11 @@ subject { described_class.new } + additional_attributes = { user: %w[id] } + Gitlab::ImportExport::Config.new.to_h[:included_attributes].each do |relation_sym, permitted_attributes| context "for #{relation_sym}" do - it_behaves_like 'a permitted attribute', relation_sym, permitted_attributes + it_behaves_like 'a permitted attribute', relation_sym, permitted_attributes, additional_attributes[relation_sym] end end end -- GitLab