5 files changed, 92 insertions, 63 deletions

diff --git a/lib/gitlab/auth/result.rb b/lib/gitlab/auth/result.rb
index 6be7f690676..39b86c61a18 100644
--- a/lib/gitlab/auth/result.rb
+++ b/lib/gitlab/auth/result.rb
@@ -9,8 +9,7 @@ module Gitlab
 
       def lfs_deploy_token?(for_project)
         type == :lfs_deploy_token &&
-          actor &&
-          actor.projects.include?(for_project)
+          actor.try(:has_access_to?, for_project)
       end
 
       def success?
diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb
index cb1065223d4..6b6a86ffde9 100644
--- a/lib/gitlab/checks/change_access.rb
+++ b/lib/gitlab/checks/change_access.rb
@@ -1,13 +1,15 @@
 module Gitlab
   module Checks
     class ChangeAccess
-      attr_reader :user_access, :project
+      attr_reader :user_access, :project, :skip_authorization
 
-      def initialize(change, user_access:, project:)
+      def initialize(
+        change, user_access:, project:, skip_authorization: false)
         @oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref)
         @branch_name = Gitlab::Git.branch_name(@ref)
         @user_access = user_access
         @project = project
+        @skip_authorization = skip_authorization
       end
 
       def exec
@@ -23,6 +25,7 @@ module Gitlab
       protected
 
       def protected_branch_checks
+        return if skip_authorization
         return unless @branch_name
         return unless project.protected_branch?(@branch_name)
 
@@ -48,6 +51,8 @@ module Gitlab
       end
 
       def tag_checks
+        return if skip_authorization
+
         tag_ref = Gitlab::Git.tag_name(@ref)
 
         if tag_ref && protected_tag?(tag_ref) && user_access.cannot_do_action?(:admin_project)
@@ -56,6 +61,8 @@ module Gitlab
       end
 
       def push_checks
+        return if skip_authorization
+
         if user_access.cannot_do_action?(:push_code)
           "You are not allowed to push code to this project."
         end
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index db07b7c5fcc..9563fa7cafb 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -7,7 +7,10 @@ module Gitlab
     ERROR_MESSAGES = {
       upload: 'You are not allowed to upload code for this project.',
       download: 'You are not allowed to download code from this project.',
-      deploy_key: 'Deploy keys are not allowed to push code.',
+      deploy_key_upload:
+        'This deploy key does not have write access to this project.',
+      deploy_key:
+        'This deploy key does not have access to this project.',
       no_repo: 'A repository for this project does not exist yet.'
     }
 
@@ -27,7 +30,7 @@ module Gitlab
 
     def check(cmd, changes)
       check_protocol!
-      check_active_user!
+      check_active_user! unless deploy_key?
       check_project_accessibility!
       check_command_existence!(cmd)
 
@@ -44,31 +47,42 @@ module Gitlab
     end
 
     def download_access_check
-      if user
-        user_download_access_check
-      elsif deploy_key.nil? && !guest_can_downlod_code?
-        raise UnauthorizedError, ERROR_MESSAGES[:download]
+      passed = if deploy_key
+                 deploy_key.has_access_to?(project)
+               elsif user
+                 user_can_download_code? || build_can_download_code?
+               end || Guest.can?(:download_code, project)
+
+      unless passed
+        message = if deploy_key
+                    ERROR_MESSAGES[:deploy_key]
+                  else
+                    ERROR_MESSAGES[:download]
+                  end
+
+        raise UnauthorizedError, message
       end
     end
 
     def push_access_check(changes)
-      if user
-        user_push_access_check(changes)
+      if deploy_key
+        deploy_key_push_access_check
+      elsif user
+        user_push_access_check
       else
-        raise UnauthorizedError, ERROR_MESSAGES[deploy_key ? :deploy_key : :upload]
+        raise UnauthorizedError, ERROR_MESSAGES[:upload]
       end
+
+      return if changes.blank? # Allow access.
+
+      check_repository_existence!
+      check_change_access!(changes)
     end
 
     def guest_can_downlod_code?
       Guest.can?(:download_code, project)
     end
 
-    def user_download_access_check
-      unless user_can_download_code? || build_can_download_code?
-        raise UnauthorizedError, ERROR_MESSAGES[:download]
-      end
-    end
-
     def user_can_download_code?
       authentication_abilities.include?(:download_code) && user_access.can_do_action?(:download_code)
     end
@@ -77,33 +91,16 @@ module Gitlab
       authentication_abilities.include?(:build_download_code) && user_access.can_do_action?(:build_download_code)
     end
 
-    def user_push_access_check(changes)
+    def user_push_access_check
       unless authentication_abilities.include?(:push_code)
         raise UnauthorizedError, ERROR_MESSAGES[:upload]
       end
-
-      if changes.blank?
-        return # Allow access.
-      end
-
-      unless project.repository.exists?
-        raise UnauthorizedError, ERROR_MESSAGES[:no_repo]
-      end
-
-      changes_list = Gitlab::ChangesList.new(changes)
-
-      # Iterate over all changes to find if user allowed all of them to be applied
-      changes_list.each do |change|
-        status = change_access_check(change)
-        unless status.allowed?
-          # If user does not have access to make at least one change - cancel all push
-          raise UnauthorizedError, status.message
-        end
-      end
     end
 
-    def change_access_check(change)
-      Checks::ChangeAccess.new(change, user_access: user_access, project: project).exec
+    def deploy_key_push_access_check
+      unless deploy_key.can_push_to?(project)
+        raise UnauthorizedError, ERROR_MESSAGES[:deploy_key_upload]
+      end
     end
 
     def protocol_allowed?
@@ -136,31 +133,51 @@ module Gitlab
       end
     end
 
+    def check_repository_existence!
+      unless project.repository.exists?
+        raise UnauthorizedError, ERROR_MESSAGES[:no_repo]
+      end
+    end
+
+    def check_change_access!(changes)
+      changes_list = Gitlab::ChangesList.new(changes)
+
+      # Iterate over all changes to find if user allowed all of them to be applied
+      changes_list.each do |change|
+        status = check_single_change_access(change)
+        unless status.allowed?
+          # If user does not have access to make at least one change - cancel all push
+          raise UnauthorizedError, status.message
+        end
+      end
+    end
+
+    def check_single_change_access(change)
+      Checks::ChangeAccess.new(
+        change,
+        user_access: user_access,
+        project: project,
+        skip_authorization: deploy_key?).exec
+    end
+
     def matching_merge_request?(newrev, branch_name)
       Checks::MatchingMergeRequest.new(newrev, branch_name, project).match?
     end
 
     def deploy_key
-      actor if actor.is_a?(DeployKey)
+      actor if deploy_key?
     end
 
-    def deploy_key_can_read_project?
-      if deploy_key
-        return true if project.public?
-        deploy_key.projects.include?(project)
-      else
-        false
-      end
+    def deploy_key?
+      actor.is_a?(DeployKey)
     end
 
     def can_read_project?
-      if user
-        user_access.can_read_project?
-      elsif deploy_key
-        deploy_key_can_read_project?
-      else
-        Guest.can?(:read_project, project)
-      end
+      if deploy_key
+        deploy_key.has_access_to?(project)
+      elsif user
+        user.can?(:read_project, project)
+      end || Guest.can?(:read_project, project)
     end
 
     protected
@@ -172,8 +189,6 @@ module Gitlab
         case actor
         when User
           actor
-        when DeployKey
-          nil
         when Key
           actor.user
         end
diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb
index 2c06c4ff1ef..74171f4f90e 100644
--- a/lib/gitlab/git_access_wiki.rb
+++ b/lib/gitlab/git_access_wiki.rb
@@ -8,7 +8,7 @@ module Gitlab
       authentication_abilities.include?(:download_code) && user_access.can_do_action?(:download_wiki_code)
     end
 
-    def change_access_check(change)
+    def check_single_change_access(change)
       if user_access.can_do_action?(:create_wiki)
         build_status_object(true)
       else
diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb
index 9858d2e7d83..6c7e673fb9f 100644
--- a/lib/gitlab/user_access.rb
+++ b/lib/gitlab/user_access.rb
@@ -8,6 +8,8 @@ module Gitlab
     end
 
     def can_do_action?(action)
+      return false if no_user_or_blocked?
+
       @permission_cache ||= {}
       @permission_cache[action] ||= user.can?(action, project)
     end
@@ -17,7 +19,7 @@ module Gitlab
     end
 
     def allowed?
-      return false if user.blank? || user.blocked?
+      return false if no_user_or_blocked?
 
       if user.requires_ldap_check? && user.try_obtain_ldap_lease
         return false unless Gitlab::LDAP::Access.allowed?(user)
@@ -27,7 +29,7 @@ module Gitlab
     end
 
     def can_push_to_branch?(ref)
-      return false unless user
+      return false if no_user_or_blocked?
 
       if project.protected_branch?(ref)
         return true if project.empty_repo? && project.user_can_push_to_empty_repo?(user)
@@ -40,7 +42,7 @@ module Gitlab
     end
 
     def can_merge_to_branch?(ref)
-      return false unless user
+      return false if no_user_or_blocked?
 
       if project.protected_branch?(ref)
         access_levels = project.protected_branches.matching(ref).map(&:merge_access_levels).flatten
@@ -51,9 +53,15 @@ module Gitlab
     end
 
     def can_read_project?
-      return false unless user
+      return false if no_user_or_blocked?
 
       user.can?(:read_project, project)
     end
+
+    private
+
+    def no_user_or_blocked?
+      user.nil? || user.blocked?
+    end
   end
 end