Merge branch 'security-update_project_authorizations_on_group_delete-master' into 'master'

Refresh ProjectAuthorization during Group deletion

Closes #112

See merge request gitlab-org/security/gitlab!417

app/services/groups/destroy_service.rb

@@ -29,7 +29,15 @@ module Groups
 
       group.chat_team&.remove_mattermost_team(current_user)
 
+      user_ids_for_project_authorizations_refresh = group.user_ids_for_project_authorizations
+
       group.destroy
+
+      UserProjectAccessChangedService
+        .new(user_ids_for_project_authorizations_refresh)
+        .execute(blocking: true)
+
+      group
     end
     # rubocop: enable CodeReuse/ActiveRecord
   end

changelogs/unreleased/213665-update_project_authorizations_on_group_delete.yml

+---
+title: Refresh ProjectAuthorization during Group deletion
+merge_request:
+author:
+type: security

db/post_migrate/20200204113225_schedule_recalculate_project_authorizations_third_run.rb

+# frozen_string_literal: true
+
+class ScheduleRecalculateProjectAuthorizationsThirdRun < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  MIGRATION = 'RecalculateProjectAuthorizationsWithMinMaxUserId'
+  BATCH_SIZE = 2_500
+  DELAY_INTERVAL = 2.minutes.to_i
+
+  disable_ddl_transaction!
+
+  class User < ActiveRecord::Base
+    include ::EachBatch
+
+    self.table_name = 'users'
+  end
+
+  def up
+    say "Scheduling #{MIGRATION} jobs"
+
+    queue_background_migration_jobs_by_range_at_intervals(User, MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE)
+  end
+
+  def down
+  end
+end

db/structure.sql

@@ -12899,6 +12899,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200204070729
 20200204113223
 20200204113224
+20200204113225
 20200204131054
 20200204131831
 20200205143231

spec/migrations/schedule_recalculate_project_authorizations_third_run_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200204113225_schedule_recalculate_project_authorizations_third_run.rb')
+
+describe ScheduleRecalculateProjectAuthorizationsThirdRun do
+  let(:users_table) { table(:users) }
+
+  before do
+    stub_const("#{described_class}::BATCH_SIZE", 2)
+
+    1.upto(4) do |i|
+      users_table.create!(id: i, name: "user#{i}", email: "user#{i}@example.com", projects_limit: 1)
+    end
+  end
+
+  it 'schedules background migration' do
+    Sidekiq::Testing.fake! do
+      Timecop.freeze do
+        migrate!
+
+        expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+        expect(described_class::MIGRATION).to be_scheduled_migration(1, 2)
+        expect(described_class::MIGRATION).to be_scheduled_migration(3, 4)
+      end
+    end
+  end
+end

spec/services/groups/destroy_service_spec.rb

@@ -133,4 +133,55 @@ describe Groups::DestroyService do
       end
     end
   end
+
+  describe 'authorization updates', :sidekiq_inline do
+    context 'shared groups' do
+      let!(:shared_group) { create(:group, :private) }
+      let!(:shared_group_child) { create(:group, :private, parent: shared_group) }
+
+      let!(:project) { create(:project, group: shared_group) }
+      let!(:project_child) { create(:project, group: shared_group_child) }
+
+      before do
+        create(:group_group_link, shared_group: shared_group, shared_with_group: group)
+        group.refresh_members_authorized_projects
+      end
+
+      it 'updates project authorization' do
+        expect(user.can?(:read_project, project)).to eq(true)
+        expect(user.can?(:read_project, project_child)).to eq(true)
+
+        destroy_group(group, user, false)
+
+        expect(user.can?(:read_project, project)).to eq(false)
+        expect(user.can?(:read_project, project_child)).to eq(false)
+      end
+    end
+
+    context 'shared groups in the same group hierarchy' do
+      let!(:subgroup) { create(:group, :private, parent: group) }
+      let!(:subgroup_user) { create(:user) }
+
+      before do
+        subgroup.add_user(subgroup_user, Gitlab::Access::MAINTAINER)
+
+        create(:group_group_link, shared_group: group, shared_with_group: subgroup)
+        subgroup.refresh_members_authorized_projects
+      end
+
+      context 'group is deleted' do
+        it 'updates project authorization' do
+          expect { destroy_group(group, user, false) }.to(
+            change { subgroup_user.can?(:read_project, project) }.from(true).to(false))
+        end
+      end
+
+      context 'subgroup is deleted' do
+        it 'updates project authorization' do
+          expect { destroy_group(subgroup, user, false) }.to(
+            change { subgroup_user.can?(:read_project, project) }.from(true).to(false))
+        end
+      end
+    end
+  end
 end