Merge branch 'security-import-project-visibility' into 'master'

[master] Fix Imported Project Retains Prior Visibility Setting

See merge request gitlab/gitlabhq!2734

5 files changed, 219 insertions, 2 deletions

diff --git a/changelogs/unreleased/security-import-project-visibility.yml b/changelogs/unreleased/security-import-project-visibility.yml
new file mode 100644
index 00000000000..04ae172a9a1
--- /dev/null
+++ b/changelogs/unreleased/security-import-project-visibility.yml
@@ -0,0 +1,5 @@
+---
+title: Restrict project import visibility based on its group
+merge_request:
+author:
+type: security
diff --git a/db/post_migrate/20181219130552_update_project_import_visibility_level.rb b/db/post_migrate/20181219130552_update_project_import_visibility_level.rb
new file mode 100644
index 00000000000..6209de88b31
--- /dev/null
+++ b/db/post_migrate/20181219130552_update_project_import_visibility_level.rb
@@ -0,0 +1,60 @@
+# frozen_string_literal: true
+
+class UpdateProjectImportVisibilityLevel < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  BATCH_SIZE = 100
+
+  PRIVATE = 0
+  INTERNAL = 10
+
+  disable_ddl_transaction!
+
+  class Namespace < ActiveRecord::Base
+    self.table_name = 'namespaces'
+  end
+
+  class Project < ActiveRecord::Base
+    include EachBatch
+
+    belongs_to :namespace
+
+    IMPORT_TYPE = 'gitlab_project'
+
+    scope :with_group_visibility, ->(visibility) do
+      joins(:namespace)
+        .where(namespaces: { type: 'Group', visibility_level: visibility })
+        .where(import_type: IMPORT_TYPE)
+        .where('projects.visibility_level > namespaces.visibility_level')
+    end
+
+    self.table_name = 'projects'
+  end
+
+  def up
+    # Update project's visibility to be the same as the group
+    # if it is more restrictive than `PUBLIC`.
+    update_projects_visibility(PRIVATE)
+    update_projects_visibility(INTERNAL)
+  end
+
+  def down
+    # no-op: unrecoverable data migration
+  end
+
+  private
+
+  def update_projects_visibility(visibility)
+    say_with_time("Updating project visibility to #{visibility} on #{Project::IMPORT_TYPE} imports.") do
+      Project.with_group_visibility(visibility).select(:id).each_batch(of: BATCH_SIZE) do |batch, _index|
+        batch_sql = Gitlab::Database.mysql? ? batch.pluck(:id).join(', ') : batch.select(:id).to_sql
+
+        say("Updating #{batch.size} items.", true)
+
+        execute("UPDATE projects SET visibility_level = '#{visibility}' WHERE id IN (#{batch_sql})")
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/import_export/project_tree_restorer.rb b/lib/gitlab/import_export/project_tree_restorer.rb
index a56ec65b9f1..51001750a6c 100644
--- a/lib/gitlab/import_export/project_tree_restorer.rb
+++ b/lib/gitlab/import_export/project_tree_restorer.rb
@@ -107,7 +107,7 @@ module Gitlab
 
       def project_params
         @project_params ||= begin
-          attrs = json_params.merge(override_params)
+          attrs = json_params.merge(override_params).merge(visibility_level)
 
           # Cleaning all imported and overridden params
           Gitlab::ImportExport::AttributeCleaner.clean(relation_hash: attrs,
@@ -127,6 +127,13 @@ module Gitlab
         end
       end
 
+      def visibility_level
+        level = override_params['visibility_level'] || json_params['visibility_level'] || @project.visibility_level
+        level = @project.group.visibility_level if @project.group && level > @project.group.visibility_level
+
+        { 'visibility_level' => level }
+      end
+
       # Given a relation hash containing one or more models and its relationships,
       # loops through each model and each object from a model type and
       # and assigns its correspondent attributes hash from +tree_hash+
diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
index 242c16c4bdc..9b0da882390 100644
--- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
+++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
@@ -273,6 +273,11 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
     it 'has group milestone' do
       expect(project.group.milestones.size).to eq(results.fetch(:milestones, 0))
     end
+
+    it 'has the correct visibility level' do
+      # INTERNAL in the `project.json`, group's is PRIVATE
+      expect(project.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE)
+    end
   end
 
   context 'Light JSON' do
@@ -347,7 +352,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
                :issues_disabled,
                name: 'project',
                path: 'project',
-               group: create(:group))
+               group: create(:group, visibility_level: Gitlab::VisibilityLevel::PRIVATE))
       end
 
       before do
@@ -434,4 +439,58 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
       end
     end
   end
+
+  describe '#restored_project' do
+    let(:project) { create(:project) }
+    let(:shared) { project.import_export_shared }
+    let(:tree_hash) { { 'visibility_level' => visibility } }
+    let(:restorer) { described_class.new(user: nil, shared: shared, project: project) }
+
+    before do
+      restorer.instance_variable_set(:@tree_hash, tree_hash)
+    end
+
+    context 'no group visibility' do
+      let(:visibility) { Gitlab::VisibilityLevel::PRIVATE }
+
+      it 'uses the project visibility' do
+        expect(restorer.restored_project.visibility_level).to eq(visibility)
+      end
+    end
+
+    context 'with group visibility' do
+      before do
+        group = create(:group, visibility_level: group_visibility)
+
+        project.update(group: group)
+      end
+
+      context 'private group visibility' do
+        let(:group_visibility) { Gitlab::VisibilityLevel::PRIVATE }
+        let(:visibility) { Gitlab::VisibilityLevel::PUBLIC }
+
+        it 'uses the group visibility' do
+          expect(restorer.restored_project.visibility_level).to eq(group_visibility)
+        end
+      end
+
+      context 'public group visibility' do
+        let(:group_visibility) { Gitlab::VisibilityLevel::PUBLIC }
+        let(:visibility) { Gitlab::VisibilityLevel::PRIVATE }
+
+        it 'uses the project visibility' do
+          expect(restorer.restored_project.visibility_level).to eq(visibility)
+        end
+      end
+
+      context 'internal group visibility' do
+        let(:group_visibility) { Gitlab::VisibilityLevel::INTERNAL }
+        let(:visibility) { Gitlab::VisibilityLevel::PUBLIC }
+
+        it 'uses the group visibility' do
+          expect(restorer.restored_project.visibility_level).to eq(group_visibility)
+        end
+      end
+    end
+  end
 end
diff --git a/spec/migrations/update_project_import_visibility_level_spec.rb b/spec/migrations/update_project_import_visibility_level_spec.rb
new file mode 100644
index 00000000000..9ea9b956f67
--- /dev/null
+++ b/spec/migrations/update_project_import_visibility_level_spec.rb
@@ -0,0 +1,86 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20181219130552_update_project_import_visibility_level.rb')
+
+describe UpdateProjectImportVisibilityLevel, :migration do
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:project) { projects.find_by_name(name) }
+
+  before do
+    stub_const("#{described_class}::BATCH_SIZE", 1)
+  end
+
+  context 'private visibility level' do
+    let(:name) { 'private-public' }
+
+    it 'updates the project visibility' do
+      create_namespace(name, Gitlab::VisibilityLevel::PRIVATE)
+      create_project(name, Gitlab::VisibilityLevel::PUBLIC)
+
+      expect { migrate! }.to change { project.reload.visibility_level }.to(Gitlab::VisibilityLevel::PRIVATE)
+    end
+  end
+
+  context 'internal visibility level' do
+    let(:name) { 'internal-public' }
+
+    it 'updates the project visibility' do
+      create_namespace(name, Gitlab::VisibilityLevel::INTERNAL)
+      create_project(name, Gitlab::VisibilityLevel::PUBLIC)
+
+      expect { migrate! }.to change { project.reload.visibility_level }.to(Gitlab::VisibilityLevel::INTERNAL)
+    end
+  end
+
+  context 'public visibility level' do
+    let(:name) { 'public-public' }
+
+    it 'does not update the project visibility' do
+      create_namespace(name, Gitlab::VisibilityLevel::PUBLIC)
+      create_project(name, Gitlab::VisibilityLevel::PUBLIC)
+
+      expect { migrate! }.not_to change { project.reload.visibility_level }
+    end
+  end
+
+  context 'private project visibility level' do
+    let(:name) { 'public-private' }
+
+    it 'does not update the project visibility' do
+      create_namespace(name, Gitlab::VisibilityLevel::PUBLIC)
+      create_project(name, Gitlab::VisibilityLevel::PRIVATE)
+
+      expect { migrate! }.not_to change { project.reload.visibility_level }
+    end
+  end
+
+  context 'no namespace' do
+    let(:name) { 'no-namespace' }
+
+    it 'does not update the project visibility' do
+      create_namespace(name, Gitlab::VisibilityLevel::PRIVATE, type: nil)
+      create_project(name, Gitlab::VisibilityLevel::PUBLIC)
+
+      expect { migrate! }.not_to change { project.reload.visibility_level }
+    end
+  end
+
+  def create_namespace(name, visibility, options = {})
+    namespaces.create({
+                        name: name,
+                        path: name,
+                        type: 'Group',
+                        visibility_level: visibility
+                      }.merge(options))
+  end
+
+  def create_project(name, visibility)
+    projects.create!(namespace_id: namespaces.find_by_name(name).id,
+                     name: name,
+                     path: name,
+                     import_type: 'gitlab_project',
+                     visibility_level: visibility)
+  end
+end