fixing a couple of bugs and lots of refactoring of the export file spec

spec/features/projects/import_export/export_file_spec.rb

 require 'spec_helper'
 
+# Integration test that exports a file using the Import/EXport feature
+# It looks up for any sensitive word inside the JSON, so if a sensitive word is found
+# we''l have to either include it adding the model containing it to the +safe_list+
+# or make sure the attribute is blacklisted in the +import_export.yml+ configuration
 feature 'project export', feature: true, js: true do
   include Select2Helper
   include ExportFileHelper
 
   let(:user) { create(:admin) }
   let(:export_path) { "#{Dir::tmpdir}/import_file_spec" }
+  let(:config_hash) { YAML.load_file(Gitlab::ImportExport.config_file).deep_stringify_keys }
 
   let(:sensitive_words) { %w[pass secret token key] }
-  let(:safe_models) do
+  let(:safe_list) do
     {
-      token: [ProjectHook, Ci::Trigger]
+      token: [ProjectHook, Ci::Trigger],
+      key: [Project, Ci::Variable, :yaml_variables]
     }
   end
+  let(:safe_hashes) { { yaml_variables: %w[key value public] } }
 
   let(:project) { setup_project }
 
@@ -49,7 +56,9 @@ feature 'project export', feature: true, js: true do
         project_hash = JSON.parse(IO.read(project_json_path))
 
         sensitive_words.each do |sensitive_word|
-          expect(has_sensitive_attributes?(sensitive_word, project_hash)).to be false
+          found = find_sensitive_attributes(sensitive_word, project_hash)
+
+          expect(found).to be_nil, "Found a new sensitive word <#{found.try(:key_found)}>, which is part of the hash #{found.try(:parent)}"
         end
       end
     end

spec/lib/gitlab/import_export/all_models.json

-{
-  "issues": [
-    "subscriptions",
-    "award_emoji",
-    "author",
-    "assignee",
-    "updated_by",
-    "milestone",
-    "notes",
-    "label_links",
-    "labels",
-    "todos",
-    "user_agent_detail",
-    "project",
-    "moved_to",
-    "events"
-  ],
-  "events": [
-    "author",
-    "project",
-    "target"
-  ],
-  "notes": [
-    "award_emoji",
-    "project",
-    "noteable",
-    "author",
-    "updated_by",
-    "resolved_by",
-    "todos",
-    "events"
-  ],
-  "label_links": [
-    "target",
-    "label"
-  ],
-  "label": [
-    "subscriptions",
-    "project",
-    "lists",
-    "label_links",
-    "issues",
-    "merge_requests"
-  ],
-  "milestone": [
-    "project",
-    "issues",
-    "labels",
-    "merge_requests",
-    "participants",
-    "events"
-  ],
-  "snippets": [
-    "author",
-    "project",
-    "notes"
-  ],
-  "releases": [
-    "project"
-  ],
-  "project_members": [
-    "created_by",
-    "user",
-    "source",
-    "project"
-  ],
-  "merge_requests": [
-    "subscriptions",
-    "award_emoji",
-    "author",
-    "assignee",
-    "updated_by",
-    "milestone",
-    "notes",
-    "label_links",
-    "labels",
-    "todos",
-    "target_project",
-    "source_project",
-    "merge_user",
-    "merge_request_diffs",
-    "merge_request_diff",
-    "events"
-  ],
-  "merge_request_diff": [
-    "merge_request"
-  ],
-  "pipelines": [
-    "project",
-    "user",
-    "statuses",
-    "builds",
-    "trigger_requests"
-  ],
-  "statuses": [
-    "project",
-    "pipeline",
-    "user"
-  ],
-  "variables": [
-    "project"
-  ],
-  "triggers": [
-    "project",
-    "trigger_requests"
-  ],
-  "deploy_keys": [
-    "user",
-    "deploy_keys_projects",
-    "projects"
-  ],
-  "services": [
-    "project",
-    "service_hook"
-  ],
-  "hooks": [
-    "project"
-  ],
-  "protected_branches": [
-    "project",
-    "merge_access_levels",
-    "push_access_levels"
-  ],
-  "project": [
-    "taggings",
-    "base_tags",
-    "tag_taggings",
-    "tags",
-    "creator",
-    "group",
-    "namespace",
-    "board",
-    "last_event",
-    "services",
-    "campfire_service",
-    "drone_ci_service",
-    "emails_on_push_service",
-    "builds_email_service",
-    "irker_service",
-    "pivotaltracker_service",
-    "hipchat_service",
-    "flowdock_service",
-    "assembla_service",
-    "asana_service",
-    "gemnasium_service",
-    "slack_service",
-    "buildkite_service",
-    "bamboo_service",
-    "teamcity_service",
-    "pushover_service",
-    "jira_service",
-    "redmine_service",
-    "custom_issue_tracker_service",
-    "bugzilla_service",
-    "gitlab_issue_tracker_service",
-    "external_wiki_service",
-    "forked_project_link",
-    "forked_from_project",
-    "forked_project_links",
-    "forks",
-    "merge_requests",
-    "fork_merge_requests",
-    "issues",
-    "labels",
-    "events",
-    "milestones",
-    "notes",
-    "snippets",
-    "hooks",
-    "protected_branches",
-    "project_members",
-    "users",
-    "requesters",
-    "deploy_keys_projects",
-    "deploy_keys",
-    "users_star_projects",
-    "starrers",
-    "releases",
-    "lfs_objects_projects",
-    "lfs_objects",
-    "project_group_links",
-    "invited_groups",
-    "todos",
-    "notification_settings",
-    "import_data",
-    "commit_statuses",
-    "pipelines",
-    "builds",
-    "runner_projects",
-    "runners",
-    "variables",
-    "triggers",
-    "environments",
-    "deployments"
-  ]
-}
\ No newline at end of file

spec/lib/gitlab/import_export/all_models.yml

+---
+issues:
+- subscriptions
+- award_emoji
+- author
+- assignee
+- updated_by
+- milestone
+- notes
+- label_links
+- labels
+- todos
+- user_agent_detail
+- project
+- moved_to
+- events
+events:
+- author
+- project
+- target
+notes:
+- award_emoji
+- project
+- noteable
+- author
+- updated_by
+- resolved_by
+- todos
+- events
+label_links:
+- target
+- label
+label:
+- subscriptions
+- project
+- lists
+- label_links
+- issues
+- merge_requests
+milestone:
+- project
+- issues
+- labels
+- merge_requests
+- participants
+- events
+snippets:
+- author
+- project
+- notes
+releases:
+- project
+project_members:
+- created_by
+- user
+- source
+- project
+merge_requests:
+- subscriptions
+- award_emoji
+- author
+- assignee
+- updated_by
+- milestone
+- notes
+- label_links
+- labels
+- todos
+- target_project
+- source_project
+- merge_user
+- merge_request_diffs
+- merge_request_diff
+- events
+merge_request_diff:
+- merge_request
+pipelines:
+- project
+- user
+- statuses
+- builds
+- trigger_requests
+statuses:
+- project
+- pipeline
+- user
+variables:
+- project
+triggers:
+- project
+- trigger_requests
+deploy_keys:
+- user
+- deploy_keys_projects
+- projects
+services:
+- project
+- service_hook
+hooks:
+- project
+protected_branches:
+- project
+- merge_access_levels
+- push_access_levels
+project:
+- taggings
+- base_tags
+- tag_taggings
+- tags
+- creator
+- group
+- namespace
+- board
+- last_event
+- services
+- campfire_service
+- drone_ci_service
+- emails_on_push_service
+- builds_email_service
+- irker_service
+- pivotaltracker_service
+- hipchat_service
+- flowdock_service
+- assembla_service
+- asana_service
+- gemnasium_service
+- slack_service
+- buildkite_service
+- bamboo_service
+- teamcity_service
+- pushover_service
+- jira_service
+- redmine_service
+- custom_issue_tracker_service
+- bugzilla_service
+- gitlab_issue_tracker_service
+- external_wiki_service
+- forked_project_link
+- forked_from_project
+- forked_project_links
+- forks
+- merge_requests
+- fork_merge_requests
+- issues
+- labels
+- events
+- milestones
+- notes
+- snippets
+- hooks
+- protected_branches
+- project_members
+- users
+- requesters
+- deploy_keys_projects
+- deploy_keys
+- users_star_projects
+- starrers
+- releases
+- lfs_objects_projects
+- lfs_objects
+- project_group_links
+- invited_groups
+- todos
+- notification_settings
+- import_data
+- commit_statuses
+- pipelines
+- builds
+- runner_projects
+- runners
+- variables
+- triggers
+- environments
+- deployments

spec/lib/gitlab/import_export/attribute_configuration_spec.rb

 require 'spec_helper'
 
+
+# Part of the Import/Export feature security testing
 # Checks whether there are new attributes in models that are currently being exported as part of the
 # project Import/Export feature.
 # If there are new attributes, these will have to either be added to this spec in case we want them
@@ -73,16 +75,6 @@ describe 'Attribute configuration', lib: true do
     MSG
   end
 
-  def parsed_attributes(relation_name, attributes)
-    excluded_attributes = config_hash['excluded_attributes'][relation_name]
-    included_attributes = config_hash['included_attributes'][relation_name]
-
-    attributes = attributes - JSON[excluded_attributes.to_json] if excluded_attributes
-    attributes = attributes & JSON[included_attributes.to_json] if included_attributes
-
-    attributes
-  end
-
   class Author < User
   end
 end

spec/lib/gitlab/import_export/model_configuration_spec.rb

 require 'spec_helper'
 
+# Part of the Import/Export feature security testing
 # Finds if a new model has been added that can potentially be part of the Import/Export
 # If it finds a new model, it will show a +failure_message+ with the options available.
 describe 'Model configuration', lib: true do
@@ -12,12 +13,12 @@ describe 'Model configuration', lib: true do
     # Remove duplicated or add missing models
     # - project is not part of the tree, so it has to be added manually.
     # - milestone, labels have both singular and plural versions in the tree, so remove the duplicates.
-    # - User, Author... Models we don not care about for checking relations
+    # - User, Author... Models we do not care about for checking relations
     names.flatten.uniq - ['milestones', 'labels', 'user', 'author'] + ['project']
   end
 
-  let(:all_models_json) { 'spec/lib/gitlab/import_export/all_models.json' }
-  let(:all_models) { JSON.parse(File.read(all_models_json)) }
+  let(:all_models_yml) { 'spec/lib/gitlab/import_export/all_models.yml' }
+  let(:all_models) { YAML.load_file(all_models_yml) }
   let(:current_models) { setup_models }
 
   it 'has no new models' do
@@ -43,15 +44,15 @@ describe 'Model configuration', lib: true do
     all_models_hash
   end
 
-  def failure_message(relation_name, new_models)
+  def failure_message(parent_model_name, new_models)
     <<-MSG
-      New model(s) <#{new_models.join(',')}> have been added, related to #{relation_name}, which is exported by
+      New model(s) <#{new_models.join(',')}> have been added, related to #{parent_model_name}, which is exported by
       the Import/Export feature.
 
-      If you don't think this should be exported, please add it to MODELS_JSON, inside the #{relation_name} hash.
-      If you think we should export this new model, please add it to IMPORT_EXPORT_CONFIG.
+      If you don't think this should be exported, please add it to MODELS_JSON, inside the #{parent_model_name} hash.
+      If you think we should export this new model, please add it to IMPORT_EXPORT_CONFIG and to MODELS_JSON.
 
-      MODELS_JSON: #{File.expand_path(all_models_json)}
+      MODELS_JSON: #{File.expand_path(all_models_yml)}
       IMPORT_EXPORT_CONFIG: #{Gitlab::ImportExport.config_file}
     MSG
   end

spec/support/import_export/configuration_helper.rb

@@ -12,4 +12,14 @@ module ConfigurationHelper
     relation_name = Gitlab::ImportExport::RelationFactory::OVERRIDES[relation_name.to_sym] || relation_name
     relation_name.to_s.classify.constantize
   end
+
+  def parsed_attributes(relation_name, attributes)
+    excluded_attributes = config_hash['excluded_attributes'][relation_name]
+    included_attributes = config_hash['included_attributes'][relation_name]
+
+    attributes = attributes - JSON[excluded_attributes.to_json] if excluded_attributes
+    attributes = attributes & JSON[included_attributes.to_json] if included_attributes
+
+    attributes
+  end
 end

spec/support/import_export/export_file_helper.rb

 module ExportFileHelper
+  include ConfigurationHelper
+
+  ObjectWithParent = Struct.new(:object, :parent, :key_found)
+
   def setup_project
     project = create(:project, :public)
 
@@ -54,48 +58,78 @@ module ExportFileHelper
 
   # Recursively finds key/values including +key+ as part of the key, inside a nested hash
   def deep_find_with_parent(sensitive_key_word, object, found = nil)
+    sensitive_key_found = object_contains_key?(object, sensitive_key_word)
+
     # Returns the parent object and the object found containing a sensitive word as part of the key
-    if object_contains_key?(object, sensitive_key_word)
-      [object[sensitive_key_word], object] if object[sensitive_key_word]
+    if sensitive_key_found && object[sensitive_key_found]
+      ObjectWithParent.new(object[sensitive_key_found], object, sensitive_key_found)
     elsif object.is_a?(Enumerable)
       # Recursively lookup for keys containing sensitive words in a Hash or Array
-      object.find { |*hash_or_array| found, object = deep_find_with_parent(sensitive_key_word, hash_or_array.last, found) }
-      [found, object] if found
+      object_with_parent = nil
+
+      object.find do |*hash_or_array|
+        object_with_parent = deep_find_with_parent(sensitive_key_word, hash_or_array.last, found)
+      end
+
+      object_with_parent
     end
   end
 
   # Return true if the hash has a key containing a sensitive word
   def object_contains_key?(object, sensitive_key_word)
-    object.is_a?(Hash) && object.keys.any? { |key| key.include?(sensitive_key_word) }
+    return false unless object.is_a?(Hash)
+
+    object.keys.find { |key| key.include?(sensitive_key_word) }
   end
 
-  # Returns true if a sensitive word is found inside a hash, excluding safe hashes
-  def has_sensitive_attributes?(sensitive_word, project_hash)
+  # Returns the offended ObjectWithParent object if a sensitive word is found inside a hash,
+  # excluding the whitelisted safe hashes.
+  def find_sensitive_attributes(sensitive_word, project_hash)
     loop do
-      object, parent = deep_find_with_parent(sensitive_word, project_hash)
+      object_with_parent = deep_find_with_parent(sensitive_word, project_hash)
+
+      return nil unless object_with_parent && object_with_parent.object
 
-      if object && is_safe_hash?(parent, sensitive_word)
+      if is_safe_hash?(object_with_parent.parent, sensitive_word)
         # It's in the safe list, remove hash and keep looking
-        parent.delete(object)
-      elsif object
-        return true
+        object_with_parent.parent.delete(object_with_parent.key_found)
       else
-        return false
+        return object_with_parent
       end
+
+      nil
     end
   end
 
-  # Returns true if it's one of the excluded models in +safe_models+
+  # Returns true if it's one of the excluded models in +safe_list+
   def is_safe_hash?(parent, sensitive_word)
-    return false unless parent
+    return false unless parent && safe_list[sensitive_word.to_sym]
 
     # Extra attributes that appear in a model but not in the exported hash.
     excluded_attributes = ['type']
 
-    safe_models[sensitive_word.to_sym].each do |safe_model|
-      return true if (safe_model.attribute_names - parent.keys - excluded_attributes).empty?
+    safe_list[sensitive_word.to_sym].each do |model|
+      # Check whether this is a hash attribute inside a model
+      if model.is_a?(Symbol)
+        return true if (safe_hashes[model] - parent.keys).empty?
+      else
+        return true if safe_model?(model, excluded_attributes, parent)
+      end
     end
 
     false
   end
+
+  def associations_for(safe_model)
+    safe_model.reflect_on_all_associations.map { |assoc| assoc.name.to_s }
+  end
+
+  # Compares model attributes with those those found in the hash
+  # and returns true if there is a match, ignoring some excluded attributes.
+  def safe_model?(model, excluded_attributes, parent)
+    excluded_attributes += associations_for(model)
+    parsed_model_attributes = parsed_attributes(model.name.underscore, model.attribute_names)
+
+    (parsed_model_attributes - parent.keys - excluded_attributes).empty?
+  end
 end