Read about our upcoming Code of Conduct on this issue

Commit 28b2ac62 authored by Georges Racinet's avatar Georges Racinet 🦑
Browse files

Branch acces checks: direct identification of legit merges

Closes #368

To let merges done from the Rails application be checked
against the distinct set of merge rules rather that push
rules, GitLab looks for MRs matching the pre-receive changes by
comparing with the merge commit sha that's always available
(and set by the Repository Model) before the actual branch move.

Because the Mercurial change can be atomic, we have no other
option than passing a special parameter down from
`Repository#merge` through the Mercurial backend, with the
`HEPTAPOD_ACCEPT_MERGE_ID` environment variable.
In turn the Mercurial process will add it to its pre-receive
hook payload, and the target branch can be compared to switch
from (direct) push to (Rails) merge mode.

In the case of merges intended from the very beginning to be
fast forwards, the commit sha is already known in advance,
but we're using the same mechanism for consistency in the
check.

Finally, there was a good dose of wrestling with those RSpec
examples that care about merging to a protected branch, because
of the on-demand nature of instantiation.

--HG--
branch : heptapod-stable
parent 1290d9cfb0b7
Pipeline #12786 skipped with stages
......@@ -891,7 +891,7 @@ def multi_action(user, **options)
def merge(user, source_sha, merge_request, message)
with_cache_hooks do
raw_repository.merge(user, source_sha, merge_request.target_branch, message) do |commit_id|
raw_repository.merge(user, source_sha, merge_request.target_branch, message, for_mr_iid: merge_request&.iid) do |commit_id|
merge_request.update(in_progress_merge_commit_sha: commit_id)
nil # Return value does not matter.
end
......@@ -914,7 +914,7 @@ def ff_merge(user, source, target_branch, merge_request: nil)
merge_request&.update(in_progress_merge_commit_sha: their_commit_id)
with_cache_hooks { raw.ff_merge(user, their_commit_id, target_branch) }
with_cache_hooks { raw.ff_merge(user, their_commit_id, target_branch, for_mr_iid: merge_request&.iid) }
end
def revert(
......
......@@ -28,13 +28,16 @@ def container
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
def access_checker_for(actor, protocol, hg: false)
def access_checker_for(actor, protocol, hg: false, accept_mr_iid: nil)
klass = hg ? hg_access_checker_klass : access_checker_klass
klass.new(actor.key_or_user, container, protocol,
named_args = {
authentication_abilities: ssh_authentication_abilities,
namespace_path: namespace_path,
repository_path: project_path,
redirected_path: redirected_path)
redirected_path: redirected_path
}
named_args[:accept_mr_iid] = accept_mr_iid if hg
klass.new(actor.key_or_user, container, protocol, **named_args)
end
def access_checker_klass
......
......@@ -91,6 +91,7 @@ def check_allowed(params)
def access_check!(actor, params)
access_checker = access_checker_for(actor, params[:protocol],
accept_mr_iid: params[:accept_mr_iid],
hg: params[:vcs] == "hg")
access_checker.check(params[:action], params[:changes]).tap do |result|
break result if @project || !repo_type.project?
......
......@@ -144,7 +144,11 @@ def project_members_url
end
def matching_merge_request?
Checks::MatchingMergeRequest.new(newrev, branch_name, project).match?
if project.mercurial?
branch_name == change_access.target_branch_for_mr
else
Checks::MatchingMergeRequest.new(newrev, branch_name, project).match?
end
end
def forced_push?
......
......@@ -11,7 +11,7 @@ class ChangeAccess
def initialize(
change, user_access:, project:,
skip_lfs_integrity_check: false, protocol:, logger:
skip_lfs_integrity_check: false, protocol:, accept_mr_iid:, logger:
)
@oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref)
@branch_name = Gitlab::Git.branch_name(@ref)
......@@ -20,11 +20,36 @@ def initialize(
@project = project
@skip_lfs_integrity_check = skip_lfs_integrity_check
@protocol = protocol
@accept_mr_iid = accept_mr_iid
@logger = logger
@logger.append_message("Running checks for ref: #{@branch_name || @tag_name}")
end
# If the pre-receive is ultimately due to a merge from Rails,
# return the corresponding MR target branch name.
#
# The proof it comes from Rails is the presence of @accept_mr_iid.
# In other cases, `nil` is returned. On Mercurial that will lead
# to the harsher (direct) push checks running.
def target_branch_for_mr
return if @accept_mr_iid.nil?
# rubocop: disable CodeReuse/ActiveRecord
# gracinet, could not find a direct helper for this in 10 seconds
# this is a very simple call (no perf issue) anyway
mr = project.merge_requests.find_by(iid: @accept_mr_iid)
# rubocop: enable CodeReuse/ActiveRecord
if mr.nil?
Rails.logger.warn(
"Gitlab::Check::ChangeAccess: "\
"no MR !#{@accept_mr_iid} found for project id=#{project.id}")
end
mr&.target_branch
end
def validate!
ref_level_checks
# Check of commits should happen as the last step
......
......@@ -588,13 +588,16 @@ def merge_to_ref(user, source_sha, branch, target_ref, message, first_parent_ref
end
end
def merge(user, source_sha, target_branch, message, &block)
# for_mr_iid is Heptapod specific and used to transmit information,
# ultimately to distinguish merge from direct push, see heptapod#368
def merge(user, source_sha, target_branch, message, for_mr_iid: nil, &block)
wrapped_gitaly_errors do
gitaly_operation_client.user_merge_branch(user, source_sha, target_branch, message, &block)
end
end
def ff_merge(user, source_sha, target_branch)
# about `for_mr_iid`, see #merge
def ff_merge(user, source_sha, target_branch, for_mr_iid: nil, &block)
wrapped_gitaly_errors do
gitaly_operation_client.user_ff_branch(user, source_sha, target_branch)
end
......
......@@ -366,6 +366,7 @@ def check_single_change_access(change, skip_lfs_integrity_check: false)
project: project,
skip_lfs_integrity_check: skip_lfs_integrity_check,
protocol: protocol,
accept_mr_iid: @accept_mr_iid,
logger: logger
).validate!
rescue Checks::TimedLogger::TimeoutError
......
......@@ -28,6 +28,11 @@ class HgAccess < GitAccess
PUSH_COMMANDS = %w{ unbundle pushkey putlfile }.freeze
ALL_COMMANDS = DOWNLOAD_COMMANDS + PUSH_COMMANDS
def initialize(actor, container, protocol, accept_mr_iid: nil, **named_args)
super(actor, container, protocol, **named_args)
@accept_mr_iid = accept_mr_iid
end
# This method is for cases the command is not known upfront
# typically SSH, because it is inside the protocol, hence obscure to
# GitLab components
......
......@@ -427,9 +427,10 @@ def hg_prepare_user_encoding(hg_env, user, message)
#
# The given `user` information will be passed over to Mercurial
# in the usual environment variables
def hg_changeset_publish(user, hgsha, notify_gitlab: false)
def hg_changeset_publish(user, hgsha, notify_gitlab: false, for_mr_iid: nil)
env = hg_env_for_write(user)
env.delete('HEPTAPOD_SKIP_ALL_GITLAB_HOOKS') if notify_gitlab && !unit_tests_skip_hooks
env['HEPTAPOD_ACCEPT_MR_IID'] = for_mr_iid.to_s unless for_mr_iid.nil?
out, status = popen([Gitlab.config.mercurial.bin_path,
'phase', '--public', '-r', hgsha],
@hgpath, env)
......@@ -559,7 +560,7 @@ def forbid_merge_in_topic(target_branch)
raise ArgumentError, "Merge Requests targeting topics aren't supported yet" unless target_topic.nil?
end
def merge(user, source_sha, target_branch, message)
def merge(user, source_sha, target_branch, message, for_mr_iid: nil)
logprefix = "hg_merge for #{@relative_path} "\
"source_sha=#{source_sha} target_branch=#{target_branch}"
logger = Rails.logger # could need to be adapted for gitaly-ruby (see initialize())
......@@ -595,7 +596,7 @@ def merge(user, source_sha, target_branch, message)
logger.info("#{logprefix} publishing changeset #{result_hgsha}")
begin
hg_changeset_publish(user, result_hgsha, notify_gitlab: true)
hg_changeset_publish(user, result_hgsha, notify_gitlab: true, for_mr_iid: for_mr_iid)
rescue HgError => e
raise MergeError, e.message
end
......@@ -660,7 +661,7 @@ def can_be_merged?(source_sha, target_branch)
end
end
def ff_merge(user, source_sha, target_branch)
def ff_merge(user, source_sha, target_branch, for_mr_iid: nil)
forbid_merge_in_topic(target_branch)
source_hgsha = hg_sha_map[source_sha]
......@@ -680,7 +681,8 @@ def ff_merge(user, source_sha, target_branch)
end
begin
hg_changeset_publish(user, source_hgsha, notify_gitlab: true)
hg_changeset_publish(user, source_hgsha, notify_gitlab: true,
for_mr_iid: for_mr_iid)
rescue HgError => e
raise MergeError, e
end
......
......@@ -668,17 +668,28 @@ def disable_protocol(protocol)
mergeable_into_protected_branch
end
let(:mr_iid) { nil }
# last element of each list is the accept_mr_iid
let(:changes) do
{ any: Gitlab::GitAccess::ANY,
push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow",
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 #{mergeable_into_protected_branch} refs/heads/branch/feature" }
{ any: [Gitlab::GitAccess::ANY, nil],
push_new_branch: ["#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow",
nil],
push_master: ['6f6d7e7ed 570e7b2ab refs/heads/branch/default', nil],
push_protected_branch: [
'6f6d7e7ed 570e7b2ab refs/heads/branch/feature', nil
],
push_remove_protected_branch: ["570e7b2ab #{Gitlab::Git::BLANK_SHA} "\
'refs/heads/branch/feature', nil],
push_tag: ['6f6d7e7ed 570e7b2ab refs/tags/v1.0.0', nil],
push_new_tag: [
"#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/tags/v7.8.9", nil],
push_all: ['6f6d7e7ed 570e7b2ab refs/heads/master',
'6f6d7e7ed 570e7b2ab refs/heads/branch/feature',
nil],
merge_into_protected_branch: [
"0b4bc9a cafe1234 refs/heads/branch/feature", mr_iid
] }
end
# GR rename as prepare_mergeable_into_protected_branch ?
......@@ -821,11 +832,7 @@ def self.run_permission_checks(permissions_matrix)
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
let(:mr_iid) { create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'branch/feature').iid }
run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: true }))
end
......@@ -1118,15 +1125,16 @@ def self.run_permission_checks(permissions_matrix)
private
def access
def access(accept_mr_iid: nil)
described_class.new(actor, project, protocol,
authentication_abilities: authentication_abilities,
namespace_path: namespace_path, repository_path: project_path,
redirected_path: redirected_path, auth_result_type: auth_result_type)
redirected_path: redirected_path, auth_result_type: auth_result_type, accept_mr_iid: accept_mr_iid)
end
def push_changes(changes)
access.check('unbundle', changes)
mr_iid = changes.pop
access(accept_mr_iid: mr_iid).check('unbundle', changes)
end
def raise_forbidden(message)
......
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