Merge branch 'api-internal-errors' into 'master'

Respond with full GitAccess error if user has project read access.

Should help with debugging #1236.

cc @marin

See merge request !437

5 files changed, 105 insertions, 68 deletions

diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 753d0fcbd98..f98a17773e7 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -17,42 +17,40 @@ module API
       post "/allowed" do
         status 200
 
-        actor = if params[:key_id]
-                  Key.find_by(id: params[:key_id])
-                elsif params[:user_id]
-                  User.find_by(id: params[:user_id])
-                end
+        actor = 
+          if params[:key_id]
+            Key.find_by(id: params[:key_id])
+          elsif params[:user_id]
+            User.find_by(id: params[:user_id])
+          end
 
         unless actor
           return Gitlab::GitAccessStatus.new(false, 'No such user or key')
         end
 
         project_path = params[:project]
-
+        
         # Check for *.wiki repositories.
         # Strip out the .wiki from the pathname before finding the
         # project. This applies the correct project permissions to
         # the wiki repository as well.
-        access =
-          if project_path.end_with?('.wiki')
-            project_path.chomp!('.wiki')
-            Gitlab::GitAccessWiki.new
-          else
-            Gitlab::GitAccess.new
-          end
+        wiki = project_path.end_with?('.wiki')
+        project_path.chomp!('.wiki') if wiki
 
         project = Project.find_with_namespace(project_path)
 
         if project
-          status = access.check(
-            actor,
-            params[:action],
-            project,
-            params[:changes]
-          )
+          access =
+            if wiki
+              Gitlab::GitAccessWiki.new(actor, project)
+            else
+              Gitlab::GitAccess.new(actor, project)
+            end
+
+          status = access.check(params[:action], params[:changes])
         end
 
-        if project && status && status.allowed?
+        if project && access.can_read_project?
           status
         else
           Gitlab::GitAccessStatus.new(false, 'No such project')
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index 25b7857f4b1..f3765f5ab03 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -178,7 +178,8 @@ module API
       put ":id/merge_request/:merge_request_id/merge" do
         merge_request = user_project.merge_requests.find(params[:merge_request_id])
 
-        allowed = ::Gitlab::GitAccess.can_push_to_branch?(current_user, user_project, merge_request.target_branch)
+        allowed = ::Gitlab::GitAccess.new(current_user, user_project).
+          can_push_to_branch?(merge_request.target_branch)
 
         if allowed
           if merge_request.unchecked?
diff --git a/lib/gitlab/backend/grack_auth.rb b/lib/gitlab/backend/grack_auth.rb
index ffe4565ef1e..050b5ba29dd 100644
--- a/lib/gitlab/backend/grack_auth.rb
+++ b/lib/gitlab/backend/grack_auth.rb
@@ -129,7 +129,7 @@ module Grack
       case git_cmd
       when *Gitlab::GitAccess::DOWNLOAD_COMMANDS
         if user
-          Gitlab::GitAccess.new.download_access_check(user, project).allowed?
+          Gitlab::GitAccess.new(user, project).download_access_check.allowed?
         elsif project.public?
           # Allow clone/fetch for public projects
           true
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index cb69e4b13d3..d6e609e2c44 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -3,9 +3,32 @@ module Gitlab
     DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive }
     PUSH_COMMANDS = %w{ git-receive-pack }
 
-    attr_reader :params, :project, :git_cmd, :user
+    attr_reader :actor, :project
 
-    def self.can_push_to_branch?(user, project, ref)
+    def initialize(actor, project)
+      @actor    = actor
+      @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)  &&
@@ -16,51 +39,65 @@ module Gitlab
       end
     end
 
-    def check(actor, cmd, project, changes = nil)
+    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)
       case cmd
       when *DOWNLOAD_COMMANDS
-        download_access_check(actor, project)
+        download_access_check
       when *PUSH_COMMANDS
-        if actor.is_a? User
-          push_access_check(actor, project, changes)
-        elsif actor.is_a? DeployKey
-          return build_status_object(false, "Deploy key not allowed to push")
-        elsif actor.is_a? Key
-          push_access_check(actor.user, project, changes)
-        else
-          raise 'Wrong actor'
-        end
+        push_access_check(changes)
       else
-        return build_status_object(false, "Wrong command")
+        build_status_object(false, "Wrong command")
       end
     end
 
-    def download_access_check(actor, project)
-      if actor.is_a?(User)
-        user_download_access_check(actor, project)
-      elsif actor.is_a?(DeployKey)
-        if actor.projects.include?(project)
-          build_status_object(true)
-        else
-          build_status_object(false, "Deploy key not allowed to access this project")
-        end
-      elsif actor.is_a? Key
-        user_download_access_check(actor.user, project)
+    def download_access_check
+      if user
+        user_download_access_check
+      elsif deploy_key
+        deploy_key_download_access_check
       else
         raise 'Wrong actor'
       end
     end
 
-    def user_download_access_check(user, project)
-      if user && user_allowed?(user) && user.can?(:download_code, project)
+    def push_access_check(changes)
+      if user
+        user_push_access_check(changes)
+      elsif deploy_key
+        build_status_object(false, "Deploy key not allowed to push")
+      else
+        raise 'Wrong actor'
+      end
+    end
+
+    def user_download_access_check
+      if user && user_allowed? && user.can?(:download_code, project)
         build_status_object(true)
       else
         build_status_object(false, "You don't have access")
       end
     end
 
-    def push_access_check(user, project, changes)
-      unless user && user_allowed?(user)
+    def deploy_key_download_access_check
+      if can_read_project?
+        build_status_object(true)
+      else
+        build_status_object(false, "Deploy key not allowed to access this project")
+      end
+    end
+
+    def user_push_access_check(changes)
+      unless user && user_allowed?
         return build_status_object(false, "You don't have access")
       end
 
@@ -76,27 +113,28 @@ module Gitlab
 
       # Iterate over all changes to find if user allowed all of them to be applied
       changes.map(&:strip).reject(&:blank?).each do |change|
-        status = change_access_check(user, project, change)
+        status = change_access_check(change)
         unless status.allowed?
           # If user does not have access to make at least one change - cancel all push
           return status
         end
       end
 
-      return build_status_object(true)
+      build_status_object(true)
     end
 
-    def change_access_check(user, project, change)
+    def change_access_check(change)
       oldrev, newrev, ref = change.split(' ')
 
-      action = if project.protected_branch?(branch_name(ref))
-                 protected_branch_action(project, oldrev, newrev, branch_name(ref))
-               elsif protected_tag?(project, tag_name(ref))
-                 # Prevent any changes to existing git tag unless user has permissions
-                 :admin_project
-               else
-                 :push_code
-               end
+      action = 
+        if project.protected_branch?(branch_name(ref))
+          protected_branch_action(oldrev, newrev, branch_name(ref))
+        elsif protected_tag?(tag_name(ref))
+          # Prevent any changes to existing git tag unless user has permissions
+          :admin_project
+        else
+          :push_code
+        end
 
       if user.can?(action, project)
         build_status_object(true)
@@ -105,15 +143,15 @@ module Gitlab
       end
     end
 
-    def forced_push?(project, oldrev, newrev)
+    def forced_push?(oldrev, newrev)
       Gitlab::ForcePushCheck.force_push?(project, oldrev, newrev)
     end
 
     private
 
-    def protected_branch_action(project, oldrev, newrev, branch_name)
+    def protected_branch_action(oldrev, newrev, branch_name)
       # we dont allow force push to protected branch
-      if forced_push?(project, oldrev, newrev)
+      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
@@ -125,11 +163,11 @@ module Gitlab
       end
     end
 
-    def protected_tag?(project, tag_name)
+    def protected_tag?(tag_name)
       project.repository.tag_names.include?(tag_name)
     end
 
-    def user_allowed?(user)
+    def user_allowed?
       Gitlab::UserAccess.allowed?(user)
     end
 
diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb
index a2177c8d548..73d99b96202 100644
--- a/lib/gitlab/git_access_wiki.rb
+++ b/lib/gitlab/git_access_wiki.rb
@@ -1,6 +1,6 @@
 module Gitlab
   class GitAccessWiki < GitAccess
-    def change_access_check(user, project, change)
+    def change_access_check(change)
       if user.can?(:write_wiki, project)
         build_status_object(true)
       else