Better message for failed pushes because of git hooks

Conflicts:
	lib/gitlab/git_access.rb
	spec/lib/gitlab/git_access_spec.rb

5 files changed, 53 insertions, 25 deletions

diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index ebf2296097d..1648834f034 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -43,7 +43,7 @@ module API
 
         return false unless actor
 
-        access.allowed?(
+        access.check(
           actor,
           params[:action],
           project,
diff --git a/lib/gitlab/backend/grack_auth.rb b/lib/gitlab/backend/grack_auth.rb
index df1461a45c9..762639414e0 100644
--- a/lib/gitlab/backend/grack_auth.rb
+++ b/lib/gitlab/backend/grack_auth.rb
@@ -80,7 +80,7 @@ module Grack
       case git_cmd
       when *Gitlab::GitAccess::DOWNLOAD_COMMANDS
         if user
-          Gitlab::GitAccess.new.download_allowed?(user, project)
+          Gitlab::GitAccess.new.download_access_check(user, project).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 129881060d5..3452240dad8 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -5,61 +5,60 @@ module Gitlab
 
     attr_reader :params, :project, :git_cmd, :user
 
-    def allowed?(actor, cmd, project, changes = nil)
+    def check(actor, cmd, project, changes = nil)
       case cmd
       when *DOWNLOAD_COMMANDS
         if actor.is_a? User
-          download_allowed?(actor, project)
+          download_access_check(actor, project)
         elsif actor.is_a? DeployKey
           actor.projects.include?(project)
         elsif actor.is_a? Key
-          download_allowed?(actor.user, project)
+          download_access_check(actor.user, project)
         else
           raise 'Wrong actor'
         end
       when *PUSH_COMMANDS
         if actor.is_a? User
-          push_allowed?(actor, project, changes)
+          push_access_check(actor, project, changes)
         elsif actor.is_a? DeployKey
-          # Deploy key not allowed to push
-          return false
+          return build_status_object(false, "Deploy key not allowed to push")
         elsif actor.is_a? Key
-          push_allowed?(actor.user, project, changes)
+          push_access_check(actor.user, project, changes)
         else
           raise 'Wrong actor'
         end
       else
-        false
+        return build_status_object(false, "Wrong command")
       end
     end
 
-    def download_allowed?(user, project)
-      if user && user_allowed?(user)
-        user.can?(:download_code, project)
+    def download_access_check(user, project)
+      if user && user_allowed?(user) && user.can?(:download_code, project)
+        build_status_object(true)
       else
-        false
+        build_status_object(false, "You don't have access")
       end
     end
 
-    def push_allowed?(user, project, changes)
-      return false unless user && user_allowed?(user)
-      return true if changes.blank?
+    def push_access_check(user, project, changes)
+      return build_status_object(false, "You don't have access") unless user && user_allowed?(user)
+      return build_status_object(true) if changes.blank?
 
       changes = changes.lines if changes.kind_of?(String)
 
       # Iterate over all changes to find if user allowed all of them to be applied
       changes.each do |change|
-        unless change_allowed?(user, project, change)
+        status = change_access_check(user, project, change)
+        unless status.allowed?
           # If user does not have access to make at least one change - cancel all push
-          return false
+          return status
         end
       end
 
-      # If user has access to make all changes
-      true
+      return build_status_object(true)
     end
 
-    def change_allowed?(user, project, change)
+    def change_access_check(user, project, change)
       oldrev, newrev, ref = change.split(' ')
 
       action = if project.protected_branch?(branch_name(ref))
@@ -79,7 +78,11 @@ module Gitlab
                  :push_code
                end
 
-      user.can?(action, project)
+      if user.can?(action, project)
+        build_status_object(true)
+      else
+        build_status_object(false, "You don't have permission")
+      end
     end
 
     def forced_push?(project, oldrev, newrev)
@@ -116,5 +119,11 @@ module Gitlab
         nil
       end
     end
+
+    protected
+
+    def build_status_object(status, message = '')
+      GitAccessStatus.new(status, message)
+    end
   end
 end
diff --git a/lib/gitlab/git_access_status.rb b/lib/gitlab/git_access_status.rb
new file mode 100644
index 00000000000..3d451ecebee
--- /dev/null
+++ b/lib/gitlab/git_access_status.rb
@@ -0,0 +1,15 @@
+module Gitlab
+  class GitAccessStatus
+    attr_accessor :status, :message
+    alias_method :allowed?, :status
+
+    def initialize(status, message = '')
+      @status = status
+      @message = message
+    end
+
+    def to_json
+      {status: @status, message: @message}.to_json
+    end
+  end
+end
\ No newline at end of file
diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb
index 9f0eb3be20f..f7d1428deb2 100644
--- a/lib/gitlab/git_access_wiki.rb
+++ b/lib/gitlab/git_access_wiki.rb
@@ -1,7 +1,11 @@
 module Gitlab
   class GitAccessWiki < GitAccess
-    def change_allowed?(user, project, change)
-      user.can?(:write_wiki, project)
+    def change_allowed_check(user, project, change)
+      if user.can?(:write_wiki, project)
+        build_status_object(true)
+      else
+        build_status_object(false, "You don't have access")
+      end
     end
   end
 end