diff --git a/CHANGELOG b/CHANGELOG index f1dcc40a27326a6aa32154ab65ba92b67aa63f8b..e144ba8513fbab7e7a0227302402e994ed95dce5 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -497,6 +497,7 @@ v 8.7.7 - Prevent unauthorized access to other projects build traces - Forbid scripting for wiki files - Only show notes through JSON on confidential issues that the user has access to + - Added protected branch setting that allows Developers to accept merge requests while push is still disallowed. !4220 (Mathias Vestergaard) v 8.7.6 - Fix links on wiki pages for relative url setups. !4131 (Artem Sidorenko) diff --git a/app/assets/javascripts/protected_branches.js.coffee b/app/assets/javascripts/protected_branches.js.coffee index 79c2306e4d25de078caaa95f8d001708fac40d1a..ce2fc883620bbf9218d8d5b918a879c6606b1ae0 100644 --- a/app/assets/javascripts/protected_branches.js.coffee +++ b/app/assets/javascripts/protected_branches.js.coffee @@ -1,9 +1,11 @@ $ -> $(".protected-branches-list :checkbox").change (e) -> name = $(this).attr("name") - if name == "developers_can_push" + row = $(this).parents("tr") + if name == "developers_can_push" || name == "developers_can_merge" id = $(this).val() - checked = $(this).is(":checked") + can_push = row.find("input[name=developers_can_push]").is(":checked") + can_merge = row.find("input[name=developers_can_merge]").is(":checked") url = $(this).data("url") $.ajax type: "PUT" @@ -12,7 +14,8 @@ $ -> data: id: id protected_branch: - developers_can_push: checked + developers_can_push: can_push + developers_can_merge: can_merge success: -> row = $(e.target) diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb index 80dad758afa298d229abad2aaafc3a33b251b39c..10dca47fdede1c49b774663d3a28c6f35899c48e 100644 --- a/app/controllers/projects/protected_branches_controller.rb +++ b/app/controllers/projects/protected_branches_controller.rb @@ -50,6 +50,6 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController end def protected_branch_params - params.require(:protected_branch).permit(:name, :developers_can_push) + params.require(:protected_branch).permit(:name, :developers_can_push, :developers_can_merge) end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 157901378d32167ee1e84a90e3ec46e826b8d354..23d68706527ce1592a6f2dc32dcf83a7bc07b311 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -552,7 +552,8 @@ class MergeRequest < ActiveRecord::Base end def can_be_merged_by?(user) - ::Gitlab::GitAccess.new(user, project, 'web').can_push_to_branch?(target_branch) + access = ::Gitlab::GitAccess.new(user, project, 'web') + access.can_push_to_branch?(target_branch) || access.can_merge_to_branch?(target_branch) end def mergeable_ci_state? diff --git a/app/models/project.rb b/app/models/project.rb index a66b750cd48633aaf67b6e6143091799fb3698bc..2bd97fc48f1d66e712155d1ddfae8f2a8bfa6073 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -832,6 +832,10 @@ class Project < ActiveRecord::Base protected_branches.matching(branch_name).any?(&:developers_can_push) end + def developers_can_merge_to_protected_branch?(branch_name) + protected_branches.matching(branch_name).any?(&:developers_can_merge) + end + def forked? !(forked_project_link.nil? || forked_project_link.forked_from_project.nil?) end diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index a886f35981f42c8c21d05b35c6ae1fc6900392a9..e02b50ff9a2d8440bc1ffdc582699f70439e0ca8 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -89,7 +89,8 @@ class GitPushService < BaseService # Set protection on the default branch if configured if current_application_settings.default_branch_protection != PROTECTION_NONE developers_can_push = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? true : false - @project.protected_branches.create({ name: @project.default_branch, developers_can_push: developers_can_push }) + 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, developers_can_merge: developers_can_merge }) end end diff --git a/app/views/projects/protected_branches/_branches_list.html.haml b/app/views/projects/protected_branches/_branches_list.html.haml index 97cb1a9052b497b1c9ede989fbbb9e54307fee5a..181d1a27c735c637a0e52620142aab5969c1738f 100644 --- a/app/views/projects/protected_branches/_branches_list.html.haml +++ b/app/views/projects/protected_branches/_branches_list.html.haml @@ -8,6 +8,7 @@ .table-responsive %table.table.protected-branches-list %colgroup + %col{ width: "27%" } %col{ width: "30%" } %col{ width: "25%" } %col{ width: "25%" } @@ -18,6 +19,7 @@ %th Protected Branch %th Commit %th Developers Can Push + %th Developers can Merge - if can_admin_project %th %tbody diff --git a/app/views/projects/protected_branches/_protected_branch.html.haml b/app/views/projects/protected_branches/_protected_branch.html.haml index 474aec3a97cfa19474fbb242c1fcd588c4680c95..7fda7f96047342065fd51532bb16c33b4e873e01 100644 --- a/app/views/projects/protected_branches/_protected_branch.html.haml +++ b/app/views/projects/protected_branches/_protected_branch.html.haml @@ -16,6 +16,8 @@ (branch was removed from repository) %td = 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 %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" diff --git a/app/views/projects/protected_branches/index.html.haml b/app/views/projects/protected_branches/index.html.haml index 3fab95751e071bc46982aa16b9e3c70e9aeabe24..101b3f3b452084a231eeb69c4f7a8aa9e7a4468f 100644 --- a/app/views/projects/protected_branches/index.html.haml +++ b/app/views/projects/protected_branches/index.html.haml @@ -36,6 +36,14 @@ = f.label :developers_can_push, "Developers can push", class: "label-light append-bottom-0" %p.light.append-bottom-0 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 + %hr = render "branches_list" diff --git a/db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb b/db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb new file mode 100644 index 0000000000000000000000000000000000000000..15ad8e8bcbb6b42184b5a807e9915344c960d53f --- /dev/null +++ b/db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb @@ -0,0 +1,9 @@ +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 diff --git a/db/schema.rb b/db/schema.rb index f24e47b85b29830d8f890fa1e93af4b3d64308c6..4562e6bb0c3395ea024888f7e94feb222bde033a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -860,11 +860,12 @@ ActiveRecord::Schema.define(version: 20160712171823) do add_index "projects", ["visibility_level"], name: "index_projects_on_visibility_level", using: :btree create_table "protected_branches", force: :cascade do |t| - t.integer "project_id", null: false - t.string "name", null: false + t.integer "project_id", null: false + t.string "name", null: false t.datetime "created_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 add_index "protected_branches", ["project_id"], name: "index_protected_branches_on_project_id", using: :btree diff --git a/lib/gitlab/access.rb b/lib/gitlab/access.rb index 831f1e635baca56b041c8c4e1f0c0de61eb3fb47..de41ea415a67d17ab461dde421c7a6fc551d7649 100644 --- a/lib/gitlab/access.rb +++ b/lib/gitlab/access.rb @@ -14,9 +14,10 @@ module Gitlab OWNER = 50 # Branch protection settings - PROTECTION_NONE = 0 - PROTECTION_DEV_CAN_PUSH = 1 - PROTECTION_FULL = 2 + PROTECTION_NONE = 0 + PROTECTION_DEV_CAN_PUSH = 1 + PROTECTION_FULL = 2 + PROTECTION_DEV_CAN_MERGE = 3 class << self def values @@ -54,6 +55,7 @@ module Gitlab def protection_options { "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, "Fully protected: Developers cannot push new commits, force push, or delete the branch. Only masters can do any of those." => PROTECTION_FULL, } diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 7679c7e4bb8e4192918417c4859214213150406b..e20e33382622df468cde01f1519b926c3f9794f0 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -39,6 +39,16 @@ module Gitlab end end + def can_merge_to_branch?(ref) + return false unless user + + if project.protected_branch?(ref) && !project.developers_can_merge_to_protected_branch?(ref) + user.can?(:push_code_to_protected_branches, project) + else + user.can?(:push_code, project) + end + end + def can_read_project? if user user.can?(:read_project, project) diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index c79ba11f782dbe0e946e483a41db568908f47be3..b90d9c724f13049a0391e6d73826e6a8e67b1882 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -65,6 +65,27 @@ describe Gitlab::GitAccess, lib: true do expect(access.can_push_to_branch?(@branch.name)).to be_falsey end end + + describe 'merge to protected branch if allowed for developers' do + before do + @branch = create :protected_branch, project: project, developers_can_merge: true + end + + it "returns true if user is a master" do + project.team << [user, :master] + expect(access.can_merge_to_branch?(@branch.name)).to be_truthy + end + + it "returns true if user is a developer" do + project.team << [user, :developer] + expect(access.can_merge_to_branch?(@branch.name)).to be_truthy + end + + it "returns false if user is a reporter" do + project.team << [user, :reporter] + expect(access.can_merge_to_branch?(@branch.name)).to be_falsey + end + end end describe '#check with single protocols allowed' do diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index afabeed4a80f095df92c7efff04047189acca865..0f30a84a2cfbb54a311bc5f050b7bb85dbc9403d 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -224,7 +224,7 @@ describe GitPushService, services: true do it "when pushing a branch for the first time" do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") - expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false }) + expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false, developers_can_merge: false }) execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) end @@ -242,7 +242,16 @@ describe GitPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") - expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: true }) + expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: true, developers_can_merge: false }) + execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) + end + + it "when pushing a branch for the first time with default branch protection set to 'developers can merge'" do + stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE) + + expect(project).to receive(:execute_hooks) + expect(project.default_branch).to eq("master") + expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false, developers_can_merge: true }) execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) end