Commit 530f5158 authored by Robert Speicher's avatar Robert Speicher

Revert "Merge branch '18193-developers-can-merge' into 'master'

This reverts commit 9ca633eb, reversing
changes made to fb229bbf.
parent 9b0ef155
...@@ -44,7 +44,6 @@ v 8.10.0 (unreleased) ...@@ -44,7 +44,6 @@ v 8.10.0 (unreleased)
- Add "Enabled Git access protocols" to Application Settings - Add "Enabled Git access protocols" to Application Settings
- Diffs will create button/diff form on demand no on server side - Diffs will create button/diff form on demand no on server side
- Reduce size of HTML used by diff comment forms - Reduce size of HTML used by diff comment forms
- Protected branches have a "Developers can Merge" setting. !4892 (original implementation by Mathias Vestergaard)
- Fix user creation with stronger minimum password requirements !4054 (nathan-pmt) - Fix user creation with stronger minimum password requirements !4054 (nathan-pmt)
- Only show New Snippet button to users that can create snippets. - Only show New Snippet button to users that can create snippets.
- PipelinesFinder uses git cache data - PipelinesFinder uses git cache data
......
$ -> $ ->
$(".protected-branches-list :checkbox").change (e) -> $(".protected-branches-list :checkbox").change (e) ->
name = $(this).attr("name") name = $(this).attr("name")
if name == "developers_can_push" || name == "developers_can_merge" if name == "developers_can_push"
id = $(this).val() id = $(this).val()
can_push = $(this).is(":checked") checked = $(this).is(":checked")
url = $(this).data("url") url = $(this).data("url")
$.ajax $.ajax
type: "PATCH" type: "PUT"
url: url url: url
dataType: "json" dataType: "json"
data: data:
id: id id: id
protected_branch: protected_branch:
"#{name}": can_push developers_can_push: checked
success: -> success: ->
row = $(e.target) row = $(e.target)
......
...@@ -50,6 +50,6 @@ def load_protected_branch ...@@ -50,6 +50,6 @@ def load_protected_branch
end end
def protected_branch_params def protected_branch_params
params.require(:protected_branch).permit(:name, :developers_can_push, :developers_can_merge) params.require(:protected_branch).permit(:name, :developers_can_push)
end end
end end
...@@ -12,7 +12,7 @@ def can_remove_branch?(project, branch_name) ...@@ -12,7 +12,7 @@ def can_remove_branch?(project, branch_name)
def can_push_branch?(project, branch_name) def can_push_branch?(project, branch_name)
return false unless project.repository.branch_exists?(branch_name) return false unless project.repository.branch_exists?(branch_name)
::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(branch_name) ::Gitlab::GitAccess.new(current_user, project, 'web').can_push_to_branch?(branch_name)
end end
def project_branches def project_branches
......
...@@ -552,13 +552,7 @@ def broken? ...@@ -552,13 +552,7 @@ def broken?
end end
def can_be_merged_by?(user) def can_be_merged_by?(user)
access = ::Gitlab::UserAccess.new(user, project: project) ::Gitlab::GitAccess.new(user, project, 'web').can_push_to_branch?(target_branch)
access.can_push_to_branch?(target_branch) || access.can_merge_to_branch?(target_branch)
end
def can_be_merged_via_command_line_by?(user)
access = ::Gitlab::UserAccess.new(user, project: project)
access.can_push_to_branch?(target_branch)
end end
def mergeable_ci_state? def mergeable_ci_state?
......
...@@ -832,10 +832,6 @@ def developers_can_push_to_protected_branch?(branch_name) ...@@ -832,10 +832,6 @@ def developers_can_push_to_protected_branch?(branch_name)
protected_branches.matching(branch_name).any?(&:developers_can_push) protected_branches.matching(branch_name).any?(&:developers_can_push)
end end
def developers_can_merge_to_protected_branch?(branch_name)
protected_branches.matching(branch_name).any?(&:developers_can_merge)
end
def forked? def forked?
!(forked_project_link.nil? || forked_project_link.forked_from_project.nil?) !(forked_project_link.nil? || forked_project_link.forked_from_project.nil?)
end end
......
...@@ -769,9 +769,9 @@ def can_be_merged?(source_sha, target_branch) ...@@ -769,9 +769,9 @@ def can_be_merged?(source_sha, target_branch)
end end
end end
def merge(user, merge_request, options = {}) def merge(user, source_sha, target_branch, options = {})
our_commit = rugged.branches[merge_request.target_branch].target our_commit = rugged.branches[target_branch].target
their_commit = rugged.lookup(merge_request.diff_head_sha) their_commit = rugged.lookup(source_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,16 +779,14 @@ def merge(user, merge_request, options = {}) ...@@ -779,16 +779,14 @@ def merge(user, merge_request, options = {})
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, merge_request.target_branch) do |tmp_ref| commit_with_hooks(user, target_branch) do |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: tmp_ref update_ref: ref
) )
commit_id = Rugged::Commit.create(rugged, actual_options) Rugged::Commit.create(rugged, actual_options)
merge_request.update(in_progress_merge_commit_sha: commit_id)
commit_id
end end
end end
......
...@@ -23,7 +23,7 @@ def commit ...@@ -23,7 +23,7 @@ def commit
private private
def check_push_permissions def check_push_permissions
allowed = ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(@target_branch) allowed = ::Gitlab::GitAccess.new(current_user, project, 'web').can_push_to_branch?(@target_branch)
unless allowed unless allowed
raise ValidationError.new('You are not allowed to push into this branch') raise ValidationError.new('You are not allowed to push into this branch')
...@@ -31,7 +31,7 @@ def check_push_permissions ...@@ -31,7 +31,7 @@ def check_push_permissions
true true
end end
def create_target_branch(new_branch) def create_target_branch(new_branch)
# Temporary branch exists and contains the change commit # Temporary branch exists and contains the change commit
return success if repository.find_branch(new_branch) return success if repository.find_branch(new_branch)
......
...@@ -42,7 +42,7 @@ def raise_error(message) ...@@ -42,7 +42,7 @@ def raise_error(message)
end end
def validate def validate
allowed = ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(@target_branch) allowed = ::Gitlab::GitAccess.new(current_user, project, 'web').can_push_to_branch?(@target_branch)
unless allowed unless allowed
raise_error("You are not allowed to push into this branch") raise_error("You are not allowed to push into this branch")
......
...@@ -89,8 +89,7 @@ def process_default_branch ...@@ -89,8 +89,7 @@ def process_default_branch
# Set protection on the default branch if configured # Set protection on the default branch if configured
if current_application_settings.default_branch_protection != PROTECTION_NONE if current_application_settings.default_branch_protection != PROTECTION_NONE
developers_can_push = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? true : false developers_can_push = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? true : false
developers_can_merge = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_MERGE ? true : false @project.protected_branches.create({ name: @project.default_branch, developers_can_push: developers_can_push })
@project.protected_branches.create({ name: @project.default_branch, developers_can_push: developers_can_push, developers_can_merge: developers_can_merge })
end end
end end
......
...@@ -34,7 +34,7 @@ def commit ...@@ -34,7 +34,7 @@ def commit
committer: committer committer: committer
} }
commit_id = repository.merge(current_user, merge_request, options) commit_id = repository.merge(current_user, merge_request.diff_head_sha, merge_request.target_branch, 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,8 +43,6 @@ def commit ...@@ -43,8 +43,6 @@ def commit
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
......
...@@ -48,7 +48,7 @@ def close_merge_requests ...@@ -48,7 +48,7 @@ def close_merge_requests
end end
def force_push? def force_push?
Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev) Gitlab::ForcePushCheck.force_push?(@project, @oldrev, @newrev)
end end
# Refresh merge request diff if we push to source or target branch of merge request # Refresh merge request diff if we push to source or target branch of merge request
......
...@@ -4,7 +4,7 @@ ...@@ -4,7 +4,7 @@
%p %p
Please resolve these conflicts or Please resolve these conflicts or
- if @merge_request.can_be_merged_via_command_line_by?(current_user) - if @merge_request.can_be_merged_by?(current_user)
#{link_to "merge this request manually", "#modal_merge_info", class: "how_to_merge_link vlink", "data-toggle" => "modal"}. #{link_to "merge this request manually", "#modal_merge_info", class: "how_to_merge_link vlink", "data-toggle" => "modal"}.
- else - else
ask someone with write access to this repository to merge this request manually. ask someone with write access to this repository to merge this request manually.
...@@ -8,7 +8,6 @@ ...@@ -8,7 +8,6 @@
.table-responsive .table-responsive
%table.table.protected-branches-list %table.table.protected-branches-list
%colgroup %colgroup
%col{ width: "20%" }
%col{ width: "30%" } %col{ width: "30%" }
%col{ width: "25%" } %col{ width: "25%" }
%col{ width: "25%" } %col{ width: "25%" }
...@@ -19,7 +18,6 @@ ...@@ -19,7 +18,6 @@
%th Protected Branch %th Protected Branch
%th Commit %th Commit
%th Developers Can Push %th Developers Can Push
%th Developers Can Merge
- if can_admin_project - if can_admin_project
%th %th
%tbody %tbody
......
...@@ -16,8 +16,6 @@ ...@@ -16,8 +16,6 @@
(branch was removed from repository) (branch was removed from repository)
%td %td
= check_box_tag("developers_can_push", protected_branch.id, protected_branch.developers_can_push, data: { url: url }) = check_box_tag("developers_can_push", protected_branch.id, protected_branch.developers_can_push, data: { url: url })
%td
= check_box_tag("developers_can_merge", protected_branch.id, protected_branch.developers_can_merge, data: { url: url })
- if can_admin_project - if can_admin_project
%td %td
= link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, protected_branch], data: { confirm: 'Branch will be writable for developers. Are you sure?' }, method: :delete, class: "btn btn-warning btn-sm pull-right" = link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, protected_branch], data: { confirm: 'Branch will be writable for developers. Are you sure?' }, method: :delete, class: "btn btn-warning btn-sm pull-right"
...@@ -36,14 +36,6 @@ ...@@ -36,14 +36,6 @@
= f.label :developers_can_push, "Developers can push", class: "label-light append-bottom-0" = f.label :developers_can_push, "Developers can push", class: "label-light append-bottom-0"
%p.light.append-bottom-0 %p.light.append-bottom-0
Allow developers to push to this branch Allow developers to push to this branch
.form-group
= f.check_box :developers_can_merge, class: "pull-left"
.prepend-left-20
= f.label :developers_can_merge, "Developers can merge", class: "label-light append-bottom-0"
%p.light.append-bottom-0
Allow developers to accept merge requests to this branch
= f.submit "Protect", class: "btn-create btn protect-branch-btn", disabled: true = f.submit "Protect", class: "btn-create btn protect-branch-btn", disabled: true
%hr %hr
= render "branches_list" = render "branches_list"
class AddDevelopersCanMergeToProtectedBranches < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
disable_ddl_transaction!
def change
add_column_with_default :protected_branches, :developers_can_merge, :boolean, default: false, allow_null: false
end
end
# 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,7 +626,6 @@ ...@@ -626,7 +626,6 @@
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
...@@ -861,12 +860,11 @@ ...@@ -861,12 +860,11 @@
add_index "projects", ["visibility_level"], name: "index_projects_on_visibility_level", using: :btree add_index "projects", ["visibility_level"], name: "index_projects_on_visibility_level", using: :btree
create_table "protected_branches", force: :cascade do |t| create_table "protected_branches", force: :cascade do |t|
t.integer "project_id", null: false t.integer "project_id", null: false
t.string "name", null: false t.string "name", null: false
t.datetime "created_at" t.datetime "created_at"
t.datetime "updated_at" t.datetime "updated_at"
t.boolean "developers_can_push", default: false, null: false t.boolean "developers_can_push", default: false, null: false
t.boolean "developers_can_merge", default: false, null: false
end end
add_index "protected_branches", ["project_id"], name: "index_protected_branches_on_project_id", using: :btree add_index "protected_branches", ["project_id"], name: "index_protected_branches_on_project_id", using: :btree
......
...@@ -17,7 +17,7 @@ def find_user_by_private_token ...@@ -17,7 +17,7 @@ def find_user_by_private_token
def current_user def current_user
@current_user ||= (find_user_by_private_token || doorkeeper_guard) @current_user ||= (find_user_by_private_token || doorkeeper_guard)
unless @current_user && Gitlab::UserAccess.new(@current_user).allowed? unless @current_user && Gitlab::UserAccess.allowed?(@current_user)
return nil return nil
end end
......
...@@ -14,10 +14,9 @@ class AccessDeniedError < StandardError; end ...@@ -14,10 +14,9 @@ class AccessDeniedError < StandardError; end
OWNER = 50 OWNER = 50
# Branch protection settings # Branch protection settings
PROTECTION_NONE = 0 PROTECTION_NONE = 0
PROTECTION_DEV_CAN_PUSH = 1 PROTECTION_DEV_CAN_PUSH = 1
PROTECTION_FULL = 2 PROTECTION_FULL = 2
PROTECTION_DEV_CAN_MERGE = 3
class << self class << self
def values def values
...@@ -55,7 +54,6 @@ def sym_options ...@@ -55,7 +54,6 @@ def sym_options
def protection_options def protection_options
{ {
"Not protected: Both developers and masters can push new commits, force push, or delete the branch." => PROTECTION_NONE, "Not protected: Both developers and masters can push new commits, force push, or delete the branch." => PROTECTION_NONE,
"Protected against pushes: Developers cannot push new commits, but are allowed to accept merge requests to the branch." => PROTECTION_DEV_CAN_MERGE,
"Partially protected: Developers can push new commits, but cannot force push or delete the branch. Masters can do all of those." => PROTECTION_DEV_CAN_PUSH, "Partially protected: Developers can push new commits, but cannot force push or delete the branch. Masters can do all of those." => PROTECTION_DEV_CAN_PUSH,
"Fully protected: Developers cannot push new commits, force push, or delete the branch. Only masters can do any of those." => PROTECTION_FULL, "Fully protected: Developers cannot push new commits, force push, or delete the branch. Only masters can do any of those." => PROTECTION_FULL,
} }
......
module Gitlab
module Checks
class ChangeAccess
attr_reader :user_access, :project
def initialize(change, user_access:, project:)
@oldrev, @newrev, @ref = change.split(' ')
@branch_name = branch_name(@ref)
@user_access = user_access
@project = project
end
def exec
error = protected_branch_checks || tag_checks || push_checks
if error
GitAccessStatus.new(false, error)
else
GitAccessStatus.new(true)
end
end
protected
def protected_branch_checks
return unless project.protected_branch?(@branch_name)
if forced_push? && user_access.cannot_do_action?(:force_push_code_to_protected_branches)
return "You are not allowed to force push code to a protected branch on this project."
elsif Gitlab::Git.blank_ref?(@newrev) && user_access.cannot_do_action?(:remove_protected_branches)
return "You are not allowed to delete protected branches from this project."
end
if matching_merge_request?
if user_access.can_merge_to_branch?(@branch_name) || user_access.can_push_to_branch?(@branch_name)
return
else
"You are not allowed to merge code into protected branches on this project."
end
else
if user_access.can_push_to_branch?(@branch_name)
return
else
"You are not allowed to push code to protected branches on this project."
end
end
end
def tag_checks
tag_ref = tag_name(@ref)
if tag_ref && protected_tag?(tag_ref) && user_access.cannot_do_action?(:admin_project)
"You are not allowed to change existing tags on this project."
end
end
def push_checks
if user_access.cannot_do_action?(:push_code)
"You are not allowed to push code to this project."
end
end
private
def protected_tag?(tag_name)
project.repository.tag_exists?(tag_name)
end
def forced_push?
Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev)
end
def matching_merge_request?
Checks::MatchingMergeRequest.new(@newrev, @branch_name, @project).match?
end
def branch_name(ref)
ref = @ref.to_s
if Gitlab::Git.branch_ref?(ref)
Gitlab::Git.ref_name(ref)
else
nil
end
end
def tag_name(ref)
ref = @ref.to_s
if Gitlab::Git.tag_ref?(ref)
Gitlab::Git.ref_name(ref)
else
nil
end
end
end
end
end
module Gitlab
module Checks
class ForcePush
def self.force_push?(project, oldrev, newrev)
return false if project.empty_repo?
# Created or deleted branch
if Gitlab::Git.blank_ref?(oldrev) || Gitlab::Git.blank_ref?(newrev)
false
else
missed_refs, _ = Gitlab::Popen.popen(%W(#{Gitlab.config.git.bin_path} --git-dir=#{project.repository.path_to_repo} rev-list #{oldrev} ^#{newrev}))
missed_refs.split("\n").size > 0
end
end
end
end
end
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
module Gitlab
class ForcePushCheck
def self.force_push?(project, oldrev, newrev)
return false if project.empty_repo?
# Created or deleted branch
if Gitlab::Git.blank_ref?(oldrev) || Gitlab::Git.blank_ref?(newrev)
false
else
missed_refs, _ = Gitlab::Popen.popen(%W(#{Gitlab.config.git.bin_path} --git-dir=#{project.repository.path_to_repo} rev-list #{oldrev} ^#{newrev}))
missed_refs.split("\n").size > 0
end
end
end
end
# Check a user's access to perform a git action. All public methods in this
# class return an instance of `GitlabAccessStatus`
module Gitlab module Gitlab
class GitAccess class GitAccess
DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive }