Skip to content

G
gitlab-ce
Commit de990d4b authored by Marc Shaw's avatar Marc Shaw
Browse files
Options

Remove diff_check_with_paths_changed_rpc feature flag 

Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/288827
MR: gitlab.com/gitlab-org/gitlab/-/merge_requests/50623
parent d71d8af3
Hide whitespace changes
Inline Side-by-side
Showing with 345 additions and 404 deletions
config/feature_flags/development/diff_check_with_paths_changed_rpc.yml deleted 100644 → 0
View file @ d71d8af3
---
name: diff_check_with_paths_changed_rpc
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/46116
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/288827
milestone: '13.7'
type: development
group: group::code review
default_enabled: true
ee/lib/ee/gitlab/checks/diff_check.rb
View file @ de990d4b
...@@ -9,7 +9,7 @@ module EE ...@@ -9,7 +9,7 @@ module EE
private private
def path_validations def file_paths_validations
validations = [super].flatten validations = [super].flatten
if validate_code_owners? if validate_code_owners?
...@@ -48,8 +48,8 @@ module EE ...@@ -48,8 +48,8 @@ module EE
push_rule.file_name_regex.present? || push_rule.prevent_secrets push_rule.file_name_regex.present? || push_rule.prevent_secrets
end end
override :validations_for_diff override :validations_for_path
def validations_for_diff def validations_for_path
super.tap do |validations| super.tap do |validations|
validations.push(path_locks_validation) if validate_path_locks? validations.push(path_locks_validation) if validate_path_locks?
validations.push(file_name_validation) if push_rule_checks_commit? validations.push(file_name_validation) if push_rule_checks_commit?
...@@ -57,17 +57,8 @@ module EE ...@@ -57,17 +57,8 @@ module EE
end end
def path_locks_validation def path_locks_validation
lambda do |diff| lambda do |changed_path|
path = if ::Feature.enabled?(:diff_check_with_paths_changed_rpc, project, default_enabled: true) path = changed_path.path
diff.path
else
if diff.renamed_file?
diff.old_path
else
diff.new_path || diff.old_path
end
end
lock_info = project.find_path_lock(path) lock_info = project.find_path_lock(path)
if lock_info && lock_info.user != user_access.user if lock_info && lock_info.user != user_access.user
...@@ -76,24 +67,12 @@ module EE ...@@ -76,24 +67,12 @@ module EE
end end
end end
def new_file?(path)
path.status == :ADDED
end
def file_name_validation def file_name_validation
lambda do |diff| lambda do |changed_path|
if ::Feature.enabled?(:diff_check_with_paths_changed_rpc, project, default_enabled: true) if changed_path.new_file? && denylisted_regex = push_rule.filename_denylisted?(changed_path.path)
if new_file?(diff) && denylisted_regex = push_rule.filename_denylisted?(diff.path) return unless denylisted_regex.present?
return unless denylisted_regex.present?
"File name #{changed_path.path} was blacklisted by the pattern #{denylisted_regex}."
"File name #{diff.path} was blacklisted by the pattern #{denylisted_regex}."
end
else
if (diff.renamed_file || diff.new_file) && denylisted_regex = push_rule.filename_denylisted?(diff.new_path)
return unless denylisted_regex.present?
"File name #{diff.new_path} was blacklisted by the pattern #{denylisted_regex}."
end
end end
rescue ::PushRule::MatchError => e rescue ::PushRule::MatchError => e
raise ::Gitlab::GitAccess::ForbiddenError, e.message raise ::Gitlab::GitAccess::ForbiddenError, e.message
......
ee/spec/lib/gitlab/checks/diff_check_spec.rb
View file @ de990d4b
...@@ -5,70 +5,60 @@ require 'spec_helper' ...@@ -5,70 +5,60 @@ require 'spec_helper'
RSpec.describe Gitlab::Checks::DiffCheck do RSpec.describe Gitlab::Checks::DiffCheck do
include FakeBlobHelpers include FakeBlobHelpers
shared_examples_for "diff check" do include_context 'push rules checks context'
include_context 'push rules checks context'
describe '#validate!' do describe '#validate!' do
let(:push_allowed) { false } let(:push_allowed) { false }
before do before do
allow(user_access).to receive(:can_push_to_branch?).and_return(push_allowed) allow(user_access).to receive(:can_push_to_branch?).and_return(push_allowed)
end end
shared_examples_for "returns codeowners validation message" do shared_examples_for "returns codeowners validation message" 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")
end
end end
end
context 'no push rules active' do context 'no push rules active' do
let_it_be(:push_rule) { create(:push_rule) } let_it_be(:push_rule) { create(:push_rule) }
it "does not attempt to check commits" do it "does not attempt to check commits" do
expect(subject).not_to receive(:process_commits) expect(subject).not_to receive(:process_commits)
subject.validate! subject.validate!
end
end end
end
describe '#validate_code_owners?' do describe '#validate_code_owners?' do
let_it_be(:push_rule) { create(:push_rule, file_name_regex: 'READ*') } let_it_be(:push_rule) { create(:push_rule, file_name_regex: 'READ*') }
let(:validate_code_owners) { subject.send(:validate_code_owners?) } let(:validate_code_owners) { subject.send(:validate_code_owners?) }
let(:protocol) { 'ssh' } let(:protocol) { 'ssh' }
let(:push_allowed) { false } let(:push_allowed) { false }
context 'when push_rules_supersede_code_owners is disabled' do
before do
stub_feature_flags(push_rules_supersede_code_owners: false)
end
it 'returns branch_requires_code_owner_approval?' do
expect(project).to receive(:branch_requires_code_owner_approval?).and_return(true)
expect(validate_code_owners).to eq(true) context 'when push_rules_supersede_code_owners is disabled' do
end before do
stub_feature_flags(push_rules_supersede_code_owners: false)
end end
context 'when user can not push to the branch' do it 'returns branch_requires_code_owner_approval?' do
context 'when not updated from web' do expect(project).to receive(:branch_requires_code_owner_approval?).and_return(true)
it 'checks if the branch requires code owner approval' do
expect(project).to receive(:branch_requires_code_owner_approval?).and_return(true)
expect(validate_code_owners).to eq(true) expect(validate_code_owners).to eq(true)
end end
end end
context 'when updated from the web' do context 'when user can not push to the branch' do
let(:protocol) { 'web' } context 'when not updated from web' do
it 'checks if the branch requires code owner approval' do
expect(project).to receive(:branch_requires_code_owner_approval?).and_return(true)
it 'returns false' do expect(validate_code_owners).to eq(true)
expect(validate_code_owners).to eq(false)
end
end end
end end
context 'when a user can push to the branch' do context 'when updated from the web' do
let(:push_allowed) { true } let(:protocol) { 'web' }
it 'returns false' do it 'returns false' do
expect(validate_code_owners).to eq(false) expect(validate_code_owners).to eq(false)
...@@ -76,278 +66,276 @@ RSpec.describe Gitlab::Checks::DiffCheck do ...@@ -76,278 +66,276 @@ RSpec.describe Gitlab::Checks::DiffCheck do
end end
end end
describe "#validate_code_owners" do context 'when a user can push to the branch' do
let!(:code_owner) { create(:user, username: "owner-1") } let(:push_allowed) { true }
let(:project) { create(:project, :repository) }
let(:codeowner_content) { "*.rb @#{code_owner.username}\ndocs/CODEOWNERS @owner-1\n*.js.coffee @owner-1" }
let(:codeowner_blob) { fake_blob(path: "CODEOWNERS", data: codeowner_content) }
let(:codeowner_blob_ref) { fake_blob(path: "CODEOWNERS", data: codeowner_content) }
let(:codeowner_lookup_ref) { merge_request.target_branch }
let(:merge_request) do
build(
:merge_request,
source_project: project,
source_branch: 'feature',
target_project: project,
target_branch: 'master'
)
end
before do it 'returns false' do
allow(project.repository).to receive(:code_owners_blob) expect(validate_code_owners).to eq(false)
.with(ref: codeowner_lookup_ref)
.and_return(codeowner_blob)
end end
end
end
context 'the MR contains a renamed file matching a file path' do describe "#validate_code_owners" do
let(:diff_check) { described_class.new(change_access) } let!(:code_owner) { create(:user, username: "owner-1") }
let(:protected_branch) { build(:protected_branch, name: 'master', project: project) } let(:project) { create(:project, :repository) }
let(:codeowner_content) { "*.rb @#{code_owner.username}\ndocs/CODEOWNERS @owner-1\n*.js.coffee @owner-1" }
let(:codeowner_blob) { fake_blob(path: "CODEOWNERS", data: codeowner_content) }
let(:codeowner_blob_ref) { fake_blob(path: "CODEOWNERS", data: codeowner_content) }
let(:codeowner_lookup_ref) { merge_request.target_branch }
let(:merge_request) do
build(
:merge_request,
source_project: project,
source_branch: 'feature',
target_project: project,
target_branch: 'master'
)
end
before do before do
expect(project).to receive(:branch_requires_code_owner_approval?) allow(project.repository).to receive(:code_owners_blob)
.at_least(:once).and_return(true) .with(ref: codeowner_lookup_ref)
.and_return(codeowner_blob)
end
# This particular commit renames a file: context 'the MR contains a renamed file matching a file path' do
allow(project.repository).to receive(:new_commits).and_return( let(:diff_check) { described_class.new(change_access) }
[project.repository.commit('6907208d755b60ebeacb2e9dfea74c92c3449a1f')] let(:protected_branch) { build(:protected_branch, name: 'master', project: project) }
)
end
it "returns an error message" do before do
expect { diff_check.validate! }.to raise_error do |error| expect(project).to receive(:branch_requires_code_owner_approval?)
expect(error).to be_a(Gitlab::GitAccess::ForbiddenError) .at_least(:once).and_return(true)
expect(error.message).to include("CODEOWNERS` were matched:\n- *.js.coffee")
end # This particular commit renames a file:
end allow(project.repository).to receive(:new_commits).and_return(
[project.repository.commit('6907208d755b60ebeacb2e9dfea74c92c3449a1f')]
)
end end
context "the MR contains a matching file path" do it "returns an error message" do
let(:validation_result) do expect { diff_check.validate! }.to raise_error do |error|
subject.send(:validate_code_owners).call(["docs/CODEOWNERS", "README"]) expect(error).to be_a(Gitlab::GitAccess::ForbiddenError)
expect(error.message).to include("CODEOWNERS` were matched:\n- *.js.coffee")
end end
end
end
before do context "the MR contains a matching file path" do
expect(project).to receive(:branch_requires_code_owner_approval?) let(:validation_result) do
.at_least(:once).and_return(true) subject.send(:validate_code_owners).call(["docs/CODEOWNERS", "README"])
end end
it_behaves_like "returns codeowners validation message" before do
expect(project).to receive(:branch_requires_code_owner_approval?)
.at_least(:once).and_return(true)
end end
context "the MR doesn't contain a matching file path" do it_behaves_like "returns codeowners validation message"
it "returns nil" do end
expect(subject.send(:validate_code_owners)
.call(["docs/SAFE_FILE_NAME", "README"])).to be_nil context "the MR doesn't contain a matching file path" do
end it "returns nil" do
expect(subject.send(:validate_code_owners)
.call(["docs/SAFE_FILE_NAME", "README"])).to be_nil
end end
end end
end
describe "#path_validations" do describe "#file_paths_validations" do
include_context 'change access checks context' include_context 'change access checks context'
context "when the feature isn't enabled on the project" do context "when the feature isn't enabled on the project" do
before do
expect(project).to receive(:branch_requires_code_owner_approval?)
.once.and_return(false)
end
it "returns an empty array" do
expect(subject.send(:file_paths_validations)).to eq([])
end
end
context "when the feature is enabled on the project" do
context "updated_from_web? == false" do
before do before do
expect(subject).to receive(:updated_from_web?).and_return(false)
expect(project).to receive(:branch_requires_code_owner_approval?) expect(project).to receive(:branch_requires_code_owner_approval?)
.once.and_return(false) .once.and_return(true)
end end
it "returns an empty array" do it "returns an array of Proc(s)" do
expect(subject.send(:path_validations)).to eq([]) validations = subject.send(:file_paths_validations)
expect(validations.any?).to be_truthy
expect(validations.any? { |v| !v.is_a? Proc }).to be_falsy
end end
end end
context "when the feature is enabled on the project" do context "updated_from_web? == true" do
context "updated_from_web? == false" do before do
before do expect(subject).to receive(:updated_from_web?).and_return(true)
expect(subject).to receive(:updated_from_web?).and_return(false)
expect(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 end
context "updated_from_web? == true" do it "returns an empty array" do
before do expect(subject.send(:file_paths_validations)).to eq([])
expect(subject).to receive(:updated_from_web?).and_return(true)
end
it "returns an empty array" do
expect(subject.send(:path_validations)).to eq([])
end
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'
context 'file name regex check' do context 'file name regex check' do
let!(:push_rule) { create(:push_rule, file_name_regex: 'READ*') } let!(:push_rule) { create(:push_rule, file_name_regex: 'READ*') }
it_behaves_like 'check ignored when push rule unlicensed' it_behaves_like 'check ignored when push rule unlicensed'
it "returns an error if a new or renamed filed doesn't match the file name regex" do it "returns an error if a new or renamed filed doesn't match the file name regex" do
expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "File name README was blacklisted by the pattern READ*.") expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "File name README was blacklisted by the pattern READ*.")
end end
it 'returns an error if the regex is invalid' do it 'returns an error if the regex is invalid' do
push_rule.file_name_regex = '+' push_rule.file_name_regex = '+'
expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, /\ARegular expression '\+' is invalid/) expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, /\ARegular expression '\+' is invalid/)
end
end end
end
context 'blacklisted files check' do context 'blacklisted files check' do
let(:push_rule) { create(:push_rule, prevent_secrets: true) } let(:push_rule) { create(:push_rule, prevent_secrets: true) }
it_behaves_like 'check ignored when push rule unlicensed' it_behaves_like 'check ignored when push rule unlicensed'
it "returns true if there is no blacklisted files" do it "returns true if there is no blacklisted files" do
new_rev = nil new_rev = nil
white_listed = white_listed =
[ [
'readme.txt', 'any/ida_rsa.pub', 'any/id_dsa.pub', 'any_2/id_ed25519.pub', 'readme.txt', 'any/ida_rsa.pub', 'any/id_dsa.pub', 'any_2/id_ed25519.pub',
'random_file.pdf', 'folder/id_ecdsa.pub', 'docs/aws/credentials.md', 'ending_withhistory' 'random_file.pdf', 'folder/id_ecdsa.pub', 'docs/aws/credentials.md', 'ending_withhistory'
] ]
white_listed.each do |file_path| white_listed.each do |file_path|
old_rev = 'be93687618e4b132087f430a4d8fc3a609c9b77c' old_rev = 'be93687618e4b132087f430a4d8fc3a609c9b77c'
old_rev = new_rev if new_rev old_rev = new_rev if new_rev
new_rev = project.repository.create_file(user, file_path, "commit #{file_path}", message: "commit #{file_path}", branch_name: "master") new_rev = project.repository.create_file(user, file_path, "commit #{file_path}", message: "commit #{file_path}", branch_name: "master")
allow(project.repository).to receive(:new_commits).and_return( allow(project.repository).to receive(:new_commits).and_return(
project.repository.commits_between(old_rev, new_rev) project.repository.commits_between(old_rev, new_rev)
) )
expect(subject.validate!).to be_truthy expect(subject.validate!).to be_truthy
end
end end
end
it "returns an error if a new or renamed filed doesn't match the file name regex" do it "returns an error if a new or renamed filed doesn't match the file name regex" do
new_rev = nil new_rev = nil
black_listed = black_listed =
[ [
'aws/credentials', '.ssh/personal_rsa', 'config/server_rsa', '.ssh/id_rsa', '.ssh/id_dsa', 'aws/credentials', '.ssh/personal_rsa', 'config/server_rsa', '.ssh/id_rsa', '.ssh/id_dsa',
'.ssh/personal_dsa', 'config/server_ed25519', 'any/id_ed25519', '.ssh/personal_ecdsa', 'config/server_ecdsa', '.ssh/personal_dsa', 'config/server_ed25519', 'any/id_ed25519', '.ssh/personal_ecdsa', 'config/server_ecdsa',
'any_place/id_ecdsa', 'some_pLace/file.key', 'other_PlAcE/other_file.pem', 'bye_bug.history', 'pg_sql_history' 'any_place/id_ecdsa', 'some_pLace/file.key', 'other_PlAcE/other_file.pem', 'bye_bug.history', 'pg_sql_history'
] ]
black_listed.each do |file_path| black_listed.each do |file_path|
old_rev = 'be93687618e4b132087f430a4d8fc3a609c9b77c' old_rev = 'be93687618e4b132087f430a4d8fc3a609c9b77c'
old_rev = new_rev if new_rev old_rev = new_rev if new_rev
new_rev = project.repository.create_file(user, file_path, "commit #{file_path}", message: "commit #{file_path}", branch_name: "master") new_rev = project.repository.create_file(user, file_path, "commit #{file_path}", message: "commit #{file_path}", branch_name: "master")
allow(subject).to receive(:commits).and_return( allow(subject).to receive(:commits).and_return(
project.repository.commits_between(old_rev, new_rev) project.repository.commits_between(old_rev, new_rev)
) )
expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, /File name #{file_path} was blacklisted by the pattern/) expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, /File name #{file_path} was blacklisted by the pattern/)
end
end end
end end
end end
end
context 'file lock rules' do context 'file lock rules' do
let_it_be(:push_rule) { create(:push_rule) } let_it_be(:push_rule) { create(:push_rule) }
let_it_be(:owner) { create(:user) } let_it_be(:owner) { create(:user) }
let(:path_lock) { create(:path_lock, path: 'README', project: project) } let(:path_lock) { create(:path_lock, path: 'README', project: project) }
before do before do
project.add_developer(owner) project.add_developer(owner)
end end
shared_examples 'a locked file' do shared_examples 'a locked file' do
let!(:path_lock) { create(:path_lock, path: filename, project: project, user: owner) } let!(:path_lock) { create(:path_lock, path: filename, project: project, user: owner) }
before do before do
allow(project.repository).to receive(:new_commits).and_return( allow(project.repository).to receive(:new_commits).and_return(
[project.repository.commit(sha)] [project.repository.commit(sha)]
) )
end end
context 'and path is locked by another user' do context 'and path is locked by another user' do
it 'returns an error' do it 'returns an error' do
path_lock path_lock
expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "The path '#{filename}' is locked by #{path_lock.user.name}") expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "The path '#{filename}' is locked by #{path_lock.user.name}")
end
end end
end
context 'and path is locked by current user' do context 'and path is locked by current user' do
let(:user) { owner } let(:user) { owner }
it 'is allows changes' do it 'is allows changes' do
path_lock path_lock
expect { subject.validate! }.not_to raise_error expect { subject.validate! }.not_to raise_error
end
end end
end end
end
context 'when file has changes' do context 'when file has changes' do
let_it_be(:filename) { 'files/ruby/popen.rb' } let_it_be(:filename) { 'files/ruby/popen.rb' }
let_it_be(:sha) { '570e7b2abdd848b95f2f578043fc23bd6f6fd24d' } let_it_be(:sha) { '570e7b2abdd848b95f2f578043fc23bd6f6fd24d' }
it_behaves_like 'a locked file' it_behaves_like 'a locked file'
end end
context 'when file is renamed' do context 'when file is renamed' do
let_it_be(:filename) { 'files/js/commit.js.coffee' } let_it_be(:filename) { 'files/js/commit.js.coffee' }
let_it_be(:sha) { '6907208d755b60ebeacb2e9dfea74c92c3449a1f' } let_it_be(:sha) { '6907208d755b60ebeacb2e9dfea74c92c3449a1f' }
it_behaves_like 'a locked file' it_behaves_like 'a locked file'
end end
context 'when file is deleted' do context 'when file is deleted' do
let_it_be(:filename) { 'files/js/commit.js.coffee' } let_it_be(:filename) { 'files/js/commit.js.coffee' }
let_it_be(:sha) { 'd59c60028b053793cecfb4022de34602e1a9218e' } let_it_be(:sha) { 'd59c60028b053793cecfb4022de34602e1a9218e' }
it_behaves_like 'a locked file' it_behaves_like 'a locked file'
end end
it 'memoizes the validate_path_locks? call' do it 'memoizes the validate_path_locks? call' do
expect(project).to receive(:any_path_locks?).once.and_call_original expect(project).to receive(:any_path_locks?).once.and_call_original
2.times { subject.validate! } 2.times { subject.validate! }
end end
context 'when the branch is being deleted' do context 'when the branch is being deleted' do
let(:newrev) { Gitlab::Git::BLANK_SHA } let(:newrev) { Gitlab::Git::BLANK_SHA }
it 'does not run' do it 'does not run' do
path_lock path_lock
expect { subject.validate! }.not_to raise_error expect { subject.validate! }.not_to raise_error
end
end end
end
context 'when there is no valid change' do context 'when there is no valid change' do
let(:changes) { { oldrev: '_any', newrev: nil, ref: nil } } let(:changes) { { oldrev: '_any', newrev: nil, ref: nil } }
it 'does not run' do it 'does not run' do
path_lock path_lock
expect { subject.validate! }.not_to raise_error expect { subject.validate! }.not_to raise_error
end
end end
end end
end end
end end
it_behaves_like "diff check"
context 'when diff check with paths rpc feature flag is false' do
before do
stub_feature_flags(diff_check_with_paths_changed_rpc: false)
end
it_behaves_like "diff check"
end
end end
lib/gitlab/checks/diff_check.rb
View file @ de990d4b
...@@ -6,37 +6,20 @@ module Gitlab ...@@ -6,37 +6,20 @@ module Gitlab
include Gitlab::Utils::StrongMemoize include Gitlab::Utils::StrongMemoize
LOG_MESSAGES = { LOG_MESSAGES = {
validate_file_paths: "Validating diffs' file paths...", validate_file_paths: "Validating diffs' file paths..."
diff_content_check: "Validating diff contents..."
}.freeze }.freeze
def validate! def validate!
return if deletion? return if deletion?
return unless should_run_diff_validations? return unless should_run_validations?
return if commits.empty? return if commits.empty?
file_paths = [] paths = project.repository.find_changed_paths(commits.map(&:sha))
paths.each do |path|
if ::Feature.enabled?(:diff_check_with_paths_changed_rpc, project, default_enabled: true) validate_path(path)
paths = project.repository.find_changed_paths(commits.map(&:sha))
paths.each do |path|
file_paths.concat([path.path])
validate_diff(path)
end
else
process_commits do |commit|
validate_once(commit) do
commit.raw_deltas.each do |diff|
file_paths.concat([diff.new_path, diff.old_path].compact)
validate_diff(diff)
end
end
end
end end
validate_file_paths(file_paths.uniq) validate_file_paths(paths.map(&:path).uniq)
end end
private private
...@@ -47,43 +30,30 @@ module Gitlab ...@@ -47,43 +30,30 @@ module Gitlab
end end
end end
def should_run_diff_validations? def should_run_validations?
validations_for_diff.present? || path_validations.present? validations_for_path.present? || file_paths_validations.present?
end end
def validate_diff(diff) def validate_path(path)
validations_for_diff.each do |validation| validations_for_path.each do |validation|
if error = validation.call(diff) if error = validation.call(path)
raise ::Gitlab::GitAccess::ForbiddenError, error raise ::Gitlab::GitAccess::ForbiddenError, error
end end
end end
end end
# Method overwritten in EE to inject custom validations # Method overwritten in EE to inject custom validations
def validations_for_diff def validations_for_path
[] []
end end
def path_validations def file_paths_validations
validate_lfs_file_locks? ? [lfs_file_locks_validation] : [] validate_lfs_file_locks? ? [lfs_file_locks_validation] : []
end end
def process_commits
logger.log_timed(LOG_MESSAGES[:diff_content_check]) do
# n+1: https://gitlab.com/gitlab-org/gitlab/issues/3593
::Gitlab::GitalyClient.allow_n_plus_1_calls do
commits.each do |commit|
logger.check_timeout_reached
yield(commit)
end
end
end
end
def validate_file_paths(file_paths) def validate_file_paths(file_paths)
logger.log_timed(LOG_MESSAGES[__method__]) do logger.log_timed(LOG_MESSAGES[__method__]) do
path_validations.each do |validation| file_paths_validations.each do |validation|
if error = validation.call(file_paths) if error = validation.call(file_paths)
raise ::Gitlab::GitAccess::ForbiddenError, error raise ::Gitlab::GitAccess::ForbiddenError, error
end end
......
lib/gitlab/git/changed_path.rb 0 → 100644
View file @ de990d4b
# frozen_string_literal: true
module Gitlab
module Git
class ChangedPath
attr_reader :status, :path
def initialize(status:, path:)
@status = status
@path = path
end
def new_file?
status == :ADDED
end
end
end
end
lib/gitlab/gitaly_client/commit_service.rb
View file @ de990d4b
...@@ -225,7 +225,7 @@ module Gitlab ...@@ -225,7 +225,7 @@ module Gitlab
response = GitalyClient.call(@repository.storage, :diff_service, :find_changed_paths, request, timeout: GitalyClient.medium_timeout) response = GitalyClient.call(@repository.storage, :diff_service, :find_changed_paths, request, timeout: GitalyClient.medium_timeout)
response.flat_map do |msg| response.flat_map do |msg|
msg.paths.map do |path| msg.paths.map do |path|
OpenStruct.new( Gitlab::Git::ChangedPath.new(
status: path.status, status: path.status,
path: EncodingHelper.encode!(path.path) path: EncodingHelper.encode!(path.path)
) )
......
spec/lib/gitlab/checks/diff_check_spec.rb
View file @ de990d4b
...@@ -6,96 +6,63 @@ RSpec.describe Gitlab::Checks::DiffCheck do ...@@ -6,96 +6,63 @@ RSpec.describe Gitlab::Checks::DiffCheck do
include_context 'change access checks context' include_context 'change access checks context'
describe '#validate!' do describe '#validate!' do
let(:owner) { create(:user) } context 'when commits is empty' do
it 'does not call find_changed_paths' do
before do expect(project.repository).not_to receive(:find_changed_paths)
allow(project.repository).to receive(:new_commits).and_return(
project.repository.commits_between('be93687618e4b132087f430a4d8fc3a609c9b77c', '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51')
)
end
context 'with LFS not enabled' do
before do
allow(project).to receive(:lfs_enabled?).and_return(false)
end
it 'does not invoke :lfs_file_locks_validation' do
expect(subject).not_to receive(:lfs_file_locks_validation)
subject.validate! subject.validate!
end end
end end
context 'with LFS enabled' do context 'when commits is not empty' do
let!(:lock) { create(:lfs_file_lock, user: owner, project: project, path: 'README') }
before do before do
allow(project).to receive(:lfs_enabled?).and_return(true) allow(project.repository).to receive(:new_commits).and_return(
project.repository.commits_between('be93687618e4b132087f430a4d8fc3a609c9b77c', '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51')
)
end end
context 'when change is sent by a different user' do context 'when deletion is true' do
context 'when diff check with paths rpc feature flag is true' do let(:newrev) { Gitlab::Git::BLANK_SHA }
it 'raises an error if the user is not allowed to update the file' do
expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "The path 'README' is locked in Git LFS by #{lock.user.name}")
end
end
context 'when diff check with paths rpc feature flag is false' do it 'does not call find_changed_paths' do
before do expect(project.repository).not_to receive(:find_changed_paths)
stub_feature_flags(diff_check_with_paths_changed_rpc: false)
end
it 'raises an error if the user is not allowed to update the file' do subject.validate!
expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "The path 'README' is locked in Git LFS by #{lock.user.name}")
end
end end
end end
context 'when change is sent by the author of the lock' do context 'with LFS not enabled' do
let(:user) { owner } before do
allow(project).to receive(:lfs_enabled?).and_return(false)
it "doesn't raise any error" do
expect { subject.validate! }.not_to raise_error
end end
end
end
context 'commit diff validations' do
before do
allow(subject).to receive(:validations_for_diff).and_return([lambda { |diff| return }])
expect_any_instance_of(Commit).to receive(:raw_deltas).and_call_original
stub_feature_flags(diff_check_with_paths_changed_rpc: false)
subject.validate!
end
context 'when request store is inactive' do it 'does not invoke :lfs_file_locks_validation' do
it 'are run for every commit' do expect(subject).not_to receive(:lfs_file_locks_validation)
expect_any_instance_of(Commit).to receive(:raw_deltas).and_call_original
subject.validate! subject.validate!
end end
end end
context 'when request store is active', :request_store do context 'with LFS enabled' do
it 'are cached for every commit' do let(:owner) { create(:user) }
expect_any_instance_of(Commit).not_to receive(:raw_deltas) let!(:lock) { create(:lfs_file_lock, user: owner, project: project, path: 'README') }
subject.validate! before do
allow(project).to receive(:lfs_enabled?).and_return(true)
end end
it 'are run for not cached commits' do context 'when change is sent by a different user' do
allow(project.repository).to receive(:new_commits).and_return( it 'raises an error if the user is not allowed to update the file' do
project.repository.commits_between('be93687618e4b132087f430a4d8fc3a609c9b77c', 'a5391128b0ef5d21df5dd23d98557f4ef12fae20') expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "The path 'README' is locked in Git LFS by #{lock.user.name}")
) end
change_access.instance_variable_set(:@commits, project.repository.new_commits) end
expect(project.repository.new_commits.first).not_to receive(:raw_deltas).and_call_original context 'when change is sent by the author of the lock' do
expect(project.repository.new_commits.last).to receive(:raw_deltas).and_call_original let(:user) { owner }
subject.validate! it "doesn't raise any error" do
expect { subject.validate! }.not_to raise_error
end
end end
end end
end end
......
spec/lib/gitlab/git/changed_path_spec.rb 0 → 100644
View file @ de990d4b
# frozen_string_literal: true
require "spec_helper"
RSpec.describe Gitlab::Git::ChangedPath do
subject(:changed_path) { described_class.new(path: path, status: status) }
let(:path) { 'test_path' }
describe '#new_file?' do
subject(:new_file?) { changed_path.new_file? }
context 'when it is a new file' do
let(:status) { :ADDED }
it 'returns true' do
expect(new_file?).to eq(true)
end
end
context 'when it is not a new file' do
let(:status) { :MODIFIED }
it 'returns false' do
expect(new_file?).to eq(false)
end
end
end
end
spec/lib/gitlab/git/repository_spec.rb
View file @ de990d4b
...@@ -1191,25 +1191,25 @@ RSpec.describe Gitlab::Git::Repository, :seed_helper do ...@@ -1191,25 +1191,25 @@ RSpec.describe Gitlab::Git::Repository, :seed_helper do
let(:commit_3) { '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9' } let(:commit_3) { '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9' }
let(:commit_1_files) do let(:commit_1_files) do
[ [
OpenStruct.new(status: :ADDED, path: "files/executables/ls"), Gitlab::Git::ChangedPath.new(status: :ADDED, path: "files/executables/ls"),
OpenStruct.new(status: :ADDED, path: "files/executables/touch"), Gitlab::Git::ChangedPath.new(status: :ADDED, path: "files/executables/touch"),
OpenStruct.new(status: :ADDED, path: "files/links/regex.rb"), Gitlab::Git::ChangedPath.new(status: :ADDED, path: "files/links/regex.rb"),
OpenStruct.new(status: :ADDED, path: "files/links/ruby-style-guide.md"), Gitlab::Git::ChangedPath.new(status: :ADDED, path: "files/links/ruby-style-guide.md"),
OpenStruct.new(status: :ADDED, path: "files/links/touch"), Gitlab::Git::ChangedPath.new(status: :ADDED, path: "files/links/touch"),
OpenStruct.new(status: :MODIFIED, path: ".gitmodules"), Gitlab::Git::ChangedPath.new(status: :MODIFIED, path: ".gitmodules"),
OpenStruct.new(status: :ADDED, path: "deeper/nested/six"), Gitlab::Git::ChangedPath.new(status: :ADDED, path: "deeper/nested/six"),
OpenStruct.new(status: :ADDED, path: "nested/six") Gitlab::Git::ChangedPath.new(status: :ADDED, path: "nested/six")
] ]
end end
let(:commit_2_files) do let(:commit_2_files) do
[OpenStruct.new(status: :ADDED, path: "bin/executable")] [Gitlab::Git::ChangedPath.new(status: :ADDED, path: "bin/executable")]
end end
let(:commit_3_files) do let(:commit_3_files) do
[ [
OpenStruct.new(status: :MODIFIED, path: ".gitmodules"), Gitlab::Git::ChangedPath.new(status: :MODIFIED, path: ".gitmodules"),
OpenStruct.new(status: :ADDED, path: "gitlab-shell") Gitlab::Git::ChangedPath.new(status: :ADDED, path: "gitlab-shell")
] ]
end end
...@@ -1217,7 +1217,7 @@ RSpec.describe Gitlab::Git::Repository, :seed_helper do ...@@ -1217,7 +1217,7 @@ RSpec.describe Gitlab::Git::Repository, :seed_helper do
collection = repository.find_changed_paths([commit_1, commit_2, commit_3]) collection = repository.find_changed_paths([commit_1, commit_2, commit_3])
expect(collection).to be_a(Enumerable) expect(collection).to be_a(Enumerable)
expect(collection.to_a).to eq(commit_1_files + commit_2_files + commit_3_files) expect(collection.as_json).to eq((commit_1_files + commit_2_files + commit_3_files).as_json)
end end
it 'returns no paths when SHAs are invalid' do it 'returns no paths when SHAs are invalid' do
...@@ -1231,7 +1231,7 @@ RSpec.describe Gitlab::Git::Repository, :seed_helper do ...@@ -1231,7 +1231,7 @@ RSpec.describe Gitlab::Git::Repository, :seed_helper do
collection = repository.find_changed_paths([nil, commit_1]) collection = repository.find_changed_paths([nil, commit_1])
expect(collection).to be_a(Enumerable) expect(collection).to be_a(Enumerable)
expect(collection.to_a).to eq(commit_1_files) expect(collection.as_json).to eq(commit_1_files.as_json)
end end
it 'returns no paths when the commits are nil' do it 'returns no paths when the commits are nil' do
......
spec/lib/gitlab/gitaly_client/commit_service_spec.rb
View file @ de990d4b
...@@ -162,11 +162,9 @@ RSpec.describe Gitlab::GitalyClient::CommitService do ...@@ -162,11 +162,9 @@ RSpec.describe Gitlab::GitalyClient::CommitService do
.with(request, kind_of(Hash)).and_return([changed_paths_response]) .with(request, kind_of(Hash)).and_return([changed_paths_response])
returned_value = described_class.new(repository).find_changed_paths(commits) returned_value = described_class.new(repository).find_changed_paths(commits)
mapped_expected_value = changed_paths_response.paths.map { |path| Gitlab::Git::ChangedPath.new(status: path.status, path: path.path) }
mapped_returned_value = returned_value.map(&:to_h) expect(returned_value.as_json).to eq(mapped_expected_value.as_json)
mapped_expected_value = changed_paths_response.paths.map(&:to_h)
expect(mapped_returned_value).to eq(mapped_expected_value)
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
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7