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

@@ -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

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
 

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')

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")

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
 

lib/gitlab/git_access.rb

+# 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
+    def matching_merge_request?(newrev, branch_name)
+      Checks::MatchingMergeRequest.new(newrev, branch_name, project).match?
     end
 
-    def protected_tag?(tag_name)
-      project.repository.tag_exists?(tag_name)
+    def deploy_key
+      actor if actor.is_a?(DeployKey)
     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)
+    def deploy_key_can_read_project?
+      if deploy_key
+        deploy_key.projects.include?(project)
       else
-        nil
+        false
       end
     end
 
-    def tag_name(ref)
-      ref = ref.to_s
-      if Gitlab::Git.tag_ref?(ref)
-        Gitlab::Git.ref_name(ref)
-      else
+    protected
+
+    def user
+      return @user if defined?(@user)
+
+      @user =
+        case actor
+        when User
+          actor
+        when DeployKey
           nil
+        when Key
+          actor.user
         end
     end
 
-    protected
-
     def build_status_object(status, message = '')
       GitAccessStatus.new(status, message)
     end

lib/gitlab/git_access/change_access_check.rb

+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

lib/gitlab/git_access_wiki.rb

 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.")

lib/gitlab/user_access.rb

 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

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

spec/lib/gitlab/user_access_spec.rb

+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

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