Merge branch '343277-vfazio-fix-prj-auth-on-transfer' into 'master'

Recalculate project authorizations on group transfer

See merge request gitlab-org/gitlab!72550

app/services/authorized_project_update/project_access_changed_service.rb

+# frozen_string_literal: true
+
+module AuthorizedProjectUpdate
+  class ProjectAccessChangedService
+    def initialize(project_ids)
+      @project_ids = Array.wrap(project_ids)
+    end
+
+    def execute(blocking: true)
+      bulk_args = @project_ids.map { |id| [id] }
+
+      if blocking
+        AuthorizedProjectUpdate::ProjectRecalculateWorker.bulk_perform_and_wait(bulk_args)
+      else
+        AuthorizedProjectUpdate::ProjectRecalculateWorker.bulk_perform_async(bulk_args) # rubocop:disable Scalability/BulkPerformWithContext
+      end
+    end
+  end
+end

app/services/groups/transfer_service.rb

@@ -175,21 +175,18 @@ module Groups
     end
 
     def refresh_project_authorizations
-      ProjectAuthorization.where(project_id: @group.all_projects.select(:id)).delete_all # rubocop: disable CodeReuse/ActiveRecord
+      projects_to_update = Set.new
 
-      # refresh authorized projects for current_user immediately
-      current_user.refresh_authorized_projects
-
-      # schedule refreshing projects for all the members of the group
-      @group.refresh_members_authorized_projects
+      # All projects in this hierarchy need to have their project authorizations recalculated
+      @group.all_projects.each_batch { |prjs| projects_to_update.merge(prjs.ids) } # rubocop: disable CodeReuse/ActiveRecord
 
       # When a group is transferred, it also affects who gets access to the projects shared to
       # the subgroups within its hierarchy, so we also schedule jobs that refresh authorizations for all such shared projects.
-      project_group_shares_within_the_hierarchy = ProjectGroupLink.in_group(group.self_and_descendants.select(:id))
-
-      project_group_shares_within_the_hierarchy.find_each do |project_group_link|
-        AuthorizedProjectUpdate::ProjectRecalculateWorker.perform_async(project_group_link.project_id)
+      ProjectGroupLink.in_group(@group.self_and_descendants.select(:id)).each_batch do |project_group_links|
+        projects_to_update.merge(project_group_links.pluck(:project_id)) # rubocop: disable CodeReuse/ActiveRecord
       end
+
+      AuthorizedProjectUpdate::ProjectAccessChangedService.new(projects_to_update.to_a).execute unless projects_to_update.empty?
     end
 
     def raise_transfer_error(message)

app/workers/authorized_project_update/project_recalculate_worker.rb

@@ -7,6 +7,8 @@ module AuthorizedProjectUpdate
     data_consistency :always
     include Gitlab::ExclusiveLeaseHelpers
 
+    prepend WaitableWorker
+
     feature_category :authentication_and_authorization
     urgency :high
     queue_namespace :authorized_project_update

spec/services/authorized_project_update/project_access_changed_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe AuthorizedProjectUpdate::ProjectAccessChangedService do
+  describe '#execute' do
+    it 'schedules the project IDs' do
+      expect(AuthorizedProjectUpdate::ProjectRecalculateWorker).to receive(:bulk_perform_and_wait)
+        .with([[1], [2]])
+
+      described_class.new([1, 2]).execute
+    end
+
+    it 'permits non-blocking operation' do
+      expect(AuthorizedProjectUpdate::ProjectRecalculateWorker).to receive(:bulk_perform_async)
+        .with([[1], [2]])
+
+      described_class.new([1, 2]).execute(blocking: false)
+    end
+  end
+end

spec/services/groups/transfer_service_spec.rb

@@ -593,11 +593,16 @@ RSpec.describe Groups::TransferService, :sidekiq_inline do
           let_it_be_with_reload(:group) { create(:group, :private, parent: old_parent_group) }
           let_it_be(:new_group_member) { create(:user) }
           let_it_be(:old_group_member) { create(:user) }
+          let_it_be(:unique_subgroup_member) { create(:user) }
+          let_it_be(:direct_project_member) { create(:user) }
 
           before do
             new_parent_group.add_maintainer(new_group_member)
             old_parent_group.add_maintainer(old_group_member)
+            subgroup1.add_developer(unique_subgroup_member)
+            nested_project.add_developer(direct_project_member)
             group.refresh_members_authorized_projects
+            subgroup1.refresh_members_authorized_projects
           end
 
           it 'removes old project authorizations' do
@@ -613,7 +618,7 @@ RSpec.describe Groups::TransferService, :sidekiq_inline do
           end
 
           it 'performs authorizations job immediately' do
-            expect(AuthorizedProjectsWorker).to receive(:bulk_perform_inline)
+            expect(AuthorizedProjectUpdate::ProjectRecalculateWorker).to receive(:bulk_perform_inline)
 
             transfer_service.execute(new_parent_group)
           end
@@ -630,14 +635,24 @@ RSpec.describe Groups::TransferService, :sidekiq_inline do
                 ProjectAuthorization.where(project_id: nested_project.id, user_id: new_group_member.id).size
               }.from(0).to(1)
             end
+
+            it 'preserves existing project authorizations for direct project members' do
+              expect { transfer_service.execute(new_parent_group) }.not_to change {
+                ProjectAuthorization.where(project_id: nested_project.id, user_id: direct_project_member.id).count
+              }
+            end
           end
 
-          context 'for groups with many members' do
-            before do
-              11.times do
-                new_parent_group.add_maintainer(create(:user))
-              end
+          context 'for nested groups with unique members' do
+            it 'preserves existing project authorizations' do
+              expect { transfer_service.execute(new_parent_group) }.not_to change {
+                ProjectAuthorization.where(project_id: nested_project.id, user_id: unique_subgroup_member.id).count
+              }
             end
+          end
+
+          context 'for groups with many projects' do
+            let_it_be(:project_list) { create_list(:project, 11, :repository, :private, namespace: group) }
 
             it 'adds new project authorizations for the user which makes a transfer' do
               transfer_service.execute(new_parent_group)
@@ -646,9 +661,21 @@ RSpec.describe Groups::TransferService, :sidekiq_inline do
               expect(ProjectAuthorization.where(project_id: nested_project.id, user_id: user.id).size).to eq(1)
             end
 
+            it 'adds project authorizations for users in the new hierarchy' do
+              expect { transfer_service.execute(new_parent_group) }.to change {
+                ProjectAuthorization.where(project_id: project_list.map { |project| project.id }, user_id: new_group_member.id).size
+              }.from(0).to(project_list.count)
+            end
+
+            it 'removes project authorizations for users in the old hierarchy' do
+              expect { transfer_service.execute(new_parent_group) }.to change {
+                ProjectAuthorization.where(project_id: project_list.map { |project| project.id }, user_id: old_group_member.id).size
+              }.from(project_list.count).to(0)
+            end
+
             it 'schedules authorizations job' do
-              expect(AuthorizedProjectsWorker).to receive(:bulk_perform_async)
-                .with(array_including(new_parent_group.members_with_parents.pluck(:user_id).map {|id| [id, anything] }))
+              expect(AuthorizedProjectUpdate::ProjectRecalculateWorker).to receive(:bulk_perform_async)
+                .with(array_including(group.all_projects.ids.map { |id| [id, anything] }))
 
               transfer_service.execute(new_parent_group)
             end