From 60245bbe228014a9f689adafd64b571883ef6eb3 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Fri, 24 Jun 2016 10:37:04 +0530 Subject: [PATCH] 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 | 163 ++++--------------- 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, 272 insertions(+), 225 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) + ChangeAccessCheck.new(change, user_access: user_access, project: project).exec end - def forced_push?(oldrev, newrev) - Gitlab::ForcePushCheck.force_push?(project, oldrev, newrev) - end - - def protocol_allowed? Gitlab::ProtocolAccess.allowed?(protocol) end + private + def matching_merge_request?(newrev, branch_name) Checks::MatchingMergeRequest.new(newrev, branch_name, project).match? end - private + def deploy_key + actor if actor.is_a?(DeployKey) + end - 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 + + def deploy_key_can_read_project? + if deploy_key + deploy_key.projects.include?(project) else - :push_code_to_protected_branches - end - end - - def protected_tag?(tag_name) - project.repository.tag_exists?(tag_name) - end - - 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 - nil - end - end - - def tag_name(ref) - ref = ref.to_s - if Gitlab::Git.tag_ref?(ref) - Gitlab::Git.ref_name(ref) - 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