Commit c56b2421 authored by Igor Drozdov's avatar Igor Drozdov

Merge branch 'mk/apply-codeowner-validations-to-web-requests' into 'master'

Apply codeowner validations to web requests

See merge request gitlab-org/gitlab!31283
parents 222ae11b 0bedee33
---
title: Apply CODEOWNERS validations to web requests
merge_request: 31283
author:
type: security
...@@ -12,13 +12,28 @@ module EE ...@@ -12,13 +12,28 @@ module EE
def path_validations def path_validations
validations = [super].flatten validations = [super].flatten
if !updated_from_web? && project.branch_requires_code_owner_approval?(branch_name) if validate_code_owners?
validations << validate_code_owners validations << validate_code_owners
end end
validations validations
end end
def validate_code_owners?
return false if updated_from_web? && skip_web_ui_code_owner_validations?
project.branch_requires_code_owner_approval?(branch_name)
end
# To allow self-hosted installations to ignore CODEOWNERS rules when
# clicking Merge in the UI. By default, these rules are not skipped.
#
# Issue to remove this feature flag:
# https://gitlab.com/gitlab-org/gitlab/-/issues/217427
def skip_web_ui_code_owner_validations?
::Feature.enabled?(:skip_web_ui_code_owner_validations, project)
end
def validate_code_owners def validate_code_owners
lambda do |paths| lambda do |paths|
loader = ::Gitlab::CodeOwners::Loader.new(project, branch_name, paths) loader = ::Gitlab::CodeOwners::Loader.new(project, branch_name, paths)
...@@ -34,11 +49,13 @@ module EE ...@@ -34,11 +49,13 @@ module EE
matched_rules = loader.entries.collect { |e| "- #{e.pattern}" } matched_rules = loader.entries.collect { |e| "- #{e.pattern}" }
code_owner_path = project.repository.code_owners_blob(ref: branch_name).path || "CODEOWNERS" code_owner_path = project.repository.code_owners_blob(ref: branch_name).path || "CODEOWNERS"
"Pushes to protected branches that contain changes to files that\n" \ msg = "Pushes to protected branches that contain changes to files that\n" \
"match patterns defined in `#{code_owner_path}` are disabled for\n" \ "match patterns defined in `#{code_owner_path}` are disabled for\n" \
"this project. Please submit these changes via a merge request.\n\n" \ "this project. Please submit these changes via a merge request.\n\n" \
"The following pattern(s) from `#{code_owner_path}` were matched:\n" \ "The following pattern(s) from `#{code_owner_path}` were matched:\n" \
"#{matched_rules.join('\n')}\n" "#{matched_rules.join('\n')}\n"
updated_from_web? ? msg.tr("\n", " ") : msg
end end
def validate_path_locks? def validate_path_locks?
......
...@@ -89,8 +89,22 @@ describe Gitlab::Checks::DiffCheck do ...@@ -89,8 +89,22 @@ describe Gitlab::Checks::DiffCheck do
stub_feature_flags(sectional_codeowners: false) stub_feature_flags(sectional_codeowners: false)
end end
context "for a non-web-based request" do
it "returns an error message" do it "returns an error message" do
expect(validation_result).to include("Pushes to protected branches") expect(validation_result).to include("Pushes to protected branches")
expect(validation_result).to include("\n")
end
end
context "for a web-based request" do
before do
allow(subject).to receive(:updated_from_web?).and_return(true)
end
it "returns an error message with newline chars removed" do
expect(validation_result).to include("Pushes to protected branches")
expect(validation_result).not_to include("\n")
end
end end
end end
...@@ -211,6 +225,26 @@ describe Gitlab::Checks::DiffCheck do ...@@ -211,6 +225,26 @@ describe Gitlab::Checks::DiffCheck do
context "updated_from_web? == true" do context "updated_from_web? == true" do
before do before do
expect(subject).to receive(:updated_from_web?).and_return(true) expect(subject).to receive(:updated_from_web?).and_return(true)
end
context "when skip_web_ui_code_owner_validations is disabled" do
before do
stub_feature_flags(skip_web_ui_code_owner_validations: false)
allow(project).to receive(:branch_requires_code_owner_approval?)
.once.and_return(true)
end
it "returns an array of Proc(s)" do
validations = subject.send(:path_validations)
expect(validations.any?).to be_truthy
expect(validations.any? { |v| !v.is_a? Proc }).to be_falsy
end
end
context "when skip_web_ui_code_owner_validations is enabled" do
before do
stub_feature_flags(skip_web_ui_code_owner_validations: true)
expect(project).not_to receive(:branch_requires_code_owner_approval?) expect(project).not_to receive(:branch_requires_code_owner_approval?)
end end
...@@ -220,6 +254,7 @@ describe Gitlab::Checks::DiffCheck do ...@@ -220,6 +254,7 @@ describe Gitlab::Checks::DiffCheck do
end end
end end
end end
end
context 'file name rules' do context 'file name rules' do
# Notice that the commit used creates a file named 'README' # Notice that the commit used creates a file named 'README'
......
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