From 25033eae13e0027c0ce6d5a5f25648e6ec4e19a9 Mon Sep 17 00:00:00 2001
From: Olaoluwa Oluro <olaoluro@gitlab.com>
Date: Sat, 22 Feb 2025 20:27:18 +0000
Subject: [PATCH 1/3] Skip entry owners check for exclusion pattern

**Problem**

When the exclusion pattern(!) is used and entry owners are not defined,
checks are still run on the exclusion owners.

**Solution**

When an exclusion pattern is defined, we should skip all checks on the
entry owners

Changelog: fixed
---
 ee/lib/gitlab/code_owners/file.rb           | 43 +++++++++++++--------
 ee/spec/lib/gitlab/code_owners/file_spec.rb | 17 ++++++++
 2 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/ee/lib/gitlab/code_owners/file.rb b/ee/lib/gitlab/code_owners/file.rb
index aae9565e5a305c..9699178254c7c5 100644
--- a/ee/lib/gitlab/code_owners/file.rb
+++ b/ee/lib/gitlab/code_owners/file.rb
@@ -65,7 +65,7 @@ def entries_for_path(path)
         matches = []
 
         parsed_data.each do |_, section_entries|
-          if Feature.enabled?(:codeowners_file_exclusions, project)
+          if codeowners_file_exclusions_enabled?
             matching_patterns = section_entries.keys.reverse.select { |pattern| path_matches?(pattern, path) }
             matching_entries = matching_patterns.map { |pattern| section_entries[pattern] }
 
@@ -93,6 +93,10 @@ def valid?
 
       private
 
+      def codeowners_file_exclusions_enabled?
+        Feature.enabled?(:codeowners_file_exclusions, project)
+      end
+
       def project
         @blob&.repository&.project
       end
@@ -150,32 +154,39 @@ def get_parsed_data
       end
 
       def parse_entry(line, parsed, section, line_number)
-        pattern, _separator, entry_owners = line.partition(/(?<!\\)\s+/)
+        pattern, normalized_pattern, entry_owners, is_exclusion_pattern = extract_line_pattern_and_entry_owners(line)
+        owners = is_exclusion_pattern ? nil : validate_and_get_owners(entry_owners, section, line_number)
 
-        if Feature.enabled?(:codeowners_file_exclusions, project)
-          is_exclusion = pattern.start_with?('!')
-          pattern = pattern[1..] if is_exclusion
-        end
+        parsed[section.name][normalized_pattern] = Entry.new(
+          pattern,
+          owners,
+          section: section.name,
+          optional: section.optional,
+          approvals_required: section.approvals,
+          exclusion: is_exclusion_pattern,
+          line_number: line_number
+        )
+      end
 
+      def extract_line_pattern_and_entry_owners(line)
+        pattern, _separator, entry_owners = line.partition(/(?<!\\)\s+/)
+
+        is_exclusion_pattern = codeowners_file_exclusions_enabled? && pattern.start_with?('!')
+        pattern = pattern[1..] if is_exclusion_pattern
         normalized_pattern = normalize_pattern(pattern)
 
+        [pattern, normalized_pattern, entry_owners, is_exclusion_pattern]
+      end
+
+      def validate_and_get_owners(entry_owners, section, line_number)
         if entry_owners.split.any? { |owner| invalid_owner?(owner) }
           add_error(Error::MALFORMED_ENTRY_OWNER, line_number)
         end
 
         owners = entry_owners.presence || section.default_owners
-
         add_error(Error::MISSING_ENTRY_OWNER, line_number) if owners.blank?
 
-        parsed[section.name][normalized_pattern] = Entry.new(
-          pattern,
-          owners,
-          section: section.name,
-          optional: section.optional,
-          approvals_required: section.approvals,
-          exclusion: Feature.enabled?(:codeowners_file_exclusions, project) && is_exclusion,
-          line_number: line_number
-        )
+        owners
       end
 
       def invalid_owner?(owner)
diff --git a/ee/spec/lib/gitlab/code_owners/file_spec.rb b/ee/spec/lib/gitlab/code_owners/file_spec.rb
index 8a37e77842d49c..cad92f10165dc2 100644
--- a/ee/spec/lib/gitlab/code_owners/file_spec.rb
+++ b/ee/spec/lib/gitlab/code_owners/file_spec.rb
@@ -484,6 +484,11 @@ def owner_line(pattern)
         CONTENT
       end
 
+      before do
+        allow(Gitlab::CodeOwners::UserPermissionCheck).to receive(:new).and_return(instance_double(
+          Gitlab::CodeOwners::UserPermissionCheck, errors: []))
+      end
+
       it 'matches non-excluded files to default owner' do
         entry = file.entries_for_path('file.txt').first
 
@@ -504,6 +509,18 @@ def owner_line(pattern)
         expect(entries).to be_empty
       end
 
+      it 'does not require owners for exclusion patterns' do
+        expect(file.valid?).to eq(true)
+
+        expect(file.errors).to be_empty
+        expect(file.errors).not_to match_array(
+          [
+            Gitlab::CodeOwners::Error.new(message: :missing_entry_owner, line_number: 2, path: 'CODEOWNERS'),
+            Gitlab::CodeOwners::Error.new(message: :missing_entry_owner, line_number: 6, path: 'CODEOWNERS')
+          ]
+        )
+      end
+
       context 'with nested exclusions' do
         let(:file_content) do
           <<~CONTENT
-- 
GitLab


From f3bcd72678619bb61a5c1f1168f1ed06704ee6df Mon Sep 17 00:00:00 2001
From: Olaoluwa Oluro <olaoluro@gitlab.com>
Date: Tue, 25 Feb 2025 13:43:51 +0000
Subject: [PATCH 2/3] Apply code review suggestion

---
 ee/lib/gitlab/code_owners/file.rb | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/ee/lib/gitlab/code_owners/file.rb b/ee/lib/gitlab/code_owners/file.rb
index 9699178254c7c5..974357baaeeab9 100644
--- a/ee/lib/gitlab/code_owners/file.rb
+++ b/ee/lib/gitlab/code_owners/file.rb
@@ -154,8 +154,9 @@ def get_parsed_data
       end
 
       def parse_entry(line, parsed, section, line_number)
-        pattern, normalized_pattern, entry_owners, is_exclusion_pattern = extract_line_pattern_and_entry_owners(line)
-        owners = is_exclusion_pattern ? nil : validate_and_get_owners(entry_owners, section, line_number)
+        pattern, entry_owners, is_exclusion_pattern = extract_entry_info(line)
+        normalized_pattern = normalize_pattern(pattern)
+        owners = validate_and_get_owners(entry_owners, section, line_number) unless is_exclusion_pattern
 
         parsed[section.name][normalized_pattern] = Entry.new(
           pattern,
@@ -168,14 +169,13 @@ def parse_entry(line, parsed, section, line_number)
         )
       end
 
-      def extract_line_pattern_and_entry_owners(line)
+      def extract_entry_info(line)
         pattern, _separator, entry_owners = line.partition(/(?<!\\)\s+/)
 
         is_exclusion_pattern = codeowners_file_exclusions_enabled? && pattern.start_with?('!')
         pattern = pattern[1..] if is_exclusion_pattern
-        normalized_pattern = normalize_pattern(pattern)
 
-        [pattern, normalized_pattern, entry_owners, is_exclusion_pattern]
+        [pattern, entry_owners, is_exclusion_pattern]
       end
 
       def validate_and_get_owners(entry_owners, section, line_number)
-- 
GitLab


From 0687273928659a22a93d18d060c615b8b76e6c4e Mon Sep 17 00:00:00 2001
From: Olaoluwa Oluro <olaoluro@gitlab.com>
Date: Thu, 27 Feb 2025 16:27:20 +0000
Subject: [PATCH 3/3] Code review suggestion

---
 ee/spec/lib/gitlab/code_owners/file_spec.rb | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/ee/spec/lib/gitlab/code_owners/file_spec.rb b/ee/spec/lib/gitlab/code_owners/file_spec.rb
index cad92f10165dc2..e65bbdcd73e0f4 100644
--- a/ee/spec/lib/gitlab/code_owners/file_spec.rb
+++ b/ee/spec/lib/gitlab/code_owners/file_spec.rb
@@ -513,12 +513,6 @@ def owner_line(pattern)
         expect(file.valid?).to eq(true)
 
         expect(file.errors).to be_empty
-        expect(file.errors).not_to match_array(
-          [
-            Gitlab::CodeOwners::Error.new(message: :missing_entry_owner, line_number: 2, path: 'CODEOWNERS'),
-            Gitlab::CodeOwners::Error.new(message: :missing_entry_owner, line_number: 6, path: 'CODEOWNERS')
-          ]
-        )
       end
 
       context 'with nested exclusions' do
-- 
GitLab