Commit 495db096 authored by Timothy Andrew's avatar Timothy Andrew

Enforce "developers can merge" during `pre-receive`.

1. When a merge request is being merged, save the merge commit SHA in
   the `in_progress_merge_commit_sha` database column.

2. The `pre-receive` hook looks for any locked (in progress) merge
   request with `in_progress_merge_commit_sha` matching the `newrev` it
   is passed.

3. If it finds a matching MR, the merge is legitimate.

4. Update `git_access_spec` to test the behaviour we added here. Also
   refactored this spec a bit to make it easier to add more contexts / conditions.
parent f0577d83
...@@ -769,9 +769,9 @@ class Repository ...@@ -769,9 +769,9 @@ class Repository
end end
end end
def merge(user, source_sha, target_branch, options = {}) def merge(user, merge_request, options = {})
our_commit = rugged.branches[target_branch].target our_commit = rugged.branches[merge_request.target_branch].target
their_commit = rugged.lookup(source_sha) their_commit = rugged.lookup(merge_request.diff_head_sha)
raise "Invalid merge target" if our_commit.nil? raise "Invalid merge target" if our_commit.nil?
raise "Invalid merge source" if their_commit.nil? raise "Invalid merge source" if their_commit.nil?
...@@ -779,14 +779,16 @@ class Repository ...@@ -779,14 +779,16 @@ class Repository
merge_index = rugged.merge_commits(our_commit, their_commit) merge_index = rugged.merge_commits(our_commit, their_commit)
return false if merge_index.conflicts? return false if merge_index.conflicts?
commit_with_hooks(user, target_branch) do |ref| commit_with_hooks(user, merge_request.target_branch) do |tmp_ref|
actual_options = options.merge( actual_options = options.merge(
parents: [our_commit, their_commit], parents: [our_commit, their_commit],
tree: merge_index.write_tree(rugged), tree: merge_index.write_tree(rugged),
update_ref: ref update_ref: tmp_ref
) )
Rugged::Commit.create(rugged, actual_options) commit_id = Rugged::Commit.create(rugged, actual_options)
merge_request.update(in_progress_merge_commit_sha: commit_id)
commit_id
end end
end end
......
...@@ -34,7 +34,7 @@ module MergeRequests ...@@ -34,7 +34,7 @@ module MergeRequests
committer: committer committer: committer
} }
commit_id = repository.merge(current_user, merge_request.diff_head_sha, merge_request.target_branch, options) commit_id = repository.merge(current_user, merge_request, options)
merge_request.update(merge_commit_sha: commit_id) merge_request.update(merge_commit_sha: commit_id)
rescue GitHooksService::PreReceiveError => e rescue GitHooksService::PreReceiveError => e
merge_request.update(merge_error: e.message) merge_request.update(merge_error: e.message)
...@@ -43,6 +43,8 @@ module MergeRequests ...@@ -43,6 +43,8 @@ module MergeRequests
merge_request.update(merge_error: "Something went wrong during merge") merge_request.update(merge_error: "Something went wrong during merge")
Rails.logger.error(e.message) Rails.logger.error(e.message)
false false
ensure
merge_request.update(in_progress_merge_commit_sha: nil)
end end
def after_merge def after_merge
......
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddColumnInProgressMergeCommitShaToMergeRequests < ActiveRecord::Migration
def change
add_column :merge_requests, :in_progress_merge_commit_sha, :string
end
end
...@@ -626,6 +626,7 @@ ActiveRecord::Schema.define(version: 20160712171823) do ...@@ -626,6 +626,7 @@ ActiveRecord::Schema.define(version: 20160712171823) do
t.string "merge_commit_sha" t.string "merge_commit_sha"
t.datetime "deleted_at" t.datetime "deleted_at"
t.integer "lock_version", default: 0, null: false t.integer "lock_version", default: 0, null: false
t.string "in_progress_merge_commit_sha"
end end
add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree
......
module Gitlab
module Checks
class MatchingMergeRequest
def initialize(newrev, branch_name, project)
@newrev = newrev
@branch_name = branch_name
@project = project
end
def match?
@project.merge_requests
.with_state(:locked)
.where(in_progress_merge_commit_sha: @newrev, target_branch: @branch_name)
.exists?
end
end
end
end
...@@ -177,10 +177,15 @@ module Gitlab ...@@ -177,10 +177,15 @@ module Gitlab
Gitlab::ForcePushCheck.force_push?(project, oldrev, newrev) Gitlab::ForcePushCheck.force_push?(project, oldrev, newrev)
end end
def protocol_allowed? def protocol_allowed?
Gitlab::ProtocolAccess.allowed?(protocol) Gitlab::ProtocolAccess.allowed?(protocol)
end end
def matching_merge_request?(newrev, branch_name)
Checks::MatchingMergeRequest.new(newrev, branch_name, project).match?
end
private private
def protected_branch_action(oldrev, newrev, branch_name) def protected_branch_action(oldrev, newrev, branch_name)
...@@ -190,6 +195,8 @@ module Gitlab ...@@ -190,6 +195,8 @@ module Gitlab
elsif Gitlab::Git.blank_ref?(newrev) elsif Gitlab::Git.blank_ref?(newrev)
# and we dont allow remove of protected branch # and we dont allow remove of protected branch
:remove_protected_branches :remove_protected_branches
elsif matching_merge_request?(newrev, branch_name) && project.developers_can_merge_to_protected_branch?(branch_name)
:push_code
elsif project.developers_can_push_to_protected_branch?(branch_name) elsif project.developers_can_push_to_protected_branch?(branch_name)
:push_code :push_code
else else
......
...@@ -1639,7 +1639,8 @@ describe Gitlab::Diff::PositionTracer, lib: true do ...@@ -1639,7 +1639,8 @@ describe Gitlab::Diff::PositionTracer, lib: true do
committer: committer committer: committer
} }
repository.merge(current_user, second_create_file_commit.sha, branch_name, options) merge_request = create(:merge_request, source_branch: second_create_file_commit.sha, target_branch: branch_name, source_project: project)
repository.merge(current_user, merge_request, options)
project.commit(branch_name) project.commit(branch_name)
end end
......
...@@ -181,25 +181,58 @@ describe Gitlab::GitAccess, lib: true do ...@@ -181,25 +181,58 @@ describe Gitlab::GitAccess, lib: true do
end end
describe 'push_access_check' do describe 'push_access_check' do
def protect_feature_branch before { merge_into_protected_branch }
create(:protected_branch, name: 'feature', project: project) let(:unprotected_branch) { FFaker::Internet.user_name }
end
def changes let(:changes) do
{ { push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow",
push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow",
push_master: '6f6d7e7ed 570e7b2ab refs/heads/master', push_master: '6f6d7e7ed 570e7b2ab refs/heads/master',
push_protected_branch: '6f6d7e7ed 570e7b2ab refs/heads/feature', push_protected_branch: '6f6d7e7ed 570e7b2ab refs/heads/feature',
push_remove_protected_branch: "570e7b2ab #{Gitlab::Git::BLANK_SHA} "\ push_remove_protected_branch: "570e7b2ab #{Gitlab::Git::BLANK_SHA} "\
'refs/heads/feature', 'refs/heads/feature',
push_tag: '6f6d7e7ed 570e7b2ab refs/tags/v1.0.0', push_tag: '6f6d7e7ed 570e7b2ab refs/tags/v1.0.0',
push_new_tag: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/tags/v7.8.9", push_new_tag: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/tags/v7.8.9",
push_all: ['6f6d7e7ed 570e7b2ab refs/heads/master', '6f6d7e7ed 570e7b2ab refs/heads/feature'] push_all: ['6f6d7e7ed 570e7b2ab refs/heads/master', '6f6d7e7ed 570e7b2ab refs/heads/feature'],
} merge_into_protected_branch: "0b4bc9a #{merge_into_protected_branch} refs/heads/feature" }
end
def stub_git_hooks
# Running the `pre-receive` hook is expensive, and not necessary for this test.
allow_any_instance_of(GitHooksService).to receive(:execute).and_yield
end end
def self.permissions_matrix def merge_into_protected_branch
{ @protected_branch_merge_commit ||= begin
stub_git_hooks
project.repository.add_branch(user, unprotected_branch, 'feature')
target_branch = project.repository.lookup('feature')
source_branch = project.repository.commit_file(user, FFaker::InternetSE.login_user_name, FFaker::HipsterIpsum.paragraph, FFaker::HipsterIpsum.sentence, unprotected_branch, false)
rugged = project.repository.rugged
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
end
def self.run_permission_checks(permissions_matrix)
permissions_matrix.keys.each do |role|
describe "#{role} access" do
before { project.team << [user, role] }
permissions_matrix[role].each do |action, allowed|
context action do
subject { access.push_access_check(changes[action]) }
it { expect(subject.allowed?).to allowed ? be_truthy : be_falsey }
end
end
end
end
end
permissions_matrix = {
master: { master: {
push_new_branch: true, push_new_branch: true,
push_master: true, push_master: true,
...@@ -208,6 +241,7 @@ describe Gitlab::GitAccess, lib: true do ...@@ -208,6 +241,7 @@ describe Gitlab::GitAccess, lib: true do
push_tag: true, push_tag: true,
push_new_tag: true, push_new_tag: true,
push_all: true, push_all: true,
merge_into_protected_branch: true
}, },
developer: { developer: {
...@@ -218,6 +252,7 @@ describe Gitlab::GitAccess, lib: true do ...@@ -218,6 +252,7 @@ describe Gitlab::GitAccess, lib: true do
push_tag: false, push_tag: false,
push_new_tag: true, push_new_tag: true,
push_all: false, push_all: false,
merge_into_protected_branch: false
}, },
reporter: { reporter: {
...@@ -228,6 +263,7 @@ describe Gitlab::GitAccess, lib: true do ...@@ -228,6 +263,7 @@ describe Gitlab::GitAccess, lib: true do
push_tag: false, push_tag: false,
push_new_tag: false, push_new_tag: false,
push_all: false, push_all: false,
merge_into_protected_branch: false
}, },
guest: { guest: {
...@@ -238,46 +274,53 @@ describe Gitlab::GitAccess, lib: true do ...@@ -238,46 +274,53 @@ describe Gitlab::GitAccess, lib: true do
push_tag: false, push_tag: false,
push_new_tag: false, push_new_tag: false,
push_all: false, push_all: false,
merge_into_protected_branch: false
} }
} }
[['feature', 'exact'], ['feat*', 'wildcard']].each do |protected_branch_name, protected_branch_type|
context do
before { create(:protected_branch, name: protected_branch_name, project: project) }
run_permission_checks(permissions_matrix)
end end
def self.updated_permissions_matrix context "when 'developers can push' is turned on for the #{protected_branch_type} protected branch" do
updated_permissions_matrix = permissions_matrix.dup before { create(:protected_branch, name: protected_branch_name, developers_can_push: true, project: project) }
updated_permissions_matrix[:developer][:push_protected_branch] = true
updated_permissions_matrix[:developer][:push_all] = true run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
updated_permissions_matrix
end end
permissions_matrix.keys.each do |role| context "when 'developers can merge' is turned on for the #{protected_branch_type} protected branch" do
describe "#{role} access" do before { create(:protected_branch, name: protected_branch_name, developers_can_merge: true, project: project) }
before { protect_feature_branch }
before { project.team << [user, role] }
permissions_matrix[role].each do |action, allowed| context "when a merge request exists for the given source/target branch" do
context action do context "when the merge request is in progress" do
subject { access.push_access_check(changes[action]) } before do
create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch)
end
it { expect(subject.allowed?).to allowed ? be_truthy : be_falsey } run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: true }))
end 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: 'feature', in_progress_merge_commit_sha: nil)
end end
run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
end end
end end
context "with enabled developers push to protected branches " do context "when a merge request does not exist for the given source/target branch" do
updated_permissions_matrix.keys.each do |role| run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
describe "#{role} access" do
before { create(:protected_branch, name: 'feature', developers_can_push: true, project: project) }
before { project.team << [user, role] }
updated_permissions_matrix[role].each do |action, allowed|
context action do
subject { access.push_access_check(changes[action]) }
it { expect(subject.allowed?).to allowed ? be_truthy : be_falsey }
end
end end
end end
context "when 'developers can merge' and 'developers can push' are turned on for the #{protected_branch_type} protected branch" do
before { create(:protected_branch, name: protected_branch_name, developers_can_merge: true, developers_can_push: true, project: project) }
run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
end end
end end
end end
......
...@@ -4,16 +4,17 @@ describe Repository, models: true do ...@@ -4,16 +4,17 @@ describe Repository, models: true do
include RepoHelpers include RepoHelpers
TestBlob = Struct.new(:name) TestBlob = Struct.new(:name)
let(:repository) { create(:project).repository } let(:project) { create(:project) }
let(:repository) { project.repository }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:commit_options) do let(:commit_options) do
author = repository.user_to_committer(user) author = repository.user_to_committer(user)
{ message: 'Test message', committer: author, author: author } { message: 'Test message', committer: author, author: author }
end end
let(:merge_commit) do let(:merge_commit) do
source_sha = repository.find_branch('feature').target merge_request = create(:merge_request, source_branch: 'feature', target_branch: 'master', source_project: project)
merge_commit_sha = repository.merge(user, source_sha, 'master', commit_options) merge_commit_id = repository.merge(user, merge_request, commit_options)
repository.commit(merge_commit_sha) repository.commit(merge_commit_id)
end end
describe '#branch_names_contains' do describe '#branch_names_contains' do
......
...@@ -88,8 +88,7 @@ describe MergeRequests::RefreshService, services: true do ...@@ -88,8 +88,7 @@ describe MergeRequests::RefreshService, services: true do
# Merge master -> feature branch # Merge master -> feature branch
author = { email: 'test@gitlab.com', time: Time.now, name: "Me" } author = { email: 'test@gitlab.com', time: Time.now, name: "Me" }
commit_options = { message: 'Test message', committer: author, author: author } commit_options = { message: 'Test message', committer: author, author: author }
master_commit = @project.repository.commit('master') @project.repository.merge(@user, @merge_request, commit_options)
@project.repository.merge(@user, master_commit.id, 'feature', commit_options)
commit = @project.repository.commit('feature') commit = @project.repository.commit('feature')
service.new(@project, @user).execute(@oldrev, commit.id, 'refs/heads/feature') service.new(@project, @user).execute(@oldrev, commit.id, 'refs/heads/feature')
reload_mrs reload_mrs
......
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