Read about our upcoming Code of Conduct on this issue

Commit 3b77417e authored by Georges Racinet's avatar Georges Racinet 🦑
Browse files

Restore branch protection for Mercurial

The fix is simple enough and all functional tests are passing.
This is the occasion to restore/adapt the RSpec tests for
branch protections.

This still have the unwanted consequence that some MR merges
will fail, because the related permission checks (occurring
in pre-receive API endpoint) won't find the matching MR.

This will be handled in a subsequent changeset, for more
on this, see #368.

--HG--
branch : heptapod-stable
parent cdb21e4bda27
......@@ -50,10 +50,9 @@ def prohibited_branch_checks
def protected_branch_checks
logger.log_timed(LOG_MESSAGES[:protected_branch_checks]) do
return unless ProtectedBranch.protected?(project, branch_name) # rubocop:disable Cop/AvoidReturnFromBlocks
return if project.vcs_type == 'hg_git'
if forced_push?
raise GitAccess::ForbiddenError, ERROR_MESSAGES[:force_push_protected_branch]
raise GitAccess::ForbiddenError, ERROR_MESSAGES[:force_push_protected_branch] unless project.mercurial?
end
end
......
......@@ -656,41 +656,55 @@ def disable_protocol(protocol)
end
end
# very simplified from Git case because we don't test behaviour on protected
# branches, and we won't do it before the 0.16 cycle as it may involve
# change of behaviour at a bad time.
describe '#check_push_access!' do
let(:unprotected_branch) { 'unprotected_branch' }
let(:unprotected_branch) { 'branch/unprotected' }
# yes, the protected branch is 'feature'. That probably means it's the
# *potentially* protected branch
# TODO we'll actually need a method to spit out a topic name
# for a protected branch name (either branch/feature or branch/exact
let(:topic_protected_branch) { 'topic/feature/follow-up' }
before do
mergeable_into_protected_branch
end
let(:changes) do
{ any: Gitlab::GitAccess::ANY,
push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow",
push_master: '6f6d7e7ed 570e7b2ab refs/heads/master',
push_master: '6f6d7e7ed 570e7b2ab refs/heads/branch/default',
push_protected_branch: '6f6d7e7ed 570e7b2ab refs/heads/branch/feature',
push_remove_protected_branch: "570e7b2ab #{Gitlab::Git::BLANK_SHA} "\
'refs/heads/branch/feature',
push_tag: '6f6d7e7ed 570e7b2ab refs/tags/v1.0.0',
push_new_tag: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/tags/v7.8.9",
push_all: ['6f6d7e7ed 570e7b2ab refs/heads/master', '6f6d7e7ed 570e7b2ab refs/heads/branch/feature'],
merge_into_protected_branch: "0b4bc9a #{merge_into_protected_branch} refs/heads/branch/feature" }
merge_into_protected_branch: "0b4bc9a #{mergeable_into_protected_branch} refs/heads/branch/feature" }
end
def merge_into_protected_branch
@protected_branch_merge_commit ||= begin
project.repository.add_branch(user, unprotected_branch, 'feature')
rugged = rugged_repo(project.repository)
target_branch = rugged.rev_parse('feature')
source_branch = project.repository.create_file(
user,
'filename',
'This is the file content',
message: 'This is a good commit message',
branch_name: unprotected_branch)
author = { email: "email@example.com", time: Time.now, name: "Example Git User" }
merge_index = rugged.merge_commits(target_branch, source_branch)
Rugged::Commit.create(rugged, author: author, committer: author, message: "commit message", parents: [target_branch, source_branch], tree: merge_index.write_tree(rugged))
end
# GR rename as prepare_mergeable_into_protected_branch ?
def mergeable_into_protected_branch
# This is obviously to avoid repeated setup. The examples don't
# actually perform actual pushes, so we can indeed just keep it as is
@mergeable_into_protected_branch_sha ||=
begin
project.repository.create_file(
project.creator,
'feature', # file name
'start of "feature" branch', # file content
branch_name: 'branch/feature',
start_branch_name: 'branch/default',
message: "potentially protected branch creation"
)
project.repository.create_file(
user,
'filename',
'This is the file content',
message: 'This is a good commit message',
start_branch_name: 'branch/feature',
branch_name: topic_protected_branch
)
project.repository.find_branch(topic_protected_branch).target
end
end
def self.run_permission_checks(permissions_matrix)
......@@ -708,7 +722,7 @@ def self.run_permission_checks(permissions_matrix)
project.add_role(user, role)
end
protected_branch.save
protected_branch.save!
aggregate_failures do
matrix.each do |action, allowed|
......@@ -789,9 +803,64 @@ def self.run_permission_checks(permissions_matrix)
}
}
[%w(branch/feature exact), ['branch/feat*', 'wildcard']].each do |protected_branch_name, protected_branch_type|
context do
let(:protected_branch) { create(:protected_branch, :maintainers_can_push, name: protected_branch_name, project: project) }
run_permission_checks(permissions_matrix)
end
context "when developers are allowed to push into the #{protected_branch_type} protected branch" do
let(:protected_branch) { create(:protected_branch, :developers_can_push, name: protected_branch_name, project: project) }
run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
end
context "developers are allowed to merge into the #{protected_branch_type} protected branch" do
let(:protected_branch) { create(:protected_branch, :developers_can_merge, name: protected_branch_name, project: project) }
context "when a merge request exists for the given source/target branch" do
context "when the merge request is in progress" do
before do
create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'branch/feature',
state: 'locked', in_progress_merge_commit_sha: mergeable_into_protected_branch)
end
run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: true }))
end
context "when the merge request is not in progress" do
before do
create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'branch/feature', in_progress_merge_commit_sha: nil)
end
run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
end
context "when a merge request does not exist for the given source/target branch" do
run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
end
end
end
context "when developers are allowed to push and merge into the #{protected_branch_type} protected branch" do
let(:protected_branch) { create(:protected_branch, :developers_can_merge, :developers_can_push, name: protected_branch_name, project: project) }
run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
end
context "when no one is allowed to push to the #{protected_branch_name} protected branch" do
let(:protected_branch) { build(:protected_branch, :no_one_can_push, name: protected_branch_name, project: project) }
run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false },
maintainer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false },
admin: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false }))
end
end
context 'when pushing to a project' do
let(:project) { create(:hg_git_project, :public, :hg_repository) }
let(:changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow" }
let(:changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/topic/default/wow" }
before do
project.add_developer(user)
......@@ -808,24 +877,32 @@ def self.run_permission_checks(permissions_matrix)
expect { push_access_check }.not_to raise_error
end
it 'avoids N+1 queries', :request_store do
# Run this once to establish a baseline. Cached queries should get
# cached, so that when we introduce another change we shouldn't see
# additional queries.
access.check('unbundle', changes)
control_count = ActiveRecord::QueryRecorder.new do
it 'avoids N+1 queries' do
# the `config.around` (it 'blabla', :request_store') in the
# corresponding GitAccess test wreaks havoc with our setup, with
# even the first "baseline" call being refused (not allowed to push)
# and that's really hard to debug.
# But putting it inside the example as we're doing here solves
# this issue:
Gitlab::WithRequestStore.with_request_store do
# Run this once to establish a baseline. Cached queries should get
# cached, so that when we introduce another change we shouldn't see
# additional queries.
access.check('unbundle', changes)
end
changes = ['6f6d7e7ed 570e7b2ab refs/heads/master', '6f6d7e7ed 570e7b2ab refs/heads/feature']
control_count = ActiveRecord::QueryRecorder.new do
access.check('unbundle', changes)
end
changes = ['6f6d7e7ed 570e7b2ab refs/heads/branch/default', '6f6d7e7ed 570e7b2ab refs/heads/feature']
# There is still an N+1 query with protected branches
expect { access.check('unbundle', changes) }.not_to exceed_query_limit(control_count).with_threshold(2)
# There is still an N+1 query with protected branches
expect { access.check('unbundle', changes) }.not_to exceed_query_limit(control_count).with_threshold(2)
end
end
it 'raises TimeoutError when #check_single_change_access raises a timeout error' do
message = "Push operation timed out\n\nTiming information for debugging purposes:\nRunning checks for ref: wow"
message = "Push operation timed out\n\nTiming information for debugging purposes:\nRunning checks for ref: topic/default/wow"
expect_next_instance_of(Gitlab::Checks::ChangeAccess) do |check|
expect(check).to receive(:validate!).and_raise(Gitlab::Checks::TimedLogger::TimeoutError)
......@@ -1049,7 +1126,6 @@ def access
end
def push_changes(changes)
puts "push_changes #{changes}"
access.check('unbundle', changes)
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