From 19f9666abf847efd4a5cb50d38faee77f91ec233 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 19 Dec 2018 12:49:59 +0100 Subject: [PATCH] Fix tree restorer visibility level --- .../security-import-project-visibility.yml | 5 ++ ..._update_project_import_visibility_level.rb | 60 +++++++++++++ .../import_export/project_tree_restorer.rb | 9 +- .../project_tree_restorer_spec.rb | 61 ++++++++++++- ...te_project_import_visibility_level_spec.rb | 86 +++++++++++++++++++ 5 files changed, 219 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/security-import-project-visibility.yml create mode 100644 db/post_migrate/20181219130552_update_project_import_visibility_level.rb create mode 100644 spec/migrations/update_project_import_visibility_level_spec.rb 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 -- GitLab