From 495db09653bafb0371e5d5a5f12d5bc33cdb584b Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Thu, 23 Jun 2016 14:58:14 +0530
Subject: [PATCH] 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.
---
app/models/repository.rb | 14 +-
app/services/merge_requests/merge_service.rb | 4 +-
...ress_merge_commit_sha_to_merge_requests.rb | 8 +
db/schema.rb | 1 +
lib/gitlab/checks/matching_merge_request.rb | 18 ++
lib/gitlab/git_access.rb | 7 +
spec/lib/gitlab/diff/position_tracer_spec.rb | 3 +-
spec/lib/gitlab/git_access_spec.rb | 185 +++++++++++-------
spec/models/repository_spec.rb | 9 +-
.../merge_requests/refresh_service_spec.rb | 3 +-
10 files changed, 167 insertions(+), 85 deletions(-)
create mode 100644 db/migrate/20160629025435_add_column_in_progress_merge_commit_sha_to_merge_requests.rb
create mode 100644 lib/gitlab/checks/matching_merge_request.rb
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 5b670cb4b8f..09487b62f98 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -769,9 +769,9 @@ class Repository
end
end
- def merge(user, source_sha, target_branch, options = {})
- our_commit = rugged.branches[target_branch].target
- their_commit = rugged.lookup(source_sha)
+ def merge(user, merge_request, options = {})
+ our_commit = rugged.branches[merge_request.target_branch].target
+ their_commit = rugged.lookup(merge_request.diff_head_sha)
raise "Invalid merge target" if our_commit.nil?
raise "Invalid merge source" if their_commit.nil?
@@ -779,14 +779,16 @@ class Repository
merge_index = rugged.merge_commits(our_commit, their_commit)
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(
parents: [our_commit, their_commit],
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
diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index f1b1d90c457..0dac0614141 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -34,7 +34,7 @@ module MergeRequests
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)
rescue GitHooksService::PreReceiveError => e
merge_request.update(merge_error: e.message)
@@ -43,6 +43,8 @@ module MergeRequests
merge_request.update(merge_error: "Something went wrong during merge")
Rails.logger.error(e.message)
false
+ ensure
+ merge_request.update(in_progress_merge_commit_sha: nil)
end
def after_merge
diff --git a/db/migrate/20160629025435_add_column_in_progress_merge_commit_sha_to_merge_requests.rb b/db/migrate/20160629025435_add_column_in_progress_merge_commit_sha_to_merge_requests.rb
new file mode 100644
index 00000000000..7c5f76572ef
--- /dev/null
+++ b/db/migrate/20160629025435_add_column_in_progress_merge_commit_sha_to_merge_requests.rb
@@ -0,0 +1,8 @@
+# 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
diff --git a/db/schema.rb b/db/schema.rb
index 4562e6bb0c3..64019cf79bb 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -626,6 +626,7 @@ ActiveRecord::Schema.define(version: 20160712171823) do
t.string "merge_commit_sha"
t.datetime "deleted_at"
t.integer "lock_version", default: 0, null: false
+ t.string "in_progress_merge_commit_sha"
end
add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree
diff --git a/lib/gitlab/checks/matching_merge_request.rb b/lib/gitlab/checks/matching_merge_request.rb
new file mode 100644
index 00000000000..849848515da
--- /dev/null
+++ b/lib/gitlab/checks/matching_merge_request.rb
@@ -0,0 +1,18 @@
+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
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index e20e3338262..feaf845209e 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -177,10 +177,15 @@ module Gitlab
Gitlab::ForcePushCheck.force_push?(project, oldrev, newrev)
end
+
def protocol_allowed?
Gitlab::ProtocolAccess.allowed?(protocol)
end
+ def matching_merge_request?(newrev, branch_name)
+ Checks::MatchingMergeRequest.new(newrev, branch_name, project).match?
+ end
+
private
def protected_branch_action(oldrev, newrev, branch_name)
@@ -190,6 +195,8 @@ module Gitlab
elsif Gitlab::Git.blank_ref?(newrev)
# and we dont allow remove of protected branch
: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)
:push_code
else
diff --git a/spec/lib/gitlab/diff/position_tracer_spec.rb b/spec/lib/gitlab/diff/position_tracer_spec.rb
index 08312e60f4a..c268f84c759 100644
--- a/spec/lib/gitlab/diff/position_tracer_spec.rb
+++ b/spec/lib/gitlab/diff/position_tracer_spec.rb
@@ -1639,7 +1639,8 @@ describe Gitlab::Diff::PositionTracer, lib: true do
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)
end
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index b90d9c724f1..50cf2f1e93f 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -181,96 +181,47 @@ describe Gitlab::GitAccess, lib: true do
end
describe 'push_access_check' do
- def protect_feature_branch
- create(:protected_branch, name: 'feature', project: project)
- end
+ before { merge_into_protected_branch }
+ let(:unprotected_branch) { FFaker::Internet.user_name }
- def changes
- {
- push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow",
+ let(:changes) do
+ { push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow",
push_master: '6f6d7e7ed 570e7b2ab refs/heads/master',
push_protected_branch: '6f6d7e7ed 570e7b2ab refs/heads/feature',
push_remove_protected_branch: "570e7b2ab #{Gitlab::Git::BLANK_SHA} "\
'refs/heads/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/feature']
- }
- end
-
- def self.permissions_matrix
- {
- master: {
- push_new_branch: true,
- push_master: true,
- push_protected_branch: true,
- push_remove_protected_branch: false,
- push_tag: true,
- push_new_tag: true,
- push_all: true,
- },
-
- developer: {
- push_new_branch: true,
- push_master: true,
- push_protected_branch: false,
- push_remove_protected_branch: false,
- push_tag: false,
- push_new_tag: true,
- push_all: false,
- },
-
- reporter: {
- push_new_branch: false,
- push_master: false,
- push_protected_branch: false,
- push_remove_protected_branch: false,
- push_tag: false,
- push_new_tag: false,
- push_all: false,
- },
-
- guest: {
- push_new_branch: false,
- push_master: false,
- push_protected_branch: false,
- push_remove_protected_branch: false,
- push_tag: false,
- push_new_tag: false,
- push_all: false,
- }
- }
+ 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 self.updated_permissions_matrix
- updated_permissions_matrix = permissions_matrix.dup
- updated_permissions_matrix[:developer][:push_protected_branch] = true
- updated_permissions_matrix[:developer][:push_all] = true
- updated_permissions_matrix
+ 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
- permissions_matrix.keys.each do |role|
- describe "#{role} access" do
- before { protect_feature_branch }
- before { project.team << [user, role] }
+ 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" }
- 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
+ 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
- context "with enabled developers push to protected branches " do
- updated_permissions_matrix.keys.each do |role|
+ def self.run_permission_checks(permissions_matrix)
+ permissions_matrix.keys.each do |role|
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|
+ permissions_matrix[role].each do |action, allowed|
context action do
subject { access.push_access_check(changes[action]) }
@@ -280,5 +231,97 @@ describe Gitlab::GitAccess, lib: true do
end
end
end
+
+ permissions_matrix = {
+ master: {
+ push_new_branch: true,
+ push_master: true,
+ push_protected_branch: true,
+ push_remove_protected_branch: false,
+ push_tag: true,
+ push_new_tag: true,
+ push_all: true,
+ merge_into_protected_branch: true
+ },
+
+ developer: {
+ push_new_branch: true,
+ push_master: true,
+ push_protected_branch: false,
+ push_remove_protected_branch: false,
+ push_tag: false,
+ push_new_tag: true,
+ push_all: false,
+ merge_into_protected_branch: false
+ },
+
+ reporter: {
+ push_new_branch: false,
+ push_master: false,
+ push_protected_branch: false,
+ push_remove_protected_branch: false,
+ push_tag: false,
+ push_new_tag: false,
+ push_all: false,
+ merge_into_protected_branch: false
+ },
+
+ guest: {
+ push_new_branch: false,
+ push_master: false,
+ push_protected_branch: false,
+ push_remove_protected_branch: false,
+ push_tag: false,
+ push_new_tag: 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
+
+ context "when 'developers can push' is turned on for the #{protected_branch_type} protected branch" do
+ before { create(:protected_branch, name: protected_branch_name, 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
+
+ context "when 'developers can merge' is turned on for the #{protected_branch_type} protected branch" do
+ before { create(:protected_branch, name: protected_branch_name, developers_can_merge: true, 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: 'feature', state: 'locked', in_progress_merge_commit_sha: merge_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: 'feature', in_progress_merge_commit_sha: nil)
+ end
+
+ run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
+ end
+ 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
+
+ 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
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index b39b958450c..e14cec589fe 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -4,16 +4,17 @@ describe Repository, models: true do
include RepoHelpers
TestBlob = Struct.new(:name)
- let(:repository) { create(:project).repository }
+ let(:project) { create(:project) }
+ let(:repository) { project.repository }
let(:user) { create(:user) }
let(:commit_options) do
author = repository.user_to_committer(user)
{ message: 'Test message', committer: author, author: author }
end
let(:merge_commit) do
- source_sha = repository.find_branch('feature').target
- merge_commit_sha = repository.merge(user, source_sha, 'master', commit_options)
- repository.commit(merge_commit_sha)
+ merge_request = create(:merge_request, source_branch: 'feature', target_branch: 'master', source_project: project)
+ merge_commit_id = repository.merge(user, merge_request, commit_options)
+ repository.commit(merge_commit_id)
end
describe '#branch_names_contains' do
diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb
index 06f56d85aa8..ce643b3f860 100644
--- a/spec/services/merge_requests/refresh_service_spec.rb
+++ b/spec/services/merge_requests/refresh_service_spec.rb
@@ -88,8 +88,7 @@ describe MergeRequests::RefreshService, services: true do
# Merge master -> feature branch
author = { email: 'test@gitlab.com', time: Time.now, name: "Me" }
commit_options = { message: 'Test message', committer: author, author: author }
- master_commit = @project.repository.commit('master')
- @project.repository.merge(@user, master_commit.id, 'feature', commit_options)
+ @project.repository.merge(@user, @merge_request, commit_options)
commit = @project.repository.commit('feature')
service.new(@project, @user).execute(@oldrev, commit.id, 'refs/heads/feature')
reload_mrs
--
GitLab