From f0577d838544152f558411ef1101d56c5852d92e Mon Sep 17 00:00:00 2001 From: Mathias Vestergaard Date: Fri, 20 May 2016 01:58:34 +0200 Subject: Added "developers can merge" setting to protected branches - Cherry-picked from `mvestergaard:branch-protection-dev-merge` - https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4220 --- CHANGELOG | 1 + app/assets/javascripts/protected_branches.js.coffee | 9 ++++++--- .../projects/protected_branches_controller.rb | 2 +- app/models/merge_request.rb | 3 ++- app/models/project.rb | 4 ++++ app/services/git_push_service.rb | 3 ++- .../protected_branches/_branches_list.html.haml | 2 ++ .../protected_branches/_protected_branch.html.haml | 2 ++ .../projects/protected_branches/index.html.haml | 8 ++++++++ ...dd_developers_can_merge_to_protected_branches.rb | 9 +++++++++ db/schema.rb | 7 ++++--- lib/gitlab/access.rb | 8 +++++--- lib/gitlab/git_access.rb | 10 ++++++++++ spec/lib/gitlab/git_access_spec.rb | 21 +++++++++++++++++++++ spec/services/git_push_service_spec.rb | 13 +++++++++++-- 15 files changed, 88 insertions(+), 14 deletions(-) create mode 100644 db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb diff --git a/CHANGELOG b/CHANGELOG index f1dcc40a273..e144ba8513f 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 79c2306e4d2..ce2fc883620 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 80dad758afa..10dca47fded 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 157901378d3..23d68706527 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 a66b750cd48..2bd97fc48f1 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 a886f35981f..e02b50ff9a2 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 97cb1a9052b..181d1a27c73 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 474aec3a97c..7fda7f96047 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 3fab95751e0..101b3f3b452 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 00000000000..15ad8e8bcbb --- /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 f24e47b85b2..4562e6bb0c3 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 831f1e635ba..de41ea415a6 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 7679c7e4bb8..e20e3338262 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 c79ba11f782..b90d9c724f1 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 afabeed4a80..0f30a84a2cf 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 -- cgit v1.2.1 From 495db09653bafb0371e5d5a5f12d5bc33cdb584b Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 23 Jun 2016 14:58:14 +0530 Subject: 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 +- ..._progress_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 -- cgit v1.2.1 From 60245bbe228014a9f689adafd64b571883ef6eb3 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Fri, 24 Jun 2016 10:37:04 +0530 Subject: Refactor `Gitlab::GitAccess` 1. Don't use case statements for dispatch anymore. This leads to a lot of duplication, and makes the logic harder to follow. 2. Remove duplicated logic. - For example, the `can_push_to_branch?` exists, but we also have a different way of checking the same condition within `change_access_check`. - This kind of duplication is removed, and the `can_push_to_branch?` method is used in both places. 3. Move checks returning true/false to `UserAccess`. - All public methods in `GitAccess` now return an instance of `GitAccessStatus`. Previously, some methods would return true/false as well, which was confusing. - It makes sense for these kinds of checks to be at the level of a user, so the `UserAccess` class was repurposed for this. The prior `UserAccess.allowed?` classmethod is converted into an instance method. - All external uses of these checks have been migrated to use the `UserAccess` class 4. Move the "change_access_check" into a separate class. - Create the `GitAccess::ChangeAccessCheck` class to run these checks, which are quite substantial. - `ChangeAccessCheck` returns an instance of `GitAccessStatus` as well. 5. Break out the boolean logic in `ChangeAccessCheck` into `if/else` chains - this seems more readable. 6. I can understand that this might look like overkill for !4892, but I think this is a good opportunity to clean it up. - http://martinfowler.com/bliki/OpportunisticRefactoring.html --- app/helpers/branches_helper.rb | 2 +- app/models/merge_request.rb | 2 +- app/services/commits/change_service.rb | 4 +- app/services/files/base_service.rb | 2 +- lib/api/helpers.rb | 2 +- lib/gitlab/git_access.rb | 159 +++++---------------------- lib/gitlab/git_access/change_access_check.rb | 96 ++++++++++++++++ lib/gitlab/git_access_wiki.rb | 2 +- lib/gitlab/user_access.rb | 48 +++++++- spec/lib/gitlab/git_access_spec.rb | 82 -------------- spec/lib/gitlab/user_access_spec.rb | 90 +++++++++++++++ spec/requests/api/api_helpers_spec.rb | 4 +- 12 files changed, 270 insertions(+), 223 deletions(-) create mode 100644 lib/gitlab/git_access/change_access_check.rb create mode 100644 spec/lib/gitlab/user_access_spec.rb diff --git a/app/helpers/branches_helper.rb b/app/helpers/branches_helper.rb index 601df5c18df..bfd23aa4e04 100644 --- a/app/helpers/branches_helper.rb +++ b/app/helpers/branches_helper.rb @@ -12,7 +12,7 @@ module BranchesHelper def can_push_branch?(project, branch_name) return false unless project.repository.branch_exists?(branch_name) - ::Gitlab::GitAccess.new(current_user, project, 'web').can_push_to_branch?(branch_name) + ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(branch_name) end def project_branches diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 23d68706527..e5853bdd589 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -552,7 +552,7 @@ class MergeRequest < ActiveRecord::Base end def can_be_merged_by?(user) - access = ::Gitlab::GitAccess.new(user, project, 'web') + access = ::Gitlab::UserAccess.new(user, project: project) access.can_push_to_branch?(target_branch) || access.can_merge_to_branch?(target_branch) end diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb index c578097376a..ed73d8cb8c2 100644 --- a/app/services/commits/change_service.rb +++ b/app/services/commits/change_service.rb @@ -23,7 +23,7 @@ module Commits private def check_push_permissions - allowed = ::Gitlab::GitAccess.new(current_user, project, 'web').can_push_to_branch?(@target_branch) + allowed = ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(@target_branch) unless allowed raise ValidationError.new('You are not allowed to push into this branch') @@ -31,7 +31,7 @@ module Commits true end - + def create_target_branch(new_branch) # Temporary branch exists and contains the change commit return success if repository.find_branch(new_branch) diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb index 37c5e321b39..55da949f56a 100644 --- a/app/services/files/base_service.rb +++ b/app/services/files/base_service.rb @@ -42,7 +42,7 @@ module Files end def validate - allowed = ::Gitlab::GitAccess.new(current_user, project, 'web').can_push_to_branch?(@target_branch) + allowed = ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(@target_branch) unless allowed raise_error("You are not allowed to push into this branch") diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 77e407b54c5..73557cf7db6 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -17,7 +17,7 @@ module API def current_user @current_user ||= (find_user_by_private_token || doorkeeper_guard) - unless @current_user && Gitlab::UserAccess.allowed?(@current_user) + unless @current_user && Gitlab::UserAccess.new(@current_user).allowed? return nil end diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index feaf845209e..cfb48ac2382 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -1,62 +1,17 @@ +# Check a user's access to perform a git action. All public methods in this +# class return an instance of `GitlabAccessStatus` module Gitlab class GitAccess DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive } PUSH_COMMANDS = %w{ git-receive-pack } - attr_reader :actor, :project, :protocol + attr_reader :actor, :project, :protocol, :user_access def initialize(actor, project, protocol) @actor = actor @project = project @protocol = protocol - end - - def user - return @user if defined?(@user) - - @user = - case actor - when User - actor - when DeployKey - nil - when Key - actor.user - end - end - - def deploy_key - actor if actor.is_a?(DeployKey) - end - - def can_push_to_branch?(ref) - return false unless user - - if project.protected_branch?(ref) && !project.developers_can_push_to_protected_branch?(ref) - user.can?(:push_code_to_protected_branches, project) - else - user.can?(:push_code, project) - 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) - elsif deploy_key - deploy_key.projects.include?(project) - else - false - end + @user_access = UserAccess.new(user, project: project) end def check(cmd, changes = nil) @@ -66,11 +21,11 @@ module Gitlab return build_status_object(false, "No user or key was provided.") end - if user && !user_allowed? + if user && !user_access.allowed? return build_status_object(false, "Your account has been blocked.") end - unless project && can_read_project? + unless project && (user_access.can_read_project? || deploy_key_can_read_project?) return build_status_object(false, 'The project you were looking for could not be found.') end @@ -105,7 +60,7 @@ module Gitlab end def user_download_access_check - unless user.can?(:download_code, project) + unless user_access.can_do_action?(:download_code) return build_status_object(false, "You are not allowed to download code from this project.") end @@ -135,103 +90,49 @@ module Gitlab build_status_object(true) end - def can_user_do_action?(action) - @permission_cache ||= {} - @permission_cache[action] ||= user.can?(action, project) - end - def change_access_check(change) - oldrev, newrev, ref = change.split(' ') - - action = - if project.protected_branch?(branch_name(ref)) - protected_branch_action(oldrev, newrev, branch_name(ref)) - elsif (tag_ref = tag_name(ref)) && protected_tag?(tag_ref) - # Prevent any changes to existing git tag unless user has permissions - :admin_project - else - :push_code - end - - unless can_user_do_action?(action) - status = - case action - when :force_push_code_to_protected_branches - build_status_object(false, "You are not allowed to force push code to a protected branch on this project.") - when :remove_protected_branches - build_status_object(false, "You are not allowed to deleted protected branches from this project.") - when :push_code_to_protected_branches - build_status_object(false, "You are not allowed to push code to protected branches on this project.") - when :admin_project - build_status_object(false, "You are not allowed to change existing tags on this project.") - else # :push_code - build_status_object(false, "You are not allowed to push code to this project.") - end - return status - end - - build_status_object(true) - end - - def forced_push?(oldrev, newrev) - Gitlab::ForcePushCheck.force_push?(project, oldrev, newrev) + ChangeAccessCheck.new(change, user_access: user_access, project: project).exec 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) - # we dont allow force push to protected branch - if forced_push?(oldrev, newrev) - :force_push_code_to_protected_branches - 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 - :push_code_to_protected_branches - end - end - - def protected_tag?(tag_name) - project.repository.tag_exists?(tag_name) + def matching_merge_request?(newrev, branch_name) + Checks::MatchingMergeRequest.new(newrev, branch_name, project).match? end - def user_allowed? - Gitlab::UserAccess.allowed?(user) + def deploy_key + actor if actor.is_a?(DeployKey) 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) + def deploy_key_can_read_project? + if deploy_key + deploy_key.projects.include?(project) else - nil + false end end protected + def user + return @user if defined?(@user) + + @user = + case actor + when User + actor + when DeployKey + nil + when Key + actor.user + end + end + def build_status_object(status, message = '') GitAccessStatus.new(status, message) end diff --git a/lib/gitlab/git_access/change_access_check.rb b/lib/gitlab/git_access/change_access_check.rb new file mode 100644 index 00000000000..9268da4ec6e --- /dev/null +++ b/lib/gitlab/git_access/change_access_check.rb @@ -0,0 +1,96 @@ +module Gitlab + class GitAccess + class ChangeAccessCheck + 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) + + 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 deleted 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 + if (tag_ref = tag_name(@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::ForcePushCheck.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 diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb index 8672cbc0ec4..f71d3575909 100644 --- a/lib/gitlab/git_access_wiki.rb +++ b/lib/gitlab/git_access_wiki.rb @@ -1,7 +1,7 @@ module Gitlab class GitAccessWiki < GitAccess def change_access_check(change) - if user.can?(:create_wiki, project) + if user_access.can_do_action?(:create_wiki) build_status_object(true) else build_status_object(false, "You are not allowed to write to this project's wiki.") diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index d1b42c1f9b9..c0f85e9b3a8 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -1,7 +1,23 @@ module Gitlab - module UserAccess - def self.allowed?(user) - return false if user.blocked? + class UserAccess + attr_reader :user, :project + + def initialize(user, project: nil) + @user = user + @project = project + end + + def can_do_action?(action) + @permission_cache ||= {} + @permission_cache[action] ||= user.can?(action, project) + end + + def cannot_do_action?(action) + !can_do_action?(action) + end + + def allowed? + return false if user.blank? || user.blocked? if user.requires_ldap_check? && user.try_obtain_ldap_lease return false unless Gitlab::LDAP::Access.allowed?(user) @@ -9,5 +25,31 @@ module Gitlab true end + + def can_push_to_branch?(ref) + return false unless user + + if project.protected_branch?(ref) && !project.developers_can_push_to_protected_branch?(ref) + user.can?(:push_code_to_protected_branches, project) + else + user.can?(:push_code, project) + 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? + return false unless user + + user.can?(:read_project, project) + end end end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 50cf2f1e93f..059d8a2c355 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -6,88 +6,6 @@ describe Gitlab::GitAccess, lib: true do let(:user) { create(:user) } let(:actor) { user } - describe 'can_push_to_branch?' do - describe 'push to none protected branch' do - it "returns true if user is a master" do - project.team << [user, :master] - expect(access.can_push_to_branch?("random_branch")).to be_truthy - end - - it "returns true if user is a developer" do - project.team << [user, :developer] - expect(access.can_push_to_branch?("random_branch")).to be_truthy - end - - it "returns false if user is a reporter" do - project.team << [user, :reporter] - expect(access.can_push_to_branch?("random_branch")).to be_falsey - end - end - - describe 'push to protected branch' do - before do - @branch = create :protected_branch, project: project - end - - it "returns true if user is a master" do - project.team << [user, :master] - expect(access.can_push_to_branch?(@branch.name)).to be_truthy - end - - it "returns false if user is a developer" do - project.team << [user, :developer] - expect(access.can_push_to_branch?(@branch.name)).to be_falsey - end - - it "returns false if user is a reporter" do - project.team << [user, :reporter] - expect(access.can_push_to_branch?(@branch.name)).to be_falsey - end - end - - describe 'push to protected branch if allowed for developers' do - before do - @branch = create :protected_branch, project: project, developers_can_push: true - end - - it "returns true if user is a master" do - project.team << [user, :master] - expect(access.can_push_to_branch?(@branch.name)).to be_truthy - end - - it "returns true if user is a developer" do - project.team << [user, :developer] - expect(access.can_push_to_branch?(@branch.name)).to be_truthy - end - - it "returns false if user is a reporter" do - project.team << [user, :reporter] - 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 def disable_protocol(protocol) settings = ::ApplicationSetting.create_from_defaults diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb new file mode 100644 index 00000000000..c77b746cc88 --- /dev/null +++ b/spec/lib/gitlab/user_access_spec.rb @@ -0,0 +1,90 @@ +require 'spec_helper' + +describe Gitlab::UserAccess, lib: true do + let(:access) { Gitlab::UserAccess.new(user, project: project) } + let(:project) { create(:project) } + let(:user) { create(:user) } + + describe 'can_push_to_branch?' do + describe 'push to none protected branch' do + it "returns true if user is a master" do + project.team << [user, :master] + expect(access.can_push_to_branch?("random_branch")).to be_truthy + end + + it "returns true if user is a developer" do + project.team << [user, :developer] + expect(access.can_push_to_branch?("random_branch")).to be_truthy + end + + it "returns false if user is a reporter" do + project.team << [user, :reporter] + expect(access.can_push_to_branch?("random_branch")).to be_falsey + end + end + + describe 'push to protected branch' do + before do + @branch = create :protected_branch, project: project + end + + it "returns true if user is a master" do + project.team << [user, :master] + expect(access.can_push_to_branch?(@branch.name)).to be_truthy + end + + it "returns false if user is a developer" do + project.team << [user, :developer] + expect(access.can_push_to_branch?(@branch.name)).to be_falsey + end + + it "returns false if user is a reporter" do + project.team << [user, :reporter] + expect(access.can_push_to_branch?(@branch.name)).to be_falsey + end + end + + describe 'push to protected branch if allowed for developers' do + before do + @branch = create :protected_branch, project: project, developers_can_push: true + end + + it "returns true if user is a master" do + project.team << [user, :master] + expect(access.can_push_to_branch?(@branch.name)).to be_truthy + end + + it "returns true if user is a developer" do + project.team << [user, :developer] + expect(access.can_push_to_branch?(@branch.name)).to be_truthy + end + + it "returns false if user is a reporter" do + project.team << [user, :reporter] + 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 +end diff --git a/spec/requests/api/api_helpers_spec.rb b/spec/requests/api/api_helpers_spec.rb index 83025953889..3d5c19aeff3 100644 --- a/spec/requests/api/api_helpers_spec.rb +++ b/spec/requests/api/api_helpers_spec.rb @@ -49,7 +49,7 @@ describe API::Helpers, api: true do it "should return nil for a user without access" do env[API::Helpers::PRIVATE_TOKEN_HEADER] = user.private_token - allow(Gitlab::UserAccess).to receive(:allowed?).and_return(false) + allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false) expect(current_user).to be_nil end @@ -73,7 +73,7 @@ describe API::Helpers, api: true do it "should return nil for a user without access" do env[API::Helpers::PRIVATE_TOKEN_HEADER] = personal_access_token.token - allow(Gitlab::UserAccess).to receive(:allowed?).and_return(false) + allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false) expect(current_user).to be_nil end -- cgit v1.2.1 From 959d63ab1a5559f5f60ba7378a59e2d0fdb17c73 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Fri, 24 Jun 2016 11:13:02 +0530 Subject: Clean up `protected_branches.js.coffee` - Only send a param for the currently changed checkbox. - Have the controller use strong parameters correctly, so that the PATCH works as expected. --- app/assets/javascripts/protected_branches.js.coffee | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/protected_branches.js.coffee b/app/assets/javascripts/protected_branches.js.coffee index ce2fc883620..14afef2e2ee 100644 --- a/app/assets/javascripts/protected_branches.js.coffee +++ b/app/assets/javascripts/protected_branches.js.coffee @@ -1,21 +1,18 @@ $ -> $(".protected-branches-list :checkbox").change (e) -> name = $(this).attr("name") - row = $(this).parents("tr") if name == "developers_can_push" || name == "developers_can_merge" id = $(this).val() - can_push = row.find("input[name=developers_can_push]").is(":checked") - can_merge = row.find("input[name=developers_can_merge]").is(":checked") + can_push = $(this).is(":checked") url = $(this).data("url") $.ajax - type: "PUT" + type: "PATCH" url: url dataType: "json" data: id: id protected_branch: - developers_can_push: can_push - developers_can_merge: can_merge + "#{name}": can_push success: -> row = $(e.target) -- cgit v1.2.1 From af7e75162efe07e9fcb1186462e56bd2325018de Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 4 Jul 2016 11:06:32 +0530 Subject: Don't ask the user to "merge this request manually". 1. If they are a developer with "Developers can Merge" switched on. --- app/models/merge_request.rb | 5 +++++ app/views/projects/merge_requests/widget/open/_conflicts.html.haml | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index e5853bdd589..471e32f3b60 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -556,6 +556,11 @@ class MergeRequest < ActiveRecord::Base 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 + def mergeable_ci_state? return true unless project.only_allow_merge_if_build_succeeds? diff --git a/app/views/projects/merge_requests/widget/open/_conflicts.html.haml b/app/views/projects/merge_requests/widget/open/_conflicts.html.haml index 06ab0a3fa00..f000cc38a65 100644 --- a/app/views/projects/merge_requests/widget/open/_conflicts.html.haml +++ b/app/views/projects/merge_requests/widget/open/_conflicts.html.haml @@ -4,7 +4,7 @@ %p Please resolve these conflicts or - - if @merge_request.can_be_merged_by?(current_user) + - if @merge_request.can_be_merged_via_command_line_by?(current_user) #{link_to "merge this request manually", "#modal_merge_info", class: "how_to_merge_link vlink", "data-toggle" => "modal"}. - else ask someone with write access to this repository to merge this request manually. -- cgit v1.2.1 From dffdc7b27c1f89e4e26b4714834e4f147c656206 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 4 Jul 2016 11:07:37 +0530 Subject: Add "Developers can Merge" (to protected branches) to the CHANGELOG. --- CHANGELOG | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index e144ba8513f..b0d86e4acad 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -43,6 +43,7 @@ v 8.10.0 (unreleased) - Add "Enabled Git access protocols" to Application Settings - Diffs will create button/diff form on demand no on server side - 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) - Only show New Snippet button to users that can create snippets. - PipelinesFinder uses git cache data @@ -497,7 +498,6 @@ 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) -- cgit v1.2.1 From 4d00ed21ebbc9fd4a1f1b13cbed9a0a9ad2a2a9e Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 5 Jul 2016 08:55:46 +0530 Subject: Appease rubocop. --- lib/gitlab/git_access.rb | 1 - spec/lib/gitlab/git_access_spec.rb | 1 - 2 files changed, 2 deletions(-) diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index cfb48ac2382..9f5bb9d62cd 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -108,7 +108,6 @@ module Gitlab actor if actor.is_a?(DeployKey) end - def deploy_key_can_read_project? if deploy_key deploy_key.projects.include?(project) diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 059d8a2c355..db33c7a22bb 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -128,7 +128,6 @@ describe Gitlab::GitAccess, lib: true do 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 -- cgit v1.2.1 From ea9e8f4609f46f9c5713a8346f91dc19d310c2e1 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 7 Jul 2016 11:36:01 +0530 Subject: Move all "checks" under `GitLab::Checks`. - https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4892#note_12892160 - This is more consistent. --- app/services/merge_requests/refresh_service.rb | 2 +- lib/gitlab/checks/change_access.rb | 94 +++++++++++++++++++++++++ lib/gitlab/checks/force_push.rb | 17 +++++ lib/gitlab/force_push_check.rb | 15 ---- lib/gitlab/git_access.rb | 2 +- lib/gitlab/git_access/change_access_check.rb | 96 -------------------------- 6 files changed, 113 insertions(+), 113 deletions(-) create mode 100644 lib/gitlab/checks/change_access.rb create mode 100644 lib/gitlab/checks/force_push.rb delete mode 100644 lib/gitlab/force_push_check.rb delete mode 100644 lib/gitlab/git_access/change_access_check.rb diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index b11ecd97a57..1daf6bbf553 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -48,7 +48,7 @@ module MergeRequests end def force_push? - Gitlab::ForcePushCheck.force_push?(@project, @oldrev, @newrev) + Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev) end # Refresh merge request diff if we push to source or target branch of merge request diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb new file mode 100644 index 00000000000..cb8f4f2cbdd --- /dev/null +++ b/lib/gitlab/checks/change_access.rb @@ -0,0 +1,94 @@ +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 deleted 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 + if (tag_ref = tag_name(@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 diff --git a/lib/gitlab/checks/force_push.rb b/lib/gitlab/checks/force_push.rb new file mode 100644 index 00000000000..dfa83a0eab3 --- /dev/null +++ b/lib/gitlab/checks/force_push.rb @@ -0,0 +1,17 @@ +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 diff --git a/lib/gitlab/force_push_check.rb b/lib/gitlab/force_push_check.rb deleted file mode 100644 index 93c6a5bb7f5..00000000000 --- a/lib/gitlab/force_push_check.rb +++ /dev/null @@ -1,15 +0,0 @@ -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 diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 9f5bb9d62cd..308f23bc9bc 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -91,7 +91,7 @@ module Gitlab end def change_access_check(change) - ChangeAccessCheck.new(change, user_access: user_access, project: project).exec + Checks::ChangeAccess.new(change, user_access: user_access, project: project).exec end def protocol_allowed? diff --git a/lib/gitlab/git_access/change_access_check.rb b/lib/gitlab/git_access/change_access_check.rb deleted file mode 100644 index 9268da4ec6e..00000000000 --- a/lib/gitlab/git_access/change_access_check.rb +++ /dev/null @@ -1,96 +0,0 @@ -module Gitlab - class GitAccess - class ChangeAccessCheck - 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) - - 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 deleted 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 - if (tag_ref = tag_name(@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::ForcePushCheck.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 -- cgit v1.2.1 From bb81f2afc1d82d6e88d7039025c8a8be0794aac6 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 12 Jul 2016 10:47:36 +0530 Subject: Implement last round of review comments from !4892. 1. Fix typos, minor styling errors. 2. Use single quotes rather than double quotes in `user_access_spec`. 3. Test formatting. --- .../protected_branches/_branches_list.html.haml | 4 +-- lib/gitlab/checks/change_access.rb | 6 ++-- spec/lib/gitlab/user_access_spec.rb | 40 ++++++++++------------ spec/services/git_push_service_spec.rb | 3 +- 4 files changed, 27 insertions(+), 26 deletions(-) diff --git a/app/views/projects/protected_branches/_branches_list.html.haml b/app/views/projects/protected_branches/_branches_list.html.haml index 181d1a27c73..720d67dff7c 100644 --- a/app/views/projects/protected_branches/_branches_list.html.haml +++ b/app/views/projects/protected_branches/_branches_list.html.haml @@ -8,7 +8,7 @@ .table-responsive %table.table.protected-branches-list %colgroup - %col{ width: "27%" } + %col{ width: "20%" } %col{ width: "30%" } %col{ width: "25%" } %col{ width: "25%" } @@ -19,7 +19,7 @@ %th Protected Branch %th Commit %th Developers Can Push - %th Developers can Merge + %th Developers Can Merge - if can_admin_project %th %tbody diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb index cb8f4f2cbdd..5551fac4b8b 100644 --- a/lib/gitlab/checks/change_access.rb +++ b/lib/gitlab/checks/change_access.rb @@ -28,7 +28,7 @@ module Gitlab 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 deleted protected branches from this project." + return "You are not allowed to delete protected branches from this project." end if matching_merge_request? @@ -47,7 +47,9 @@ module Gitlab end def tag_checks - if (tag_ref = tag_name(@ref)) && protected_tag?(tag_ref) && user_access.cannot_do_action?(:admin_project) + 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 diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb index c77b746cc88..aa9ec243498 100644 --- a/spec/lib/gitlab/user_access_spec.rb +++ b/spec/lib/gitlab/user_access_spec.rb @@ -7,40 +7,38 @@ describe Gitlab::UserAccess, lib: true do describe 'can_push_to_branch?' do describe 'push to none protected branch' do - it "returns true if user is a master" do + it 'returns true if user is a master' do project.team << [user, :master] - expect(access.can_push_to_branch?("random_branch")).to be_truthy + expect(access.can_push_to_branch?('random_branch')).to be_truthy end - it "returns true if user is a developer" do + it 'returns true if user is a developer' do project.team << [user, :developer] - expect(access.can_push_to_branch?("random_branch")).to be_truthy + expect(access.can_push_to_branch?('random_branch')).to be_truthy end - it "returns false if user is a reporter" do + it 'returns false if user is a reporter' do project.team << [user, :reporter] - expect(access.can_push_to_branch?("random_branch")).to be_falsey + expect(access.can_push_to_branch?('random_branch')).to be_falsey end end describe 'push to protected branch' do - before do - @branch = create :protected_branch, project: project - end + let(:branch) { create :protected_branch, project: project } - it "returns true if user is a master" do + it 'returns true if user is a master' do project.team << [user, :master] - expect(access.can_push_to_branch?(@branch.name)).to be_truthy + expect(access.can_push_to_branch?(branch.name)).to be_truthy end - it "returns false if user is a developer" do + it 'returns false if user is a developer' do project.team << [user, :developer] - expect(access.can_push_to_branch?(@branch.name)).to be_falsey + expect(access.can_push_to_branch?(branch.name)).to be_falsey end - it "returns false if user is a reporter" do + it 'returns false if user is a reporter' do project.team << [user, :reporter] - expect(access.can_push_to_branch?(@branch.name)).to be_falsey + expect(access.can_push_to_branch?(branch.name)).to be_falsey end end @@ -49,17 +47,17 @@ describe Gitlab::UserAccess, lib: true do @branch = create :protected_branch, project: project, developers_can_push: true end - it "returns true if user is a master" do + it 'returns true if user is a master' do project.team << [user, :master] expect(access.can_push_to_branch?(@branch.name)).to be_truthy end - it "returns true if user is a developer" do + it 'returns true if user is a developer' do project.team << [user, :developer] expect(access.can_push_to_branch?(@branch.name)).to be_truthy end - it "returns false if user is a reporter" do + it 'returns false if user is a reporter' do project.team << [user, :reporter] expect(access.can_push_to_branch?(@branch.name)).to be_falsey end @@ -70,17 +68,17 @@ describe Gitlab::UserAccess, lib: true do @branch = create :protected_branch, project: project, developers_can_merge: true end - it "returns true if user is a master" do + 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 + 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 + 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 diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index 0f30a84a2cf..47c0580e0f0 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -243,7 +243,8 @@ 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, developers_can_merge: false }) - execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) + + 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 -- cgit v1.2.1