Merge branch...

Merge branch '299034-group-group-links-project-authorizations-refresh-can-be-limited-to-only-direct-members-of' into 'master'

Resolve "Group-Group sharing: Project Authorizations refresh can be limited to only direct members of `shared_with_group`" [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!51869

app/models/group.rb

@@ -365,13 +365,28 @@ class Group < Namespace
   # rubocop: enable CodeReuse/ServiceClass
 
   # rubocop: disable CodeReuse/ServiceClass
-  def refresh_members_authorized_projects(blocking: true, priority: UserProjectAccessChangedService::HIGH_PRIORITY)
+  def refresh_members_authorized_projects(
+    blocking: true,
+    priority: UserProjectAccessChangedService::HIGH_PRIORITY,
+    direct_members_only: false
+  )
+
+    user_ids = if direct_members_only
+                 users_ids_of_direct_members
+               else
+                 user_ids_for_project_authorizations
+               end
+
     UserProjectAccessChangedService
-      .new(user_ids_for_project_authorizations)
+      .new(user_ids)
       .execute(blocking: blocking, priority: priority)
   end
   # rubocop: enable CodeReuse/ServiceClass
 
+  def users_ids_of_direct_members
+    direct_members.pluck(:user_id)
+  end
+
   def user_ids_for_project_authorizations
     members_with_parents.pluck(:user_id)
   end
@@ -382,6 +397,12 @@ class Group < Namespace
     end
   end
 
+  def direct_members
+    GroupMember.active_without_invites_and_requests
+               .non_minimal_access
+               .where(source_id: id)
+  end
+
   def members_with_parents
     # Avoids an unnecessary SELECT when the group has no parents
     source_ids =

app/services/groups/destroy_service.rb

@@ -31,11 +31,11 @@ module Groups
 
       # If any other groups are shared with the group that is being destroyed,
       # we should specifically trigger update of all project authorizations
-      # for users that are the members of this group.
+      # for users that are the direct members of this group.
       # If not, the project authorization records of these users to projects within the shared groups
       # will never be removed, causing inconsistencies with access permissions.
       if any_other_groups_are_shared_with_this_group?
-        user_ids_for_project_authorizations_refresh = group.user_ids_for_project_authorizations
+        user_ids_for_project_authorizations_refresh = group.users_ids_of_direct_members
       end
 
       group.destroy

app/services/groups/group_links/create_service.rb

@@ -18,7 +18,7 @@ module Groups
         )
 
         if link.save
-          group.refresh_members_authorized_projects
+          group.refresh_members_authorized_projects(direct_members_only: true)
           success(link: link)
         else
           error(link.errors.full_messages.to_sentence, 409)

app/services/groups/group_links/destroy_service.rb

@@ -16,7 +16,7 @@ module Groups
 
           groups_to_refresh = links.map(&:shared_with_group)
           groups_to_refresh.uniq.each do |group|
-            group.refresh_members_authorized_projects
+            group.refresh_members_authorized_projects(direct_members_only: true)
           end
         else
           Gitlab::AppLogger.info(

app/services/groups/group_links/update_service.rb

@@ -13,7 +13,7 @@ module Groups
         group_link.update!(group_link_params)
 
         if requires_authorization_refresh?(group_link_params)
-          group_link.shared_with_group.refresh_members_authorized_projects
+          group_link.shared_with_group.refresh_members_authorized_projects(direct_members_only: true)
         end
       end
 

changelogs/unreleased/299034-group-group-links-project-authorizations-refresh-can-be-limited-to.yml

+---
+title: Limit Project Authorizations refresh for shared groups only to direct members
+  of the group being shared with
+merge_request: 51869
+author:
+type: performance

spec/models/group_spec.rb

@@ -740,6 +740,33 @@ RSpec.describe Group do
     end
   end
 
+  describe '#direct_members' do
+    let_it_be(:group) { create(:group, :nested) }
+    let_it_be(:maintainer) { group.parent.add_user(create(:user), GroupMember::MAINTAINER) }
+    let_it_be(:developer) { group.add_user(create(:user), GroupMember::DEVELOPER) }
+
+    it 'does not return members of the parent' do
+      expect(group.direct_members).not_to include(maintainer)
+    end
+
+    it 'returns the direct member of the group' do
+      expect(group.direct_members).to include(developer)
+    end
+
+    context 'group sharing' do
+      let!(:shared_group) { create(:group) }
+
+      before do
+        create(:group_group_link, shared_group: shared_group, shared_with_group: group)
+      end
+
+      it 'does not return members of the shared_with group' do
+        expect(shared_group.direct_members).not_to(
+          include(developer))
+      end
+    end
+  end
+
   describe '#members_with_parents' do
     let!(:group) { create(:group, :nested) }
     let!(:maintainer) { group.parent.add_user(create(:user), GroupMember::MAINTAINER) }
@@ -932,6 +959,65 @@ RSpec.describe Group do
     end
   end
 
+  describe '#refresh_members_authorized_projects' do
+    let_it_be(:group) { create(:group, :nested) }
+    let_it_be(:parent_group_user) { create(:user) }
+    let_it_be(:group_user) { create(:user) }
+
+    before do
+      group.parent.add_maintainer(parent_group_user)
+      group.add_developer(group_user)
+    end
+
+    context 'users for which authorizations refresh is executed' do
+      it 'processes authorizations refresh for all members of the group' do
+        expect(UserProjectAccessChangedService).to receive(:new).with(contain_exactly(group_user.id, parent_group_user.id)).and_call_original
+
+        group.refresh_members_authorized_projects
+      end
+
+      context 'when explicitly specified to run only for direct members' do
+        it 'processes authorizations refresh only for direct members of the group' do
+          expect(UserProjectAccessChangedService).to receive(:new).with(contain_exactly(group_user.id)).and_call_original
+
+          group.refresh_members_authorized_projects(direct_members_only: true)
+        end
+      end
+    end
+  end
+
+  describe '#users_ids_of_direct_members' do
+    let_it_be(:group) { create(:group, :nested) }
+    let_it_be(:parent_group_user) { create(:user) }
+    let_it_be(:group_user) { create(:user) }
+
+    before do
+      group.parent.add_maintainer(parent_group_user)
+      group.add_developer(group_user)
+    end
+
+    it 'does not return user ids of the members of the parent' do
+      expect(group.users_ids_of_direct_members).not_to include(parent_group_user.id)
+    end
+
+    it 'returns the user ids of the direct member of the group' do
+      expect(group.users_ids_of_direct_members).to include(group_user.id)
+    end
+
+    context 'group sharing' do
+      let!(:shared_group) { create(:group) }
+
+      before do
+        create(:group_group_link, shared_group: shared_group, shared_with_group: group)
+      end
+
+      it 'does not return the user ids of members of the shared_with group' do
+        expect(shared_group.users_ids_of_direct_members).not_to(
+          include(group_user.id))
+      end
+    end
+  end
+
   describe '#user_ids_for_project_authorizations' do
     it 'returns the user IDs for which to refresh authorizations' do
       maintainer = create(:user)

spec/services/groups/destroy_service_spec.rb

@@ -229,10 +229,10 @@ RSpec.describe Groups::DestroyService do
           # will still be executed for the nested group as they fall under the same hierarchy
           # and hence we need to account for this scenario.
           expect(UserProjectAccessChangedService)
-            .to receive(:new).with(shared_with_group.user_ids_for_project_authorizations).and_call_original
+            .to receive(:new).with(shared_with_group.users_ids_of_direct_members).and_call_original
 
           expect(UserProjectAccessChangedService)
-            .not_to receive(:new).with(shared_group.user_ids_for_project_authorizations)
+            .not_to receive(:new).with(shared_group.users_ids_of_direct_members)
 
           destroy_group(shared_group, user, false)
         end
@@ -246,7 +246,7 @@ RSpec.describe Groups::DestroyService do
 
         it 'makes use of a specific service to update project authorizations' do
           expect(UserProjectAccessChangedService)
-            .to receive(:new).with(shared_with_group.user_ids_for_project_authorizations).and_call_original
+            .to receive(:new).with(shared_with_group.users_ids_of_direct_members).and_call_original
 
           destroy_group(shared_with_group, user, false)
         end

spec/services/groups/group_links/create_service_spec.rb

@@ -74,46 +74,56 @@ RSpec.describe Groups::GroupLinks::CreateService, '#execute' do
     end
   end
 
-  context 'group hierarchies' do
+  context 'project authorizations based on group hierarchies' do
     before do
       group_parent.add_owner(parent_group_user)
       group.add_owner(group_user)
       group_child.add_owner(child_group_user)
     end
 
-    context 'group user' do
-      let(:user) { group_user }
+    context 'project authorizations refresh' do
+      it 'is executed only for the direct members of the group' do
+        expect(UserProjectAccessChangedService).to receive(:new).with(contain_exactly(group_user.id)).and_call_original
 
-      it 'create proper authorizations' do
         subject.execute(shared_group)
-
-        expect(Ability.allowed?(user, :read_project, project_parent)).to be_falsey
-        expect(Ability.allowed?(user, :read_project, project)).to be_truthy
-        expect(Ability.allowed?(user, :read_project, project_child)).to be_truthy
       end
     end
 
-    context 'parent group user' do
-      let(:user) { parent_group_user }
+    context 'project authorizations' do
+      context 'group user' do
+        let(:user) { group_user }
 
-      it 'create proper authorizations' do
-        subject.execute(shared_group)
+        it 'create proper authorizations' do
+          subject.execute(shared_group)
 
-        expect(Ability.allowed?(user, :read_project, project_parent)).to be_falsey
-        expect(Ability.allowed?(user, :read_project, project)).to be_falsey
-        expect(Ability.allowed?(user, :read_project, project_child)).to be_falsey
+          expect(Ability.allowed?(user, :read_project, project_parent)).to be_falsey
+          expect(Ability.allowed?(user, :read_project, project)).to be_truthy
+          expect(Ability.allowed?(user, :read_project, project_child)).to be_truthy
+        end
       end
-    end
 
-    context 'child group user' do
-      let(:user) { child_group_user }
+      context 'parent group user' do
+        let(:user) { parent_group_user }
 
-      it 'create proper authorizations' do
-        subject.execute(shared_group)
+        it 'create proper authorizations' do
+          subject.execute(shared_group)
+
+          expect(Ability.allowed?(user, :read_project, project_parent)).to be_falsey
+          expect(Ability.allowed?(user, :read_project, project)).to be_falsey
+          expect(Ability.allowed?(user, :read_project, project_child)).to be_falsey
+        end
+      end
+
+      context 'child group user' do
+        let(:user) { child_group_user }
+
+        it 'create proper authorizations' do
+          subject.execute(shared_group)
 
-        expect(Ability.allowed?(user, :read_project, project_parent)).to be_falsey
-        expect(Ability.allowed?(user, :read_project, project)).to be_falsey
-        expect(Ability.allowed?(user, :read_project, project_child)).to be_falsey
+          expect(Ability.allowed?(user, :read_project, project_parent)).to be_falsey
+          expect(Ability.allowed?(user, :read_project, project)).to be_falsey
+          expect(Ability.allowed?(user, :read_project, project_child)).to be_falsey
+        end
       end
     end
   end

spec/services/groups/group_links/destroy_service_spec.rb

@@ -47,8 +47,8 @@ RSpec.describe Groups::GroupLinks::DestroyService, '#execute' do
 
     it 'updates project authorization once per group' do
       expect(GroupGroupLink).to receive(:delete).and_call_original
-      expect(group).to receive(:refresh_members_authorized_projects).once
-      expect(another_group).to receive(:refresh_members_authorized_projects).once
+      expect(group).to receive(:refresh_members_authorized_projects).with(direct_members_only: true).once
+      expect(another_group).to receive(:refresh_members_authorized_projects).with(direct_members_only: true).once
 
       subject.execute(links)
     end

spec/services/groups/group_links/update_service_spec.rb

@@ -8,7 +8,7 @@ RSpec.describe Groups::GroupLinks::UpdateService, '#execute' do
   let_it_be(:group) { create(:group, :private) }
   let_it_be(:shared_group) { create(:group, :private) }
   let_it_be(:project) { create(:project, group: shared_group) }
-  let(:group_member) { create(:user) }
+  let(:group_member_user) { create(:user) }
   let!(:link) { create(:group_group_link, shared_group: shared_group, shared_with_group: group) }
 
   let(:expiry_date) { 1.month.from_now.to_date }
@@ -20,7 +20,7 @@ RSpec.describe Groups::GroupLinks::UpdateService, '#execute' do
   subject { described_class.new(link).execute(group_link_params) }
 
   before do
-    group.add_developer(group_member)
+    group.add_developer(group_member_user)
   end
 
   it 'updates existing link' do
@@ -36,11 +36,11 @@ RSpec.describe Groups::GroupLinks::UpdateService, '#execute' do
   end
 
   it 'updates project permissions' do
-    expect { subject }.to change { group_member.can?(:create_release, project) }.from(true).to(false)
+    expect { subject }.to change { group_member_user.can?(:create_release, project) }.from(true).to(false)
   end
 
   it 'executes UserProjectAccessChangedService' do
-    expect_next_instance_of(UserProjectAccessChangedService) do |service|
+    expect_next_instance_of(UserProjectAccessChangedService, [group_member_user.id]) do |service|
       expect(service).to receive(:execute)
     end