Merge branch 'security-202687-members-transfer-problem' into 'master'

Add refreshing projects to transfering groups

Closes #182

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

app/services/groups/transfer_service.rb

@@ -37,6 +37,7 @@ module Groups
 
     # Overridden in EE
     def post_update_hooks(updated_project_ids)
+      refresh_project_authorizations
     end
 
     def ensure_allowed_transfer
@@ -136,6 +137,16 @@ module Groups
       @group.add_owner(current_user)
     end
 
+    def refresh_project_authorizations
+      ProjectAuthorization.where(project_id: @group.all_projects.select(:id)).delete_all # rubocop: disable CodeReuse/ActiveRecord
+
+      # 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
+    end
+
     def raise_transfer_error(message)
       raise TransferError, localized_error_messages[message]
     end

app/workers/authorized_projects_worker.rb

@@ -16,6 +16,9 @@ class AuthorizedProjectsWorker
   if Rails.env.test?
     def self.bulk_perform_and_wait(args_list, timeout: 10)
     end
+
+    def self.bulk_perform_inline(args_list)
+    end
   end
 
   # rubocop: disable CodeReuse/ActiveRecord

changelogs/unreleased/security-202687-members-transfer-problem.yml

+---
+title: Refresh project authorizations when transferring groups
+merge_request:
+author:
+type: security

ee/app/services/ee/groups/transfer_service.rb

@@ -18,6 +18,7 @@ module EE
         ::Project.id_in(updated_project_ids).find_each do |project|
           project.maintain_elasticsearch_update if project.maintaining_elasticsearch?
         end
+        super
       end
 
       def lost_groups

spec/services/groups/transfer_service_spec.rb

@@ -414,15 +414,20 @@ RSpec.describe Groups::TransferService do
       end
 
       context 'when transferring a group with nested groups and projects' do
-        let!(:group) { create(:group, :public) }
+        let(:subgroup1) { create(:group, :private, parent: group) }
         let!(:project1) { create(:project, :repository, :private, namespace: group) }
-        let!(:subgroup1) { create(:group, :private, parent: group) }
         let!(:nested_subgroup) { create(:group, :private, parent: subgroup1) }
         let!(:nested_project) { create(:project, :repository, :private, namespace: subgroup1) }
 
         before do
           TestEnv.clean_test_path
           create(:group_member, :owner, group: new_parent_group, user: user)
+        end
+
+        context 'updated paths' do
+          let(:group) { create(:group, :public) }
+
+          before do
             transfer_service.execute(new_parent_group)
           end
 
@@ -455,6 +460,74 @@ RSpec.describe Groups::TransferService do
           end
         end
 
+        context 'resets project authorizations' do
+          let(:old_parent_group) { create(:group) }
+          let(:group) { create(:group, :private, parent: old_parent_group) }
+          let(:new_group_member) { create(:user) }
+          let(:old_group_member) { create(:user) }
+
+          before do
+            new_parent_group.add_maintainer(new_group_member)
+            old_parent_group.add_maintainer(old_group_member)
+            group.refresh_members_authorized_projects
+          end
+
+          it 'removes old project authorizations' do
+            expect { transfer_service.execute(new_parent_group) }.to change {
+              ProjectAuthorization.where(project_id: project1.id, user_id: old_group_member.id).size
+            }.from(1).to(0)
+          end
+
+          it 'adds new project authorizations' do
+            expect { transfer_service.execute(new_parent_group) }.to change {
+              ProjectAuthorization.where(project_id: project1.id, user_id: new_group_member.id).size
+            }.from(0).to(1)
+          end
+
+          it 'performs authorizations job immediately' do
+            expect(AuthorizedProjectsWorker).to receive(:bulk_perform_inline)
+
+            transfer_service.execute(new_parent_group)
+          end
+
+          context 'for nested projects' do
+            it 'removes old project authorizations' do
+              expect { transfer_service.execute(new_parent_group) }.to change {
+                ProjectAuthorization.where(project_id: nested_project.id, user_id: old_group_member.id).size
+              }.from(1).to(0)
+            end
+
+            it 'adds new project authorizations' do
+              expect { transfer_service.execute(new_parent_group) }.to change {
+                ProjectAuthorization.where(project_id: nested_project.id, user_id: new_group_member.id).size
+              }.from(0).to(1)
+            end
+          end
+
+          context 'for groups with many members' do
+            before do
+              11.times do
+                new_parent_group.add_maintainer(create(:user))
+              end
+            end
+
+            it 'adds new project authorizations for the user which makes a transfer' do
+              transfer_service.execute(new_parent_group)
+
+              expect(ProjectAuthorization.where(project_id: project1.id, user_id: user.id).size).to eq(1)
+              expect(ProjectAuthorization.where(project_id: nested_project.id, user_id: user.id).size).to eq(1)
+            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] }))
+
+              transfer_service.execute(new_parent_group)
+            end
+          end
+        end
+      end
+
       context 'when updating the group goes wrong' do
         let!(:subgroup1) { create(:group, :public, parent: group) }
         let!(:subgroup2) { create(:group, :public, parent: group) }