diff --git a/app/services/groups/destroy_service.rb b/app/services/groups/destroy_service.rb index c9c6b54a791d650a93ebec32dc1438a8d0d0339d..9437eb9eede691ffff9278656a88c001b605601d 100644 --- a/app/services/groups/destroy_service.rb +++ b/app/services/groups/destroy_service.rb @@ -29,7 +29,15 @@ def execute 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 diff --git a/changelogs/unreleased/213665-update_project_authorizations_on_group_delete.yml b/changelogs/unreleased/213665-update_project_authorizations_on_group_delete.yml new file mode 100644 index 0000000000000000000000000000000000000000..8b7443f25ba1f028fa7cf298c807218d364acd67 --- /dev/null +++ b/changelogs/unreleased/213665-update_project_authorizations_on_group_delete.yml @@ -0,0 +1,5 @@ +--- +title: Refresh ProjectAuthorization during Group deletion +merge_request: +author: +type: security diff --git a/db/post_migrate/20200204113225_schedule_recalculate_project_authorizations_third_run.rb b/db/post_migrate/20200204113225_schedule_recalculate_project_authorizations_third_run.rb new file mode 100644 index 0000000000000000000000000000000000000000..47b22b4800aeaf24781c1f95c633d481990b1335 --- /dev/null +++ b/db/post_migrate/20200204113225_schedule_recalculate_project_authorizations_third_run.rb @@ -0,0 +1,28 @@ +# 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 diff --git a/db/structure.sql b/db/structure.sql index 895afed92e68996d65a921d2bb69e0a309eb103e..cfd854be9f15f07339c04a2edc6a46c4c2041c82 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -12899,6 +12899,7 @@ COPY "schema_migrations" (version) FROM STDIN; 20200204070729 20200204113223 20200204113224 +20200204113225 20200204131054 20200204131831 20200205143231 diff --git a/spec/migrations/schedule_recalculate_project_authorizations_third_run_spec.rb b/spec/migrations/schedule_recalculate_project_authorizations_third_run_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..19ba89842245bd0a19c1f4b5af1032ca9f33782f --- /dev/null +++ b/spec/migrations/schedule_recalculate_project_authorizations_third_run_spec.rb @@ -0,0 +1,28 @@ +# 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 diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb index a45c7cdffa641be03850332caf962568aef5f124..bf639153b9926a1eaed238c8f20e6ba051ea24de 100644 --- a/spec/services/groups/destroy_service_spec.rb +++ b/spec/services/groups/destroy_service_spec.rb @@ -133,4 +133,55 @@ def destroy_group(group, user, async) 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