From 530f5158e297f3cde27f3566cfe13bad74ba3b50 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 13 Jul 2016 13:57:30 -0500 Subject: Revert "Merge branch '18193-developers-can-merge' into 'master' " This reverts commit 9ca633eb4c62231e4ddff5466c723cf8e2bdb25d, reversing changes made to fb229bbf7970ba908962b837b270adf56f14098f. --- CHANGELOG | 1 - .../javascripts/protected_branches.js.coffee | 8 +- .../projects/protected_branches_controller.rb | 2 +- app/helpers/branches_helper.rb | 2 +- app/models/merge_request.rb | 8 +- app/models/project.rb | 4 - app/models/repository.rb | 14 +- app/services/commits/change_service.rb | 4 +- app/services/files/base_service.rb | 2 +- app/services/git_push_service.rb | 3 +- app/services/merge_requests/merge_service.rb | 4 +- app/services/merge_requests/refresh_service.rb | 2 +- .../widget/open/_conflicts.html.haml | 2 +- .../protected_branches/_branches_list.html.haml | 2 - .../protected_branches/_protected_branch.html.haml | 2 - .../projects/protected_branches/index.html.haml | 8 - ...d_developers_can_merge_to_protected_branches.rb | 9 - ..._progress_merge_commit_sha_to_merge_requests.rb | 8 - db/schema.rb | 8 +- lib/api/helpers.rb | 2 +- lib/gitlab/access.rb | 8 +- lib/gitlab/checks/change_access.rb | 96 -------- lib/gitlab/checks/force_push.rb | 17 -- lib/gitlab/checks/matching_merge_request.rb | 18 -- lib/gitlab/force_push_check.rb | 15 ++ lib/gitlab/git_access.rb | 143 +++++++++--- lib/gitlab/git_access_wiki.rb | 2 +- lib/gitlab/user_access.rb | 48 +--- spec/lib/gitlab/diff/position_tracer_spec.rb | 3 +- spec/lib/gitlab/git_access_spec.rb | 247 +++++++++++---------- spec/lib/gitlab/user_access_spec.rb | 88 -------- spec/models/repository_spec.rb | 9 +- spec/requests/api/api_helpers_spec.rb | 4 +- spec/services/git_push_service_spec.rb | 14 +- .../merge_requests/refresh_service_spec.rb | 3 +- 35 files changed, 303 insertions(+), 507 deletions(-) delete mode 100644 db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb delete mode 100644 db/migrate/20160629025435_add_column_in_progress_merge_commit_sha_to_merge_requests.rb delete mode 100644 lib/gitlab/checks/change_access.rb delete mode 100644 lib/gitlab/checks/force_push.rb delete mode 100644 lib/gitlab/checks/matching_merge_request.rb create mode 100644 lib/gitlab/force_push_check.rb delete mode 100644 spec/lib/gitlab/user_access_spec.rb diff --git a/CHANGELOG b/CHANGELOG index a42f8c7b916..71987a4a695 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -44,7 +44,6 @@ 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 diff --git a/app/assets/javascripts/protected_branches.js.coffee b/app/assets/javascripts/protected_branches.js.coffee index 14afef2e2ee..79c2306e4d2 100644 --- a/app/assets/javascripts/protected_branches.js.coffee +++ b/app/assets/javascripts/protected_branches.js.coffee @@ -1,18 +1,18 @@ $ -> $(".protected-branches-list :checkbox").change (e) -> name = $(this).attr("name") - if name == "developers_can_push" || name == "developers_can_merge" + if name == "developers_can_push" id = $(this).val() - can_push = $(this).is(":checked") + checked = $(this).is(":checked") url = $(this).data("url") $.ajax - type: "PATCH" + type: "PUT" url: url dataType: "json" data: id: id protected_branch: - "#{name}": can_push + developers_can_push: checked success: -> row = $(e.target) diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb index 10dca47fded..80dad758afa 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, :developers_can_merge) + params.require(:protected_branch).permit(:name, :developers_can_push) end end diff --git a/app/helpers/branches_helper.rb b/app/helpers/branches_helper.rb index bfd23aa4e04..601df5c18df 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::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 def project_branches diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 471e32f3b60..157901378d3 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -552,13 +552,7 @@ class MergeRequest < ActiveRecord::Base end def can_be_merged_by?(user) - access = ::Gitlab::UserAccess.new(user, project: project) - 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) + ::Gitlab::GitAccess.new(user, project, 'web').can_push_to_branch?(target_branch) end def mergeable_ci_state? diff --git a/app/models/project.rb b/app/models/project.rb index 2bd97fc48f1..a66b750cd48 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -832,10 +832,6 @@ 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/models/repository.rb b/app/models/repository.rb index 09487b62f98..5b670cb4b8f 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -769,9 +769,9 @@ class Repository end end - def merge(user, merge_request, options = {}) - our_commit = rugged.branches[merge_request.target_branch].target - their_commit = rugged.lookup(merge_request.diff_head_sha) + def merge(user, source_sha, target_branch, options = {}) + our_commit = rugged.branches[target_branch].target + their_commit = rugged.lookup(source_sha) raise "Invalid merge target" if our_commit.nil? raise "Invalid merge source" if their_commit.nil? @@ -779,16 +779,14 @@ class Repository merge_index = rugged.merge_commits(our_commit, their_commit) 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( parents: [our_commit, their_commit], tree: merge_index.write_tree(rugged), - update_ref: tmp_ref + update_ref: ref ) - commit_id = Rugged::Commit.create(rugged, actual_options) - merge_request.update(in_progress_merge_commit_sha: commit_id) - commit_id + Rugged::Commit.create(rugged, actual_options) end end diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb index ed73d8cb8c2..c578097376a 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::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 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 55da949f56a..37c5e321b39 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::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 raise_error("You are not allowed to push into this branch") diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index e02b50ff9a2..a886f35981f 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -89,8 +89,7 @@ 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 - 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 }) + @project.protected_branches.create({ name: @project.default_branch, developers_can_push: developers_can_push }) end end diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 0dac0614141..f1b1d90c457 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, 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) rescue GitHooksService::PreReceiveError => e merge_request.update(merge_error: e.message) @@ -43,8 +43,6 @@ 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/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index 1daf6bbf553..b11ecd97a57 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::Checks::ForcePush.force_push?(@project, @oldrev, @newrev) + Gitlab::ForcePushCheck.force_push?(@project, @oldrev, @newrev) end # Refresh merge request diff if we push to source or target branch of merge request 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 f000cc38a65..06ab0a3fa00 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_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"}. - else ask someone with write access to this repository to merge this request manually. diff --git a/app/views/projects/protected_branches/_branches_list.html.haml b/app/views/projects/protected_branches/_branches_list.html.haml index 720d67dff7c..97cb1a9052b 100644 --- a/app/views/projects/protected_branches/_branches_list.html.haml +++ b/app/views/projects/protected_branches/_branches_list.html.haml @@ -8,7 +8,6 @@ .table-responsive %table.table.protected-branches-list %colgroup - %col{ width: "20%" } %col{ width: "30%" } %col{ width: "25%" } %col{ width: "25%" } @@ -19,7 +18,6 @@ %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 7fda7f96047..474aec3a97c 100644 --- a/app/views/projects/protected_branches/_protected_branch.html.haml +++ b/app/views/projects/protected_branches/_protected_branch.html.haml @@ -16,8 +16,6 @@ (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 101b3f3b452..3fab95751e0 100644 --- a/app/views/projects/protected_branches/index.html.haml +++ b/app/views/projects/protected_branches/index.html.haml @@ -36,14 +36,6 @@ = 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 deleted file mode 100644 index 15ad8e8bcbb..00000000000 --- a/db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb +++ /dev/null @@ -1,9 +0,0 @@ -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/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 deleted file mode 100644 index 7c5f76572ef..00000000000 --- a/db/migrate/20160629025435_add_column_in_progress_merge_commit_sha_to_merge_requests.rb +++ /dev/null @@ -1,8 +0,0 @@ -# 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 64019cf79bb..f24e47b85b2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -626,7 +626,6 @@ 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 @@ -861,12 +860,11 @@ 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_merge", default: false, null: false + t.boolean "developers_can_push", default: false, null: false end add_index "protected_branches", ["project_id"], name: "index_protected_branches_on_project_id", using: :btree diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 73557cf7db6..77e407b54c5 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.new(@current_user).allowed? + unless @current_user && Gitlab::UserAccess.allowed?(@current_user) return nil end diff --git a/lib/gitlab/access.rb b/lib/gitlab/access.rb index de41ea415a6..831f1e635ba 100644 --- a/lib/gitlab/access.rb +++ b/lib/gitlab/access.rb @@ -14,10 +14,9 @@ module Gitlab OWNER = 50 # Branch protection settings - PROTECTION_NONE = 0 - PROTECTION_DEV_CAN_PUSH = 1 - PROTECTION_FULL = 2 - PROTECTION_DEV_CAN_MERGE = 3 + PROTECTION_NONE = 0 + PROTECTION_DEV_CAN_PUSH = 1 + PROTECTION_FULL = 2 class << self def values @@ -55,7 +54,6 @@ 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/checks/change_access.rb b/lib/gitlab/checks/change_access.rb deleted file mode 100644 index 5551fac4b8b..00000000000 --- a/lib/gitlab/checks/change_access.rb +++ /dev/null @@ -1,96 +0,0 @@ -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 diff --git a/lib/gitlab/checks/force_push.rb b/lib/gitlab/checks/force_push.rb deleted file mode 100644 index dfa83a0eab3..00000000000 --- a/lib/gitlab/checks/force_push.rb +++ /dev/null @@ -1,17 +0,0 @@ -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/checks/matching_merge_request.rb b/lib/gitlab/checks/matching_merge_request.rb deleted file mode 100644 index 849848515da..00000000000 --- a/lib/gitlab/checks/matching_merge_request.rb +++ /dev/null @@ -1,18 +0,0 @@ -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/force_push_check.rb b/lib/gitlab/force_push_check.rb new file mode 100644 index 00000000000..93c6a5bb7f5 --- /dev/null +++ b/lib/gitlab/force_push_check.rb @@ -0,0 +1,15 @@ +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 308f23bc9bc..7679c7e4bb8 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -1,17 +1,52 @@ -# 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, :user_access + attr_reader :actor, :project, :protocol def initialize(actor, project, protocol) @actor = actor @project = project @protocol = protocol - @user_access = UserAccess.new(user, project: project) + 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_read_project? + if user + user.can?(:read_project, project) + elsif deploy_key + deploy_key.projects.include?(project) + else + false + end end def check(cmd, changes = nil) @@ -21,11 +56,11 @@ module Gitlab return build_status_object(false, "No user or key was provided.") end - if user && !user_access.allowed? + if user && !user_allowed? return build_status_object(false, "Your account has been blocked.") end - unless project && (user_access.can_read_project? || deploy_key_can_read_project?) + unless project && can_read_project? return build_status_object(false, 'The project you were looking for could not be found.') end @@ -60,7 +95,7 @@ module Gitlab end def user_download_access_check - unless user_access.can_do_action?(:download_code) + unless user.can?(:download_code, project) return build_status_object(false, "You are not allowed to download code from this project.") end @@ -90,8 +125,46 @@ 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) - Checks::ChangeAccess.new(change, user_access: user_access, project: project).exec + 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) end def protocol_allowed? @@ -100,38 +173,48 @@ module Gitlab private - def matching_merge_request?(newrev, branch_name) - Checks::MatchingMergeRequest.new(newrev, branch_name, project).match? + 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 project.developers_can_push_to_protected_branch?(branch_name) + :push_code + else + :push_code_to_protected_branches + end end - def deploy_key - actor if actor.is_a?(DeployKey) + def protected_tag?(tag_name) + project.repository.tag_exists?(tag_name) end - def deploy_key_can_read_project? - if deploy_key - deploy_key.projects.include?(project) + def user_allowed? + Gitlab::UserAccess.allowed?(user) + end + + def branch_name(ref) + ref = ref.to_s + if Gitlab::Git.branch_ref?(ref) + Gitlab::Git.ref_name(ref) else - false + nil end end - protected - - def user - return @user if defined?(@user) - - @user = - case actor - when User - actor - when DeployKey - nil - when Key - actor.user - end + def tag_name(ref) + ref = ref.to_s + if Gitlab::Git.tag_ref?(ref) + Gitlab::Git.ref_name(ref) + else + nil + end end + protected + def build_status_object(status, message = '') GitAccessStatus.new(status, message) end diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb index f71d3575909..8672cbc0ec4 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_access.can_do_action?(:create_wiki) + if user.can?(:create_wiki, project) 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 c0f85e9b3a8..d1b42c1f9b9 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -1,23 +1,7 @@ module Gitlab - 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? + module UserAccess + def self.allowed?(user) + return false if user.blocked? if user.requires_ldap_check? && user.try_obtain_ldap_lease return false unless Gitlab::LDAP::Access.allowed?(user) @@ -25,31 +9,5 @@ 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/diff/position_tracer_spec.rb b/spec/lib/gitlab/diff/position_tracer_spec.rb index c268f84c759..08312e60f4a 100644 --- a/spec/lib/gitlab/diff/position_tracer_spec.rb +++ b/spec/lib/gitlab/diff/position_tracer_spec.rb @@ -1639,8 +1639,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do committer: committer } - 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) + repository.merge(current_user, second_create_file_commit.sha, branch_name, 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 db33c7a22bb..c79ba11f782 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -6,6 +6,67 @@ 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 + end + describe '#check with single protocols allowed' do def disable_protocol(protocol) settings = ::ApplicationSetting.create_from_defaults @@ -99,145 +160,103 @@ describe Gitlab::GitAccess, lib: true do end describe 'push_access_check' do - before { merge_into_protected_branch } - let(:unprotected_branch) { FFaker::Internet.user_name } + def protect_feature_branch + create(:protected_branch, name: 'feature', project: project) + end - let(:changes) do - { push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow", + def changes + { + 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'], - merge_into_protected_branch: "0b4bc9a #{merge_into_protected_branch} refs/heads/feature" } + push_all: ['6f6d7e7ed 570e7b2ab refs/heads/master', '6f6d7e7ed 570e7b2ab refs/heads/feature'] + } end - def stub_git_hooks - # Running the `pre-receive` hook is expensive, and not necessary for this test. - allow_any_instance_of(GitHooksService).to receive(:execute).and_yield + 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, + } + } end - def merge_into_protected_branch - @protected_branch_merge_commit ||= begin - stub_git_hooks - project.repository.add_branch(user, unprotected_branch, 'feature') - target_branch = project.repository.lookup('feature') - source_branch = project.repository.commit_file(user, FFaker::InternetSE.login_user_name, FFaker::HipsterIpsum.paragraph, FFaker::HipsterIpsum.sentence, unprotected_branch, false) - rugged = project.repository.rugged - author = { email: "email@example.com", time: Time.now, name: "Example Git User" } - - merge_index = rugged.merge_commits(target_branch, source_branch) - Rugged::Commit.create(rugged, author: author, committer: author, message: "commit message", parents: [target_branch, source_branch], tree: merge_index.write_tree(rugged)) - end + 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 end - def self.run_permission_checks(permissions_matrix) - permissions_matrix.keys.each do |role| - describe "#{role} access" do - before { project.team << [user, role] } + permissions_matrix.keys.each do |role| + describe "#{role} access" do + before { protect_feature_branch } + before { project.team << [user, role] } - permissions_matrix[role].each do |action, allowed| - context action do - subject { access.push_access_check(changes[action]) } + 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 + it { expect(subject.allowed?).to allowed ? be_truthy : be_falsey } end 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 + context "with enabled developers push to protected branches " do + updated_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] } - run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: true })) - end + updated_permissions_matrix[role].each do |action, allowed| + context action do + subject { access.push_access_check(changes[action]) } - 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) + it { expect(subject.allowed?).to allowed ? be_truthy : be_falsey } 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 diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb deleted file mode 100644 index aa9ec243498..00000000000 --- a/spec/lib/gitlab/user_access_spec.rb +++ /dev/null @@ -1,88 +0,0 @@ -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 - let(:branch) { create :protected_branch, project: project } - - 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/models/repository_spec.rb b/spec/models/repository_spec.rb index e14cec589fe..b39b958450c 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -4,17 +4,16 @@ describe Repository, models: true do include RepoHelpers TestBlob = Struct.new(:name) - let(:project) { create(:project) } - let(:repository) { project.repository } + let(:repository) { create(: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 - 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) + source_sha = repository.find_branch('feature').target + merge_commit_sha = repository.merge(user, source_sha, 'master', commit_options) + repository.commit(merge_commit_sha) end describe '#branch_names_contains' do diff --git a/spec/requests/api/api_helpers_spec.rb b/spec/requests/api/api_helpers_spec.rb index 3d5c19aeff3..83025953889 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_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false) + allow(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_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false) + allow(Gitlab::UserAccess).to receive(:allowed?).and_return(false) expect(current_user).to be_nil end diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index 47c0580e0f0..afabeed4a80 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, developers_can_merge: false }) + expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false }) execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) end @@ -242,17 +242,7 @@ 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') - 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 }) + expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: true }) execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) end diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index ce643b3f860..06f56d85aa8 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -88,7 +88,8 @@ 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 } - @project.repository.merge(@user, @merge_request, commit_options) + master_commit = @project.repository.commit('master') + @project.repository.merge(@user, master_commit.id, 'feature', commit_options) commit = @project.repository.commit('feature') service.new(@project, @user).execute(@oldrev, commit.id, 'refs/heads/feature') reload_mrs -- cgit v1.2.1