Commit 6dd84efd authored by Kerri Miller's avatar Kerri Miller

Add project.group to list when specified as owner

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.
parent 0d07b239
---
title: Add a Project's group to list of groups when parsing for codeowner entries
merge_request: 30934
author:
type: fixed
...@@ -23,7 +23,15 @@ module Gitlab ...@@ -23,7 +23,15 @@ module Gitlab
return Group.none if extractor.names.empty? return Group.none if extractor.names.empty?
groups = project.invited_groups.where_full_path_in(extractor.names) groups = project.invited_groups.where_full_path_in(extractor.names)
groups.with_route.with_users group_list = groups.with_route.with_users.to_a
if extractor.names.include?(project.group&.full_path)
project.group.users.load
group_list << project.group
end
group_list
end end
end end
end end
......
...@@ -39,6 +39,8 @@ describe Gitlab::Checks::DiffCheck do ...@@ -39,6 +39,8 @@ describe Gitlab::Checks::DiffCheck do
allow(project.repository).to receive(:code_owners_blob) allow(project.repository).to receive(:code_owners_blob)
.with(ref: codeowner_lookup_ref) .with(ref: codeowner_lookup_ref)
.and_return(codeowner_blob) .and_return(codeowner_blob)
stub_feature_flags(sectional_codeowners: false)
end end
context 'the MR contains a renamed file matching a file path' do context 'the MR contains a renamed file matching a file path' do
...@@ -56,10 +58,6 @@ describe Gitlab::Checks::DiffCheck do ...@@ -56,10 +58,6 @@ describe Gitlab::Checks::DiffCheck do
end end
context 'and the user is not listed as a codeowner' do context 'and the user is not listed as a codeowner' do
before do
stub_feature_flags(sectional_codeowners: false)
end
it "returns an error message" do it "returns an error message" do
expect { diff_check.validate! }.to raise_error do |error| expect { diff_check.validate! }.to raise_error do |error|
expect(error).to be_a(Gitlab::GitAccess::ForbiddenError) expect(error).to be_a(Gitlab::GitAccess::ForbiddenError)
...@@ -84,16 +82,16 @@ describe Gitlab::Checks::DiffCheck do ...@@ -84,16 +82,16 @@ describe Gitlab::Checks::DiffCheck do
subject.send(:validate_code_owners).call(["docs/CODEOWNERS", "README"]) subject.send(:validate_code_owners).call(["docs/CODEOWNERS", "README"])
end end
context "and the user is not listed as a code owner" do shared_examples_for "returns an error message" do
before do it "returns the expected error message" do
stub_feature_flags(sectional_codeowners: false)
end
it "returns an error message" do
expect(validation_result).to include("Pushes to protected branches") expect(validation_result).to include("Pushes to protected branches")
end end
end end
context "and the user is not listed as a code owner" do
it_behaves_like "returns an error message"
end
context "and the user is listed as a code owner" do context "and the user is listed as a code owner" do
# `user` is set as the owner of the incoming change by the shared # `user` is set as the owner of the incoming change by the shared
# context found in 'push rules checks context' # context found in 'push rules checks context'
...@@ -103,6 +101,32 @@ describe Gitlab::Checks::DiffCheck do ...@@ -103,6 +101,32 @@ describe Gitlab::Checks::DiffCheck do
expect(validation_result).to be_nil expect(validation_result).to be_nil
end end
end end
context "when the codeowner entity is a group" do
let(:group_a) { create(:group) }
let(:project) { create(:project, :repository, namespace: group_a) }
let(:codeowner_content) do
<<~CODEOWNERS
*.rb @#{code_owner.username}
docs/CODEOWNERS @#{group_a.name}
*.js.coffee @#{group_a.name}
CODEOWNERS
end
context "and the user is part of the codeowning-group" do
before do
group_a.add_developer(user)
end
it "returns nil" do
expect(validation_result).to be_nil
end
end
context "and the user is not part of the codeowning-group" do
it_behaves_like "returns an error message"
end
end
end end
context "the MR doesn't contain a matching file path" do context "the MR doesn't contain a matching file path" do
......
...@@ -51,7 +51,18 @@ describe Gitlab::CodeOwners::GroupsLoader do ...@@ -51,7 +51,18 @@ describe Gitlab::CodeOwners::GroupsLoader do
group = create(:group, path: "GROUP-1") group = create(:group, path: "GROUP-1")
create(:group, path: "GROUP-2") create(:group, path: "GROUP-2")
project.invited_groups << group project.invited_groups << group
load_groups
expect(entry).to have_received(:add_matching_groups_from).with([group])
end
end
context "input matches project.group" do
let(:group) { create(:group) }
let(:project) { create(:project, :repository, namespace: group) }
let(:text) { "@#{project.group.name}" }
it "returns the project's group" do
load_groups load_groups
expect(entry).to have_received(:add_matching_groups_from).with([group]) expect(entry).to have_received(:add_matching_groups_from).with([group])
......
...@@ -71,7 +71,7 @@ describe Gitlab::CodeOwners::Loader do ...@@ -71,7 +71,7 @@ describe Gitlab::CodeOwners::Loader do
expect(loader.entries).to contain_exactly(expected_entry, other_entry) expect(loader.entries).to contain_exactly(expected_entry, other_entry)
end end
it 'only performs 3 query for users and groups' do it 'only performs 4 queries for users and groups' do
test_group = create(:group, path: 'test-group') test_group = create(:group, path: 'test-group')
test_group.add_developer(create(:user)) test_group.add_developer(create(:user))
...@@ -80,9 +80,12 @@ describe Gitlab::CodeOwners::Loader do ...@@ -80,9 +80,12 @@ describe Gitlab::CodeOwners::Loader do
project.invited_groups << [test_group, another_group] project.invited_groups << [test_group, another_group]
# One query for users, one for the emails to later divide them across the entries # - 1 query for users
# one for groups with joined routes and users # - 1 for the emails to later divide them across the entries
expect { loader.entries }.not_to exceed_query_limit(3) # - 1 for groups with joined routes and users
# - 1 for loading the users for the parent group
#
expect { loader.entries }.not_to exceed_query_limit(4)
end end
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