Commit c099a291 authored by Kerri Miller's avatar Kerri Miller Committed by Bob Van Landuyt

Add project group and ancestors to group list

This addresses an issue that arises when the owner of a path is the
group under which the project lives (such as `@gitlab-org` for the
Gitlab project..) Gitlab::CodeOwners::GroupsLoader did not add that
group to the list of groups when attempting to determine ownership. This
list of groups is expanded into a list of users that the user
attempting the action (typically a merge) is compared to. Sinc the
project's group is not added to the group list, its members are not
expanded and are thus missing from the list of owners for a given path.

Additionally, this will look not simply at the immediate group of the
project, but at the entire group tree ancestors to the root group. For
example, if the "merge_service" project is at

gitlab-org/engineering/source_code/merge_service

..then any of these groups would be considerable available for listing
as a code owner without having to be invited to the project:

@gitlab-org
@gitlab-org/engineering
@gitlab-org/engineering/source_code

This address both the proximate cause of
https://gitlab.com/gitlab-org/gitlab/-/issues/216345 but of the
long-requested feature https://gitlab.com/gitlab-org/gitlab/-/issues/32432
parent ad9e6899
---
title: Consider project group and group ancestors when processing CODEOWNERS entries
merge_request: 31152
author:
type: added
......@@ -23,7 +23,29 @@ module Gitlab
return Group.none if extractor.names.empty?
groups = project.invited_groups.where_full_path_in(extractor.names)
groups.with_route.with_users
if Feature.enabled?(:codeowners_match_ancestor_groups, default_enabled: true)
group_list = groups.with_route.with_users.to_a
if project.group
# If the project.group's ancestor group(s) are listed as owners, add
# them to group_list
#
if applicable_ancestors(extractor.names).any?
group_list.concat(applicable_ancestors(extractor.names))
end
end
group_list.uniq
else
groups.with_route.with_users
end
end
def applicable_ancestors(extractor_names)
ancestor_groups = project.group.self_and_ancestors.with_route.with_users
ancestor_groups.select { |group| extractor_names.include?(group.full_path) }
end
end
end
......
......@@ -103,6 +103,71 @@ describe Gitlab::Checks::DiffCheck do
expect(validation_result).to be_nil
end
end
context "when the codeowner entity is a group" do
let(:group) { create(:group) }
let(:project) { create(:project, :repository, namespace: group) }
let(:codeowner_content) do
<<~CODEOWNERS
*.rb @#{code_owner.username}
docs/CODEOWNERS @#{group.full_path}
*.js.coffee @#{group.full_path}
CODEOWNERS
end
before do
stub_feature_flags(sectional_codeowners: false)
end
shared_examples_for "returns nil" do
it "returns nil" do
expect(validation_result).to be_nil
end
end
shared_examples_for "returns codeowners validation message" do
it "returns an error message" do
expect(validation_result).to include("Pushes to protected branches")
end
end
context "and the project is under a subgroup" do
let(:subgroup) { create(:group, parent: group) }
let(:project) { create(:project, :repository, namespace: subgroup) }
context "and the user is part of the codeowning-group" do
before do
group.add_developer(user)
end
it_behaves_like "returns nil"
end
context "and the user is not part of the codeowning-group" do
it_behaves_like "returns codeowners validation message"
end
context "with :codeowners_match_ancestor_groups disabled" do
before do
stub_feature_flags(codeowners_match_ancestor_groups: false)
end
it_behaves_like "returns codeowners validation message"
end
end
context "and the user is part of the codeowning-group" do
before do
group.add_developer(user)
end
it_behaves_like "returns nil"
end
context "and the user is not part of the codeowning-group" do
it_behaves_like "returns codeowners validation message"
end
end
end
context "the MR doesn't contain a matching file path" do
......
......@@ -58,6 +58,18 @@ describe Gitlab::CodeOwners::GroupsLoader do
end
end
context "input matches project.group" do
let(:group) { create(:group) }
let(:project) { create(:project, :repository, namespace: group) }
let(:text) { "@#{project.group.full_path}" }
it "returns the project's group" do
load_groups
expect(entry).to have_received(:add_matching_groups_from).with([group])
end
end
context 'input as array of strings' do
let(:text) { super().lines }
......
......@@ -71,7 +71,7 @@ describe Gitlab::CodeOwners::Loader do
expect(loader.entries).to contain_exactly(expected_entry, other_entry)
end
it 'only performs 3 query for users and groups' do
it 'performs 6 query for users and groups' do
test_group = create(:group, path: 'test-group')
test_group.add_developer(create(:user))
......@@ -80,9 +80,12 @@ describe Gitlab::CodeOwners::Loader do
project.invited_groups << [test_group, another_group]
# One query for users, one for the emails to later divide them across the entries
# one for groups with joined routes and users
expect { loader.entries }.not_to exceed_query_limit(3)
# - 1 query for users
# - 1 for the emails to later divide them across the entries
# - 2 for groups with joined routes and users
# - 2 for loading the users for the parent group(s)
#
expect { loader.entries }.not_to exceed_query_limit(6)
end
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment