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

14 files changed, 134 insertions, 94 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 737831eb195..7b24fb00c6d 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -36,6 +36,7 @@ v 7.10.0 (unreleased)
   - Don't include system notes in issue/MR comment count.
   - Don't mark merge request as updated when merge status relative to target branch changes.
   - Link note avatar to user.
+  - Make Git-over-SSH errors more descriptive.
 
 v 7.9.0
   - Send EmailsOnPush email when branch or tag is created or deleted.
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index e9b7d7e0083..47ce8467358 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -257,7 +257,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def allowed_to_push_code?(project, branch)
-    ::Gitlab::GitAccess.can_push_to_branch?(current_user, project, branch)
+    ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(branch)
   end
 
   def merge_request_params
diff --git a/app/helpers/branches_helper.rb b/app/helpers/branches_helper.rb
index 4a5edf6d101..d6eaa7d57bc 100644
--- a/app/helpers/branches_helper.rb
+++ b/app/helpers/branches_helper.rb
@@ -12,6 +12,6 @@ module BranchesHelper
   def can_push_branch?(project, branch_name)
     return false unless project.repository.branch_names.include?(branch_name)
     
-    ::Gitlab::GitAccess.can_push_to_branch?(current_user, project, branch_name)
+    ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(branch_name)
   end
 end
diff --git a/app/helpers/tree_helper.rb b/app/helpers/tree_helper.rb
index b6fb7a8aa5a..bf6726574ec 100644
--- a/app/helpers/tree_helper.rb
+++ b/app/helpers/tree_helper.rb
@@ -56,7 +56,7 @@ module TreeHelper
     ref ||= @ref
     return false unless project.repository.branch_names.include?(ref)
 
-    ::Gitlab::GitAccess.can_push_to_branch?(current_user, project, ref)
+    ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref)
   end
 
   def tree_breadcrumbs(tree, max_links = 2)
diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb
index de5322e990a..eeafefc25af 100644
--- a/app/services/files/create_service.rb
+++ b/app/services/files/create_service.rb
@@ -3,7 +3,7 @@ require_relative "base_service"
 module Files
   class CreateService < BaseService
     def execute
-      allowed = Gitlab::GitAccess.can_push_to_branch?(current_user, project, ref)
+      allowed = Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref)
 
       unless allowed
         return error("You are not allowed to create file in this branch")
diff --git a/app/services/files/delete_service.rb b/app/services/files/delete_service.rb
index 8e73c2e2727..1497a0f883b 100644
--- a/app/services/files/delete_service.rb
+++ b/app/services/files/delete_service.rb
@@ -3,7 +3,7 @@ require_relative "base_service"
 module Files
   class DeleteService < BaseService
     def execute
-      allowed = ::Gitlab::GitAccess.can_push_to_branch?(current_user, project, ref)
+      allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref)
 
       unless allowed
         return error("You are not allowed to push into this branch")
diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb
index 328cf3a4b06..0724d3ae634 100644
--- a/app/services/files/update_service.rb
+++ b/app/services/files/update_service.rb
@@ -3,7 +3,7 @@ require_relative "base_service"
 module Files
   class UpdateService < BaseService
     def execute
-      allowed = ::Gitlab::GitAccess.can_push_to_branch?(current_user, project, ref)
+      allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref)
 
       unless allowed
         return error("You are not allowed to push into this branch")
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
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 666398eedd4..39be9d64644 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -1,25 +1,26 @@
 require 'spec_helper'
 
 describe Gitlab::GitAccess do
-  let(:access) { Gitlab::GitAccess.new }
+  let(:access) { Gitlab::GitAccess.new(actor, project) }
   let(:project) { create(:project) }
   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(Gitlab::GitAccess.can_push_to_branch?(user, project, "random_branch")).to be_truthy
+        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(Gitlab::GitAccess.can_push_to_branch?(user, project, "random_branch")).to be_truthy
+        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(Gitlab::GitAccess.can_push_to_branch?(user, project, "random_branch")).to be_falsey
+        expect(access.can_push_to_branch?("random_branch")).to be_falsey
       end
     end
 
@@ -30,17 +31,17 @@ describe Gitlab::GitAccess do
       
       it "returns true if user is a master" do
         project.team << [user, :master]
-        expect(Gitlab::GitAccess.can_push_to_branch?(user, project, @branch.name)).to be_truthy
+        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(Gitlab::GitAccess.can_push_to_branch?(user, project, @branch.name)).to be_falsey
+        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(Gitlab::GitAccess.can_push_to_branch?(user, project, @branch.name)).to be_falsey
+        expect(access.can_push_to_branch?(@branch.name)).to be_falsey
       end
     end
 
@@ -51,17 +52,17 @@ describe Gitlab::GitAccess do
       
       it "returns true if user is a master" do
         project.team << [user, :master]
-        expect(Gitlab::GitAccess.can_push_to_branch?(user, project, @branch.name)).to be_truthy
+        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(Gitlab::GitAccess.can_push_to_branch?(user, project, @branch.name)).to be_truthy
+        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(Gitlab::GitAccess.can_push_to_branch?(user, project, @branch.name)).to be_falsey
+        expect(access.can_push_to_branch?(@branch.name)).to be_falsey
       end
     end
 
@@ -72,7 +73,7 @@ describe Gitlab::GitAccess do
       before { project.team << [user, :master] }
 
       context 'pull code' do
-        subject { access.download_access_check(user, project) }
+        subject { access.download_access_check }
 
         it { expect(subject.allowed?).to be_truthy }
       end
@@ -82,7 +83,7 @@ describe Gitlab::GitAccess do
       before { project.team << [user, :guest] }
 
       context 'pull code' do
-        subject { access.download_access_check(user, project) }
+        subject { access.download_access_check }
 
         it { expect(subject.allowed?).to be_falsey }
       end
@@ -95,7 +96,7 @@ describe Gitlab::GitAccess do
       end
 
       context 'pull code' do
-        subject { access.download_access_check(user, project) }
+        subject { access.download_access_check }
 
         it { expect(subject.allowed?).to be_falsey }
       end
@@ -103,7 +104,7 @@ describe Gitlab::GitAccess do
 
     describe 'without acccess to project' do
       context 'pull code' do
-        subject { access.download_access_check(user, project) }
+        subject { access.download_access_check }
 
         it { expect(subject.allowed?).to be_falsey }
       end
@@ -111,17 +112,18 @@ describe Gitlab::GitAccess do
 
     describe 'deploy key permissions' do
       let(:key) { create(:deploy_key) }
+      let(:actor) { key }
 
       context 'pull code' do
         context 'allowed' do
           before { key.projects << project }
-          subject { access.download_access_check(key, project) }
+          subject { access.download_access_check }
 
           it { expect(subject.allowed?).to be_truthy }
         end
 
         context 'denied' do
-          subject { access.download_access_check(key, project) }
+          subject { access.download_access_check }
 
           it { expect(subject.allowed?).to be_falsey }
         end
@@ -205,7 +207,7 @@ describe Gitlab::GitAccess do
 
         permissions_matrix[role].each do |action, allowed|
           context action do
-            subject { access.push_access_check(user, project, changes[action]) }
+            subject { access.push_access_check(changes[action]) }
 
             it { expect(subject.allowed?).to allowed ? be_truthy : be_falsey }
           end
@@ -221,7 +223,7 @@ describe Gitlab::GitAccess do
 
           updated_permissions_matrix[role].each do |action, allowed|
             context action do
-              subject { access.push_access_check(user, project, changes[action]) }
+              subject { access.push_access_check(changes[action]) }
 
               it { expect(subject.allowed?).to allowed ? be_truthy : be_falsey }
             end
diff --git a/spec/lib/gitlab/git_access_wiki_spec.rb b/spec/lib/gitlab/git_access_wiki_spec.rb
index c31c6764091..4cb91094cb3 100644
--- a/spec/lib/gitlab/git_access_wiki_spec.rb
+++ b/spec/lib/gitlab/git_access_wiki_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe Gitlab::GitAccessWiki do
-  let(:access) { Gitlab::GitAccessWiki.new }
+  let(:access) { Gitlab::GitAccessWiki.new(user, project) }
   let(:project) { create(:project) }
   let(:user) { create(:user) }
 
@@ -11,7 +11,7 @@ describe Gitlab::GitAccessWiki do
       project.team << [user, :developer]
     end
 
-    subject { access.push_access_check(user, project, changes) }
+    subject { access.push_access_check(changes) }
 
     it { expect(subject.allowed?).to be_truthy }
   end