Merge branch '22406-update-existing-subgroups-to-match-visibility-level-of-parent-v2' into 'master'

Fix visibility levels of specific subgroups

See merge request gitlab-org/gitlab!22889

changelogs/unreleased/22406-update-existing-subgroups-to-match-visibility-level-of-parent-v2.yml

+---
+title: Fix visibility levels of subgroups to be not higher than their parents' level
+merge_request: 22889
+author:
+type: other

db/post_migrate/20200110121314_schedule_update_existing_subgroup_to_match_visibility_level_of_parent.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class ScheduleUpdateExistingSubgroupToMatchVisibilityLevelOfParent < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  MIGRATION = 'UpdateExistingSubgroupToMatchVisibilityLevelOfParent'
+  DELAY_INTERVAL = 5.minutes.to_i
+  BATCH_SIZE = 1000
+  VISIBILITY_LEVELS = {
+    internal: 10,
+    private: 0
+  }
+
+  disable_ddl_transaction!
+
+  def up
+    offset = update_groups(VISIBILITY_LEVELS[:internal])
+    update_groups(VISIBILITY_LEVELS[:private], offset: offset)
+  end
+
+  def down
+    # no-op
+  end
+
+  private
+
+  def update_groups(level, offset: 0)
+    groups = exec_query <<~SQL
+      SELECT id
+      FROM namespaces
+      WHERE visibility_level = #{level}
+      AND type = 'Group'
+      AND EXISTS (SELECT 1
+                  FROM namespaces AS children
+                  WHERE children.parent_id = namespaces.id)
+    SQL
+
+    ids = groups.rows.flatten
+
+    iterator = 1
+
+    ids.in_groups_of(BATCH_SIZE, false) do |batch_of_ids|
+      delay = DELAY_INTERVAL * (iterator + offset)
+      BackgroundMigrationWorker.perform_in(delay, MIGRATION, [batch_of_ids, level])
+      iterator += 1
+    end
+
+    say("Background jobs for visibility level #{level} scheduled in #{iterator} iterations")
+
+    offset + iterator
+  end
+end

lib/gitlab/background_migration/update_existing_subgroup_to_match_visibility_level_of_parent.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # This background migration updates children of group to match visibility of a parent
+    class UpdateExistingSubgroupToMatchVisibilityLevelOfParent
+      def perform(parents_groups_ids, level)
+        groups_ids = Gitlab::ObjectHierarchy.new(Group.where(id: parents_groups_ids))
+          .base_and_descendants
+          .where("visibility_level > ?", level)
+          .select(:id)
+
+        return if groups_ids.empty?
+
+        Group
+          .where(id: groups_ids)
+          .update_all(visibility_level: level)
+      end
+    end
+  end
+end

spec/lib/gitlab/background_migration/update_existing_subgroup_to_match_visibility_level_of_parent_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::UpdateExistingSubgroupToMatchVisibilityLevelOfParent, :migration, schema: 2020_01_10_121314 do
+  include MigrationHelpers::NamespacesHelpers
+
+  context 'private visibility level' do
+    it 'updates the project visibility' do
+      parent = create_namespace('parent', Gitlab::VisibilityLevel::PRIVATE)
+      child = create_namespace('child', Gitlab::VisibilityLevel::PUBLIC, parent_id: parent.id)
+
+      expect { subject.perform([parent.id], Gitlab::VisibilityLevel::PRIVATE) }.to change { child.reload.visibility_level }.to(Gitlab::VisibilityLevel::PRIVATE)
+    end
+
+    it 'updates sub-sub groups' do
+      parent = create_namespace('parent', Gitlab::VisibilityLevel::PRIVATE)
+      middle_group = create_namespace('middle', Gitlab::VisibilityLevel::PRIVATE, parent_id: parent.id)
+      child = create_namespace('child', Gitlab::VisibilityLevel::PUBLIC, parent_id: middle_group.id)
+
+      subject.perform([parent.id, middle_group.id], Gitlab::VisibilityLevel::PRIVATE)
+
+      expect(child.reload.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE)
+    end
+
+    it 'updates all sub groups' do
+      parent = create_namespace('parent', Gitlab::VisibilityLevel::PRIVATE)
+      middle_group = create_namespace('middle', Gitlab::VisibilityLevel::PUBLIC, parent_id: parent.id)
+      child = create_namespace('child', Gitlab::VisibilityLevel::PUBLIC, parent_id: middle_group.id)
+
+      subject.perform([parent.id], Gitlab::VisibilityLevel::PRIVATE)
+
+      expect(child.reload.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE)
+      expect(middle_group.reload.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE)
+    end
+  end
+
+  context 'internal visibility level' do
+    it 'updates the project visibility' do
+      parent = create_namespace('parent', Gitlab::VisibilityLevel::INTERNAL)
+      child = create_namespace('child', Gitlab::VisibilityLevel::PUBLIC, parent_id: parent.id)
+
+      expect { subject.perform([parent.id], Gitlab::VisibilityLevel::INTERNAL) }.to change { child.reload.visibility_level }.to(Gitlab::VisibilityLevel::INTERNAL)
+    end
+  end
+end

spec/migrations/schedule_update_existing_subgroup_to_match_visibility_level_of_parent_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200110121314_schedule_update_existing_subgroup_to_match_visibility_level_of_parent.rb')
+
+describe ScheduleUpdateExistingSubgroupToMatchVisibilityLevelOfParent, :migration, :sidekiq do
+  include MigrationHelpers::NamespacesHelpers
+  let(:migration_class) { described_class::MIGRATION }
+  let(:migration_name)  { migration_class.to_s.demodulize }
+
+  context 'private visibility level' do
+    it 'correctly schedules background migrations' do
+      parent = create_namespace('parent', Gitlab::VisibilityLevel::PRIVATE)
+      create_namespace('child', Gitlab::VisibilityLevel::PUBLIC, parent_id: parent.id)
+
+      Sidekiq::Testing.fake! do
+        Timecop.freeze do
+          migrate!
+
+          expect(BackgroundMigrationWorker.jobs.size).to eq(1)
+          expect(migration_name).to be_scheduled_migration_with_multiple_args([parent.id], Gitlab::VisibilityLevel::PRIVATE)
+        end
+      end
+    end
+
+    it 'correctly schedules background migrations for groups and subgroups' do
+      parent = create_namespace('parent', Gitlab::VisibilityLevel::PRIVATE)
+      middle_group = create_namespace('middle_group', Gitlab::VisibilityLevel::PRIVATE, parent_id: parent.id)
+      create_namespace('middle_empty_group', Gitlab::VisibilityLevel::PRIVATE, parent_id: parent.id)
+      create_namespace('child', Gitlab::VisibilityLevel::PUBLIC, parent_id: middle_group.id)
+
+      Sidekiq::Testing.fake! do
+        Timecop.freeze do
+          migrate!
+
+          expect(BackgroundMigrationWorker.jobs.size).to eq(1)
+          expect(migration_name).to be_scheduled_migration_with_multiple_args([middle_group.id, parent.id], Gitlab::VisibilityLevel::PRIVATE)
+        end
+      end
+    end
+  end
+
+  context 'internal visibility level' do
+    it 'correctly schedules background migrations' do
+      parent = create_namespace('parent', Gitlab::VisibilityLevel::INTERNAL)
+      middle_group = create_namespace('child', Gitlab::VisibilityLevel::INTERNAL, parent_id: parent.id)
+      create_namespace('child', Gitlab::VisibilityLevel::PUBLIC, parent_id: middle_group.id)
+
+      Sidekiq::Testing.fake! do
+        Timecop.freeze do
+          migrate!
+
+          expect(BackgroundMigrationWorker.jobs.size).to eq(1)
+          expect(migration_name).to be_scheduled_migration_with_multiple_args([parent.id, middle_group.id], Gitlab::VisibilityLevel::INTERNAL)
+        end
+      end
+    end
+  end
+
+  context 'mixed visibility levels' do
+    it 'correctly schedules background migrations' do
+      parent1 = create_namespace('parent1', Gitlab::VisibilityLevel::INTERNAL)
+      create_namespace('child', Gitlab::VisibilityLevel::PUBLIC, parent_id: parent1.id)
+      parent2 = create_namespace('parent2', Gitlab::VisibilityLevel::PRIVATE)
+      middle_group = create_namespace('middle_group', Gitlab::VisibilityLevel::INTERNAL, parent_id: parent2.id)
+      create_namespace('child', Gitlab::VisibilityLevel::PUBLIC, parent_id: middle_group.id)
+
+      Sidekiq::Testing.fake! do
+        Timecop.freeze do
+          migrate!
+
+          expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+          expect(migration_name).to be_scheduled_migration_with_multiple_args([parent1.id, middle_group.id], Gitlab::VisibilityLevel::INTERNAL)
+          expect(migration_name).to be_scheduled_migration_with_multiple_args([parent2.id], Gitlab::VisibilityLevel::PRIVATE)
+        end
+      end
+    end
+  end
+end

spec/support/matchers/background_migrations_matchers.rb

@@ -26,3 +26,26 @@ RSpec::Matchers.define :be_scheduled_migration do |*expected|
     "Migration `#{migration}` with args `#{expected.inspect}` not scheduled!"
   end
 end
+
+RSpec::Matchers.define :be_scheduled_migration_with_multiple_args do |*expected|
+  match do |migration|
+    BackgroundMigrationWorker.jobs.any? do |job|
+      args = job['args'].size == 1 ? [BackgroundMigrationWorker.jobs[0]['args'][0], []] : job['args']
+      args[0] == migration && compare_args(args, expected)
+    end
+  end
+
+  failure_message do |migration|
+    "Migration `#{migration}` with args `#{expected.inspect}` not scheduled!"
+  end
+
+  def compare_args(args, expected)
+    args[1].map.with_index do |arg, i|
+      arg.is_a?(Array) ? same_arrays?(arg, expected[i]) : arg == expected[i]
+    end.all?
+  end
+
+  def same_arrays?(arg, expected)
+    arg.sort == expected.sort
+  end
+end

spec/support/migrations_helpers/namespaces_helper.rb

+# frozen_string_literal: true
+
+module MigrationHelpers
+  module NamespacesHelpers
+    def create_namespace(name, visibility, options = {})
+      table(:namespaces).create({
+                          name: name,
+                          path: name,
+                          type: 'Group',
+                          visibility_level: visibility
+                        }.merge(options))
+    end
+  end
+end