Better message for failed pushes because of git hooks

GITLAB_SHELL_VERSION

-2.2.0
+2.3.0

lib/api/internal.rb

@@ -43,7 +43,7 @@ module API
 
         return false unless actor
 
-        access.allowed?(
+        access.check(
           actor,
           params[:action],
           project,

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

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,9 @@ module Gitlab
                  :push_code
                end
 
-      user.can?(action, project) &&
+      unless user.can?(action, project)
+        return build_status_object(false, "You don't have permission")
+      end
       pass_git_hooks?(user, project, ref, oldrev, newrev)
     end
 
@@ -95,16 +96,16 @@ module Gitlab
     end
 
     def pass_git_hooks?(user, project, ref, oldrev, newrev)
-      return true unless project.git_hook
+      return build_status_object(true) unless project.git_hook
 
-      return true unless newrev && oldrev
+      return build_status_object(true) unless newrev && oldrev
 
       git_hook = project.git_hook
 
       # Prevent tag removal
       if git_hook.deny_delete_tag
         if project.repository.tag_names.include?(ref) && newrev =~ /0000000/
-          return false
+          return build_status_object(false, "You can not delete tag")
         end
       end
 
@@ -113,33 +114,43 @@ module Gitlab
         commits = project.repository.commits_between(oldrev, newrev)
         commits.each do |commit|
           if git_hook.commit_message_regex.present?
-            return false unless commit.safe_message =~ Regexp.new(git_hook.commit_message_regex)
+            unless commit.safe_message =~ Regexp.new(git_hook.commit_message_regex)
+              return build_status_object(false, "Commit message does not follow the pattern")
+            end
           end
 
           if git_hook.author_email_regex.present?
-            return false unless commit.committer_email =~ Regexp.new(git_hook.author_email_regex)
-            return false unless commit.author_email =~ Regexp.new(git_hook.author_email_regex)
+            unless commit.committer_email =~ Regexp.new(git_hook.author_email_regex)
+              return build_status_object(false, "Commiter's email does not follow the pattern")
+            end
+            unless commit.author_email =~ Regexp.new(git_hook.author_email_regex)
+              return build_status_object(false, "Author's email does not follow the pattern")
+            end
           end
 
           # Check whether author is a GitLab member
           if git_hook.member_check
-            return false unless User.existing_member?(commit.author_email)
+            unless User.existing_member?(commit.author_email)
+              return build_status_object(false, "Author is not a member of team")
+            end
             if commit.author_email != commit.committer_email
-              return false unless User.existing_member?(commit.committer_email)
+              unless User.existing_member?(commit.committer_email)
+                return build_status_object(false, "Commiter is not a member of team")
+              end
             end
           end
 
           if git_hook.file_name_regex.present?
             commit.diffs.each do |diff|
-              if diff.renamed_file || diff.new_file
-                return false if diff.new_path =~ Regexp.new(git_hook.file_name_regex)
+              if (diff.renamed_file || diff.new_file) && diff.new_path =~ Regexp.new(git_hook.file_name_regex)
+                return build_status_object(false, "File name #{diff.new_path} does not follow the pattern")
               end
             end
           end
         end
       end
 
-      true
+      build_status_object(true)
     end
 
     private
@@ -166,5 +177,11 @@ module Gitlab
       end
     end
 
+    protected
+
+    def build_status_object(status, message = '')
+      GitAccessStatus.new(status, message)
+    end
+    
   end
 end

lib/gitlab/git_access_status.rb

+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

lib/gitlab/git_access_wiki.rb

 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

spec/lib/gitlab/git_access_spec.rb

@@ -5,14 +5,14 @@ describe Gitlab::GitAccess do
   let(:project) { create(:project) }
   let(:user) { create(:user) }
 
-  describe 'download_allowed?' do
+  describe 'download_access_check' do
     describe 'master permissions' do
       before { project.team << [user, :master] }
 
       context 'pull code' do
-        subject { access.download_allowed?(user, project) }
+        subject { access.download_access_check(user, project) }
 
-        it { should be_true }
+        it { subject.allowed?.should be_true }
       end
     end
 
@@ -20,9 +20,9 @@ describe Gitlab::GitAccess do
       before { project.team << [user, :guest] }
 
       context 'pull code' do
-        subject { access.download_allowed?(user, project) }
+        subject { access.download_access_check(user, project) }
 
-        it { should be_false }
+        it { subject.allowed?.should be_false }
       end
     end
 
@@ -33,22 +33,22 @@ describe Gitlab::GitAccess do
       end
 
       context 'pull code' do
-        subject { access.download_allowed?(user, project) }
+        subject { access.download_access_check(user, project) }
 
-        it { should be_false }
+        it { subject.allowed?.should be_false }
       end
     end
 
     describe 'without acccess to project' do
       context 'pull code' do
-        subject { access.download_allowed?(user, project) }
+        subject { access.download_access_check(user, project) }
 
-        it { should be_false }
+        it { subject.allowed?.should be_false }
       end
     end
   end
 
-  describe 'push_allowed?' do
+  describe 'push_access_check' do
     def protect_feature_branch
       create(:protected_branch, name: 'feature', project: project)
     end
@@ -117,9 +117,9 @@ describe Gitlab::GitAccess do
 
         permissions_matrix[role].each do |action, allowed|
           context action do
-            subject { access.push_allowed?(user, project, changes[action]) }
+            subject { access.push_access_check(user, project, changes[action]) }
 
-            it { should allowed ? be_true : be_false }
+            it { subject.allowed?.should allowed ? be_true : be_false }
           end
         end
       end
@@ -135,7 +135,7 @@ describe Gitlab::GitAccess do
       it 'returns false' do
         project.create_git_hook
         project.git_hook.update(commit_message_regex: "@only.com")
-        access.pass_git_hooks?(user, project, 'refs/heads/master', '6f6d7e7ed', '570e7b2ab').should be_false
+        access.pass_git_hooks?(user, project, 'refs/heads/master', '6f6d7e7ed', '570e7b2ab').allowed?.should be_false
       end
     end
 
@@ -146,12 +146,12 @@ describe Gitlab::GitAccess do
       end
 
       it 'returns false for non-member user' do
-        access.pass_git_hooks?(user, project, 'refs/heads/master', '6f6d7e7ed', '570e7b2ab').should be_false
+        access.pass_git_hooks?(user, project, 'refs/heads/master', '6f6d7e7ed', '570e7b2ab').allowed?.should be_false
       end
 
       it 'returns true if committer is a gitlab member' do
         create(:user, email: 'dmitriy.zaporozhets@gmail.com')
-        access.pass_git_hooks?(user, project, 'refs/heads/master', '6f6d7e7ed', '570e7b2ab').should be_true
+        access.pass_git_hooks?(user, project, 'refs/heads/master', '6f6d7e7ed', '570e7b2ab').allowed?.should be_true
       end
     end
 
@@ -159,13 +159,13 @@ describe Gitlab::GitAccess do
       it 'returns false when filename is prohibited' do
         project.create_git_hook
         project.git_hook.update(file_name_regex: "jpg$")
-        access.pass_git_hooks?(user, project, 'refs/heads/master', '913c66a37', '33f3729a4').should be_false
+        access.pass_git_hooks?(user, project, 'refs/heads/master', '913c66a37', '33f3729a4').allowed?.should be_false
       end
 
       it 'returns true if file name is allowed' do
         project.create_git_hook
         project.git_hook.update(file_name_regex: "exe$")
-        access.pass_git_hooks?(user, project, 'refs/heads/master', '913c66a37', '33f3729a4').should be_true
+        access.pass_git_hooks?(user, project, 'refs/heads/master', '913c66a37', '33f3729a4').allowed?.should be_true
       end
     end
   end

spec/lib/gitlab/git_access_wiki_spec.rb

@@ -11,9 +11,9 @@ describe Gitlab::GitAccessWiki do
       project.team << [user, :developer]
     end
 
-    subject { access.push_allowed?(user, project, changes) }
+    subject { access.push_access_check(user, project, changes) }
 
-    it { should be_true }
+    it { subject.should be_true }
   end
 
   def changes

spec/requests/api/internal_spec.rb

@@ -37,7 +37,7 @@ describe API::API, api: true  do
           pull(key, project)
 
           response.status.should == 200
-          response.body.should == 'true'
+          JSON.parse(response.body)["status"].should be_true
         end
       end
 
@@ -46,7 +46,7 @@ describe API::API, api: true  do
           push(key, project)
 
           response.status.should == 200
-          response.body.should == 'true'
+          JSON.parse(response.body)["status"].should be_true
         end
       end
     end
@@ -61,7 +61,7 @@ describe API::API, api: true  do
           pull(key, project)
 
           response.status.should == 200
-          response.body.should == 'false'
+          JSON.parse(response.body)["status"].should be_false
         end
       end
 
@@ -70,7 +70,7 @@ describe API::API, api: true  do
           push(key, project)
 
           response.status.should == 200
-          response.body.should == 'false'
+          JSON.parse(response.body)["status"].should be_false
         end
       end
     end
@@ -87,7 +87,7 @@ describe API::API, api: true  do
           pull(key, personal_project)
 
           response.status.should == 200
-          response.body.should == 'false'
+          JSON.parse(response.body)["status"].should be_false
         end
       end
 
@@ -96,7 +96,7 @@ describe API::API, api: true  do
           push(key, personal_project)
 
           response.status.should == 200
-          response.body.should == 'false'
+          JSON.parse(response.body)["status"].should be_false
         end
       end
     end
@@ -114,7 +114,7 @@ describe API::API, api: true  do
           pull(key, project)
 
           response.status.should == 200
-          response.body.should == 'true'
+          JSON.parse(response.body)["status"].should be_true
         end
       end
 
@@ -123,7 +123,7 @@ describe API::API, api: true  do
           push(key, project)
 
           response.status.should == 200
-          response.body.should == 'false'
+          JSON.parse(response.body)["status"].should be_false
         end
       end
     end