Use consistent names and move checks to the method,

and move those checks to be private. Feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7383#note_20285012

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7383#note_20285279

3 files changed, 47 insertions, 43 deletions

diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 545506f3dfd..f0b241fb5e6 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -29,16 +29,16 @@ module Gitlab
 
     def check(cmd, changes)
       check_protocol!
-      check_active_user! unless deploy_key?
+      check_active_user!
       check_project_accessibility!
       check_command_existence!(cmd)
       check_repository_existence!
 
       case cmd
       when *DOWNLOAD_COMMANDS
-        download_access_check unless deploy_key?
+        check_download_access!
       when *PUSH_COMMANDS
-        push_access_check(changes)
+        check_push_access!(changes)
       end
 
       build_status_object(true)
@@ -46,30 +46,6 @@ module Gitlab
       build_status_object(false, ex.message)
     end
 
-    def download_access_check
-      passed = user_can_download_code? ||
-               build_can_download_code? ||
-               guest_can_download_code?
-
-      unless passed
-        raise UnauthorizedError, ERROR_MESSAGES[:download]
-      end
-    end
-
-    def push_access_check(changes)
-      if deploy_key
-        deploy_key_push_access_check
-      elsif user
-        user_push_access_check
-      else
-        raise UnauthorizedError, ERROR_MESSAGES[:upload]
-      end
-
-      return if changes.blank? # Allow access.
-
-      check_change_access!(changes)
-    end
-
     def guest_can_download_code?
       Guest.can?(:download_code, project)
     end
@@ -82,18 +58,6 @@ module Gitlab
       authentication_abilities.include?(:build_download_code) && user_access.can_do_action?(:build_download_code)
     end
 
-    def user_push_access_check
-      unless authentication_abilities.include?(:push_code)
-        raise UnauthorizedError, ERROR_MESSAGES[:upload]
-      end
-    end
-
-    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?
       Gitlab::ProtocolAccess.allowed?(protocol)
     end
@@ -107,6 +71,8 @@ module Gitlab
     end
 
     def check_active_user!
+      return if deploy_key?
+
       if user && !user_access.allowed?
         raise UnauthorizedError, "Your account has been blocked."
       end
@@ -130,6 +96,44 @@ module Gitlab
       end
     end
 
+    def check_download_access!
+      return if deploy_key?
+
+      passed = user_can_download_code? ||
+               build_can_download_code? ||
+               guest_can_download_code?
+
+      unless passed
+        raise UnauthorizedError, ERROR_MESSAGES[:download]
+      end
+    end
+
+    def check_push_access!(changes)
+      if deploy_key
+        check_deploy_key_push_access!
+      elsif user
+        check_user_push_access!
+      else
+        raise UnauthorizedError, ERROR_MESSAGES[:upload]
+      end
+
+      return if changes.blank? # Allow access.
+
+      check_change_access!(changes)
+    end
+
+    def check_user_push_access!
+      unless authentication_abilities.include?(:push_code)
+        raise UnauthorizedError, ERROR_MESSAGES[:upload]
+      end
+    end
+
+    def check_deploy_key_push_access!
+      unless deploy_key.can_push_to?(project)
+        raise UnauthorizedError, ERROR_MESSAGES[:deploy_key_upload]
+      end
+    end
+
     def check_change_access!(changes)
       changes_list = Gitlab::ChangesList.new(changes)
 
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 2c90397cc78..44b84afde47 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -50,7 +50,7 @@ describe Gitlab::GitAccess, lib: true do
     end
   end
 
-  describe 'download_access_check' do
+  describe '#check_download_access!' do
     subject { access.check('git-upload-pack', '_any') }
 
     describe 'master permissions' do
@@ -183,7 +183,7 @@ describe Gitlab::GitAccess, lib: true do
     end
   end
 
-  describe 'push_access_check' do
+  describe '#check_push_access!' do
     before { merge_into_protected_branch }
     let(:unprotected_branch) { FFaker::Internet.user_name }
 
@@ -231,7 +231,7 @@ describe Gitlab::GitAccess, lib: true do
 
             permissions_matrix[role].each do |action, allowed|
               context action do
-                subject { access.push_access_check(changes[action]) }
+                subject { access.send(:check_push_access!, changes[action]) }
                 it { expect(subject.allowed?).to allowed ? be_truthy : be_falsey }
               end
             end
diff --git a/spec/lib/gitlab/git_access_wiki_spec.rb b/spec/lib/gitlab/git_access_wiki_spec.rb
index 578db51631e..a5d172233cc 100644
--- a/spec/lib/gitlab/git_access_wiki_spec.rb
+++ b/spec/lib/gitlab/git_access_wiki_spec.rb
@@ -27,7 +27,7 @@ describe Gitlab::GitAccessWiki, lib: true do
     ['6f6d7e7ed 570e7b2ab refs/heads/master']
   end
 
-  describe '#download_access_check' do
+  describe '#access_check_download!' do
     subject { access.check('git-upload-pack', '_any') }
 
     before do