Merge branch 'if-share_groups_with_groups_expiry' into 'master'

Remove expired GroupGroupLinks

See merge request gitlab-org/gitlab!19215

app/services/groups/group_links/destroy_service.rb

+# frozen_string_literal: true
+
+module Groups
+  module GroupLinks
+    class DestroyService < BaseService
+      def execute(one_or_more_links)
+        links = Array(one_or_more_links)
+
+        GroupGroupLink.transaction do
+          GroupGroupLink.delete(links)
+
+          groups_to_refresh = links.map(&:shared_with_group)
+          groups_to_refresh.uniq.each do |group|
+            group.refresh_members_authorized_projects
+          end
+
+          Gitlab::AppLogger.info("GroupGroupLinks with ids: #{links.map(&:id)} have been deleted.")
+        rescue => ex
+          Gitlab::AppLogger.error(ex)
+
+          raise
+        end
+      end
+    end
+  end
+end

app/workers/remove_expired_group_links_worker.rb

@@ -8,5 +8,9 @@ class RemoveExpiredGroupLinksWorker
 
   def perform
     ProjectGroupLink.expired.destroy_all # rubocop: disable DestroyAll
+
+    GroupGroupLink.expired.find_in_batches do |link_batch|
+      Groups::GroupLinks::DestroyService.new(nil, nil).execute(link_batch)
+    end
   end
 end

spec/services/groups/group_links/destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Groups::GroupLinks::DestroyService, '#execute' do
+  let(:user) { create(:user) }
+
+  let_it_be(:group) { create(:group, :private) }
+  let_it_be(:shared_group) { create(:group, :private) }
+  let_it_be(:project) { create(:project, group: shared_group) }
+
+  subject { described_class.new(nil, nil) }
+
+  context 'single link' do
+    let!(:link) { create(:group_group_link, shared_group: shared_group, shared_with_group: group) }
+
+    it 'destroys link' do
+      expect { subject.execute(link) }.to change { GroupGroupLink.count }.from(1).to(0)
+    end
+
+    it 'revokes project authorization' do
+      group.add_developer(user)
+
+      expect { subject.execute(link) }.to(
+        change { Ability.allowed?(user, :read_project, project) }.from(true).to(false))
+    end
+  end
+
+  context 'multiple links' do
+    let_it_be(:another_group) { create(:group, :private) }
+    let_it_be(:another_shared_group) { create(:group, :private) }
+
+    let!(:links) do
+      [
+        create(:group_group_link, shared_group: shared_group, shared_with_group: group),
+        create(:group_group_link, shared_group: shared_group, shared_with_group: another_group),
+        create(:group_group_link, shared_group: another_shared_group, shared_with_group: group),
+        create(:group_group_link, shared_group: another_shared_group, shared_with_group: another_group)
+      ]
+    end
+
+    it 'updates project authorization once per group' do
+      expect(GroupGroupLink).to receive(:delete)
+      expect(group).to receive(:refresh_members_authorized_projects).once
+      expect(another_group).to receive(:refresh_members_authorized_projects).once
+
+      subject.execute(links)
+    end
+
+    it 'rolls back changes when error happens' do
+      group.add_developer(user)
+
+      expect(group).to receive(:refresh_members_authorized_projects).once.and_call_original
+      expect(another_group).to(
+        receive(:refresh_members_authorized_projects).and_raise('boom'))
+
+      expect { subject.execute(links) }.to raise_error('boom')
+
+      expect(GroupGroupLink.count).to eq(links.length)
+      expect(Ability.allowed?(user, :read_project, project)).to be_truthy
+    end
+  end
+end

spec/workers/remove_expired_group_links_worker_spec.rb

@@ -4,23 +4,54 @@ require 'spec_helper'
 
 describe RemoveExpiredGroupLinksWorker do
   describe '#perform' do
-    let!(:expired_project_group_link) { create(:project_group_link, expires_at: 1.hour.ago) }
-    let!(:project_group_link_expiring_in_future) { create(:project_group_link, expires_at: 10.days.from_now) }
-    let!(:non_expiring_project_group_link) { create(:project_group_link, expires_at: nil) }
+    context 'ProjectGroupLinks' do
+      let!(:expired_project_group_link) { create(:project_group_link, expires_at: 1.hour.ago) }
+      let!(:project_group_link_expiring_in_future) { create(:project_group_link, expires_at: 10.days.from_now) }
+      let!(:non_expiring_project_group_link) { create(:project_group_link, expires_at: nil) }
 
-    it 'removes expired group links' do
-      expect { subject.perform }.to change { ProjectGroupLink.count }.by(-1)
-      expect(ProjectGroupLink.find_by(id: expired_project_group_link.id)).to be_nil
-    end
+      it 'removes expired group links' do
+        expect { subject.perform }.to change { ProjectGroupLink.count }.by(-1)
+        expect(ProjectGroupLink.find_by(id: expired_project_group_link.id)).to be_nil
+      end
+
+      it 'leaves group links that expire in the future' do
+        subject.perform
+        expect(project_group_link_expiring_in_future.reload).to be_present
+      end
 
-    it 'leaves group links that expire in the future' do
-      subject.perform
-      expect(project_group_link_expiring_in_future.reload).to be_present
+      it 'leaves group links that do not expire at all' do
+        subject.perform
+        expect(non_expiring_project_group_link.reload).to be_present
+      end
     end
 
-    it 'leaves group links that do not expire at all' do
-      subject.perform
-      expect(non_expiring_project_group_link.reload).to be_present
+    context 'GroupGroupLinks' do
+      let(:mock_destroy_service) { instance_double(Groups::GroupLinks::DestroyService) }
+
+      before do
+        allow(Groups::GroupLinks::DestroyService).to(
+          receive(:new).and_return(mock_destroy_service))
+      end
+
+      context 'expired GroupGroupLink exists' do
+        before do
+          create(:group_group_link, expires_at: 1.hour.ago)
+        end
+
+        it 'calls Groups::GroupLinks::DestroyService' do
+          expect(mock_destroy_service).to receive(:execute).once
+
+          subject.perform
+        end
+      end
+
+      context 'expired GroupGroupLink does not exist' do
+        it 'does not call Groups::GroupLinks::DestroyService' do
+          expect(mock_destroy_service).not_to receive(:execute)
+
+          subject.perform
+        end
+      end
     end
   end
 end