From bed323992625d7d91f181d307af2422c034f7d55 Mon Sep 17 00:00:00 2001 From: Joe Woodward Date: Fri, 22 Nov 2024 11:19:49 +0000 Subject: [PATCH] Include groups invited to ancestral groups as Code Owners This change allows groups that have been shared into the project's group or any ancestor of that group to be specified as a Code Owner in the project's CODEOWNERS file. Only direct members of the group are eligible. Co-Authored-By: Emma Park --- app/helpers/groups/group_members_helper.rb | 4 +- app/models/group.rb | 37 +++++++++---------- app/models/project.rb | 2 +- ee/lib/gitlab/code_owners/groups_loader.rb | 9 ++++- .../gitlab/code_owners/groups_loader_spec.rb | 22 +++++++++++ spec/models/group_spec.rb | 8 ++-- spec/models/project_spec.rb | 2 +- 7 files changed, 55 insertions(+), 29 deletions(-) diff --git a/app/helpers/groups/group_members_helper.rb b/app/helpers/groups/group_members_helper.rb index 1bba9d789f5cfd..ecf7886cf9707d 100644 --- a/app/helpers/groups/group_members_helper.rb +++ b/app/helpers/groups/group_members_helper.rb @@ -67,9 +67,9 @@ def group_group_links(group, include_relations) when [:direct] group.shared_with_group_links when [:inherited] - group.shared_with_group_links.of_ancestors + group.shared_with_group_links_of_ancestors else - group.shared_with_group_links.of_ancestors_and_self + group.shared_with_group_links_of_ancestors_and_self end group_links.distinct_on_shared_with_group_id_with_group_access diff --git a/app/models/group.rb b/app/models/group.rb index b17c85dbe5cf2a..822fd8482303c0 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -63,31 +63,28 @@ def self.supported_keyset_orderings has_many :milestones has_many :integrations - has_many :shared_group_links, foreign_key: :shared_with_group_id, class_name: 'GroupGroupLink' - has_many :shared_with_group_links, foreign_key: :shared_group_id, class_name: 'GroupGroupLink' do - def of_ancestors - group = proxy_association.owner - return GroupGroupLink.none unless group.has_parent? + with_options class_name: 'GroupGroupLink' do + has_many :shared_group_links, foreign_key: :shared_with_group_id - GroupGroupLink.where(shared_group_id: group.ancestors.reorder(nil).select(:id)) - end - - def of_ancestors_and_self - group = proxy_association.owner - - source_ids = - if group.has_parent? - group.self_and_ancestors.reorder(nil).select(:id) - else - group.id - end - - GroupGroupLink.where(shared_group_id: source_ids) + with_options foreign_key: :shared_group_id do + has_many :shared_with_group_links + has_many :shared_with_group_links_of_ancestors, ->(group) do + unscope(where: :shared_group_id).where(shared_group: group.ancestors) + end + has_many :shared_with_group_links_of_ancestors_and_self, ->(group) do + unscope(where: :shared_group_id).where(shared_group: group.self_and_ancestors) + end end end + has_many :shared_groups, through: :shared_group_links, source: :shared_group - has_many :shared_with_groups, through: :shared_with_group_links, source: :shared_with_group + with_options source: :shared_with_group do + has_many :shared_with_groups, through: :shared_with_group_links + has_many :shared_with_groups_of_ancestors, through: :shared_with_group_links_of_ancestors + has_many :shared_with_groups_of_ancestors_and_self, through: :shared_with_group_links_of_ancestors_and_self + end + has_many :project_group_links, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :shared_projects, through: :project_group_links, source: :project diff --git a/app/models/project.rb b/app/models/project.rb index 4910b37616f526..bb0beae3df1a70 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -3315,7 +3315,7 @@ def refreshing_build_artifacts_size? end def group_group_links - group&.shared_with_group_links&.of_ancestors_and_self || GroupGroupLink.none + group&.shared_with_group_links_of_ancestors_and_self || GroupGroupLink.none end def security_training_available? diff --git a/ee/lib/gitlab/code_owners/groups_loader.rb b/ee/lib/gitlab/code_owners/groups_loader.rb index 7b6379f43a8969..66acfaa5ad2d72 100644 --- a/ee/lib/gitlab/code_owners/groups_loader.rb +++ b/ee/lib/gitlab/code_owners/groups_loader.rb @@ -25,9 +25,16 @@ def load_groups relations = [ project.invited_groups.where_full_path_in(extractor.names, preload_routes: false) ] - # Include the projects ancestor group(s) if they are listed as owners + if project.group + # Include the projects ancestor group(s) if they are listed as owners relations << project.group.self_and_ancestors.where_full_path_in(extractor.names, preload_routes: false) + + # Retrieve groups that have access to ancestors, filtering by relevant paths + shared_groups_via_ancestors = project.group.shared_with_groups_of_ancestors_and_self.where_full_path_in( + extractor.names, preload_routes: false) + + relations << shared_groups_via_ancestors end Group.from_union(relations).with_route.with_users diff --git a/ee/spec/lib/gitlab/code_owners/groups_loader_spec.rb b/ee/spec/lib/gitlab/code_owners/groups_loader_spec.rb index fc919709b21860..1bca9321dcd65d 100644 --- a/ee/spec/lib/gitlab/code_owners/groups_loader_spec.rb +++ b/ee/spec/lib/gitlab/code_owners/groups_loader_spec.rb @@ -102,5 +102,27 @@ end end end + + context "groups invited to the project's parent group" do + let(:text) { "@group_1\n@group_1/subgroup_2\n@group_1/subgroup_3" } + + let(:group_1) { create(:group, path: 'group_1') } + let(:group_2) { create(:group, path: 'subgroup_2', parent: group_1) } + let(:group_3) { create(:group, path: 'subgroup_3', parent: group_1) } + let(:project) { create(:project, :repository, namespace: group_3) } + + # group 2 is invited to group 3 + before do + create(:group_group_link, shared_group: group_3, shared_with_group: group_2) + end + + it 'returns invited and direct groups matching the input' do + load_groups + + expect(entry).to have_received(:add_matching_groups_from) do |args| + expect(args).to contain_exactly(group_1, group_2, group_3) + end + end + end end end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index d1aa8f41e99b99..116803db9c0ab3 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -3946,7 +3946,7 @@ def define_cache_expectations(cache_key) sub_sub_group.shared_with_groups << shared_group_3 end - describe '#shared_with_group_links.of_ancestors' do + describe '#shared_with_groups_of_ancestors' do where(:subject_group, :result) do ref(:group) | [] ref(:sub_group) | lazy { [shared_group_1].map(&:id) } @@ -3955,12 +3955,12 @@ def define_cache_expectations(cache_key) with_them do it 'returns correct group shares' do - expect(subject_group.shared_with_group_links.of_ancestors.map(&:shared_with_group_id)).to match_array(result) + expect(subject_group.shared_with_groups_of_ancestors.ids).to match_array(result) end end end - describe '#shared_with_group_links.of_ancestors_and_self' do + describe '#shared_with_groups_of_ancestors_and_self' do where(:subject_group, :result) do ref(:group) | lazy { [shared_group_1].map(&:id) } ref(:sub_group) | lazy { [shared_group_1, shared_group_2].map(&:id) } @@ -3969,7 +3969,7 @@ def define_cache_expectations(cache_key) with_them do it 'returns correct group shares' do - expect(subject_group.shared_with_group_links.of_ancestors_and_self.map(&:shared_with_group_id)).to match_array(result) + expect(subject_group.shared_with_groups_of_ancestors_and_self.ids).to match_array(result) end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index c72de28f9f6220..bab59055c78bfb 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -9142,7 +9142,7 @@ def has_external_wiki let_it_be(:project) { create(:project, group: group) } it 'returns group links of group' do - expect(group).to receive_message_chain(:shared_with_group_links, :of_ancestors_and_self) + expect(group).to receive(:shared_with_group_links_of_ancestors_and_self) project.group_group_links end -- GitLab