Clarify error messages

And refactor to self-document a little better.

3 files changed, 31 insertions, 13 deletions

diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index f43359d7dbd..176b0991971 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -14,8 +14,8 @@ module Gitlab
       project_not_found: 'The project you were looking for could not be found.',
       account_blocked: 'Your account has been blocked.',
       command_not_allowed: "The command you're trying to execute is not allowed.",
-      upload_pack_disabled_in_config: 'The command "git-upload-pack" is not allowed.',
-      receive_pack_disabled_in_config: 'The command "git-receive-pack" is not allowed.'
+      upload_pack_disabled_over_http: 'Pulling over HTTP is not allowed.',
+      receive_pack_disabled_over_http: 'Pushing over HTTP is not allowed.'
     }.freeze
 
     DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive }.freeze
@@ -94,12 +94,22 @@ module Gitlab
     end
 
     def check_command_disabled!(cmd)
-      if http?
-        if upload_pack?(cmd) && !Gitlab.config.gitlab_shell.upload_pack
-          raise UnauthorizedError, ERROR_MESSAGES[:upload_pack_disabled_in_config]
-        elsif receive_pack?(cmd) && !Gitlab.config.gitlab_shell.receive_pack
-          raise UnauthorizedError, ERROR_MESSAGES[:receive_pack_disabled_in_config]
-        end
+      if upload_pack?(cmd)
+        check_upload_pack_disabled!
+      elsif receive_pack?(cmd)
+        check_receive_pack_disabled!
+      end
+    end
+
+    def check_upload_pack_disabled!
+      if http? && upload_pack_disabled_over_http?
+        raise UnauthorizedError, ERROR_MESSAGES[:upload_pack_disabled_over_http]
+      end
+    end
+
+    def check_receive_pack_disabled!
+      if http? && receive_pack_disabled_over_http?
+        raise UnauthorizedError, ERROR_MESSAGES[:receive_pack_disabled_over_http]
       end
     end
 
@@ -215,6 +225,14 @@ module Gitlab
       command == 'git-receive-pack'
     end
 
+    def upload_pack_disabled_over_http?
+      !Gitlab.config.gitlab_shell.upload_pack
+    end
+
+    def receive_pack_disabled_over_http?
+      !Gitlab.config.gitlab_shell.receive_pack
+    end
+
     protected
 
     def user
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 10a7222c2b6..36d1d777583 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -170,7 +170,7 @@ describe Gitlab::GitAccess, lib: true do
         end
 
         context 'when calling git-upload-pack' do
-          it { expect { pull_access_check }.to raise_unauthorized('The command "git-upload-pack" is not allowed.') }
+          it { expect { pull_access_check }.to raise_unauthorized('Pulling over HTTP is not allowed.') }
         end
 
         context 'when calling git-receive-pack' do
@@ -184,7 +184,7 @@ describe Gitlab::GitAccess, lib: true do
         end
 
         context 'when calling git-receive-pack' do
-          it { expect { push_access_check }.to raise_unauthorized('The command "git-receive-pack" is not allowed.') }
+          it { expect { push_access_check }.to raise_unauthorized('Pushing over HTTP is not allowed.') }
         end
 
         context 'when calling git-upload-pack' do
diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index ab7c56fcdf0..f018b48ceb2 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -254,7 +254,7 @@ describe 'Git HTTP requests', lib: true do
               it 'rejects pushes with 403 Forbidden' do
                 upload(path, env) do |response|
                   expect(response).to have_http_status(:forbidden)
-                  expect(response.body).to eq(git_access_error(:receive_pack_disabled_in_config))
+                  expect(response.body).to eq(git_access_error(:receive_pack_disabled_over_http))
                 end
               end
             end
@@ -265,7 +265,7 @@ describe 'Git HTTP requests', lib: true do
 
                 download(path, env) do |response|
                   expect(response).to have_http_status(:forbidden)
-                  expect(response.body).to eq(git_access_error(:upload_pack_disabled_in_config))
+                  expect(response.body).to eq(git_access_error(:upload_pack_disabled_over_http))
                 end
               end
             end
@@ -541,7 +541,7 @@ describe 'Git HTTP requests', lib: true do
 
               context 'when the repo does not exist' do
                 let(:project) { create(:empty_project) }
-                
+
                 it 'rejects pulls with 403 Forbidden' do
                   clone_get path, env